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

["Michael S. Tsirkin" <mst@redhat.com>]

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