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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Dec 07, 2022 at 12:50:42PM +0530, Viresh Kumar wrote:
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index fa3d61f1e882..ab3668b3b8a3 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> +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)) {

So with `len` been the strlen() of `type`, I think that a type "v" or
"virtio" or even "virtio,device" is going to create the "i2c" node. So I
don't think is going to be possible to create a generic virtio device
node.

Using strcmp() would be good enough here I think.

> +        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 if (!strncmp(type, "virtio,device", len)) {

The example in in the commit message has "virtio,devices" but that would
be rejected by this test due to the extra 's'.

> +        /* Generic Virtio Device */
> +        res = fdt_end_node(fdt);

Isn't this an extra end_node() call? As there's already one for the
return of the function.

> +        if (res) return res;
> +    } else {
> +        LOG(ERROR, "Invalid type for virtio device: %s", type);
> +        return -EINVAL;
> +    }
> +
> +    return fdt_end_node(fdt);
> +}
> +
> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
> new file mode 100644
> index 000000000000..64cec989c674
> --- /dev/null
> +++ b/tools/libs/light/libxl_virtio.c
> +static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
> +                                       libxl_devid devid,
> +                                       libxl_device_virtio *virtio)
> +{
> +    const char *be_path, *tmp = NULL;
> +    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__backendpath_parse_domid(gc, be_path, &virtio->backend_domid);
> +    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);
> +    }
> +
> +    tmp = NULL;
> +    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);
> +    }
> +
> +    tmp = NULL;
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +                             GCSPRINTF("%s/transport", be_path), &tmp);
> +    if (rc) goto out;
> +
> +    if (tmp) {
> +        if (!strncmp(tmp, "mmio", strlen(tmp))) {

Maybe just use strcmp(), otherwise we have "m" going to be match as MMIO
for example.

> +            virtio->transport = LIBXL_VIRTIO_TRANSPORT_MMIO;
> +        } else if (!strncmp(tmp, "unknown", strlen(tmp))) {
> +            virtio->transport = LIBXL_VIRTIO_TRANSPORT_UNKNOWN;

I don't think that value should be allowed in xenstore. If `transport`
isn't set to a proper value (only "mmio"), then I think that invalid.

> +        } else {
> +            return ERROR_INVAL;
> +        }
> +    }
> +
> +    tmp = NULL;
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +                             GCSPRINTF("%s/type", be_path), &tmp);
> +    if (rc) goto out;
> +
> +    if (tmp) {
> +        if (!strncmp(tmp, "virtio,device", strlen("virtio,device"))) {

Nit: Something like:
    const char check[] = "virtio,device";
    const size_t checkl = sizeof(check) - 1;
    ... strncmp(tmp, check, checkl)...
    (or just strncmp(tmp, check, sizeof(check)-1))
would avoid issue with both string "virtio,device" potentially been
different.


> +            virtio->type = strdup(tmp);

Could you use libxl__strdup(NO_GC, tmp) instead? That function is going
to check that strdup() doesn't fail the allocation.


Thanks,

-- 
Anthony PERARD