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

[Jie Deng <jie.deng@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4263 bytes --]


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.

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]

     buf[/* this is write data, since read = 0 */] // scatterlist[1]

     virtio_i2c_in_hdr // scatterlist[2]

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]

>> @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.

[-- Attachment #2: Type: text/html, Size: 7820 bytes --]