[RFC XEN PATCH v3 0/3] Support device passthrough when dom0 is PVH on Xen

[RFC XEN PATCH v3 0/3] Support device passthrough when dom0 is PVH on Xen

[Jiqian Chen]

Hi All,
This is v3 series to support passthrough when dom0 is PVH
v2->v3 changes:
* patch#1: move the content out of pci_reset_device_state and delete pci_reset_device_state; add xsm_resource_setup_pci check for PHYSDEVOP_pci_device_state_reset; add description for PHYSDEVOP_pci_device_state_reset;
* patch#2: du to changes in the implementation of the second patch on kernel side(that it will do setup_gsi and map_pirq when assigning a device to passthrough), add PHYSDEVOP_setup_gsi for PVH dom0, and we need to support self mapping.
* patch#3: du to changes in the implementation of the second patch on kernel side(that adds a new sysfs for gsi instead of a new syscall), so read gsi number from the sysfs of gsi.

v3 link of the kernel patch:
https://lore.kernel.org/lkml/20231210161519.1550860-1-Jiqian.Chen@amd.com/T/#t

v2 link:
https://lore.kernel.org/xen-devel/20231124104136.3263722-1-Jiqian.Chen@amd.com/T/#t
Below is the description of v2 cover letter:
This series of patches are the v2 of the implementation of passthrough when dom0 is PVH on Xen.
We sent the v1 to upstream before, but the v1 had so many problems and we got lots of suggestions.
I will introduce all issues that these patches try to fix and the differences between v1 and v2.

Issues we encountered:
1. pci_stub failed to write bar for a passthrough device.
Problem: when we run “sudo xl pci-assignable-add <sbdf>” to assign a device, pci_stub will
call “pcistub_init_device() -> pci_restore_state() -> pci_restore_config_space() ->
pci_restore_config_space_range() -> pci_restore_config_dword() -> pci_write_config_dword()”,
the pci config write will trigger an io interrupt to bar_write() in the xen, but the
bar->enabled was set before, the write is not allowed now, and then when Qemu config the
passthrough device in xen_pt_realize(), it gets invalid bar values.

Reason: the reason is that we don't tell vPCI that the device has been reset, so the current
cached state in pdev->vpci is all out of date and is different from the real device state.

