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

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

[Shengtan Mao]

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.

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

[PATCH v4 1/2] hw/i2c: add remote I2C device

[Shengtan Mao]

This patch adds the remote I2C device, which supports the usage of
external I2C devices.
Signed-off-by: Shengtan Mao <stmao@google.com>
---
 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 ++++++++++++++++++++++++++++++++++
 6 files changed, 340 insertions(+)
 create mode 100644 hw/i2c/remote-i2c.c
 create mode 100644 tests/qtest/remote-i2c-test.c

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 4ba0aca067..922a3efdcc 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -393,6 +393,7 @@ config NPCM7XX
     select MAX34451
     select PL310  # cache controller
     select PMBUS
+    select REMOTE_I2C
     select SERIAL
     select SSI
     select UNIMP
diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
index 8217cb5041..278156991d 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -1,6 +1,10 @@
 config I2C
     bool
 
+config REMOTE_I2C
+    bool
+    select I2C
+
 config SMBUS
     bool
     select I2C
diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
index d3df273251..ba0215db61 100644
--- a/hw/i2c/meson.build
+++ b/hw/i2c/meson.build
@@ -6,6 +6,7 @@ i2c_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('smbus_ich9.c'))
 i2c_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_i2c.c'))
 i2c_ss.add(when: 'CONFIG_BITBANG_I2C', if_true: files('bitbang_i2c.c'))
 i2c_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4210_i2c.c'))
+i2c_ss.add(when: 'CONFIG_REMOTE_I2C', if_true: files('remote-i2c.c'))
 i2c_ss.add(when: 'CONFIG_IMX_I2C', if_true: files('imx_i2c.c'))
 i2c_ss.add(when: 'CONFIG_MPC_I2C', if_true: files('mpc_i2c.c'))
 i2c_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('microbit_i2c.c'))
