[PATCH 0/3] IGD Passthrough fixes for linux based stubdomains

[PATCH 0/3] IGD Passthrough fixes for linux based stubdomains

[Grzegorz Uriasz]

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

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

[Grzegorz Uriasz]

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

[PATCH 2/3] tools/libxl: Grant permission for mapping opregions to the target domain

[Grzegorz Uriasz]

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

[PATCH 3/3] tools/libxl: Directly map VBIOS to stubdomain

[Grzegorz Uriasz]

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

Re: [PATCH 3/3] tools/libxl: Directly map VBIOS to stubdomain

[Roger Pau Monné]

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

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

[Roger Pau Monné]

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.

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

[Ian Jackson]

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.

Re: [PATCH 2/3] tools/libxl: Grant permission for mapping opregions to the target domain

[Roger Pau Monné]

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.