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

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev 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 Wed, Dec 30, 2020 at 10:17:23AM +0800, Jie Deng wrote:
> On 2020/12/29 19:09, Stefan Hajnoczi wrote:
> 
> > On Thu, Dec 24, 2020 at 04:10:43PM +0800, Jie Deng wrote:
> > > +\subsection{Device Operation}\label{sec:Device Types / I2C Adapter Device / Device Operation}
> > > +
> > > +\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Adapter Device / Device Operation: Request Queue}
> > > +
> > > +The driver queues requests to the virtqueue, and they are used by the
> > > +device. The request is the representation of segments of an I2C
> > > +transaction. Each request is of the form:
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_i2c_out_hdr {
> > > +        le16 addr;
> > > +        le16 padding;
> > > +        le32 flags;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_i2c_in_hdr {
> > > +        u8 status;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_i2c_req {
> > > +        struct virtio_i2c_out_hdr out_hdr;
> > > +        u8 write_buf[];
> > > +        u8 read_buf[];
> > > +        struct virtio_i2c_in_hdr in_hdr;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +The \field{addr} of the request is the address of the I2C controlled device.
> > > +The format of \field{addr} is defined in paragraph 3.1.10 and 3.1.11 of
> > > +\hyperref[intro:I2C]{I2C}.
> > It's still unclear how addresses are represented in le16 addr even after
> > reading the I2C paragraphs mentioned above.
> > 
> > Here is my guess:
> > 
> > le16 addr  15 14 13 12 11 10  9  8  7  6  5  4  3  2  1  0
> > 7-bit       0  0  0  0  0  0  0  0  A6 A5 A4 A3 A2 A1 A0 0
> > 10-bit     A7 A6 A5 A4 A3 A2 A1 A0   1  1  1  1  0 A9 A8 0
> > 
> > The R/W bit is ignored and always set to 0 since the I/O direction is
> > already implicit in the size of write_buf[] and read_buf[].
> > 
> > Is this correct? Please document the exact layout in the virtio spec
> > since the I2C spec does not describe an le16 representation.
> Correct. But, I prefer to keep the definition exactly the same with the I2C
> protocol.
> Not to ignore the R/W bit in addr.
> 
> le16 addr  15 14 13 12 11 10  9  8  7  6  5  4  3  2  1  0
> 7-bit       0  0  0  0  0  0  0  0  A6 A5 A4 A3 A2 A1 A0*R/W*
> 10-bit     A7 A6 A5 A4 A3 A2 A1 A0   1  1  1  1  0 A9 A8*R/W*
> 
> The backend may check the R/W bit in addr to make sure it is compatible
> with that in the descriptor. If not, then it is an error request.

The R/W bit is unable to describe bi-directional requests where
read_buf[] and write_buf[] are both non-zero.

The device also needs to check the sizeof(read_buf) == 0 and
sizeof(write_buf) == 0 case separately (it's not handled by the R/W
bit).

I don't think the R/W bit adds much, except maybe adding a sanity check
for read-only and write-only requests. Given that it adds complexity and
some device implementations may forget to validate it, I suggest
dropping it.

> > > +
> > > +The \field{padding} is used to pad to full dword.
> > > +
> > > +The \field{flags} of the request is currently reserved as zero for future
> > > +feature extensibility.
> > > +
> > > +The \field{write_buf} of the request contains one segment of an I2C transaction
> > > +being written to the device.
> > > +
> > > +The \field{read_buf} of the request contains one segment of an I2C transaction
> > > +being read from the device.
> > > +
> > > +The final \field{status} byte of the request is written by the device: either
> > > +VIRTIO_I2C_MSG_OK for success or VIRTIO_I2C_MSG_ERR for error.
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_I2C_MSG_OK     0
> > > +#define VIRTIO_I2C_MSG_ERR    1
> > > +\end{lstlisting}
> > > +
> > > +If ``length of \field{read_buf}''=0 and ``length of \field{write_buf}''>0,
> > > +the request is called write request.
> > > +
> > > +If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''=0,
> > > +the request is called read request.
> > > +
> > > +If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''>0,
> > > +the request is called write-read request. It means an I2C write segment followed
> > > +by a read segment. Usually, the write segment provides the number of an I2C
> > > +controlled device register to be read.
> > > +
> > > +The case when ``length of \field{write_buf}''=0, and at the same time,
> > > +``length of \field{read_buf}''=0 doesn't make any sense.
> > > +
> > > +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C Adapter Device / Device Operation: Operation Status}
> > > +
> > > +\field{addr}, ``length of \field{write_buf}'' and ``length of \field{read_buf}''
> > > +are determined by the driver, while \field{status} is determined by the processing
> > > +of the devcie. A driver puts the data written to the device into \field{write_buf}, while
> > s/devcie/device/
> Will fix this typo.
> > 
> > > +a device puts the data of the corresponding length into \field{read_buf} according the
> > > +request of the driver.
> > > +
> > > +A driver may send one request or multiple requests to the device at a time.
> > > +The requests in the virtqueue are both queued and processed in order.
> > > +
> > > +If a driver sends multiple requests at a time and a device fails to process
> > > +some of them, then the first failed request will have its \field{status}
> > > +being set to VIRTIO_I2C_MSG_ERR by the device and the requests after the first
> > > +failed one will also be treated as VIRTIO_I2C_MSG_ERR by the driver,
> > > +no matter what \field{status} of them. In this case, the number of successfully
> > > +sent requests this time is the number of the last request being successfully
> > > +processed.
> > If I understand correctly all requests submitted in a single available
> > buffer notification are grouped together. Once the first request fails
> > the driver must treat all remaining requests as failed?
> Right. That's how it works.
> > Devices may poll vrings instead of using available buffer notifications.
> > Polled devices cannot provide the grouping semantics described above.
> Poll is not recommended for this device. But if poll is used, the device
> must notify
> the driver once it encounters an error request.
> 
> So if poll is used and the virtqueue has following requests (May be
> submitted by many kick operation, no need to group)
> 
> Y Y Y Y N X X X X
> 
> (Y: OK,   N: error,   X: no need to care about, must be treated as error.)
> 
> The device must notify  the driver at the first "N" and driver must treat
> the N and "X X X X" as error.
> 
> > 
> > If you want to group requests then it is necessary to include a
> > per-request group ID or continuation/end-of-group field. virtio-net
> > VIRTIO_NET_F_MRG_RXBUF is an example of this.
> > 
> > Additionally, this paragraph does not specify whether the device
> > executes the remaining requests or not. This seems problematic to me
> > because the driver does not know which requests actually executed when
> > a request in a group of requests fails?
> The device must not execute the remaining requests.

Please add that to the spec so it's clear the device must fail remaining
requests insteads of attempting to execute them.

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

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.

Attachment: signature.asc
Description: PGP signature



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