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

[virtio-comment] [PATCH v5] 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   | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 167 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..9f016d5 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.cn/docs/en/user-guide/UM10204.pdf}\\
 
 \end{longtable}
 
diff --git a/virtio-i2c.tex b/virtio-i2c.tex
new file mode 100644
index 0000000..fdb0050
--- /dev/null
+++ b/virtio-i2c.tex
@@ -0,0 +1,139 @@
+\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 slave devices from the guest. By attaching
+the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
+communicate with them without changing or adding extra drivers for these
+slave 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 form:
+
+\begin{lstlisting}
+struct virtio_i2c_req {
+        le16 addr;
+        le32 flags;
+        le16 written;
+        le16 read;
+        u8 write_buf[];
+        u8 read_buf[];
+        u8 status;
+};
+\end{lstlisting}
+
+The \field{addr} of the request is the address of the I2C slave device.
+
+The \field{flags} of the request is currently reserved as zero for future
+feature extensibility.
+
+The \field{written} of the request is the number of data bytes in the \field{write_buf}
+being written to the I2C slave address.
+
+The \field{read} of the request is the number of data bytes in the \field{read_buf}
+being read from the I2C slave address.
+
+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 the \field{read}=0 and the \field{written}>0, the request is called write request.
+
+If the \field{read}>0 and the \field{written}=0, the request is called read request.
+
+If the \field{read}>0 and the \field{written}>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 slave device register to be read.
+
+The \field{read}=0 and at the same time the \field{written}=0 doesn't make any sense.
+The driver SHOULD never send such request.
+
+\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C Adapter Device / Device Operation: Operation Status}
+
+A driver may send one request or multiple requests to the device at a time.
+The requests in the virtqueue MUST be 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 MUST have its \field{status}
+being set to VIRTIO_I2C_MSG_ERR by the device and the requests after the first
+failed one MUST 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}, \field{flags}(currently reserved as zero),
+\field{written} and \field{read} before sending the request.
+
+A driver MUST place one segment of an I2C transaction into \field{write_buf} if the
+\field{written}>0.
+
+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}, \field{written},
+\field{read} and \field{write_buf}.
+
+A device MUST place one segment of an I2C transaction into \field{read_buf} if the
+\field{read}>0.
+
+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 some of them being processed failed,
+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: [PATCH v5] virtio-i2c: add the device specification

[Andy Shevchenko]

On Wed, Nov 25, 2020 at 01:55:18PM +0800, Jie Deng wrote:
> 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

Hmm... Is it used like this here, in this tree?
I would rather expect BugLink:.

> Signed-off-by: Jie Deng <jie.deng@intel.com>

...

> +virtio-i2c is a virtual I2C adapter device. It provides a way to flexibly
> +organize and use the host I2C slave devices from the guest. By attaching
> +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
> +communicate with them without changing or adding extra drivers for these
> +slave I2C devices. 

Trailing white space.

-- 
With Best Regards,
Andy Shevchenko

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

[Jie Deng]

On 2020/11/25 20:52, Andy Shevchenko wrote:
> On Wed, Nov 25, 2020 at 01:55:18PM +0800, Jie Deng wrote:
>> 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
> Hmm... Is it used like this here, in this tree?
> I would rather expect BugLink:.

This is the rule of virtio-spec. It uses github issue to track the 
development.

>> Signed-off-by: Jie Deng <jie.deng@intel.com>
> ...
>
>> +virtio-i2c is a virtual I2C adapter device. It provides a way to flexibly
>> +organize and use the host I2C slave devices from the guest. By attaching
>> +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
>> +communicate with them without changing or adding extra drivers for these
>> +slave I2C devices.
> Trailing white space.
Good catch. Thank you.

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/

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

[Jie Deng]

Hi,

Any update for this ? When will we start to vote on it ?

Regards,
Jie

On 2020/11/25 13:55, Jie Deng wrote:

> 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   | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 167 insertions(+), 4 deletions(-)
>   create mode 100644 virtio-i2c.tex
>

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 v5] virtio-i2c: add the device specification

[Stefan Hajnoczi]

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

On Wed, Nov 25, 2020 at 01:55:18PM +0800, Jie Deng wrote:
> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> new file mode 100644
> index 0000000..fdb0050
> --- /dev/null
> +++ b/virtio-i2c.tex
> @@ -0,0 +1,139 @@
> +\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 slave devices from the guest. By attaching
> +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
> +communicate with them without changing or adding extra drivers for these
> +slave I2C devices. 

Is there a way to identify I2C busses if more than one virtio-i2c device
is present? For example, imagine a host with 2 I2C busses. How does the
guest know which virtio-i2c device connects to which host bus?

> +\begin{lstlisting}
> +struct virtio_i2c_req {
> +        le16 addr;
> +        le32 flags;

What is the memory layout?

1. 0x0 addr, 0x2 flags

or

2. 0x0 addr, 0x4 flags

This is unclear to me. I don't see a general statement in the spec about
struct field alignment/padding and no details in this new spec change.

> +        le16 written;
> +        le16 read;
> +        u8 write_buf[];
> +        u8 read_buf[];
> +        u8 status;
> +};
> +\end{lstlisting}
> +
> +The \field{addr} of the request is the address of the I2C slave device.

https://en.wikipedia.org/wiki/I2c#Addressing_structure suggests there
are at least 7-bit and 10-bit addressing schemes in I2C. How does this
map to the little-endian 16-bit addr field?

> +The \field{flags} of the request is currently reserved as zero for future
> +feature extensibility.
> +
> +The \field{written} of the request is the number of data bytes in the \field{write_buf}
> +being written to the I2C slave address.

This field seems redundant since the device can determine the size of
write_buf implicitly from the total out buffer size. virtio-blk takes
this approach.

> +The \field{read} of the request is the number of data bytes in the \field{read_buf}
> +being read from the I2C slave address.

Same here. virtio-blk doesn't use an explicit read size field because
the device can determine it.

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

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

[Jie Deng]

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


On 2020/12/16 23:52, Stefan Hajnoczi wrote:
> On Wed, Nov 25, 2020 at 01:55:18PM +0800, Jie Deng wrote:
>> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
>> new file mode 100644
>> index 0000000..fdb0050
>> --- /dev/null
>> +++ b/virtio-i2c.tex
>> @@ -0,0 +1,139 @@
>> +\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 slave devices from the guest. By attaching
>> +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
>> +communicate with them without changing or adding extra drivers for these
>> +slave I2C devices.
> Is there a way to identify I2C busses if more than one virtio-i2c device
> is present? For example, imagine a host with 2 I2C busses. How does the
> guest know which virtio-i2c device connects to which host bus?
This virtio-i2c is a master device. The slave devices attached to it can 
be configured
by the backend in the host. These slave devices can be under different 
host I2C master devices.
The guest will see this virtio-i2c master device and its slave devices.

There is a example about the backend which shows how it works

