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
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: + */
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 nodesdiff --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:
- */
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:
- 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() ?
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.
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.
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:
- 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.
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);
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);
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.
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) ])
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,
Sorry, I've replied to the wrong version, but those comment apply to V7.
Cheers,
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
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.... https://www.kernel.org/doc/Documentation/devicetree/bindings/gpio/gpio-virti... [2] https://www.kernel.org/doc/Documentation/devicetree/bindings/virtio/mmio.yam...
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):
- 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.
- 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 ?
- 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.
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):
- 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.
- 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.
- 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
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.
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.
stratos-dev@op-lists.linaro.org
-
Anthony PERARD -
Oleksandr Tyshchenko -
Viresh Kumar