diff --git a/hw/i2c/remote-i2c.c b/hw/i2c/remote-i2c.c
new file mode 100644
index 0000000000..083eaf2210
--- /dev/null
+++ b/hw/i2c/remote-i2c.c
@@ -0,0 +1,117 @@
+/*
+ * Remote I2C Device
+ *
+ * Copyright (c) 2021 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+
+#include "chardev/char-fe.h"
+#include "hw/i2c/i2c.h"
+#include "hw/qdev-properties-system.h"
+
+#define TYPE_REMOTE_I2C "remote-i2c"
+#define REMOTE_I2C(obj) OBJECT_CHECK(RemoteI2CState, (obj), TYPE_REMOTE_I2C)
+#define ONE_BYTE 1
+
+typedef struct {
+    I2CSlave parent_obj;
+    CharBackend chr;
+} RemoteI2CState;
+
+typedef enum {
+    REMOTE_I2C_START_RECV = 0,
+    REMOTE_I2C_START_SEND = 1,
+    REMOTE_I2C_FINISH = 2,
+    REMOTE_I2C_NACK = 3,
+    REMOTE_I2C_RECV = 4,
+    REMOTE_I2C_SEND = 5,
+} RemoteI2CCommand;
+
+static uint8_t remote_i2c_recv(I2CSlave *s)
+{
+    RemoteI2CState *i2c = REMOTE_I2C(s);
+    uint8_t resp = 0;
+    uint8_t type = REMOTE_I2C_RECV;
+    qemu_chr_fe_write_all(&i2c->chr, &type, ONE_BYTE);
+
+    qemu_chr_fe_read_all(&i2c->chr, &resp, ONE_BYTE);
+    return resp;
+}
+
+static int remote_i2c_send(I2CSlave *s, uint8_t data)
+{
+    RemoteI2CState *i2c = REMOTE_I2C(s);
+    uint8_t type = REMOTE_I2C_SEND;
+    uint8_t resp = 1;
+    qemu_chr_fe_write_all(&i2c->chr, &type, ONE_BYTE);
+    qemu_chr_fe_write_all(&i2c->chr, &data, ONE_BYTE);
+
+    qemu_chr_fe_read_all(&i2c->chr, &resp, ONE_BYTE);
+    return resp ? -1 : 0;
+}
+
+/* Returns non-zero when no response from the device. */
+static int remote_i2c_event(I2CSlave *s, enum i2c_event event)
+{
+    RemoteI2CState *i2c = REMOTE_I2C(s);
+    uint8_t type;
+    uint8_t resp = 1;
+    switch (event) {
+    case I2C_START_RECV:
+        type = REMOTE_I2C_START_RECV;
+        break;
+    case I2C_START_SEND:
+        type = REMOTE_I2C_START_SEND;
+        break;
+    case I2C_FINISH:
+        type = REMOTE_I2C_FINISH;
+        break;
+    case I2C_NACK:
+        type = REMOTE_I2C_NACK;
+    }
+    qemu_chr_fe_write_all(&i2c->chr, &type, ONE_BYTE);
+    qemu_chr_fe_read_all(&i2c->chr, &resp, ONE_BYTE);
+    return resp ? -1 : 0;
+}
+
+static Property remote_i2c_props[] = {
+    DEFINE_PROP_CHR("chardev", RemoteI2CState, chr),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void remote_i2c_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+
+    k->recv = &remote_i2c_recv;
+    k->send = &remote_i2c_send;
+    k->event = &remote_i2c_event;
+    device_class_set_props(dc, remote_i2c_props);
+}
+
+static const TypeInfo remote_i2c_type = {
+    .name = TYPE_REMOTE_I2C,
+    .parent = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(RemoteI2CState),
+    .class_size = sizeof(I2CSlaveClass),
+    .class_init = remote_i2c_class_init,
+};
+
+static void remote_i2c_register(void)
+{
+    type_register_static(&remote_i2c_type);
+}
+
+type_init(remote_i2c_register)
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e22a0792c5..95faa2c379 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -218,6 +218,7 @@ qos_test_ss.add(
   'pca9552-test.c',
   'pci-test.c',
   'pcnet-test.c',
+  'remote-i2c-test.c',
   'sdhci-test.c',
   'spapr-phb-test.c',
   'tmp105-test.c',
diff --git a/tests/qtest/remote-i2c-test.c b/tests/qtest/remote-i2c-test.c
new file mode 100644
index 0000000000..b6ab210e4c
--- /dev/null
+++ b/tests/qtest/remote-i2c-test.c
@@ -0,0 +1,216 @@
+/*
+ * QTests for Remote I2C Device
+ *
+ * Copyright (c) 2021 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+
+#include "qemu/config-file.h"
+#include "sysemu/sysemu.h"
+#include "libqos/qgraph.h"
+#include "libqos/i2c.h"
+
+#include <sys/socket.h>
+
+#define TEST_ID "remote-i2c-test"
+#define TEST_ADDR (0x62)
+#define QEMU_CMD_CHR                                                           \
+    " -chardev socket,id=i2c-chardev,host=localhost,port=%d,reconnect=10"
+
+typedef enum {
+    REMOTE_I2C_START_RECV = 0,
+    REMOTE_I2C_START_SEND = 1,
+    REMOTE_I2C_FINISH = 2,
+    REMOTE_I2C_NACK = 3,
+    REMOTE_I2C_RECV = 4,
+    REMOTE_I2C_SEND = 5,
+} RemoteI2CCommand;
+
+static int setup_fd(int *sock)
+{
+    fd_set readfds;
+    int fd;
+
+    FD_ZERO(&readfds);
+    FD_SET(*sock, &readfds);
+    g_assert(select((*sock) + 1, &readfds, NULL, NULL, NULL) == 1);
+
+    fd = accept(*sock, NULL, 0);
+    g_assert(fd >= 0);
+
+    return fd;
+}
+
+static void test_recv(QI2CDevice *i2cdev, int fd, uint8_t *msg, uint16_t len)
+{
+    uint16_t buf_size = len + 2;
+    uint8_t *buf = g_new(uint8_t, buf_size);
+    uint16_t bytes_read = 0;
+    uint8_t zero = 0;
+    ssize_t rv;
+
+    /* write device responses to socket */
+    rv = write(fd, &zero, 1);
+    g_assert_cmpint(rv, ==, 1);
+    rv = write(fd, msg, len);
+    g_assert_cmpint(rv, ==, len);
+    rv = write(fd, &zero, 1);
+    g_assert_cmpint(rv, ==, 1);
+
+    /* check received value */
+    qi2c_recv(i2cdev, buf, len);
+    for (int i = 0; i < len; ++i) {
+        g_assert_cmphex(buf[i], ==, msg[i]);
+    }
+
+    /* check controller writes to chardev */
+    do {
+        bytes_read += read(fd, buf + bytes_read, buf_size - bytes_read);
+    } while (bytes_read < buf_size);
+
+    g_assert_cmphex(buf[0], ==, REMOTE_I2C_START_RECV);
+    for (int i = 1; i < len - 1; ++i) {
+        g_assert_cmphex(buf[i], ==, REMOTE_I2C_RECV);
+    }
+    g_assert_cmphex(buf[buf_size - 1], ==, REMOTE_I2C_FINISH);
+
+    g_free(buf);
+}
+
+static void test_send(QI2CDevice *i2cdev, int fd, uint8_t *msg, uint16_t len)
+{
+    uint16_t buf_size = len * 2 + 2;
+    uint8_t *buf = g_new0(uint8_t, buf_size);
+    uint16_t bytes_read = 0;
+    ssize_t rv;
+    int j = 0;
+
+    /* write device ACKs to socket*/
+    rv = write(fd, buf, len + 2);
+    g_assert_cmpint(rv, ==, len + 2);
+
+    qi2c_send(i2cdev, msg, len);
+
+    /* check controller writes to chardev */
+    do {
+        bytes_read += read(fd, buf + bytes_read, buf_size - bytes_read);
+    } while (bytes_read < buf_size);
+
+    g_assert_cmphex(buf[0], ==, REMOTE_I2C_START_SEND);
+    for (int i = 1; i < buf_size - 1; i += 2) {
+        g_assert_cmphex(buf[i], ==, REMOTE_I2C_SEND);
+        g_assert_cmphex(buf[i + 1], ==, msg[j++]);
+    }
+    g_assert_cmphex(buf[buf_size - 1], ==, REMOTE_I2C_FINISH);
+
+    g_free(buf);
+}
+
+static void test_remote_i2c_recv(void *obj, void *data,
+                                 QGuestAllocator *t_alloc)
+{
+    QI2CDevice *i2cdev = (QI2CDevice *)obj;
+    int *sock = (int *)data;
+    int fd = setup_fd(sock);
+
+    uint8_t msg[] = {0x47, 0x48, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x4E, 0x4F};
+
+    test_recv(i2cdev, fd, msg, 1);
+    test_recv(i2cdev, fd, msg, 2);
+    test_recv(i2cdev, fd, msg, 3);
+    test_recv(i2cdev, fd, msg, 4);
+    test_recv(i2cdev, fd, msg, 5);
+    test_recv(i2cdev, fd, msg, 6);
+    test_recv(i2cdev, fd, msg, 7);
+    test_recv(i2cdev, fd, msg, 8);
+    test_recv(i2cdev, fd, msg, 9);
+}
+
+static void test_remote_i2c_send(void *obj, void *data,
+                                 QGuestAllocator *t_alloc)
+{
+    QI2CDevice *i2cdev = (QI2CDevice *)obj;
+    int *sock = (int *)data;
+    int fd = setup_fd(sock);
+
+    uint8_t msg[] = {0x47, 0x48, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x4E, 0x4F};
+
+    test_send(i2cdev, fd, msg, 1);
+    test_send(i2cdev, fd, msg, 2);
+    test_send(i2cdev, fd, msg, 3);
+    test_send(i2cdev, fd, msg, 4);
+    test_send(i2cdev, fd, msg, 5);
+    test_send(i2cdev, fd, msg, 6);
+    test_send(i2cdev, fd, msg, 7);
+    test_send(i2cdev, fd, msg, 8);
+    test_send(i2cdev, fd, msg, 9);
+}
+
+static in_port_t open_socket(int *sock)
+{
+    struct sockaddr_in myaddr;
+    socklen_t addrlen;
+
+    myaddr.sin_family = AF_INET;
+    myaddr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+    myaddr.sin_port = 0;
+
+    *sock = socket(AF_INET, SOCK_STREAM, 0);
+    g_assert(*sock != -1);
+    g_assert(bind(*sock, (struct sockaddr *)&myaddr, sizeof(myaddr)) != -1);
+
+    addrlen = sizeof(myaddr);
+    g_assert(getsockname(*sock, (struct sockaddr *)&myaddr, &addrlen) != -1);
+    g_assert(listen(*sock, 1) != -1);
+
+    return ntohs(myaddr.sin_port);
+}
+
+static void remote_i2c_test_cleanup(void *socket)
+{
+    int *s = socket;
+
+    close(*s);
+    qos_invalidate_command_line();
+    g_free(s);
+}
+
+static void *remote_i2c_test_setup(GString *cmd_line, void *arg)
+{
+    int *sock = g_new(int, 1);
+
+    g_string_append_printf(cmd_line, QEMU_CMD_CHR, open_socket(sock));
+    g_test_queue_destroy(remote_i2c_test_cleanup, sock);
+    return sock;
+}
+
+static void register_remote_i2c_test(void)
+{
+    QOSGraphEdgeOptions edge = {
+        .extra_device_opts = "id=" TEST_ID ",address=0x62,chardev=i2c-chardev"};
+    add_qi2c_address(&edge, &(QI2CAddress){TEST_ADDR});
+
+    qos_node_create_driver("remote-i2c", i2c_device_create);
+    qos_node_consumes("remote-i2c", "i2c-bus", &edge);
+
+    QOSGraphTestOptions opts = {
+        .before = remote_i2c_test_setup,
+    };
+    qemu_add_opts(&qemu_chardev_opts);
+    qos_add_test("test_remote_i2c_recv", "remote-i2c", test_remote_i2c_recv,
+                 &opts);
+    qos_add_test("test_remote_i2c_send", "remote-i2c", test_remote_i2c_send,
+                 &opts);
+}
+libqos_init(register_remote_i2c_test);
-- 
2.32.0.605.g8dce9f2422-goog