https://projectacrn.github.io/latest/developer-guides/hld/virtio-i2c.html?highlight=i2c
https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/pci/virtio/virtio_i2c.c
>
>> +\begin{lstlisting}
>> +struct virtio_i2c_req {
>> +        le16 addr;
>> +        le32 flags;
> What is the memory layout?
>
> 1. 0x0 addr, 0x2 flags
>
> or
>
> 2. 0x0 addr, 0x4 flags
>
> This is unclear to me. I don't see a general statement in the spec about
> struct field alignment/padding and no details in this new spec change.

Both are OK to me. I used to use "packed" but Michael said there was no 
need to pack it.

https://lkml.org/lkml/2020/9/3/339

So you prefer it to be clear ?

>> +        le16 written;
>> +        le16 read;
>> +        u8 write_buf[];
>> +        u8 read_buf[];
>> +        u8 status;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{addr} of the request is the address of the I2C slave device.
> https://en.wikipedia.org/wiki/I2c#Addressing_structure suggests there
> are at least 7-bit and 10-bit addressing schemes in I2C. How does this
> map to the little-endian 16-bit addr field?

This field maps to the kernel struct "i2c_msg.addr".

struct virtio_i2c_req  *vmsg;
struct i2c_msg *msg;

vmsg->addr  = cpu_to_le16(msg->addr);

The field in the request can be seen as host byte order
while the link can be seen as the I2C network byte order.
The host driver may take care thisconversion.

>> +The \field{flags} of the request is currently reserved as zero for future
>> +feature extensibility.
>> +
>> +The \field{written} of the request is the number of data bytes in the \field{write_buf}
>> +being written to the I2C slave address.
> This field seems redundant since the device can determine the size of
> write_buf implicitly from the total out buffer size. virtio-blk takes
> this approach.
The read/write are the actual number of data bytes being read from or 
written to the device
which is not determined by the device. So I don't think it is redundant.
>> +The \field{read} of the request is the number of data bytes in the \field{read_buf}
>> +being read from the I2C slave address.
> Same here. virtio-blk doesn't use an explicit read size field because
> the device can determine it.

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

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

[Michael S. Tsirkin]

On Thu, Dec 17, 2020 at 03:08:07PM +0800, Jie Deng wrote:
> 
> On 2020/12/16 23:52, Stefan Hajnoczi wrote:
> 
>     On Wed, Nov 25, 2020 at 01:55:18PM +0800, Jie Deng wrote:
> 
>         diff --git a/virtio-i2c.tex b/virtio-i2c.tex
>         new file mode 100644
>         index 0000000..fdb0050
>         --- /dev/null
>         +++ b/virtio-i2c.tex
>         @@ -0,0 +1,139 @@
>         +\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 slave devices from the guest. By attaching
>         +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
>         +communicate with them without changing or adding extra drivers for these
>         +slave I2C devices.
> 
>     Is there a way to identify I2C busses if more than one virtio-i2c device
>     is present? For example, imagine a host with 2 I2C busses. How does the
>     guest know which virtio-i2c device connects to which host bus?
> 
> This virtio-i2c is a master device. The slave devices attached to it can be
> configured
> by the backend in the host. These slave devices can be under different host I2C
> master devices.
> The guest will see this virtio-i2c master device and its slave devices.
> 
> There is a example about the backend which shows how it works
> 
> https://projectacrn.github.io/latest/developer-guides/hld/virtio-i2c.html?
> highlight=i2c
> https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/pci/
> virtio/virtio_i2c.c
> 
> 
> 
>         +\begin{lstlisting}
>         +struct virtio_i2c_req {
>         +        le16 addr;
>         +        le32 flags;
> 
>     What is the memory layout?
> 
>     1. 0x0 addr, 0x2 flags
> 
>     or
> 
>     2. 0x0 addr, 0x4 flags
> 
>     This is unclear to me. I don't see a general statement in the spec about
>     struct field alignment/padding and no details in this new spec change.
> 
> Both are OK to me. I used to use "packed" but Michael said there was no need to
> pack it. 
> 
> https://lkml.org/lkml/2020/9/3/339
> 
> So you prefer it to be clear ?
> 
>         +        le16 written;
>         +        le16 read;
>         +        u8 write_buf[];
>         +        u8 read_buf[];
>         +        u8 status;
>         +};
>         +\end{lstlisting}
>         +
>         +The \field{addr} of the request is the address of the I2C slave device.
> 
>     https://en.wikipedia.org/wiki/I2c#Addressing_structure suggests there
>     are at least 7-bit and 10-bit addressing schemes in I2C. How does this
>     map to the little-endian 16-bit addr field?
> 
> This field maps to the kernel struct "i2c_msg.addr".
> 
> struct virtio_i2c_req *vmsg;
> struct i2c_msg *msg;
> 
> vmsg->addr = cpu_to_le16(msg->addr);
> 
> The field in the request can be seen as host byte order
> while the link can be seen as the I2C network byte order.
> The host driver may take care this conversion.
> 
>         +The \field{flags} of the request is currently reserved as zero for future
>         +feature extensibility.
>         +
>         +The \field{written} of the request is the number of data bytes in the \field{write_buf}
>         +being written to the I2C slave address.
> 
>     This field seems redundant since the device can determine the size of
>     write_buf implicitly from the total out buffer size. virtio-blk takes
>     this approach.
> 
> The read/write are the actual number of data bytes being read from or written
> to the device
> which is not determined by the device. So I don't think it is redundant.

I am still not sure I understand the difference.
This point is unclear to multiple people.



>         +The \field{read} of the request is the number of data bytes in the \field{read_buf}
>         +being read from the I2C slave address.
> 
>     Same here. virtio-blk doesn't use an explicit read size field because
>     the device can determine it.
> 


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 v5] virtio-i2c: add the device specification

[Stefan Hajnoczi]

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

