diff mbox series

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

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

Commit Message

Jiqian Chen Jan. 12, 2024, 6:13 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.

Co-developed-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 | 16 ++++++++++++++--
 xen/arch/x86/domctl.c        | 31 +++++++++++++++++++++++++++++++
 xen/include/public/domctl.h  |  9 +++++++++
 xen/xsm/flask/hooks.c        |  1 +
 6 files changed, 75 insertions(+), 2 deletions(-)

Comments

Anthony PERARD Feb. 21, 2024, 3:03 p.m. UTC | #1
On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> index f2d9d14b4d9f..448ba2c59ae1 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 = {};
> +
> +    domctl.cmd = XEN_DOMCTL_gsi_permission;
> +    domctl.domain = domid;
> +    domctl.u.gsi_permission.gsi = gsi;
> +    domctl.u.gsi_permission.allow_access = allow_access;

This could be written as:
    struct xen_domctl domctl = {
        .cmd = XEN_DOMCTL_gsi_permission,
        .domain = domid,
        .u.gsi_permission.gsi = gsi,
        .u.gsi_permission.allow_access = allow_access,
    };

but your change is fine too.


> +
> +    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 a1c6e82631e9..4136a860a048 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 has_gsi = true;

Why is this function has "gsi" variable, and "gsi = irq" but the next
function pci_remove_detached() does only have "irq" variable?

So, gsi can only be positive integer, right? Could we forgo `has_gsi` and
just set "gsi = -1" when there's no gsi?

>      /* Convenience aliases */
>      bool starting = pas->starting;
> @@ -1482,6 +1484,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>                                  pci->bus, pci->dev, pci->func);
>  
>      if ( access(sysfs_path, F_OK) != 0 ) {
> +        has_gsi = false;
>          if ( errno == ENOENT )
>              sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>                                  pci->bus, pci->dev, pci->func);
> @@ -1497,6 +1500,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>          goto out_no_irq;
>      }
>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
> +        gsi = irq;

Why do you do this, that that mean that `gsi` and `irq` are the same? Or
does `irq` happen to sometime contain a `gsi` value?

>          r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
>          if (r < 0) {
>              LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
> @@ -1505,7 +1509,10 @@ 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 ( has_gsi )
> +            r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
> +        if ( !has_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);

Looks like this error message could be wrong, I think we want to know if
which call between xc_domain_irq_permission() and
xc_domain_gsi_permission() has failed.

> @@ -2185,6 +2192,7 @@ static void pci_remove_detached(libxl__egc *egc,
>      FILE *f;
>      uint32_t domainid = prs->domid;
>      bool isstubdom;
> +    bool has_gsi = true;
>  
>      /* Convenience aliases */
>      libxl_device_pci *const pci = &prs->pci;
> @@ -2244,6 +2252,7 @@ skip_bar:
>                             pci->bus, pci->dev, pci->func);
>  
>      if ( access(sysfs_path, F_OK) != 0 ) {
> +        has_gsi = false;
>          if ( errno == ENOENT )
>              sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>                                  pci->bus, pci->dev, pci->func);
> @@ -2270,7 +2279,10 @@ skip_bar:
>               */
>              LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
>          }
> -        rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
> +        if ( has_gsi )
> +            rc = xc_domain_gsi_permission(ctx->xch, domid, irq, 0);

Why do you use the xc_domain_gsi_permission() with "irq" here, but not
in pci_add_dm_done() ?

> +        if ( !has_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);
>          }


These changes to libxl are quite confusing, there's a mixed up between
"gsi" and "irq", it's hard to follow.

Thanks,
Jiqian Chen Feb. 22, 2024, 7:22 a.m. UTC | #2
Hi Anthony,

On 2024/2/21 23:03, Anthony PERARD wrote:
> On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
>> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
>> index f2d9d14b4d9f..448ba2c59ae1 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 = {};
>> +
>> +    domctl.cmd = XEN_DOMCTL_gsi_permission;
>> +    domctl.domain = domid;
>> +    domctl.u.gsi_permission.gsi = gsi;
>> +    domctl.u.gsi_permission.allow_access = allow_access;
> 
> This could be written as:
>     struct xen_domctl domctl = {
>         .cmd = XEN_DOMCTL_gsi_permission,
>         .domain = domid,
>         .u.gsi_permission.gsi = gsi,
>         .u.gsi_permission.allow_access = allow_access,
>     };
> 
That's fine, I will change to this in next version.

