Re: [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain

["Roger Pau Monné" <roger.pau@citrix.com>]

On Sun, Jun 14, 2020 at 10:12:01PM +0000, Grzegorz Uriasz wrote:
> When qemu is running inside a linux based stubdomain, qemu does not
> have the necessary permissions to map the ioports to the target domain.
> Currently, libxl is granting permissions only for the VGA RAM memory region
> and not passing the required ioports. This patch grants the required
> permission for the necessary vga io ports.
> 
> Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com>
> ---
>  tools/libxl/libxl_pci.c | 99 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 73 insertions(+), 26 deletions(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 957ff5c8e9..436190f790 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -2441,17 +2441,75 @@ void libxl__device_pci_destroy_all(libxl__egc *egc, uint32_t domid,
>      }
>  }
>  
> +static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t domid) {
> +    int ret, i;

Nit: i can be unsigned int since it's only used as a loop counter.

> +    uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
> +    uint64_t vga_iomem_npages = 0x20;

unsigned int is fine to store the size.

> +    uint32_t stubdom_domid = libxl_get_stubdom_id(CTX, domid);
> +    uint64_t vga_ioport_start[] = {0x3B0, 0x3C0};
> +    uint64_t vga_ioport_size[] = {0xC, 0x20};

For IO ports ranges/sizes you can safely use unsigned ints, those only
go up to 65535, and also constify the array since it's read-only.

Can you expand a bit on where those ranges are taken from?

Why not pass the whole 0x03B0-0x03DF range?

> +
> +    /* VGA RAM */
> +    ret = xc_domain_iomem_permission(CTX->xch, stubdom_domid,
> +                                     vga_iomem_start, vga_iomem_npages, 1);
> +    if (ret < 0) {
> +        LOGED(ERROR, domid,
> +              "failed to give stubdom%d access to iomem range "
> +              "%"PRIx64"-%"PRIx64" for VGA passthru",
> +              stubdom_domid,
> +              vga_iomem_start, (vga_iomem_start + (vga_iomem_npages << XC_PAGE_SHIFT) - 1));
> +        return ret;

I think it would be better to return a libxl error code here:
ERROR_FAIL. You already log the error code, and libxl functions have
their own set of error codes.

> +    }
> +    ret = xc_domain_iomem_permission(CTX->xch, domid,
> +                                     vga_iomem_start, vga_iomem_npages, 1);
> +    if (ret < 0) {
> +        LOGED(ERROR, domid,
> +              "failed to give dom%d access to iomem range "
> +              "%"PRIx64"-%"PRIx64" for VGA passthru",
> +              domid, vga_iomem_start, (vga_iomem_start + (vga_iomem_npages << XC_PAGE_SHIFT) - 1));
> +        return ret;
> +    }
> +
> +    /* VGA IOPORTS */
> +    for (i = 0 ; i < 2 ; i++) {

Please use ARRAY_SIZE(vga_ioport_start) here. And I would also assert
that both vga_ioport_start and vga_ioport_size arrays have the same
sizes before the loop.

> +        ret = xc_domain_ioport_permission(CTX->xch, stubdom_domid,
> +                                          vga_ioport_start[i], vga_ioport_size[i], 1);
> +        if (ret < 0) {
> +            LOGED(ERROR, domid,
> +                  "failed to give stubdom%d access to ioport range "
> +                  "%"PRIx64"-%"PRIx64" for VGA passthru",
> +                  stubdom_domid,
> +                  vga_ioport_start[i], (vga_ioport_start[i] + vga_ioport_size[i] - 1));
> +            return ret;
> +        }
> +        ret = xc_domain_ioport_permission(CTX->xch, domid,
> +                                          vga_ioport_start[i], vga_ioport_size[i], 1);
> +        if (ret < 0) {
> +            LOGED(ERROR, domid,
> +                  "failed to give dom%d access to ioport range "
> +                  "%"PRIx64"-%"PRIx64" for VGA passthru",
> +                  domid, vga_ioport_start[i], (vga_ioport_start[i] + vga_ioport_size[i] - 1));
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static int libxl__grant_igd_opregion_permission(libxl__gc *gc, const uint32_t domid) {
> +    return 0;
> +}
> +
>  int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid,
>                                        libxl_domain_config *const d_config)
>  {
> -    int i, ret;
> +    int i, ret = 0;
> +    bool vga_found = false, igd_found = false;
>  
>      if (!libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru))
>          return 0;
>  
> -    for (i = 0 ; i < d_config->num_pcidevs ; i++) {
> -        uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
> -        uint32_t stubdom_domid;
> +    for (i = 0 ; i < d_config->num_pcidevs && !igd_found ; i++) {
>          libxl_device_pci *pcidev = &d_config->pcidevs[i];
>          unsigned long pci_device_class;
>  
> @@ -2460,30 +2518,19 @@ int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid,
>          if (pci_device_class != 0x030000) /* VGA class */
>              continue;
>  
> -        stubdom_domid = libxl_get_stubdom_id(CTX, domid);
> -        ret = xc_domain_iomem_permission(CTX->xch, stubdom_domid,
> -                                         vga_iomem_start, 0x20, 1);
> -        if (ret < 0) {
> -            LOGED(ERROR, domid,
> -                  "failed to give stubdom%d access to iomem range "
> -                  "%"PRIx64"-%"PRIx64" for VGA passthru",
> -                  stubdom_domid,
> -                  vga_iomem_start, (vga_iomem_start + 0x20 - 1));
> -            return ret;
> -        }
> -        ret = xc_domain_iomem_permission(CTX->xch, domid,
> -                                         vga_iomem_start, 0x20, 1);
> -        if (ret < 0) {
> -            LOGED(ERROR, domid,
> -                  "failed to give dom%d access to iomem range "
> -                  "%"PRIx64"-%"PRIx64" for VGA passthru",
> -                  domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
> -            return ret;
> -        }
> -        break;
> +        vga_found = true;
> +        if (pcidev->bus == 0 && pcidev->dev == 2 && pcidev->func == 0)
> +            igd_found = true;

Could you check that the vendor is Intel also using
sysfs_dev_get_vendor?

Or else this could trigger on AMD boxes if they happen to have a VGA
PCI device at 00:2.0.

Thanks, Roger.