On Thu, Dec 17, 2020 at 03:00:55AM -0500, Michael S. Tsirkin wrote:
> On Thu, Dec 17, 2020 at 03:08:07PM +0800, Jie Deng wrote:
> > 
> > On 2020/12/16 23:52, Stefan Hajnoczi wrote:
> > 
> >     On Wed, Nov 25, 2020 at 01:55:18PM +0800, Jie Deng wrote:
> > 
> >         diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> >         new file mode 100644
> >         index 0000000..fdb0050
> >         --- /dev/null
> >         +++ b/virtio-i2c.tex
> >         @@ -0,0 +1,139 @@
> >         +\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 slave devices from the guest. By attaching
> >         +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
> >         +communicate with them without changing or adding extra drivers for these
> >         +slave I2C devices.
> > 
> >     Is there a way to identify I2C busses if more than one virtio-i2c device
> >     is present? For example, imagine a host with 2 I2C busses. How does the
> >     guest know which virtio-i2c device connects to which host bus?
> > 
> > This virtio-i2c is a master device. The slave devices attached to it can be
> > configured
> > by the backend in the host. These slave devices can be under different host I2C
> > master devices.
> > The guest will see this virtio-i2c master device and its slave devices.
> > 
> > There is a example about the backend which shows how it works
> > 
> > https://projectacrn.github.io/latest/developer-guides/hld/virtio-i2c.html?
> > highlight=i2c
> > https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/pci/
> > virtio/virtio_i2c.c
> > 
> > 
> > 
> >         +\begin{lstlisting}
> >         +struct virtio_i2c_req {
> >         +        le16 addr;
> >         +        le32 flags;
> > 
> >     What is the memory layout?
> > 
> >     1. 0x0 addr, 0x2 flags
> > 
> >     or
> > 
> >     2. 0x0 addr, 0x4 flags
> > 
> >     This is unclear to me. I don't see a general statement in the spec about
> >     struct field alignment/padding and no details in this new spec change.
> > 
> > Both are OK to me. I used to use "packed" but Michael said there was no need to
> > pack it. 
> > 
> > https://lkml.org/lkml/2020/9/3/339
> > 
> > So you prefer it to be clear ?
> > 
> >         +        le16 written;
> >         +        le16 read;
> >         +        u8 write_buf[];
> >         +        u8 read_buf[];
> >         +        u8 status;
> >         +};
> >         +\end{lstlisting}
> >         +
> >         +The \field{addr} of the request is the address of the I2C slave device.
> > 
> >     https://en.wikipedia.org/wiki/I2c#Addressing_structure suggests there
> >     are at least 7-bit and 10-bit addressing schemes in I2C. How does this
> >     map to the little-endian 16-bit addr field?
> > 
> > This field maps to the kernel struct "i2c_msg.addr".
> > 
> > struct virtio_i2c_req *vmsg;
> > struct i2c_msg *msg;
> > 
> > vmsg->addr = cpu_to_le16(msg->addr);
> > 
> > The field in the request can be seen as host byte order
> > while the link can be seen as the I2C network byte order.
> > The host driver may take care this conversion.
> > 
> >         +The \field{flags} of the request is currently reserved as zero for future
> >         +feature extensibility.
> >         +
> >         +The \field{written} of the request is the number of data bytes in the \field{write_buf}
> >         +being written to the I2C slave address.
> > 
> >     This field seems redundant since the device can determine the size of
> >     write_buf implicitly from the total out buffer size. virtio-blk takes
> >     this approach.
> > 
> > The read/write are the actual number of data bytes being read from or written
> > to the device
> > which is not determined by the device. So I don't think it is redundant.
> 
> I am still not sure I understand the difference.
> This point is unclear to multiple people.

I think I get it now. This is made clear by splitting the struct:

  /* Driver->device fields */
  struct virtio_i2c_out_hdr
  {
      le16 addr;
      le16 padding;
      le32 flags;
  };

  /* Device->driver fields */
  struct virtio_i2c_in_hdr
  {
      le16 written;
      le16 read;
      u8 status;
  };

  /*
   * Virtqueue element layout looks like this:
   *
   * struct virtio_i2c_out_hdr out_hdr; /* OUT */
   * u8 write_buf[]; /* OUT */
   * u8 read_buf[]; /* IN */
   * struct virtio_i2c_in_hdr in_hdr; /* IN */
   */

This makes sense to me: a bi-directional request has both write_buf[]
and read_buf[] so the vring used.len field is not enough to report back
how many bytes were written and read. The virtio_i2c_in_hdr fields are
really needed.

Please split the struct in the spec so it's clear how this works.

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

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

[Jie Deng]

On 2020/12/17 18:26, Stefan Hajnoczi wrote:
> On Thu, Dec 17, 2020 at 03:00:55AM -0500, Michael S. Tsirkin wrote:
>> On Thu, Dec 17, 2020 at 03:08:07PM +0800, Jie Deng wrote:
>>>
>>>          +The \field{flags} of the request is currently reserved as zero for future
>>>          +feature extensibility.
>>>          +
>>>          +The \field{written} of the request is the number of data bytes in the \field{write_buf}
>>>          +being written to the I2C slave address.
>>>
>>>      This field seems redundant since the device can determine the size of
>>>      write_buf implicitly from the total out buffer size. virtio-blk takes
>>>      this approach.
>>>
>>> The read/write are the actual number of data bytes being read from or written
>>> to the device
>>> which is not determined by the device. So I don't think it is redundant.
>> I am still not sure I understand the difference.
>> This point is unclear to multiple people.
> I think I get it now. This is made clear by splitting the struct:
>
>    /* Driver->device fields */
>    struct virtio_i2c_out_hdr
>    {
>        le16 addr;
>        le16 padding;
>        le32 flags;
>    };
>
>    /* Device->driver fields */
>    struct virtio_i2c_in_hdr
>    {
>        le16 written;
>        le16 read;
>        u8 status;
>    };

written/read are not device->driver fields. They are driver->device 
fields. They are not determined by the device but the driver(user).

However, Michael said that the two fields may duplicate buf size 
available in the descriptor. He intended to remove them.

"
I note that read and written actually duplicate buf size
available in the descriptor.
Given we no longer mirror i2c_msg 1:1 do we still want to do this?
It will be trivial for the host device to populate these fields
correctly for linux.
Duplication of information iten leads to errors ...
"

But there is a corner case I'm not sure if you have noticed.

read and written can be 0. I think we may not put a buf with size 0 into 
the virtqueue.

@Stefan @Paolo

So what's your opinion about these two fields ?

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

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 v5] virtio-i2c: add the device specification

[Michael S. Tsirkin]

On Fri, Dec 18, 2020 at 10:06:45AM +0800, Jie Deng wrote:
> 
> On 2020/12/17 18:26, Stefan Hajnoczi wrote:
> > On Thu, Dec 17, 2020 at 03:00:55AM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Dec 17, 2020 at 03:08:07PM +0800, Jie Deng wrote:
> > > > 
> > > >          +The \field{flags} of the request is currently reserved as zero for future
> > > >          +feature extensibility.
> > > >          +
> > > >          +The \field{written} of the request is the number of data bytes in the \field{write_buf}
> > > >          +being written to the I2C slave address.
> > > > 
> > > >      This field seems redundant since the device can determine the size of
> > > >      write_buf implicitly from the total out buffer size. virtio-blk takes
> > > >      this approach.
> > > > 
> > > > The read/write are the actual number of data bytes being read from or written
> > > > to the device
> > > > which is not determined by the device. So I don't think it is redundant.
> > > I am still not sure I understand the difference.
> > > This point is unclear to multiple people.
> > I think I get it now. This is made clear by splitting the struct:
> > 
> >    /* Driver->device fields */
> >    struct virtio_i2c_out_hdr
> >    {
> >        le16 addr;
> >        le16 padding;
> >        le32 flags;
> >    };
> > 
> >    /* Device->driver fields */
> >    struct virtio_i2c_in_hdr
> >    {
> >        le16 written;
> >        le16 read;
> >        u8 status;
> >    };
> 
> written/read are not device->driver fields. They are driver->device fields.
> They are not determined by the device but the driver(user).
> 
> However, Michael said that the two fields may duplicate buf size available
> in the descriptor. He intended to remove them.
> 
> "
> I note that read and written actually duplicate buf size
> available in the descriptor.
> Given we no longer mirror i2c_msg 1:1 do we still want to do this?
> It will be trivial for the host device to populate these fields
> correctly for linux.
> Duplication of information iten leads to errors ...
> "
> 
> But there is a corner case I'm not sure if you have noticed.
> 
> read and written can be 0. I think we may not put a buf with size 0 into the
> virtqueue.

You always have the header and the status, right?
E.g. with the below, the total buffer size is virtio_i2c_out_hdr size +
write size for writes and read size + virtio_i2c_in_hdr size for reads.
Neither result is ever 0.

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


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 v5] virtio-i2c: add the device specification

[Jie Deng]

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


