[PATCH V6 0/3] Virtio toolstack support for I2C and GPIO on Arm

[PATCH V6 0/3] Virtio toolstack support for I2C and GPIO on Arm

[Viresh Kumar]

Hello,

This patchset adds toolstack support for I2C and GPIO 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 backend [1].

This is based of origin/staging (e61a78981364 xen/arm: add iounmap after initrd
has been loaded in domain_build) and the earlier posted cleanup patches [2].

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://lore.kernel.org/all/cover.1662626550.git.viresh.kumar@linaro.org/



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                  |  21 ++++
 tools/libs/light/Makefile                 |   1 +
 tools/libs/light/libxl_arm.c              |  89 +++++++++++++++
 tools/libs/light/libxl_create.c           |   5 +
 tools/libs/light/libxl_internal.h         |   1 +
 tools/libs/light/libxl_types.idl          |  29 +++++
 tools/libs/light/libxl_types_internal.idl |   1 +
 tools/libs/light/libxl_virtio.c           | 127 ++++++++++++++++++++++
 tools/ocaml/libs/xl/genwrap.py            |   1 +
 tools/ocaml/libs/xl/xenlight_stubs.c      |   1 +
 tools/xl/xl_parse.c                       |  84 ++++++++++++++
 11 files changed, 360 insertions(+)
 create mode 100644 tools/libs/light/libxl_virtio.c

-- 
2.31.1.272.g89b43f80a514

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

[Viresh Kumar]

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

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

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

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 tools/libs/light/Makefile                 |   1 +
 tools/libs/light/libxl_arm.c              |  89 +++++++++++++++
 tools/libs/light/libxl_create.c           |   5 +
 tools/libs/light/libxl_internal.h         |   1 +
 tools/libs/light/libxl_types.idl          |  29 +++++
 tools/libs/light/libxl_types_internal.idl |   1 +
 tools/libs/light/libxl_virtio.c           | 127 ++++++++++++++++++++++
 7 files changed, 253 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 b4928dbf673c..f33c9b273a4f 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.
@@ -968,6 +981,68 @@ static int make_virtio_mmio_node(libxl__gc *gc, void *fdt, uint64_t base,
     return fdt_end_node(fdt);
 }
 
