From: Stewart Hildebrand <stewart.hildebrand@amd.com>

Enable the use of IOMMU + PCI in dom0 without having to specify
"pci-passthrough=yes". Due to possible platform specific dependencies
of the PCI host, we rely on dom0 to initialize it and perform
a PHYSDEVOP_pci_device_add call to add each device to SMMU.

Because pci_passthrough is not always enabled on all architectures, add
a new function arch_pci_device_physdevop that checks if we need to enable
a subset of the PCI subsystem related to managing IOMMU configuration
for PCI devices.

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

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
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.

v9->v10:
* move iommu_enabled check in a separate arch function
* add Stefano's RB

v8->v9:
* move iommu_enabled check inside is_pci_passthrough_enabled()

v7->v8:
* bring back x86 definition of is_pci_passthrough_enabled()

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/include/asm/pci.h | 2 ++
 xen/arch/arm/pci/pci.c         | 8 +++++++-
 xen/arch/x86/include/asm/pci.h | 5 +++++
 xen/drivers/pci/physdev.c      | 4 ++--
 xen/include/xen/pci.h          | 5 +++++
 5 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 7f77226c9b..d6e05f16b0 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -128,6 +128,8 @@ int pci_host_bridge_mappings(struct domain *d);
 
 bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
 
+bool arch_pci_device_physdevop(void);
+
 #else   /*!CONFIG_HAS_PCI*/
 
 struct pci_dev;
diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index 8d9692c92e..78e9ef28d5 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>
 
@@ -75,6 +76,11 @@ static int __init acpi_pci_init(void)
 }
 #endif
 
+bool arch_pci_device_physdevop(void)
+{
+    return iommu_enabled;
+}
+
 /* By default pci passthrough is disabled. */
 bool __read_mostly pci_passthrough_enabled;
 boolean_param("pci-passthrough", pci_passthrough_enabled);
@@ -85,7 +91,7 @@ static int __init pci_init(void)
      * Enable PCI passthrough when has been enabled explicitly
      * (pci-passthrough=on).
      */
-    if ( !pci_passthrough_enabled )
+    if ( !is_pci_passthrough_enabled() && !arch_pci_device_physdevop() )
         return 0;
 
     if ( pci_add_segment(0) )
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index fd5480d67d..1d59437147 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -67,4 +67,9 @@ static inline bool pci_check_bar(const struct pci_dev *pdev,
     return is_memory_hole(start, end);
 }
 
+static always_inline bool arch_pci_device_physdevop(void)
+{
+    return false;
+}
+
 #endif /* __X86_PCI_H__ */
diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
index 0161a85e1e..21c8a3a90e 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() && !arch_pci_device_physdevop())
             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() && !arch_pci_device_physdevop())
             return -EOPNOTSUPP;
 
         ret = -EFAULT;
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index ef60196653..130c2a8c1a 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -79,6 +79,11 @@ static inline bool is_pci_passthrough_enabled(void)
     return false;
 }
 
+static inline bool arch_pci_device_physdevop(void)
+{
+    return false;
+}
+
 #endif
 
 struct pci_dev_info {
-- 
2.34.1

On 29.04.2025 13:52, Mykyta Poturai wrote:
> @@ -75,6 +76,11 @@ static int __init acpi_pci_init(void)
>  }
>  #endif
>  
> +bool arch_pci_device_physdevop(void)
> +{
> +    return iommu_enabled;
> +}

I'm not an Arm maintainer, but if I was, I'd demand that a clarifying comment
was added here.

> --- a/xen/arch/x86/include/asm/pci.h
> +++ b/xen/arch/x86/include/asm/pci.h
> @@ -67,4 +67,9 @@ static inline bool pci_check_bar(const struct pci_dev *pdev,
>      return is_memory_hole(start, end);
>  }
>  
> +static always_inline bool arch_pci_device_physdevop(void)

I see not particular reason to use always_inline here; you don't do so ...

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -79,6 +79,11 @@ static inline bool is_pci_passthrough_enabled(void)
>      return false;
>  }
>  
> +static inline bool arch_pci_device_physdevop(void)

... here either.

I further notice that you didn't adjust the "reset" sub-op handling, despite
my earlier hint in that direction. Looking at the commit message, you only
mention PHYSDEVOP_pci_device_add anyway. I think all three need mentioning
there, which would then (hopefully) clarify what the deal is with
PHYSDEVOP_pci_device_reset.

Jan

On 29.04.25 15:20, Jan Beulich wrote:
> On 29.04.2025 13:52, Mykyta Poturai wrote: 
> I further notice that you didn't adjust the "reset" sub-op handling, despite
> my earlier hint in that direction. Looking at the commit message, you only
> mention PHYSDEVOP_pci_device_add anyway. I think all three need mentioning
> there, which would then (hopefully) clarify what the deal is with
> PHYSDEVOP_pci_device_reset.
> 
> Jan

PHYSDEVOP_pci_device_reset doesn't check if PCI passthrough is enabled 
or not, so there is no reason to add special clauses. I will add this to 
the commit message to be clear.

-- 
Mykyta

On 27.05.2025 10:14, Mykyta Poturai wrote:
> On 29.04.25 15:20, Jan Beulich wrote:
>> On 29.04.2025 13:52, Mykyta Poturai wrote: 
>> I further notice that you didn't adjust the "reset" sub-op handling, despite
>> my earlier hint in that direction. Looking at the commit message, you only
>> mention PHYSDEVOP_pci_device_add anyway. I think all three need mentioning
>> there, which would then (hopefully) clarify what the deal is with
>> PHYSDEVOP_pci_device_reset.
>>
>> Jan
> 
> PHYSDEVOP_pci_device_reset doesn't check if PCI passthrough is enabled 
> or not,

And you're convinced this is correct this way?

Jan

> so there is no reason to add special clauses. I will add this to 
> the commit message to be clear.
>