On 2020/12/20 3:05, Michael S. Tsirkin wrote:
> On Fri, Dec 18, 2020 at 10:06:45AM +0800, Jie Deng wrote:
>> On 2020/12/17 18:26, Stefan Hajnoczi wrote:
>>> On Thu, Dec 17, 2020 at 03:00:55AM -0500, Michael S. Tsirkin wrote:
>>>> On Thu, Dec 17, 2020 at 03:08:07PM +0800, Jie Deng wrote:
>>>>>           +The \field{flags} of the request is currently reserved as zero for future
>>>>>           +feature extensibility.
>>>>>           +
>>>>>           +The \field{written} of the request is the number of data bytes in the \field{write_buf}
>>>>>           +being written to the I2C slave address.
>>>>>
>>>>>       This field seems redundant since the device can determine the size of
>>>>>       write_buf implicitly from the total out buffer size. virtio-blk takes
>>>>>       this approach.
>>>>>
>>>>> The read/write are the actual number of data bytes being read from or written
>>>>> to the device
>>>>> which is not determined by the device. So I don't think it is redundant.
>>>> I am still not sure I understand the difference.
>>>> This point is unclear to multiple people.
>>> I think I get it now. This is made clear by splitting the struct:
>>>
>>>     /* Driver->device fields */
>>>     struct virtio_i2c_out_hdr
>>>     {
>>>         le16 addr;
>>>         le16 padding;
>>>         le32 flags;
>>>     };
>>>
>>>     /* Device->driver fields */
>>>     struct virtio_i2c_in_hdr
>>>     {
>>>         le16 written;
>>>         le16 read;
>>>         u8 status;
>>>     };
>> written/read are not device->driver fields. They are driver->device fields.
>> They are not determined by the device but the driver(user).
>>
>> However, Michael said that the two fields may duplicate buf size available
>> in the descriptor. He intended to remove them.
>>
>> "
>> I note that read and written actually duplicate buf size
>> available in the descriptor.
>> Given we no longer mirror i2c_msg 1:1 do we still want to do this?
>> It will be trivial for the host device to populate these fields
>> correctly for linux.
>> Duplication of information iten leads to errors ...
>> "
>>
>> But there is a corner case I'm not sure if you have noticed.
>>
>> read and written can be 0. I think we may not put a buf with size 0 into the
>> virtqueue.
> You always have the header and the status, right?
> E.g. with the below, the total buffer size is virtio_i2c_out_hdr size +
> write size for writes and read size + virtio_i2c_in_hdr size for reads.
> Neither result is ever 0.

Then how to distinguish the request type the buffer contains.

Each type will have both virtio_i2c_out_hdr and virtio_i2c_in_hdr.
the backend can know the type by checking the read/written.

If the read=0 and the written>0, the request is a write request
The buffer may contains 3 scatterlist:

virtio_i2c_out_hdr // scatterlist[0]

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

     virtio_i2c_in_hdr // scatterlist[2]

If the read>0 and the written=0, the request is a read request.
The buffer may contains 3 scatterlist:

virtio_i2c_out_hdr // scatterlist[0]

     buf[/* This is read data, since written = 0 */] // scatterlist[1]

     virtio_i2c_in_hdr // scatterlist[2]

If the read>0 and the written>0, the request is a write-read request.
The buffer may contains 4 scatterlist:

virtio_i2c_out_hdr   // scatterlist[0]

     buf[/*write data*/]  // scatterlist[1]

     buf[/*read data*/] // scatterlist[2]

     virtio_i2c_in_hdr // scatterlist[3]

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

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

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

[Cornelia Huck]

On Tue, 22 Dec 2020 14:11:24 +0800
Jie Deng <jie.deng@intel.com> wrote:

> On 2020/12/20 3:05, Michael S. Tsirkin wrote:
> > On Fri, Dec 18, 2020 at 10:06:45AM +0800, Jie Deng wrote:  
> >> On 2020/12/17 18:26, Stefan Hajnoczi wrote:  
> >>> On Thu, Dec 17, 2020 at 03:00:55AM -0500, Michael S. Tsirkin wrote:  
> >>>> On Thu, Dec 17, 2020 at 03:08:07PM +0800, Jie Deng wrote:  
> >>>>>           +The \field{flags} of the request is currently reserved as zero for future
> >>>>>           +feature extensibility.
> >>>>>           +
> >>>>>           +The \field{written} of the request is the number of data bytes in the \field{write_buf}
> >>>>>           +being written to the I2C slave address.
> >>>>>
> >>>>>       This field seems redundant since the device can determine the size of
> >>>>>       write_buf implicitly from the total out buffer size. virtio-blk takes
> >>>>>       this approach.
> >>>>>
> >>>>> The read/write are the actual number of data bytes being read from or written
> >>>>> to the device
> >>>>> which is not determined by the device. So I don't think it is redundant.  
> >>>> I am still not sure I understand the difference.
> >>>> This point is unclear to multiple people.  
> >>> I think I get it now. This is made clear by splitting the struct:
> >>>
> >>>     /* Driver->device fields */
> >>>     struct virtio_i2c_out_hdr
> >>>     {
> >>>         le16 addr;
> >>>         le16 padding;
> >>>         le32 flags;
> >>>     };
> >>>
> >>>     /* Device->driver fields */
> >>>     struct virtio_i2c_in_hdr
> >>>     {
> >>>         le16 written;
> >>>         le16 read;
> >>>         u8 status;
> >>>     };  
> >> written/read are not device->driver fields. They are driver->device fields.
> >> They are not determined by the device but the driver(user).
> >>
> >> However, Michael said that the two fields may duplicate buf size available
> >> in the descriptor. He intended to remove them.
> >>
> >> "
> >> I note that read and written actually duplicate buf size
> >> available in the descriptor.
> >> Given we no longer mirror i2c_msg 1:1 do we still want to do this?
> >> It will be trivial for the host device to populate these fields
> >> correctly for linux.
> >> Duplication of information iten leads to errors ...
> >> "
> >>
> >> But there is a corner case I'm not sure if you have noticed.
> >>
> >> read and written can be 0. I think we may not put a buf with size 0 into the
> >> virtqueue.  
> > You always have the header and the status, right?
> > E.g. with the below, the total buffer size is virtio_i2c_out_hdr size +
> > write size for writes and read size + virtio_i2c_in_hdr size for reads.
> > Neither result is ever 0.  
> 
> Then how to distinguish the request type the buffer contains.

I have read through the thread and I remain confused.

> 
> Each type will have both virtio_i2c_out_hdr and virtio_i2c_in_hdr.
> the backend can know the type by checking the read/written.
> 
> If the read=0 and the written>0, the request is a write request
> The buffer may contains 3 scatterlist:
> 
> virtio_i2c_out_hdr // scatterlist[0]

So, what does virtio_i2c_{out,in}_hdr contain here? If it is the one from
above, ...

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

...we do not know whether there's read data, write data, or what their
length is, until we've actually consumed the whole buffer, and then we
have to go backwards.

