diff mbox series

[RFC,XEN,v12,7/7] tools: Add new function to do PIRQ (un)map on PVH dom0

Message ID 20240708114124.407797-8-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 July 8, 2024, 11:41 a.m. UTC
When dom0 is PVH, and passthrough a device to dumU, xl will
use the gsi number of device to do a pirq mapping, see
pci_add_dm_done->xc_physdev_map_pirq, but the gsi number is
got from file /sys/bus/pci/devices/<sbdf>/irq, that confuses
irq and gsi, they are in different space and are not equal,
so it will fail when mapping.
To solve this issue, use xc_physdev_gsi_from_dev to get the
real gsi and then to map pirq.

Besides, PVH dom doesn't have PIRQ flag, it doesn't do
PHYSDEVOP_map_pirq for each gsi. So grant function callstack
pci_add_dm_done->XEN_DOMCTL_irq_permission will fail at function
domain_pirq_to_irq. And old hypercall XEN_DOMCTL_irq_permission
requires passing in pirq, it is not suitable for dom0 that
doesn't have PIRQs to grant irq permission.
To solve this issue, use the new hypercall
XEN_DOMCTL_gsi_permission to grant the permission of irq(
translate from gsi) to dumU when dom0 has no PIRQs.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Chen Jiqian <Jiqian.Chen@amd.com>
---
RFC: it needs to wait for the corresponding third patch on linux kernel side to be merged.
https://lore.kernel.org/xen-devel/20240607075109.126277-4-Jiqian.Chen@amd.com/
This patch must be merged after the patch on linux kernel side
---
 tools/include/xenctrl.h       |   5 ++
 tools/libs/ctrl/xc_domain.c   |  15 +++++
 tools/libs/light/libxl_arch.h |   4 ++
 tools/libs/light/libxl_arm.c  |  10 +++
 tools/libs/light/libxl_pci.c  |  17 ++++++
 tools/libs/light/libxl_x86.c  | 111 ++++++++++++++++++++++++++++++++++
 6 files changed, 162 insertions(+)

Comments

