* [PATCH 0/3] IGD Passthrough fixes for linux based stubdomains @ 2020-06-14 22:12 Grzegorz Uriasz 2020-06-14 22:12 ` [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain Grzegorz Uriasz ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Grzegorz Uriasz @ 2020-06-14 22:12 UTC (permalink / raw) To: xen-devel Cc: Wei Liu, jakub, Ian Jackson, marmarek, Grzegorz Uriasz, j.nowak26, Anthony PERARD, contact Hi, The included patches are a small subset of a bigger patch set spanning few projects aiming to isolate the GPU in Qubes OS to a dedicated security domain. I'm doing this together with 3 colleagues as part of our Bachelors thesis. Right now qemu assumes it runs in dom0 so it may grant access to required memory regions and ioports to the target domain for the IGD to work. This is no longer the case with linux based stubdomains as the stubdom is not privileged. Moving some logic from qemu to libxl is necessary for some features to work inside a stubdomain. The included patches were tested on a few laptops(together with the linked qemu patchset) and they work fine. Grzegorz Uriasz (3): tools/libxl: Grant VGA IO port permission for stubdom/target domain tools/libxl: Grant permission for mapping opregions to the target domain tools/libxl: Directly map VBIOS to stubdomain tools/libxl/libxl_pci.c | 153 +++++++++++++++++++++++++++++++++------- 1 file changed, 127 insertions(+), 26 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain 2020-06-14 22:12 [PATCH 0/3] IGD Passthrough fixes for linux based stubdomains Grzegorz Uriasz @ 2020-06-14 22:12 ` Grzegorz Uriasz 2020-06-15 11:17 ` Roger Pau Monné 2020-06-15 11:32 ` Ian Jackson 2020-06-14 22:12 ` [PATCH 2/3] tools/libxl: Grant permission for mapping opregions to the target domain Grzegorz Uriasz 2020-06-14 22:12 ` [PATCH 3/3] tools/libxl: Directly map VBIOS to stubdomain Grzegorz Uriasz 2 siblings, 2 replies; 8+ messages in thread From: Grzegorz Uriasz @ 2020-06-14 22:12 UTC (permalink / raw) To: xen-devel Cc: Wei Liu, jakub, Ian Jackson, marmarek, Grzegorz Uriasz, j.nowak26, Anthony PERARD, contact 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; + 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, -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain 2020-06-14 22:12 ` [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain Grzegorz Uriasz @ 2020-06-15 11:17 ` Roger Pau Monné 2020-06-15 11:32 ` Ian Jackson 1 sibling, 0 replies; 8+ messages in thread From: Roger Pau Monné @ 2020-06-15 11:17 UTC (permalink / raw) To: Grzegorz Uriasz Cc: Wei Liu, jakub, Ian Jackson, marmarek, Anthony PERARD, j.nowak26, xen-devel, contact 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain 2020-06-14 22:12 ` [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain Grzegorz Uriasz 2020-06-15 11:17 ` Roger Pau Monné @ 2020-06-15 11:32 ` Ian Jackson 1 sibling, 0 replies; 8+ messages in thread From: Ian Jackson @ 2020-06-15 11:32 UTC (permalink / raw) To: Grzegorz Uriasz Cc: Wei Liu, jakub, marmarek, j.nowak26, Anthony Perard, xen-devel, contact 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] tools/libxl: Grant permission for mapping opregions to the target domain 2020-06-14 22:12 [PATCH 0/3] IGD Passthrough fixes for linux based stubdomains Grzegorz Uriasz 2020-06-14 22:12 ` [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain Grzegorz Uriasz @ 2020-06-14 22:12 ` Grzegorz Uriasz 2020-06-15 12:44 ` Roger Pau Monné 2020-06-14 22:12 ` [PATCH 3/3] tools/libxl: Directly map VBIOS to stubdomain Grzegorz Uriasz 2 siblings, 1 reply; 8+ messages in thread From: Grzegorz Uriasz @ 2020-06-14 22:12 UTC (permalink / raw) To: xen-devel Cc: Wei Liu, jakub, Ian Jackson, marmarek, Grzegorz Uriasz, j.nowak26, Anthony PERARD, contact IGD VGA devices require a special opregion MMIO region which functions as an extra BAR in the PCI configuration space. Right now qemu is assigning those permissions - this is failing inside a linux based stubdomain as the stubdomain is not privileged. This patch grants the permissions for opregions in libxl instead of qemu. Granting permissions in qemu may be removed when this patch get's merged - for now linux based stubdomains which use IGD's need to patch qemu and include this patch in xen for IGD passthrough to work. Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com> --- tools/libxl/libxl_pci.c | 46 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c index 436190f790..48b1d8073b 100644 --- a/tools/libxl/libxl_pci.c +++ b/tools/libxl/libxl_pci.c @@ -2497,7 +2497,51 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom } static int libxl__grant_igd_opregion_permission(libxl__gc *gc, const uint32_t domid) { - return 0; + char* sysfs_path; + FILE* f; + uint32_t igd_host_opregion; + int ret = 0; + uint32_t stubdom_domid = libxl_get_stubdom_id(CTX, domid); + + sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/config", 0, 0, 2, 0); + f = fopen(sysfs_path, "r"); + if (!f) { + LOGED(ERROR, domid, "Unable to access IGD config space"); + return ERROR_FAIL; + } + + ret = fseek(f, 0xfc, SEEK_SET); + if (ret < 0) { + LOGED(ERROR, domid, "Unable to lseek in PCI config space"); + goto out; + } + + ret = fread((void*)&igd_host_opregion, 4, 1, f); + if (ret < 0) { + LOGED(ERROR, domid, "Unable to read opregion register"); + goto out; + } + + ret = xc_domain_iomem_permission(CTX->xch, stubdom_domid, + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), 0x3, 1); + if (ret < 0) { + LOGED(ERROR, domid, + "failed to give stubdom%d access to %"PRIx32" opregions for igd passthrough", stubdom_domid, igd_host_opregion); + goto out; + } + + ret = xc_domain_iomem_permission(CTX->xch, domid, + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), 0x3, 1); + if (ret < 0) { + LOGED(ERROR, domid, + "failed to give dom%d access to %"PRIx32" opregions for igd passthrough", domid, igd_host_opregion); + goto out; + } + + out: + if(f) + fclose(f); + return ret; } int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid, -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] tools/libxl: Grant permission for mapping opregions to the target domain 2020-06-14 22:12 ` [PATCH 2/3] tools/libxl: Grant permission for mapping opregions to the target domain Grzegorz Uriasz @ 2020-06-15 12:44 ` Roger Pau Monné 0 siblings, 0 replies; 8+ messages in thread From: Roger Pau Monné @ 2020-06-15 12:44 UTC (permalink / raw) To: Grzegorz Uriasz Cc: Wei Liu, jakub, Ian Jackson, marmarek, Anthony PERARD, j.nowak26, xen-devel, contact On Sun, Jun 14, 2020 at 10:12:02PM +0000, Grzegorz Uriasz wrote: > IGD VGA devices require a special opregion MMIO region which functions as > an extra BAR in the PCI configuration space. Right now qemu is assigning those > permissions - this is failing inside a linux based stubdomain as the > stubdomain is not privileged. This patch grants the permissions for > opregions in libxl instead of qemu. Granting permissions in qemu may be removed > when this patch get's merged - for now linux based stubdomains which use IGD's > need to patch qemu and include this patch in xen for IGD passthrough to work. > > Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com> Thanks for the patch! > --- > tools/libxl/libxl_pci.c | 46 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 45 insertions(+), 1 deletion(-) > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > index 436190f790..48b1d8073b 100644 > --- a/tools/libxl/libxl_pci.c > +++ b/tools/libxl/libxl_pci.c > @@ -2497,7 +2497,51 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom > } > > static int libxl__grant_igd_opregion_permission(libxl__gc *gc, const uint32_t domid) { > - return 0; > + char* sysfs_path; > + FILE* f; > + uint32_t igd_host_opregion; > + int ret = 0; No need to init ret. > + uint32_t stubdom_domid = libxl_get_stubdom_id(CTX, domid); > + > + sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/config", 0, 0, 2, 0); > + f = fopen(sysfs_path, "r"); > + if (!f) { > + LOGED(ERROR, domid, "Unable to access IGD config space"); > + return ERROR_FAIL; > + } > + > + ret = fseek(f, 0xfc, SEEK_SET); > + if (ret < 0) { > + LOGED(ERROR, domid, "Unable to lseek in PCI config space"); > + goto out; > + } > + > + ret = fread((void*)&igd_host_opregion, 4, 1, f); > + if (ret < 0) { > + LOGED(ERROR, domid, "Unable to read opregion register"); > + goto out; > + } > + > + ret = xc_domain_iomem_permission(CTX->xch, stubdom_domid, > + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), 0x3, 1); > + if (ret < 0) { > + LOGED(ERROR, domid, > + "failed to give stubdom%d access to %"PRIx32" opregions for igd passthrough", stubdom_domid, igd_host_opregion); > + goto out; > + } I think you only need to do this if there's a stubdomain? If stubdom_domid is 0 then you don't need to do this. Also, I'm not sure hardcoding the size is correct, AFAICT the size should be fetched from the OpRegion Header size field? The specification I'm reading for IGD OpRegion for Skylake processors mentions the size of the OpRegion is 8KiB (so 2 pages). > + > + ret = xc_domain_iomem_permission(CTX->xch, domid, > + (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT), 0x3, 1); > + if (ret < 0) { > + LOGED(ERROR, domid, > + "failed to give dom%d access to %"PRIx32" opregions for igd passthrough", domid, igd_host_opregion); > + goto out; > + } > + > + out: You should remove the leading spaces on the label. > + if(f) No need for the 'if (f)', since all code paths leading here will have f != NULL. > + fclose(f); > + return ret; I think you want to return ERROR_FAIL if ret, ie: return ret ? ERROR_FAIL : 0; Thanks, Roger. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] tools/libxl: Directly map VBIOS to stubdomain 2020-06-14 22:12 [PATCH 0/3] IGD Passthrough fixes for linux based stubdomains Grzegorz Uriasz 2020-06-14 22:12 ` [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain Grzegorz Uriasz 2020-06-14 22:12 ` [PATCH 2/3] tools/libxl: Grant permission for mapping opregions to the target domain Grzegorz Uriasz @ 2020-06-14 22:12 ` Grzegorz Uriasz 2020-06-15 10:26 ` Roger Pau Monné 2 siblings, 1 reply; 8+ messages in thread From: Grzegorz Uriasz @ 2020-06-14 22:12 UTC (permalink / raw) To: xen-devel Cc: Wei Liu, jakub, Ian Jackson, marmarek, Grzegorz Uriasz, j.nowak26, Anthony PERARD, contact When passing through a IGD VGA device qemu needs to copy the host VBIOS to the target domain. Right now the current implementation on the qemu side is one big undefined behavior as described in my qemu patchset here: https://patchew.org/QEMU/20200428062847.7764-1-gorbak25@gmail.com/ This patch is tied to the linked patchset for qemu but fortunately this patch still works without the qemu part merged. When the qemu part gets merged then qemu will access the VBIOS using /dev/mem - this is required as currently the linux kernel forbids accessing this memory region when the VBIOS is corrupted - which will always be the case as described in the linked patchset. When qemu is running inside a linux based stubdomain then the stubdomain does not have access to the VBIOS. This patch maps the VBIOS to the stubdomain so qemu with my fixes may create a shadow copy for the target domain. Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com> --- tools/libxl/libxl_pci.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c index 48b1d8073b..9b9564dd73 100644 --- a/tools/libxl/libxl_pci.c +++ b/tools/libxl/libxl_pci.c @@ -2445,6 +2445,8 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom int ret, i; uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT; uint64_t vga_iomem_npages = 0x20; + uint64_t vga_vbios_start = 0xc0000 >> XC_PAGE_SHIFT; + uint64_t vga_vbios_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}; @@ -2460,6 +2462,7 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom 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) { @@ -2470,6 +2473,13 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom return ret; } + /* VGA ROM */ + ret = xc_domain_memory_mapping(CTX->xch, stubdom_domid, vga_vbios_start, vga_vbios_start, vga_vbios_npages, DPCI_ADD_MAPPING); + if (ret < 0) { + LOGED(ERROR, domid, "failed to map VBIOS to stubdom%d", stubdom_domid); + return ret; + } + /* VGA IOPORTS */ for (i = 0 ; i < 2 ; i++) { ret = xc_domain_ioport_permission(CTX->xch, stubdom_domid, -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] tools/libxl: Directly map VBIOS to stubdomain 2020-06-14 22:12 ` [PATCH 3/3] tools/libxl: Directly map VBIOS to stubdomain Grzegorz Uriasz @ 2020-06-15 10:26 ` Roger Pau Monné 0 siblings, 0 replies; 8+ messages in thread From: Roger Pau Monné @ 2020-06-15 10:26 UTC (permalink / raw) To: Grzegorz Uriasz Cc: Wei Liu, jakub, Ian Jackson, marmarek, Anthony PERARD, j.nowak26, xen-devel, contact On Sun, Jun 14, 2020 at 10:12:03PM +0000, Grzegorz Uriasz wrote: > When passing through a IGD VGA device qemu needs to copy the host VBIOS > to the target domain. Right now the current implementation on the qemu side > is one big undefined behavior as described in my qemu patchset here: > https://patchew.org/QEMU/20200428062847.7764-1-gorbak25@gmail.com/ I think it would be good to elaborate a bit more here on the issue, that link could go away and we still need to keep the reasoning behind this change in the Xen changelog IMO. > This patch is tied to the linked patchset for qemu but fortunately > this patch still works without the qemu part merged. When the qemu part > gets merged then qemu will access the VBIOS using /dev/mem - this is > required as currently the linux kernel forbids accessing this memory > region when the VBIOS is corrupted - which will always be the case as > described in the linked patchset. When qemu is running inside a linux > based stubdomain then the stubdomain does not have access to the VBIOS. > This patch maps the VBIOS to the stubdomain so qemu with my fixes may > create a shadow copy for the target domain. > > Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com> > --- > tools/libxl/libxl_pci.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > index 48b1d8073b..9b9564dd73 100644 > --- a/tools/libxl/libxl_pci.c > +++ b/tools/libxl/libxl_pci.c > @@ -2445,6 +2445,8 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom > int ret, i; > uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT; > uint64_t vga_iomem_npages = 0x20; > + uint64_t vga_vbios_start = 0xc0000 >> XC_PAGE_SHIFT; > + uint64_t vga_vbios_npages = 0x20; Is this always 32 pages? Some claim [0] it's 32KiB (so 8 pages). Wouldn't it be safer to fetch the size from sysfs or some other place if possible? > uint32_t stubdom_domid = libxl_get_stubdom_id(CTX, domid); > uint64_t vga_ioport_start[] = {0x3B0, 0x3C0}; > uint64_t vga_ioport_size[] = {0xC, 0x20}; > @@ -2460,6 +2462,7 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom > vga_iomem_start, (vga_iomem_start + (vga_iomem_npages << XC_PAGE_SHIFT) - 1)); > return ret; > } > + Stray change? > ret = xc_domain_iomem_permission(CTX->xch, domid, > vga_iomem_start, vga_iomem_npages, 1); > if (ret < 0) { > @@ -2470,6 +2473,13 @@ static int libxl__grant_legacy_vga_permissions(libxl__gc *gc, const uint32_t dom > return ret; > } > > + /* VGA ROM */ > + ret = xc_domain_memory_mapping(CTX->xch, stubdom_domid, vga_vbios_start, vga_vbios_start, vga_vbios_npages, DPCI_ADD_MAPPING); Line is too long, please limit lines to 75 characters (see libxl/CODING_STYLE). > + if (ret < 0) { > + LOGED(ERROR, domid, "failed to map VBIOS to stubdom%d", stubdom_domid); AFAICT you have to use LOGEVD here, since the errno value will be returned in 'ret'. Ie: if (ret < 0) { LOGED(ERROR, ret, domid, "failed to map VBIOS to stubdom%d", stubdom_domid); return ERROR_FAIL; } Thanks, Roger. [0] https://wiki.osdev.org/Memory_Map_(x86)#Overview ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-06-15 12:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-14 22:12 [PATCH 0/3] IGD Passthrough fixes for linux based stubdomains Grzegorz Uriasz 2020-06-14 22:12 ` [PATCH 1/3] tools/libxl: Grant VGA IO port permission for stubdom/target domain Grzegorz Uriasz 2020-06-15 11:17 ` Roger Pau Monné 2020-06-15 11:32 ` Ian Jackson 2020-06-14 22:12 ` [PATCH 2/3] tools/libxl: Grant permission for mapping opregions to the target domain Grzegorz Uriasz 2020-06-15 12:44 ` Roger Pau Monné 2020-06-14 22:12 ` [PATCH 3/3] tools/libxl: Directly map VBIOS to stubdomain Grzegorz Uriasz 2020-06-15 10:26 ` Roger Pau Monné
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).