+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);
+}
+
+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 {
+        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)
 {
@@ -1290,6 +1365,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) );
+        }
+
         /*
          * Note, this should be only called after creating all virtio-mmio
          * device nodes
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 612eacfc7fac..15a32c75c045 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1802,6 +1802,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
                               &d_config->vkbs[i]);
         }
 
+        for (i = 0; i < d_config->num_virtios; i++) {
+            libxl__device_add(gc, domid, &libxl__virtio_devtype,
+                              &d_config->virtios[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_internal.h b/tools/libs/light/libxl_internal.h
index cb9e8b3b8b5a..e5716f9bef80 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 d634f304cda2..d97a0795d312 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -278,6 +278,14 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
     (2, "LINUX")
     ])
 
+libxl_virtio_backend = Enumeration("virtio_backend", [
+    (1, "STANDALONE")
+    ])
+
+libxl_virtio_transport = Enumeration("virtio_transport", [
+    (1, "MMIO"),
+    ])
+
 libxl_passthrough = Enumeration("passthrough", [
     (0, "default"),
     (1, "disabled"),
@@ -705,6 +713,17 @@ libxl_device_vkb = Struct("device_vkb", [
     ("multi_touch_num_contacts", uint32)
     ])
 
+libxl_device_virtio = Struct("device_virtio", [
+    ("backend_domid", libxl_domid),
+    ("backend_domname", string),
+    ("backend", libxl_virtio_backend),
+    ("type", string),
+    ("transport", libxl_virtio_transport),
+    ("devid", libxl_devid),
+    ("irq", uint32),
+    ("base", uint64)
+    ])
+
 libxl_device_disk = Struct("device_disk", [
     ("backend_domid", libxl_domid),
     ("backend_domname", string),
@@ -982,6 +1001,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")),
@@ -1145,6 +1165,15 @@ libxl_vkbinfo = Struct("vkbinfo", [
     ("rref", integer)
     ], dir=DIR_OUT)
 
+libxl_virtioinfo = Struct("virtioinfo", [
+    ("backend", string),
+    ("backend_id", uint32),
+    ("frontend", string),
+    ("frontend_id", uint32),
+    ("devid", libxl_devid),
+    ("state", integer),
+    ], dir=DIR_OUT)
+
 # NUMA node characteristics: size and free are how much memory it has, and how
 # much of it is free, respectively. dists is an array of distances from this
 # node to each other node.
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..14b0c016a0a2
--- /dev/null
+++ b/tools/libs/light/libxl_virtio.c
@@ -0,0 +1,127 @@
+/*
+ * 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, *fe_path, *tmp;
+    libxl__device dev;
+    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__xs_read_mandatory(gc, XBT_NULL,
+                                  GCSPRINTF("%s/frontend", libxl_path),
+                                  &fe_path);
+    if (rc) goto out;
+
+    rc = libxl__backendpath_parse_domid(gc, be_path, &virtio->backend_domid);
+    if (rc) goto out;
+
+    rc = libxl__parse_backend_path(gc, be_path, &dev);
+    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);
+    }
+
+    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);
+    }
+
+    rc = 0;
+
+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 V6 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                  | 84 ++++++++++++++++++++++++++++
 3 files changed, 86 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 1b5381cef033..c6f35c069d2a 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1208,6 +1208,87 @@ 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 if (MATCH_OPTION("irq", token, oparg)) {
+        virtio->irq = strtoul(oparg, NULL, 0);
+    } else if (MATCH_OPTION("base", token, oparg)) {
+        virtio->base = strtoul(oparg, NULL, 0);
+    } 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 +2390,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 +2835,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 V6 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 | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 31e58b73b0c9..1056b03df846 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -1585,6 +1585,27 @@ 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<compatible=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.
+
+=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 V6 0/3] Virtio toolstack support for I2C and GPIO on Arm

[Viresh Kumar]

On 08-11-22, 16:53, Viresh Kumar wrote:
> Hello,
> 
> This patchset adds toolstack support for I2C and GPIO 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 backend [1].
> 
> This is based of origin/staging (e61a78981364 xen/arm: add iounmap after initrd
> has been loaded in domain_build) and the earlier posted cleanup patches [2].
> 
> 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.

Ping.

-- 
viresh

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

[Oleksandr Tyshchenko]

On 08.11.22 13:23, Viresh Kumar wrote:


Hello Viresh

[sorry for the possible format issues if any]


> This patch adds basic support for configuring and assisting generic
> Virtio backend which could run in any domain.
> 
> An example of domain configuration for mmio based Virtio I2C device is:
> virtio = ["type=virtio,device22,transport=mmio"]
> 
> Also to make this work on Arm, allocate Virtio MMIO params (IRQ and
> memory region) and pass them to the backend. Update guest device-tree as
> well to create a DT node for the Virtio devices.


Some NITs regarding the commit description:
1. Besides making generic things current patch also adds i2c/gpio device 
nodes, I would mention that in the description.
2. I assume current patch is not enough to make this work on Arm, at 
least the subsequent patch is needed, I would mention that as well.
3. I understand where "virtio,device22"/"virtio,device29" came from, but 
I think that links to the corresponding device-tree bindings should be 
mentioned here (and/or maybe in the code).



> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   tools/libs/light/Makefile                 |   1 +
>   tools/libs/light/libxl_arm.c              |  89 +++++++++++++++
>   tools/libs/light/libxl_create.c           |   5 +
>   tools/libs/light/libxl_internal.h         |   1 +
>   tools/libs/light/libxl_types.idl          |  29 +++++
>   tools/libs/light/libxl_types_internal.idl |   1 +
>   tools/libs/light/libxl_virtio.c           | 127 ++++++++++++++++++++++
>   7 files changed, 253 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 b4928dbf673c..f33c9b273a4f 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.
> @@ -968,6 +981,68 @@ static int make_virtio_mmio_node(libxl__gc *gc, void *fdt, uint64_t base,
>       return fdt_end_node(fdt);
>   }
>   
> +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);
> +}
> +
> +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 {
> +        LOG(ERROR, "Invalid type for virtio device: %s", type);
> +        return -EINVAL;
> +    }

I am not sure whether it is the best place to ask, but I will try 
anyway. So I assume that with the whole series applied it would be 
possible to configure only two specific device types ("22" and "29").
But what to do if user, for example, is interested in usual virtio 
device (which doesn't require specific device-tree sub node with 
specific compatible in it). For these usual virtio devices just calling 
make_virtio_mmio_node_common() would be enough.


Maybe we should introduce something like type "common" which would mean 
we don't need any additional device-tree sub nodes?

virtio = ["type=common,transport=mmio"]


> +
> +    return fdt_end_node(fdt);
> +}
> +
>   static const struct arch_info *get_arch_info(libxl__gc *gc,
>                                                const struct xc_dom_image *dom)
>   {
> @@ -1290,6 +1365,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) );
> +        }
> +
>           /*
>            * Note, this should be only called after creating all virtio-mmio
>            * device nodes
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index 612eacfc7fac..15a32c75c045 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -1802,6 +1802,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>                                 &d_config->vkbs[i]);
>           }
>   
> +        for (i = 0; i < d_config->num_virtios; i++) {
> +            libxl__device_add(gc, domid, &libxl__virtio_devtype,
> +                              &d_config->virtios[i]);
> +        }


I am wondering whether this is the best place to put this call. This 
gets called for LIBXL_DOMAIN_TYPE_PV and LIBXL_DOMAIN_TYPE_PVH (our Arm 
case), and not for LIBXL_DOMAIN_TYPE_HVM. Is it what we want?


> +
>           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_internal.h b/tools/libs/light/libxl_internal.h
> index cb9e8b3b8b5a..e5716f9bef80 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 d634f304cda2..d97a0795d312 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -278,6 +278,14 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
>       (2, "LINUX")
>       ])
>   
> +libxl_virtio_backend = Enumeration("virtio_backend", [

I would add (0, "UNKNOWN") here ...

> +    (1, "STANDALONE")
> +    ])
> +
> +libxl_virtio_transport = Enumeration("virtio_transport", [

    ... and here (these might be needed by parsing code)

> +    (1, "MMIO"),
> +    ])
> +
>   libxl_passthrough = Enumeration("passthrough", [
>       (0, "default"),
>       (1, "disabled"),
> @@ -705,6 +713,17 @@ libxl_device_vkb = Struct("device_vkb", [
>       ("multi_touch_num_contacts", uint32)
>       ])
>   
> +libxl_device_virtio = Struct("device_virtio", [
> +    ("backend_domid", libxl_domid),
> +    ("backend_domname", string),
> +    ("backend", libxl_virtio_backend),
> +    ("type", string),
> +    ("transport", libxl_virtio_transport),
> +    ("devid", libxl_devid),
> +    ("irq", uint32),
> +    ("base", uint64)
> +    ])
> +
>   libxl_device_disk = Struct("device_disk", [
>       ("backend_domid", libxl_domid),
>       ("backend_domname", string),
> @@ -982,6 +1001,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")),
> @@ -1145,6 +1165,15 @@ libxl_vkbinfo = Struct("vkbinfo", [
>       ("rref", integer)
>       ], dir=DIR_OUT)
>   
> +libxl_virtioinfo = Struct("virtioinfo", [
> +    ("backend", string),
> +    ("backend_id", uint32),
> +    ("frontend", string),
> +    ("frontend_id", uint32),
> +    ("devid", libxl_devid),
> +    ("state", integer),
> +    ], dir=DIR_OUT)

I failed to find where libxl_virtioinfo is used within the series. Why 
do we need it?


> +
>   # NUMA node characteristics: size and free are how much memory it has, and how
>   # much of it is free, respectively. dists is an array of distances from this
>   # node to each other node.
> 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..14b0c016a0a2
> --- /dev/null
> +++ b/tools/libs/light/libxl_virtio.c
> @@ -0,0 +1,127 @@
> +/*
> + * 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, *fe_path, *tmp;
> +    libxl__device dev;
> +    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__xs_read_mandatory(gc, XBT_NULL,
> +                                  GCSPRINTF("%s/frontend", libxl_path),
> +                                  &fe_path);
> +    if (rc) goto out;

fe_path is not used anywhere down the code even within the series, why 
do we need it? Or we just read it to make sure that corresponding entry 
is present in Xenstore (some kind of sanity check)?


> +
> +    rc = libxl__backendpath_parse_domid(gc, be_path, &virtio->backend_domid);
> +    if (rc) goto out;
> +
> +    rc = libxl__parse_backend_path(gc, be_path, &dev);
> +    if (rc) goto out;

The same question for dev variable.



> +
> +    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);
> +    }
> +
> +    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);
> +    }
> +
> +    rc = 0;


It feels to me, that we also need to read "type" and "transport" from 
the Xenstore (and probably check them for the valid values).


> +
> +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:
> + */

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

