[0/5] virtio: Implement generic vhost-user-i2c backend

Message

Viresh Kumar March 24, 2021, 7:33 a.m. UTC

Hello,

This is an initial implementation of a generic vhost-user backend for
the I2C bus. This is based of the virtio specifications (already merged)
for the I2C bus.

The kernel virtio I2C driver is still under review, here is the latest
version (v10):

  https://lore.kernel.org/lkml/[e-mail]/

The backend is implemented as a vhost-user device because we want to
experiment in making portable backends that can be used with multiple
hypervisors.  We also want to support backends isolated in their own
separate service VMs with limited memory cross-sections with the
principle guest. This is part of a wider initiative by Linaro called
"project Stratos" for which you can find information here:

  https://collaborate.linaro.org/display/STR/Stratos+Home

I mentioned this to explain the decision to write the daemon as a fairly
pure glib application that just depends on libvhost-user.

We are not sure where the vhost-user backend should get queued, qemu or
a separate repository. Similar questions were raised by an earlier
thread by Alex Bennée for his RPMB work:

  https://lore.kernel.org/qemu-devel/[e-mail]/


Testing:
-------

I didn't have access to a real hardware where I can play with a I2C
client device (like RTC, eeprom, etc) to verify the working of the
backend daemon, so I decided to test it on my x86 box itself with
hierarchy of two ARM64 guests.

The first ARM64 guest was passed "-device ds1338,address=0x20" option,
so it could emulate a ds1338 RTC device, which connects to an I2C bus.
Once the guest came up, ds1338 device instance was created within the
guest kernel by doing:

  echo ds1338 0x20 > /sys/bus/i2c/devices/i2c-0/new_device

[
  Note that this may end up binding the ds1338 device to its driver,
  which won't let our i2c daemon talk to the device. For that we need to
  manually unbind the device from the driver:

  echo 0-0020 > /sys/bus/i2c/devices/0-0020/driver/unbind
]

After this is done, you will get /dev/rtc1. This is the device we wanted
to emulate, which will be accessed by the vhost-user-i2c backend daemon
via the /dev/i2c-0 file present in the guest VM.

At this point we need to start the backend daemon and give it a
socket-path to talk to from qemu (you can pass -v to it to get more
detailed messages):

  vhost-user-i2c --socket-path=vi2c.sock --device-list 0:20

[ Here, 0:20 is the bus/device mapping, 0 for /dev/i2c-0 and 20 is
client address of ds1338 that we used while creating the device. ]

Now we need to start the second level ARM64 guest (from within the first
guest) to get the i2c-virtio.c Linux driver up. The second level guest
is passed the following options to connect to the same socket:

  -chardev socket,path=vi2c.sock,id=vi2c \
  -device vhost-user-i2c-pci,chardev=vi2c,id=i2c

Once the second level guest boots up, we will see the i2c-virtio bus at
/sys/bus/i2c/devices/i2c-X/. From there we can now make it emulate the
ds1338 device again by doing:

  echo ds1338 0x20 > /sys/bus/i2c/devices/i2c-0/new_device

[ This time we want ds1338's driver to be bound to the device, so it
should be enabled in the kernel as well. ]

And we will get /dev/rtc1 device again here in the second level guest.
Now we can play with the rtc device with help of hwclock utility and we
can see the following sequence of transfers happening if we try to
update rtc's time from system time.

hwclock -w -f /dev/rtc1 (in guest2) ->
  Reaches i2c-virtio.c (Linux bus driver in guest2) ->
    transfer over virtio ->
      Reaches the qemu's vhost-i2c device emulation (running over guest1) ->
        Reaches the backend daemon vhost-user-i2c started earlier (in guest1) ->
          ioctl(/dev/i2c-0, I2C_RDWR, ..); (in guest1) ->
	    reaches qemu's hw/rtc/ds1338.c (running over host)


I hope I was able to give a clear picture of my test setup here :)

Thanks.

