[XEN PATCH v9 0/5] Support device passthrough when dom0 is PVH on Xen

[XEN PATCH v9 0/5] Support device passthrough when dom0 is PVH on Xen

[Jiqian Chen]

Hi All,
This is v9 series to support passthrough when dom0 is PVH
v8->v9 changes:
* patch#1: Move pcidevs_unlock below write_lock, and remove "ASSERT(pcidevs_locked());" from vpci_reset_device_state;
           Add pci_device_state_reset_type to distinguish the reset types.
* patch#2: Add a comment above PHYSDEVOP_map_pirq to describe why need this hypercall.
           Change "!is_pv_domain(d)" to "is_hvm_domain(d)", and "map.domid == DOMID_SELF" to "d == current->domian".
* patch#3: Remove the check of PHYSDEVOP_setup_gsi, since there is same checke in below.
* patch#5: Change the commit message to describe more why we need this new hypercall.
           Add comment above "if ( is_pv_domain(current->domain) || has_pirq(current->domain) )" to explain why we need this check.
		   Add gsi_2_irq to transform gsi to irq, instead of considering gsi == irq.
		   Add explicit padding to struct xen_domctl_gsi_permission.


Best regards,
Jiqian Chen



v7->v8 changes:
* patch#2: Add the domid check(domid == DOMID_SELF) to prevent self map when guest doesn't use pirq.
           That check was missed in the previous version.
* patch#4: Due to changes in the implementation of obtaining gsi in the kernel. Change to add a new function
           to get gsi by passing in the sbdf of pci device.
* patch#5: Remove the parameter "is_gsi", when there exist gsi, in pci_add_dm_done use a new function
           pci_device_set_gsi to do map_pirq and grant permission. That gets more intuitive code logic.


v6->v7 changes:
* patch#4: Due to changes in the implementation of obtaining gsi in the kernel. Change to add a new function
           to get gsi from irq, instead of gsi sysfs.
* patch#5: Fix the issue with variable usage, rc->r.


v5->v6 changes:
* patch#1: Add Reviewed-by Stefano and Stewart. Rebase code and change old function vpci_remove_device,
           vpci_add_handlers to vpci_deassign_device, vpci_assign_device
* patch#2: Add Reviewed-by Stefano
* patch#3: Remove unnecessary "ASSERT(!has_pirq(currd));"
* patch#4: Fix some coding style issues below directory tools
* patch#5: Modified some variable names and code logic to make code easier to be understood, which to use
           gsi by default and be compatible with older kernel versions to continue to use irq


v4->v5 changes:
* patch#1: add pci_lock wrap function vpci_reset_device_state
* patch#2: move the check of self map_pirq to physdev.c, and change to check if the caller has PIRQ flag, and
           just break for PHYSDEVOP_(un)map_pirq in hvm_physdev_op
* patch#3: return -EOPNOTSUPP instead, and use ASSERT(!has_pirq(currd));
* patch#4: is the patch#5 in v4 because patch#5 in v5 has some dependency on it. And add the handling of errno
           and add the Reviewed-by Stefano
* patch#5: is the patch#4 in v4. New implementation to add new hypercall XEN_DOMCTL_gsi_permission to grant gsi


v3->v4 changes:
* patch#1: change the comment of PHYSDEVOP_pci_device_state_reset; move printings behind pcidevs_unlock
* patch#2: add check to prevent PVH self map
* patch#3: new patch, The implementation of adding PHYSDEVOP_setup_gsi for PVH is treated as a separate patch
* patch#4: new patch to solve the map_pirq problem of PVH dom0. use gsi to grant irq permission in
           XEN_DOMCTL_irq_permission.
* patch#5: to be compatible with previous kernel versions, when there is no gsi sysfs, still use irq
v4 link:
https://lore.kernel.org/xen-devel/20240105070920.350113-1-Jiqian.Chen@amd.com/T/#t

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:
https://lore.kernel.org/xen-devel/20231210164009.1551147-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 \u201csudo xl pci-assignable-add <sbdf>\u201d 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()\u201d, 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 
bar->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\u2019t, 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\u2019s 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 (5):
  xen/vpci: Clear all vpci status of device
  x86/pvh: Allow (un)map_pirq when dom0 is PVH
  x86/pvh: Add PHYSDEVOP_setup_gsi for PVH dom0
  tools: Add new function to get gsi from dev
  domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

 tools/include/xen-sys/Linux/privcmd.h |  7 +++
 tools/include/xencall.h               |  2 +
 tools/include/xenctrl.h               |  7 +++
 tools/libs/call/core.c                |  5 +++
 tools/libs/call/libxencall.map        |  2 +
 tools/libs/call/linux.c               | 15 +++++++
 tools/libs/call/private.h             |  9 ++++
 tools/libs/ctrl/xc_domain.c           | 15 +++++++
 tools/libs/ctrl/xc_physdev.c          |  4 ++
 tools/libs/light/libxl_pci.c          | 63 +++++++++++++++++++++++++++
 xen/arch/x86/domctl.c                 | 38 ++++++++++++++++
 xen/arch/x86/hvm/hypercall.c          |  8 ++++
 xen/arch/x86/include/asm/io_apic.h    |  2 +
 xen/arch/x86/io_apic.c                | 21 +++++++++
 xen/arch/x86/mpparse.c                |  3 +-
 xen/arch/x86/physdev.c                | 24 ++++++++++
 xen/drivers/pci/physdev.c             | 43 ++++++++++++++++++
 xen/drivers/vpci/vpci.c               |  9 ++++
 xen/include/public/domctl.h           | 10 +++++
 xen/include/public/physdev.h          |  7 +++
 xen/include/xen/pci.h                 | 16 +++++++
 xen/include/xen/vpci.h                |  6 +++
 xen/xsm/flask/hooks.c                 |  1 +
 23 files changed, 315 insertions(+), 2 deletions(-)

-- 
2.34.1

[XEN PATCH v9 1/5] 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.

Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Reviewed-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/x86/hvm/hypercall.c |  1 +
 xen/drivers/pci/physdev.c    | 43 ++++++++++++++++++++++++++++++++++++
 xen/drivers/vpci/vpci.c      |  9 ++++++++
 xen/include/public/physdev.h |  7 ++++++
 xen/include/xen/pci.h        | 16 ++++++++++++++
 xen/include/xen/vpci.h       |  6 +++++
 6 files changed, 82 insertions(+)

diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 7fb3136f0c7c..0fab670a4871 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -83,6 +83,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 42db3e6d133c..1cce508a73b1 100644
--- a/xen/drivers/pci/physdev.c
+++ b/xen/drivers/pci/physdev.c
@@ -2,11 +2,17 @@
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
 #include <xen/init.h>
+#include <xen/vpci.h>
 
 #ifndef COMPAT
 typedef long ret_t;
 #endif
 
+static const struct pci_device_state_reset_method
+                    pci_device_state_reset_methods[] = {
+    [ DEVICE_RESET_FLR ].reset_fn = vpci_reset_device_state,
+};
+
 ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     ret_t ret;
@@ -67,6 +73,43 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case PHYSDEVOP_pci_device_state_reset: {
+        struct pci_device_state_reset dev_reset;
+        struct physdev_pci_device *dev;
+        struct pci_dev *pdev;
+        pci_sbdf_t sbdf;
+
+        if ( !is_pci_passthrough_enabled() )
+            return -EOPNOTSUPP;
+
+        ret = -EFAULT;
+        if ( copy_from_guest(&dev_reset, arg, 1) != 0 )
+            break;
+        dev = &dev_reset.dev;
+        sbdf = PCI_SBDF(dev->seg, dev->bus, dev->devfn);
+
+        ret = xsm_resource_setup_pci(XSM_PRIV, sbdf.sbdf);
+        if ( ret )
+            break;
+
+        pcidevs_lock();
+        pdev = pci_get_pdev(NULL, sbdf);
+        if ( !pdev )
+        {
+            pcidevs_unlock();
+            ret = -ENODEV;
+            break;
+        }
+
+        write_lock(&pdev->domain->pci_lock);
+        pcidevs_unlock();
+        ret = pci_device_state_reset_methods[dev_reset.reset_type].reset_fn(pdev);
+        write_unlock(&pdev->domain->pci_lock);
+        if ( ret )
+            printk(XENLOG_ERR "%pp: failed to reset vPCI device state\n", &sbdf);
+        break;
+    }
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 1e6aa5d799b9..ff67c2550ccb 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -172,6 +172,15 @@ int vpci_assign_device(struct pci_dev *pdev)
 
     return rc;
 }