> 
> If the read>0 and the written=0, the request is a read request.
> The buffer may contains 3 scatterlist:
> 
> virtio_i2c_out_hdr // scatterlist[0]
> 
>      buf[/* This is read data, since written = 0 */] // scatterlist[1]
> 
>      virtio_i2c_in_hdr // scatterlist[2]
> 
> If the read>0 and the written>0, the request is a write-read request.
> The buffer may contains 4 scatterlist:
> 
> virtio_i2c_out_hdr   // scatterlist[0]
> 
>      buf[/*write data*/]  // scatterlist[1]
> 
>      buf[/*read data*/] // scatterlist[2]
> 
>      virtio_i2c_in_hdr // scatterlist[3]

Is there any reason why we need to infer the type of the request by
checking some lengths? Can't we just specify explicit flags for read
and write? What am I missing?

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


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 v5] virtio-i2c: add the device specification

[Michael S. Tsirkin]

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

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

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

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


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


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 v5] virtio-i2c: add the device specification

[Jie Deng]

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


On 2020/12/23 6:31, Michael S. Tsirkin wrote:
> On Tue, Dec 22, 2020 at 12:29:09PM +0100, Cornelia Huck wrote:
>> Is there any reason why we need to infer the type of the request by
>> checking some lengths? Can't we just specify explicit flags for read
>> and write? What am I missing?
> Point is descriptors already have flags for read/write.
> If there is a read buffer and length > sizeof virtio_i2c_in_hdr then
> we know it's a read request.
> If there is a write buffer and length > sizeof virtio_i2c_out_hdr then
> we know it's a write request.
> If both then both.
>
> All this is known before buffer itself is consumed, which is nice.
>
> Putting this info in flags will duplicate info which is often
> a source of errors.
>
Sent the v6 with field written/read removed.
>


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

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

[Stefan Hajnoczi]

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

On Thu, Dec 17, 2020 at 03:08:07PM +0800, Jie Deng wrote:
> 
> On 2020/12/16 23:52, Stefan Hajnoczi wrote:
> > On Wed, Nov 25, 2020 at 01:55:18PM +0800, Jie Deng wrote:
> > > diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> > > new file mode 100644
> > > index 0000000..fdb0050
> > > --- /dev/null
> > > +++ b/virtio-i2c.tex
> > > @@ -0,0 +1,139 @@
> > > +\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 slave devices from the guest. By attaching
> > > +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
> > > +communicate with them without changing or adding extra drivers for these
> > > +slave I2C devices.
> > Is there a way to identify I2C busses if more than one virtio-i2c device
> > is present? For example, imagine a host with 2 I2C busses. How does the
> > guest know which virtio-i2c device connects to which host bus?
> This virtio-i2c is a master device. The slave devices attached to it can be
> configured
> by the backend in the host. These slave devices can be under different host
> I2C master devices.
> The guest will see this virtio-i2c master device and its slave devices.

There could still be multiple virtio-i2c devices. I guess the guest can
use the device's bus address (e.g. virtio-pci address, virtio-mmio base
address, etc) to differentiate (e.g. guest udev rules).

No changes to the spec are needed here.

> > 
> > > +\begin{lstlisting}
> > > +struct virtio_i2c_req {
> > > +        le16 addr;
> > > +        le32 flags;
> > What is the memory layout?
> > 
> > 1. 0x0 addr, 0x2 flags
> > 
> > or
> > 
> > 2. 0x0 addr, 0x4 flags
> > 
> > This is unclear to me. I don't see a general statement in the spec about
> > struct field alignment/padding and no details in this new spec change.
> 
> Both are OK to me. I used to use "packed" but Michael said there was no need
> to pack it.
> 
> https://lkml.org/lkml/2020/9/3/339
> 
> So you prefer it to be clear ?

Packing wasn't necessary because all fields were aligned in that patch:

  struct virtio_i2c_hdr {
    __virtio16 addr;
    __virtio16 flags;
    __virtio16 len;
  } __packed;

The struct virtio_i2c_req definition in the virtio spec patch is not
naturally aligned though. There are a few options: the fields can be
reordered to achieve that, explicit padding fields can be introduced, or
it can be explicitly marked packed.

That way the memory layout will be clear for anyone wishing to implement
the spec.

> > > +        le16 written;
> > > +        le16 read;
> > > +        u8 write_buf[];
> > > +        u8 read_buf[];
> > > +        u8 status;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +The \field{addr} of the request is the address of the I2C slave device.
> > https://en.wikipedia.org/wiki/I2c#Addressing_structure suggests there
> > are at least 7-bit and 10-bit addressing schemes in I2C. How does this
> > map to the little-endian 16-bit addr field?
> 
> This field maps to the kernel struct "i2c_msg.addr".
> 
> struct virtio_i2c_req  *vmsg;
> struct i2c_msg *msg;
> 
> vmsg->addr  = cpu_to_le16(msg->addr);
> 
> The field in the request can be seen as host byte order
> while the link can be seen as the I2C network byte order.
> The host driver may take care thisconversion.

Please include a description in the spec of how to represent 7-bit and
10-bit I2C addresses as defined by the I2C spec in the le16 addr field.

The other option would be to refer to Linux as you did in your reply,
but nothing else in this device seems Linux-specific so let's avoid the
reference. That way the vitio-i2c spec will be more general and easy to
implement by people who are unfamiliar with Linux.

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

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

[Cornelia Huck]

On Wed, 25 Nov 2020 13:55:18 +0800
Jie Deng <jie.deng@intel.com> wrote:

<some mostly editorial comments; sorry about bringing this up now>

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

s/flexibly/flexibly/

> 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   | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 167 insertions(+), 4 deletions(-)
>  create mode 100644 virtio-i2c.tex
> 

(...)


> diff --git a/introduction.tex b/introduction.tex
> index cc38e29..9f016d5 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.cn/docs/en/user-guide/UM10204.pdf}\\

I think this url should use www.nxp.com instead of www.nxp.com.cn (even
though both seem to lead to the same document).

>  
>  \end{longtable}
>  
> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> new file mode 100644
> index 0000000..fdb0050
> --- /dev/null
> +++ b/virtio-i2c.tex
> @@ -0,0 +1,139 @@
> +\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 slave devices from the guest. By attaching
> +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
> +communicate with them without changing or adding extra drivers for these
> +slave I2C devices. 

I know that the i2c spec talks about 'slave' devices, but can we find a
better terminology for the virtio standard? E.g. prefix this with

"Note: This standard uses the term 'controlled I2C device' to refer to
what the I2C standard calls 'slave I2C device'."

(Or whatever better term might exist.)

> +
> +\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 form:
> +
> +\begin{lstlisting}
> +struct virtio_i2c_req {
> +        le16 addr;
> +        le32 flags;
> +        le16 written;
> +        le16 read;
> +        u8 write_buf[];
> +        u8 read_buf[];
> +        u8 status;
> +};
> +\end{lstlisting}
> +
> +The \field{addr} of the request is the address of the I2C slave device.
> +
> +The \field{flags} of the request is currently reserved as zero for future
> +feature extensibility.
> +
> +The \field{written} of the request is the number of data bytes in the \field{write_buf}
> +being written to the I2C slave address.
> +
> +The \field{read} of the request is the number of data bytes in the \field{read_buf}
> +being read from the I2C slave address.
> +
> +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 the \field{read}=0 and the \field{written}>0, the request is called write request.
> +
> +If the \field{read}>0 and the \field{written}=0, the request is called read request.
> +
> +If the \field{read}>0 and the \field{written}>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 slave device register to be read.
> +
> +The \field{read}=0 and at the same time the \field{written}=0 doesn't make any sense.
> +The driver SHOULD never send such request.

