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

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

[Jie Deng]

virtio-i2c is a virtual I2C adapter device. It provides a way
to flexibly communicate with the host I2C slave devices from
the guest.

This patch adds the specification for this device.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/88
Signed-off-by: Jie Deng <jie.deng@intel.com>
---
 conformance.tex  |  28 ++++++++--
 content.tex      |   1 +
 introduction.tex |   3 ++
 virtio-i2c.tex   | 157 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 185 insertions(+), 4 deletions(-)
 create mode 100644 virtio-i2c.tex

diff --git a/conformance.tex b/conformance.tex
index eb33240..e78adfd 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -27,8 +27,10 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \ref{sec:Conformance / Driver Conformance / Socket Driver Conformance},
 \ref{sec:Conformance / Driver Conformance / RPMB Driver Conformance},
 \ref{sec:Conformance / Driver Conformance / IOMMU Driver Conformance},
-\ref{sec:Conformance / Driver Conformance / Sound Driver Conformance} or
-\ref{sec:Conformance / Driver Conformance / Memory Driver Conformance}.
+\ref{sec:Conformance / Driver Conformance / Sound Driver Conformance}
+\ref{sec:Conformance / Driver Conformance / Memory Driver Conformance} or
+\ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance}.
+
     \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
   \end{itemize}
 \item[Device] A device MUST conform to four conformance clauses:
@@ -47,8 +49,10 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \ref{sec:Conformance / Device Conformance / Socket Device Conformance}, 
 \ref{sec:Conformance / Device Conformance / RPMB Device Conformance},
 \ref{sec:Conformance / Device Conformance / IOMMU Device Conformance},
-\ref{sec:Conformance / Device Conformance / Sound Device Conformance} or
-\ref{sec:Conformance / Device Conformance / Memory Device Conformance}.
+\ref{sec:Conformance / Device Conformance / Sound Device Conformance}
+\ref{sec:Conformance / Device Conformance / Memory Device Conformance} or
+\ref{sec:Conformance / Device Conformance / I2C Adapter Device Conformance}.
+
     \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
   \end{itemize}
 \end{description}
@@ -264,6 +268,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Device Types / Memory Device / Device Operation / STATE request}
 \end{itemize}
 
+\conformance{\subsection}{I2C Adapter Driver Conformance}\label{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance}
+
+An I2C Adapter driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / I2C Adapter Device / Device Operation}
+\end{itemize}
+
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
 
 A device MUST conform to the following normative statements:
@@ -483,6 +495,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Device Types / Memory Device / Device Operation / STATE request}
 \end{itemize}
 