> but your change is fine too.
> 
> 
>> +
>> +    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 a1c6e82631e9..4136a860a048 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 has_gsi = true;
> 
> Why is this function has "gsi" variable, and "gsi = irq" but the next
> function pci_remove_detached() does only have "irq" variable?
Because in pci_add_dm_done, it calls " r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq); ", it passes the pointer of irq in, and then irq will be changed, so I need to use gsi to save the origin value.

> 
> So, gsi can only be positive integer, right? Could we forgo `has_gsi` and
> just set "gsi = -1" when there's no gsi?
No, we can't forgo 'has_gsi', because we need to set gsi = irq to save the original val.

> 
>>      /* Convenience aliases */
>>      bool starting = pas->starting;
>> @@ -1482,6 +1484,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>                                  pci->bus, pci->dev, pci->func);
>>  
>>      if ( access(sysfs_path, F_OK) != 0 ) {
>> +        has_gsi = false;
>>          if ( errno == ENOENT )
>>              sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>>                                  pci->bus, pci->dev, pci->func);
>> @@ -1497,6 +1500,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>          goto out_no_irq;
>>      }
>>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> +        gsi = irq;
> 
> Why do you do this, that that mean that `gsi` and `irq` are the same? Or
> does `irq` happen to sometime contain a `gsi` value?
Above reason.

> 
>>          r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
>>          if (r < 0) {
>>              LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
>> @@ -1505,7 +1509,10 @@ 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 ( has_gsi )
>> +            r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
>> +        if ( !has_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);
> 
> Looks like this error message could be wrong, I think we want to know if
> which call between xc_domain_irq_permission() and
> xc_domain_gsi_permission() has failed.
You are right, I will separate them in next version.

> 
>> @@ -2185,6 +2192,7 @@ static void pci_remove_detached(libxl__egc *egc,
>>      FILE *f;
>>      uint32_t domainid = prs->domid;
>>      bool isstubdom;
>> +    bool has_gsi = true;
>>  
>>      /* Convenience aliases */
>>      libxl_device_pci *const pci = &prs->pci;
>> @@ -2244,6 +2252,7 @@ skip_bar:
>>                             pci->bus, pci->dev, pci->func);
>>  
>>      if ( access(sysfs_path, F_OK) != 0 ) {
>> +        has_gsi = false;
>>          if ( errno == ENOENT )
>>              sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>>                                  pci->bus, pci->dev, pci->func);
>> @@ -2270,7 +2279,10 @@ skip_bar:
>>               */
>>              LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
>>          }
>> -        rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
>> +        if ( has_gsi )
>> +            rc = xc_domain_gsi_permission(ctx->xch, domid, irq, 0);
> 
> Why do you use the xc_domain_gsi_permission() with "irq" here, but not
> in pci_add_dm_done() ?
Because xc_physdev_unmap_pirq doesn't change the value of irq, but in pci_add_dm_done, the value of irq is changed by xc_physdev_map_pirq.

> 
>> +        if ( !has_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);
>>          }
> 
> 
> These changes to libxl are quite confusing, there's a mixed up between
> "gsi" and "irq", it's hard to follow.
> 
> Thanks,
>
Anthony PERARD Feb. 23, 2024, 3:59 p.m. UTC | #3
On Thu, Feb 22, 2024 at 07:22:45AM +0000, Chen, Jiqian wrote:
> On 2024/2/21 23:03, Anthony PERARD wrote:
> > On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
> >> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> >> index a1c6e82631e9..4136a860a048 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 has_gsi = true;
> > 
> > Why is this function has "gsi" variable, and "gsi = irq" but the next
> > function pci_remove_detached() does only have "irq" variable?
> Because in pci_add_dm_done, it calls " r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq); ", it passes the pointer of irq in, and then irq will be changed, so I need to use gsi to save the origin value.

I'm sorry but unconditional "gsi = irq" looks very wrong, that line
needs to be removed or changed, otherwise we have two functions doing the
same thing just after that (xc_domain_irq_permission and
xc_domain_gsi_permission). Instead, my guess is that the
arguments of xc_physdev_map_pirq() needs to be changes. What does
contain `irq` after the map_pirq() call? A "pirq" of some kind? Maybe
xc_physdev_map_pirq(ctx->xch, domid, irq, &pirq) would make things more
clear about what's going on.


And BTW, maybe renaming the variable "has_gsi" to "is_gsi" might make
things a bit clearer as well, as in: "the variable 'irq' $is_gsi",
instead of a variable that have a meaning of "the system $has_gsi"
without necessarily meaning that the code is using it.

Maybe using (abusing?) the variable name "irq" might be a bit wrong now?
What does "GSI" stand for anyway? And about "PIRQ"? This is just some
question to figure out if there's potentially a better name for the
variables names.