'SHOULD' makes this a normative statement.

> +
> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C Adapter Device / Device Operation: Operation Status}
> +
> +A driver may send one request or multiple requests to the device at a time.
> +The requests in the virtqueue MUST be queued and processed in order.

Same here, 'MUST' makes this a normative statement. I think lowercasing
these terms would make it correct.

> +
> +If a driver sends multiple requests at a time and a device fails to process
> +some of them, then the first failed request MUST have its \field{status}
> +being set to VIRTIO_I2C_MSG_ERR by the device and the requests after the first
> +failed one MUST 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}, \field{flags}(currently reserved as zero),

I'd drop "(currently reserved as zero)" here and add

"A driver MUST set \field{flags} to zero."


> +\field{written} and \field{read} before sending the request.
> +
> +A driver MUST place one segment of an I2C transaction into \field{write_buf} if the

s/if the/if/

> +\field{written}>0.  
> +
> +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}, \field{written},
> +\field{read} and \field{write_buf}.
> +
> +A device MUST place one segment of an I2C transaction into \field{read_buf} if the

s/if/if the/

> +\field{read}>0.  
> +
> +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 some of them being processed failed,
> +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.


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 v5] virtio-i2c: add the device specification

[Michael S. Tsirkin]

On Wed, Dec 16, 2020 at 06:38:59PM +0100, Cornelia Huck wrote:
> On Wed, 25 Nov 2020 13:55:18 +0800
> Jie Deng <jie.deng@intel.com> wrote:
> 
> <some mostly editorial comments; sorry about bringing this up now>
> 
> > virtio-i2c is a virtual I2C adapter device. It provides a way
> > to flexibly communicate with the host I2C slave devices from
> 
> s/flexibly/flexibly/
> 
> > 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>


Given the comments do we want to 

> > ---
> >  conformance.tex  |  28 +++++++++--
> >  content.tex      |   1 +
> >  introduction.tex |   3 ++
> >  virtio-i2c.tex   | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 167 insertions(+), 4 deletions(-)
> >  create mode 100644 virtio-i2c.tex
> > 
> 
> (...)
> 
> 
> > diff --git a/introduction.tex b/introduction.tex
> > index cc38e29..9f016d5 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.cn/docs/en/user-guide/UM10204.pdf}\\
> 
> I think this url should use www.nxp.com instead of www.nxp.com.cn (even
> though both seem to lead to the same document).
> 
> >  
> >  \end{longtable}
> >  
> > diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> > new file mode 100644
> > index 0000000..fdb0050
> > --- /dev/null
> > +++ b/virtio-i2c.tex
> > @@ -0,0 +1,139 @@
> > +\section{I2C Adapter Device}\label{sec:Device Types / I2C Adapter Device}
> > +
> > +virtio-i2c is a virtual I2C adapter device.

BTW is this neessarily a virtual device? One can build a physical
one to this spec, no?


> It provides a way to flexibly
> > +organize and use the host I2C slave devices from the guest. By attaching
> > +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
> > +communicate with them without changing or adding extra drivers for these
> > +slave I2C devices. 
> 
> I know that the i2c spec talks about 'slave' devices, but can we find a
> better terminology for the virtio standard? E.g. prefix this with
> 
> "Note: This standard uses the term 'controlled I2C device' to refer to
> what the I2C standard calls 'slave I2C device'."
> 
> (Or whatever better term might exist.)
> 
> > +
> > +\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 form:

of the form

> > +
> > +\begin{lstlisting}
> > +struct virtio_i2c_req {
> > +        le16 addr;
> > +        le32 flags;
> > +        le16 written;
> > +        le16 read;
> > +        u8 write_buf[];
> > +        u8 read_buf[];
> > +        u8 status;
> > +};
> > +\end{lstlisting}
> > +
> > +The \field{addr} of the request is the address of the I2C slave device.
> > +
> > +The \field{flags} of the request is currently reserved as zero for future
> > +feature extensibility.
> > +
> > +The \field{written} of the request is the number of data bytes in the \field{write_buf}
> > +being written to the I2C slave address.
> > +
> > +The \field{read} of the request is the number of data bytes in the \field{read_buf}
> > +being read from the I2C slave address.

I note that read and written actually duplicate buf size
available in the descriptor.
Given we no longer mirror i2c_msg 1:1 do we still want to do this?
It will be trivial for the host device to populate these fields
correctly for linux.
Duplication of information iten leads to errors ...


> > +
> > +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 the \field{read}=0 and the \field{written}>0, the request is called write request.
> > +
> > +If the \field{read}>0 and the \field{written}=0, the request is called read request.
> > +
> > +If the \field{read}>0 and the \field{written}>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 slave device register to be read.
> > +
> > +The \field{read}=0 and at the same time the \field{written}=0 doesn't make any sense.
> > +The driver SHOULD never send such request.
> 
> 'SHOULD' makes this a normative statement.
> 
> > +
> > +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C Adapter Device / Device Operation: Operation Status}
> > +
> > +A driver may send one request or multiple requests to the device at a time.
> > +The requests in the virtqueue MUST be queued and processed in order.
> 
> Same here, 'MUST' makes this a normative statement. I think lowercasing
> these terms would make it correct.
> 


It won't, the relevant rfc includes MUST and must etc.

If this is normative move it to a normative statement.
Or preferably, both change this to e.g.

The requests in the virtqueue are both queued and processed in order.

and add a normative statement in the normative section.


> > +
> > +If a driver sends multiple requests at a time and a device fails to process
> > +some of them, then the first failed request MUST have its \field{status}
> > +being set to VIRTIO_I2C_MSG_ERR by the device and the requests after the first
> > +failed one MUST 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}, \field{flags}(currently reserved as zero),
> 
> I'd drop "(currently reserved as zero)" here and add
> 
> "A driver MUST set \field{flags} to zero."
> 
> 
> > +\field{written} and \field{read} before sending the request.
> > +
> > +A driver MUST place one segment of an I2C transaction into \field{write_buf} if the
> 
> s/if the/if/
> 
> > +\field{written}>0.  
> > +
> > +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}, \field{written},
> > +\field{read} and \field{write_buf}.
> > +
> > +A device MUST place one segment of an I2C transaction into \field{read_buf} if the
> 
> s/if/if the/
> 
> > +\field{read}>0.  
> > +
> > +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 some of them being processed failed,

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.

So what is it saying? That VIRTIO_I2C_MSG_ERR can also mean
"previous request failed"?

-- 
MST


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 v5] virtio-i2c: add the device specification

[Jie Deng]

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


On 2020/12/17 3:55, Michael S. Tsirkin wrote:
> On Wed, Dec 16, 2020 at 06:38:59PM +0100, Cornelia Huck wrote:
>> On Wed, 25 Nov 2020 13:55:18 +0800
>> Jie Deng <jie.deng@intel.com> wrote:
>>
>> <some mostly editorial comments; sorry about bringing this up now>
>>
>>> virtio-i2c is a virtual I2C adapter device. It provides a way
>>> to flexibly communicate with the host I2C slave devices from
>> s/flexibly/flexibly/
>>
>>> 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>
>
> Given the comments do we want to

