diff mbox series

[RFC,XEN,v6,5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

Message ID 20240328063402.354496-6-Jiqian.Chen@amd.com (mailing list archive)
State Superseded
Headers show
Series Support device passthrough when dom0 is PVH on Xen | expand

Commit Message

Chen, Jiqian March 28, 2024, 6:34 a.m. UTC 
Some type of domain don't have PIRQ, like PVH, when
passthrough a device to guest on PVH dom0, callstack
pci_add_dm_done->XEN_DOMCTL_irq_permission will failed
at domain_pirq_to_irq.

So, add a new hypercall to grant/revoke gsi permission
when dom0 is not PV or dom0 has not PIRQ flag.

Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 tools/include/xenctrl.h      |  5 ++++
 tools/libs/ctrl/xc_domain.c  | 15 +++++++++++
 tools/libs/light/libxl_pci.c | 52 +++++++++++++++++++++++++++++-------
 xen/arch/x86/domctl.c        | 31 +++++++++++++++++++++
 xen/include/public/domctl.h  |  9 +++++++
 xen/xsm/flask/hooks.c        |  1 +
 6 files changed, 103 insertions(+), 10 deletions(-)

Comments

Anthony PERARD April 3, 2024, 2:48 p.m. UTC | #1 
On Thu, Mar 28, 2024 at 02:34:02PM +0800, Jiqian Chen wrote:
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 2cec83e0b734..debf6ec6ddc7 100644
> @@ -1500,13 +1510,25 @@ static void pci_add_dm_done(libxl__egc *egc,
>              rc = ERROR_FAIL;
>              goto out;
>          }
> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
> -        if (r < 0) {
> -            LOGED(ERROR, domainid,
> -                  "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
> -            fclose(f);
> -            rc = ERROR_FAIL;
> -            goto out;
> +        if (is_gsi) {
> +            r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
> +            if (r < 0 && r != -EOPNOTSUPP) {

Is it correct to check `r` for "-EOPNOTSUPP" ? Because `man ioctl` and
"xenctrl.h:105" says that `r` is 0 on success or -1 on error with `errno`
set.

> +                LOGED(ERROR, domainid,
> +                      "xc_domain_gsi_permission gsi=%d (error=%d)", gsi, r);

Is the `r` value useful to print? Because LOGED() already prints
strerror(errno).

> +                fclose(f);
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
> +        }
> +        if (!is_gsi || r == -EOPNOTSUPP) {
> +            r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
> +            if (r < 0) {
> +                LOGED(ERROR, domainid,
> +                    "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
> +                fclose(f);
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
>          }
>      }
>      fclose(f);
> @@ -2180,6 +2202,7 @@ static void pci_remove_detached(libxl__egc *egc,
>      FILE *f;
>      uint32_t domainid = prs->domid;
>      bool isstubdom;
> +    bool is_gsi = true;
>  
>      /* Convenience aliases */
>      libxl_device_pci *const pci = &prs->pci;
> @@ -2243,6 +2266,7 @@ skip_bar:
>          /* To compitable with old version of kernel, still need to use irq */
>          sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>                                 pci->bus, pci->dev, pci->func);
> +        is_gsi = false;
>      }
>      f = fopen(sysfs_path, "r");
>      if (f == NULL) {
> @@ -2261,9 +2285,17 @@ skip_bar:
>               */
>              LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
>          }
> -        rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
> -        if (rc < 0) {
> -            LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
> +        if (is_gsi) {
> +            rc = xc_domain_gsi_permission(ctx->xch, domid, irq, 0);

Could you store the return code of this system call into `r` instead?
`rc` is supposed to be use exclusively for libxl error code, so the
current code is wrong, but we can partially fixed that in your patch.

> +            if (rc < 0 && rc != -EOPNOTSUPP) {

Shouldn't you check EOPNOTSUPP in `errno` instead?

> +                LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d", irq);
> +            }
> +        }
> +        if (!is_gsi || rc == -EOPNOTSUPP) {
> +            rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);

Same here, use `r` instead of `rc`.

> +            if (rc < 0) {
> +                LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
> +            }
>          }
>      }
>  

Thanks,
diff mbox series

Patch

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2ef8b4e05422..519c860a00d5 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1382,6 +1382,11 @@  int xc_domain_irq_permission(xc_interface *xch,
                              uint32_t pirq,
                              bool allow_access);
 
+int xc_domain_gsi_permission(xc_interface *xch,
+                             uint32_t domid,
+                             uint32_t gsi,
+                             bool allow_access);
+
 int xc_domain_iomem_permission(xc_interface *xch,
                                uint32_t domid,
                                unsigned long first_mfn,
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index f2d9d14b4d9f..8540e84fda93 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -1394,6 +1394,21 @@  int xc_domain_irq_permission(xc_interface *xch,
     return do_domctl(xch, &domctl);
 }
 
+int xc_domain_gsi_permission(xc_interface *xch,
+                             uint32_t domid,
+                             uint32_t gsi,
+                             bool allow_access)
+{
+    struct xen_domctl domctl = {
+        .cmd = XEN_DOMCTL_gsi_permission,
+        .domain = domid,
+        .u.gsi_permission.gsi = gsi,
+        .u.gsi_permission.allow_access = allow_access,
+    };
+
+    return do_domctl(xch, &domctl);
+}
+
 int xc_domain_iomem_permission(xc_interface *xch,
                                uint32_t domid,
                                unsigned long first_mfn,
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 2cec83e0b734..debf6ec6ddc7 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1421,6 +1421,8 @@  static void pci_add_dm_done(libxl__egc *egc,
     uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
     uint32_t domainid = domid;
     bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
+    int gsi;
+    bool is_gsi = true;
 
     /* Convenience aliases */
     bool starting = pas->starting;
@@ -1485,6 +1487,7 @@  static void pci_add_dm_done(libxl__egc *egc,
         /* To compitable with old version of kernel, still need to use irq */
         sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                                pci->bus, pci->dev, pci->func);
+        is_gsi = false;
     }
     f = fopen(sysfs_path, "r");
     if (f == NULL) {
@@ -1492,6 +1495,13 @@  static void pci_add_dm_done(libxl__egc *egc,
         goto out_no_irq;
     }
     if ((fscanf(f, "%u", &irq) == 1) && irq) {
+        /*
+         * If use gsi, save the value, because the value of irq
+         * will be changed by function xc_physdev_map_pirq
+         */
+        if (is_gsi) {
+            gsi = irq;
+        }
         r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
         if (r < 0) {
             LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
@@ -1500,13 +1510,25 @@  static void pci_add_dm_done(libxl__egc *egc,
             rc = ERROR_FAIL;
             goto out;
         }
-        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
-        if (r < 0) {
-            LOGED(ERROR, domainid,
-                  "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
-            fclose(f);
-            rc = ERROR_FAIL;
-            goto out;
+        if (is_gsi) {
+            r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
+            if (r < 0 && r != -EOPNOTSUPP) {
+                LOGED(ERROR, domainid,
+                      "xc_domain_gsi_permission gsi=%d (error=%d)", gsi, r);
+                fclose(f);
+                rc = ERROR_FAIL;
+                goto out;
+            }
+        }
+        if (!is_gsi || r == -EOPNOTSUPP) {
+            r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
+            if (r < 0) {
+                LOGED(ERROR, domainid,
+                    "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
+                fclose(f);
+                rc = ERROR_FAIL;
+                goto out;
+            }
         }
     }
     fclose(f);
@@ -2180,6 +2202,7 @@  static void pci_remove_detached(libxl__egc *egc,
     FILE *f;
     uint32_t domainid = prs->domid;
     bool isstubdom;
+    bool is_gsi = true;
 
     /* Convenience aliases */
     libxl_device_pci *const pci = &prs->pci;
@@ -2243,6 +2266,7 @@  skip_bar:
         /* To compitable with old version of kernel, still need to use irq */
         sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                                pci->bus, pci->dev, pci->func);
+        is_gsi = false;
     }
     f = fopen(sysfs_path, "r");
     if (f == NULL) {
@@ -2261,9 +2285,17 @@  skip_bar:
              */
             LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
         }
-        rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
-        if (rc < 0) {
-            LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
+        if (is_gsi) {
+            rc = xc_domain_gsi_permission(ctx->xch, domid, irq, 0);
+            if (rc < 0 && rc != -EOPNOTSUPP) {
+                LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d", irq);
+            }
+        }
+        if (!is_gsi || rc == -EOPNOTSUPP) {
+            rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
+            if (rc < 0) {
+                LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
+            }
         }
     }
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9a72d57333e9..9b8a08b2a81d 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -237,6 +237,37 @@  long arch_do_domctl(
         break;
     }
 
+    case XEN_DOMCTL_gsi_permission:
+    {
+        unsigned int gsi = domctl->u.gsi_permission.gsi;
+        int allow = domctl->u.gsi_permission.allow_access;
+
+        if ( is_pv_domain(current->domain) || has_pirq(current->domain) )
+        {
+            ret = -EOPNOTSUPP;
+            break;
+        }
+
+        if ( gsi >= nr_irqs_gsi )
+        {
+            ret = -EINVAL;
+            break;
+        }
+
+        if ( !irq_access_permitted(current->domain, gsi) ||
+             xsm_irq_permission(XSM_HOOK, d, gsi, allow) )
+        {
+            ret = -EPERM;
+            break;
+        }
+
+        if ( allow )
+            ret = irq_permit_access(d, gsi);
+        else
+            ret = irq_deny_access(d, gsi);
+        break;
+    }
+
     case XEN_DOMCTL_getpageframeinfo3:
     {
         unsigned int num = domctl->u.getpageframeinfo3.num;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index a33f9ec32b08..47e95f9ee824 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -447,6 +447,13 @@  struct xen_domctl_irq_permission {
 };
 
 
+/* XEN_DOMCTL_gsi_permission */
+struct xen_domctl_gsi_permission {
+    uint32_t gsi;
+    uint8_t allow_access;    /* flag to specify enable/disable of x86 gsi access */
+};
+
+
 /* XEN_DOMCTL_iomem_permission */
 struct xen_domctl_iomem_permission {
     uint64_aligned_t first_mfn;/* first page (physical page number) in range */
@@ -1277,6 +1284,7 @@  struct xen_domctl {
 #define XEN_DOMCTL_vmtrace_op                    84
 #define XEN_DOMCTL_get_paging_mempool_size       85
 #define XEN_DOMCTL_set_paging_mempool_size       86
+#define XEN_DOMCTL_gsi_permission                87
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1299,6 +1307,7 @@  struct xen_domctl {
         struct xen_domctl_setdomainhandle   setdomainhandle;
         struct xen_domctl_setdebugging      setdebugging;
         struct xen_domctl_irq_permission    irq_permission;
+        struct xen_domctl_gsi_permission    gsi_permission;
         struct xen_domctl_iomem_permission  iomem_permission;
         struct xen_domctl_ioport_permission ioport_permission;
         struct xen_domctl_hypercall_init    hypercall_init;
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 78225f68c15c..376076865198 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -685,6 +685,7 @@  static int cf_check flask_domctl(struct domain *d, int cmd)
     case XEN_DOMCTL_shadow_op:
     case XEN_DOMCTL_ioport_permission:
     case XEN_DOMCTL_ioport_mapping:
+    case XEN_DOMCTL_gsi_permission:
 #endif
 #ifdef CONFIG_HAS_PASSTHROUGH
     /*