Viresh Kumar (5):
  hw/virtio: add boilerplate for vhost-user-i2c device
  hw/virtio: add vhost-user-i2c-pci boilerplate
  tools/vhost-user-i2c: Add backend driver
  docs: add a man page for vhost-user-i2c
  MAINTAINERS: Add entry for virtio-i2c

 MAINTAINERS                                 |   9 +
 docs/tools/index.rst                        |   1 +
 docs/tools/vhost-user-i2c.rst               |  75 +++
 hw/virtio/Kconfig                           |   5 +
 hw/virtio/meson.build                       |   2 +
 hw/virtio/vhost-user-i2c-pci.c              |  79 +++
 hw/virtio/vhost-user-i2c.c                  | 286 +++++++++
 include/hw/virtio/vhost-user-i2c.h          |  37 ++
 include/standard-headers/linux/virtio_ids.h |   1 +
 tools/meson.build                           |   8 +
 tools/vhost-user-i2c/50-qemu-i2c.json.in    |   5 +
 tools/vhost-user-i2c/main.c                 | 652 ++++++++++++++++++++
 tools/vhost-user-i2c/meson.build            |  10 +
 13 files changed, 1170 insertions(+)
 create mode 100644 docs/tools/vhost-user-i2c.rst
 create mode 100644 hw/virtio/vhost-user-i2c-pci.c
 create mode 100644 hw/virtio/vhost-user-i2c.c
 create mode 100644 include/hw/virtio/vhost-user-i2c.h
 create mode 100644 tools/vhost-user-i2c/50-qemu-i2c.json.in
 create mode 100644 tools/vhost-user-i2c/main.c
 create mode 100644 tools/vhost-user-i2c/meson.build

Comments

[e-mail] March 24, 2021, 7:42 a.m. UTC | #1

Patchew URL: https://patchew.org/QEMU/[e-mail]/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: [e-mail]
Subject: [PATCH 0/5] virtio: Implement generic vhost-user-i2c backend

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/[e-mail] -> patchew/[e-mail]
Switched to a new branch 'test'
40ccc27 MAINTAINERS: Add entry for virtio-i2c
9b632f4 docs: add a man page for vhost-user-i2c
5da2cca tools/vhost-user-i2c: Add backend driver
6716f9a hw/virtio: add vhost-user-i2c-pci boilerplate
d6439eb hw/virtio: add boilerplate for vhost-user-i2c device

=== OUTPUT BEGIN ===
1/5 Checking commit d6439ebcaa1d (hw/virtio: add boilerplate for vhost-user-i2c device)
Use of uninitialized value $acpi_testexpected in string eq at ./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#46: 
new file mode 100644

total: 0 errors, 1 warnings, 344 lines checked

Patch 1/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/5 Checking commit 6716f9a6b4c1 (hw/virtio: add vhost-user-i2c-pci boilerplate)
Use of uninitialized value $acpi_testexpected in string eq at ./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#31: 
new file mode 100644

total: 0 errors, 1 warnings, 86 lines checked

Patch 2/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/5 Checking commit 5da2ccae459c (tools/vhost-user-i2c: Add backend driver)
Use of uninitialized value $acpi_testexpected in string eq at ./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#52: 
new file mode 100644

ERROR: spaces required around that '*' (ctx:WxV)
#137: FILE: tools/vhost-user-i2c/main.c:70:
+        const typeof(((type *) 0)->member) *__mptr = (ptr);     \
                                            ^

ERROR: space required after that ';' (ctx:VxV)
#138: FILE: tools/vhost-user-i2c/main.c:71:
+        (type *) ((char *) __mptr - offsetof(type, member));})
                                                            ^

ERROR: line over 90 characters
#163: FILE: tools/vhost-user-i2c/main.c:96:
+    { "socket-path", 's', 0, G_OPTION_ARG_FILENAME, &socket_path, "Location of vhost-user Unix domain socket, incompatible with --fd", "PATH" },

ERROR: line over 90 characters
#164: FILE: tools/vhost-user-i2c/main.c:97:
+    { "fd", 'f', 0, G_OPTION_ARG_INT, &socket_fd, "Specify the file-descriptor of the backend, incompatible with --socket-path", "FD" },

ERROR: line over 90 characters
#165: FILE: tools/vhost-user-i2c/main.c:98:
+    { "device-list", 'l', 0, G_OPTION_ARG_STRING, &device_list, "List of i2c-dev bus and attached devices", "I2C Devices" },

ERROR: line over 90 characters
#166: FILE: tools/vhost-user-i2c/main.c:99:
+    { "print-capabilities", 'c', 0, G_OPTION_ARG_NONE, &print_cap, "Output to stdout the backend capabilities in JSON format and exit", NULL},

WARNING: line over 80 characters
#167: FILE: tools/vhost-user-i2c/main.c:100:
+    { "verbose", 'v', 0, G_OPTION_ARG_NONE, &verbose, "Be more verbose in output", NULL},

WARNING: line over 80 characters
#209: FILE: tools/vhost-user-i2c/main.c:142:
+     * Adapter        | adapter2 | none  | adapter1 | adapter3 | none | none| (val)

