|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain
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@xxxxxxxxx>
> ---
> 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.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |