[Stratos-dev] Re: [PATCH V5 1/6] libxl: Add support for Virtio I2C device

Hi Viresh,

On Mon, Aug 22, 2022 at 02:45:13PM +0530, Viresh Kumar wrote:

This patch adds basic support for configuring and assisting virtio-mmio based virtio-i2c backend (emualator) which is intended to run out of Qemu and could be run in any domain.

"to run out of Qemu" -> this is ambiguous. It can both mean that virtio-i2c is provided by QEMU and also that virtio-i2c is provided by a different piece of software.

An example of domain configuration for Virtio I2c:

I believe a proper spelling is "I2C", not "I2c" which looks weird to me.

i2c = [ "" ]

Is this doing something meaningful (with the whole series applied)?

Please note, this patch is not enough for virtio-i2c to work on Xen (Arm), as for every Virtio device we need to allocate Virtio MMIO params (IRQ and memory region) and pass them to the backend, also update Guest device-tree. A subsequent patch will add these missing bits. For the current patch, the default "irq" and "base" are just written to the Xenstore.

Is having irq/base set in a different patch still useful? While it was probably useful to do this way on the virtio-disk series, it doesn't seems useful anymore.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

tools/golang/xenlight/helpers.gen.go | 108 +++++++++++ tools/golang/xenlight/types.gen.go | 27 +++

Those .gen.go files aren't up-to-date. You could just add a note after a '---' line to tell the committer to regenerate them, or make sure to regenerate them before a new submission.

tools/include/libxl.h | 32 +++ tools/include/libxl_utils.h | 3 + tools/libs/light/Makefile | 1 + tools/libs/light/libxl_create.c | 13 ++ tools/libs/light/libxl_dm.c | 19 +- tools/libs/light/libxl_i2c.c | 226 ++++++++++++++++++++++ tools/libs/light/libxl_internal.h | 1 + tools/libs/light/libxl_types.idl | 24 +++ tools/libs/light/libxl_types_internal.idl | 1 + tools/ocaml/libs/xl/genwrap.py | 1 + tools/ocaml/libs/xl/xenlight_stubs.c | 1 + tools/xl/Makefile | 2 +-

Could you take care of the change in `xl` in a separate patch? Also, we will want documentation, at least changes in the man pages about the new commands and configurations.

tools/xl/xl.h | 3 + tools/xl/xl_cmdtable.c | 15 ++ tools/xl/xl_i2c.c | 142 ++++++++++++++ tools/xl/xl_parse.c | 80 ++++++++ tools/xl/xl_parse.h | 1 + tools/xl/xl_sxp.c | 2 + 20 files changed, 699 insertions(+), 3 deletions(-) create mode 100644 tools/libs/light/libxl_i2c.c create mode 100644 tools/xl/xl_i2c.c

diff --git a/tools/include/libxl.h b/tools/include/libxl.h index 2321a648a59a..ab18c0b8c794 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -829,6 +829,15 @@ typedef struct libxl__ctx libxl_ctx; */ #define LIBXL_HAVE_BUILDINFO_VKB_DEVICE 1 +/*

• • LIBXL_HAVE_BUILDINFO_I2C_DEVICE
• • If this is defined, then the libxl_domain_build_info structure will
• • contain a boolean hvm.i2c_device which instructs libxl whether to include
• • a i2c at build time or not.

"at built time" ? Maybe "at guest creation time" would be a bit more descriptive, or "domain build time".

Could you add that "i2cs" is available in "libxl_domain_config" as well? Or maybe a more generic description and macro name speaking about the availability of I2C.

• */

+#define LIBXL_HAVE_BUILDINFO_I2C_DEVICE 1

Also, can this be put at the end of one of the LIBXL_HAVE_* list? Around line 1444, just before typedef **libxl_string_list would probably be better.