Solution: to solve this problem, the first patch of kernel(xen/pci: Add xen_reset_device_state
function) and the fist patch of xen(xen/vpci: Clear all vpci status of device) add a new
hypercall to reset the state stored in vPCI when the state of real device has changed.
Thank Roger for the suggestion of this v2, and it is different from v1
(https://lore.kernel.org/xen-devel/20230312075455.450187-3-ray.huang@amd.com/), v1 simply allow
domU to write pci bar, it does not comply with the design principles of vPCI.

2. failed to do PHYSDEVOP_map_pirq when dom0 is PVH
Problem: HVM domU will do PHYSDEVOP_map_pirq for a passthrough device by using gsi. See
xen_pt_realize->xc_physdev_map_pirq and pci_add_dm_done->xc_physdev_map_pirq. Then
xc_physdev_map_pirq will call into Xen, but in hvm_physdev_op(), PHYSDEVOP_map_pirq is not allowed.

Reason: In hvm_physdev_op(), the variable "currd" is PVH dom0 and PVH has no X86_EMU_USE_PIRQ flag,
it will fail at has_pirq check.

Solution: I think we may need to allow PHYSDEVOP_map_pirq when "currd" is dom0 (at present dom0 is
PVH). The second patch of xen(x86/pvh: Open PHYSDEVOP_map_pirq for PVH dom0) allow PVH dom0 do
PHYSDEVOP_map_pirq. This v2 patch is better than v1, v1 simply remove the has_pirq check(xen
https://lore.kernel.org/xen-devel/20230312075455.450187-4-ray.huang@amd.com/).

3. the gsi of a passthrough device doesn't be unmasked
 3.1 failed to check the permission of pirq
 3.2 the gsi of passthrough device was not registered in PVH dom0

Problem:
3.1 callback function pci_add_dm_done() will be called when qemu config a passthrough device for domU.
This function will call xc_domain_irq_permission()-> pirq_access_permitted() to check if the gsi has
corresponding mappings in dom0. But it didn’t, so failed. See
XEN_DOMCTL_irq_permission->pirq_access_permitted, "current" is PVH dom0 and it return irq is 0.
3.2 it's possible for a gsi (iow: vIO-APIC pin) to never get registered on PVH dom0, because the
devices of PVH are using MSI(-X) interrupts. However, the IO-APIC pin must be configured for it to be
able to be mapped into a domU.

Reason: After searching codes, I find "map_pirq" and "register_gsi" will be done in function
vioapic_write_redirent->vioapic_hwdom_map_gsi when the gsi(aka ioapic's pin) is unmasked in PVH dom0.
So the two problems can be concluded to that the gsi of a passthrough device doesn't be unmasked.

Solution: to solve these problems, the second patch of kernel(xen/pvh: Unmask irq for passthrough
device in PVH dom0) call the unmask_irq() when we assign a device to be passthrough. So that
passthrough devices can have the mapping of gsi on PVH dom0 and gsi can be registered. This v2 patch
is different from the v1(
kernel https://lore.kernel.org/xen-devel/20230312120157.452859-5-ray.huang@amd.com/,
kernel https://lore.kernel.org/xen-devel/20230312120157.452859-5-ray.huang@amd.com/ and
xen https://lore.kernel.org/xen-devel/20230312075455.450187-5-ray.huang@amd.com/),
v1 performed "map_pirq" and "register_gsi" on all pci devices on PVH dom0, which is unnecessary and
may cause multiple registration.

4. failed to map pirq for gsi
Problem: qemu will call xc_physdev_map_pirq() to map a passthrough device’s gsi to pirq in function
xen_pt_realize(). But failed.

Reason: According to the implement of xc_physdev_map_pirq(), it needs gsi instead of irq, but qemu
pass irq to it and treat irq as gsi, it is got from file /sys/bus/pci/devices/xxxx:xx:xx.x/irq in
function xen_host_pci_device_get(). But actually the gsi number is not equal with irq. On PVH dom0,
when it allocates irq for a gsi in function acpi_register_gsi_ioapic(), allocation is dynamic, and
follow the principle of applying first, distributing first. And if you debug the kernel codes(see
function __irq_alloc_descs), you will find the irq number is allocated from small to large by order,
but the applying gsi number is not, gsi 38 may come before gsi 28, that causes gsi 38 get a smaller
irq number than gsi 28, and then gsi != irq.

Solution: we can record the relation between gsi and irq, then when userspace(qemu) want to use gsi,
we can do a translation. The third patch of kernel(xen/privcmd: Add new syscall to get gsi from irq)
records all the relations in acpi_register_gsi_xen_pvh() when dom0 initialize pci devices, and provide
a syscall for userspace to get the gsi from irq. The third patch of xen(tools: Add new function to get
gsi from irq) add a new function xc_physdev_gsi_from_irq() to call the new syscall added on kernel side.
And then userspace can use that function to get gsi. Then xc_physdev_map_pirq() will success. This v2
patch is the same as v1(
kernel https://lore.kernel.org/xen-devel/20230312120157.452859-6-ray.huang@amd.com/ and
xen https://lore.kernel.org/xen-devel/20230312075455.450187-6-ray.huang@amd.com/)

About the v2 patch of qemu, just change an included head file, other are similar to the v1 (
qemu https://lore.kernel.org/xen-devel/20230312092244.451465-19-ray.huang@amd.com/), just call
xc_physdev_gsi_from_irq() to get gsi from irq.

Jiqian Chen (3):
  xen/vpci: Clear all vpci status of device
  x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0
  libxl: Use gsi instead of irq for mapping pirq

 tools/libs/light/libxl_pci.c | 18 +++++++++---------
 xen/arch/x86/hvm/hypercall.c |  4 ++++
 xen/drivers/pci/physdev.c    | 35 +++++++++++++++++++++++++++++++++++
 xen/drivers/vpci/vpci.c      |  9 +++++++++
 xen/include/public/physdev.h |  8 ++++++++
 xen/include/xen/vpci.h       |  6 ++++++
 6 files changed, 71 insertions(+), 9 deletions(-)

-- 
2.34.1

[RFC XEN PATCH v3 1/3] xen/vpci: Clear all vpci status of device

[Jiqian Chen]

When a device has been reset on dom0 side, the vpci on Xen
side won't get notification, so the cached state in vpci is
all out of date compare with the real device state.
To solve that problem, add a new hypercall to clear all vpci
device state. When the state of device is reset on dom0 side,
dom0 can call this hypercall to notify vpci.

Co-developed-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 xen/arch/x86/hvm/hypercall.c |  1 +
 xen/drivers/pci/physdev.c    | 35 +++++++++++++++++++++++++++++++++++
 xen/drivers/vpci/vpci.c      |  9 +++++++++
 xen/include/public/physdev.h |  8 ++++++++
 xen/include/xen/vpci.h       |  6 ++++++
 5 files changed, 59 insertions(+)

diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index eeb73e1aa5..6ad5b4d5f1 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case PHYSDEVOP_pci_mmcfg_reserved:
     case PHYSDEVOP_pci_device_add:
     case PHYSDEVOP_pci_device_remove:
+    case PHYSDEVOP_pci_device_state_reset:
     case PHYSDEVOP_dbgp_op:
         if ( !is_hardware_domain(currd) )
             return -ENOSYS;
diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
index 42db3e6d13..6ee2edb86a 100644
--- a/xen/drivers/pci/physdev.c
+++ b/xen/drivers/pci/physdev.c
@@ -2,6 +2,7 @@
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
 #include <xen/init.h>
+#include <xen/vpci.h>
 
 #ifndef COMPAT
 typedef long ret_t;
@@ -67,6 +68,40 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case PHYSDEVOP_pci_device_state_reset: {
+        struct physdev_pci_device dev;
+        struct pci_dev *pdev;
+
+        if ( !is_pci_passthrough_enabled() )
+            return -EOPNOTSUPP;
+
+        ret = -EFAULT;
+        if ( copy_from_guest(&dev, arg, 1) != 0 )
+            break;
+
+        ret = xsm_resource_setup_pci(XSM_PRIV,
+                                     (dev.seg << 16) | (dev.bus << 8) |
+                                     dev.devfn);
+        if ( ret )
+            break;
+
+        pcidevs_lock();
+        pdev = pci_get_pdev(NULL, PCI_SBDF(dev.seg, dev.bus, dev.devfn));
+        if ( !pdev )
+        {
+            ret = -ENODEV;
+            pcidevs_unlock();
+            break;
+        }
+
+        ret = vpci_reset_device_state(pdev);
+        if ( ret )
+            printk(XENLOG_ERR "PCI reset device %pp state failed\n",
+                   &pdev->sbdf);
+        pcidevs_unlock();
+        break;
+    }
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 72ef277c4f..3c64cb10cc 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -107,6 +107,15 @@ int vpci_add_handlers(struct pci_dev *pdev)
 
     return rc;
 }
+
+int vpci_reset_device_state(struct pci_dev *pdev)
+{
+    ASSERT(pcidevs_locked());
+
+    vpci_remove_device(pdev);
+    return vpci_add_handlers(pdev);
+}
+
 #endif /* __XEN__ */
 
 static int vpci_register_cmp(const struct vpci_register *r1,
diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
index f0c0d4727c..92c2f28bca 100644
--- a/xen/include/public/physdev.h
+++ b/xen/include/public/physdev.h
@@ -296,6 +296,14 @@ DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_add_t);
  */
 #define PHYSDEVOP_prepare_msix          30
 #define PHYSDEVOP_release_msix          31
+/*
+ * On PVH dom0, when device is reset, the vpci on Xen side
+ * won't get notification, so that the cached state in vpci is
+ * all out of date with the real device state. Use this to reset
+ * the vpci state of device.
+ */
+#define PHYSDEVOP_pci_device_state_reset     32
+
 struct physdev_pci_device {
     /* IN */
     uint16_t seg;
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index d20c301a3d..d6377424f0 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -30,6 +30,7 @@ int __must_check vpci_add_handlers(struct pci_dev *pdev);
 
 /* Remove all handlers and free vpci related structures. */
 void vpci_remove_device(struct pci_dev *pdev);
+int vpci_reset_device_state(struct pci_dev *pdev);
 
 /* Add/remove a register handler. */
 int __must_check vpci_add_register_mask(struct vpci *vpci,
@@ -262,6 +263,11 @@ static inline int vpci_add_handlers(struct pci_dev *pdev)
 
 static inline void vpci_remove_device(struct pci_dev *pdev) { }
 
+static inline int vpci_reset_device_state(struct pci_dev *pdev)
+{
+    return 0;
+}
+
 static inline void vpci_dump_msi(void) { }
 
 static inline uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
-- 
2.34.1

[RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Jiqian Chen]

If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
a passthrough device by using gsi, see
xen_pt_realize->xc_physdev_map_pirq and
pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq
will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
is not allowed because currd is PVH dom0 and PVH has no
X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
So, allow PHYSDEVOP_map_pirq when currd is dom0 no matter if
dom0 has X86_EMU_USE_PIRQ flag and also allow
PHYSDEVOP_unmap_pirq for the failed path to unmap pirq.

What's more, in PVH dom0, the gsis don't get registered, but
the gsi of a passthrough device must be configured for it to
be able to be mapped into a hvm domU.
So, add PHYSDEVOP_setup_gsi for PVH dom0, because PVH dom0
will setup gsi during assigning a device to passthrough.

Co-developed-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 xen/arch/x86/hvm/hypercall.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 6ad5b4d5f1..621d789bd3 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     switch ( cmd )
     {
+    case PHYSDEVOP_setup_gsi:
     case PHYSDEVOP_map_pirq:
     case PHYSDEVOP_unmap_pirq:
+        if ( is_hardware_domain(currd) )
+            break;
     case PHYSDEVOP_eoi:
     case PHYSDEVOP_irq_status_query:
     case PHYSDEVOP_get_free_pirq:
-- 
2.34.1

[RFC XEN PATCH v3 3/3] libxl: Use gsi instead of irq for mapping pirq

[Jiqian Chen]

In PVH dom0, it uses the linux local interrupt mechanism,
when it allocs irq for a gsi, it is dynamic, and follow
the principle of applying first, distributing first. And
the irq number is alloced from small to large, but the
applying gsi number is not, may gsi 38 comes before gsi
28, that causes the irq number is not equal with the gsi
number. And when passthrough a device, xl wants to use
gsi to map pirq, see pci_add_dm_done->xc_physdev_map_pirq,
but the gsi number is got from file
/sys/bus/pci/devices/<sbdf>/irq in current code, so it
will fail when mapping.

So, use real gsi number read from gsi sysfs.

Co-developed-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 tools/libs/light/libxl_pci.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 96cb4da079..9e75f0c263 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1416,7 +1416,7 @@ static void pci_add_dm_done(libxl__egc *egc,
     char *sysfs_path;
     FILE *f;
     unsigned long long start, end, flags, size;
-    int irq, i;
+    int gsi, i;
     int r;
     uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
     uint32_t domainid = domid;
@@ -1439,7 +1439,7 @@ static void pci_add_dm_done(libxl__egc *egc,
                            pci->bus, pci->dev, pci->func);
     f = fopen(sysfs_path, "r");
     start = end = flags = size = 0;
-    irq = 0;
+    gsi = 0;
 
     if (f == NULL) {
         LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
@@ -1478,26 +1478,26 @@ static void pci_add_dm_done(libxl__egc *egc,
     fclose(f);
     if (!pci_supp_legacy_irq())
         goto out_no_irq;
-    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
+    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/gsi", pci->domain,
                                 pci->bus, pci->dev, pci->func);
     f = fopen(sysfs_path, "r");
     if (f == NULL) {
         LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
         goto out_no_irq;
     }
-    if ((fscanf(f, "%u", &irq) == 1) && irq) {
-        r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
+    if ((fscanf(f, "%u", &gsi) == 1) && gsi) {
+        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &gsi);
         if (r < 0) {
-            LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
-                  irq, r);
+            LOGED(ERROR, domainid, "xc_physdev_map_pirq gsi=%d (error=%d)",
+                  gsi, r);
             fclose(f);
             rc = ERROR_FAIL;
             goto out;
         }
-        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
+        r = xc_domain_irq_permission(ctx->xch, domid, gsi, 1);
         if (r < 0) {
             LOGED(ERROR, domainid,
-                  "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
+                  "xc_domain_irq_permission gsi=%d (error=%d)", gsi, r);
             fclose(f);
             rc = ERROR_FAIL;
             goto out;
-- 
2.34.1

Re: [RFC XEN PATCH v3 1/3] xen/vpci: Clear all vpci status of device

[Roger Pau Monné]

On Mon, Dec 11, 2023 at 12:40:07AM +0800, Jiqian Chen wrote:
> When a device has been reset on dom0 side, the vpci on Xen
> side won't get notification, so the cached state in vpci is
> all out of date compare with the real device state.
> To solve that problem, add a new hypercall to clear all vpci
> device state. When the state of device is reset on dom0 side,
> dom0 can call this hypercall to notify vpci.
> 
> Co-developed-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
>  xen/arch/x86/hvm/hypercall.c |  1 +
>  xen/drivers/pci/physdev.c    | 35 +++++++++++++++++++++++++++++++++++
>  xen/drivers/vpci/vpci.c      |  9 +++++++++
>  xen/include/public/physdev.h |  8 ++++++++
>  xen/include/xen/vpci.h       |  6 ++++++
>  5 files changed, 59 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index eeb73e1aa5..6ad5b4d5f1 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      case PHYSDEVOP_pci_mmcfg_reserved:
>      case PHYSDEVOP_pci_device_add:
>      case PHYSDEVOP_pci_device_remove:
> +    case PHYSDEVOP_pci_device_state_reset:
>      case PHYSDEVOP_dbgp_op:
>          if ( !is_hardware_domain(currd) )
>              return -ENOSYS;
> diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
> index 42db3e6d13..6ee2edb86a 100644
> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -2,6 +2,7 @@
>  #include <xen/guest_access.h>
>  #include <xen/hypercall.h>
>  #include <xen/init.h>
> +#include <xen/vpci.h>
>  
>  #ifndef COMPAT
>  typedef long ret_t;
> @@ -67,6 +68,40 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case PHYSDEVOP_pci_device_state_reset: {
> +        struct physdev_pci_device dev;
> +        struct pci_dev *pdev;
> +
> +        if ( !is_pci_passthrough_enabled() )
> +            return -EOPNOTSUPP;
> +
> +        ret = -EFAULT;
> +        if ( copy_from_guest(&dev, arg, 1) != 0 )
> +            break;
> +
> +        ret = xsm_resource_setup_pci(XSM_PRIV,
> +                                     (dev.seg << 16) | (dev.bus << 8) |
> +                                     dev.devfn);
> +        if ( ret )
> +            break;
> +
> +        pcidevs_lock();
> +        pdev = pci_get_pdev(NULL, PCI_SBDF(dev.seg, dev.bus, dev.devfn));
> +        if ( !pdev )
> +        {
> +            ret = -ENODEV;
> +            pcidevs_unlock();

Nit: assigning -ENODEV could be done after dropping the lock.

> +            break;
> +        }
> +
> +        ret = vpci_reset_device_state(pdev);
> +        if ( ret )
> +            printk(XENLOG_ERR "PCI reset device %pp state failed\n",
> +                   &pdev->sbdf);

Please do the print outside of the locked region, there's no need to
hold the pcidevs lock after the vpci_reset_device_state() call if you
use the data in the local 'dev' variable to print the SBDF.

It would also be fine if you want to introduce a local sbdf variable
that you can use both here and in the call to pci_get_pdev() above.

I think it's also easier to parse if the SBDF is at the begging of the
message:

"%pp: failed to reset PCI device state\n"

That's however a question of taste.

> +        pcidevs_unlock();
> +        break;
> +    }
> +
>      default:
>          ret = -ENOSYS;
>          break;
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 72ef277c4f..3c64cb10cc 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -107,6 +107,15 @@ int vpci_add_handlers(struct pci_dev *pdev)
>  
>      return rc;
>  }
> +
> +int vpci_reset_device_state(struct pci_dev *pdev)
> +{
> +    ASSERT(pcidevs_locked());
> +
> +    vpci_remove_device(pdev);
> +    return vpci_add_handlers(pdev);
> +}
> +
>  #endif /* __XEN__ */
>  
>  static int vpci_register_cmp(const struct vpci_register *r1,
> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
> index f0c0d4727c..92c2f28bca 100644
> --- a/xen/include/public/physdev.h
> +++ b/xen/include/public/physdev.h
> @@ -296,6 +296,14 @@ DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_add_t);
>   */
>  #define PHYSDEVOP_prepare_msix          30
>  #define PHYSDEVOP_release_msix          31
> +/*
> + * On PVH dom0, when device is reset, the vpci on Xen side
> + * won't get notification, so that the cached state in vpci is
> + * all out of date with the real device state. Use this to reset
> + * the vpci state of device.
> + */

I get this feeling this is too specific to vpci, when the hypercall
itself could be used for other purposes in the future.  I would
instead write:

/*
 * Notify the hypervisor that a PCI device has been reset, so that any
 * internally cached state is regenerated.  Should be called after any
 * device reset performed by the hardware domain.
 */

> +#define PHYSDEVOP_pci_device_state_reset     32
> +
>  struct physdev_pci_device {
>      /* IN */
>      uint16_t seg;
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index d20c301a3d..d6377424f0 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -30,6 +30,7 @@ int __must_check vpci_add_handlers(struct pci_dev *pdev);
>  
>  /* Remove all handlers and free vpci related structures. */
>  void vpci_remove_device(struct pci_dev *pdev);
> +int vpci_reset_device_state(struct pci_dev *pdev);

__must_check please.

Thanks, Roger.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Roger Pau Monné]

On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
> a passthrough device by using gsi, see
> xen_pt_realize->xc_physdev_map_pirq and
> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq
> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
> is not allowed because currd is PVH dom0 and PVH has no
> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
> So, allow PHYSDEVOP_map_pirq when currd is dom0 no matter if
> dom0 has X86_EMU_USE_PIRQ flag and also allow
> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq.
> 
> What's more, in PVH dom0, the gsis don't get registered, but
> the gsi of a passthrough device must be configured for it to
> be able to be mapped into a hvm domU.
> So, add PHYSDEVOP_setup_gsi for PVH dom0, because PVH dom0
> will setup gsi during assigning a device to passthrough.
> 
> Co-developed-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
>  xen/arch/x86/hvm/hypercall.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index 6ad5b4d5f1..621d789bd3 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      switch ( cmd )
>      {
> +    case PHYSDEVOP_setup_gsi:

I think given the new approach on the Linux side patches, where
pciback will configure the interrupt, there's no need to expose
setup_gsi anymore?

>      case PHYSDEVOP_map_pirq:
>      case PHYSDEVOP_unmap_pirq:
> +        if ( is_hardware_domain(currd) )
> +            break;

Also Jan already pointed this out in v2: this hypercall needs to be
limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
the hypercall if DOMID_SELF or the passed domid matches the current
domain domid.

Thanks, Roger.

Re: [RFC XEN PATCH v3 3/3] libxl: Use gsi instead of irq for mapping pirq

[Roger Pau Monné]

On Mon, Dec 11, 2023 at 12:40:09AM +0800, Jiqian Chen wrote:
> In PVH dom0, it uses the linux local interrupt mechanism,
> when it allocs irq for a gsi, it is dynamic, and follow
> the principle of applying first, distributing first. And
> the irq number is alloced from small to large, but the
> applying gsi number is not, may gsi 38 comes before gsi
> 28, that causes the irq number is not equal with the gsi
> number. And when passthrough a device, xl wants to use
> gsi to map pirq, see pci_add_dm_done->xc_physdev_map_pirq,
> but the gsi number is got from file
> /sys/bus/pci/devices/<sbdf>/irq in current code, so it
> will fail when mapping.
> 
> So, use real gsi number read from gsi sysfs.
> 
> Co-developed-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
>  tools/libs/light/libxl_pci.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 96cb4da079..9e75f0c263 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -1416,7 +1416,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>      char *sysfs_path;
>      FILE *f;
>      unsigned long long start, end, flags, size;
> -    int irq, i;
> +    int gsi, i;
>      int r;
>      uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>      uint32_t domainid = domid;
> @@ -1439,7 +1439,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>                             pci->bus, pci->dev, pci->func);
>      f = fopen(sysfs_path, "r");
>      start = end = flags = size = 0;
> -    irq = 0;
> +    gsi = 0;

unsigned int (so it matches the fscanf format), and initialized at
definition.

>  
>      if (f == NULL) {
>          LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
> @@ -1478,26 +1478,26 @@ static void pci_add_dm_done(libxl__egc *egc,
>      fclose(f);
>      if (!pci_supp_legacy_irq())
>          goto out_no_irq;
> -    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
> +    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/gsi", pci->domain,
>                                  pci->bus, pci->dev, pci->func);

You need to keep the fallback mechanism of reading the irq node, or
else xl would stop working on any kernel that doesn't expose this
sysfs node, you would break passthrough on all current Linux versions.

Thanks, Roger.

Re: [RFC XEN PATCH v3 1/3] xen/vpci: Clear all vpci status of device

[Chen, Jiqian]

On 2023/12/11 23:22, Roger Pau Monné wrote:
> On Mon, Dec 11, 2023 at 12:40:07AM +0800, Jiqian Chen wrote:
>> When a device has been reset on dom0 side, the vpci on Xen
>> side won't get notification, so the cached state in vpci is
>> all out of date compare with the real device state.
>> To solve that problem, add a new hypercall to clear all vpci
>> device state. When the state of device is reset on dom0 side,
>> dom0 can call this hypercall to notify vpci.
>>
>> Co-developed-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>>  xen/arch/x86/hvm/hypercall.c |  1 +
>>  xen/drivers/pci/physdev.c    | 35 +++++++++++++++++++++++++++++++++++
>>  xen/drivers/vpci/vpci.c      |  9 +++++++++
>>  xen/include/public/physdev.h |  8 ++++++++
>>  xen/include/xen/vpci.h       |  6 ++++++
>>  5 files changed, 59 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
>> index eeb73e1aa5..6ad5b4d5f1 100644
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>      case PHYSDEVOP_pci_mmcfg_reserved:
>>      case PHYSDEVOP_pci_device_add:
>>      case PHYSDEVOP_pci_device_remove:
>> +    case PHYSDEVOP_pci_device_state_reset:
>>      case PHYSDEVOP_dbgp_op:
>>          if ( !is_hardware_domain(currd) )
>>              return -ENOSYS;
>> diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
>> index 42db3e6d13..6ee2edb86a 100644
>> --- a/xen/drivers/pci/physdev.c
>> +++ b/xen/drivers/pci/physdev.c
>> @@ -2,6 +2,7 @@
>>  #include <xen/guest_access.h>
>>  #include <xen/hypercall.h>
>>  #include <xen/init.h>
>> +#include <xen/vpci.h>
>>  
>>  #ifndef COMPAT
>>  typedef long ret_t;
>> @@ -67,6 +68,40 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>          break;
>>      }
>>  
>> +    case PHYSDEVOP_pci_device_state_reset: {
>> +        struct physdev_pci_device dev;
>> +        struct pci_dev *pdev;
>> +
>> +        if ( !is_pci_passthrough_enabled() )
>> +            return -EOPNOTSUPP;
>> +
>> +        ret = -EFAULT;
>> +        if ( copy_from_guest(&dev, arg, 1) != 0 )
>> +            break;
>> +
>> +        ret = xsm_resource_setup_pci(XSM_PRIV,
>> +                                     (dev.seg << 16) | (dev.bus << 8) |
>> +                                     dev.devfn);
>> +        if ( ret )
>> +            break;
>> +
>> +        pcidevs_lock();
>> +        pdev = pci_get_pdev(NULL, PCI_SBDF(dev.seg, dev.bus, dev.devfn));
>> +        if ( !pdev )
>> +        {
>> +            ret = -ENODEV;
>> +            pcidevs_unlock();
> 
> Nit: assigning -ENODEV could be done after dropping the lock.
I will adjust the sequence in next version, thanks.

> 
>> +            break;
>> +        }
>> +
>> +        ret = vpci_reset_device_state(pdev);
>> +        if ( ret )
>> +            printk(XENLOG_ERR "PCI reset device %pp state failed\n",
>> +                   &pdev->sbdf);
> 
> Please do the print outside of the locked region, there's no need to
> hold the pcidevs lock after the vpci_reset_device_state() call if you
> use the data in the local 'dev' variable to print the SBDF.
> 
> It would also be fine if you want to introduce a local sbdf variable
> that you can use both here and in the call to pci_get_pdev() above.
> 
> I think it's also easier to parse if the SBDF is at the begging of the
> message:
> 
> "%pp: failed to reset PCI device state\n"
> 
> That's however a question of taste.
Nice suggestion, I will adjust according to all what you said. Thanks.

> 
>> +        pcidevs_unlock();
>> +        break;
>> +    }
>> +
>>      default:
>>          ret = -ENOSYS;
>>          break;
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 72ef277c4f..3c64cb10cc 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -107,6 +107,15 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>  
>>      return rc;
>>  }
>> +
>> +int vpci_reset_device_state(struct pci_dev *pdev)
>> +{
>> +    ASSERT(pcidevs_locked());
>> +
>> +    vpci_remove_device(pdev);
>> +    return vpci_add_handlers(pdev);
>> +}
>> +
>>  #endif /* __XEN__ */
>>  
>>  static int vpci_register_cmp(const struct vpci_register *r1,
>> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
>> index f0c0d4727c..92c2f28bca 100644
>> --- a/xen/include/public/physdev.h
>> +++ b/xen/include/public/physdev.h
>> @@ -296,6 +296,14 @@ DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_add_t);
>>   */
>>  #define PHYSDEVOP_prepare_msix          30
>>  #define PHYSDEVOP_release_msix          31
>> +/*
>> + * On PVH dom0, when device is reset, the vpci on Xen side
>> + * won't get notification, so that the cached state in vpci is
>> + * all out of date with the real device state. Use this to reset
>> + * the vpci state of device.
>> + */
> 
> I get this feeling this is too specific to vpci, when the hypercall
> itself could be used for other purposes in the future.  I would
> instead write:
> 
> /*
>  * Notify the hypervisor that a PCI device has been reset, so that any
>  * internally cached state is regenerated.  Should be called after any
>  * device reset performed by the hardware domain.
>  */
Thanks, will use this in next version.

> 
>> +#define PHYSDEVOP_pci_device_state_reset     32
>> +
>>  struct physdev_pci_device {
>>      /* IN */
>>      uint16_t seg;
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index d20c301a3d..d6377424f0 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -30,6 +30,7 @@ int __must_check vpci_add_handlers(struct pci_dev *pdev);
>>  
>>  /* Remove all handlers and free vpci related structures. */
>>  void vpci_remove_device(struct pci_dev *pdev);
>> +int vpci_reset_device_state(struct pci_dev *pdev);
> 
> __must_check please.
> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Chen, Jiqian]

On 2023/12/11 23:31, Roger Pau Monné wrote:
> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
>> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
>> a passthrough device by using gsi, see
>> xen_pt_realize->xc_physdev_map_pirq and
>> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq
>> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
>> is not allowed because currd is PVH dom0 and PVH has no
>> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
>> So, allow PHYSDEVOP_map_pirq when currd is dom0 no matter if
>> dom0 has X86_EMU_USE_PIRQ flag and also allow
>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq.
>>
>> What's more, in PVH dom0, the gsis don't get registered, but
>> the gsi of a passthrough device must be configured for it to
>> be able to be mapped into a hvm domU.
>> So, add PHYSDEVOP_setup_gsi for PVH dom0, because PVH dom0
>> will setup gsi during assigning a device to passthrough.
>>
>> Co-developed-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>>  xen/arch/x86/hvm/hypercall.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
>> index 6ad5b4d5f1..621d789bd3 100644
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>  
>>      switch ( cmd )
>>      {
>> +    case PHYSDEVOP_setup_gsi:
> 
> I think given the new approach on the Linux side patches, where
> pciback will configure the interrupt, there's no need to expose
> setup_gsi anymore?
The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.

> 
>>      case PHYSDEVOP_map_pirq:
>>      case PHYSDEVOP_unmap_pirq:
>> +        if ( is_hardware_domain(currd) )
>> +            break;
> 
> Also Jan already pointed this out in v2: this hypercall needs to be
> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
> the hypercall if DOMID_SELF or the passed domid matches the current
> domain domid.
Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v3 3/3] libxl: Use gsi instead of irq for mapping pirq

[Chen, Jiqian]

On 2023/12/11 23:48, Roger Pau Monné wrote:
> On Mon, Dec 11, 2023 at 12:40:09AM +0800, Jiqian Chen wrote:
>> In PVH dom0, it uses the linux local interrupt mechanism,
>> when it allocs irq for a gsi, it is dynamic, and follow
>> the principle of applying first, distributing first. And
>> the irq number is alloced from small to large, but the
>> applying gsi number is not, may gsi 38 comes before gsi
>> 28, that causes the irq number is not equal with the gsi
>> number. And when passthrough a device, xl wants to use
>> gsi to map pirq, see pci_add_dm_done->xc_physdev_map_pirq,
>> but the gsi number is got from file
>> /sys/bus/pci/devices/<sbdf>/irq in current code, so it
>> will fail when mapping.
>>
>> So, use real gsi number read from gsi sysfs.
>>
>> Co-developed-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>>  tools/libs/light/libxl_pci.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index 96cb4da079..9e75f0c263 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1416,7 +1416,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      char *sysfs_path;
>>      FILE *f;
>>      unsigned long long start, end, flags, size;
>> -    int irq, i;
>> +    int gsi, i;
>>      int r;
>>      uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>>      uint32_t domainid = domid;
>> @@ -1439,7 +1439,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>                             pci->bus, pci->dev, pci->func);
>>      f = fopen(sysfs_path, "r");
>>      start = end = flags = size = 0;
>> -    irq = 0;
>> +    gsi = 0;
> 
> unsigned int (so it matches the fscanf format), and initialized at
> definition.
As what you said below, I need to use irq if there is no gsi sysfs. So, I think it is not necessary to change the name of this local variable.

> 
>>  
>>      if (f == NULL) {
>>          LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
>> @@ -1478,26 +1478,26 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      fclose(f);
>>      if (!pci_supp_legacy_irq())
>>          goto out_no_irq;
>> -    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>> +    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/gsi", pci->domain,
>>                                  pci->bus, pci->dev, pci->func);
> 
> You need to keep the fallback mechanism of reading the irq node, or
> else xl would stop working on any kernel that doesn't expose this
> sysfs node, you would break passthrough on all current Linux versions.
Yes, you are right. If there is no gsi sysfs, will still use irq, in next version. Thanks.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Jan Beulich]

On 12.12.2023 07:49, Chen, Jiqian wrote:
> On 2023/12/11 23:31, Roger Pau Monné wrote:
>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
>>> --- a/xen/arch/x86/hvm/hypercall.c
>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>  
>>>      switch ( cmd )
>>>      {
>>> +    case PHYSDEVOP_setup_gsi:
>>
>> I think given the new approach on the Linux side patches, where
>> pciback will configure the interrupt, there's no need to expose
>> setup_gsi anymore?
> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
> 
>>
>>>      case PHYSDEVOP_map_pirq:
>>>      case PHYSDEVOP_unmap_pirq:
>>> +        if ( is_hardware_domain(currd) )
>>> +            break;
>>
>> Also Jan already pointed this out in v2: this hypercall needs to be
>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
>> the hypercall if DOMID_SELF or the passed domid matches the current
>> domain domid.
> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.

And why exactly would it do specifically the map_pirq? (Even the setup_gsi
looks questionable to me, but there might be reasons there.)

Jan

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Chen, Jiqian]

On 2023/12/12 17:30, Jan Beulich wrote:
> On 12.12.2023 07:49, Chen, Jiqian wrote:
>> On 2023/12/11 23:31, Roger Pau Monné wrote:
>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>  
>>>>      switch ( cmd )
>>>>      {
>>>> +    case PHYSDEVOP_setup_gsi:
>>>
>>> I think given the new approach on the Linux side patches, where
>>> pciback will configure the interrupt, there's no need to expose
>>> setup_gsi anymore?
>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
>>
>>>
>>>>      case PHYSDEVOP_map_pirq:
>>>>      case PHYSDEVOP_unmap_pirq:
>>>> +        if ( is_hardware_domain(currd) )
>>>> +            break;
>>>
>>> Also Jan already pointed this out in v2: this hypercall needs to be
>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
>>> the hypercall if DOMID_SELF or the passed domid matches the current
>>> domain domid.
>> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.
> 
> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
> looks questionable to me, but there might be reasons there.)
Map_pirq is to solve the check failure problem. (pci_add_dm_done-> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
Setup_gsi is because the gsi is never be unmasked, so the gsi is never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Jan Beulich]

On 13.12.2023 03:47, Chen, Jiqian wrote:
> On 2023/12/12 17:30, Jan Beulich wrote:
>> On 12.12.2023 07:49, Chen, Jiqian wrote:
>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>  
>>>>>      switch ( cmd )
>>>>>      {
>>>>> +    case PHYSDEVOP_setup_gsi:
>>>>
>>>> I think given the new approach on the Linux side patches, where
>>>> pciback will configure the interrupt, there's no need to expose
>>>> setup_gsi anymore?
>>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
>>>
>>>>
>>>>>      case PHYSDEVOP_map_pirq:
>>>>>      case PHYSDEVOP_unmap_pirq:
>>>>> +        if ( is_hardware_domain(currd) )
>>>>> +            break;
>>>>
>>>> Also Jan already pointed this out in v2: this hypercall needs to be
>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
>>>> the hypercall if DOMID_SELF or the passed domid matches the current
>>>> domain domid.
>>> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.
>>
>> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
>> looks questionable to me, but there might be reasons there.)
> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
> Setup_gsi is because the gsi is never be unmasked, so the gsi is never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).

And it was previously made pretty clear by Roger, I think, that doing a "map"
just for the purpose of granting permission is, well, at best a temporary
workaround in the early development phase. If there's presently no hypercall
to _only_ grant permission to IRQ, we need to add one. In fact "map" would
likely better not have done two things at a time from the very beginning ...

Jan

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Chen, Jiqian]

On 2023/12/13 15:03, Jan Beulich wrote:
> On 13.12.2023 03:47, Chen, Jiqian wrote:
>> On 2023/12/12 17:30, Jan Beulich wrote:
>>> On 12.12.2023 07:49, Chen, Jiqian wrote:
>>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
>>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>  
>>>>>>      switch ( cmd )
>>>>>>      {
>>>>>> +    case PHYSDEVOP_setup_gsi:
>>>>>
>>>>> I think given the new approach on the Linux side patches, where
>>>>> pciback will configure the interrupt, there's no need to expose
>>>>> setup_gsi anymore?
>>>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
>>>>
>>>>>
>>>>>>      case PHYSDEVOP_map_pirq:
>>>>>>      case PHYSDEVOP_unmap_pirq:
>>>>>> +        if ( is_hardware_domain(currd) )
>>>>>> +            break;
>>>>>
>>>>> Also Jan already pointed this out in v2: this hypercall needs to be
>>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
>>>>> the hypercall if DOMID_SELF or the passed domid matches the current
>>>>> domain domid.
>>>> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.
>>>
>>> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
>>> looks questionable to me, but there might be reasons there.)
>> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
>> Setup_gsi is because the gsi is never be unmasked, so the gsi is never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).
> 
> And it was previously made pretty clear by Roger, I think, that doing a "map"
> just for the purpose of granting permission is, well, at best a temporary
> workaround in the early development phase. If there's presently no hypercall
> to _only_ grant permission to IRQ, we need to add one.
Could you please describe it in detail? Do you mean to add a new hypercall to grant irq access for dom0 or domU?
It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq access from dom0 to domU(see XEN_DOMCTL_irq_permission-> irq_permit_access). There is no need to add hypercall to grant irq access.
We failed here (XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because the PVH dom0 didn't use PIRQ, so we can't get irq from pirq if "current" is PVH dom0.
So, it seems the logic of XEN_DOMCTL_irq_permission is not suitable when PVH dom0? Maybe it directly needs to get irq from the caller(domU) instead of "current" if the "current" has no PIRQ flag?

> In fact "map" would likely better not have done two things at a time from the very beginning ...
> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Jan Beulich]

On 14.12.2023 09:55, Chen, Jiqian wrote:
> On 2023/12/13 15:03, Jan Beulich wrote:
>> On 13.12.2023 03:47, Chen, Jiqian wrote:
>>> On 2023/12/12 17:30, Jan Beulich wrote:
>>>> On 12.12.2023 07:49, Chen, Jiqian wrote:
>>>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
>>>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>  
>>>>>>>      switch ( cmd )
>>>>>>>      {
>>>>>>> +    case PHYSDEVOP_setup_gsi:
>>>>>>
>>>>>> I think given the new approach on the Linux side patches, where
>>>>>> pciback will configure the interrupt, there's no need to expose
>>>>>> setup_gsi anymore?
>>>>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
>>>>>
>>>>>>
>>>>>>>      case PHYSDEVOP_map_pirq:
>>>>>>>      case PHYSDEVOP_unmap_pirq:
>>>>>>> +        if ( is_hardware_domain(currd) )
>>>>>>> +            break;
>>>>>>
>>>>>> Also Jan already pointed this out in v2: this hypercall needs to be
>>>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
>>>>>> the hypercall if DOMID_SELF or the passed domid matches the current
>>>>>> domain domid.
>>>>> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.
>>>>
>>>> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
>>>> looks questionable to me, but there might be reasons there.)
>>> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
>>> Setup_gsi is because the gsi is never be unmasked, so the gsi is never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).
>>
>> And it was previously made pretty clear by Roger, I think, that doing a "map"
>> just for the purpose of granting permission is, well, at best a temporary
>> workaround in the early development phase. If there's presently no hypercall
>> to _only_ grant permission to IRQ, we need to add one.
> Could you please describe it in detail? Do you mean to add a new hypercall to grant irq access for dom0 or domU?
> It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq access from dom0 to domU(see XEN_DOMCTL_irq_permission-> irq_permit_access). There is no need to add hypercall to grant irq access.

Hmm, yes and no. May I turn your attention to
https://lists.xen.org/archives/html/xen-devel/2023-07/msg02056.html
and its earlier version
https://lists.xen.org/archives/html/xen-devel/2023-05/msg00301.html
(it's imo a shame that this series continues to be stuck)?

Both make pretty clear that without pIRQ, this domctl cannot be used in
its present shape anyway, for ...

> We failed here (XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because the PVH dom0 didn't use PIRQ, so we can't get irq from pirq if "current" is PVH dom0.

... this very reason. Addressing this one way or another is a necessary
part of making passthrough work with PVH Dom0. So _effectively_ there is
no hypercall allowing PVH Dom0 to grant IRQ permission.

> So, it seems the logic of XEN_DOMCTL_irq_permission is not suitable when PVH dom0?

That's my view, yes.

> Maybe it directly needs to get irq from the caller(domU) instead of "current" if the "current" has no PIRQ flag?

I don't think the IRQ mapping in the DomU is necessary to be known here.
What we want to grant is access to a host resource. That host resource is
therefore all that should need specifying for the operation to be carried
out. It just so happens that a PV Dom0 would specify the host IRQ by way
of supplying its own equivalent pIRQ.

Things are more "interesting" for MSI, though: The (Xen) IRQ may not be
known early enough. There wants to be a way of indicating that when such
an IRQ is created, permission should be granted to the domain that is
going to use that IRQ (by way of being assigned the respective device).
(This aspect may be part of why "map" presently also grants permission,
yet I continue to think that was wrong from the start. The more that
access there is [likely needlessly] granted to the domain requesting the
mapping, just for it to then further grant access to the DomU.)

Jan

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Roger Pau Monné]

On Thu, Dec 14, 2023 at 08:55:45AM +0000, Chen, Jiqian wrote:
> On 2023/12/13 15:03, Jan Beulich wrote:
> > On 13.12.2023 03:47, Chen, Jiqian wrote:
> >> On 2023/12/12 17:30, Jan Beulich wrote:
> >>> On 12.12.2023 07:49, Chen, Jiqian wrote:
> >>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
> >>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
> >>>>>> --- a/xen/arch/x86/hvm/hypercall.c
> >>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
> >>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>>>  
> >>>>>>      switch ( cmd )
> >>>>>>      {
> >>>>>> +    case PHYSDEVOP_setup_gsi:
> >>>>>
> >>>>> I think given the new approach on the Linux side patches, where
> >>>>> pciback will configure the interrupt, there's no need to expose
> >>>>> setup_gsi anymore?
> >>>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
> >>>>
> >>>>>
> >>>>>>      case PHYSDEVOP_map_pirq:
> >>>>>>      case PHYSDEVOP_unmap_pirq:
> >>>>>> +        if ( is_hardware_domain(currd) )
> >>>>>> +            break;
> >>>>>
> >>>>> Also Jan already pointed this out in v2: this hypercall needs to be
> >>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
> >>>>> the hypercall if DOMID_SELF or the passed domid matches the current
> >>>>> domain domid.
> >>>> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.
> >>>
> >>> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
> >>> looks questionable to me, but there might be reasons there.)
> >> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
> >> Setup_gsi is because the gsi is never be unmasked, so the gsi is never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).
> > 
> > And it was previously made pretty clear by Roger, I think, that doing a "map"
> > just for the purpose of granting permission is, well, at best a temporary
> > workaround in the early development phase. If there's presently no hypercall
> > to _only_ grant permission to IRQ, we need to add one.
> Could you please describe it in detail? Do you mean to add a new hypercall to grant irq access for dom0 or domU?
> It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq access from dom0 to domU(see XEN_DOMCTL_irq_permission-> irq_permit_access). There is no need to add hypercall to grant irq access.
> We failed here (XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because the PVH dom0 didn't use PIRQ, so we can't get irq from pirq if "current" is PVH dom0.

One way to bodge this would be to detect whether the caller of
XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
assume the pirq field is a GSI.  I'm unsure however how that will work
with non-x86 architectures.

It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or
maybe XEN_DOMCTL_intr_permission that can take a struct we can use to
accommodate GSIs and other arch specific interrupt identifiers.

I'm also wondering whether the hypercall should be in a stable
interface so it could be easily used from QEMU if needed.

> So, it seems the logic of XEN_DOMCTL_irq_permission is not suitable when PVH dom0? Maybe it directly needs to get irq from the caller(domU) instead of "current" if the "current" has no PIRQ flag?

Hm, I'm kind of confused by this last sentence, as you mention "the
caller(domU)".  The caller of XEN_DOMCTL_irq_permission will always be
dom0 or the hardware domain.

Thanks, Roger.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Jan Beulich]

On 14.12.2023 10:55, Roger Pau Monné wrote:
> On Thu, Dec 14, 2023 at 08:55:45AM +0000, Chen, Jiqian wrote:
>> On 2023/12/13 15:03, Jan Beulich wrote:
>>> On 13.12.2023 03:47, Chen, Jiqian wrote:
>>>> On 2023/12/12 17:30, Jan Beulich wrote:
>>>>> On 12.12.2023 07:49, Chen, Jiqian wrote:
>>>>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
>>>>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
>>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>  
>>>>>>>>      switch ( cmd )
>>>>>>>>      {
>>>>>>>> +    case PHYSDEVOP_setup_gsi:
>>>>>>>
>>>>>>> I think given the new approach on the Linux side patches, where
>>>>>>> pciback will configure the interrupt, there's no need to expose
>>>>>>> setup_gsi anymore?
>>>>>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
>>>>>>
>>>>>>>
>>>>>>>>      case PHYSDEVOP_map_pirq:
>>>>>>>>      case PHYSDEVOP_unmap_pirq:
>>>>>>>> +        if ( is_hardware_domain(currd) )
>>>>>>>> +            break;
>>>>>>>
>>>>>>> Also Jan already pointed this out in v2: this hypercall needs to be
>>>>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
>>>>>>> the hypercall if DOMID_SELF or the passed domid matches the current
>>>>>>> domain domid.
>>>>>> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.
>>>>>
>>>>> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
>>>>> looks questionable to me, but there might be reasons there.)
>>>> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
>>>> Setup_gsi is because the gsi is never be unmasked, so the gsi is never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).
>>>
>>> And it was previously made pretty clear by Roger, I think, that doing a "map"
>>> just for the purpose of granting permission is, well, at best a temporary
>>> workaround in the early development phase. If there's presently no hypercall
>>> to _only_ grant permission to IRQ, we need to add one.
>> Could you please describe it in detail? Do you mean to add a new hypercall to grant irq access for dom0 or domU?
>> It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq access from dom0 to domU(see XEN_DOMCTL_irq_permission-> irq_permit_access). There is no need to add hypercall to grant irq access.
>> We failed here (XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because the PVH dom0 didn't use PIRQ, so we can't get irq from pirq if "current" is PVH dom0.
> 
> One way to bodge this would be to detect whether the caller of
> XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
> assume the pirq field is a GSI.  I'm unsure however how that will work
> with non-x86 architectures.
> 
> It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or
> maybe XEN_DOMCTL_intr_permission that can take a struct we can use to
> accommodate GSIs and other arch specific interrupt identifiers.

How would you see MSI being handled then?

Jan

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Roger Pau Monné]

On Thu, Dec 14, 2023 at 10:58:24AM +0100, Jan Beulich wrote:
> On 14.12.2023 10:55, Roger Pau Monné wrote:
> > On Thu, Dec 14, 2023 at 08:55:45AM +0000, Chen, Jiqian wrote:
> >> On 2023/12/13 15:03, Jan Beulich wrote:
> >>> On 13.12.2023 03:47, Chen, Jiqian wrote:
> >>>> On 2023/12/12 17:30, Jan Beulich wrote:
> >>>>> On 12.12.2023 07:49, Chen, Jiqian wrote:
> >>>>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
> >>>>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
> >>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
> >>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
> >>>>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>>>>>  
> >>>>>>>>      switch ( cmd )
> >>>>>>>>      {
> >>>>>>>> +    case PHYSDEVOP_setup_gsi:
> >>>>>>>
> >>>>>>> I think given the new approach on the Linux side patches, where
> >>>>>>> pciback will configure the interrupt, there's no need to expose
> >>>>>>> setup_gsi anymore?
> >>>>>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
> >>>>>>
> >>>>>>>
> >>>>>>>>      case PHYSDEVOP_map_pirq:
> >>>>>>>>      case PHYSDEVOP_unmap_pirq:
> >>>>>>>> +        if ( is_hardware_domain(currd) )
> >>>>>>>> +            break;
> >>>>>>>
> >>>>>>> Also Jan already pointed this out in v2: this hypercall needs to be
> >>>>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
> >>>>>>> the hypercall if DOMID_SELF or the passed domid matches the current
> >>>>>>> domain domid.
> >>>>>> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.
> >>>>>
> >>>>> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
> >>>>> looks questionable to me, but there might be reasons there.)
> >>>> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
> >>>> Setup_gsi is because the gsi is never be unmasked, so the gsi is never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).
> >>>
> >>> And it was previously made pretty clear by Roger, I think, that doing a "map"
> >>> just for the purpose of granting permission is, well, at best a temporary
> >>> workaround in the early development phase. If there's presently no hypercall
> >>> to _only_ grant permission to IRQ, we need to add one.
> >> Could you please describe it in detail? Do you mean to add a new hypercall to grant irq access for dom0 or domU?
> >> It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq access from dom0 to domU(see XEN_DOMCTL_irq_permission-> irq_permit_access). There is no need to add hypercall to grant irq access.
> >> We failed here (XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because the PVH dom0 didn't use PIRQ, so we can't get irq from pirq if "current" is PVH dom0.
> > 
> > One way to bodge this would be to detect whether the caller of
> > XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
> > assume the pirq field is a GSI.  I'm unsure however how that will work
> > with non-x86 architectures.
> > 
> > It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or
> > maybe XEN_DOMCTL_intr_permission that can take a struct we can use to
> > accommodate GSIs and other arch specific interrupt identifiers.
> 
> How would you see MSI being handled then?

I wasn't really accounting for MSI here, as MSI is not handled by
XEN_DOMCTL_irq_permission now either.  My plan long term was to
introduce a new hypercall (part of dm_ops possibly) in order to be
able to bind MSI directly without having to 'map' it first.

Roger.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4445 bytes --]

On Thu, 14 Dec 2023, Roger Pau Monné wrote:
> On Thu, Dec 14, 2023 at 10:58:24AM +0100, Jan Beulich wrote:
> > On 14.12.2023 10:55, Roger Pau Monné wrote:
> > > On Thu, Dec 14, 2023 at 08:55:45AM +0000, Chen, Jiqian wrote:
> > >> On 2023/12/13 15:03, Jan Beulich wrote:
> > >>> On 13.12.2023 03:47, Chen, Jiqian wrote:
> > >>>> On 2023/12/12 17:30, Jan Beulich wrote:
> > >>>>> On 12.12.2023 07:49, Chen, Jiqian wrote:
> > >>>>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
> > >>>>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
> > >>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
> > >>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
> > >>>>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> > >>>>>>>>  
> > >>>>>>>>      switch ( cmd )
> > >>>>>>>>      {
> > >>>>>>>> +    case PHYSDEVOP_setup_gsi:
> > >>>>>>>
> > >>>>>>> I think given the new approach on the Linux side patches, where
> > >>>>>>> pciback will configure the interrupt, there's no need to expose
> > >>>>>>> setup_gsi anymore?
> > >>>>>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
> > >>>>>>
> > >>>>>>>
> > >>>>>>>>      case PHYSDEVOP_map_pirq:
> > >>>>>>>>      case PHYSDEVOP_unmap_pirq:
> > >>>>>>>> +        if ( is_hardware_domain(currd) )
> > >>>>>>>> +            break;
> > >>>>>>>
> > >>>>>>> Also Jan already pointed this out in v2: this hypercall needs to be
> > >>>>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
> > >>>>>>> the hypercall if DOMID_SELF or the passed domid matches the current
> > >>>>>>> domain domid.
> > >>>>>> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.
> > >>>>>
> > >>>>> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
> > >>>>> looks questionable to me, but there might be reasons there.)
> > >>>> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
> > >>>> Setup_gsi is because the gsi is never be unmasked, so the gsi is never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).
> > >>>
> > >>> And it was previously made pretty clear by Roger, I think, that doing a "map"
> > >>> just for the purpose of granting permission is, well, at best a temporary
> > >>> workaround in the early development phase. If there's presently no hypercall
> > >>> to _only_ grant permission to IRQ, we need to add one.
> > >> Could you please describe it in detail? Do you mean to add a new hypercall to grant irq access for dom0 or domU?
> > >> It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq access from dom0 to domU(see XEN_DOMCTL_irq_permission-> irq_permit_access). There is no need to add hypercall to grant irq access.
> > >> We failed here (XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because the PVH dom0 didn't use PIRQ, so we can't get irq from pirq if "current" is PVH dom0.
> > > 
> > > One way to bodge this would be to detect whether the caller of
> > > XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
> > > assume the pirq field is a GSI.  I'm unsure however how that will work
> > > with non-x86 architectures.

PIRQ is an x86-only concept. We have event channels but no PIRQs on ARM.
I expect RISC-V will be the same.


> > > It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or

"GSI" is another x86-only concept.

So actually the best name was indeed XEN_DOMCTL_irq_permission, given
that it is using the more arch-neutral "irq" terminology.

Perhaps it was always a mistake to pass PIRQs to
XEN_DOMCTL_irq_permission and we should always have passed the real
interrupt number (GSI on x86, SPI on ARM).

So your "bodge" is actually kind of OK in my opinion. Basically everyone
else (x86 HVM/PVH, ARM, RISC-V, probably PPC too) will use
XEN_DOMCTL_irq_permission with hardware interrupt numbers (GSIs, SPIs,
etc.), the only special case is x86 PV. It is x86 PV the odd one.

Given that DOMCTL is an unstable interface anyway, I feel OK making
changes to it, even better if backward compatible.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Chen, Jiqian]