WARNING: line over 80 characters
#211: FILE: tools/vhost-user-i2c/main.c:144:
+     * Slave Address  | addr 1   | none  | addr 2   | addr 3   | none | none| (idx)

ERROR: do not use assignment in if condition
#241: FILE: tools/vhost-user-i2c/main.c:174:
+    if (addr < MAX_I2C_VDEV && ((idx = i2c->adapter_map[addr]) != 0)) {

ERROR: braces {} are necessary for all arms of this statement
#288: FILE: tools/vhost-user-i2c/main.c:221:
+    if (bus < 0)
[...]

WARNING: line over 80 characters
#316: FILE: tools/vhost-user-i2c/main.c:249:
+                g_printerr("client addr 0x%x repeat, not allowed.\n", client_addr[i]);

ERROR: code indent should never use tabs
#340: FILE: tools/vhost-user-i2c/main.c:273:
+ * ^Ie.g. 2 for /dev/i2c-2$

ERROR: code indent should never use tabs
#342: FILE: tools/vhost-user-i2c/main.c:275:
+ * ^Ie.g. 0x1C or 1C$

ERROR: spaces required around that '==' (ctx:WxV)
#359: FILE: tools/vhost-user-i2c/main.c:292:
+        if (!cp || *cp =='\0') {
                        ^

ERROR: unnecessary whitespace before a quoted newline
#364: FILE: tools/vhost-user-i2c/main.c:297:
+            g_printerr("too many adapter (%d), only support %d \n", n_adapter,

ERROR: spaces required around that '!=' (ctx:WxV)
#378: FILE: tools/vhost-user-i2c/main.c:311:
+        while (cp != NULL && *cp !='\0') {
                                  ^

ERROR: braces {} are necessary for all arms of this statement
#379: FILE: tools/vhost-user-i2c/main.c:312:
+            if (*cp == ':')
[...]

ERROR: unnecessary whitespace before a quoted newline
#383: FILE: tools/vhost-user-i2c/main.c:316:
+                g_printerr("too many devices (%d), only support %d \n",

WARNING: line over 80 characters
#388: FILE: tools/vhost-user-i2c/main.c:321:
+            if (qemu_strtol(cp, &t, 16, &addr) || addr < 0 || addr > MAX_I2C_VDEV) {

WARNING: line over 80 characters
#400: FILE: tools/vhost-user-i2c/main.c:333:
+        i2c->adapter[n_adapter] = vi2c_create_adapter(bus, client_addr, n_client);

ERROR: braces {} are necessary for all arms of this statement
#401: FILE: tools/vhost-user-i2c/main.c:334:
+        if (!i2c->adapter[n_adapter])
[...]

WARNING: line over 80 characters
#438: FILE: tools/vhost-user-i2c/main.c:371:
+        g_printerr("Failed to transfer data to address %x : %d\n", msg->addr, errno);

ERROR: that open brace { should be on the previous line
#632: FILE: tools/vhost-user-i2c/main.c:565:
+    if (!g_option_context_parse(context, &argc, &argv, &error))
+    {

WARNING: line over 80 characters
#653: FILE: tools/vhost-user-i2c/main.c:586:
+                          G_LOG_LEVEL_WARNING | G_LOG_LEVEL_CRITICAL | G_LOG_LEVEL_ERROR,

ERROR: line over 90 characters
#664: FILE: tools/vhost-user-i2c/main.c:597:
+        g_autoptr(GSocket) bind_socket = g_socket_new(G_SOCKET_FAMILY_UNIX, G_SOCKET_TYPE_STREAM,

WARNING: line over 80 characters
#665: FILE: tools/vhost-user-i2c/main.c:598:
+                                                      G_SOCKET_PROTOCOL_DEFAULT, &error);

total: 18 errors, 10 warnings, 686 lines checked

Patch 3/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/5 Checking commit 9b632f47c605 (docs: add a man page for vhost-user-i2c)
Use of uninitialized value $acpi_testexpected in string eq at ./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#26: 
new file mode 100644

total: 0 errors, 1 warnings, 79 lines checked

Patch 4/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/5 Checking commit 40ccc271213e (MAINTAINERS: Add entry for virtio-i2c)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/[e-mail]/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to [e-mail]

Viresh Kumar March 24, 2021, 11:05 a.m. UTC | #2

On 24-03-21, 00:42, [e-mail] wrote:
> Patchew URL: https://patchew.org/QEMU/[e-mail]/
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..

I missed the fact that we have an implementation of checkpatch here as well. I
have updated the patches already after running checkpatch at my end now, and
will fix them while sending V2.