/*

• LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST

diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c index b9dd2deedf13..84fe9f80c8fe 100644 --- a/tools/libs/light/libxl_create.c +++ b/tools/libs/light/libxl_create.c @@ -375,6 +375,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, libxl_defbool_setdefault(&b_info->u.hvm.altp2m, false); libxl_defbool_setdefault(&b_info->u.hvm.usb, false); libxl_defbool_setdefault(&b_info->u.hvm.vkb_device, true);

•    libxl_defbool_setdefault(&b_info->u.hvm.i2c_device,         true);
     libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
  

libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false); @@ -1753,6 +1754,7 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, libxl__device_console console; libxl__device device; libxl_device_vkb vkb;

•    libxl_device_i2c i2c;
  

init_console_info(gc, &console, 0); console.backend_domid = state->console_domid; @@ -1765,6 +1767,12 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, libxl_device_vkb_dispose(&vkb); }

•    if (libxl_defbool_val(d_config->b_info.u.hvm.i2c_device)) {
  

•        libxl_device_i2c_init(&i2c);
  

•        libxl__device_add(gc, domid, &libxl__i2c_devtype, &i2c);
  

•        libxl_device_i2c_dispose(&i2c);
  

So, every HVM guest are going to get an I2C device? I don't think that make sense, especially on x86, or even on Arm.

•    }
  

•    dcs->sdss.dm.guest_domid = domid;
     if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
         libxl__spawn_stub_dm(egc, &dcs->sdss);
  

@@ -1797,6 +1805,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, &d_config->vkbs[i]); }

•    for (i = 0; i < d_config->num_i2cs; i++) {
  

•        libxl__device_add(gc, domid, &libxl__i2c_devtype,
  

•                          &d_config->i2cs[i]);
  

•    }
  

•    if (d_config->b_info.arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) {
         init_console_info(gc, &vuart, 0);
         vuart.backend_domid = state->console_domid;
  

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c index fc264a3a13a6..362c0596f497 100644 --- a/tools/libs/light/libxl_dm.c +++ b/tools/libs/light/libxl_dm.c @@ -2112,7 +2112,8 @@ static void libxl__dm_vifs_from_hvm_guest_config(libxl__gc *gc, static int libxl__vfb_and_vkb_from_hvm_guest_config(libxl__gc *gc, const libxl_domain_config *guest_config, libxl_device_vfb *vfb,

•                                    libxl_device_vkb *vkb)
  

•                                    libxl_device_vkb *vkb,
  

•                                    libxl_device_i2c *i2c)
  

That function is about taking care of adding Xen PV virtual framebuffer and keyboard to a guest. What is the relationship with between that and I2C?

{ const libxl_domain_build_info *b_info = &guest_config->b_info; diff --git a/tools/libs/light/libxl_i2c.c b/tools/libs/light/libxl_i2c.c new file mode 100644 index 000000000000..2c46351ce3a4 --- /dev/null +++ b/tools/libs/light/libxl_i2c.c @@ -0,0 +1,226 @@ +/*

• • Copyright (C) 2022 Linaro Ltd.
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU Lesser General Public License as published
• • by the Free Software Foundation; version 2.1 only. with the special
• • exception on linking described in file LICENSE.
• • 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 Lesser General Public License for more details.
• */

+#include "libxl_internal.h"

+static int libxl__device_i2c_setdefault(libxl__gc *gc, uint32_t domid,

•                                    libxl_device_i2c *i2c, bool hotplug)
  

+{

• if (i2c->backend_type != LIBXL_I2C_BACKEND_VIRTIO) {

•    i2c->backend_type = LIBXL_I2C_BACKEND_VIRTIO;
  

• }
• return libxl__resolve_domid(gc, i2c->backend_domname, &i2c->backend_domid);

+}

