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/remove call to add each device to SMMU.
PHYSDEVOP_pci_device_reset is left untouched as it does not have the
pci_passthrough_enabled check.
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.
v10->v11:
* always_inline -> inline
* add comments
* clarify reset sub-op handling in the commit message
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 | 14 +++++++++++++-
xen/arch/x86/include/asm/pci.h | 6 ++++++
xen/drivers/pci/physdev.c | 4 ++--
xen/include/xen/pci.h | 5 +++++
5 files changed, 28 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..ca825ee3a6 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,17 @@ static int __init acpi_pci_init(void)
}
#endif
+/*
+ * Platform-specific PCI host dependencies require dom0 to handle
+ * initialization and issue PHYSDEVOP_pci_device_add/remove calls for SMMU
+ * device registration. This check is used to enable the minimal PCI
+ * subsystem required for dom0 operation when PCI passthrough is disabled.
+ */
+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 +97,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..61d8043fa3 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -67,4 +67,10 @@ static inline bool pci_check_bar(const struct pci_dev *pdev,
return is_memory_hole(start, end);
}
+/* PCI passthrough is always enabled on x86 so no special handling is needed */
+static 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.1On 28.05.2025 11:12, Mykyta Poturai wrote:
> 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/remove call to add each device to SMMU.
> PHYSDEVOP_pci_device_reset is left untouched as it does not have the
> pci_passthrough_enabled check.
Just to re-raise the question here: Is this actually correct?
> --- 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;
Nit (style): You're losing a relevant blank each.
Jan
On 02.06.25 11:11, Jan Beulich wrote:
> On 28.05.2025 11:12, Mykyta Poturai wrote:
>> 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/remove call to add each device to SMMU.
>> PHYSDEVOP_pci_device_reset is left untouched as it does not have the
>> pci_passthrough_enabled check.
>
> Just to re-raise the question here: Is this actually correct?
>
I'm afraid I don't quite understand your concerns here.
The purpose of this patch is to relax the pci_passthrough_enabled checks
and make PCI physdev ops work with passthrough disabled.
The reset op worked independently of PCI passthrough being on or off and
will continue to do so after this patch.
If your concerns are about the correctness of allowing reset to always
work, you specifically requested this behavior in the patches
implementing it here[1].
>> --- 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;
>
> Nit (style): You're losing a relevant blank each.
I will update this, thanks.
> Jan
[1]:
https://patchew.org/Xen/20240816110820.75672-1-Jiqian.Chen@amd.com/20240816110820.75672-2-Jiqian.Chen@amd.com/#1e0eee6c-0dcd-4ed4-970f-3d7e569cec09@suse.com
--
Mykyta
On 03.06.2025 15:31, Mykyta Poturai wrote: > On 02.06.25 11:11, Jan Beulich wrote: >> On 28.05.2025 11:12, Mykyta Poturai wrote: >>> 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/remove call to add each device to SMMU. >>> PHYSDEVOP_pci_device_reset is left untouched as it does not have the >>> pci_passthrough_enabled check. >> >> Just to re-raise the question here: Is this actually correct? > > I'm afraid I don't quite understand your concerns here. > > The purpose of this patch is to relax the pci_passthrough_enabled checks > and make PCI physdev ops work with passthrough disabled. > The reset op worked independently of PCI passthrough being on or off and > will continue to do so after this patch. > If your concerns are about the correctness of allowing reset to always > work, you specifically requested this behavior in the patches > implementing it here[1]. Right, yet even there I had already asked for possible differing opinions. Plus the case I had mentioned was specifically Dom0, which fits here. Jan
On 04.06.25 08:52, Jan Beulich wrote: > On 03.06.2025 15:31, Mykyta Poturai wrote: >> On 02.06.25 11:11, Jan Beulich wrote: >>> On 28.05.2025 11:12, Mykyta Poturai wrote: >>>> 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/remove call to add each device to SMMU. >>>> PHYSDEVOP_pci_device_reset is left untouched as it does not have the >>>> pci_passthrough_enabled check. >>> >>> Just to re-raise the question here: Is this actually correct? >> >> I'm afraid I don't quite understand your concerns here. >> >> The purpose of this patch is to relax the pci_passthrough_enabled checks >> and make PCI physdev ops work with passthrough disabled. >> The reset op worked independently of PCI passthrough being on or off and >> will continue to do so after this patch. >> If your concerns are about the correctness of allowing reset to always >> work, you specifically requested this behavior in the patches >> implementing it here[1]. > > Right, yet even there I had already asked for possible differing opinions. > Plus the case I had mentioned was specifically Dom0, which fits here. > > Jan So I've done some testing to see the actual behavior with different combinations of pci-passthrough and iommu switches. With passthrough=off and iommu=on the reset works fine. But with both of them off, it fails because PHYSDEVOP_pci_device_add is not adding anything and therefore pci_get_pdev can't find the pdev. I am not sure which behavior would be the correct one here for passthrought=off and iommu=off. 1. Leave it as is, reset returns -ENODEV and pciback probe fails 2. Add the same check as in add/remove, reset will return -EOPNOTSUPP and pciback probe will also fail 3. Add the same check as in add/remove but return 0 so pciback can probe the device. Maybe you have some thoughts on this. I can't come up with an actual good reason for using pciback without pci-passthrough enabled, outside of maybe "not breaking some abstract scripts". And EOPNOTSUPP seems more descriptive than ENODEV so I strive towards option 2 if everyone okay with that. -- Mykyta
On 01.07.2025 10:29, Mykyta Poturai wrote: > On 04.06.25 08:52, Jan Beulich wrote: >> On 03.06.2025 15:31, Mykyta Poturai wrote: >>> On 02.06.25 11:11, Jan Beulich wrote: >>>> On 28.05.2025 11:12, Mykyta Poturai wrote: >>>>> 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/remove call to add each device to SMMU. >>>>> PHYSDEVOP_pci_device_reset is left untouched as it does not have the >>>>> pci_passthrough_enabled check. >>>> >>>> Just to re-raise the question here: Is this actually correct? >>> >>> I'm afraid I don't quite understand your concerns here. >>> >>> The purpose of this patch is to relax the pci_passthrough_enabled checks >>> and make PCI physdev ops work with passthrough disabled. >>> The reset op worked independently of PCI passthrough being on or off and >>> will continue to do so after this patch. >>> If your concerns are about the correctness of allowing reset to always >>> work, you specifically requested this behavior in the patches >>> implementing it here[1]. >> >> Right, yet even there I had already asked for possible differing opinions. >> Plus the case I had mentioned was specifically Dom0, which fits here. > > So I've done some testing to see the actual behavior with different > combinations of pci-passthrough and iommu switches. With passthrough=off > and iommu=on the reset works fine. But with both of them off, it fails > because PHYSDEVOP_pci_device_add is not adding anything and therefore > pci_get_pdev can't find the pdev. > > I am not sure which behavior would be the correct one here for > passthrought=off and iommu=off. > > 1. Leave it as is, reset returns -ENODEV and pciback probe fails > 2. Add the same check as in add/remove, reset will return -EOPNOTSUPP > and pciback probe will also fail > 3. Add the same check as in add/remove but return 0 so pciback can probe > the device. > > Maybe you have some thoughts on this. I can't come up with an actual > good reason for using pciback without pci-passthrough enabled, outside > of maybe "not breaking some abstract scripts". And EOPNOTSUPP seems more > descriptive than ENODEV so I strive towards option 2 if everyone okay > with that. I think I'd favor option 2, too. Without pass-through, PHYSDEVOP_pci_device_reset is pretty meaningless aiui. vPCI in particular builds on top of pass-through aiui, even if that isn't expressed like that right now (e.g. by having HAS_VPCI select HAS_PASSTHROUGH). Jan
© 2016 - 2025 Red Hat, Inc.