Re: [PATCH v4 0/2] Add remote I2C device to support external I2C device

[Corey Minyard]

On Fri, Aug 13, 2021 at 10:37:00AM -0400, Shengtan Mao wrote:
> Hi Corey,
> Thank you so much for your feedback. I took some time to discuss these
> points with my team.
> 
> 1. Blocking QEMU I/O
> Thanks for bringing this to our attention. We acknowledge it as a drawback,
> but we hope that by indicating this clearly in an updated docs, the user
> will accept these drawbacks when they use it. In short, we hope that you
> will consider merging this patch despite this drawback.

I understand the complexity this will add.  I'm not sure of the general
QEMU policy on this.  Everything that I know of that relies on remote
responses is done with async I/O.  The way you have proposed it would be
ok for testing, I suppose, but I don't think it would be ok in a
production system.  Things like this have a way of sneaking in to more
uses than you imagined.  (Hey, we can use this to tie in to a real
TPM!).

So I don't know.  I'll add Peter to the To: and see if he can speak to
this.

> 
> 2. Error Handling
> We will add them for the next version.
> 
> 3. Remote end NACK
> It  would be helpful to have a bit more information on this. The remote I2C
> in QEMU redirects the return value (which can indicate NACK / ACK) from the
> external device, so the remote end should be able to NACK if prompted by
> the external device. If you are saying that the remote I2C should be able
> to NACK independently, then no, it doesn't support that, but we can take
> steps to add it.

I believe you will eventually need the ability to get a remote NACK.
You may also need the ability to signal an interrupt from the remote
device (though that will require async input).  The information back
from the remote needs some sort of tag to make it extensible.

> 
> I am leaving google, so I will be passing this project to my team. Thank
> you again for your feedback.

Well, bummer.  This is a good idea hopefully we can see it through.

Thanks,

-corey


> 
> best,
> Shengtan
> 
> On Fri, Aug 6, 2021 at 9:55 PM Corey Minyard <cminyard@mvista.com> wrote:
> 
> > On Fri, Aug 06, 2021 at 11:49:16PM +0000, Shengtan Mao wrote:
> > > This patch implements the remote I2C device.  The remote I2C device
> > allows an
> > > external I2C device to communicate with the I2C controller in QEMU
> > through the
> > > remote I2C protocol.  Users no longer have to directly modify QEMU to
> > add new
> > > I2C devices and can instead implement the emulated device externally and
> > > connect it to the remote I2C device.
> >
> > I got to spend some time on this today, and I like the concept, but
> > there is one major issue.  When you do a read, you are blocking the
> > entire qemu I/O system until the read returns, which may result in
> > issues.  At least that's may understanding of how the qemu I/O system
> > works, which may be dated or wrong.
> >
> > If you look at the IPMI code, it as an external BMC that can handle
> > async I/O from the chardev.  But the IPMI subsystem is designed to
> > handle this sort of thing.
> >
> > Unfortunately, the I2C code really isn't set up to handle async
> > operations.  I'm not sure how hard it would be to modify the I2C core to
> > handle this, but it doesn't look trivial.  Well, the changes to the core
> > wouldn't be terrible, but all the host devices are set up for
> > synchronous operation.  You could add a separate asynchronous interface,
> > and only host devices that were modified could use it, and your device
> > would only work on those host devices.
> >
> > Another issue is that you aren't handling errors from the chr read/write
> > calls.  If the remote connection dies, this isn't going to be good.  You
> > have to do error handling.
> >
> > There is also no way for the remote end to return a NACK.  That's pretty
> > important, I think.  It will, unfortunately, complicate your nice simple
> > protocol.
> >
> > Sorry to be the bearer of bad news.  Maybe I'm wrong about the blocking
> > thing, I'd be happy to be wrong.
> >
> > -corey
> >
> > >
> > > Previous work by Wolfram Sang
> > > (
> > https://git.kernel.org/pub/scm/virt/qemu/wsa/qemu.git/commit/?h=i2c-passthrough
> > )
> > > was referenced.  It shares the similar idea of redirecting the actual
> > I2C device
> > > functionalities, but Sang focuses on physical devices, and we focus on
> > emulated devices.
> > > The work by Sang mainly utilizes file descriptors while ours utilizes
> > character
> > > devices, which offers better support for emulated devices. The work by
> > Sang is
> > > not meant to offer full I2C device support; it only implements the
> > receive
> > > functionality.  Our work implements full support for I2C devices: send,
> > recv,
> > > and event (match_and_add is not applicable for external devices).
> > >
> > > v1 -> v2
> > >     fixed terminology errors in the description comments.
> > > v2 -> v3
> > >     corrected patch set emailing errors.
> > > v3 -> v4
> > >     added remote I2C protocol description in docs/specs
> > >
> > > Shengtan Mao (2):
> > >   hw/i2c: add remote I2C device
> > >   docs/specs: add remote i2c docs
> > >
> > >  docs/specs/index.rst          |   1 +
> > >  docs/specs/remote-i2c.rst     |  51 ++++++++
> > >  hw/arm/Kconfig                |   1 +
> > >  hw/i2c/Kconfig                |   4 +
> > >  hw/i2c/meson.build            |   1 +
> > >  hw/i2c/remote-i2c.c           | 117 ++++++++++++++++++
> > >  tests/qtest/meson.build       |   1 +
> > >  tests/qtest/remote-i2c-test.c | 216 ++++++++++++++++++++++++++++++++++
> > >  8 files changed, 392 insertions(+)
> > >  create mode 100644 docs/specs/remote-i2c.rst
> > >  create mode 100644 hw/i2c/remote-i2c.c
> > >  create mode 100644 tests/qtest/remote-i2c-test.c
> > >
> > > --
> > > 2.32.0.605.g8dce9f2422-goog
> > >
> >