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.