On 2023/12/15 06:49, Stefano Stabellini wrote:
> On Thu, 14 Dec 2023, Roger Pau Monné wrote:
>> On Thu, Dec 14, 2023 at 10:58:24AM +0100, Jan Beulich wrote:
>>> On 14.12.2023 10:55, Roger Pau Monné wrote:
>>>> On Thu, Dec 14, 2023 at 08:55:45AM +0000, Chen, Jiqian wrote:
>>>>> On 2023/12/13 15:03, Jan Beulich wrote:
>>>>>> On 13.12.2023 03:47, Chen, Jiqian wrote:
>>>>>>> On 2023/12/12 17:30, Jan Beulich wrote:
>>>>>>>> On 12.12.2023 07:49, Chen, Jiqian wrote:
>>>>>>>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
>>>>>>>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
>>>>>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>>>>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>>>>  
>>>>>>>>>>>      switch ( cmd )
>>>>>>>>>>>      {
>>>>>>>>>>> +    case PHYSDEVOP_setup_gsi:
>>>>>>>>>>
>>>>>>>>>> I think given the new approach on the Linux side patches, where
>>>>>>>>>> pciback will configure the interrupt, there's no need to expose
>>>>>>>>>> setup_gsi anymore?
>>>>>>>>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>      case PHYSDEVOP_map_pirq:
>>>>>>>>>>>      case PHYSDEVOP_unmap_pirq:
>>>>>>>>>>> +        if ( is_hardware_domain(currd) )
>>>>>>>>>>> +            break;
>>>>>>>>>>
>>>>>>>>>> Also Jan already pointed this out in v2: this hypercall needs to be
>>>>>>>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
>>>>>>>>>> the hypercall if DOMID_SELF or the passed domid matches the current
>>>>>>>>>> domain domid.
>>>>>>>>> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.
>>>>>>>>
>>>>>>>> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
>>>>>>>> looks questionable to me, but there might be reasons there.)
>>>>>>> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
>>>>>>> Setup_gsi is because the gsi is never be unmasked, so the gsi is never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).
>>>>>>
>>>>>> And it was previously made pretty clear by Roger, I think, that doing a "map"
>>>>>> just for the purpose of granting permission is, well, at best a temporary
>>>>>> workaround in the early development phase. If there's presently no hypercall
>>>>>> to _only_ grant permission to IRQ, we need to add one.
>>>>> Could you please describe it in detail? Do you mean to add a new hypercall to grant irq access for dom0 or domU?
>>>>> It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq access from dom0 to domU(see XEN_DOMCTL_irq_permission-> irq_permit_access). There is no need to add hypercall to grant irq access.
>>>>> We failed here (XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because the PVH dom0 didn't use PIRQ, so we can't get irq from pirq if "current" is PVH dom0.
>>>>
>>>> One way to bodge this would be to detect whether the caller of
>>>> XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
>>>> assume the pirq field is a GSI.  I'm unsure however how that will work
>>>> with non-x86 architectures.
> 
> PIRQ is an x86-only concept. We have event channels but no PIRQs on ARM.
> I expect RISC-V will be the same.
> 
> 
>>>> It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or
> 
> "GSI" is another x86-only concept.
> 
> So actually the best name was indeed XEN_DOMCTL_irq_permission, given
> that it is using the more arch-neutral "irq" terminology.
> 
> Perhaps it was always a mistake to pass PIRQs to
> XEN_DOMCTL_irq_permission and we should always have passed the real
> interrupt number (GSI on x86, SPI on ARM).
> 
> So your "bodge" is actually kind of OK in my opinion. Basically everyone
> else (x86 HVM/PVH, ARM, RISC-V, probably PPC too) will use
> XEN_DOMCTL_irq_permission with hardware interrupt numbers (GSIs, SPIs,
> etc.), the only special case is x86 PV. It is x86 PV the odd one.
> 
> Given that DOMCTL is an unstable interface anyway, I feel OK making
> changes to it, even better if backward compatible.
I try to understand your discussion about the modification of XEN_DOMCTL_irq_permission. At the xl level, gsi needs to be passed in instead of pirq, and then a judgment is added to XEN_DOMCTL_irq_permission, just like the implementation below?
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index d3507d13a029..f665d17afbf5 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1486,6 +1486,7 @@ static void pci_add_dm_done(libxl__egc *egc,
         goto out_no_irq;
     }
     if ((fscanf(f, "%u", &irq) == 1) && irq) {
+        int 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)",
@@ -1494,7 +1495,7 @@ static void pci_add_dm_done(libxl__egc *egc,
             rc = ERROR_FAIL;
             goto out;
         }
-        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
+        r = xc_domain_irq_permission(ctx->xch, domid, gsi, 1);
         if (r < 0) {
             LOGED(ERROR, domainid,
                   "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index f5a71ee5f78d..782c4a7a70a4 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -658,7 +658,12 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             ret = -EINVAL;
             break;
         }
-        irq = pirq_access_permitted(current->domain, pirq);
+
+        if ( is_hvm_domain(current->domain) )
+            irq = pirq;
+        else
+            irq = pirq_access_permitted(current->domain, pirq);
+
         if ( !irq || xsm_irq_permission(XSM_HOOK, d, irq, allow) )
             ret = -EPERM;
         else if ( allow )

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Roger Pau Monné]

On Thu, Dec 14, 2023 at 02:49:18PM -0800, Stefano Stabellini wrote:
> On Thu, 14 Dec 2023, Roger Pau Monné wrote:
> > On Thu, Dec 14, 2023 at 10:58:24AM +0100, Jan Beulich wrote:
> > > On 14.12.2023 10:55, Roger Pau Monné wrote:
> > > > On Thu, Dec 14, 2023 at 08:55:45AM +0000, Chen, Jiqian wrote:
> > > >> On 2023/12/13 15:03, Jan Beulich wrote:
> > > >>> On 13.12.2023 03:47, Chen, Jiqian wrote:
> > > >>>> On 2023/12/12 17:30, Jan Beulich wrote:
> > > >>>>> On 12.12.2023 07:49, Chen, Jiqian wrote:
> > > >>>>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
> > > >>>>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
> > > >>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
> > > >>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
> > > >>>>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> > > >>>>>>>>  
> > > >>>>>>>>      switch ( cmd )
> > > >>>>>>>>      {
> > > >>>>>>>> +    case PHYSDEVOP_setup_gsi:
> > > >>>>>>>
> > > >>>>>>> I think given the new approach on the Linux side patches, where
> > > >>>>>>> pciback will configure the interrupt, there's no need to expose
> > > >>>>>>> setup_gsi anymore?
> > > >>>>>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and map_pirq for passthrough device in pciback, so we need this and below.
> > > >>>>>>
> > > >>>>>>>
> > > >>>>>>>>      case PHYSDEVOP_map_pirq:
> > > >>>>>>>>      case PHYSDEVOP_unmap_pirq:
> > > >>>>>>>> +        if ( is_hardware_domain(currd) )
> > > >>>>>>>> +            break;
> > > >>>>>>>
> > > >>>>>>> Also Jan already pointed this out in v2: this hypercall needs to be
> > > >>>>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
> > > >>>>>>> the hypercall if DOMID_SELF or the passed domid matches the current
> > > >>>>>>> domain domid.
> > > >>>>>> Yes, I remember Jan's suggestion, but since the latest patch(the second patch of v3 on kernel side) has change the implementation, it does setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF check.
> > > >>>>>
> > > >>>>> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
> > > >>>>> looks questionable to me, but there might be reasons there.)
> > > >>>> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
> > > >>>> Setup_gsi is because the gsi is never be unmasked, so the gsi is never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).
> > > >>>
> > > >>> And it was previously made pretty clear by Roger, I think, that doing a "map"
> > > >>> just for the purpose of granting permission is, well, at best a temporary
> > > >>> workaround in the early development phase. If there's presently no hypercall
> > > >>> to _only_ grant permission to IRQ, we need to add one.
> > > >> Could you please describe it in detail? Do you mean to add a new hypercall to grant irq access for dom0 or domU?
> > > >> It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq access from dom0 to domU(see XEN_DOMCTL_irq_permission-> irq_permit_access). There is no need to add hypercall to grant irq access.
> > > >> We failed here (XEN_DOMCTL_irq_permission-> pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because the PVH dom0 didn't use PIRQ, so we can't get irq from pirq if "current" is PVH dom0.
> > > > 
> > > > One way to bodge this would be to detect whether the caller of
> > > > XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
> > > > assume the pirq field is a GSI.  I'm unsure however how that will work
> > > > with non-x86 architectures.
> 
> PIRQ is an x86-only concept. We have event channels but no PIRQs on ARM.
> I expect RISC-V will be the same.
> 
> 
> > > > It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or
> 
> "GSI" is another x86-only concept.

Yes, that hypercall would be x86-specific.

> So actually the best name was indeed XEN_DOMCTL_irq_permission, given
> that it is using the more arch-neutral "irq" terminology.
> 
> Perhaps it was always a mistake to pass PIRQs to
> XEN_DOMCTL_irq_permission and we should always have passed the real
> interrupt number (GSI on x86, SPI on ARM).

I really don't know much about Arm, but don't you also have LPIs, and
would need to add some kind of type field to
xen_domctl_irq_permission?

> So your "bodge" is actually kind of OK in my opinion. Basically everyone
> else (x86 HVM/PVH, ARM, RISC-V, probably PPC too) will use
> XEN_DOMCTL_irq_permission with hardware interrupt numbers (GSIs, SPIs,
> etc.), the only special case is x86 PV. It is x86 PV the odd one.

x86 PV could also pass the GSI if we wanted to change the interface
uniformly.  AFAICT the hypercall is only used by libxl, so would
likely be fine to change.

> Given that DOMCTL is an unstable interface anyway, I feel OK making
> changes to it, even better if backward compatible.

Me calling this a 'bodge' was mostly because I think it would be nice
to take the opportunity to move the hypercall to a stable
interface.

Thanks, Roger.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Jan Beulich]

