[PATCH V7 0/3] toolstack support for generic virtio devices on Arm

[PATCH V7 0/3] toolstack support for generic virtio devices on Arm

[Viresh Kumar]

Hello,

This patchset adds toolstack support for I2C, GPIO and generic virtio devices.
This is inspired from the work done by Oleksandr for the Disk device.

This is developed as part of Linaro's Project Stratos, where we are working
towards Hypervisor agnostic Rust based backends [1].

This is based of Julien's for-next/4.18 branch [2].

V6->V7:
- Support generic virtio devices too. They are passed with type=virtio,device,
  and we only create the MMIO DT node for them.
- Add links to DT bindings of I2C and GPIO, in code and documentation.
- Call libxl__device_add() for all hypervisor types.
- Add (0, "UNKNOWN") for libxl_virtio_transport.
- Removed libxl_virtio_backend and libxl_virtioinfo, as they were unused.
- Remove unnecessary stuff from libxl__virtio_from_xenstore() and add support
  for type and transport.
- Add backend=domid in documentation and replace compatible with type.
- Improved commit logs.

V5->V6:
- The cleanup patches are sent separately [2].
- We don't add I2C or GPIO specific device changes anymore, rather we create
  generic "virtio" devices. The "type" of a virtio devices helps us identify the
  right device, and create an entry in the DT node. The same can be used for all
  Virtio devices now.
- Update man page xl.cfg.

V4->V5:
- Fixed indentation at few places.
- Removed/added blank lines.
- Added few comments.
- Added review tags from Oleksandr.
- Rebased over latest staging branch.

V3->V4:
- Update virtio_enabled independently of all devices, so we don't miss setting
  it to true.

- Add iommu handling for i2c/gpio and move it as part of
  make_virtio_mmio_node_common(), which gets backend_domid parameter as a
  result.

V2->V3:
- Rebased over latest tree and made changes according to changes in Oleksandr's
  patches from sometime back.
- Minor cleanups.

V1->V2:
- Patches 3/6 and 4/6 are new.
- Patches 5/6 and 6/6 updated based on the above two patches.
- Added link to the bindings for I2C and GPIO.
- Rebased over latest master branch.


Thanks.

--
Viresh

[1] https://lore.kernel.org/xen-devel/20220414092358.kepxbmnrtycz7mhe@vireshk-i7/
[2] https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/for-next/4.18

Viresh Kumar (3):
  libxl: Add support for generic virtio device
  xl: Add support to parse generic virtio device
  docs: Add documentation for generic virtio devices

 docs/man/xl.cfg.5.pod.in                  |  28 +++++
 tools/libs/light/Makefile                 |   1 +
 tools/libs/light/libxl_arm.c              | 103 ++++++++++++++++
 tools/libs/light/libxl_create.c           |   4 +
 tools/libs/light/libxl_internal.h         |   1 +
 tools/libs/light/libxl_types.idl          |  18 +++
 tools/libs/light/libxl_types_internal.idl |   1 +
 tools/libs/light/libxl_virtio.c           | 144 ++++++++++++++++++++++
 tools/ocaml/libs/xl/genwrap.py            |   1 +
 tools/ocaml/libs/xl/xenlight_stubs.c      |   1 +
 tools/xl/xl_parse.c                       |  80 ++++++++++++
 11 files changed, 382 insertions(+)
 create mode 100644 tools/libs/light/libxl_virtio.c

-- 
2.31.1.272.g89b43f80a514

[PATCH V7 1/3] libxl: Add support for generic virtio device

[Viresh Kumar]

This patch adds basic support for configuring and assisting generic
Virtio backends, which could run in any domain.

An example of domain configuration for mmio based Virtio I2C device is:
virtio = ["type=virtio,device22,transport=mmio"]

To make this work on Arm, allocate Virtio MMIO params (IRQ and memory
region) and pass them to the backend and update guest device-tree to
create a DT node for the Virtio devices.

Add special support for I2C and GPIO devices, which require the
"compatible" DT property to be set, among other device specific
properties. Support for generic virtio devices is also added, which just
need a MMIO node but not any special DT properties, for such devices the
user needs to pass "virtio,devices" in the "type" string.

The parsing of generic virtio device configurations will be done in a
separate commit.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 tools/libs/light/Makefile                 |   1 +
 tools/libs/light/libxl_arm.c              | 103 ++++++++++++++++
 tools/libs/light/libxl_create.c           |   4 +
 tools/libs/light/libxl_internal.h         |   1 +
 tools/libs/light/libxl_types.idl          |  18 +++
 tools/libs/light/libxl_types_internal.idl |   1 +
 tools/libs/light/libxl_virtio.c           | 144 ++++++++++++++++++++++
 7 files changed, 272 insertions(+)
 create mode 100644 tools/libs/light/libxl_virtio.c

diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
index 374be1cfab25..4fddcc6f51d7 100644
--- a/tools/libs/light/Makefile
+++ b/tools/libs/light/Makefile
@@ -106,6 +106,7 @@ OBJS-y += libxl_vdispl.o
 OBJS-y += libxl_pvcalls.o
 OBJS-y += libxl_vsnd.o
 OBJS-y += libxl_vkb.o
+OBJS-y += libxl_virtio.o
 OBJS-y += libxl_genid.o
 OBJS-y += _libxl_types.o
 OBJS-y += libxl_flask.o
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index fa3d61f1e882..ab3668b3b8a3 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -113,6 +113,19 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
         }
     }
 
+    for (i = 0; i < d_config->num_virtios; i++) {
+        libxl_device_virtio *virtio = &d_config->virtios[i];
+
+        if (virtio->transport != LIBXL_VIRTIO_TRANSPORT_MMIO)
+            continue;
+
+        rc = alloc_virtio_mmio_params(gc, &virtio->base, &virtio->irq,
+                                      &virtio_mmio_base, &virtio_mmio_irq);
+
+        if (rc)
+            return rc;
+    }
+
     /*
      * Every virtio-mmio device uses one emulated SPI. If Virtio devices are
      * present, make sure that we allocate enough SPIs for them.
@@ -956,6 +969,82 @@ static int make_virtio_mmio_node(libxl__gc *gc, void *fdt, uint64_t base,
     return fdt_end_node(fdt);
 }
 
+/*
+ * The DT bindings for GPIO device are present here:
+ *
+ * https://www.kernel.org/doc/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml
+ */
+static int make_virtio_mmio_node_i2c(libxl__gc *gc, void *fdt)
+{
+    int res;
+
+    res = fdt_begin_node(fdt, "i2c");
+    if (res) return res;
+
+    res = fdt_property_compat(gc, fdt, 1, "virtio,device22");
+    if (res) return res;
+
+    return fdt_end_node(fdt);
+}
+
+/*
+ * The DT bindings for GPIO device are present here:
+ *
+ * https://www.kernel.org/doc/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml
+ */
+static int make_virtio_mmio_node_gpio(libxl__gc *gc, void *fdt)
+{
+    int res;
+
+    res = fdt_begin_node(fdt, "gpio");
+    if (res) return res;
+
+    res = fdt_property_compat(gc, fdt, 1, "virtio,device29");
+    if (res) return res;
+
+    res = fdt_property(fdt, "gpio-controller", NULL, 0);
+    if (res) return res;
+
+    res = fdt_property_cell(fdt, "#gpio-cells", 2);
+    if (res) return res;
+
+    res = fdt_property(fdt, "interrupt-controller", NULL, 0);
+    if (res) return res;
+
+    res = fdt_property_cell(fdt, "#interrupt-cells", 2);
+    if (res) return res;
+
+    return fdt_end_node(fdt);
+}
+
+static int make_virtio_mmio_node_device(libxl__gc *gc, void *fdt, uint64_t base,
+                                        uint32_t irq, const char *type,
+                                        uint32_t backend_domid)
+{
+    int res, len = strlen(type);
+
+    res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid);
+    if (res) return res;
+
+    /* Add device specific nodes */
+    if (!strncmp(type, "virtio,device22", len)) {
+        res = make_virtio_mmio_node_i2c(gc, fdt);
+        if (res) return res;
+    } else if (!strncmp(type, "virtio,device29", len)) {
+        res = make_virtio_mmio_node_gpio(gc, fdt);
+        if (res) return res;
+    } else if (!strncmp(type, "virtio,device", len)) {
+        /* Generic Virtio Device */
+        res = fdt_end_node(fdt);
+        if (res) return res;
+    } else {
+        LOG(ERROR, "Invalid type for virtio device: %s", type);
+        return -EINVAL;
+    }
+
+    return fdt_end_node(fdt);
+}
+
 static const struct arch_info *get_arch_info(libxl__gc *gc,
                                              const struct xc_dom_image *dom)
 {
@@ -1277,6 +1366,20 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config,
             }
         }
 