+\conformance{\subsection}{I2C Adapter Device Conformance}\label{sec:Conformance / Device Conformance / I2C Adapter Device Conformance}
+
+An I2C Adapter device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / I2C Adapter Device / Device Operation}
+\end{itemize}
+
 \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
 A conformant implementation MUST be either transitional or
 non-transitional, see \ref{intro:Legacy
diff --git a/content.tex b/content.tex
index 61eab41..48002a2 100644
--- a/content.tex
+++ b/content.tex
@@ -6484,6 +6484,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 \input{virtio-iommu.tex}
 \input{virtio-sound.tex}
 \input{virtio-mem.tex}
+\input{virtio-i2c.tex}
 
 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
diff --git a/introduction.tex b/introduction.tex
index cc38e29..bc0217f 100644
--- a/introduction.tex
+++ b/introduction.tex
@@ -73,6 +73,9 @@ \section{Normative References}\label{sec:Normative References}
 	\phantomsection\label{intro:HDA}\textbf{[HDA]} &
 	High Definition Audio Specification,
 	\newline\url{https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/high-definition-audio-specification.pdf}\\
+	\phantomsection\label{intro:I2C}\textbf{[I2C]} &
+	I2C-bus specification and user manual,
+	\newline\url{https://www.nxp.com/docs/en/user-guide/UM10204.pdf}\\
 
 \end{longtable}
 
diff --git a/virtio-i2c.tex b/virtio-i2c.tex
new file mode 100644
index 0000000..6ff00ee
--- /dev/null
+++ b/virtio-i2c.tex
@@ -0,0 +1,157 @@
+\section{I2C Adapter Device}\label{sec:Device Types / I2C Adapter Device}
+
+virtio-i2c is a virtual I2C adapter device. It provides a way to flexibly
+organize and use the host I2C controlled devices from the guest. By attaching
+the host ACPI I2C controlled nodes to the virtual I2C adapter device, the guest can
+communicate with them without changing or adding extra drivers for these
+controlled I2C devices.
+
+\subsection{Device ID}\label{sec:Device Types / I2C Adapter Device / Device ID}
+34
+
+\subsection{Virtqueues}\label{sec:Device Types / I2C Adapter Device / Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / I2C Adapter Device / Feature bits}
+
+None currently defined.
+
+\subsection{Device configuration layout}\label{sec:Device Types / I2C Adapter Device / Device configuration layout}
+
+None currently defined.
+
+\subsection{Device Initialization}\label{sec:Device Types / I2C Adapter Device / Device Initialization}
+
+\begin{enumerate}
+\item The virtqueue is initialized.
+\end{enumerate}
+
+\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}.
+
+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
+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.
+
+\drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
+
+A driver MUST set \field{addr} and \field{flags} before sending the request.
+
+A driver MUST set \field{flags} to be zero.
+
+The driver SHOULD never send a request with ``length of \field{write_buf}''=0 and
+``length of \field{read_buf}''=0 at the same time.
+
+A driver MUST NOT use \field{read_buf} if the final \field{status} returned
+from the device is VIRTIO_I2C_MSG_ERR.
+
+A driver MAY queue one request or multiple requests at a time.
+
+A driver MUST queue the requests in order if multiple requests are going to
+be sent at a time.
+
+If multiple requests are sent at a time and some of them returned from the
+device have the \field{status} being VIRTIO_I2C_MSG_ERR, a driver MUST treat
+the first failed request and the requests after the first failed one as
+VIRTIO_I2C_MSG_ERR.
+
+\devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
+
+A device SHOULD keep consistent behaviors with the hardware as described in
+\hyperref[intro:I2C]{I2C}.
+
+A device MUST NOT change the value of \field{addr}, \field{flags} and \field{write_buf}.
+
+A device MUST place one I2C segment of the corresponding length into \field{read_buf}
+according the driver's request.
+
+A device MUST guarantee the requests in the virtqueue being processed in order
+if multiple requests are received at a time.
+
+If multiple requests are received at a time and processing of some of the
+requests fails, a device MUST set the \field{status} of the first failed
+request to be VIRTIO_I2C_MSG_ERR and MAY set the \field{status} of the
+requests after the first failed one to be VIRTIO_I2C_MSG_ERR.
-- 
2.7.4


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Stefan Hajnoczi]

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

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.

> +
> +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/

> +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?

Devices may poll vrings instead of using available buffer notifications.
Polled devices cannot provide the grouping semantics described above.

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?

> +
> +\drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
> +
> +A driver MUST set \field{addr} and \field{flags} before sending the request.
> +
> +A driver MUST set \field{flags} to be zero.
> +
> +The driver SHOULD never send a request with ``length of \field{write_buf}''=0 and
> +``length of \field{read_buf}''=0 at the same time.

s/SHOULD never/MUST NOT/?

> +
> +A driver MUST NOT use \field{read_buf} if the final \field{status} returned
> +from the device is VIRTIO_I2C_MSG_ERR.
> +
> +A driver MAY queue one request or multiple requests at a time.

I think no other VIRTIO device only allows one request queued at a time.
Therefore it's probably not necessary to include this statement - all
devices allow one or more requests to be queued at a time.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jie Deng]

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

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 \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.
>
>> +
>> +\drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>> +
>> +A driver MUST set \field{addr} and \field{flags} before sending the request.
>> +
>> +A driver MUST set \field{flags} to be zero.
>> +
>> +The driver SHOULD never send a request with ``length of \field{write_buf}''=0 and
>> +``length of \field{read_buf}''=0 at the same time.
> s/SHOULD never/MUST NOT/?
Will fix it.
>
>> +
>> +A driver MUST NOT use \field{read_buf} if the final \field{status} returned
>> +from the device is VIRTIO_I2C_MSG_ERR.
>> +
>> +A driver MAY queue one request or multiple requests at a time.
> I think no other VIRTIO device only allows one request queued at a time.
> Therefore it's probably not necessary to include this statement - all
> devices allow one or more requests to be queued at a time.

Will remove it.


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

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

[Stefan Hajnoczi]

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jie Deng]

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


On 2020/12/30 18:20, Stefan Hajnoczi wrote:
> 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.
OK. Let's ignore the R/W bit.
>>>> +
>>>> +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.
Sure. Will do that.
> 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.
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.