On 14.12.2023 23:49, Stefano Stabellini wrote:
> On Thu, 14 Dec 2023, Roger Pau Monné wrote:
>> On Thu, Dec 14, 2023 at 10:58:24AM +0100, Jan Beulich wrote:
>>> On 14.12.2023 10:55, Roger Pau Monné wrote:
>>>> One way to bodge this would be to detect whether the caller of
>>>> XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
>>>> assume the pirq field is a GSI.  I'm unsure however how that will work
>>>> with non-x86 architectures.
> 
> PIRQ is an x86-only concept. We have event channels but no PIRQs on ARM.
> I expect RISC-V will be the same.
> 
> 
>>>> It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or
> 
> "GSI" is another x86-only concept.

Just to mention it - going through the ACPI spec, this looks to be an
arch-neutral ACPI term. It is also used in places which to me look
pretty Arm-centric.

Jan

> So actually the best name was indeed XEN_DOMCTL_irq_permission, given
> that it is using the more arch-neutral "irq" terminology.
> 
> Perhaps it was always a mistake to pass PIRQs to
> XEN_DOMCTL_irq_permission and we should always have passed the real
> interrupt number (GSI on x86, SPI on ARM).
> 
> So your "bodge" is actually kind of OK in my opinion. Basically everyone
> else (x86 HVM/PVH, ARM, RISC-V, probably PPC too) will use
> XEN_DOMCTL_irq_permission with hardware interrupt numbers (GSIs, SPIs,
> etc.), the only special case is x86 PV. It is x86 PV the odd one.
> 
> Given that DOMCTL is an unstable interface anyway, I feel OK making
> changes to it, even better if backward compatible.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Roger Pau Monné]