Anthony PERARD July 8, 2024, 2:57 p.m. UTC | #1
On Mon, Jul 08, 2024 at 07:41:24PM +0800, Jiqian Chen wrote:
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index a4029e3ac810..d869bbec769e 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1774,6 +1774,16 @@ void libxl__arch_update_domain_config(libxl__gc *gc,
>  {
>  }
>  
> +int libxl__arch_hvm_map_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
> +{
> +    return -1;

It's best to return an ERROR_* for libxl error code instead of -1.
ERROR_NI seems to be the one, it probably means not-implemented. Or
maybe ERROR_INVAL would do to.

> +}
> +
> +int libxl__arch_hvm_unmap_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
> +{
> +    return -1;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 96cb4da0794e..3d25997921cc 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -17,6 +17,7 @@
>  #include "libxl_osdeps.h" /* must come before any other headers */
>  
>  #include "libxl_internal.h"
> +#include "libxl_arch.h"
>  
>  #define PCI_BDF                "%04x:%02x:%02x.%01x"
>  #define PCI_BDF_SHORT          "%02x:%02x.%01x"
> @@ -1478,6 +1479,16 @@ static void pci_add_dm_done(libxl__egc *egc,
>      fclose(f);
>      if (!pci_supp_legacy_irq())
>          goto out_no_irq;
> +
> +    /*
> +     * When dom0 is PVH and mapping a x86 gsi to pirq for domU,
> +     * should use gsi to grant irq permission.
> +     */
> +    if (!libxl__arch_hvm_map_gsi(gc, pci_encode_bdf(pci), domid))

Could you store the result of libxl__arch_hvm_map_gsi() in `rc', then
test that in the condition?

> +        goto pci_permissive;

Why do you skip part of the function on success?
But also, please avoid the "goto" coding style, in libxl, it's tolerated
for error handling when used to skip to the end of function to have a
single path (or error path) out of a function.

> +    else
> +        LOGED(WARN, domid, "libxl__arch_hvm_map_gsi failed (err=%d)", errno);

No one reads logs unless there's a failure or something doesn't work. So
here we just ignore failure returned by libxl__arch_hvm_map_gsi(), is it
the right things to do? Usually, just ignoring error is wrong.

FYI: LOGE* already logs errno.

> +
>      sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>                                  pci->bus, pci->dev, pci->func);
>      f = fopen(sysfs_path, "r");
> @@ -1505,6 +1516,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>      }
>      fclose(f);
>  
> +pci_permissive:
>      /* Don't restrict writes to the PCI config space from this VM */
>      if (pci->permissive) {
>          if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/permissive",
> @@ -2229,6 +2241,11 @@ skip_bar:
>      if (!pci_supp_legacy_irq())
>          goto skip_legacy_irq;
>  
> +    if (!libxl__arch_hvm_unmap_gsi(gc, pci_encode_bdf(pci), domid))
> +        goto skip_legacy_irq;
> +    else
> +        LOGED(WARN, domid, "libxl__arch_hvm_unmap_gsi failed (err=%d)", errno);
> +
>      sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>                             pci->bus, pci->dev, pci->func);
>  
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 60643d6f5376..e7756d323cb6 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -879,6 +879,117 @@ void libxl__arch_update_domain_config(libxl__gc *gc,
>                                   libxl_defbool_val(src->b_info.u.hvm.pirq));
>  }
>  
> +struct pcidev_map_pirq {
> +    uint32_t sbdf;
> +    uint32_t pirq;
> +    XEN_LIST_ENTRY(struct pcidev_map_pirq) entry;
> +};
> +
> +static pthread_mutex_t pcidev_pirq_mutex = PTHREAD_MUTEX_INITIALIZER;
> +static XEN_LIST_HEAD(, struct pcidev_map_pirq) pcidev_pirq_list =
> +    XEN_LIST_HEAD_INITIALIZER(pcidev_pirq_list);
> +
> +int libxl__arch_hvm_map_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
> +{
> +    int pirq = -1, gsi, r;
> +    xc_domaininfo_t info;
> +    struct pcidev_map_pirq *pcidev_pirq;
> +    libxl_ctx *ctx = libxl__gc_owner(gc);

Instead of declaring "ctx", you can use the macro "CTX" when you need
"ctx".

> +
> +    r = xc_domain_getinfo_single(ctx->xch, LIBXL_TOOLSTACK_DOMID, &info);
> +    if (r < 0) {
> +        LOGED(ERROR, domid, "getdomaininfo failed (error=%d)", errno);
> +        return r;

libxl_*() functions should return only libxl error code, that is return
code from other libxl_* functions, useally store in 'rc', or one of ERROR_*.

> +    }
> +    if ((info.flags & XEN_DOMINF_hvm_guest) &&
> +        !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ)) {
> +        gsi = xc_physdev_gsi_from_pcidev(ctx->xch, sbdf);
> +        if (gsi < 0) {
> +            return ERROR_FAIL;
> +        }
> +        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
> +        if (r < 0) {
> +            LOGED(ERROR, domid, "xc_physdev_map_pirq gsi=%d (error=%d)",
> +                  gsi, errno);
> +            return r;
> +        }
> +        r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
> +        if (r < 0) {
> +            LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d (error=%d)",
> +                  gsi, errno);
> +            return r;
> +        }
> +    } else {
> +        return ERROR_FAIL;

Is it really an error?

I few values can be returned here,
  * ERROR_INVAL meaing that the function was called on a dom0 that don't
    do "GSI",
  * 0, that is success, because the function check if it need to do
    anything, and since there's nothing to do, we can return success.

> +    }
> +
> +    /* Save the pirq for the usage of unmapping */
> +    pcidev_pirq = malloc(sizeof(struct pcidev_map_pirq));
> +    if (!pcidev_pirq) {
> +        LOGED(ERROR, domid, "no memory for saving pirq of pcidev info");
> +        return ERROR_NOMEM;
> +    }
> +    pcidev_pirq->sbdf = sbdf;
> +    pcidev_pirq->pirq = pirq;
> +
> +    assert(!pthread_mutex_lock(&pcidev_pirq_mutex));
> +    XEN_LIST_INSERT_HEAD(&pcidev_pirq_list, pcidev_pirq, entry);

I don't think that's going to work as you expect. libxl isn't a daemon
(or sometime it is but used for several domains), so anything store in
memory will be lost, or would be shared with other guest.

Do you need this mappins sbdf<-> pirq ? Is there a way to query this
information later from the environement? If not, you will need to store
the data somewhere else, probably in "libxl_domain_config *d_config" as
libxl can retrive the data with libxl__get_domain_configuration().
There's also the posibility to store that info in xenstore, but we
should probably avoid that.

Thanks,
Chen, Jiqian July 9, 2024, 6:18 a.m. UTC | #2
On 2024/7/8 22:57, Anthony PERARD wrote:
> On Mon, Jul 08, 2024 at 07:41:24PM +0800, Jiqian Chen wrote:
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index a4029e3ac810..d869bbec769e 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -1774,6 +1774,16 @@ void libxl__arch_update_domain_config(libxl__gc *gc,
>>  {
>>  }
>>  
>> +int libxl__arch_hvm_map_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
>> +{
>> +    return -1;
> 
> It's best to return an ERROR_* for libxl error code instead of -1.
> ERROR_NI seems to be the one, it probably means not-implemented. Or
> maybe ERROR_INVAL would do to.
Seems ERROR_INVAL is more suitable. Will change in next version.

> 
>> +}
>> +
>> +int libxl__arch_hvm_unmap_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
>> +{
>> +    return -1;
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index 96cb4da0794e..3d25997921cc 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -17,6 +17,7 @@
>>  #include "libxl_osdeps.h" /* must come before any other headers */
>>  
>>  #include "libxl_internal.h"
>> +#include "libxl_arch.h"
>>  
>>  #define PCI_BDF                "%04x:%02x:%02x.%01x"
>>  #define PCI_BDF_SHORT          "%02x:%02x.%01x"
>> @@ -1478,6 +1479,16 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      fclose(f);
>>      if (!pci_supp_legacy_irq())
>>          goto out_no_irq;
>> +
>> +    /*
>> +     * When dom0 is PVH and mapping a x86 gsi to pirq for domU,
>> +     * should use gsi to grant irq permission.
>> +     */
>> +    if (!libxl__arch_hvm_map_gsi(gc, pci_encode_bdf(pci), domid))
> 
> Could you store the result of libxl__arch_hvm_map_gsi() in `rc', then
> test that in the condition?
Will change in next version.
> 
>> +        goto pci_permissive;
> 
> Why do you skip part of the function on success?
Because libxl__arch_hvm_map_gsi do the same thing for PVH dom0, and the following part is for PV dom0.
If libxl__arch_hvm_map_gsi success, it should skip the following part.

> But also, please avoid the "goto" coding style, in libxl, it's tolerated
> for error handling when used to skip to the end of function to have a
> single path (or error path) out of a function.
Maybe I should split the part " xc_domain_getinfo_single(ctx->xch, LIBXL_TOOLSTACK_DOMID, &info); " in libxl__arch_hvm_map_gsi to a single function.
Then I can distinguish PVH and PV, and do different things for them.

> 
>> +    else
>> +        LOGED(WARN, domid, "libxl__arch_hvm_map_gsi failed (err=%d)", errno);
> 
> No one reads logs unless there's a failure or something doesn't work. So
> here we just ignore failure returned by libxl__arch_hvm_map_gsi(), is it
> the right things to do? Usually, just ignoring error is wrong.
Will change in next version.
> 
> FYI: LOGE* already logs errno.
> 
>> +
>>      sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>>                                  pci->bus, pci->dev, pci->func);
>>      f = fopen(sysfs_path, "r");
>> @@ -1505,6 +1516,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      }
>>      fclose(f);
>>  
>> +pci_permissive:
>>      /* Don't restrict writes to the PCI config space from this VM */
>>      if (pci->permissive) {
>>          if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/permissive",
>> @@ -2229,6 +2241,11 @@ skip_bar:
>>      if (!pci_supp_legacy_irq())
>>          goto skip_legacy_irq;
>>  
>> +    if (!libxl__arch_hvm_unmap_gsi(gc, pci_encode_bdf(pci), domid))
>> +        goto skip_legacy_irq;
>> +    else
>> +        LOGED(WARN, domid, "libxl__arch_hvm_unmap_gsi failed (err=%d)", errno);
>> +
>>      sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>>                             pci->bus, pci->dev, pci->func);
>>  
>> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
>> index 60643d6f5376..e7756d323cb6 100644
>> --- a/tools/libs/light/libxl_x86.c
>> +++ b/tools/libs/light/libxl_x86.c
>> @@ -879,6 +879,117 @@ void libxl__arch_update_domain_config(libxl__gc *gc,
>>                                   libxl_defbool_val(src->b_info.u.hvm.pirq));
>>  }
>>  
>> +struct pcidev_map_pirq {
>> +    uint32_t sbdf;
>> +    uint32_t pirq;
>> +    XEN_LIST_ENTRY(struct pcidev_map_pirq) entry;
>> +};
>> +
>> +static pthread_mutex_t pcidev_pirq_mutex = PTHREAD_MUTEX_INITIALIZER;
>> +static XEN_LIST_HEAD(, struct pcidev_map_pirq) pcidev_pirq_list =
>> +    XEN_LIST_HEAD_INITIALIZER(pcidev_pirq_list);
>> +
>> +int libxl__arch_hvm_map_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
>> +{
>> +    int pirq = -1, gsi, r;
>> +    xc_domaininfo_t info;
>> +    struct pcidev_map_pirq *pcidev_pirq;
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> 
> Instead of declaring "ctx", you can use the macro "CTX" when you need
> "ctx".
Will change in next version.

> 
>> +
>> +    r = xc_domain_getinfo_single(ctx->xch, LIBXL_TOOLSTACK_DOMID, &info);
>> +    if (r < 0) {
>> +        LOGED(ERROR, domid, "getdomaininfo failed (error=%d)", errno);
>> +        return r;
> 
> libxl_*() functions should return only libxl error code, that is return
> code from other libxl_* functions, useally store in 'rc', or one of ERROR_*.
OK, will change in next version.

> 
>> +    }
>> +    if ((info.flags & XEN_DOMINF_hvm_guest) &&
>> +        !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ)) {
>> +        gsi = xc_physdev_gsi_from_pcidev(ctx->xch, sbdf);
>> +        if (gsi < 0) {
>> +            return ERROR_FAIL;
>> +        }
>> +        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
>> +        if (r < 0) {
>> +            LOGED(ERROR, domid, "xc_physdev_map_pirq gsi=%d (error=%d)",
>> +                  gsi, errno);
>> +            return r;
>> +        }
>> +        r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
>> +        if (r < 0) {
>> +            LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d (error=%d)",
>> +                  gsi, errno);
>> +            return r;
>> +        }
>> +    } else {
>> +        return ERROR_FAIL;
> 
> Is it really an error?
> 
> I few values can be returned here,
>   * ERROR_INVAL meaing that the function was called on a dom0 that don't
>     do "GSI",
I think this is more suitable. And then the following code of PV can be done in pci_add_dm_done.

>   * 0, that is success, because the function check if it need to do
>     anything, and since there's nothing to do, we can return success.
> 
>> +    }
>> +
>> +    /* Save the pirq for the usage of unmapping */
>> +    pcidev_pirq = malloc(sizeof(struct pcidev_map_pirq));
>> +    if (!pcidev_pirq) {
>> +        LOGED(ERROR, domid, "no memory for saving pirq of pcidev info");
>> +        return ERROR_NOMEM;
>> +    }
>> +    pcidev_pirq->sbdf = sbdf;
>> +    pcidev_pirq->pirq = pirq;
>> +
>> +    assert(!pthread_mutex_lock(&pcidev_pirq_mutex));
>> +    XEN_LIST_INSERT_HEAD(&pcidev_pirq_list, pcidev_pirq, entry);
> 
> I don't think that's going to work as you expect. libxl isn't a daemon
> (or sometime it is but used for several domains), so anything store in
> memory will be lost, or would be shared with other guest.
> 
> Do you need this mappins sbdf<-> pirq ? 
I need to store the pirq that assigned to the gsi. Because libxl__arch_hvm_unmap_gsi need pirq to do xc_physdev_unmap_pirq

