| Message ID | 9817b73ea628c7ac86903bb9aa7fcfecf4f7b900.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: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
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,
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(+)