I didn't get it. Can you explain more?

>
>>> ---
>>>   conformance.tex  |  28 +++++++++--
>>>   content.tex      |   1 +
>>>   introduction.tex |   3 ++
>>>   virtio-i2c.tex   | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 167 insertions(+), 4 deletions(-)
>>>   create mode 100644 virtio-i2c.tex
>>>
>> (...)
>>
>>
>>> diff --git a/introduction.tex b/introduction.tex
>>> index cc38e29..9f016d5 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.cn/docs/en/user-guide/UM10204.pdf}\\
>> I think this url should use www.nxp.com instead of www.nxp.com.cn (even
>> though both seem to lead to the same document).
>>
>>>   
>>>   \end{longtable}
>>>   
>>> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
>>> new file mode 100644
>>> index 0000000..fdb0050
>>> --- /dev/null
>>> +++ b/virtio-i2c.tex
>>> @@ -0,0 +1,139 @@
>>> +\section{I2C Adapter Device}\label{sec:Device Types / I2C Adapter Device}
>>> +
>>> +virtio-i2c is a virtual I2C adapter device.
> BTW is this neessarily a virtual device? One can build a physical
> one to this spec, no?
The virtio-i2c is a virtual I2C master device. One can attach a host 
physical
slave I2C device to this virtual device.
>
>> It provides a way to flexibly
>>> +organize and use the host I2C slave devices from the guest. By attaching
>>> +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
>>> +communicate with them without changing or adding extra drivers for these
>>> +slave I2C devices.
>> I know that the i2c spec talks about 'slave' devices, but can we find a
>> better terminology for the virtio standard? E.g. prefix this with
>>
>> "Note: This standard uses the term 'controlled I2C device' to refer to
>> what the I2C standard calls 'slave I2C device'."
>>
>> (Or whatever better term might exist.)
>>
>>> +
>>> +\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 form:
> of the form
Will fix it.
>>> +
>>> +\begin{lstlisting}
>>> +struct virtio_i2c_req {
>>> +        le16 addr;
>>> +        le32 flags;
>>> +        le16 written;
>>> +        le16 read;
>>> +        u8 write_buf[];
>>> +        u8 read_buf[];
>>> +        u8 status;
>>> +};
>>> +\end{lstlisting}
>>> +
>>> +The \field{addr} of the request is the address of the I2C slave device.
>>> +
>>> +The \field{flags} of the request is currently reserved as zero for future
>>> +feature extensibility.
>>> +
>>> +The \field{written} of the request is the number of data bytes in the \field{write_buf}
>>> +being written to the I2C slave address.
>>> +
>>> +The \field{read} of the request is the number of data bytes in the \field{read_buf}
>>> +being read from the I2C slave address.
> I note that read and written actually duplicate buf size
> available in the descriptor.
> Given we no longer mirror i2c_msg 1:1 do we still want to do this?
> It will be trivial for the host device to populate these fields
> correctly for linux.
> Duplication of information iten leads to errors ...
Got it. I'm OK to remove read and written and use the size in the 
descriptor.
>
>>> +
>>> +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 the \field{read}=0 and the \field{written}>0, the request is called write request.
>>> +
>>> +If the \field{read}>0 and the \field{written}=0, the request is called read request.
>>> +
>>> +If the \field{read}>0 and the \field{written}>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 slave device register to be read.
>>> +
>>> +The \field{read}=0 and at the same time the \field{written}=0 doesn't make any sense.
>>> +The driver SHOULD never send such request.
>> 'SHOULD' makes this a normative statement.
>>
>>> +
>>> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C Adapter Device / Device Operation: Operation Status}
>>> +
>>> +A driver may send one request or multiple requests to the device at a time.
>>> +The requests in the virtqueue MUST be queued and processed in order.
>> Same here, 'MUST' makes this a normative statement. I think lowercasing
>> these terms would make it correct.
>>
>
> It won't, the relevant rfc includes MUST and must etc.
>
> If this is normative move it to a normative statement.
> Or preferably, both change this to e.g.
>
> The requests in the virtqueue are both queued and processed in order.
>
> and add a normative statement in the normative section.
>
OK. Will fix it.
>>> +
>>> +If a driver sends multiple requests at a time and a device fails to process
>>> +some of them, then the first failed request MUST have its \field{status}
>>> +being set to VIRTIO_I2C_MSG_ERR by the device and the requests after the first
>>> +failed one MUST 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}, \field{flags}(currently reserved as zero),
>> I'd drop "(currently reserved as zero)" here and add
>>
>> "A driver MUST set \field{flags} to zero."
>>
>>
>>> +\field{written} and \field{read} before sending the request.
>>> +
>>> +A driver MUST place one segment of an I2C transaction into \field{write_buf} if the
>> s/if the/if/
>>
>>> +\field{written}>0.
>>> +
>>> +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}, \field{written},
>>> +\field{read} and \field{write_buf}.
>>> +
>>> +A device MUST place one segment of an I2C transaction into \field{read_buf} if the
>> s/if/if the/
>>
>>> +\field{read}>0.
>>> +
>>> +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 some of them being processed failed,
> and processing of some of the requests fails
Will fix it.
>>> +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.
> So what is it saying? That VIRTIO_I2C_MSG_ERR can also mean
> "previous request failed"?

It means if for example 5 requests are sent and received with
req3 failed.Since the driver musttreat the first failed request (req3) and the requests after the first 
failed one (req4 and req5) as VIRTIO_I2C_MSG_ERR.

So we don't need to care about the status of the req4 and req5 if req3 failed.They will
be treated as failed no matter what status.
  
req1(OK)  req2(OK)  req3(FAIL)  req4  req5


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

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

[Michael S. Tsirkin]

On Thu, Dec 17, 2020 at 04:38:54PM +0800, Jie Deng wrote:
> 
> On 2020/12/17 3:55, Michael S. Tsirkin wrote:
> 
>     On Wed, Dec 16, 2020 at 06:38:59PM +0100, Cornelia Huck wrote:
> 
>         On Wed, 25 Nov 2020 13:55:18 +0800
>         Jie Deng <jie.deng@intel.com> wrote:
> 
>         <some mostly editorial comments; sorry about bringing this up now>
> 
> 
>             virtio-i2c is a virtual I2C adapter device. It provides a way
>             to flexibly communicate with the host I2C slave devices from
> 
>         s/flexibly/flexibly/
> 
> 
>             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>
> 
> 
>     Given the comments do we want to
> 
> I didn't get it. Can you explain more?

Just a heads up that it looks like the TC seems to
be asking for some comments to be addressed.

Do you want to cancel the vote for now and try to address
the comments with a new revision?