On Fri, Dec 15, 2023 at 07:20:24AM +0000, Chen, Jiqian wrote:
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index d3507d13a029..f665d17afbf5 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -1486,6 +1486,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>          goto out_no_irq;
>      }
>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
> +        int 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)",
> @@ -1494,7 +1495,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>              rc = ERROR_FAIL;
>              goto out;
>          }
> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
> +        r = xc_domain_irq_permission(ctx->xch, domid, gsi, 1);
>          if (r < 0) {
>              LOGED(ERROR, domainid,
>                    "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index f5a71ee5f78d..782c4a7a70a4 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -658,7 +658,12 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              ret = -EINVAL;
>              break;
>          }
> -        irq = pirq_access_permitted(current->domain, pirq);
> +
> +        if ( is_hvm_domain(current->domain) )
> +            irq = pirq;
> +        else
> +            irq = pirq_access_permitted(current->domain, pirq);

You are dropping an irq_access_permitted() check here for the HVM
case, as pirq_access_permitted() translates from pirq to irq and also
checks for permissions.

This would need to be something along the lines of:

irq = 0;
if ( is_hvm_domain(current->domain) &&
     irq_access_permitted(current->domain, pirq) )
    irq = pirq;
else
    irq = pirq_access_permitted(current->domain, pirq);

And then I wonder whether it wouldn't be best to uniformly use a GSI
for both PV and HVM.

Thanks, Roger.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Roger Pau Monné]