+
+int vpci_reset_device_state(struct pci_dev *pdev)
+{
+    ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
+
+    vpci_deassign_device(pdev);
+    return vpci_assign_device(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 f0c0d4727c0b..a71da5892e5f 100644
--- a/xen/include/public/physdev.h
+++ b/xen/include/public/physdev.h
@@ -296,6 +296,13 @@ DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_add_t);
  */
 #define PHYSDEVOP_prepare_msix          30
 #define PHYSDEVOP_release_msix          31
+/*
+ * 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/pci.h b/xen/include/xen/pci.h
index 63e49f0117e9..376981f9da98 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -156,6 +156,22 @@ struct pci_dev {
     struct vpci *vpci;
 };
 
+struct pci_device_state_reset_method {
+    int (*reset_fn)(struct pci_dev *pdev);
+};
+
+enum pci_device_state_reset_type {
+    DEVICE_RESET_FLR,
+    DEVICE_RESET_COLD,
+    DEVICE_RESET_WARM,
+    DEVICE_RESET_HOT,
+};
+
+struct pci_device_state_reset {
+    struct physdev_pci_device dev;
+    enum pci_device_state_reset_type reset_type;
+};
+
 #define for_each_pdev(domain, pdev) \
     list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
 
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index da8d0f41e6f4..b230fd374de5 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -38,6 +38,7 @@ int __must_check vpci_assign_device(struct pci_dev *pdev);
 
 /* Remove all handlers and free vpci related structures. */
 void vpci_deassign_device(struct pci_dev *pdev);
+int __must_check vpci_reset_device_state(struct pci_dev *pdev);
 
 /* Add/remove a register handler. */
 int __must_check vpci_add_register_mask(struct vpci *vpci,
@@ -282,6 +283,11 @@ static inline int vpci_assign_device(struct pci_dev *pdev)
 
 static inline void vpci_deassign_device(struct pci_dev *pdev) { }
 
+static inline int __must_check 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

[XEN PATCH v9 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

[Jiqian Chen]

If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
a passthrough device by using gsi, see qemu code
xen_pt_realize->xc_physdev_map_pirq and libxl code
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 dom0 is PVH and also allow
PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
add a new check to prevent self map when subject domain has no
PIRQ flag.

Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/x86/hvm/hypercall.c |  6 ++++++
 xen/arch/x86/physdev.c       | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 0fab670a4871..fa5d50a0dd22 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -71,8 +71,14 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     switch ( cmd )
     {
+    /*
+     * Only being permitted for management of other domains.
+     * Further restrictions are enforced in do_physdev_op.
+     */
     case PHYSDEVOP_map_pirq:
     case PHYSDEVOP_unmap_pirq:
+        break;
+
     case PHYSDEVOP_eoi:
     case PHYSDEVOP_irq_status_query:
     case PHYSDEVOP_get_free_pirq:
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 7efa17cf4c1e..61999882f836 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -305,11 +305,23 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case PHYSDEVOP_map_pirq: {
         physdev_map_pirq_t map;
         struct msi_info msi;
+        struct domain *d;
 
         ret = -EFAULT;
         if ( copy_from_guest(&map, arg, 1) != 0 )
             break;
 
+        d = rcu_lock_domain_by_any_id(map.domid);
+        if ( d == NULL )
+            return -ESRCH;
+        /* Prevent self-map when domain has no X86_EMU_USE_PIRQ flag */
+        if ( is_hvm_domain(d) && !has_pirq(d) && d == current->domain )
+        {
+            rcu_unlock_domain(d);
+            return -EOPNOTSUPP;
+        }
+        rcu_unlock_domain(d);
+
         switch ( map.type )
         {
         case MAP_PIRQ_TYPE_MSI_SEG:
@@ -343,11 +355,23 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case PHYSDEVOP_unmap_pirq: {
         struct physdev_unmap_pirq unmap;
+        struct domain *d;
 
         ret = -EFAULT;
         if ( copy_from_guest(&unmap, arg, 1) != 0 )
             break;
 
+        d = rcu_lock_domain_by_any_id(unmap.domid);
+        if ( d == NULL )
+            return -ESRCH;
+        /* Prevent self-unmap when domain has no X86_EMU_USE_PIRQ flag */
+        if ( is_hvm_domain(d) && !has_pirq(d) && d == current->domain )
+        {
+            rcu_unlock_domain(d);
+            return -EOPNOTSUPP;
+        }
+        rcu_unlock_domain(d);
+
         ret = physdev_unmap_pirq(unmap.domid, unmap.pirq);
         break;
     }
-- 
2.34.1

[XEN PATCH v9 3/5] x86/pvh: Add PHYSDEVOP_setup_gsi for PVH dom0

[Jiqian Chen]

On 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.
On Linux kernel side, it calles PHYSDEVOP_setup_gsi for
passthrough devices to register gsi when dom0 is PVH.
So, add PHYSDEVOP_setup_gsi for above purpose.

Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
The code link that will call this hypercall on linux kernel side is as follows
https://lore.kernel.org/lkml/20240607075109.126277-3-Jiqian.Chen@amd.com/T/#u
---
 xen/arch/x86/hvm/hypercall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index fa5d50a0dd22..164f4eefa043 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -86,6 +86,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -ENOSYS;
         break;
 
+    case PHYSDEVOP_setup_gsi:
     case PHYSDEVOP_pci_mmcfg_reserved:
     case PHYSDEVOP_pci_device_add:
     case PHYSDEVOP_pci_device_remove:
-- 
2.34.1

[RFC XEN PATCH v9 4/5] tools: Add new function to get gsi from dev

[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
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, QEMU will use its gsi number
to do pirq mapping, see xen_pt_realize->xc_physdev_map_pirq,
but the gsi number is got from file
/sys/bus/pci/devices/<sbdf>/irq, so it will fail when mapping.
And in current codes, there is no method to get gsi for
userspace.

For above purpose, add new function to get gsi. And call this
function before xc_physdev_(un)map_pirq

Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Chen Jiqian <Jiqian.Chen@amd.com>
---
RFC: it needs review and needs to wait for the corresponding third patch on linux kernel side to be merged.
---
 tools/include/xen-sys/Linux/privcmd.h |  7 +++++++
 tools/include/xencall.h               |  2 ++
 tools/include/xenctrl.h               |  2 ++
 tools/libs/call/core.c                |  5 +++++
 tools/libs/call/libxencall.map        |  2 ++
 tools/libs/call/linux.c               | 15 +++++++++++++++
 tools/libs/call/private.h             |  9 +++++++++
 tools/libs/ctrl/xc_physdev.c          |  4 ++++
 tools/libs/light/libxl_pci.c          | 23 +++++++++++++++++++++++
 9 files changed, 69 insertions(+)

diff --git a/tools/include/xen-sys/Linux/privcmd.h b/tools/include/xen-sys/Linux/privcmd.h
index bc60e8fd55eb..977f1a058797 100644
--- a/tools/include/xen-sys/Linux/privcmd.h
+++ b/tools/include/xen-sys/Linux/privcmd.h
@@ -95,6 +95,11 @@ typedef struct privcmd_mmap_resource {
 	__u64 addr;
 } privcmd_mmap_resource_t;
 
+typedef struct privcmd_gsi_from_dev {
+	__u32 sbdf;
+	int gsi;
+} privcmd_gsi_from_dev_t;
+
 /*
  * @cmd: IOCTL_PRIVCMD_HYPERCALL
  * @arg: &privcmd_hypercall_t
@@ -114,6 +119,8 @@ typedef struct privcmd_mmap_resource {
 	_IOC(_IOC_NONE, 'P', 6, sizeof(domid_t))
 #define IOCTL_PRIVCMD_MMAP_RESOURCE				\
 	_IOC(_IOC_NONE, 'P', 7, sizeof(privcmd_mmap_resource_t))
+#define IOCTL_PRIVCMD_GSI_FROM_DEV				\
+	_IOC(_IOC_NONE, 'P', 10, sizeof(privcmd_gsi_from_dev_t))
 #define IOCTL_PRIVCMD_UNIMPLEMENTED				\
 	_IOC(_IOC_NONE, 'P', 0xFF, 0)
 
diff --git a/tools/include/xencall.h b/tools/include/xencall.h
index fc95ed0fe58e..750aab070323 100644
--- a/tools/include/xencall.h
+++ b/tools/include/xencall.h
@@ -113,6 +113,8 @@ int xencall5(xencall_handle *xcall, unsigned int op,
              uint64_t arg1, uint64_t arg2, uint64_t arg3,
              uint64_t arg4, uint64_t arg5);
 
+int xen_oscall_gsi_from_dev(xencall_handle *xcall, unsigned int sbdf);
+
 /* Variant(s) of the above, as needed, returning "long" instead of "int". */
 long xencall2L(xencall_handle *xcall, unsigned int op,
                uint64_t arg1, uint64_t arg2);
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 9ceca0cffc2f..a0381f74d24b 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1641,6 +1641,8 @@ int xc_physdev_unmap_pirq(xc_interface *xch,
                           uint32_t domid,
                           int pirq);
 
+int xc_physdev_gsi_from_dev(xc_interface *xch, uint32_t sbdf);
+
 /*
  *  LOGGING AND ERROR REPORTING
  */
diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
index 02c4f8e1aefa..6dae50c9a6ba 100644
--- a/tools/libs/call/core.c
+++ b/tools/libs/call/core.c
@@ -173,6 +173,11 @@ int xencall5(xencall_handle *xcall, unsigned int op,
     return osdep_hypercall(xcall, &call);
 }
 
+int xen_oscall_gsi_from_dev(xencall_handle *xcall, unsigned int sbdf)
+{
+    return osdep_oscall(xcall, sbdf);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/call/libxencall.map b/tools/libs/call/libxencall.map
index d18a3174e9dc..b92a0b5dc12c 100644
--- a/tools/libs/call/libxencall.map
+++ b/tools/libs/call/libxencall.map
@@ -10,6 +10,8 @@ VERS_1.0 {
 		xencall4;
 		xencall5;
 
+		xen_oscall_gsi_from_dev;
+
 		xencall_alloc_buffer;
 		xencall_free_buffer;
 		xencall_alloc_buffer_pages;
diff --git a/tools/libs/call/linux.c b/tools/libs/call/linux.c
index 6d588e6bea8f..92c740e176f2 100644
--- a/tools/libs/call/linux.c
+++ b/tools/libs/call/linux.c
@@ -85,6 +85,21 @@ long osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
     return ioctl(xcall->fd, IOCTL_PRIVCMD_HYPERCALL, hypercall);
 }
 
+int osdep_oscall(xencall_handle *xcall, unsigned int sbdf)
+{
+    privcmd_gsi_from_dev_t dev_gsi = {
+        .sbdf = sbdf,
+        .gsi = -1,
+    };
+
+    if (ioctl(xcall->fd, IOCTL_PRIVCMD_GSI_FROM_DEV, &dev_gsi)) {
+        PERROR("failed to get gsi from dev");
+        return -1;
+    }
+
+    return dev_gsi.gsi;
+}
+
 static void *alloc_pages_bufdev(xencall_handle *xcall, size_t npages)
 {
     void *p;
diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
index 9c3aa432efe2..cd6eb5a3e66f 100644
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -57,6 +57,15 @@ int osdep_xencall_close(xencall_handle *xcall);
 
 long osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall);
 
+#if defined(__linux__)
+int osdep_oscall(xencall_handle *xcall, unsigned int sbdf);
+#else
+static inline int osdep_oscall(xencall_handle *xcall, unsigned int sbdf)
+{
+    return -1;
+}
+#endif
+
 void *osdep_alloc_pages(xencall_handle *xcall, size_t nr_pages);
 void osdep_free_pages(xencall_handle *xcall, void *p, size_t nr_pages);
 
diff --git a/tools/libs/ctrl/xc_physdev.c b/tools/libs/ctrl/xc_physdev.c
index 460a8e779ce8..c1458f3a38b5 100644
--- a/tools/libs/ctrl/xc_physdev.c
+++ b/tools/libs/ctrl/xc_physdev.c
@@ -111,3 +111,7 @@ int xc_physdev_unmap_pirq(xc_interface *xch,
     return rc;
 }
 
+int xc_physdev_gsi_from_dev(xc_interface *xch, uint32_t sbdf)
+{
+    return xen_oscall_gsi_from_dev(xch->xcall, sbdf);
+}
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 96cb4da0794e..7e44d4c3ae2b 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1406,6 +1406,12 @@ static bool pci_supp_legacy_irq(void)
 #endif
 }
 
+#define PCI_DEVID(bus, devfn)\
+            ((((uint16_t)(bus)) << 8) | ((devfn) & 0xff))
+
+#define PCI_SBDF(seg, bus, devfn) \
+            ((((uint32_t)(seg)) << 16) | (PCI_DEVID(bus, devfn)))
+
 static void pci_add_dm_done(libxl__egc *egc,
                             pci_add_state *pas,
                             int rc)
@@ -1418,6 +1424,7 @@ static void pci_add_dm_done(libxl__egc *egc,
     unsigned long long start, end, flags, size;
     int irq, i;
     int r;
+    uint32_t sbdf;
     uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
     uint32_t domainid = domid;
     bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
@@ -1486,6 +1493,13 @@ static void pci_add_dm_done(libxl__egc *egc,
         goto out_no_irq;
     }
     if ((fscanf(f, "%u", &irq) == 1) && irq) {
+        sbdf = PCI_SBDF(pci->domain, pci->bus,
+                        (PCI_DEVFN(pci->dev, pci->func)));
+        r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
+        /* if fail, keep using irq; if success, r is gsi, use gsi */
+        if (r != -1) {
+            irq = r;
+        }
         r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
         if (r < 0) {
             LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
@@ -2172,8 +2186,10 @@ static void pci_remove_detached(libxl__egc *egc,
     int  irq = 0, i, stubdomid = 0;
     const char *sysfs_path;
     FILE *f;
+    uint32_t sbdf;
     uint32_t domainid = prs->domid;
     bool isstubdom;
+    int r;
 
     /* Convenience aliases */
     libxl_device_pci *const pci = &prs->pci;
@@ -2239,6 +2255,13 @@ skip_bar:
     }
 
     if ((fscanf(f, "%u", &irq) == 1) && irq) {
+        sbdf = PCI_SBDF(pci->domain, pci->bus,
+                        (PCI_DEVFN(pci->dev, pci->func)));
+        r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
+        /* if fail, keep using irq; if success, r is gsi, use gsi */
+        if (r != -1) {
+            irq = r;
+        }
         rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
         if (rc < 0) {
             /*
-- 
2.34.1

[RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Jiqian Chen]

Some type of domain don't have PIRQ, like PVH, it do not do
PHYSDEVOP_map_pirq for each gsi. When passthrough a device
to guest on PVH dom0, callstack
pci_add_dm_done->XEN_DOMCTL_irq_permission will failed at
domain_pirq_to_irq, because PVH has no mapping of gsi, pirq
and irq on Xen side.

What's more, current hypercall XEN_DOMCTL_irq_permission require
passing in pirq and grant the access of irq, it is not suitable
for dom0 that has no PIRQ flag, because passthrough a device
needs gsi and grant the corresponding irq to guest. So, add a
new hypercall to grant gsi permission when dom0 is not PV or dom0
has not PIRQ flag.

Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
RFC: it needs review and needs to wait for the corresponding third patch on linux kernel side to be merged.
---
 tools/include/xenctrl.h            |  5 +++
 tools/libs/ctrl/xc_domain.c        | 15 +++++++
 tools/libs/light/libxl_pci.c       | 72 +++++++++++++++++++++++-------
 xen/arch/x86/domctl.c              | 38 ++++++++++++++++
 xen/arch/x86/include/asm/io_apic.h |  2 +
 xen/arch/x86/io_apic.c             | 21 +++++++++
 xen/arch/x86/mpparse.c             |  3 +-
 xen/include/public/domctl.h        | 10 +++++
 xen/xsm/flask/hooks.c              |  1 +
 9 files changed, 149 insertions(+), 18 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index a0381f74d24b..f3feb6848e25 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1382,6 +1382,11 @@ int xc_domain_irq_permission(xc_interface *xch,
                              uint32_t pirq,
                              bool allow_access);
 
+int xc_domain_gsi_permission(xc_interface *xch,
+                             uint32_t domid,
+                             uint32_t gsi,
+                             bool allow_access);
+
 int xc_domain_iomem_permission(xc_interface *xch,
                                uint32_t domid,
                                unsigned long first_mfn,
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index f2d9d14b4d9f..8540e84fda93 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -1394,6 +1394,21 @@ int xc_domain_irq_permission(xc_interface *xch,
     return do_domctl(xch, &domctl);
 }
 
+int xc_domain_gsi_permission(xc_interface *xch,
+                             uint32_t domid,
+                             uint32_t gsi,
+                             bool allow_access)
+{
+    struct xen_domctl domctl = {
+        .cmd = XEN_DOMCTL_gsi_permission,
+        .domain = domid,
+        .u.gsi_permission.gsi = gsi,
+        .u.gsi_permission.allow_access = allow_access,
+    };
+
+    return do_domctl(xch, &domctl);
+}
+
 int xc_domain_iomem_permission(xc_interface *xch,
                                uint32_t domid,
                                unsigned long first_mfn,
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 7e44d4c3ae2b..b8ec37d8d7e3 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1412,6 +1412,37 @@ static bool pci_supp_legacy_irq(void)
 #define PCI_SBDF(seg, bus, devfn) \
             ((((uint32_t)(seg)) << 16) | (PCI_DEVID(bus, devfn)))
 
+static int pci_device_set_gsi(libxl_ctx *ctx,
+                              libxl_domid domid,
+                              libxl_device_pci *pci,
+                              bool map,
+                              int *gsi_back)
+{
+    int r, gsi, pirq;
+    uint32_t sbdf;
+
+    sbdf = PCI_SBDF(pci->domain, pci->bus, (PCI_DEVFN(pci->dev, pci->func)));
+    r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
+    *gsi_back = r;
+    if (r < 0)
+        return r;
+
+    gsi = r;
+    pirq = r;
+    if (map)
+        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
+    else
+        r = xc_physdev_unmap_pirq(ctx->xch, domid, pirq);
+    if (r)
+        return r;
+
+    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
+    if (r && errno == EOPNOTSUPP)
+        r = xc_domain_irq_permission(ctx->xch, domid, pirq, map);
+
+    return r;
+}
+
 static void pci_add_dm_done(libxl__egc *egc,
                             pci_add_state *pas,
                             int rc)
@@ -1424,10 +1455,10 @@ static void pci_add_dm_done(libxl__egc *egc,
     unsigned long long start, end, flags, size;
     int irq, i;
     int r;
-    uint32_t sbdf;
     uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
     uint32_t domainid = domid;
     bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
+    int gsi;
 
     /* Convenience aliases */
     bool starting = pas->starting;
@@ -1485,6 +1516,19 @@ static void pci_add_dm_done(libxl__egc *egc,
     fclose(f);
     if (!pci_supp_legacy_irq())
         goto out_no_irq;
+
+    r = pci_device_set_gsi(ctx, domid, pci, 1, &gsi);
+    if (gsi >= 0) {
+        if (r < 0) {
+            rc = ERROR_FAIL;
+            LOGED(ERROR, domainid,
+                  "pci_device_set_gsi gsi=%d (error=%d)", gsi, errno);
+            goto out;
+        } else {
+            goto process_permissive;
+        }
+    }
+    /* if gsi < 0, keep using irq */
     sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                                 pci->bus, pci->dev, pci->func);
     f = fopen(sysfs_path, "r");
@@ -1493,13 +1537,6 @@ static void pci_add_dm_done(libxl__egc *egc,
         goto out_no_irq;
     }
     if ((fscanf(f, "%u", &irq) == 1) && irq) {
-        sbdf = PCI_SBDF(pci->domain, pci->bus,
-                        (PCI_DEVFN(pci->dev, pci->func)));
-        r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
-        /* if fail, keep using irq; if success, r is gsi, use gsi */
-        if (r != -1) {
-            irq = r;
-        }
         r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
         if (r < 0) {
             LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
@@ -1519,6 +1556,7 @@ static void pci_add_dm_done(libxl__egc *egc,
     }
     fclose(f);
 
+process_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",
@@ -2186,10 +2224,10 @@ static void pci_remove_detached(libxl__egc *egc,
     int  irq = 0, i, stubdomid = 0;
     const char *sysfs_path;
     FILE *f;
-    uint32_t sbdf;
     uint32_t domainid = prs->domid;
     bool isstubdom;
     int r;
+    int gsi;
 
     /* Convenience aliases */
     libxl_device_pci *const pci = &prs->pci;
@@ -2245,6 +2283,15 @@ skip_bar:
     if (!pci_supp_legacy_irq())
         goto skip_legacy_irq;
 
+    r = pci_device_set_gsi(ctx, domid, pci, 0, &gsi);
+    if (gsi >= 0) {
+        if (r < 0) {
+            LOGED(ERROR, domainid,
+                  "pci_device_set_gsi gsi=%d (error=%d)", gsi, errno);
+        }
+        goto skip_legacy_irq;
+    }
+    /* if gsi < 0, keep using irq */
     sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                            pci->bus, pci->dev, pci->func);
 
@@ -2255,13 +2302,6 @@ skip_bar:
     }
 
     if ((fscanf(f, "%u", &irq) == 1) && irq) {
-        sbdf = PCI_SBDF(pci->domain, pci->bus,
-                        (PCI_DEVFN(pci->dev, pci->func)));
-        r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
-        /* if fail, keep using irq; if success, r is gsi, use gsi */
-        if (r != -1) {
-            irq = r;
-        }
         rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
         if (rc < 0) {
             /*
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9a72d57333e9..c69b4566ac4f 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -36,6 +36,7 @@
 #include <asm/xstate.h>
 #include <asm/psr.h>
 #include <asm/cpu-policy.h>
+#include <asm/io_apic.h>
 
 static int update_domain_cpu_policy(struct domain *d,
                                     xen_domctl_cpu_policy_t *xdpc)
@@ -237,6 +238,43 @@ long arch_do_domctl(
         break;
     }
 
+    case XEN_DOMCTL_gsi_permission:
+    {
+        unsigned int gsi = domctl->u.gsi_permission.gsi;
+        int irq = gsi_2_irq(gsi);
+        bool allow = domctl->u.gsi_permission.allow_access;
+
+        /*
+         * If current domain is PV or it has PIRQ flag, it has a mapping
+         * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission
+         * to grant irq permission.
+         */
+        if ( is_pv_domain(current->domain) || has_pirq(current->domain) )
+        {
+            ret = -EOPNOTSUPP;
+            break;
+        }
+
+        if ( gsi >= nr_irqs_gsi || irq < 0 )
+        {
+            ret = -EINVAL;
+            break;
+        }
+
+        if ( !irq_access_permitted(current->domain, irq) ||
+             xsm_irq_permission(XSM_HOOK, d, irq, allow) )
+        {
+            ret = -EPERM;
+            break;
+        }
+
+        if ( allow )
+            ret = irq_permit_access(d, irq);
+        else
+            ret = irq_deny_access(d, irq);
+        break;
+    }
+
     case XEN_DOMCTL_getpageframeinfo3:
     {
         unsigned int num = domctl->u.getpageframeinfo3.num;
diff --git a/xen/arch/x86/include/asm/io_apic.h b/xen/arch/x86/include/asm/io_apic.h
index 78268ea8f666..7e86d8337758 100644
--- a/xen/arch/x86/include/asm/io_apic.h
+++ b/xen/arch/x86/include/asm/io_apic.h
@@ -213,5 +213,7 @@ unsigned highest_gsi(void);
 
 int ioapic_guest_read( unsigned long physbase, unsigned int reg, u32 *pval);
 int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val);
+int mp_find_ioapic(int gsi);
+int gsi_2_irq(int gsi);
 
 #endif
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index b48a64246548..d03bcdef4d19 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -955,6 +955,27 @@ static int pin_2_irq(int idx, int apic, int pin)
     return irq;
 }
 
+int gsi_2_irq(int gsi)
+{
+    int entry, ioapic, pin;
+
+    ioapic = mp_find_ioapic(gsi);
+    if ( ioapic < 0 )
+        return -1;
+
+    pin = gsi - io_apic_gsi_base(ioapic);
+
+    entry = find_irq_entry(ioapic, pin, mp_INT);
+    /*
+     * If there is no override mapping for irq and gsi in mp_irqs,
+     * then the default identity mapping applies.
+     */
+    if ( entry < 0 )
+        return gsi;
+
+    return pin_2_irq(entry, ioapic, pin);
+}
+
 static inline int IO_APIC_irq_trigger(int irq)
 {
     int apic, idx, pin;
diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c
index d8ccab2449c6..c95da0de5770 100644
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -841,8 +841,7 @@ static struct mp_ioapic_routing {
 } mp_ioapic_routing[MAX_IO_APICS];
 
 
-static int mp_find_ioapic (
-	int			gsi)
+int mp_find_ioapic(int gsi)
 {
 	unsigned int		i;
 
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 2a49fe46ce25..f933af8722f4 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -465,6 +465,14 @@ struct xen_domctl_irq_permission {
 };
 
 
+/* XEN_DOMCTL_gsi_permission */
+struct xen_domctl_gsi_permission {
+    uint32_t gsi;
+    uint8_t allow_access;    /* flag to specify enable/disable of x86 gsi access */
+    uint8_t pad[3];
+};
+
+
 /* XEN_DOMCTL_iomem_permission */
 struct xen_domctl_iomem_permission {
     uint64_aligned_t first_mfn;/* first page (physical page number) in range */
@@ -1306,6 +1314,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_get_paging_mempool_size       85
 #define XEN_DOMCTL_set_paging_mempool_size       86
 #define XEN_DOMCTL_dt_overlay                    87
+#define XEN_DOMCTL_gsi_permission                88
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1328,6 +1337,7 @@ struct xen_domctl {
         struct xen_domctl_setdomainhandle   setdomainhandle;
         struct xen_domctl_setdebugging      setdebugging;
         struct xen_domctl_irq_permission    irq_permission;
+        struct xen_domctl_gsi_permission    gsi_permission;
         struct xen_domctl_iomem_permission  iomem_permission;
         struct xen_domctl_ioport_permission ioport_permission;
         struct xen_domctl_hypercall_init    hypercall_init;
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 5e88c71b8e22..a5b134c91101 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -685,6 +685,7 @@ static int cf_check flask_domctl(struct domain *d, int cmd)
     case XEN_DOMCTL_shadow_op:
     case XEN_DOMCTL_ioport_permission:
     case XEN_DOMCTL_ioport_mapping:
+    case XEN_DOMCTL_gsi_permission:
 #endif
 #ifdef CONFIG_HAS_PASSTHROUGH
     /*
-- 
2.34.1

Re: [XEN PATCH v9 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

[Jan Beulich]

On 07.06.2024 10:11, 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 qemu code
> xen_pt_realize->xc_physdev_map_pirq and libxl code
> 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 dom0 is PVH and also allow
> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
> add a new check to prevent self map when subject domain has no
> PIRQ flag.
> 
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

What's imo missing in the description is a clarification / justification of
why it is going to be a good idea (or at least an acceptable one) to expose
the concept of PIRQs to PVH. If I'm not mistaken that concept so far has
been entirely a PV one.

> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -71,8 +71,14 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      switch ( cmd )
>      {
> +    /*
> +     * Only being permitted for management of other domains.
> +     * Further restrictions are enforced in do_physdev_op.
> +     */
>      case PHYSDEVOP_map_pirq:
>      case PHYSDEVOP_unmap_pirq:
> +        break;

Nit: Imo such a comment ought to be indented like code (statements), not
like the case labels.

Jan

Re: [XEN PATCH v9 3/5] x86/pvh: Add PHYSDEVOP_setup_gsi for PVH dom0

[Jan Beulich]

On 07.06.2024 10:11, Jiqian Chen wrote:
> On 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.
> On Linux kernel side, it calles PHYSDEVOP_setup_gsi for
> passthrough devices to register gsi when dom0 is PVH.

"it calls" implies that ...

> So, add PHYSDEVOP_setup_gsi for above purpose.
> 
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> The code link that will call this hypercall on linux kernel side is as follows
> https://lore.kernel.org/lkml/20240607075109.126277-3-Jiqian.Chen@amd.com/T/#u

... the code only to be added there would already be upstream. As I think the
hypervisor change wants to come first, this part of the description will want
re-wording to along the lines of "will need to" or some such.

As to GSIs not being registered: If that's not a problem for Dom0's own
operation, I think it'll also want/need explaining why what is sufficient for
Dom0 alone isn't sufficient when pass-through comes into play.

Jan

Re: [XEN PATCH v9 0/5] Support device passthrough when dom0 is PVH on Xen

[Jan Beulich]

On 07.06.2024 10:11, Jiqian Chen wrote:
> Hi All,
> This is v9 series to support passthrough when dom0 is PVH
> v8->v9 changes:
> * patch#1: Move pcidevs_unlock below write_lock, and remove "ASSERT(pcidevs_locked());" from vpci_reset_device_state;
>            Add pci_device_state_reset_type to distinguish the reset types.
> * patch#2: Add a comment above PHYSDEVOP_map_pirq to describe why need this hypercall.
>            Change "!is_pv_domain(d)" to "is_hvm_domain(d)", and "map.domid == DOMID_SELF" to "d == current->domian".
> * patch#3: Remove the check of PHYSDEVOP_setup_gsi, since there is same checke in below.

Having looked at patch 3, what check(s) is (are) being talked about here?
It feels as if to understand this revision log entry, one would still need
to go back to the earlier version. Yet the purpose of these is that one
(preferably) wouldn't need to do so.

Jan

Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Jan Beulich]

On 07.06.2024 10:11, Jiqian Chen wrote:
> Some type of domain don't have PIRQ, like PVH, it do not do
> PHYSDEVOP_map_pirq for each gsi. When passthrough a device
> to guest on PVH dom0, callstack
> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed at
> domain_pirq_to_irq, because PVH has no mapping of gsi, pirq
> and irq on Xen side.

All of this is, to me at least, in pretty sharp contradiction to what
patch 2 says and does. IOW: Do we want the concept of pIRQ in PVH, or
do we want to keep that to PV?

> What's more, current hypercall XEN_DOMCTL_irq_permission require
> passing in pirq and grant the access of irq, it is not suitable
> for dom0 that has no PIRQ flag, because passthrough a device
> needs gsi and grant the corresponding irq to guest. So, add a
> new hypercall to grant gsi permission when dom0 is not PV or dom0
> has not PIRQ flag.
> 
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>

A problem throughout the series as it seems: Who's the author of these
patches? There's no From: saying it's not you, but your S-o-b also
isn't first.

> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -1412,6 +1412,37 @@ static bool pci_supp_legacy_irq(void)
>  #define PCI_SBDF(seg, bus, devfn) \
>              ((((uint32_t)(seg)) << 16) | (PCI_DEVID(bus, devfn)))
>  
> +static int pci_device_set_gsi(libxl_ctx *ctx,
> +                              libxl_domid domid,
> +                              libxl_device_pci *pci,
> +                              bool map,
> +                              int *gsi_back)
> +{
> +    int r, gsi, pirq;
> +    uint32_t sbdf;
> +
> +    sbdf = PCI_SBDF(pci->domain, pci->bus, (PCI_DEVFN(pci->dev, pci->func)));
> +    r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
> +    *gsi_back = r;
> +    if (r < 0)
> +        return r;
> +
> +    gsi = r;
> +    pirq = r;

r is a GSI as per above; why would you store such in a variable named pirq?
And how can ...

> +    if (map)
> +        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
> +    else
> +        r = xc_physdev_unmap_pirq(ctx->xch, domid, pirq);

... that value be the correct one to pass into here? In fact, the pIRQ number
you obtain above in the "map" case isn't handed to the caller, i.e. it is
effectively lost. Yet that's what would need passing into such an unmap call.

> +    if (r)
> +        return r;
> +
> +    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);

Looking at the hypervisor side, this will fail for PV Dom0. In which case imo
you better would avoid making the call in the first place.

> +    if (r && errno == EOPNOTSUPP)

Before here you don't really need the pIRQ number; if all it really is needed
for is ...

> +        r = xc_domain_irq_permission(ctx->xch, domid, pirq, map);

... this, then it probably also should only be obtained when it's needed. Yet
overall the intentions here aren't quite clear to me.

> @@ -1485,6 +1516,19 @@ static void pci_add_dm_done(libxl__egc *egc,
>      fclose(f);
>      if (!pci_supp_legacy_irq())
>          goto out_no_irq;
> +
> +    r = pci_device_set_gsi(ctx, domid, pci, 1, &gsi);
> +    if (gsi >= 0) {
> +        if (r < 0) {

This unusual way of error checking likely wants a comment.

> +            rc = ERROR_FAIL;
> +            LOGED(ERROR, domainid,
> +                  "pci_device_set_gsi gsi=%d (error=%d)", gsi, errno);
> +            goto out;
> +        } else {
> +            goto process_permissive;
> +        }

Btw, no need for "else" when the earlier if ends in "goto" or alike.

> @@ -1493,13 +1537,6 @@ static void pci_add_dm_done(libxl__egc *egc,
>          goto out_no_irq;
>      }
>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
> -        sbdf = PCI_SBDF(pci->domain, pci->bus,
> -                        (PCI_DEVFN(pci->dev, pci->func)));
> -        r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
> -        /* if fail, keep using irq; if success, r is gsi, use gsi */
> -        if (r != -1) {
> -            irq = r;
> -        }

If I'm not mistaken, this and ...

> @@ -2255,13 +2302,6 @@ skip_bar:
>      }
>  
>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
> -        sbdf = PCI_SBDF(pci->domain, pci->bus,
> -                        (PCI_DEVFN(pci->dev, pci->func)));
> -        r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
> -        /* if fail, keep using irq; if success, r is gsi, use gsi */
> -        if (r != -1) {
> -            irq = r;
> -        }

... this is code added by the immediately preceding patch. It's pretty odd
for that to be deleted here again right away. Can the interaction of the
two patches perhaps be re-arranged to avoid this anomaly?

> @@ -237,6 +238,43 @@ long arch_do_domctl(
>          break;
>      }
>  
> +    case XEN_DOMCTL_gsi_permission:
> +    {
> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
> +        int irq = gsi_2_irq(gsi);

I'm not sure it is a good idea to issue this call ahead of the basic error
checks below.

> +        bool allow = domctl->u.gsi_permission.allow_access;

This allows any non-zero values to mean "true". I think you want to bail
on values larger than 1, much like you also want to check that the padding
fields are all zero.

> +        /*
> +         * If current domain is PV or it has PIRQ flag, it has a mapping
> +         * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission
> +         * to grant irq permission.
> +         */
> +        if ( is_pv_domain(current->domain) || has_pirq(current->domain) )

Please use currd here (and also again below).

> +        {
> +            ret = -EOPNOTSUPP;
> +            break;
> +        }
> +
> +        if ( gsi >= nr_irqs_gsi || irq < 0 )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        if ( !irq_access_permitted(current->domain, irq) ||
> +             xsm_irq_permission(XSM_HOOK, d, irq, allow) )

Daniel, is it okay to issue the XSM check using the translated value, not
the one that was originally passed into the hypercall?

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -955,6 +955,27 @@ static int pin_2_irq(int idx, int apic, int pin)
>      return irq;
>  }
>  
> +int gsi_2_irq(int gsi)
> +{
> +    int entry, ioapic, pin;
> +
> +    ioapic = mp_find_ioapic(gsi);
> +    if ( ioapic < 0 )
> +        return -1;

Can this be a proper errno value (likely -EINVAL), please?

> +    pin = gsi - io_apic_gsi_base(ioapic);

Hmm, instead of the below: Once you have an (ioapic,pin) tuple, can't
you simply call apic_pin_2_gsi_irq()?

> +    entry = find_irq_entry(ioapic, pin, mp_INT);
> +    /*
> +     * If there is no override mapping for irq and gsi in mp_irqs,
> +     * then the default identity mapping applies.
> +     */
> +    if ( entry < 0 )
> +        return gsi;
> +
> +    return pin_2_irq(entry, ioapic, pin);

Under certain conditions this may return 0. Yet you surely don't want
to pass IRQ0 back as a result; you want to hand an error back instead.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -465,6 +465,14 @@ struct xen_domctl_irq_permission {
>  };
>  
>  
> +/* XEN_DOMCTL_gsi_permission */
> +struct xen_domctl_gsi_permission {
> +    uint32_t gsi;
> +    uint8_t allow_access;    /* flag to specify enable/disable of x86 gsi access */
> +    uint8_t pad[3];
> +};
> +
> +

Nit: No (new) double blank lines please. In fact (didn't I say this before
already?) you could insert between the two above, such that the existing issue
also disappears.

Jan

Re: [XEN PATCH v9 0/5] Support device passthrough when dom0 is PVH on Xen

[Chen, Jiqian]

On 2024/6/11 00:07, Jan Beulich wrote:
> On 07.06.2024 10:11, Jiqian Chen wrote:
>> Hi All,
>> This is v9 series to support passthrough when dom0 is PVH
>> v8->v9 changes:
>> * patch#1: Move pcidevs_unlock below write_lock, and remove "ASSERT(pcidevs_locked());" from vpci_reset_device_state;
>>            Add pci_device_state_reset_type to distinguish the reset types.
>> * patch#2: Add a comment above PHYSDEVOP_map_pirq to describe why need this hypercall.
>>            Change "!is_pv_domain(d)" to "is_hvm_domain(d)", and "map.domid == DOMID_SELF" to "d == current->domian".
>> * patch#3: Remove the check of PHYSDEVOP_setup_gsi, since there is same checke in below.
> 
> Having looked at patch 3, what check(s) is (are) being talked about here?
> It feels as if to understand this revision log entry, one would still need
> to go back to the earlier version. Yet the purpose of these is that one
> (preferably) wouldn't need to do so.
Sorry, it should be:
patch#3: Remove the check of PHYSDEVOP_setup_gsi, since there is same check in below. Although their return values are different, this difference is acceptable for the sake of code consistency
                 if ( !is_hardware_domain(currd) )
	             return -ENOSYS;
                 break;
I will change in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [XEN PATCH v9 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

[Chen, Jiqian]

On 2024/6/10 23:58, Jan Beulich wrote:
> On 07.06.2024 10:11, 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 qemu code
>> xen_pt_realize->xc_physdev_map_pirq and libxl code
>> 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 dom0 is PVH and also allow
>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
>> add a new check to prevent self map when subject domain has no
>> PIRQ flag.
>>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> What's imo missing in the description is a clarification / justification of
> why it is going to be a good idea (or at least an acceptable one) to expose
> the concept of PIRQs to PVH. If I'm not mistaken that concept so far has
> been entirely a PV one.
I didn't want to expose the concept of PIRQs to PVH.
I did this patch is for HVM that use PIRQs, what I said in commit message is HVM will map a pirq for gsi, not PVH.
For the original code, it checks " !has_pirq(currd)", but currd is PVH dom0, so it failed. So I need to allow PHYSDEVOP_map_pirq
even currd has no PIRQs, but the subject domain has.

> 
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -71,8 +71,14 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>  
>>      switch ( cmd )
>>      {
>> +    /*
>> +     * Only being permitted for management of other domains.
>> +     * Further restrictions are enforced in do_physdev_op.
>> +     */
>>      case PHYSDEVOP_map_pirq:
>>      case PHYSDEVOP_unmap_pirq:
>> +        break;
> 
> Nit: Imo such a comment ought to be indented like code (statements), not
> like the case labels.
Thanks, I will change in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [XEN PATCH v9 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

[Jan Beulich]

On 12.06.2024 04:43, Chen, Jiqian wrote:
> On 2024/6/10 23:58, Jan Beulich wrote:
>> On 07.06.2024 10:11, 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 qemu code
>>> xen_pt_realize->xc_physdev_map_pirq and libxl code
>>> 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 dom0 is PVH and also allow
>>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
>>> add a new check to prevent self map when subject domain has no
>>> PIRQ flag.
>>>
>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> What's imo missing in the description is a clarification / justification of
>> why it is going to be a good idea (or at least an acceptable one) to expose
>> the concept of PIRQs to PVH. If I'm not mistaken that concept so far has
>> been entirely a PV one.
> I didn't want to expose the concept of PIRQs to PVH.
> I did this patch is for HVM that use PIRQs, what I said in commit message is HVM will map a pirq for gsi, not PVH.
> For the original code, it checks " !has_pirq(currd)", but currd is PVH dom0, so it failed. So I need to allow PHYSDEVOP_map_pirq
> even currd has no PIRQs, but the subject domain has.

But that's not what you're enforcing in do_physdev_op(). There you only
prevent self-mapping. If I'm not mistaken all you need to do is drop the
"d == current->domain" checks from those conditionals.

Further see also
https://lists.xen.org/archives/html/xen-devel/2024-06/msg00540.html.

Jan

Re: [XEN PATCH v9 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

[Chen, Jiqian]

On 2024/6/12 16:53, Jan Beulich wrote:
> On 12.06.2024 04:43, Chen, Jiqian wrote:
>> On 2024/6/10 23:58, Jan Beulich wrote:
>>> On 07.06.2024 10:11, 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 qemu code
>>>> xen_pt_realize->xc_physdev_map_pirq and libxl code
>>>> 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 dom0 is PVH and also allow
>>>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
>>>> add a new check to prevent self map when subject domain has no
>>>> PIRQ flag.
>>>>
>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>
>>> What's imo missing in the description is a clarification / justification of
>>> why it is going to be a good idea (or at least an acceptable one) to expose
>>> the concept of PIRQs to PVH. If I'm not mistaken that concept so far has
>>> been entirely a PV one.
>> I didn't want to expose the concept of PIRQs to PVH.
>> I did this patch is for HVM that use PIRQs, what I said in commit message is HVM will map a pirq for gsi, not PVH.
>> For the original code, it checks " !has_pirq(currd)", but currd is PVH dom0, so it failed. So I need to allow PHYSDEVOP_map_pirq
>> even currd has no PIRQs, but the subject domain has.
> 
> But that's not what you're enforcing in do_physdev_op(). There you only
> prevent self-mapping. If I'm not mistaken all you need to do is drop the
> "d == current->domain" checks from those conditionals.
What I want is to allow PHYSDEVOP_map_pirq when currd doesn't have PIRQs, but subject domain has.
Then I just add "break" in hvm_physdev_op without any checks, that will cause self-mapping problems.
And in previous mail thread, you suggested me to prevent self-mapping when subject domain doesn't have PIRQs.
So I added checks in do_physdev_op.

> 
> Further see also
> https://lists.xen.org/archives/html/xen-devel/2024-06/msg00540.html.
> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [XEN PATCH v9 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

[Jan Beulich]

On 12.06.2024 11:07, Chen, Jiqian wrote:
> On 2024/6/12 16:53, Jan Beulich wrote:
>> On 12.06.2024 04:43, Chen, Jiqian wrote:
>>> On 2024/6/10 23:58, Jan Beulich wrote:
>>>> On 07.06.2024 10:11, 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 qemu code
>>>>> xen_pt_realize->xc_physdev_map_pirq and libxl code
>>>>> 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 dom0 is PVH and also allow
>>>>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
>>>>> add a new check to prevent self map when subject domain has no
>>>>> PIRQ flag.
>>>>>
>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>
>>>> What's imo missing in the description is a clarification / justification of
>>>> why it is going to be a good idea (or at least an acceptable one) to expose
>>>> the concept of PIRQs to PVH. If I'm not mistaken that concept so far has
>>>> been entirely a PV one.
>>> I didn't want to expose the concept of PIRQs to PVH.
>>> I did this patch is for HVM that use PIRQs, what I said in commit message is HVM will map a pirq for gsi, not PVH.
>>> For the original code, it checks " !has_pirq(currd)", but currd is PVH dom0, so it failed. So I need to allow PHYSDEVOP_map_pirq
>>> even currd has no PIRQs, but the subject domain has.
>>
>> But that's not what you're enforcing in do_physdev_op(). There you only
>> prevent self-mapping. If I'm not mistaken all you need to do is drop the
>> "d == current->domain" checks from those conditionals.
> What I want is to allow PHYSDEVOP_map_pirq when currd doesn't have PIRQs, but subject domain has.
> Then I just add "break" in hvm_physdev_op without any checks, that will cause self-mapping problems.
> And in previous mail thread, you suggested me to prevent self-mapping when subject domain doesn't have PIRQs.
> So I added checks in do_physdev_op.

Self-mapping was a primary concern of mine. Yet why deal with only a subset
of what needs preventing, when generalizing things actually can be done by
having less code.

Jan

Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Chen, Jiqian]

Hi Jan,

On 2024/6/11 22:39, Jan Beulich wrote:
> On 07.06.2024 10:11, Jiqian Chen wrote:
>> Some type of domain don't have PIRQ, like PVH, it do not do
>> PHYSDEVOP_map_pirq for each gsi. When passthrough a device
>> to guest on PVH dom0, callstack
>> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed at
>> domain_pirq_to_irq, because PVH has no mapping of gsi, pirq
>> and irq on Xen side.
> 
> All of this is, to me at least, in pretty sharp contradiction to what
> patch 2 says and does. IOW: Do we want the concept of pIRQ in PVH, or
> do we want to keep that to PV?
It's not contradictory.
What I did is not to add the concept of PIRQs for PVH.
All previous passthrough code was implemented on the basis of pv dom0 + hvm domU.
For pv dom0, it has PIRQs. For hvm domU, it has PIRQs too.
So the codes are not suitable for PVH dom0 + hvm domU, because PVH dom0 has no PIRQs.
Patch 2 do PHYSDEVOP_map_pirq for hvm domU even when dom0 is PVH instead of PV. It didn't add PIRQs for PVH.
This patch is to grant irq( that get from gsi ) to hvm domU, why XEN_DOMCTL_irq_permission is not useful is because PVH has no PIRQs, we can't get irq through pirq like PV does.

> 
>> What's more, current hypercall XEN_DOMCTL_irq_permission require
>> passing in pirq and grant the access of irq, it is not suitable
>> for dom0 that has no PIRQ flag, because passthrough a device
>> needs gsi and grant the corresponding irq to guest. So, add a
>> new hypercall to grant gsi permission when dom0 is not PV or dom0
>> has not PIRQ flag.
>>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> 
> A problem throughout the series as it seems: Who's the author of these
> patches? There's no From: saying it's not you, but your S-o-b also
> isn't first.
So I need to change to:
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> means I am the author.
Signed-off-by: Huang Rui <ray.huang@amd.com> means Rui sent them to upstream firstly.
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> means I take continue to upstream.

> 
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1412,6 +1412,37 @@ static bool pci_supp_legacy_irq(void)
>>  #define PCI_SBDF(seg, bus, devfn) \
>>              ((((uint32_t)(seg)) << 16) | (PCI_DEVID(bus, devfn)))
>>  
>> +static int pci_device_set_gsi(libxl_ctx *ctx,
>> +                              libxl_domid domid,
>> +                              libxl_device_pci *pci,
>> +                              bool map,
>> +                              int *gsi_back)
>> +{
>> +    int r, gsi, pirq;
>> +    uint32_t sbdf;
>> +
>> +    sbdf = PCI_SBDF(pci->domain, pci->bus, (PCI_DEVFN(pci->dev, pci->func)));
>> +    r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
>> +    *gsi_back = r;
>> +    if (r < 0)
>> +        return r;
>> +
>> +    gsi = r;
>> +    pirq = r;
> 
> r is a GSI as per above; why would you store such in a variable named pirq?
> And how can ...
> 
>> +    if (map)
>> +        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
>> +    else
>> +        r = xc_physdev_unmap_pirq(ctx->xch, domid, pirq);
> 
> ... that value be the correct one to pass into here? In fact, the pIRQ number
> you obtain above in the "map" case isn't handed to the caller, i.e. it is
> effectively lost. Yet that's what would need passing into such an unmap call.
Yes r is GSI and I know pirq will be replaced by xc_physdev_map_pirq.
What I do "pirq = r" is for xc_physdev_unmap_pirq, unmap need passing in pirq,
and the number of pirq is always equal to gsi.

> 
>> +    if (r)
>> +        return r;
>> +
>> +    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
> 
> Looking at the hypervisor side, this will fail for PV Dom0. In which case imo
> you better would avoid making the call in the first place.
Yes, for PV dom0, the errno is EOPNOTSUPP, then it will do below xc_domain_irq_permission.

> 
>> +    if (r && errno == EOPNOTSUPP)
> 
> Before here you don't really need the pIRQ number; if all it really is needed
> for is ...
> 
>> +        r = xc_domain_irq_permission(ctx->xch, domid, pirq, map);
> 
> ... this, then it probably also should only be obtained when it's needed. Yet
> overall the intentions here aren't quite clear to me.
Adding the function pci_device_set_gsi is for PVH dom0, while also ensuring compatibility with PV dom0.
When PVH dom0, it does xc_physdev_map_pirq and xc_domain_gsi_permission(new hypercall for PVH dom0)
When PV dom0, it keeps the same actions as before codes, it does xc_physdev_map_pirq and xc_domain_irq_permission.

> 
>> @@ -1485,6 +1516,19 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      fclose(f);
>>      if (!pci_supp_legacy_irq())
>>          goto out_no_irq;
>> +
>> +    r = pci_device_set_gsi(ctx, domid, pci, 1, &gsi);
>> +    if (gsi >= 0) {
>> +        if (r < 0) {
> 
> This unusual way of error checking likely wants a comment.
Will add in next version.

> 
>> +            rc = ERROR_FAIL;
>> +            LOGED(ERROR, domainid,
>> +                  "pci_device_set_gsi gsi=%d (error=%d)", gsi, errno);
>> +            goto out;
>> +        } else {
>> +            goto process_permissive;
>> +        }
> 
> Btw, no need for "else" when the earlier if ends in "goto" or alike.
OK, I will change in next version.

> 
>> @@ -1493,13 +1537,6 @@ static void pci_add_dm_done(libxl__egc *egc,
>>          goto out_no_irq;
>>      }
>>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> -        sbdf = PCI_SBDF(pci->domain, pci->bus,
>> -                        (PCI_DEVFN(pci->dev, pci->func)));
>> -        r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
>> -        /* if fail, keep using irq; if success, r is gsi, use gsi */
>> -        if (r != -1) {
>> -            irq = r;
>> -        }
> 
> If I'm not mistaken, this and ...
> 
>> @@ -2255,13 +2302,6 @@ skip_bar:
>>      }
>>  
>>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> -        sbdf = PCI_SBDF(pci->domain, pci->bus,
>> -                        (PCI_DEVFN(pci->dev, pci->func)));
>> -        r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
>> -        /* if fail, keep using irq; if success, r is gsi, use gsi */
>> -        if (r != -1) {
>> -            irq = r;
>> -        }
> 
> ... this is code added by the immediately preceding patch. It's pretty odd
> for that to be deleted here again right away. Can the interaction of the
> two patches perhaps be re-arranged to avoid this anomaly?
OK, I will do in next version.

> 
>> @@ -237,6 +238,43 @@ long arch_do_domctl(
>>          break;
>>      }
>>  
>> +    case XEN_DOMCTL_gsi_permission:
>> +    {
>> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
>> +        int irq = gsi_2_irq(gsi);
> 
> I'm not sure it is a good idea to issue this call ahead of the basic error
> checks below.
I will move it below the checks.

> 
>> +        bool allow = domctl->u.gsi_permission.allow_access;
> 
> This allows any non-zero values to mean "true". I think you want to bail
> on values larger than 1, much like you also want to check that the padding
> fields are all zero.
Will change in next version.

> 
>> +        /*
>> +         * If current domain is PV or it has PIRQ flag, it has a mapping
>> +         * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission
>> +         * to grant irq permission.
>> +         */
>> +        if ( is_pv_domain(current->domain) || has_pirq(current->domain) )
> 
> Please use currd here (and also again below).
Will change in next version.
> 
>> +        {
>> +            ret = -EOPNOTSUPP;
>> +            break;
>> +        }
>> +
>> +        if ( gsi >= nr_irqs_gsi || irq < 0 )
>> +        {
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +
>> +        if ( !irq_access_permitted(current->domain, irq) ||
>> +             xsm_irq_permission(XSM_HOOK, d, irq, allow) )
> 
> Daniel, is it okay to issue the XSM check using the translated value, not
> the one that was originally passed into the hypercall?
> 
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -955,6 +955,27 @@ static int pin_2_irq(int idx, int apic, int pin)
>>      return irq;
>>  }
>>  
>> +int gsi_2_irq(int gsi)
>> +{
>> +    int entry, ioapic, pin;
>> +
>> +    ioapic = mp_find_ioapic(gsi);
>> +    if ( ioapic < 0 )
>> +        return -1;
> 
> Can this be a proper errno value (likely -EINVAL), please?
Will change in next version.
> 
>> +    pin = gsi - io_apic_gsi_base(ioapic);
> 
> Hmm, instead of the below: Once you have an (ioapic,pin) tuple, can't
> you simply call apic_pin_2_gsi_irq()?
Will change in next version.
> 
>> +    entry = find_irq_entry(ioapic, pin, mp_INT);
>> +    /*
>> +     * If there is no override mapping for irq and gsi in mp_irqs,
>> +     * then the default identity mapping applies.
>> +     */
>> +    if ( entry < 0 )
>> +        return gsi;
>> +
>> +    return pin_2_irq(entry, ioapic, pin);
> 
> Under certain conditions this may return 0. Yet you surely don't want
> to pass IRQ0 back as a result; you want to hand an error back instead.
Will add in next version.
> 
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -465,6 +465,14 @@ struct xen_domctl_irq_permission {
>>  };
>>  
>>  
>> +/* XEN_DOMCTL_gsi_permission */
>> +struct xen_domctl_gsi_permission {
>> +    uint32_t gsi;
>> +    uint8_t allow_access;    /* flag to specify enable/disable of x86 gsi access */
>> +    uint8_t pad[3];
>> +};
>> +
>> +
> 
> Nit: No (new) double blank lines please. In fact (didn't I say this before
> already?) you could insert between the two above, such that the existing issue
> also disappears.
I remember I changed it here, maybe I made a mistake somewhere, and I will modify it in the next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [XEN PATCH v9 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

[Chen, Jiqian]

On 2024/6/12 17:21, Jan Beulich wrote:
> On 12.06.2024 11:07, Chen, Jiqian wrote:
>> On 2024/6/12 16:53, Jan Beulich wrote:
>>> On 12.06.2024 04:43, Chen, Jiqian wrote:
>>>> On 2024/6/10 23:58, Jan Beulich wrote:
>>>>> On 07.06.2024 10:11, 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 qemu code
>>>>>> xen_pt_realize->xc_physdev_map_pirq and libxl code
>>>>>> 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 dom0 is PVH and also allow
>>>>>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
>>>>>> add a new check to prevent self map when subject domain has no
>>>>>> PIRQ flag.
>>>>>>
>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>>
>>>>> What's imo missing in the description is a clarification / justification of
>>>>> why it is going to be a good idea (or at least an acceptable one) to expose
>>>>> the concept of PIRQs to PVH. If I'm not mistaken that concept so far has
>>>>> been entirely a PV one.
>>>> I didn't want to expose the concept of PIRQs to PVH.
>>>> I did this patch is for HVM that use PIRQs, what I said in commit message is HVM will map a pirq for gsi, not PVH.
>>>> For the original code, it checks " !has_pirq(currd)", but currd is PVH dom0, so it failed. So I need to allow PHYSDEVOP_map_pirq
>>>> even currd has no PIRQs, but the subject domain has.
>>>
>>> But that's not what you're enforcing in do_physdev_op(). There you only
>>> prevent self-mapping. If I'm not mistaken all you need to do is drop the
>>> "d == current->domain" checks from those conditionals.
>> What I want is to allow PHYSDEVOP_map_pirq when currd doesn't have PIRQs, but subject domain has.
>> Then I just add "break" in hvm_physdev_op without any checks, that will cause self-mapping problems.
>> And in previous mail thread, you suggested me to prevent self-mapping when subject domain doesn't have PIRQs.
>> So I added checks in do_physdev_op.
> 
> Self-mapping was a primary concern of mine. Yet why deal with only a subset
> of what needs preventing, when generalizing things actually can be done by
> having less code.
Make sense. I will rebase the branch once your codes are merged.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [XEN PATCH v9 3/5] x86/pvh: Add PHYSDEVOP_setup_gsi for PVH dom0

[Chen, Jiqian]

On 2024/6/11 00:04, Jan Beulich wrote:
> On 07.06.2024 10:11, Jiqian Chen wrote:
>> On 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.
>> On Linux kernel side, it calles PHYSDEVOP_setup_gsi for
>> passthrough devices to register gsi when dom0 is PVH.
> 
> "it calls" implies that ...
> 
>> So, add PHYSDEVOP_setup_gsi for above purpose.
>>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>> The code link that will call this hypercall on linux kernel side is as follows
>> https://lore.kernel.org/lkml/20240607075109.126277-3-Jiqian.Chen@amd.com/T/#u
> 
> ... the code only to be added there would already be upstream. As I think the
> hypervisor change wants to come first, this part of the description will want
> re-wording to along the lines of "will need to" or some such.
Thanks, I will change in next version.

> 
> As to GSIs not being registered: If that's not a problem for Dom0's own
> operation, I think it'll also want/need explaining why what is sufficient for
> Dom0 alone isn't sufficient when pass-through comes into play.
OK, I will add in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Jan Beulich]

On 12.06.2024 12:12, Chen, Jiqian wrote:
> On 2024/6/11 22:39, Jan Beulich wrote:
>> On 07.06.2024 10:11, Jiqian Chen wrote:
>>> Some type of domain don't have PIRQ, like PVH, it do not do
>>> PHYSDEVOP_map_pirq for each gsi. When passthrough a device
>>> to guest on PVH dom0, callstack
>>> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed at
>>> domain_pirq_to_irq, because PVH has no mapping of gsi, pirq
>>> and irq on Xen side.
>>
>> All of this is, to me at least, in pretty sharp contradiction to what
>> patch 2 says and does. IOW: Do we want the concept of pIRQ in PVH, or
>> do we want to keep that to PV?
> It's not contradictory.
> What I did is not to add the concept of PIRQs for PVH.

After your further explanations on patch 2 - yes, I see now. But in particular
there it needs making more clear what case it is that is being enabled by the
changes.

>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>
>> A problem throughout the series as it seems: Who's the author of these
>> patches? There's no From: saying it's not you, but your S-o-b also
>> isn't first.
> So I need to change to:
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> means I am the author.
> Signed-off-by: Huang Rui <ray.huang@amd.com> means Rui sent them to upstream firstly.
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> means I take continue to upstream.

I guess so, yes.

>>> --- a/tools/libs/light/libxl_pci.c
>>> +++ b/tools/libs/light/libxl_pci.c
>>> @@ -1412,6 +1412,37 @@ static bool pci_supp_legacy_irq(void)
>>>  #define PCI_SBDF(seg, bus, devfn) \
>>>              ((((uint32_t)(seg)) << 16) | (PCI_DEVID(bus, devfn)))
>>>  
>>> +static int pci_device_set_gsi(libxl_ctx *ctx,
>>> +                              libxl_domid domid,
>>> +                              libxl_device_pci *pci,
>>> +                              bool map,
>>> +                              int *gsi_back)
>>> +{
>>> +    int r, gsi, pirq;
>>> +    uint32_t sbdf;
>>> +
>>> +    sbdf = PCI_SBDF(pci->domain, pci->bus, (PCI_DEVFN(pci->dev, pci->func)));
>>> +    r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
>>> +    *gsi_back = r;
>>> +    if (r < 0)
>>> +        return r;
>>> +
>>> +    gsi = r;
>>> +    pirq = r;
>>
>> r is a GSI as per above; why would you store such in a variable named pirq?
>> And how can ...
>>
>>> +    if (map)
>>> +        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
>>> +    else
>>> +        r = xc_physdev_unmap_pirq(ctx->xch, domid, pirq);
>>
>> ... that value be the correct one to pass into here? In fact, the pIRQ number
>> you obtain above in the "map" case isn't handed to the caller, i.e. it is
>> effectively lost. Yet that's what would need passing into such an unmap call.
> Yes r is GSI and I know pirq will be replaced by xc_physdev_map_pirq.
> What I do "pirq = r" is for xc_physdev_unmap_pirq, unmap need passing in pirq,
> and the number of pirq is always equal to gsi.

Why would that be? pIRQ is purely a software construct (of Xen's), I
don't think there's any guarantee whatsoever on the numbering. And even
if there was (for e.g. non-MSI ones), it would be pIRQ == IRQ. And recall
that elsewhere I think I meanwhile succeeded in explaining to you that
IRQ != GSI (in the common case, even if in most cases they match).

>>> +    if (r)
>>> +        return r;
>>> +
>>> +    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
>>
>> Looking at the hypervisor side, this will fail for PV Dom0. In which case imo
>> you better would avoid making the call in the first place.
> Yes, for PV dom0, the errno is EOPNOTSUPP, then it will do below xc_domain_irq_permission.

Hence why call xc_domain_gsi_permission() at all on a PV Dom0?

>>> +    if (r && errno == EOPNOTSUPP)
>>
>> Before here you don't really need the pIRQ number; if all it really is needed
>> for is ...
>>
>>> +        r = xc_domain_irq_permission(ctx->xch, domid, pirq, map);
>>
>> ... this, then it probably also should only be obtained when it's needed. Yet
>> overall the intentions here aren't quite clear to me.
> Adding the function pci_device_set_gsi is for PVH dom0, while also ensuring compatibility with PV dom0.
> When PVH dom0, it does xc_physdev_map_pirq and xc_domain_gsi_permission(new hypercall for PVH dom0)
> When PV dom0, it keeps the same actions as before codes, it does xc_physdev_map_pirq and xc_domain_irq_permission.

And why does PVH Dom0 need to call xc_physdev_map_pirq(), when in that case
the pIRQ isn't used?

Jan

Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Chen, Jiqian]

On 2024/6/12 18:34, Jan Beulich wrote:
> On 12.06.2024 12:12, Chen, Jiqian wrote:
>> On 2024/6/11 22:39, Jan Beulich wrote:
>>> On 07.06.2024 10:11, Jiqian Chen wrote:
>>>> Some type of domain don't have PIRQ, like PVH, it do not do
>>>> PHYSDEVOP_map_pirq for each gsi. When passthrough a device
>>>> to guest on PVH dom0, callstack
>>>> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed at
>>>> domain_pirq_to_irq, because PVH has no mapping of gsi, pirq
>>>> and irq on Xen side.
>>>
>>> All of this is, to me at least, in pretty sharp contradiction to what
>>> patch 2 says and does. IOW: Do we want the concept of pIRQ in PVH, or
>>> do we want to keep that to PV?
>> It's not contradictory.
>> What I did is not to add the concept of PIRQs for PVH.
> 
> After your further explanations on patch 2 - yes, I see now. But in particular
> there it needs making more clear what case it is that is being enabled by the
> changes.
OK, I will add some descriptions in next version.

> 
>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>>
>>> A problem throughout the series as it seems: Who's the author of these
>>> patches? There's no From: saying it's not you, but your S-o-b also
>>> isn't first.
>> So I need to change to:
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> means I am the author.
>> Signed-off-by: Huang Rui <ray.huang@amd.com> means Rui sent them to upstream firstly.
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> means I take continue to upstream.
> 
> I guess so, yes.
Thanks.

> 
>>>> --- a/tools/libs/light/libxl_pci.c
>>>> +++ b/tools/libs/light/libxl_pci.c
>>>> @@ -1412,6 +1412,37 @@ static bool pci_supp_legacy_irq(void)
>>>>  #define PCI_SBDF(seg, bus, devfn) \
>>>>              ((((uint32_t)(seg)) << 16) | (PCI_DEVID(bus, devfn)))
>>>>  
>>>> +static int pci_device_set_gsi(libxl_ctx *ctx,
>>>> +                              libxl_domid domid,
>>>> +                              libxl_device_pci *pci,
>>>> +                              bool map,
>>>> +                              int *gsi_back)
>>>> +{
>>>> +    int r, gsi, pirq;
>>>> +    uint32_t sbdf;
>>>> +
>>>> +    sbdf = PCI_SBDF(pci->domain, pci->bus, (PCI_DEVFN(pci->dev, pci->func)));
>>>> +    r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
>>>> +    *gsi_back = r;
>>>> +    if (r < 0)
>>>> +        return r;
>>>> +
>>>> +    gsi = r;
>>>> +    pirq = r;
>>>
>>> r is a GSI as per above; why would you store such in a variable named pirq?
>>> And how can ...
>>>
>>>> +    if (map)
>>>> +        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
>>>> +    else
>>>> +        r = xc_physdev_unmap_pirq(ctx->xch, domid, pirq);
>>>
>>> ... that value be the correct one to pass into here? In fact, the pIRQ number
>>> you obtain above in the "map" case isn't handed to the caller, i.e. it is
>>> effectively lost. Yet that's what would need passing into such an unmap call.
>> Yes r is GSI and I know pirq will be replaced by xc_physdev_map_pirq.
>> What I do "pirq = r" is for xc_physdev_unmap_pirq, unmap need passing in pirq,
>> and the number of pirq is always equal to gsi.
> 
> Why would that be? pIRQ is purely a software construct (of Xen's), I
> don't think there's any guarantee whatsoever on the numbering. And even
> if there was (for e.g. non-MSI ones), it would be pIRQ == IRQ. And recall
> that elsewhere I think I meanwhile succeeded in explaining to you that
> IRQ != GSI (in the common case, even if in most cases they match).
OK, will change in next version.

> 
>>>> +    if (r)
>>>> +        return r;
>>>> +
>>>> +    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
>>>
>>> Looking at the hypervisor side, this will fail for PV Dom0. In which case imo
>>> you better would avoid making the call in the first place.
>> Yes, for PV dom0, the errno is EOPNOTSUPP, then it will do below xc_domain_irq_permission.
> 
> Hence why call xc_domain_gsi_permission() at all on a PV Dom0?
Is there a function to distinguish that current dom0 is PV or PVH dom0 in tools/libs?

> 
>>>> +    if (r && errno == EOPNOTSUPP)
>>>
>>> Before here you don't really need the pIRQ number; if all it really is needed
>>> for is ...
>>>
>>>> +        r = xc_domain_irq_permission(ctx->xch, domid, pirq, map);
>>>
>>> ... this, then it probably also should only be obtained when it's needed. Yet
>>> overall the intentions here aren't quite clear to me.
>> Adding the function pci_device_set_gsi is for PVH dom0, while also ensuring compatibility with PV dom0.
>> When PVH dom0, it does xc_physdev_map_pirq and xc_domain_gsi_permission(new hypercall for PVH dom0)
>> When PV dom0, it keeps the same actions as before codes, it does xc_physdev_map_pirq and xc_domain_irq_permission.
> 
> And why does PVH Dom0 need to call xc_physdev_map_pirq(), when in that case
> the pIRQ isn't used?
I didn't expect that introducing pci_device_set_gsi causes so many confusions.
I will remove it in the next version and make modifications directly in pci_add_dm_done.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Jan Beulich]

On 12.06.2024 12:55, Chen, Jiqian wrote:
> On 2024/6/12 18:34, Jan Beulich wrote:
>> On 12.06.2024 12:12, Chen, Jiqian wrote:
>>> On 2024/6/11 22:39, Jan Beulich wrote:
>>>> On 07.06.2024 10:11, Jiqian Chen wrote:
>>>>> +    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
>>>>
>>>> Looking at the hypervisor side, this will fail for PV Dom0. In which case imo
>>>> you better would avoid making the call in the first place.
>>> Yes, for PV dom0, the errno is EOPNOTSUPP, then it will do below xc_domain_irq_permission.
>>
>> Hence why call xc_domain_gsi_permission() at all on a PV Dom0?
> Is there a function to distinguish that current dom0 is PV or PVH dom0 in tools/libs?

That's a question to the tools folks, I suppose?

Jan

Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Anthony PERARD]

On Wed, Jun 12, 2024 at 10:55:14AM +0000, Chen, Jiqian wrote:
> On 2024/6/12 18:34, Jan Beulich wrote:
> > On 12.06.2024 12:12, Chen, Jiqian wrote:
> >> On 2024/6/11 22:39, Jan Beulich wrote:
> >>> On 07.06.2024 10:11, Jiqian Chen wrote:
> >>>> +    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
> >>>
> >>> Looking at the hypervisor side, this will fail for PV Dom0. In which case imo
> >>> you better would avoid making the call in the first place.
> >> Yes, for PV dom0, the errno is EOPNOTSUPP, then it will do below xc_domain_irq_permission.
> > 
> > Hence why call xc_domain_gsi_permission() at all on a PV Dom0?
> Is there a function to distinguish that current dom0 is PV or PVH dom0 in tools/libs?

That might have never been needed before, so probably not. There's
libxl__domain_type() but if that works with dom0 it might return "HVM"
for PVH dom0. So if xc_domain_getinfo_single() works and give the right
info about dom0, libxl__domain_type() could be extended to deal with
dom0 I guess. I don't know if there's a good way to find out which
flavor of dom0 is running.

Cheers,

-- 


Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Chen, Jiqian]

On 2024/6/13 20:51, Anthony PERARD wrote:
> On Wed, Jun 12, 2024 at 10:55:14AM +0000, Chen, Jiqian wrote:
>> On 2024/6/12 18:34, Jan Beulich wrote:
>>> On 12.06.2024 12:12, Chen, Jiqian wrote:
>>>> On 2024/6/11 22:39, Jan Beulich wrote:
>>>>> On 07.06.2024 10:11, Jiqian Chen wrote:
>>>>>> +    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
>>>>>
>>>>> Looking at the hypervisor side, this will fail for PV Dom0. In which case imo
>>>>> you better would avoid making the call in the first place.
>>>> Yes, for PV dom0, the errno is EOPNOTSUPP, then it will do below xc_domain_irq_permission.
>>>
>>> Hence why call xc_domain_gsi_permission() at all on a PV Dom0?
>> Is there a function to distinguish that current dom0 is PV or PVH dom0 in tools/libs?
> 
> That might have never been needed before, so probably not. There's
> libxl__domain_type() but if that works with dom0 it might return "HVM"
> for PVH dom0. So if xc_domain_getinfo_single() works and give the right
> info about dom0, libxl__domain_type() could be extended to deal with
> dom0 I guess. I don't know if there's a good way to find out which
> flavor of dom0 is running.
Thanks Anthony!
I think here we really need to check is that whether current domain has PIRQ flag(X86_EMU_USE_PIRQ) or not.
And it seems xc_domain_gsi_permission already return the information.
If current domain has no PIRQs, then I should use xc_domain_gsi_permission to grant permission, otherwise I should
keep the original function xc_domain_irq_permission.

> 
> Cheers,
> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Chen, Jiqian]

Hi Daniel,

On 2024/6/11 22:39, Jan Beulich wrote:
> On 07.06.2024 10:11, Jiqian Chen wrote: 
>> +    case XEN_DOMCTL_gsi_permission:
>> +    {
>> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
>> +        int irq = gsi_2_irq(gsi); 
>> +        bool allow = domctl->u.gsi_permission.allow_access; 
>> +        /*
>> +         * If current domain is PV or it has PIRQ flag, it has a mapping
>> +         * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission
>> +         * to grant irq permission.
>> +         */
>> +        if ( is_pv_domain(current->domain) || has_pirq(current->domain) ) 
>> +        {
>> +            ret = -EOPNOTSUPP;
>> +            break;
>> +        }
>> +
>> +        if ( gsi >= nr_irqs_gsi || irq < 0 )
>> +        {
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +
>> +        if ( !irq_access_permitted(current->domain, irq) ||
>> +             xsm_irq_permission(XSM_HOOK, d, irq, allow) )
> 
> Daniel, is it okay to issue the XSM check using the translated value, not
> the one that was originally passed into the hypercall?
Is it okay?

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Jan Beulich]

On 14.06.2024 05:11, Chen, Jiqian wrote:
> On 2024/6/13 20:51, Anthony PERARD wrote:
>> On Wed, Jun 12, 2024 at 10:55:14AM +0000, Chen, Jiqian wrote:
>>> On 2024/6/12 18:34, Jan Beulich wrote:
>>>> On 12.06.2024 12:12, Chen, Jiqian wrote:
>>>>> On 2024/6/11 22:39, Jan Beulich wrote:
>>>>>> On 07.06.2024 10:11, Jiqian Chen wrote:
>>>>>>> +    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
>>>>>>
>>>>>> Looking at the hypervisor side, this will fail for PV Dom0. In which case imo
>>>>>> you better would avoid making the call in the first place.
>>>>> Yes, for PV dom0, the errno is EOPNOTSUPP, then it will do below xc_domain_irq_permission.
>>>>
>>>> Hence why call xc_domain_gsi_permission() at all on a PV Dom0?
>>> Is there a function to distinguish that current dom0 is PV or PVH dom0 in tools/libs?
>>
>> That might have never been needed before, so probably not. There's
>> libxl__domain_type() but if that works with dom0 it might return "HVM"
>> for PVH dom0. So if xc_domain_getinfo_single() works and give the right
>> info about dom0, libxl__domain_type() could be extended to deal with
>> dom0 I guess. I don't know if there's a good way to find out which
>> flavor of dom0 is running.
> Thanks Anthony!
> I think here we really need to check is that whether current domain has PIRQ flag(X86_EMU_USE_PIRQ) or not.
> And it seems xc_domain_gsi_permission already return the information.

By way of failing, if I'm not mistaken? As indicated before, I don't
think you should invoke the function when it's clear it's going to fail.

Jan

> If current domain has no PIRQs, then I should use xc_domain_gsi_permission to grant permission, otherwise I should
> keep the original function xc_domain_irq_permission.
> 
>>
>> Cheers,
>>
> 

Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Chen, Jiqian]

On 2024/6/14 14:41, Jan Beulich wrote:
> On 14.06.2024 05:11, Chen, Jiqian wrote:
>> On 2024/6/13 20:51, Anthony PERARD wrote:
>>> On Wed, Jun 12, 2024 at 10:55:14AM +0000, Chen, Jiqian wrote:
>>>> On 2024/6/12 18:34, Jan Beulich wrote:
>>>>> On 12.06.2024 12:12, Chen, Jiqian wrote:
>>>>>> On 2024/6/11 22:39, Jan Beulich wrote:
>>>>>>> On 07.06.2024 10:11, Jiqian Chen wrote:
>>>>>>>> +    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
>>>>>>>
>>>>>>> Looking at the hypervisor side, this will fail for PV Dom0. In which case imo
>>>>>>> you better would avoid making the call in the first place.
>>>>>> Yes, for PV dom0, the errno is EOPNOTSUPP, then it will do below xc_domain_irq_permission.
>>>>>
>>>>> Hence why call xc_domain_gsi_permission() at all on a PV Dom0?
>>>> Is there a function to distinguish that current dom0 is PV or PVH dom0 in tools/libs?
>>>
>>> That might have never been needed before, so probably not. There's
>>> libxl__domain_type() but if that works with dom0 it might return "HVM"
>>> for PVH dom0. So if xc_domain_getinfo_single() works and give the right
>>> info about dom0, libxl__domain_type() could be extended to deal with
>>> dom0 I guess. I don't know if there's a good way to find out which
>>> flavor of dom0 is running.
>> Thanks Anthony!
>> I think here we really need to check is that whether current domain has PIRQ flag(X86_EMU_USE_PIRQ) or not.
>> And it seems xc_domain_gsi_permission already return the information.
> 
> By way of failing, if I'm not mistaken? As indicated before, I don't
> think you should invoke the function when it's clear it's going to fail.
Sorry, I wrote wrong here, it should be " And it seems xc_domain_getinfo_single already return the information."
And next version will be like:
xc_domaininfo_t xcinfo;
xc_domain_getinfo_single(xc_handle, domid, &xcinfo);
if( xcinfo.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ )
	xc_domain_irq_permission
else
	xc_domain_gsi_permission

> 
> Jan
> 
>> If current domain has no PIRQs, then I should use xc_domain_gsi_permission to grant permission, otherwise I should
>> keep the original function xc_domain_irq_permission.
>>
>>>
>>> Cheers,
>>>
>>
> 

-- 
Best regards,
Jiqian Chen.

Re: [XEN PATCH v9 3/5] x86/pvh: Add PHYSDEVOP_setup_gsi for PVH dom0

[Stefano Stabellini]

On Fri, 7 Jun 2024, Jiqian Chen wrote:
> On 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.
> On Linux kernel side, it calles PHYSDEVOP_setup_gsi for
> passthrough devices to register gsi when dom0 is PVH.
> So, add PHYSDEVOP_setup_gsi for above purpose.
> 
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>