diff mbox series

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

Message ID 9950eff2f8344c5d658def7d2c6d7fc010607498.1739182214.git.mykyta_poturai@epam.com (mailing list archive)
State New
Headers show
Series SMMU handling for PCIe Passthrough on ARM | expand

Commit Message

Mykyta Poturai Feb. 10, 2025, 10:30 a.m. UTC 
From: Stewart Hildebrand <stewart.hildebrand@amd.com>

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.

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

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/pci/pci.c    | 5 +++--
 xen/drivers/pci/physdev.c | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Jan Beulich Feb. 10, 2025, 10:56 a.m. UTC | #1 
On 10.02.2025 11:30, 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".

Why? It _is_ passing through, even if Dom0 is special.

> @@ -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;

I can't reasonably judge on this adjustment, but ...

> --- 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;

... these two certainly look wrong to be made. If an Arm-specific adjustment
is needed (and can be justified), a per-arch hook may need introducing.

Jan
Mykyta Poturai Feb. 10, 2025, 11:28 a.m. UTC | #2 
On 10.02.25 12:56, Jan Beulich wrote:
> On 10.02.2025 11:30, 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".
> 
> Why? It _is_ passing through, even if Dom0 is special.

Do you think it would be better to drop this completely and require
pci-passthrough=yes for PCI to work in Dom0?

> 
>> @@ -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;
> 
> I can't reasonably judge on this adjustment, but ...
> 
>> --- 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;
> 
> ... these two certainly look wrong to be made. If an Arm-specific adjustment
> is needed (and can be justified), a per-arch hook may need introducing.

This should not affect x86 in any way if I'm not missing something, as
!is_pci_passthrough_enabled() will always be false. Or are you concerned 
about something else?

> 
> Jan
Jan Beulich Feb. 10, 2025, 11:44 a.m. UTC | #3 
On 10.02.2025 12:28, Mykyta Poturai wrote:
> On 10.02.25 12:56, Jan Beulich wrote:
>> On 10.02.2025 11:30, 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".
>>
>> Why? It _is_ passing through, even if Dom0 is special.
> 
> Do you think it would be better to drop this completely and require
> pci-passthrough=yes for PCI to work in Dom0?

From an abstract perspective: Yes. I don't know any of the Arm background,
though.

>>> @@ -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;
>>
>> I can't reasonably judge on this adjustment, but ...
>>
>>> --- 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;
>>
>> ... these two certainly look wrong to be made. If an Arm-specific adjustment
>> is needed (and can be justified), a per-arch hook may need introducing.
> 
> This should not affect x86 in any way if I'm not missing something, as
> !is_pci_passthrough_enabled() will always be false. Or are you concerned 
> about something else?

Indeed I am - the wrong look of it. Readers like me will have the immediate
impression of there being something fishy here.

Jan
diff mbox series

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/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;