But we already have the virtio_i2c_in_hdr::status for this purpose. When 
a request fails,
the status is then set by the device and the device fails all later 
requests currently in the
virtqueue.

We just need to make sure that once the driver adds some requests to the 
virtqueue,
it should complete them (either success or fail) before adding new 
requests. I think this
is a behavior of physical I2C adapter drivers and we can follow.


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

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

[Stefan Hajnoczi]

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

On Thu, Dec 31, 2020 at 10:50:13AM +0800, Jie Deng wrote:
> 
> On 2020/12/30 18:20, Stefan Hajnoczi wrote:
> > 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:
> > > > > +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.
> Sure. Will do that.
> > 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.
> 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
fails.

Thinking about it again, VIRTIO_I2C_FLAGS_FAIL_NEXT would be a clearer
name for this flag.

> We just need to make sure that once the driver adds some requests to the
> virtqueue,
> it should complete them (either success or fail) before adding new requests.
> I think this
> is a behavior of physical I2C adapter drivers and we can follow.

Existing VIRTIO devices do not have special empty queue semantics and I
don't think virtio-i2c should introduce a special case here. The
VIRTIO_I2C_FLAGS_FAIL_NEXT flag is simple to implement, allows drivers
more freedom to enqueue requests, and eliminates the race condition with
polled device implementations (they could complete failed request #1
before request #2 is enqueued and then request #2 would not be
automatically failed).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jie Deng]

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

Once a device fails to handle a request, it shall fail the existing 
remaining requests
since these requests needs to be handled in order.
> Thinking about it again, VIRTIO_I2C_FLAGS_FAIL_NEXT would be a clearer
> name for this flag.
>
>> We just need to make sure that once the driver adds some requests to the
>> virtqueue,
>> it should complete them (either success or fail) before adding new requests.
>> I think this
>> is a behavior of physical I2C adapter drivers and we can follow.
> Existing VIRTIO devices do not have special empty queue semantics and I
> don't think virtio-i2c should introduce a special case here. The
> VIRTIO_I2C_FLAGS_FAIL_NEXT flag is simple to implement, allows drivers
> more freedom to enqueue requests, and eliminates the race condition with
> polled device implementations (they could complete failed request #1
> before request #2 is enqueued and then request #2 would not be
> automatically failed).
The I2C core is using a synchronization semantics. It means once a I2C 
bus driver send
some messages, it should wait for the device to complete them (either 
success or fail)
before sending new messages. The virtio-i2c driver also have to follow 
this to register it into
I2C subsystem. So the freedom is already limited by the I2C core, and as 
a result, we don't have
such race conditions.

> Stefan

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Stefan Hajnoczi]

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

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

> 
> Once a device fails to handle a request, it shall fail the existing
> remaining requests
> since these requests needs to be handled in order.
> > Thinking about it again, VIRTIO_I2C_FLAGS_FAIL_NEXT would be a clearer
> > name for this flag.
> > 
> > > We just need to make sure that once the driver adds some requests to the
> > > virtqueue,
> > > it should complete them (either success or fail) before adding new requests.
> > > I think this
> > > is a behavior of physical I2C adapter drivers and we can follow.
> > Existing VIRTIO devices do not have special empty queue semantics and I
> > don't think virtio-i2c should introduce a special case here. The
> > VIRTIO_I2C_FLAGS_FAIL_NEXT flag is simple to implement, allows drivers
> > more freedom to enqueue requests, and eliminates the race condition with
> > polled device implementations (they could complete failed request #1
> > before request #2 is enqueued and then request #2 would not be
> > automatically failed).
> The I2C core is using a synchronization semantics. It means once a I2C bus
> driver send
> some messages, it should wait for the device to complete them (either
> success or fail)
> before sending new messages. The virtio-i2c driver also have to follow this
> to register it into
> I2C subsystem. So the freedom is already limited by the I2C core, and as a
> result, we don't have
> such race conditions.

VIRTIO devices are not specific to Linux so the design should avoid the
limitations of the current Linux driver behavior, if possible. It's easy
to include the VIRTIO_I2C_FLAGS_FAIL_NEXT bit that I described, both in
the spec and in the Linux driver, and it will allow flexibility in the
future.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jie Deng]

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
>>>>> 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.
>>>> 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
>>> fails.
>> 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 
requests.

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 
VIRTIO_I2C_FLAGS_FAIL_NEXT ?

