Re: [virtio-comment] [PATCH v5] virtio-i2c: add the device specification

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Dec 22, 2020 at 12:29:09PM +0100, Cornelia Huck wrote:
> On Tue, 22 Dec 2020 14:11:24 +0800
> Jie Deng <jie.deng@intel.com> wrote:
> 
> > On 2020/12/20 3:05, Michael S. Tsirkin wrote:
> > > On Fri, Dec 18, 2020 at 10:06:45AM +0800, Jie Deng wrote:  
> > >> On 2020/12/17 18:26, Stefan Hajnoczi wrote:  
> > >>> On Thu, Dec 17, 2020 at 03:00:55AM -0500, Michael S. Tsirkin wrote:  
> > >>>> On Thu, Dec 17, 2020 at 03:08:07PM +0800, Jie Deng wrote:  
> > >>>>>           +The \field{flags} of the request is currently reserved as zero for future
> > >>>>>           +feature extensibility.
> > >>>>>           +
> > >>>>>           +The \field{written} of the request is the number of data bytes in the \field{write_buf}
> > >>>>>           +being written to the I2C slave address.
> > >>>>>
> > >>>>>       This field seems redundant since the device can determine the size of
> > >>>>>       write_buf implicitly from the total out buffer size. virtio-blk takes
> > >>>>>       this approach.
> > >>>>>
> > >>>>> The read/write are the actual number of data bytes being read from or written
> > >>>>> to the device
> > >>>>> which is not determined by the device. So I don't think it is redundant.  
> > >>>> I am still not sure I understand the difference.
> > >>>> This point is unclear to multiple people.  
> > >>> I think I get it now. This is made clear by splitting the struct:
> > >>>
> > >>>     /* Driver->device fields */
> > >>>     struct virtio_i2c_out_hdr
> > >>>     {
> > >>>         le16 addr;
> > >>>         le16 padding;
> > >>>         le32 flags;
> > >>>     };
> > >>>
> > >>>     /* Device->driver fields */
> > >>>     struct virtio_i2c_in_hdr
> > >>>     {
> > >>>         le16 written;
> > >>>         le16 read;
> > >>>         u8 status;
> > >>>     };  
> > >> written/read are not device->driver fields. They are driver->device fields.
> > >> They are not determined by the device but the driver(user).
> > >>
> > >> However, Michael said that the two fields may duplicate buf size available
> > >> in the descriptor. He intended to remove them.
> > >>
> > >> "
> > >> I note that read and written actually duplicate buf size
> > >> available in the descriptor.
> > >> Given we no longer mirror i2c_msg 1:1 do we still want to do this?
> > >> It will be trivial for the host device to populate these fields
> > >> correctly for linux.
> > >> Duplication of information iten leads to errors ...
> > >> "
> > >>
> > >> But there is a corner case I'm not sure if you have noticed.
> > >>
> > >> read and written can be 0. I think we may not put a buf with size 0 into the
> > >> virtqueue.  
> > > You always have the header and the status, right?
> > > E.g. with the below, the total buffer size is virtio_i2c_out_hdr size +
> > > write size for writes and read size + virtio_i2c_in_hdr size for reads.
> > > Neither result is ever 0.  
> > 
> > Then how to distinguish the request type the buffer contains.
> 
> I have read through the thread and I remain confused.
> 
> > 
> > Each type will have both virtio_i2c_out_hdr and virtio_i2c_in_hdr.
> > the backend can know the type by checking the read/written.
> > 
> > If the read=0 and the written>0, the request is a write request
> > The buffer may contains 3 scatterlist:
> > 
> > virtio_i2c_out_hdr // scatterlist[0]
> 
> So, what does virtio_i2c_{out,in}_hdr contain here? If it is the one from
> above, ...
> 
> > 
> >  Â Â Â  buf[/* this is write data, since read = 0 */] // scatterlist[1]
> > 
> >  Â Â Â  virtio_i2c_in_hdr // scatterlist[2]
> 
> ...we do not know whether there's read data, write data, or what their
> length is, until we've actually consumed the whole buffer, and then we
> have to go backwards.
> 
> > 
> > If the read>0 and the written=0, the request is a read request.
> > The buffer may contains 3 scatterlist:
> > 
> > virtio_i2c_out_hdr // scatterlist[0]
> > 
> >  Â Â Â  buf[/* This is read data, since written = 0 */] // scatterlist[1]
> > 
> >  Â Â Â  virtio_i2c_in_hdr // scatterlist[2]
> > 
> > If the read>0 and the written>0, the request is a write-read request.
> > The buffer may contains 4 scatterlist:
> > 
> > virtio_i2c_out_hdr   // scatterlist[0]
> > 
> >  Â Â Â  buf[/*write data*/]Â  // scatterlist[1]
> > 
> >  Â Â Â  buf[/*read data*/] // scatterlist[2]
> > 
> >  Â Â Â  virtio_i2c_in_hdr // scatterlist[3]
> 
> Is there any reason why we need to infer the type of the request by
> checking some lengths? Can't we just specify explicit flags for read
> and write? What am I missing?

Point is descriptors already have flags for read/write.
If there is a read buffer and length > sizeof virtio_i2c_in_hdr then
we know it's a read request.
If there is a write buffer and length > sizeof virtio_i2c_out_hdr then
we know it's a write request.
If both then both.

All this is known before buffer itself is consumed, which is nice.

Putting this info in flags will duplicate info which is often
a source of errors.


> > 
> > >> @Stefan @Paolo
> > >>
> > >> So what's your opinion about these two fields ?
> > >>  
> > >>>     /*
> > >>>      * Virtqueue element layout looks like this:
> > >>>      *
> > >>>      * struct virtio_i2c_out_hdr out_hdr; /* OUT */
> > >>>      * u8 write_buf[]; /* OUT */
> > >>>      * u8 read_buf[]; /* IN */
> > >>>      * struct virtio_i2c_in_hdr in_hdr; /* IN */
> > >>>      */
> > >>>
> > >>> This makes sense to me: a bi-directional request has both write_buf[]
> > >>> and read_buf[] so the vring used.len field is not enough to report back
> > >>> how many bytes were written and read. The virtio_i2c_in_hdr fields are
> > >>> really needed.
> > >>>
> > >>> Please split the struct in the spec so it's clear how this works.