[Oleksandr Tyshchenko]

On 08.11.22 13:23, Viresh Kumar wrote:


Hello Viresh

[sorry for the possible format issues if any]

> 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                  | 84 ++++++++++++++++++++++++++++
>   3 files changed, 86 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 1b5381cef033..c6f35c069d2a 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1208,6 +1208,87 @@ 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 if (MATCH_OPTION("irq", token, oparg)) {
> +        virtio->irq = strtoul(oparg, NULL, 0);
> +    } else if (MATCH_OPTION("base", token, oparg)) {
> +        virtio->base = strtoul(oparg, NULL, 0);


Interesting, I see you allow user to configure virtio-mmio params (irq 
and base), as far as I remember for virtio-disk these are internal only 
(allocated by tools/libs/light/libxl_arm.c).

I am not really sure why we need to configure virtio "base", could you 
please clarify? But if we really want/need to be able to configure 
virtio "irq" (for example to avoid possible clashing with physical one), 
I am afraid, this will require more changes that current patch does. 
Within current series saving virtio->irq here doesn't have any effect as 
it will be overwritten in 
libxl__arch_domain_prepare_config()->alloc_virtio_mmio_params() anyway. 
I presume the code in libxl__arch_domain_prepare_config() shouldn't try 
to allocate virtio->irq if it is already configured by user, also the 
allocator should probably take into the account of what is already 
configured by user, to avoid allocating the same irq for another device 
assigned for the same guest.

Also doc change in the subsequent patch doesn't mention about irq/base 
configuration.


So maybe we should just drop for now?
+    } else if (MATCH_OPTION("irq", token, oparg)) {
+        virtio->irq = strtoul(oparg, NULL, 0);
+    } else if (MATCH_OPTION("base", token, oparg)) {
+        virtio->base = strtoul(oparg, NULL, 0);



> +    } 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 +2390,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 +2835,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);

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