+static int libxl__device_i2c_dm_needed(void *e, uint32_t domid) +{

• libxl_device_i2c *elem = e;
• return elem->backend_type == LIBXL_I2C_BACKEND_VIRTIO;

I'm not sure I understand the result of this function. Is QEMU needed to present VirtIO I2C devices to a guest?

diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index d634f304cda2..014a3ea8364c 100644 --- a/tools/libs/light/libxl_types.idl +++ b/tools/libs/light/libxl_types.idl @@ -278,6 +278,10 @@ libxl_vkb_backend = Enumeration("vkb_backend", [ (2, "LINUX") ]) +libxl_i2c_backend = Enumeration("i2c_backend", [

• (0, "VIRTIO")

Could you start with 1 rather than 0? This will allow libxl to find out if the backend have been set or if an application let libxl choose which backend is more appropriate.

• ])

libxl_passthrough = Enumeration("passthrough", [ (0, "default"), (1, "disabled"), @@ -626,6 +630,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ # - "mouse" for PS/2 protocol relative mouse ("usbdevice", string), ("vkb_device", libxl_defbool),

•                                   ("i2c_device",       libxl_defbool),
  

How is "i2c_device" useful? Would "i2cs" in libxl_domain_config be enough?

                                    ("soundhw",          string),
                                    ("xen_platform_pci", libxl_defbool),
                                    ("usbdevice_list",   libxl_string_list),

diff --git a/tools/xl/Makefile b/tools/xl/Makefile index b7f439121a3a..06801962f11e 100644 --- a/tools/xl/Makefile +++ b/tools/xl/Makefile @@ -23,7 +23,7 @@ XL_OBJS += xl_vtpm.o xl_block.o xl_nic.o xl_usb.o XL_OBJS += xl_sched.o xl_pci.o xl_vcpu.o xl_cdrom.o xl_mem.o XL_OBJS += xl_info.o xl_console.o xl_misc.o XL_OBJS += xl_vmcontrol.o xl_saverestore.o xl_migrate.o -XL_OBJS += xl_vdispl.o xl_vsnd.o xl_vkb.o +XL_OBJS += xl_vdispl.o xl_vsnd.o xl_vkb.o xl_i2c.o

Could you add xl_i2c.o in a new line instead?

$(XL_OBJS): CFLAGS += $(CFLAGS_libxentoollog) $(XL_OBJS): CFLAGS += $(CFLAGS_XL) diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c index 35182ca19630..c409ab1578d4 100644 --- a/tools/xl/xl_cmdtable.c +++ b/tools/xl/xl_cmdtable.c @@ -406,6 +406,21 @@ const struct cmd_spec cmd_table[] = { "Destroy a domain's virtual keyboard device", "<Domain> <DevId>", },

• { "i2c-attach",

•  &main_i2cattach, 1, 1,
  

•  "Create a new virtual i2c device",
  

•  "<Domain> <i2c-spec-component(s)>...",
  

Is i2c-attach going to work? It seems that an i2c device is presented to the guest via the device tree, but I don't think it can be modified on the fly once the guest have booted. Is there i2c device hotplug?

diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 1b5381cef033..b91ffba14d40 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -2583,6 +2657,7 @@ void parse_config_data(const char *config_source, if (vnc_enabled) { libxl_device_vfb *vfb; libxl_device_vkb *vkb;

•        libxl_device_i2c *i2c;
  

How is I2C related to VNC?

vfb = ARRAY_EXTEND_INIT(d_config->vfbs, d_config->num_vfbs, libxl_device_vfb_init); diff --git a/tools/xl/xl_sxp.c b/tools/xl/xl_sxp.c index 359a0015709e..a44c765aa515 100644 --- a/tools/xl/xl_sxp.c +++ b/tools/xl/xl_sxp.c @@ -140,6 +140,8 @@ void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh) fprintf(fh, "\t\t\t(usbdevice %s)\n", b_info->u.hvm.usbdevice); fprintf(fh, "\t\t\t(vkb_device %s)\n", libxl_defbool_to_string(b_info->u.hvm.vkb_device));

•    fprintf(fh, "\t\t\t(i2c_device %s)\n",
  

•           libxl_defbool_to_string(b_info->u.hvm.i2c_device));
  

There is a notice on the printf_info_sexp() function:

/* In general you should not add new output to this function since it * is intended only for legacy use. */ void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh)

Could you explain why this function is been modified? If not, there's probably no modification needed.

     fprintf(fh, "\t\t)\n");
     break;
 case LIBXL_DOMAIN_TYPE_PV:

Thanks,