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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- 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 --]