| Message ID | 87d74a21bde95cfc7c53fad56bf8f0e47724953e.1592171394.git.gorbak25@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IGD Passthrough fixes for linux based stubdomains | expand |
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.
Grzegorz Uriasz writes ("[PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain"):
> 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.
Thanks.
I'm afraid I don't know much about this.
The code looks plausible, although there is a minor breach of official
libxl coding style in the use of `ret' rather than `r' for the xc
return values, and retuerning that value rather than a libxl error
code. I wouldn't regard that as a blocker considering the state of
the surrounding code.
I see from SUPPPORT.md that graphics passthrough seems to be security
supported. Frankly this seems very surprising to me.
Given that, I think we need a review from someone who understood
graphics passthrough.
I think that applies to all 3 of these patches.
Ian.
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; + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT; + uint64_t vga_iomem_npages = 0x20; + uint32_t stubdom_domid = libxl_get_stubdom_id(CTX, domid); + uint64_t vga_ioport_start[] = {0x3B0, 0x3C0}; + uint64_t vga_ioport_size[] = {0xC, 0x20}; + + /* 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; + } + 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++) { + 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; } - return 0; + if (vga_found) + ret = libxl__grant_legacy_vga_permissions(gc, domid); + if (ret < 0) + return ret; + if (igd_found) + ret = libxl__grant_igd_opregion_permission(gc, domid); + + return ret; } static int libxl_device_pci_compare(const libxl_device_pci *d1,
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(-)