[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V6 1/3] libxl: Add support for generic virtio device
- To: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
- From: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>
- Date: Tue, 6 Dec 2022 16:06:30 +0200
- Cc: Vincent Guittot <vincent.guittot@xxxxxxxxxx>, stratos-dev@xxxxxxxxxxxxxxxxxxx, Alex Bennée <alex.bennee@xxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>, Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>, Mike Holmes <mike.holmes@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxx, Julien Grall <julien@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>
- Delivery-date: Tue, 06 Dec 2022 14:07:00 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
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.
|