[Oleksandr Tyshchenko]

On 08.11.22 13:24, Viresh Kumar wrote:

Hello Viresh


[sorry for the possible format issues if any]

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


So as I understand current series adds support for two virtio devices 
(i2c/gpio) that require specific device-tree sub node with specific 
compatible in it [1]. Those backends are standalone userspace 
applications (daemons) that do not require any additional configuration 
parameters from the toolstack other than just virtio-mmio irq and base 
(please correct me if I am wrong).

Well, below just some thoughts (which might be wrong) regarding the 
possible extensions for future use. Please note, I do not suggest the 
following to be implemented right now (I mean within the context of 
current series):

1. For supporting usual virtio devices that don't require specific 
device-tree sub node with specific compatible in it [2] we would 
probably need to either make "compatible" (or type?) string optional or 
to reserve some value for it ("common" for the instance).
2. For supporting Qemu based virtio devices we would probably need to 
add "backendtype" string (with "standalone" value for daemons like yours 
and "qemu" value for Qemu backends).
3. For supporting additional configuration parameters for Qemu based 
virtio devices we could probably reuse "device_model_args" (although it 
is not clear to me what alternative to use for daemons).

Any other thoughts?

> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   docs/man/xl.cfg.5.pod.in | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 31e58b73b0c9..1056b03df846 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1585,6 +1585,27 @@ 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<compatible=STRING>

Shouldn't it be "type" instead (the parsing code is looking for type and 
the example below suggests the type)?

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



Also the commit description for #1/3 mentions that Virtio backend could 
run in any domain. So looks like the "backend" string is missing here. I 
would add the following:

=item B<backend=domain-id>

Specify the backend domain name or id, defaults to dom0.


P.S. I am wondering do i2c/gpio virtio backends support Xen grant 
mappings for the virtio? Have you tried to run the backends in 
non-hardware domain with CONFIG_XEN_VIRTIO=y in Linux?


[1]
https://www.kernel.org/doc/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml
https://www.kernel.org/doc/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml
[2]
https://www.kernel.org/doc/Documentation/devicetree/bindings/virtio/mmio.yaml

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

[Viresh Kumar]

Hi Oleksandr,

On 02-12-22, 16:52, Oleksandr Tyshchenko wrote:
> > This patch adds basic support for configuring and assisting generic
> > Virtio backend which could run in any domain.
> > 
> > An example of domain configuration for mmio based Virtio I2C device is:
> > virtio = ["type=virtio,device22,transport=mmio"]
> > 
> > Also to make this work on Arm, allocate Virtio MMIO params (IRQ and
> > memory region) and pass them to the backend. Update guest device-tree as
> > well to create a DT node for the Virtio devices.
> 
> 
> Some NITs regarding the commit description:
> 1. Besides making generic things current patch also adds i2c/gpio device
> nodes, I would mention that in the description.
> 2. I assume current patch is not enough to make this work on Arm, at least
> the subsequent patch is needed, I would mention that as well.
> 3. I understand where "virtio,device22"/"virtio,device29" came from, but I
> think that links to the corresponding device-tree bindings should be
> mentioned here (and/or maybe in the code).

Agree to all.
 
> > +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 {
> > +        LOG(ERROR, "Invalid type for virtio device: %s", type);
> > +        return -EINVAL;
> > +    }
> 
> I am not sure whether it is the best place to ask, but I will try anyway. So
> I assume that with the whole series applied it would be possible to
> configure only two specific device types ("22" and "29").

Right.

> But what to do if user, for example, is interested in usual virtio device
> (which doesn't require specific device-tree sub node with specific
> compatible in it). For these usual virtio devices just calling
> make_virtio_mmio_node_common() would be enough.
> 
> 
> Maybe we should introduce something like type "common" which would mean we
> don't need any additional device-tree sub nodes?
> 
> virtio = ["type=common,transport=mmio"]