> 
> 
>             ---
>              conformance.tex  |  28 +++++++++--
>              content.tex      |   1 +
>              introduction.tex |   3 ++
>              virtio-i2c.tex   | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>              4 files changed, 167 insertions(+), 4 deletions(-)
>              create mode 100644 virtio-i2c.tex
> 
> 
>         (...)
> 
> 
> 
>             diff --git a/introduction.tex b/introduction.tex
>             index cc38e29..9f016d5 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.cn/docs/en/user-guide/UM10204.pdf}\\
> 
>         I think this url should use www.nxp.com instead of www.nxp.com.cn (even
>         though both seem to lead to the same document).
> 
> 
> 
>              \end{longtable}
> 
>             diff --git a/virtio-i2c.tex b/virtio-i2c.tex
>             new file mode 100644
>             index 0000000..fdb0050
>             --- /dev/null
>             +++ b/virtio-i2c.tex
>             @@ -0,0 +1,139 @@
>             +\section{I2C Adapter Device}\label{sec:Device Types / I2C Adapter Device}
>             +
>             +virtio-i2c is a virtual I2C adapter device.
> 
>     BTW is this neessarily a virtual device? One can build a physical
>     one to this spec, no?
> 
> The virtio-i2c is a virtual I2C master device. One can attach a host physical
> slave I2C device to this virtual device.
> 
> 
> 
>         It provides a way to flexibly
> 
>             +organize and use the host I2C slave devices from the guest. By attaching
>             +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
>             +communicate with them without changing or adding extra drivers for these
>             +slave I2C devices.
> 
>         I know that the i2c spec talks about 'slave' devices, but can we find a
>         better terminology for the virtio standard? E.g. prefix this with
> 
>         "Note: This standard uses the term 'controlled I2C device' to refer to
>         what the I2C standard calls 'slave I2C device'."
> 
>         (Or whatever better term might exist.)
> 
> 
>             +
>             +\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 form:
> 
>     of the form
> 
> Will fix it.
> 
>             +
>             +\begin{lstlisting}
>             +struct virtio_i2c_req {
>             +        le16 addr;
>             +        le32 flags;
>             +        le16 written;
>             +        le16 read;
>             +        u8 write_buf[];
>             +        u8 read_buf[];
>             +        u8 status;
>             +};
>             +\end{lstlisting}
>             +
>             +The \field{addr} of the request is the address of the I2C slave device.
>             +
>             +The \field{flags} of the request is currently reserved as zero for future
>             +feature extensibility.
>             +
>             +The \field{written} of the request is the number of data bytes in the \field{write_buf}
>             +being written to the I2C slave address.
>             +
>             +The \field{read} of the request is the number of data bytes in the \field{read_buf}
>             +being read from the I2C slave address.
> 
>     I note that read and written actually duplicate buf size
>     available in the descriptor.
>     Given we no longer mirror i2c_msg 1:1 do we still want to do this?
>     It will be trivial for the host device to populate these fields
>     correctly for linux.
>     Duplication of information iten leads to errors ...
> 
> Got it. I'm OK to remove read and written and use the size in the descriptor.
> 
> 
> 
>             +
>             +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 the \field{read}=0 and the \field{written}>0, the request is called write request.
>             +
>             +If the \field{read}>0 and the \field{written}=0, the request is called read request.
>             +
>             +If the \field{read}>0 and the \field{written}>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 slave device register to be read.
>             +
>             +The \field{read}=0 and at the same time the \field{written}=0 doesn't make any sense.
>             +The driver SHOULD never send such request.
> 
>         'SHOULD' makes this a normative statement.
> 
> 
>             +
>             +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C Adapter Device / Device Operation: Operation Status}
>             +
>             +A driver may send one request or multiple requests to the device at a time.
>             +The requests in the virtqueue MUST be queued and processed in order.
> 
>         Same here, 'MUST' makes this a normative statement. I think lowercasing
>         these terms would make it correct.
> 
> 
> 
>     It won't, the relevant rfc includes MUST and must etc.
> 
>     If this is normative move it to a normative statement.
>     Or preferably, both change this to e.g.
> 
>     The requests in the virtqueue are both queued and processed in order.
> 
>     and add a normative statement in the normative section.
> 
> 
> OK. Will fix it.
> 
>             +
>             +If a driver sends multiple requests at a time and a device fails to process
>             +some of them, then the first failed request MUST have its \field{status}
>             +being set to VIRTIO_I2C_MSG_ERR by the device and the requests after the first
>             +failed one MUST 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}, \field{flags}(currently reserved as zero),
> 
>         I'd drop "(currently reserved as zero)" here and add
> 
>         "A driver MUST set \field{flags} to zero."
> 
> 
> 
>             +\field{written} and \field{read} before sending the request.
>             +
>             +A driver MUST place one segment of an I2C transaction into \field{write_buf} if the
> 
>         s/if the/if/
> 
> 
>             +\field{written}>0.
>             +
>             +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}, \field{written},
>             +\field{read} and \field{write_buf}.
>             +
>             +A device MUST place one segment of an I2C transaction into \field{read_buf} if the
> 
>         s/if/if the/
> 
> 
>             +\field{read}>0.
>             +
>             +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 some of them being processed failed,
> 
>     and processing of some of the requests fails
> 
> Will fix it.
> 
>             +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.
> 
>     So what is it saying? That VIRTIO_I2C_MSG_ERR can also mean
>     "previous request failed"?
> 
> It means if for example 5 requests are sent and received with
> req3 failed.Since the driver must treat the first failed request (req3)
> and the requests after the first failed one (req4 and req5) as VIRTIO_I2C_MSG_ERR.
> 
> So we don't need to care about the status of the req4 and req5 if req3 failed.They will
> be treated as failed no matter what status.
> 
> req1(OK)  req2(OK)  req3(FAIL)  req4  req5
> 
> 


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 v5] virtio-i2c: add the device specification

[Jie Deng]

On 2020/12/18 3:43, Michael S. Tsirkin wrote:
> Just a heads up that it looks like the TC seems to
> be asking for some comments to be addressed.
>
> Do you want to cancel the vote for now and try to address
> the comments with a new revision?
Yes. I will address these comments and send the v6.

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 v5] virtio-i2c: add the device specification

[Jie Deng]

Thank you. I will fix them.

On 2020/12/17 1:38, Cornelia Huck wrote:
> On Wed, 25 Nov 2020 13:55:18 +0800
> Jie Deng <jie.deng@intel.com> wrote:
>
> <some mostly editorial comments; sorry about bringing this up now>
>
>> +
>> +A driver MUST place one segment of an I2C transaction into \field{write_buf} if the
>>
>> s/if the/if/
>>
>> +
>> +A device MUST place one segment of an I2C transaction into \field{read_buf} if the
> s/if/if the/

Do you mean  "s/if the/if/" here ?

>
> irst failed one to be VIRTIO_I2C_MSG_ERR.

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 v5] virtio-i2c: add the device specification

[Cornelia Huck]

On Thu, 17 Dec 2020 15:29:57 +0800
Jie Deng <jie.deng@intel.com> wrote:

> Thank you. I will fix them.
> 
> On 2020/12/17 1:38, Cornelia Huck wrote:
> > On Wed, 25 Nov 2020 13:55:18 +0800
> > Jie Deng <jie.deng@intel.com> wrote:
> >
> > <some mostly editorial comments; sorry about bringing this up now>
> >  
> >> +
> >> +A driver MUST place one segment of an I2C transaction into \field{write_buf} if the
> >>
> >> s/if the/if/
> >>
> >> +
> >> +A device MUST place one segment of an I2C transaction into \field{read_buf} if the  
> > s/if/if the/  
> 
> Do you mean  "s/if the/if/" here ?

Yes.

> 
> >
> > irst failed one to be VIRTIO_I2C_MSG_ERR.  
> 


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/