Thanks,
Jiqian Chen Feb. 26, 2024, 7:27 a.m. UTC | #4
On 2024/2/23 23:59, Anthony PERARD wrote:
> On Thu, Feb 22, 2024 at 07:22:45AM +0000, Chen, Jiqian wrote:
>> On 2024/2/21 23:03, Anthony PERARD wrote:
>>> On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
>>>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>>>> index a1c6e82631e9..4136a860a048 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 has_gsi = true;
>>>
>>> Why is this function has "gsi" variable, and "gsi = irq" but the next
>>> function pci_remove_detached() does only have "irq" variable?
>> Because in pci_add_dm_done, it calls " r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq); ", it passes the pointer of irq in, and then irq will be changed, so I need to use gsi to save the origin value.
> 
> I'm sorry but unconditional "gsi = irq" looks very wrong, that line
Make sense, I should add a condition and some comments here. Maybe:
	/* if has gsi sysfs node, then fscanf() read from the gsi sysfs and store it in irq variable, we need to save the original gsi value, because irq will be changed in xc_physdev_map_pirq */
	if (has_gsi)
		gsi = irq;

> needs to be removed or changed, otherwise we have two functions doing the
> same thing just after that (xc_domain_irq_permission and
> xc_domain_gsi_permission). Instead, my guess is that the
> arguments of xc_physdev_map_pirq() needs to be changes. What does
> contain `irq` after the map_pirq() call? A "pirq" of some kind? Maybe
Yes, pirq.

> xc_physdev_map_pirq(ctx->xch, domid, irq, &pirq) would make things more
> clear about what's going on.
> 
> 
> And BTW, maybe renaming the variable "has_gsi" to "is_gsi" might make
> things a bit clearer as well, as in: "the variable 'irq' $is_gsi",
> instead of a variable that have a meaning of "the system $has_gsi"
> without necessarily meaning that the code is using it.
Gsi is a new sysfs node added by my kernel patch, and it is still in discussion with PCI Maintainer.
And for compatible with old version of kernel that didn't has gsi sysfs node, we still need to use irq here.
So, I named this variable to has_gsi. Is it clearer that changing it to has_gsi_sysfs_node?
Maybe I need to add some comments in code to make the usage of gsi clear.

> 
> Maybe using (abusing?) the variable name "irq" might be a bit wrong now?
> What does "GSI" stand for anyway? And about "PIRQ"? This is just some
GSI is x86 specific concept, it is related to IOAPIC-PIN. PIRQ is used to route interrupts in Xen.

> question to figure out if there's potentially a better name for the
> variables names.
> 
> 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..448ba2c59ae1 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 = {};
+
+    domctl.cmd = XEN_DOMCTL_gsi_permission;
+    domctl.domain = domid;
+    domctl.u.gsi_permission.gsi = gsi;
+    domctl.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 a1c6e82631e9..4136a860a048 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 has_gsi = true;
 
     /* Convenience aliases */
     bool starting = pas->starting;
@@ -1482,6 +1484,7 @@  static void pci_add_dm_done(libxl__egc *egc,
                                 pci->bus, pci->dev, pci->func);
 
     if ( access(sysfs_path, F_OK) != 0 ) {
+        has_gsi = false;
         if ( errno == ENOENT )
             sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                                 pci->bus, pci->dev, pci->func);
@@ -1497,6 +1500,7 @@  static void pci_add_dm_done(libxl__egc *egc,
         goto out_no_irq;
     }
     if ((fscanf(f, "%u", &irq) == 1) && irq) {
+        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)",
@@ -1505,7 +1509,10 @@  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 ( has_gsi )
+            r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
+        if ( !has_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);
@@ -2185,6 +2192,7 @@  static void pci_remove_detached(libxl__egc *egc,
     FILE *f;
     uint32_t domainid = prs->domid;
     bool isstubdom;
+    bool has_gsi = true;
 
     /* Convenience aliases */
     libxl_device_pci *const pci = &prs->pci;
@@ -2244,6 +2252,7 @@  skip_bar:
                            pci->bus, pci->dev, pci->func);
 
     if ( access(sysfs_path, F_OK) != 0 ) {
+        has_gsi = false;
         if ( errno == ENOENT )
             sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                                 pci->bus, pci->dev, pci->func);
@@ -2270,7 +2279,10 @@  skip_bar:
              */
             LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
         }
-        rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
+        if ( has_gsi )
+            rc = xc_domain_gsi_permission(ctx->xch, domid, irq, 0);
+        if ( !has_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 3c0ea8554107..75f23f511ac1 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
     /*