I am fine with this. Maybe, to keep it aligned with compatibles, we
can write it as
 
virtio = ["type=virtio,device,transport=mmio"]

and document that "virtio,device" type is special and we won't add
compatible property to the DT node.

> > diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> > index 612eacfc7fac..15a32c75c045 100644
> > --- a/tools/libs/light/libxl_create.c
> > +++ b/tools/libs/light/libxl_create.c
> > @@ -1802,6 +1802,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
> >                                 &d_config->vkbs[i]);
> >           }
> > +        for (i = 0; i < d_config->num_virtios; i++) {
> > +            libxl__device_add(gc, domid, &libxl__virtio_devtype,
> > +                              &d_config->virtios[i]);
> > +        }
> 
> 
> I am wondering whether this is the best place to put this call. This gets
> called for LIBXL_DOMAIN_TYPE_PV and LIBXL_DOMAIN_TYPE_PVH (our Arm case),
> and not for LIBXL_DOMAIN_TYPE_HVM. Is it what we want?

Can you suggest where should I move this ?
 
> > +libxl_virtioinfo = Struct("virtioinfo", [
> > +    ("backend", string),
> > +    ("backend_id", uint32),
> > +    ("frontend", string),
> > +    ("frontend_id", uint32),
> > +    ("devid", libxl_devid),
> > +    ("state", integer),
> > +    ], dir=DIR_OUT)
> 
> I failed to find where libxl_virtioinfo is used within the series. Why do we
> need it?

Looks like leftover that I missed. Will remove it.
 
> > +static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
> > +                                       libxl_devid devid,
> > +                                       libxl_device_virtio *virtio)
> > +{
> > +    const char *be_path, *fe_path, *tmp;
> > +    libxl__device dev;
> > +    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__xs_read_mandatory(gc, XBT_NULL,
> > +                                  GCSPRINTF("%s/frontend", libxl_path),
> > +                                  &fe_path);
> > +    if (rc) goto out;
> 
> fe_path is not used anywhere down the code even within the series, why do we
> need it? Or we just read it to make sure that corresponding entry is present
> in Xenstore (some kind of sanity check)?

I copied it from libxl_vkb.c and it isn't using it either. So I assume
it is a sanity check, though can be removed if that's what makes
sense.
 
> > +
> > +    rc = libxl__backendpath_parse_domid(gc, be_path, &virtio->backend_domid);
> > +    if (rc) goto out;
> > +
> > +    rc = libxl__parse_backend_path(gc, be_path, &dev);
> > +    if (rc) goto out;
> 
> The same question for dev variable.

Hmm, this we aren't using at all, which KBD does use it. Maybe we
should even call libxl__parse_backend_path() ?

-- 
viresh

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

[Viresh Kumar]

On 02-12-22, 19:16, Oleksandr Tyshchenko wrote:
> Interesting, I see you allow user to configure virtio-mmio params (irq and
> base), as far as I remember for virtio-disk these are internal only
> (allocated by tools/libs/light/libxl_arm.c).

It is a mistake. Will drop it.

-- 
viresh

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

[Viresh Kumar]

On 04-12-22, 20:52, Oleksandr Tyshchenko wrote:
> So as I understand current series adds support for two virtio devices
> (i2c/gpio) that require specific device-tree sub node with specific
> compatible in it [1]. Those backends are standalone userspace applications
> (daemons) that do not require any additional configuration parameters from
> the toolstack other than just virtio-mmio irq and base (please correct me if
> I am wrong).

For now, yes. But we may want to link these devices with other devices
in DT, like GPIO line consumers. I am not pushing a half informed
solution for that right now and that can be taken up later.

> Well, below just some thoughts (which might be wrong) regarding the possible
> extensions for future use. Please note, I do not suggest the following to be
> implemented right now (I mean within the context of current series):
> 
> 1. For supporting usual virtio devices that don't require specific
> device-tree sub node with specific compatible in it [2] we would probably
> need to either make "compatible" (or type?) string optional or to reserve
> some value for it ("common" for the instance).

I agree. Maybe we can use "virtio,device" without a number for the
device in this case.

> 2. For supporting Qemu based virtio devices we would probably need to add
> "backendtype" string (with "standalone" value for daemons like yours and
> "qemu" value for Qemu backends).

Hmm, I realize now that my patch did define a new type for this,
libxl_virtio_backend, which defines STANDALONE already, but it isn't
used currently. Maybe I should remove it too.

And I am not sure sure how to use these values, STANDALONE or QEMU.
Should the DT nodes be created only for STANDALONE and never for QEMU
?

Maybe we can add these fields and a config param, once someone wants
to reuse this stuff for QEMU ?

> 3. For supporting additional configuration parameters for Qemu based virtio
> devices we could probably reuse "device_model_args" (although it is not
> clear to me what alternative to use for daemons).

I would leave it for the person who will make use of this eventually,
as then we will have more information on the same.

> > +=item B<compatible=STRING>
> 
> Shouldn't it be "type" instead (the parsing code is looking for type and the
> example below suggests the type)?

