[v7,7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no

Commit Message

Mykyta Poturai Feb. 4, 2025, 1:54 p.m. UTC

From: Stewart Hildebrand <[e-mail]>

Enable the use of IOMMU + PCI in dom0 without having to specify
"pci-passthrough=yes". We rely on dom0 to initialize the PCI controller
and perform a PHYSDEVOP_pci_device_add call to add each device to SMMU.

Enable pci_init() for initializing Xen's internal PCI subsystem, and
allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled.

is_pci_passthrough_enabled() is not an Arm-only construct, so remove the
x86 definition of the function.

Signed-off-by: Stewart Hildebrand <[e-mail]>
Signed-off-by: Mykyta Poturai <[e-mail]>
---
hmm. Since
  dec9e02f3190 ("xen: avoid generation of stub <asm/pci.h> header")
Should we also move is_pci_passthrough_enabled() back to xen/arch/arm/include/asm/pci.h ?
Not sure if PPC/RISC-V will plan on using this check.

v6->v7:
* remove x86 definition of is_pci_passthrough_enabled()
* update comments
* make pci_physdev_op checks stricter

v5->v6:
* new patch - this effectively replaces
  ("Revert "xen/arm: Add cmdline boot option "pci-passthrough = <boolean>""")
---
 xen/arch/arm/pci/pci.c         | 5 +++--
 xen/arch/x86/include/asm/pci.h | 6 ------
 xen/drivers/pci/physdev.c      | 4 ++--
 3 files changed, 5 insertions(+), 10 deletions(-)

Comments

Jan Beulich Feb. 4, 2025, 2:31 p.m. UTC | #1

On 04.02.2025 14:54, Mykyta Poturai wrote:
> From: Stewart Hildebrand <[e-mail]>
> 
> Enable the use of IOMMU + PCI in dom0 without having to specify
> "pci-passthrough=yes". We rely on dom0 to initialize the PCI controller
> and perform a PHYSDEVOP_pci_device_add call to add each device to SMMU.
> 
> Enable pci_init() for initializing Xen's internal PCI subsystem, and
> allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled.
> 
> is_pci_passthrough_enabled() is not an Arm-only construct, so remove the
> x86 definition of the function.

I can't see how x86 will continue to build correctly then. There's nowhere
else you introduce a replacement.

> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -19,7 +19,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          struct pci_dev_info pdev_info;
>          nodeid_t node = NUMA_NO_NODE;
>  
> -        if ( !is_pci_passthrough_enabled() )
> +        if ( !is_pci_passthrough_enabled() && !iommu_enabled )
>              return -EOPNOTSUPP;
>  
>          ret = -EFAULT;
> @@ -57,7 +57,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      case PHYSDEVOP_pci_device_remove: {
>          struct physdev_pci_device dev;
>  
> -        if ( !is_pci_passthrough_enabled() )
> +        if ( !is_pci_passthrough_enabled() && !iommu_enabled )
>              return -EOPNOTSUPP;
>  
>          ret = -EFAULT;

This is (potentially) a functional change each on x86, which I don't
think would be correct. "Potentially" because without seeing what the
new is_pci_passthrough_enabled() does this is impossible to determine.

Jan

Patch

diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index 78b97beaef..f2281e81aa 100644
--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -16,6 +16,7 @@ 
 #include <xen/device_tree.h>
 #include <xen/errno.h>
 #include <xen/init.h>
+#include <xen/iommu.h>
 #include <xen/param.h>
 #include <xen/pci.h>
 
@@ -83,9 +84,9 @@  static int __init pci_init(void)
 {
     /*
      * Enable PCI passthrough when has been enabled explicitly
-     * (pci-passthrough=on).
+     * (pci-passthrough=on) or IOMMU is present and enabled.
      */
-    if ( !pci_passthrough_enabled )
+    if ( !is_pci_passthrough_enabled() && !iommu_enabled )
         return 0;
 
     pci_segments_init();
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index fd5480d67d..720fcc7f09 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -49,12 +49,6 @@  bool pci_ro_mmcfg_decode(unsigned long mfn, unsigned int *seg,
 extern int pci_mmcfg_config_num;
 extern struct acpi_mcfg_allocation *pci_mmcfg_config;
 
-/* Unlike ARM, PCI passthrough is always enabled for x86. */
-static always_inline bool is_pci_passthrough_enabled(void)
-{
-    return true;
-}
-
 void arch_pci_init_pdev(struct pci_dev *pdev);
 
 static inline bool pci_check_bar(const struct pci_dev *pdev,
diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
index 0161a85e1e..d8a49cadf3 100644
--- a/xen/drivers/pci/physdev.c
+++ b/xen/drivers/pci/physdev.c
@@ -19,7 +19,7 @@  ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct pci_dev_info pdev_info;
         nodeid_t node = NUMA_NO_NODE;
 
-        if ( !is_pci_passthrough_enabled() )
+        if ( !is_pci_passthrough_enabled() && !iommu_enabled )
             return -EOPNOTSUPP;
 
         ret = -EFAULT;
@@ -57,7 +57,7 @@  ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case PHYSDEVOP_pci_device_remove: {
         struct physdev_pci_device dev;
 
-        if ( !is_pci_passthrough_enabled() )
+        if ( !is_pci_passthrough_enabled() && !iommu_enabled )
             return -EOPNOTSUPP;
 
         ret = -EFAULT;