+        for (i = 0; i < d_config->num_virtios; i++) {
+            libxl_device_virtio *virtio = &d_config->virtios[i];
+
+            if (virtio->transport != LIBXL_VIRTIO_TRANSPORT_MMIO)
+                continue;
+
+            if (virtio->backend_domid != LIBXL_TOOLSTACK_DOMID)
+                iommu_needed = true;
+
+            FDT( make_virtio_mmio_node_device(gc, fdt, virtio->base,
+                                              virtio->irq, virtio->type,
+                                              virtio->backend_domid) );
+        }
+
         /*
          * The iommu node should be created only once for all virtio-mmio
          * devices.
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 612eacfc7fac..beec3f6b6fec 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1752,6 +1752,10 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         libxl__device_add(gc, domid, &libxl__pvcallsif_devtype,
                           &d_config->pvcallsifs[i]);
 
+    for (i = 0; i < d_config->num_virtios; i++)
+        libxl__device_add(gc, domid, &libxl__virtio_devtype,
+                          &d_config->virtios[i]);
+
     switch (d_config->c_info.type) {
     case LIBXL_DOMAIN_TYPE_HVM:
     {
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index a7c447c10e5f..cdd155d925c1 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -4003,6 +4003,7 @@ static inline int *libxl__device_type_get_num(
 
 extern const libxl__device_type libxl__vfb_devtype;
 extern const libxl__device_type libxl__vkb_devtype;
+extern const libxl__device_type libxl__virtio_devtype;
 extern const libxl__device_type libxl__disk_devtype;
 extern const libxl__device_type libxl__nic_devtype;
 extern const libxl__device_type libxl__vtpm_devtype;
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 9e3d33cb5a59..0cfad8508dbd 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -278,6 +278,11 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
     (2, "LINUX")
     ])
 
+libxl_virtio_transport = Enumeration("virtio_transport", [
+    (0, "UNKNOWN"),
+    (1, "MMIO"),
+    ])
+
 libxl_passthrough = Enumeration("passthrough", [
     (0, "default"),
     (1, "disabled"),
@@ -703,6 +708,18 @@ libxl_device_vkb = Struct("device_vkb", [
     ("multi_touch_num_contacts", uint32)
     ])
 
+libxl_device_virtio = Struct("device_virtio", [
+    ("backend_domid", libxl_domid),
+    ("backend_domname", string),
+    ("type", string),
+    ("transport", libxl_virtio_transport),
+    ("devid", libxl_devid),
+    # Note that virtio-mmio parameters (irq and base) are for internal
+    # use by libxl and can't be modified.
+    ("irq", uint32),
+    ("base", uint64)
+    ])
+
 libxl_device_disk = Struct("device_disk", [
     ("backend_domid", libxl_domid),
     ("backend_domname", string),
@@ -980,6 +997,7 @@ libxl_domain_config = Struct("domain_config", [
     ("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")),
     ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
     ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
+    ("virtios", Array(libxl_device_virtio, "num_virtios")),
     ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
     ("p9s", Array(libxl_device_p9, "num_p9s")),
     ("pvcallsifs", Array(libxl_device_pvcallsif, "num_pvcallsifs")),
diff --git a/tools/libs/light/libxl_types_internal.idl b/tools/libs/light/libxl_types_internal.idl
index fb0f4f23d7c2..e24288f1a59e 100644
--- a/tools/libs/light/libxl_types_internal.idl
+++ b/tools/libs/light/libxl_types_internal.idl
@@ -33,6 +33,7 @@ libxl__device_kind = Enumeration("device_kind", [
     (15, "VSND"),
     (16, "VINPUT"),
     (17, "VIRTIO_DISK"),
+    (18, "VIRTIO"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [
diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
new file mode 100644
index 000000000000..64cec989c674
--- /dev/null
+++ b/tools/libs/light/libxl_virtio.c
@@ -0,0 +1,144 @@
+/*
+ * 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_virtio_setdefault(libxl__gc *gc, uint32_t domid,
+                                           libxl_device_virtio *virtio,
+                                           bool hotplug)
+{
+    return libxl__resolve_domid(gc, virtio->backend_domname,
+                                &virtio->backend_domid);
+}
+
+static int libxl__device_from_virtio(libxl__gc *gc, uint32_t domid,
+                                     libxl_device_virtio *virtio,
+                                     libxl__device *device)
+{
+    device->backend_devid   = virtio->devid;
+    device->backend_domid   = virtio->backend_domid;
+    device->devid           = virtio->devid;
+    device->domid           = domid;
+
+    device->backend_kind    = LIBXL__DEVICE_KIND_VIRTIO;
+    device->kind            = LIBXL__DEVICE_KIND_VIRTIO;
+
+    return 0;
+}
+
+static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid,
+                                      libxl_device_virtio *virtio,
+                                      flexarray_t *back, flexarray_t *front,
+                                      flexarray_t *ro_front)
+{
+    const char *transport = libxl_virtio_transport_to_string(virtio->transport);
+
+    flexarray_append_pair(back, "irq", GCSPRINTF("%u", virtio->irq));
+    flexarray_append_pair(back, "base", GCSPRINTF("%lu", virtio->base));
+    flexarray_append_pair(back, "type", GCSPRINTF("%s", virtio->type));
+    flexarray_append_pair(back, "transport", GCSPRINTF("%s", transport));
+
+    flexarray_append_pair(front, "irq", GCSPRINTF("%u", virtio->irq));
+    flexarray_append_pair(front, "base", GCSPRINTF("%lu", virtio->base));
+    flexarray_append_pair(front, "type", GCSPRINTF("%s", virtio->type));
+    flexarray_append_pair(front, "transport", GCSPRINTF("%s", transport));
+
+    return 0;
+}
+
+static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
+                                       libxl_devid devid,
+                                       libxl_device_virtio *virtio)
+{
+    const char *be_path, *tmp = NULL;
+    int rc;
+
+    virtio->devid = devid;
+
+    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
+                                  GCSPRINTF("%s/backend", libxl_path),
+                                  &be_path);
+    if (rc) goto out;
+
+    rc = libxl__backendpath_parse_domid(gc, be_path, &virtio->backend_domid);
+    if (rc) goto out;
+
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+				GCSPRINTF("%s/irq", be_path), &tmp);
+    if (rc) goto out;
+
+    if (tmp) {
+        virtio->irq = strtoul(tmp, NULL, 0);
+    }
+
+    tmp = NULL;
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+				GCSPRINTF("%s/base", be_path), &tmp);
+    if (rc) goto out;
+
+    if (tmp) {
+        virtio->base = strtoul(tmp, NULL, 0);
+    }
+
+    tmp = NULL;
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+				GCSPRINTF("%s/transport", be_path), &tmp);
+    if (rc) goto out;
+
+    if (tmp) {
+        if (!strncmp(tmp, "mmio", strlen(tmp))) {
+            virtio->transport = LIBXL_VIRTIO_TRANSPORT_MMIO;
+        } else if (!strncmp(tmp, "unknown", strlen(tmp))) {
+            virtio->transport = LIBXL_VIRTIO_TRANSPORT_UNKNOWN;
+        } else {
+            return ERROR_INVAL;
+        }
+    }
+
+    tmp = NULL;
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+				GCSPRINTF("%s/type", be_path), &tmp);
+    if (rc) goto out;
+
+    if (tmp) {
+        if (!strncmp(tmp, "virtio,device", strlen("virtio,device"))) {
+            virtio->type = strdup(tmp);
+        } else {
+            return ERROR_INVAL;
+        }
+    }
+
+out:
+    return rc;
+}
+
+static LIBXL_DEFINE_UPDATE_DEVID(virtio)
+
+#define libxl__add_virtios NULL
+#define libxl_device_virtio_compare NULL
+
+DEFINE_DEVICE_TYPE_STRUCT(virtio, VIRTIO, virtios,
+    .set_xenstore_config = (device_set_xenstore_config_fn_t)
+                           libxl__set_xenstore_virtio,
+    .from_xenstore = (device_from_xenstore_fn_t)libxl__virtio_from_xenstore,
+    .skip_attach = 1
+);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.31.1.272.g89b43f80a514

[PATCH V7 2/3] xl: Add support to parse generic virtio device

[Viresh Kumar]

This patch adds basic support for parsing generic Virtio backend.

An example of domain configuration for mmio based Virtio I2C device is:
virtio = ["type=virtio,device22,transport=mmio"]

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 tools/ocaml/libs/xl/genwrap.py       |  1 +
 tools/ocaml/libs/xl/xenlight_stubs.c |  1 +
 tools/xl/xl_parse.c                  | 80 ++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+)

diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
index 7bf26bdcd831..b188104299b1 100644
--- a/tools/ocaml/libs/xl/genwrap.py
+++ b/tools/ocaml/libs/xl/genwrap.py
@@ -36,6 +36,7 @@ DEVICE_LIST =      [ ("list",           ["ctx", "domid", "t list"]),
 functions = { # ( name , [type1,type2,....] )
     "device_vfb":     DEVICE_FUNCTIONS,
     "device_vkb":     DEVICE_FUNCTIONS,
+    "device_virtio":     DEVICE_FUNCTIONS,
     "device_disk":    DEVICE_FUNCTIONS + DEVICE_LIST +
                       [ ("insert",         ["ctx", "t", "domid", "?async:'a", "unit", "unit"]),
                         ("of_vdev",        ["ctx", "domid", "string", "t"]),
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 45b8af61c74a..8e54f95da7c7 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -707,6 +707,7 @@ DEVICE_ADDREMOVE(disk)
 DEVICE_ADDREMOVE(nic)
 DEVICE_ADDREMOVE(vfb)
 DEVICE_ADDREMOVE(vkb)
+DEVICE_ADDREMOVE(virtio)
 DEVICE_ADDREMOVE(pci)
 _DEVICE_ADDREMOVE(disk, cdrom, insert)
 
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 644ab8f8fd36..a187cd26e595 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1208,6 +1208,83 @@ static void parse_vkb_list(const XLU_Config *config,
     if (rc) exit(EXIT_FAILURE);
 }
 
+static int parse_virtio_config(libxl_device_virtio *virtio, char *token)
+{
+    char *oparg;
+    int rc;
+
+    if (MATCH_OPTION("backend", token, oparg)) {
+        virtio->backend_domname = strdup(oparg);
+    } else if (MATCH_OPTION("type", token, oparg)) {
+        virtio->type = strdup(oparg);
+    } else if (MATCH_OPTION("transport", token, oparg)) {
+        rc = libxl_virtio_transport_from_string(oparg, &virtio->transport);
+        if (rc) return rc;
+    } else {
+        fprintf(stderr, "Unknown string \"%s\" in virtio spec\n", token);
+        return -1;
+    }
+
+    return 0;
+}
+
+static void parse_virtio_list(const XLU_Config *config,
+                              libxl_domain_config *d_config)
+{
+    XLU_ConfigList *virtios;
+    const char *item;
+    char *buf = NULL, *oparg, *str = NULL;
+    int rc;
+
+    if (!xlu_cfg_get_list (config, "virtio", &virtios, 0, 0)) {
+        int entry = 0;
+        while ((item = xlu_cfg_get_listitem(virtios, entry)) != NULL) {
+            libxl_device_virtio *virtio;
+            char *p;
+
+            virtio = ARRAY_EXTEND_INIT(d_config->virtios, d_config->num_virtios,
+                                       libxl_device_virtio_init);
+
+            buf = strdup(item);
+
+            p = strtok(buf, ",");
+            while (p != NULL)
+            {
+                while (*p == ' ') p++;
+
+                // Type may contain a comma, do special handling.
+                if (MATCH_OPTION("type", p, oparg)) {
+                    if (!strncmp(oparg, "virtio", strlen("virtio"))) {
+                        char *p2 = strtok(NULL, ",");
+                        str = malloc(strlen(p) + strlen(p2) + 2);
+
+                        strcpy(str, p);
+                        strcat(str, ",");
+                        strcat(str, p2);
+                        p = str;
+                    }
+                }
+
+                rc = parse_virtio_config(virtio, p);
+                if (rc) goto out;
+
+                free(str);
+                str = NULL;
+                p = strtok(NULL, ",");
+            }
+
+            entry++;
+            free(buf);
+        }
+    }
+
+    return;
+
+out:
+    free(buf);
+    if (rc) exit(EXIT_FAILURE);
+}
+
 void parse_config_data(const char *config_source,
                        const char *config_data,
                        int config_len,
@@ -2309,8 +2386,10 @@ void parse_config_data(const char *config_source,
 
     d_config->num_vfbs = 0;
     d_config->num_vkbs = 0;
+    d_config->num_virtios = 0;
     d_config->vfbs = NULL;
     d_config->vkbs = NULL;
+    d_config->virtios = NULL;
 
     if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) {
         while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != NULL) {
@@ -2752,6 +2831,7 @@ void parse_config_data(const char *config_source,
     }
 
     parse_vkb_list(config, d_config);
+    parse_virtio_list(config, d_config);
 
     xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat",
                         &c_info->xend_suspend_evtchn_compat, 0);
-- 
2.31.1.272.g89b43f80a514

[PATCH V7 3/3] docs: Add documentation for generic virtio devices

[Viresh Kumar]

This patch updates xl.cfg man page with details of generic Virtio device
related information.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 docs/man/xl.cfg.5.pod.in | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index ec444fb2ba79..2523027ea54c 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -1585,6 +1585,34 @@ Set maximum height for pointer device.
 
 =back
 
+=item B<virtio=[ "VIRTIO_DEVICE_STRING", "VIRTIO_DEVICE_STRING", ...]>
+
+Specifies the Virtio devices to be provided to the guest.
+
+Each B<VIRTIO_DEVICE_STRING> is a comma-separated list of C<KEY=VALUE>
+settings from the following list:
+
+=over 4
+
+=item B<backend=domain-id>
+
+Specifies the backend domain name or id, defaults to dom0.
+
+=item B<type=STRING>
+
+Specifies the compatible string for the specific Virtio device. The same will be
+written in the Device Tree compatible property of the Virtio device. For
+example, "type=virtio,device22" for the I2C device, whose device-tree binding is
+present here:
+
+L<https://www.kernel.org/doc/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml>
+
+=item B<transport=STRING>
+
+Specifies the transport mechanism for the Virtio device, like "mmio" or "pci".
+
+=back
+
 =item B<tee="STRING">
 
 B<Arm only.> Set TEE type for the guest. TEE is a Trusted Execution
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V7 1/3] libxl: Add support for generic virtio device

[Anthony PERARD]

On Wed, Dec 07, 2022 at 12:50:42PM +0530, Viresh Kumar wrote:
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index fa3d61f1e882..ab3668b3b8a3 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> +static int make_virtio_mmio_node_device(libxl__gc *gc, void *fdt, uint64_t base,
> +                                        uint32_t irq, const char *type,
> +                                        uint32_t backend_domid)
> +{
> +    int res, len = strlen(type);
> +
> +    res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid);
> +    if (res) return res;
> +
> +    /* Add device specific nodes */
> +    if (!strncmp(type, "virtio,device22", len)) {

So with `len` been the strlen() of `type`, I think that a type "v" or
"virtio" or even "virtio,device" is going to create the "i2c" node. So I
don't think is going to be possible to create a generic virtio device
node.

Using strcmp() would be good enough here I think.

> +        res = make_virtio_mmio_node_i2c(gc, fdt);
> +        if (res) return res;
> +    } else if (!strncmp(type, "virtio,device29", len)) {
> +        res = make_virtio_mmio_node_gpio(gc, fdt);
> +        if (res) return res;
> +    } else if (!strncmp(type, "virtio,device", len)) {

The example in in the commit message has "virtio,devices" but that would
be rejected by this test due to the extra 's'.

> +        /* Generic Virtio Device */
> +        res = fdt_end_node(fdt);

Isn't this an extra end_node() call? As there's already one for the
return of the function.

> +        if (res) return res;
> +    } else {
> +        LOG(ERROR, "Invalid type for virtio device: %s", type);
> +        return -EINVAL;
> +    }
> +
> +    return fdt_end_node(fdt);
> +}
> +
> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
> new file mode 100644
> index 000000000000..64cec989c674
> --- /dev/null
> +++ b/tools/libs/light/libxl_virtio.c
> +static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
> +                                       libxl_devid devid,
> +                                       libxl_device_virtio *virtio)
> +{
> +    const char *be_path, *tmp = NULL;
> +    int rc;
> +
> +    virtio->devid = devid;
> +
> +    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
> +                                  GCSPRINTF("%s/backend", libxl_path),
> +                                  &be_path);
> +    if (rc) goto out;
> +
> +    rc = libxl__backendpath_parse_domid(gc, be_path, &virtio->backend_domid);
> +    if (rc) goto out;
> +
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +				GCSPRINTF("%s/irq", be_path), &tmp);
> +    if (rc) goto out;
> +
> +    if (tmp) {
> +        virtio->irq = strtoul(tmp, NULL, 0);
> +    }
> +
> +    tmp = NULL;
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +				GCSPRINTF("%s/base", be_path), &tmp);
> +    if (rc) goto out;
> +
> +    if (tmp) {
> +        virtio->base = strtoul(tmp, NULL, 0);
> +    }
> +
> +    tmp = NULL;
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +				GCSPRINTF("%s/transport", be_path), &tmp);
> +    if (rc) goto out;
> +
> +    if (tmp) {
> +        if (!strncmp(tmp, "mmio", strlen(tmp))) {

Maybe just use strcmp(), otherwise we have "m" going to be match as MMIO
for example.

> +            virtio->transport = LIBXL_VIRTIO_TRANSPORT_MMIO;
> +        } else if (!strncmp(tmp, "unknown", strlen(tmp))) {
> +            virtio->transport = LIBXL_VIRTIO_TRANSPORT_UNKNOWN;

I don't think that value should be allowed in xenstore. If `transport`
isn't set to a proper value (only "mmio"), then I think that invalid.

> +        } else {
> +            return ERROR_INVAL;
> +        }
> +    }
> +
> +    tmp = NULL;
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +				GCSPRINTF("%s/type", be_path), &tmp);
> +    if (rc) goto out;
> +
> +    if (tmp) {
> +        if (!strncmp(tmp, "virtio,device", strlen("virtio,device"))) {

Nit: Something like:
    const char check[] = "virtio,device";
    const size_t checkl = sizeof(check) - 1;
    ... strncmp(tmp, check, checkl)...
    (or just strncmp(tmp, check, sizeof(check)-1))
would avoid issue with both string "virtio,device" potentially been
different.


> +            virtio->type = strdup(tmp);

Could you use libxl__strdup(NO_GC, tmp) instead? That function is going
to check that strdup() doesn't fail the allocation.


Thanks,

-- 
Anthony PERARD

Re: [PATCH V7 1/3] libxl: Add support for generic virtio device

[Viresh Kumar]

On 08-12-22, 18:06, Anthony PERARD wrote:
> Nit: Something like:
>     const char check[] = "virtio,device";
>     const size_t checkl = sizeof(check) - 1;
>     ... strncmp(tmp, check, checkl)...
>     (or just strncmp(tmp, check, sizeof(check)-1))
> would avoid issue with both string "virtio,device" potentially been
> different.

I think that is a generic problem with all the strings I am using. What about
this diff over current patch ?

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index ab3668b3b8a3..292b31881210 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -981,7 +981,7 @@ static int make_virtio_mmio_node_i2c(libxl__gc *gc, void *fdt)
     res = fdt_begin_node(fdt, "i2c");
     if (res) return res;
 
-    res = fdt_property_compat(gc, fdt, 1, "virtio,device22");
+    res = fdt_property_compat(gc, fdt, 1, VIRTIO_DEVICE_TYPE_I2C);
     if (res) return res;
 
     return fdt_end_node(fdt);
@@ -999,7 +999,7 @@ static int make_virtio_mmio_node_gpio(libxl__gc *gc, void *fdt)
     res = fdt_begin_node(fdt, "gpio");
     if (res) return res;
 
-    res = fdt_property_compat(gc, fdt, 1, "virtio,device29");
+    res = fdt_property_compat(gc, fdt, 1, VIRTIO_DEVICE_TYPE_GPIO);
     if (res) return res;
 
     res = fdt_property(fdt, "gpio-controller", NULL, 0);
@@ -1021,23 +1021,20 @@ static int make_virtio_mmio_node_device(libxl__gc *gc, void *fdt, uint64_t base,
                                         uint32_t irq, const char *type,
                                         uint32_t backend_domid)
 {
-    int res, len = strlen(type);
+    int res;
 
     res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid);
     if (res) return res;
 
     /* Add device specific nodes */
-    if (!strncmp(type, "virtio,device22", len)) {
+    if (!strcmp(type, VIRTIO_DEVICE_TYPE_I2C)) {
         res = make_virtio_mmio_node_i2c(gc, fdt);
         if (res) return res;
-    } else if (!strncmp(type, "virtio,device29", len)) {
+    } else if (!strcmp(type, VIRTIO_DEVICE_TYPE_GPIO)) {
         res = make_virtio_mmio_node_gpio(gc, fdt);
         if (res) return res;
-    } else if (!strncmp(type, "virtio,device", len)) {
-        /* Generic Virtio Device */
-        res = fdt_end_node(fdt);
-        if (res) return res;
-    } else {
+    } else if (strcmp(type, VIRTIO_DEVICE_TYPE_GENERIC)) {
+        /* Doesn't match generic virtio device */
         LOG(ERROR, "Invalid type for virtio device: %s", type);
         return -EINVAL;
     }
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index cdd155d925c1..a062fca0e2bb 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -166,6 +166,11 @@
 /* Convert pfn to physical address space. */
 #define pfn_to_paddr(x) ((uint64_t)(x) << XC_PAGE_SHIFT)
 
+/* Virtio device types */
+#define VIRTIO_DEVICE_TYPE_GENERIC   "virtio,device"
+#define VIRTIO_DEVICE_TYPE_GPIO      "virtio,device22"
+#define VIRTIO_DEVICE_TYPE_I2C       "virtio,device29"
+
 /* logging */
 _hidden void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
              const char *file /* may be 0 */, int line /* ignored if !file */,
diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
index 64cec989c674..183d9c906e27 100644
--- a/tools/libs/light/libxl_virtio.c
+++ b/tools/libs/light/libxl_virtio.c
@@ -62,7 +62,7 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
                                        libxl_device_virtio *virtio)
 {
     const char *be_path, *tmp = NULL;
-    int rc;
+    int rc, len = sizeof(VIRTIO_DEVICE_TYPE_GENERIC) - 1;
 
     virtio->devid = devid;
 
@@ -97,10 +97,8 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
     if (rc) goto out;
 
     if (tmp) {
-        if (!strncmp(tmp, "mmio", strlen(tmp))) {
+        if (!strcmp(tmp, "mmio")) {
             virtio->transport = LIBXL_VIRTIO_TRANSPORT_MMIO;
-        } else if (!strncmp(tmp, "unknown", strlen(tmp))) {
-            virtio->transport = LIBXL_VIRTIO_TRANSPORT_UNKNOWN;
         } else {
             return ERROR_INVAL;
         }
@@ -112,8 +110,8 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
     if (rc) goto out;
 
     if (tmp) {
-        if (!strncmp(tmp, "virtio,device", strlen("virtio,device"))) {
-            virtio->type = strdup(tmp);
+        if (!strncmp(tmp, VIRTIO_DEVICE_TYPE_GENERIC, len)) {
+            virtio->type = libxl__strdup(NOGC, tmp);
         } else {
             return ERROR_INVAL;
         }

-- 
viresh

Re: [PATCH V7 1/3] libxl: Add support for generic virtio device

[Anthony PERARD]

On Fri, Dec 09, 2022 at 05:43:54AM +0530, Viresh Kumar wrote:
> On 08-12-22, 18:06, Anthony PERARD wrote:
> > Nit: Something like:
> >     const char check[] = "virtio,device";
> >     const size_t checkl = sizeof(check) - 1;
> >     ... strncmp(tmp, check, checkl)...
> >     (or just strncmp(tmp, check, sizeof(check)-1))
> > would avoid issue with both string "virtio,device" potentially been
> > different.
> 
> I think that is a generic problem with all the strings I am using. What about
> this diff over current patch ?
> 
> diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
> index cdd155d925c1..a062fca0e2bb 100644
> --- a/tools/libs/light/libxl_internal.h
> +++ b/tools/libs/light/libxl_internal.h
> @@ -166,6 +166,11 @@
>  /* Convert pfn to physical address space. */
>  #define pfn_to_paddr(x) ((uint64_t)(x) << XC_PAGE_SHIFT)
>  
> +/* Virtio device types */
> +#define VIRTIO_DEVICE_TYPE_GENERIC   "virtio,device"
> +#define VIRTIO_DEVICE_TYPE_GPIO      "virtio,device22"
> +#define VIRTIO_DEVICE_TYPE_I2C       "virtio,device29"

That a good idea.

>  /* logging */
>  _hidden void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
>               const char *file /* may be 0 */, int line /* ignored if !file */,
> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
> index 64cec989c674..183d9c906e27 100644
> --- a/tools/libs/light/libxl_virtio.c
> +++ b/tools/libs/light/libxl_virtio.c
> @@ -62,7 +62,7 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
>                                         libxl_device_virtio *virtio)
>  {
>      const char *be_path, *tmp = NULL;
> -    int rc;
> +    int rc, len = sizeof(VIRTIO_DEVICE_TYPE_GENERIC) - 1;

That `len` variable is initialized quite far away from where it's used,
so ...
>  
>      virtio->devid = devid;
>  
> @@ -112,8 +110,8 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
>      if (rc) goto out;
>  
>      if (tmp) {

... you could declare `len` here instead.

> -        if (!strncmp(tmp, "virtio,device", strlen("virtio,device"))) {
> -            virtio->type = strdup(tmp);
> +        if (!strncmp(tmp, VIRTIO_DEVICE_TYPE_GENERIC, len)) {
> +            virtio->type = libxl__strdup(NOGC, tmp);
>          } else {
>              return ERROR_INVAL;
>          }
> 


Otherwise, those change looks fine.
Thanks,

-- 
Anthony PERARD

Re: [PATCH V7 3/3] docs: Add documentation for generic virtio devices

[Anthony PERARD]

On Wed, Dec 07, 2022 at 12:50:44PM +0530, Viresh Kumar wrote:
> +=item B<virtio=[ "VIRTIO_DEVICE_STRING", "VIRTIO_DEVICE_STRING", ...]>
> +
> +Specifies the Virtio devices to be provided to the guest.
> +
> +Each B<VIRTIO_DEVICE_STRING> is a comma-separated list of C<KEY=VALUE>
> +settings from the following list:

We should probably say something about allowing a comma in the VALUE for
"type" when the value starts with "virtio," as otherwise it might be
confusing.

> +
> +=over 4
> +
> +=item B<backend=domain-id>
> +
> +Specifies the backend domain name or id, defaults to dom0.
> +
> +=item B<type=STRING>
> +
> +Specifies the compatible string for the specific Virtio device. The same will be
> +written in the Device Tree compatible property of the Virtio device. For
> +example, "type=virtio,device22" for the I2C device, whose device-tree binding is
> +present here:
> +
> +L<https://www.kernel.org/doc/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml>
> +
> +=item B<transport=STRING>
> +
> +Specifies the transport mechanism for the Virtio device, like "mmio" or "pci".

"pci" doesn't exist, only "mmio" is allowed here at the moment. We can
change the man page later when new options become available.


Thanks,

-- 
Anthony PERARD