OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

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

On 2021/1/7 2:04, Stefan Hajnoczi wrote:

On Tue, Jan 05, 2021 at 10:43:38AM +0800, Jie Deng wrote:
On 2021/1/5 2:02, Stefan Hajnoczi wrote:
On Mon, Jan 04, 2021 at 10:31:48AM +0800, Jie Deng wrote:
On 2020/12/31 20:42, Stefan Hajnoczi wrote:
This approach causes a bottleneck when there are multiple I2C devices.
It requires exclusive access to the virtqueue and virtual I2C bus,
slowing down applications that are accessing separate I2C devices.

This is inefficient, especially if the I2C devices are emulated or on
separate physical I2C busses where request concurrent processing is

This also creates starvation/denial-of-service problems in the driver
because an application can submit many requests and needs exclusive
access to the virtqueue until all requests have completed.

I suggest defining a VIRTIO_I2C_FLAGS_CONTINUATION bit in struct
virtio_i2c_out_hdr::flags. When a request fails and this flag is set
then the device fails all later requests until it reaches a request
where the flag is zero. This allows the driver to enqueue a sequence of
requests that fail as a group without waiting for an empty virtqueue.
The flags is designed to be a "driver->device" field for extensibility. It
is read-onlyï to the device.
So when a request fails, the device can't set this read-only field.
Sorry, "and this flag is set" means "and this flag was set by the
driver". My intention was that the driver marks a request so the device
knows that it needs to fail the next request too when this request
No. A device doesn't need to be told by a driver if it needs to fail the
next request.
Actually just the opposite, a driver may need to be notified. A driver can't
know which
one is the "next" because only a device know which request is the first
failed one.
I think we are miscommunicating. The driver is telling the device which
requests are grouped together - that is the purpose of the
VIRTIO_I2C_FLAGS_FAIL_NEXT flag. When a request fails the device is able
to fail the remaining requests in that group. Requests that are not part
of the group won't be failed.
So do you mean to mark the first one of each grouped requests ?

For example, if a driver kicks twice and the virtqueue has following

R11 R12 R21 R22 R23

(First kick: R11 R12, Second kick: R21 R22 R23)

Do you want the driver to set the flags of R11 and R21 to be
The driver clears the VIRTQ_DESC_F_NEXT flag on the final buffer in a


(This is the same approach as VIRTQ_DESC_F_NEXT in "2.7.6 Next Flag:
Descriptor Chaining".)

Doing it this way allows the device to identify the final buffer in the
group. This is helpful because the device is no longer dependent on
buffer available notifications marking the end of a group. So a poll
mode device could be implemented on systems that don't use interrupts.
Also, if a driver notifies the device before adding the last buffer into
the virtqueue for some reason, then the device will know that the group
isn't finished yet and can wait for the final buffer if it wants.

An alternative is treat the entire group as a single virtqueue buffer
instead of multiple buffers. The virtio-blk
VIRTIO_BLK_T_DISCARD/WRITE_ZEROES commands work like this. The driver
submits a single command for a group of discard/write_zeroes operations.

This would be a big change to your proposal and I'm not sure there is
much of an advantage to doing so, I'm just describing it here in case
you like it better. The request structs would look something like this:

   /* A single read, write, or read+write message */
   struct virtio_i2c_msg {
       le16 addr;      /* I2C device address */
       le16 read_len;  /* number of bytes to read from device */
       le16 write_len; /* number of bytes to write to device */
       le16 flags;     /* reserved */

   struct virtio_i2c_result {
       le16 nread;     /* number of bytes read from device */
       le16 nwritten;  /* number of bytes written to device */
       u8 status;      /* VIRTIO_I2C_MSG_OK or VIRTIO_I2C_MSG_ERR */
       u8 padding;

   struct virtio_i2c_req_out {
       le16 nmsg; /* number of struct virtio_i2c_msg */

The request layout is as follows:

   /* Driver->Device */
   struct virtio_i2c_req_out out;
   struct virtio_i2c_msg msgs[out.nmsg];
   u8 write_buf[];

   /* Device->Driver */
   u8 read_buf[];
   struct virtio_i2c_result results[out.nmsg];

Data in write_buf[] and read_buf[] is arranged in msgs[] order. A
request with 2 write messages of 8 bytes each would look like this:

   write_buf[] = {...8 bytes for msgs[0]...,
                  ...8 bytes for msgs[1]...};
   read_buf[] = { /* empty */ };

Since virtqueue buffers may consist of many vring descriptors the
write_buf[] and read_buf[] data does not need to be contiguous in

Thanks Stefan. I'd like to use the first method by adding a next flag
based on the original proposal.

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]