[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] [PATCH v5] virtio-i2c: add the device specification
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:
signature.asc
Description: PGP signature
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]