Yes.

> > +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 > +
> > +=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
> 
> Also the commit description for #1/3 mentions that Virtio backend could run
> in any domain. So looks like the "backend" string is missing here. I would
> add the following:
> 
> =item B<backend=domain-id>
> 
> Specify the backend domain name or id, defaults to dom0.

I haven't used the backend in any other domain for now, just Dom0, but
the idea is definitely there to run backends in separate user domains.

> P.S. I am wondering do i2c/gpio virtio backends support Xen grant mappings
> for the virtio?

Not yet, we haven't made much progress in that area until now, but it
is very much part of what we intend to do.

> Have you tried to run the backends in non-hardware domain
> with CONFIG_XEN_VIRTIO=y in Linux?

Not yet.

-- 
viresh

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

[Viresh Kumar]

On 05-12-22, 11:45, Viresh Kumar wrote:
> > > +    rc = libxl__backendpath_parse_domid(gc, be_path, &virtio->backend_domid);
> > > +    if (rc) goto out;
> > > +
> > > +    rc = libxl__parse_backend_path(gc, be_path, &dev);
> > > +    if (rc) goto out;
> > 
> > The same question for dev variable.
> 
> Hmm, this we aren't using at all, which KBD does use it. Maybe we
> should even call libxl__parse_backend_path() ?

Removing it works just fine for me.

-- 
viresh

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

[Oleksandr Tyshchenko]

On 05.12.22 08:15, Viresh Kumar wrote:
> Hi Oleksandr,


Hello Viresh

> 
> On 02-12-22, 16:52, Oleksandr Tyshchenko wrote:
>>> This patch adds basic support for configuring and assisting generic
>>> Virtio backend which could run in any domain.
>>>
>>> An example of domain configuration for mmio based Virtio I2C device is:
>>> virtio = ["type=virtio,device22,transport=mmio"]
>>>
>>> Also to make this work on Arm, allocate Virtio MMIO params (IRQ and
>>> memory region) and pass them to the backend. Update guest device-tree as
>>> well to create a DT node for the Virtio devices.
>>
>>
>> Some NITs regarding the commit description:
>> 1. Besides making generic things current patch also adds i2c/gpio device
>> nodes, I would mention that in the description.
>> 2. I assume current patch is not enough to make this work on Arm, at least
>> the subsequent patch is needed, I would mention that as well.
>> 3. I understand where "virtio,device22"/"virtio,device29" came from, but I
>> think that links to the corresponding device-tree bindings should be
>> mentioned here (and/or maybe in the code).
> 
> Agree to all.
>   
>>> +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 {
>>> +        LOG(ERROR, "Invalid type for virtio device: %s", type);
>>> +        return -EINVAL;
>>> +    }
>>
>> I am not sure whether it is the best place to ask, but I will try anyway. So
>> I assume that with the whole series applied it would be possible to
>> configure only two specific device types ("22" and "29").
> 
> Right.
> 
>> But what to do if user, for example, is interested in usual virtio device
>> (which doesn't require specific device-tree sub node with specific
>> compatible in it). For these usual virtio devices just calling
>> make_virtio_mmio_node_common() would be enough.
>>
>>
>> Maybe we should introduce something like type "common" which would mean we
>> don't need any additional device-tree sub nodes?
>>
>> virtio = ["type=common,transport=mmio"]
> 
> I am fine with this. Maybe, to keep it aligned with compatibles, we
> can write it as
>   
> virtio = ["type=virtio,device,transport=mmio"]
> 
> and document that "virtio,device" type is special and we won't add
> compatible property to the DT node.


Personally I am fine with this.


> 
>>> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
>>> index 612eacfc7fac..15a32c75c045 100644
>>> --- a/tools/libs/light/libxl_create.c
>>> +++ b/tools/libs/light/libxl_create.c
>>> @@ -1802,6 +1802,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>>>                                  &d_config->vkbs[i]);
>>>            }
>>> +        for (i = 0; i < d_config->num_virtios; i++) {
>>> +            libxl__device_add(gc, domid, &libxl__virtio_devtype,
>>> +                              &d_config->virtios[i]);
>>> +        }
>>
>>
>> I am wondering whether this is the best place to put this call. This gets
>> called for LIBXL_DOMAIN_TYPE_PV and LIBXL_DOMAIN_TYPE_PVH (our Arm case),
>> and not for LIBXL_DOMAIN_TYPE_HVM. Is it what we want?
> 
> Can you suggest where should I move this ?