[PATCH v4 2/2] docs/specs: add remote i2c docs

[Shengtan Mao]

Change-Id: I4ef1e31c326dbb4f741bf65d9212ff10fc3c98c3
---
 docs/specs/index.rst      |  1 +
 docs/specs/remote-i2c.rst | 51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)
 create mode 100644 docs/specs/remote-i2c.rst

diff --git a/docs/specs/index.rst b/docs/specs/index.rst
index b7b08ea30d..ac496fb8b8 100644
--- a/docs/specs/index.rst
+++ b/docs/specs/index.rst
@@ -13,3 +13,4 @@ guest hardware that is specific to QEMU.
    acpi_hw_reduced_hotplug
    tpm
    acpi_hest_ghes
+   remote-i2c
diff --git a/docs/specs/remote-i2c.rst b/docs/specs/remote-i2c.rst
new file mode 100644
index 0000000000..7f3b1e46bb
--- /dev/null
+++ b/docs/specs/remote-i2c.rst
@@ -0,0 +1,51 @@
+=================
+Remote I2C Device
+=================
+
+The remote I2C device is connected directly to the I2C controller inside QEMU,
+and the external I2C device is outside of QEMU. The communication between the
+external and remote I2C devices is done through the character device provided
+by QEMU and follows the remote I2C protocol.
+
+Remote I2C Protocol
+===================
+The remote I2C device implements three functions of the struct I2CSlaveClass:
+
+* event
+* recv
+* send
+
+Exactly one byte is written or read from the character device at a time,
+so these functions may read/write to the character device multiple times.
+Each byte may be a command or a data byte. The command are outlined
+in enum RemoteI2CCommand. The protocol describes the expected behavior
+of the external I2C device in response to the the commands.
+
+event
+=====
+A subset of the RemoteI2CCommand corresponds exactly to the enum i2c_event.
+They are:
+
+* REMOTE_I2C_START_RECV
+* REMOTE_I2C_START_SEND
+* REMOTE_I2C_FINISH
+* REMOTE_I2C_NACK
+
+The event function of remote I2C writes the command to the external I2C device.
+The external device should call its event function to process the command as
+an event and write back the return value to remote I2C. This value is then
+returned by the event function of remote I2C.
+
+recv
+====
+The recv function of remote I2C writes the RemoteI2CCommand REMOTE_I2C_RECV to
+the external I2C device. The external device should call its recv function
+and write back the return value to remote I2C. This value is then returned by
+the recv function of remote I2C.
+
+send
+====
+The send function of remote I2C writes the RemoteI2CCommand REMOTE_I2C_SEND
+followed by the data to the external I2C device. The external device should
+call its send function to process the data and write the return value back to
+remote I2C. This value is then returned by the send function of remote I2C.
-- 
2.32.0.605.g8dce9f2422-goog

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

[Corey Minyard]

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
> 

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

[Shengtan Mao]

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

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.

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 am leaving google, so I will be passing this project to my team. Thank
you again for your feedback.

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

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

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