Mailing List Archive

[RFC XEN PATCH v6 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi
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(-)

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
/*
--
2.34.1
Re: [RFC XEN PATCH v6 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi [ In reply to ]
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,

--
Anthony PERARD