>> Once a device fails to handle a request, it shall fail the existing
>> remaining requests
>> since these requests needs to be handled in order.
>>> Thinking about it again, VIRTIO_I2C_FLAGS_FAIL_NEXT would be a clearer
>>> name for this flag.
>>>
>>>> We just need to make sure that once the driver adds some requests to the
>>>> virtqueue,
>>>> it should complete them (either success or fail) before adding new requests.
>>>> I think this
>>>> is a behavior of physical I2C adapter drivers and we can follow.
>>> Existing VIRTIO devices do not have special empty queue semantics and I
>>> don't think virtio-i2c should introduce a special case here. The
>>> VIRTIO_I2C_FLAGS_FAIL_NEXT flag is simple to implement, allows drivers
>>> more freedom to enqueue requests, and eliminates the race condition with
>>> polled device implementations (they could complete failed request #1
>>> before request #2 is enqueued and then request #2 would not be
>>> automatically failed).
>> The I2C core is using a synchronization semantics. It means once a I2C bus
>> driver send
>> some messages, it should wait for the device to complete them (either
>> success or fail)
>> before sending new messages. The virtio-i2c driver also have to follow this
>> to register it into
>> I2C subsystem. So the freedom is already limited by the I2C core, and as a
>> result, we don't have
>> such race conditions.
> VIRTIO devices are not specific to Linux so the design should avoid the
> limitations of the current Linux driver behavior, if possible. It's easy
> to include the VIRTIO_I2C_FLAGS_FAIL_NEXT bit that I described, both in
> the spec and in the Linux driver, and it will allow flexibility in the
> future.
>
> Stefan

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Stefan Hajnoczi]

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

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
> > > > > > 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.
> > > > > 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
> > > > fails.
> > > 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
> requests.
> 
> 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
> VIRTIO_I2C_FLAGS_FAIL_NEXT ?

The driver clears the VIRTQ_DESC_F_NEXT flag on the final buffer in a
group:

  R11 VIRTIO_I2C_FLAGS_FAIL_NEXT=1
  R12 VIRTIO_I2C_FLAGS_FAIL_NEXT=0
  R21 VIRTIO_I2C_FLAGS_FAIL_NEXT=1
  R22 VIRTIO_I2C_FLAGS_FAIL_NEXT=1
  R23 VIRTIO_I2C_FLAGS_FAIL_NEXT=0

(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
memory.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Stefan Hajnoczi]

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

On Thu, Dec 24, 2020 at 04:10:43PM +0800, Jie Deng wrote:
> +\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}

I didn't notice that the written and read fields were removed. Does this
mean that struct virtio_i2c_req can either be read or write, but not
read+write?

This makes sense if virtqueue buffers can be grouped together so
bi-directional transfers can be described using multiple buffers. Just
wanted to check if I understand this correctly.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jie Deng]

On 2021/1/7 2:11, Stefan Hajnoczi wrote:
> On Thu, Dec 24, 2020 at 04:10:43PM +0800, Jie Deng wrote:
>> +\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}
> I didn't notice that the written and read fields were removed. Does this
> mean that struct virtio_i2c_req can either be read or write, but not
> read+write?

It can be read, write or read+write.

written/read are removed according to Michael's suggest. He preferred to 
directly use the read/write flags in descriptors.

Please refer to 
https://lists.oasis-open.org/archives/virtio-comment/202012/msg00045.html

> This makes sense if virtqueue buffers can be grouped together so
> bi-directional transfers can be described using multiple buffers. Just
> wanted to check if I understand this correctly.
>
> Thanks,
> Stefan

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Jie Deng]

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
>>>>>>> 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.
>>>>>> 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
>>>>> fails.
>>>> 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
>> requests.
>>
>> 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
>> VIRTIO_I2C_FLAGS_FAIL_NEXT ?
> The driver clears the VIRTQ_DESC_F_NEXT flag on the final buffer in a
> group:
>
>    R11 VIRTIO_I2C_FLAGS_FAIL_NEXT=1
>    R12 VIRTIO_I2C_FLAGS_FAIL_NEXT=0
>    R21 VIRTIO_I2C_FLAGS_FAIL_NEXT=1
>    R22 VIRTIO_I2C_FLAGS_FAIL_NEXT=1
>    R23 VIRTIO_I2C_FLAGS_FAIL_NEXT=0
>
> (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
> memory.
>
> Stefan
Thanks Stefan. I'd like to use the first method by adding a next flag
based on the original proposal.



This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/