I am not 100% sure, but I think if this whole enabling work is supposed 
to be indeed generic,
I would move this out of "switch (d_config->c_info.type)" at least.

>   
>>> +libxl_virtioinfo = Struct("virtioinfo", [
>>> +    ("backend", string),
>>> +    ("backend_id", uint32),
>>> +    ("frontend", string),
>>> +    ("frontend_id", uint32),
>>> +    ("devid", libxl_devid),
>>> +    ("state", integer),
>>> +    ], dir=DIR_OUT)
>>
>> I failed to find where libxl_virtioinfo is used within the series. Why do we
>> need it?
> 
> Looks like leftover that I missed. Will remove it.
>   
>>> +static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
>>> +                                       libxl_devid devid,
>>> +                                       libxl_device_virtio *virtio)
>>> +{
>>> +    const char *be_path, *fe_path, *tmp;
>>> +    libxl__device dev;
>>> +    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__xs_read_mandatory(gc, XBT_NULL,
>>> +                                  GCSPRINTF("%s/frontend", libxl_path),
>>> +                                  &fe_path);
>>> +    if (rc) goto out;
>>
>> fe_path is not used anywhere down the code even within the series, why do we
>> need it? Or we just read it to make sure that corresponding entry is present
>> in Xenstore (some kind of sanity check)?
> 
> I copied it from libxl_vkb.c and it isn't using it either. So I assume
> it is a sanity check, though can be removed if that's what makes
> sense.
>   
>>> +
>>> +    rc = libxl__backendpath_parse_domid(gc, be_path, &virtio->backend_domid);
>>> +    if (rc) goto out;
>>> +
>>> +    rc = libxl__parse_backend_path(gc, be_path, &dev);
>>> +    if (rc) goto out;
>>
>> The same question for dev variable.
> 
> Hmm, this we aren't using at all, which KBD does use it. Maybe we
> should even call libxl__parse_backend_path() ?


 From the code I see that KBD uses "dev.backend_kind" afterwards, so for 
that purpose it calls libxl__parse_backend_path() to fill in dev's 
fields, but here we do not need it. I would drop this call together with 
dev variable.

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

[Oleksandr Tyshchenko]

On 05.12.22 08:20, Viresh Kumar wrote:

Hello Viresh

> On 02-12-22, 19:16, Oleksandr Tyshchenko wrote:
>> Interesting, I see you allow user to configure virtio-mmio params (irq and
>> base), as far as I remember for virtio-disk these are internal only
>> (allocated by tools/libs/light/libxl_arm.c).
> 
> It is a mistake. Will drop it.


ok, good. Please don't forget to add a note to idl file that virtio-mmio 
params are internal only.


libxl_device_virtio = Struct("device_virtio", [
     ...

     # Note that virtio-mmio parameters (irq and base) are for internal
     # use by libxl and can't be modified.
     ("irq", uint32),
     ("base", uint64)
     ])


> 

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

[Oleksandr Tyshchenko]

On 05.12.22 11:11, Viresh Kumar wrote:


Hello Viresh

> On 04-12-22, 20:52, Oleksandr Tyshchenko wrote:
>> So as I understand current series adds support for two virtio devices
>> (i2c/gpio) that require specific device-tree sub node with specific
>> compatible in it [1]. Those backends are standalone userspace applications
>> (daemons) that do not require any additional configuration parameters from
>> the toolstack other than just virtio-mmio irq and base (please correct me if
>> I am wrong).
> 
> For now, yes. But we may want to link these devices with other devices
> in DT, like GPIO line consumers. I am not pushing a half informed
> solution for that right now and that can be taken up later.

I got it, ok.

> 
>> Well, below just some thoughts (which might be wrong) regarding the possible
>> extensions for future use. Please note, I do not suggest the following to be
>> implemented right now (I mean within the context of current series):
>>
>> 1. For supporting usual virtio devices that don't require specific
>> device-tree sub node with specific compatible in it [2] we would probably
>> need to either make "compatible" (or type?) string optional or to reserve
>> some value for it ("common" for the instance).
> 
> I agree. Maybe we can use "virtio,device" without a number for the
> device in this case.


Fine with me.


> 
>> 2. For supporting Qemu based virtio devices we would probably need to add
>> "backendtype" string (with "standalone" value for daemons like yours and
>> "qemu" value for Qemu backends).
> 
> Hmm, I realize now that my patch did define a new type for this,
> libxl_virtio_backend, which defines STANDALONE already, but it isn't
> used currently. Maybe I should remove it too.
> 
> And I am not sure sure how to use these values, STANDALONE or QEMU.
> Should the DT nodes be created only for STANDALONE and never for QEMU
> ?

If we expose virtio-mmio device to the guest via device-tree on Arm, 
then I think the DT nodes should be always created here, no matter where 
the corresponding virtio backend is located itself (either STANDALONE or 
QEMU).

> 
> Maybe we can add these fields and a config param, once someone wants
> to reuse this stuff for QEMU ?


I don't know what to suggest here, sorry.

On the one hand, it is an extra work for you trying to add functionality 
you don't need at the moment. On the other hand if we add "backendtype" 
config param right now with default to STANDALONE it might simplify work 
for someone who ends up adding other type (in particular, the QEMU). 
Let's see what the maintainers will say.



> 
>> 3. For supporting additional configuration parameters for Qemu based virtio
>> devices we could probably reuse "device_model_args" (although it is not
>> clear to me what alternative to use for daemons).
> 
> I would leave it for the person who will make use of this eventually,
> as then we will have more information on the same.

Sure, these are just thoughts for now.

> 
>>> +=item B<compatible=STRING>
>>
>> Shouldn't it be "type" instead (the parsing code is looking for type and the
>> example below suggests the type)?
> 
> Yes.
> 
>>> +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 > +
>>> +=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
>>
>> Also the commit description for #1/3 mentions that Virtio backend could run
>> in any domain. So looks like the "backend" string is missing here. I would
>> add the following:
>>
>> =item B<backend=domain-id>
>>
>> Specify the backend domain name or id, defaults to dom0.
> 
> I haven't used the backend in any other domain for now, just Dom0, but
> the idea is definitely there to run backends in separate user domains.


