Re: [PATCH] xen: Fix host pci for stubdom

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Sun, Mar 19, 2023 at 08:05:54PM -0400, Jason Andryuk wrote:
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 8c6e9a1716..51a72b432d 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -33,13 +34,101 @@
>  #define IORESOURCE_PREFETCH     0x00001000      /* No side effects */
>  #define IORESOURCE_MEM_64       0x00100000
>  
> +/*
> + * Non-passthrough (dom0) accesses are local PCI devices and use the given 
> BDF
> + * Passthough (stubdom) accesses are through PV frontend PCI device.  Those
> + * either have a BDF identical to the backend's BFD 
> (xen-backend.passthrough=1)
> + * or a local virtual BDF (xen-backend.passthrough=0)
> + *
> + * We are always given the backend's BDF and need to lookup the appropriate
> + * local BDF for sysfs access.
> + */
> +static void xen_host_pci_fill_local_addr(XenHostPCIDevice *d, Error **errp)
> +{
> +    unsigned int num_devs, len, i;
> +    unsigned int domain, bus, dev, func;
> +    char *be_path;
> +    char path[80];
> +    char *msg;
> +
> +    be_path = qemu_xen_xs_read(xenstore, 0, "device/pci/0/backend", &len);
> +    if (!be_path) {
> +        /*
> +         * be_path doesn't exist, so we are dealing with a local
> +         * (non-passthough) device.
> +         */
> +        d->local_domain = d->domain;
> +        d->local_bus = d->bus;
> +        d->local_dev = d->dev;
> +        d->local_func = d->func;
> +
> +        return;
> +    }
> +
> +    snprintf(path, sizeof(path), "%s/num_devs", be_path);

Is 80 bytes for `path` enough?
What if the path is truncated due to the limit?


There's xs_node_scanf() which might be useful. It does the error
handling and call scanf(). But I'm not sure if it can be used here, in
this file.

> +    msg = qemu_xen_xs_read(xenstore, 0, path, &len);
> +    if (!msg) {
> +        goto err_out;
> +    }
> +
> +    if (sscanf(msg, "%u", &num_devs) != 1) {

libxl writes `num_devs` as "%d". So I think qemu should read a %d.


> +        error_setg(errp, "Failed to parse %s (%s)", msg, path);
> +        goto err_out;
> +    }
> +    free(msg);
> +
> +    for (i = 0; i < num_devs; i++) {
> +        snprintf(path, sizeof(path), "%s/dev-%u", be_path, i);

Same here, the path is written with a %d, even if that doesn't change the
result.


Thanks,

-- 
Anthony PERARD