On Fri, Dec 15, 2023 at 09:24:22AM +0100, Jan Beulich wrote:
> On 14.12.2023 23:49, Stefano Stabellini wrote:
> > On Thu, 14 Dec 2023, Roger Pau Monné wrote:
> >> On Thu, Dec 14, 2023 at 10:58:24AM +0100, Jan Beulich wrote:
> >>> On 14.12.2023 10:55, Roger Pau Monné wrote:
> >>>> One way to bodge this would be to detect whether the caller of
> >>>> XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
> >>>> assume the pirq field is a GSI.  I'm unsure however how that will work
> >>>> with non-x86 architectures.
> > 
> > PIRQ is an x86-only concept. We have event channels but no PIRQs on ARM.
> > I expect RISC-V will be the same.
> > 
> > 
> >>>> It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or
> > 
> > "GSI" is another x86-only concept.
> 
> Just to mention it - going through the ACPI spec, this looks to be an
> arch-neutral ACPI term. It is also used in places which to me look
> pretty Arm-centric.

Oh, indeed, they have retrofitted GSI(V?) for Arm also, as a way to have a
"flat" uniform interrupt space.  So I guess Arm would also need the
GSI type, unless the translation from GSI to SPI or whatever platform
interrupt type is done by the guest and Xen is completely agnostic to
GSIs (if that's even possible).

Thanks, Roger.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

On Fri, 15 Dec 2023, Roger Pau Monné wrote:
> On Fri, Dec 15, 2023 at 09:24:22AM +0100, Jan Beulich wrote:
> > On 14.12.2023 23:49, Stefano Stabellini wrote:
> > > On Thu, 14 Dec 2023, Roger Pau Monné wrote:
> > >> On Thu, Dec 14, 2023 at 10:58:24AM +0100, Jan Beulich wrote:
> > >>> On 14.12.2023 10:55, Roger Pau Monné wrote:
> > >>>> One way to bodge this would be to detect whether the caller of
> > >>>> XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
> > >>>> assume the pirq field is a GSI.  I'm unsure however how that will work
> > >>>> with non-x86 architectures.
> > > 
> > > PIRQ is an x86-only concept. We have event channels but no PIRQs on ARM.
> > > I expect RISC-V will be the same.
> > > 
> > > 
> > >>>> It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or
> > > 
> > > "GSI" is another x86-only concept.
> > 
> > Just to mention it - going through the ACPI spec, this looks to be an
> > arch-neutral ACPI term. It is also used in places which to me look
> > pretty Arm-centric.
> 
> Oh, indeed, they have retrofitted GSI(V?) for Arm also, as a way to have a
> "flat" uniform interrupt space.

Interesting, and I am not surprised. (I don't usually work with ACPI on
ARM because none of our boards come with ACPI, they are all Device
Tree.)


> So I guess Arm would also need the
> GSI type, unless the translation from GSI to SPI or whatever platform
> interrupt type is done by the guest and Xen is completely agnostic to
> GSIs (if that's even possible).

I am guessing that GSIs on ARM must be mapped 1:1 to SPIs otherwise we
would have severe inconsistencies between ACPI and DeviceTree booting
and some boards support both.

Also to answer your question about LPIs: those are MSIs on ARM.

Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Chen, Jiqian]

On 2023/12/15 16:29, Roger Pau Monné wrote:
> On Fri, Dec 15, 2023 at 07:20:24AM +0000, Chen, Jiqian wrote:
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index d3507d13a029..f665d17afbf5 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1486,6 +1486,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>          goto out_no_irq;
>>      }
>>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> +        int 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)",
>> @@ -1494,7 +1495,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>              rc = ERROR_FAIL;
>>              goto out;
>>          }
>> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>> +        r = xc_domain_irq_permission(ctx->xch, domid, gsi, 1);
>>          if (r < 0) {
>>              LOGED(ERROR, domainid,
>>                    "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index f5a71ee5f78d..782c4a7a70a4 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -658,7 +658,12 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>              ret = -EINVAL;
>>              break;
>>          }
>> -        irq = pirq_access_permitted(current->domain, pirq);
>> +
>> +        if ( is_hvm_domain(current->domain) )
>> +            irq = pirq;
>> +        else
>> +            irq = pirq_access_permitted(current->domain, pirq);
> 
> You are dropping an irq_access_permitted() check here for the HVM
> case, as pirq_access_permitted() translates from pirq to irq and also
> checks for permissions.
> 
> This would need to be something along the lines of:
> 
> irq = 0;
> if ( is_hvm_domain(current->domain) &&
>      irq_access_permitted(current->domain, pirq) )
Oh, yes, it should add this check.

>     irq = pirq;
> else
>     irq = pirq_access_permitted(current->domain, pirq);
> 
> And then I wonder whether it wouldn't be best to uniformly use a GSI
> for both PV and HVM.
If we only look at the value(seems the number of gsi == pirq == irq in PV), it seems that gsi can also be used uniformly for PV.
And then here should be. 
if ( irq_access_permitted(current->domain, pirq) )
	irq = pirq;
else
{
	ret = -EPERM;
	break;
}

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.