ok, good. My point is the following: if backend domain is configurable 
then it should be documented here.

> 
>> P.S. I am wondering do i2c/gpio virtio backends support Xen grant mappings
>> for the virtio?
> 
> Not yet, we haven't made much progress in that area until now, but it
> is very much part of what we intend to do.


Thanks for the information.

> 
>> Have you tried to run the backends in non-hardware domain
>> with CONFIG_XEN_VIRTIO=y in Linux?
> 
> Not yet.

ok

> 

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

[Oleksandr Tyshchenko]

On 05.12.22 13:29, Viresh Kumar wrote:

Hello Viresh

> On 05-12-22, 11:45, Viresh Kumar wrote:
>>>> +    rc = libxl__backendpath_parse_domid(gc, be_path, &virtio->backend_domid);
>>>> +    if (rc) goto out;
>>>> +
>>>> +    rc = libxl__parse_backend_path(gc, be_path, &dev);
>>>> +    if (rc) goto out;
>>>
>>> The same question for dev variable.
>>
>> Hmm, this we aren't using at all, which KBD does use it. Maybe we
>> should even call libxl__parse_backend_path() ?
> 
> Removing it works just fine for me.


Perfect. We will be able to add it when it is *really* needed.

> 

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

[Viresh Kumar]

On 06-12-22, 17:53, Oleksandr Tyshchenko wrote:
> On 05.12.22 11:11, Viresh Kumar wrote:
> > Maybe we can add these fields and a config param, once someone wants
> > to reuse this stuff for QEMU ?
> 
> 
> I don't know what to suggest here, sorry.
> 
> On the one hand, it is an extra work for you trying to add functionality you
> don't need at the moment. On the other hand if we add "backendtype" config
> param right now with default to STANDALONE it might simplify work for
> someone who ends up adding other type (in particular, the QEMU). Let's see
> what the maintainers will say.

The extra work is minimal and that isn't something that worries me.
What worries me, based on past experience, is adding code that _may_
be required in future, and is never used eventually. And for that I
always vouch for not adding something unless we are sure we need it
and there is some code which makes use of that configuration right
away.

-- 
viresh

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

[Anthony PERARD]

On Tue, Nov 08, 2022 at 04:53:59PM +0530, Viresh Kumar wrote:
> 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)

I don't think these ocaml changes are necessary, because they don't
build. I'm guessing those adds the ability to hotplug devices which
virtio device don't have, so function for that are missing.

> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 1b5381cef033..c6f35c069d2a 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2309,8 +2390,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;

These look a bit out of place, I think it would be fine to set
num_virtios and virtios just before calling parse_virtio_list(), as
array are usually initialised just before parsing the associated config
option in parse_config_data().

>      if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) {
>          while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != NULL) {
> @@ -2752,6 +2835,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);

Thanks,

-- 
Anthony PERARD

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

[Anthony PERARD]

Sorry, I've replied to the wrong version, but those comment apply to V7.

Cheers,

-- 
Anthony PERARD