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

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

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