> Is there a way to query this information later from the environement? 
What I can think of is before xc_physdev_unmap_pirq, use xc_physdev_map_pirq to get the already mapped pirq, but I am not sure if it is suitable.

> If not, you will need to store the data somewhere else, probably in "libxl_domain_config *d_config" as
> libxl can retrive the data with libxl__get_domain_configuration().
However, pirq is dynamically mapped during starting domU, it may not be suitable for saving in d_config.

> There's also the posibility to store that info in xenstore, but we
> should probably avoid that.
> 
> Thanks,
>
diff mbox series

Patch

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 3720e22b399a..9ff5f1810cf8 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,
+                             uint8_t access_flag);
+
 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..4c89f07e4d6e 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,
+                             uint8_t access_flag)
+{
+    struct xen_domctl domctl = {
+        .cmd = XEN_DOMCTL_gsi_permission,
+        .domain = domid,
+        .u.gsi_permission.gsi = gsi,
+        .u.gsi_permission.access_flag = access_flag,
+    };
+
+    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_arch.h b/tools/libs/light/libxl_arch.h
index f88f11d6de1d..11b736067951 100644
--- a/tools/libs/light/libxl_arch.h
+++ b/tools/libs/light/libxl_arch.h
@@ -91,6 +91,10 @@  void libxl__arch_update_domain_config(libxl__gc *gc,
                                       libxl_domain_config *dst,
                                       const libxl_domain_config *src);
 
+_hidden
+int libxl__arch_hvm_map_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid);
+_hidden
+int libxl__arch_hvm_unmap_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid);
 #if defined(__i386__) || defined(__x86_64__)
 
 #define LAPIC_BASE_ADDRESS  0xfee00000
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index a4029e3ac810..d869bbec769e 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -1774,6 +1774,16 @@  void libxl__arch_update_domain_config(libxl__gc *gc,
 {
 }
 
+int libxl__arch_hvm_map_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
+{
+    return -1;
+}
+
+int libxl__arch_hvm_unmap_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
+{
+    return -1;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 96cb4da0794e..3d25997921cc 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -17,6 +17,7 @@ 
 #include "libxl_osdeps.h" /* must come before any other headers */
 
 #include "libxl_internal.h"
+#include "libxl_arch.h"
 
 #define PCI_BDF                "%04x:%02x:%02x.%01x"
 #define PCI_BDF_SHORT          "%02x:%02x.%01x"
@@ -1478,6 +1479,16 @@  static void pci_add_dm_done(libxl__egc *egc,
     fclose(f);
     if (!pci_supp_legacy_irq())
         goto out_no_irq;
+
+    /*
+     * When dom0 is PVH and mapping a x86 gsi to pirq for domU,
+     * should use gsi to grant irq permission.
+     */
+    if (!libxl__arch_hvm_map_gsi(gc, pci_encode_bdf(pci), domid))
+        goto pci_permissive;
+    else
+        LOGED(WARN, domid, "libxl__arch_hvm_map_gsi failed (err=%d)", errno);
+
     sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                                 pci->bus, pci->dev, pci->func);
     f = fopen(sysfs_path, "r");
@@ -1505,6 +1516,7 @@  static void pci_add_dm_done(libxl__egc *egc,
     }
     fclose(f);
 
+pci_permissive:
     /* Don't restrict writes to the PCI config space from this VM */
     if (pci->permissive) {
         if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/permissive",
@@ -2229,6 +2241,11 @@  skip_bar:
     if (!pci_supp_legacy_irq())
         goto skip_legacy_irq;
 
+    if (!libxl__arch_hvm_unmap_gsi(gc, pci_encode_bdf(pci), domid))
+        goto skip_legacy_irq;
+    else
+        LOGED(WARN, domid, "libxl__arch_hvm_unmap_gsi failed (err=%d)", errno);
+
     sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                            pci->bus, pci->dev, pci->func);
 
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 60643d6f5376..e7756d323cb6 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -879,6 +879,117 @@  void libxl__arch_update_domain_config(libxl__gc *gc,
                                  libxl_defbool_val(src->b_info.u.hvm.pirq));
 }
 
+struct pcidev_map_pirq {
+    uint32_t sbdf;
+    uint32_t pirq;
+    XEN_LIST_ENTRY(struct pcidev_map_pirq) entry;
+};
+
+static pthread_mutex_t pcidev_pirq_mutex = PTHREAD_MUTEX_INITIALIZER;
+static XEN_LIST_HEAD(, struct pcidev_map_pirq) pcidev_pirq_list =
+    XEN_LIST_HEAD_INITIALIZER(pcidev_pirq_list);
+
+int libxl__arch_hvm_map_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
+{
+    int pirq = -1, gsi, r;
+    xc_domaininfo_t info;
+    struct pcidev_map_pirq *pcidev_pirq;
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+
+    r = xc_domain_getinfo_single(ctx->xch, LIBXL_TOOLSTACK_DOMID, &info);
+    if (r < 0) {
+        LOGED(ERROR, domid, "getdomaininfo failed (error=%d)", errno);
+        return r;
+    }
+    if ((info.flags & XEN_DOMINF_hvm_guest) &&
+        !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ)) {
+        gsi = xc_physdev_gsi_from_pcidev(ctx->xch, sbdf);
+        if (gsi < 0) {
+            return ERROR_FAIL;
+        }
+        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
+        if (r < 0) {
+            LOGED(ERROR, domid, "xc_physdev_map_pirq gsi=%d (error=%d)",
+                  gsi, errno);
+            return r;
+        }
+        r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
+        if (r < 0) {
+            LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d (error=%d)",
+                  gsi, errno);
+            return r;
+        }
+    } else {
+        return ERROR_FAIL;
+    }
+
+    /* Save the pirq for the usage of unmapping */
+    pcidev_pirq = malloc(sizeof(struct pcidev_map_pirq));
+    if (!pcidev_pirq) {
+        LOGED(ERROR, domid, "no memory for saving pirq of pcidev info");
+        return ERROR_NOMEM;
+    }
+    pcidev_pirq->sbdf = sbdf;
+    pcidev_pirq->pirq = pirq;
+
+    assert(!pthread_mutex_lock(&pcidev_pirq_mutex));
+    XEN_LIST_INSERT_HEAD(&pcidev_pirq_list, pcidev_pirq, entry);
+    assert(!pthread_mutex_unlock(&pcidev_pirq_mutex));
+
+    return 0;
+}
+
+int libxl__arch_hvm_unmap_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
+{
+    int pirq = -1, gsi, r;
+    xc_domaininfo_t info;
+    struct pcidev_map_pirq *pcidev_pirq;
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+
+    r = xc_domain_getinfo_single(ctx->xch, LIBXL_TOOLSTACK_DOMID, &info);
+    if (r < 0) {
+        LOGED(ERROR, domid, "getdomaininfo failed (error=%d)", errno);
+        return r;
+    }
+    if ((info.flags & XEN_DOMINF_hvm_guest) &&
+        !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ)) {
+        gsi = xc_physdev_gsi_from_pcidev(ctx->xch, sbdf);
+        if (gsi < 0) {
+            return ERROR_FAIL;
+        }
+        assert(!pthread_mutex_lock(&pcidev_pirq_mutex));
+        XEN_LIST_FOREACH(pcidev_pirq, &pcidev_pirq_list, entry) {
+            if (pcidev_pirq->sbdf == sbdf) {
+                pirq = pcidev_pirq->pirq;
+                XEN_LIST_REMOVE(pcidev_pirq, entry);
+                free(pcidev_pirq);
+                break;
+            }
+        }
+        assert(!pthread_mutex_unlock(&pcidev_pirq_mutex));
+        if (pirq < 0) {
+            /* pirq has been unmapped, so return directly */
+            return 0;
+        }
+        r = xc_physdev_unmap_pirq(ctx->xch, domid, pirq);
+        if (r < 0) {
+            LOGED(ERROR, domid, "xc_physdev_unmap_pirq pirq=%d (error=%d)",
+                  pirq, errno);
+            return r;
+        }
+        r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 0);
+        if (r < 0) {
+            LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d (error=%d)",
+                  gsi, errno);
+            return r;
+        }
+    } else {
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C