diff mbox series

[v1,4/6] pci/arm: Use iommu_add_dt_pci_device() instead of arch hook

Message ID 20230501200305.168058-5-stewart.hildebrand@amd.com (mailing list archive)
State Superseded
Headers show
Series SMMU handling for PCIe Passthrough on ARM | expand

Commit Message

Stewart Hildebrand May 1, 2023, 8:03 p.m. UTC 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

On Arm we need to parse DT PCI-IOMMU specifier and provide it to
the driver (for describing the relationship between PCI devices
and IOMMUs) before adding a device to it.

Also clarify the check of the return value as iommu_add_pci_device
can return >0 if a device doesn't need to be protected by the IOMMU
and print a warning if iommu_add_pci_device failed.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
downstream->v1:
* rebase
* add __maybe_unused attribute to const struct domain_iommu *hd;
* Rename: s/iommu_add_pci_device/iommu_add_dt_pci_device/
* guard iommu_add_dt_pci_device call with CONFIG_HAS_DEVICE_TREE instead of
  CONFIG_ARM

(cherry picked from commit 2b9d26badab8b24b5a80d028c4499a5022817213 from
 the downstream branch poc/pci-passthrough from
 https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---
 xen/drivers/passthrough/pci.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Jan Beulich May 2, 2023, 7:50 a.m. UTC | #1 
On 01.05.2023 22:03, Stewart Hildebrand wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1305,7 +1305,7 @@ __initcall(setup_dump_pcidevs);
>  
>  static int iommu_add_device(struct pci_dev *pdev)
>  {
> -    const struct domain_iommu *hd;
> +    const struct domain_iommu *hd __maybe_unused;
>      int rc;
>      unsigned int devfn = pdev->devfn;
>  
> @@ -1318,17 +1318,30 @@ static int iommu_add_device(struct pci_dev *pdev)
>      if ( !is_iommu_enabled(pdev->domain) )
>          return 0;
>  
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +    rc = iommu_add_dt_pci_device(devfn, pdev);
> +#else
>      rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
> -    if ( rc || !pdev->phantom_stride )
> +#endif
> +    if ( rc < 0 || !pdev->phantom_stride )
> +    {
> +        if ( rc < 0 )
> +            printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
> +                   &pdev->sbdf, rc);
>          return rc;
> +    }
>  
>      for ( ; ; )
>      {
>          devfn += pdev->phantom_stride;
>          if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
>              return 0;
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +        rc = iommu_add_dt_pci_device(devfn, pdev);
> +#else
>          rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
> -        if ( rc )
> +#endif
> +        if ( rc < 0 )
>              printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>                     &PCI_SBDF(pdev->seg, pdev->bus, devfn), rc);
>      }

Such #ifdef-ary may be okay at the call site(s), but replacing a per-
IOMMU hook with a system-wide DT function here looks wrong to me. The
!= 0 => < 0 changes also would want, after respective auditing,
clarifying that they're indeed no functional change to existing code.

Jan
Stewart Hildebrand May 8, 2023, 2:16 p.m. UTC | #2 
On 5/2/23 03:50, Jan Beulich wrote:
> On 01.05.2023 22:03, Stewart Hildebrand wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -1305,7 +1305,7 @@ __initcall(setup_dump_pcidevs);
>>
>>  static int iommu_add_device(struct pci_dev *pdev)
>>  {
>> -    const struct domain_iommu *hd;
>> +    const struct domain_iommu *hd __maybe_unused;
>>      int rc;
>>      unsigned int devfn = pdev->devfn;
>>
>> @@ -1318,17 +1318,30 @@ static int iommu_add_device(struct pci_dev *pdev)
>>      if ( !is_iommu_enabled(pdev->domain) )
>>          return 0;
>>
>> +#ifdef CONFIG_HAS_DEVICE_TREE
>> +    rc = iommu_add_dt_pci_device(devfn, pdev);
>> +#else
>>      rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>> -    if ( rc || !pdev->phantom_stride )
>> +#endif
>> +    if ( rc < 0 || !pdev->phantom_stride )
>> +    {
>> +        if ( rc < 0 )
>> +            printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>> +                   &pdev->sbdf, rc);
>>          return rc;
>> +    }
>>
>>      for ( ; ; )
>>      {
>>          devfn += pdev->phantom_stride;
>>          if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
>>              return 0;
>> +#ifdef CONFIG_HAS_DEVICE_TREE
>> +        rc = iommu_add_dt_pci_device(devfn, pdev);
>> +#else
>>          rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>> -        if ( rc )
>> +#endif
>> +        if ( rc < 0 )
>>              printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>>                     &PCI_SBDF(pdev->seg, pdev->bus, devfn), rc);
>>      }
> 
> Such #ifdef-ary may be okay at the call site(s), but replacing a per-
> IOMMU hook with a system-wide DT function here looks wrong to me.

Perhaps a better approach would be to rely on the existing iommu add_device call.

This might look something like:

#ifdef CONFIG_HAS_DEVICE_TREE
    rc = iommu_add_dt_pci_device(pdev);
    if ( !rc ) /* or rc >= 0, or something... */
#endif
        rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));

There would be no need to call iommu_add_dt_pci_device() in the loop iterating over phantom functions, as I will handle those inside iommu_add_dt_pci_device() in v2.

> The != 0 => < 0 changes also would want, after respective auditing,
> clarifying that they're indeed no functional change to existing code.

Okay.

Stewart
Jan Beulich May 8, 2023, 2:51 p.m. UTC | #3 
On 08.05.2023 16:16, Stewart Hildebrand wrote:
> On 5/2/23 03:50, Jan Beulich wrote:
>> On 01.05.2023 22:03, Stewart Hildebrand wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1305,7 +1305,7 @@ __initcall(setup_dump_pcidevs);
>>>
>>>  static int iommu_add_device(struct pci_dev *pdev)
>>>  {
>>> -    const struct domain_iommu *hd;
>>> +    const struct domain_iommu *hd __maybe_unused;
>>>      int rc;
>>>      unsigned int devfn = pdev->devfn;
>>>
>>> @@ -1318,17 +1318,30 @@ static int iommu_add_device(struct pci_dev *pdev)
>>>      if ( !is_iommu_enabled(pdev->domain) )
>>>          return 0;
>>>
>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>> +    rc = iommu_add_dt_pci_device(devfn, pdev);
>>> +#else
>>>      rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>> -    if ( rc || !pdev->phantom_stride )
>>> +#endif
>>> +    if ( rc < 0 || !pdev->phantom_stride )
>>> +    {
>>> +        if ( rc < 0 )
>>> +            printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>>> +                   &pdev->sbdf, rc);
>>>          return rc;
>>> +    }
>>>
>>>      for ( ; ; )
>>>      {
>>>          devfn += pdev->phantom_stride;
>>>          if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
>>>              return 0;
>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>> +        rc = iommu_add_dt_pci_device(devfn, pdev);
>>> +#else
>>>          rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>> -        if ( rc )
>>> +#endif
>>> +        if ( rc < 0 )
>>>              printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>>>                     &PCI_SBDF(pdev->seg, pdev->bus, devfn), rc);
>>>      }
>>
>> Such #ifdef-ary may be okay at the call site(s), but replacing a per-
>> IOMMU hook with a system-wide DT function here looks wrong to me.
> 
> Perhaps a better approach would be to rely on the existing iommu add_device call.
> 
> This might look something like:
> 
> #ifdef CONFIG_HAS_DEVICE_TREE
>     rc = iommu_add_dt_pci_device(pdev);
>     if ( !rc ) /* or rc >= 0, or something... */
> #endif
>         rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
> 
> There would be no need to call iommu_add_dt_pci_device() in the loop iterating over phantom functions, as I will handle those inside iommu_add_dt_pci_device() in v2.

But that still leaves #ifdef-ary inside the function. If for whatever reason
the hd->platform_ops hook isn't suitable (which I still don't understand),
then - as said - I'd view such #ifdef as possibly okay at the call site of
iommu_add_device(), but not inside.

Jan
Stewart Hildebrand May 11, 2023, 1:49 p.m. UTC | #4 
On 5/8/23 10:51, Jan Beulich wrote:
> On 08.05.2023 16:16, Stewart Hildebrand wrote:
>> On 5/2/23 03:50, Jan Beulich wrote:
>>> On 01.05.2023 22:03, Stewart Hildebrand wrote:
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -1305,7 +1305,7 @@ __initcall(setup_dump_pcidevs);
>>>>
>>>>  static int iommu_add_device(struct pci_dev *pdev)
>>>>  {
>>>> -    const struct domain_iommu *hd;
>>>> +    const struct domain_iommu *hd __maybe_unused;
>>>>      int rc;
>>>>      unsigned int devfn = pdev->devfn;
>>>>
>>>> @@ -1318,17 +1318,30 @@ static int iommu_add_device(struct pci_dev *pdev)
>>>>      if ( !is_iommu_enabled(pdev->domain) )
>>>>          return 0;
>>>>
>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>> +    rc = iommu_add_dt_pci_device(devfn, pdev);
>>>> +#else
>>>>      rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>>> -    if ( rc || !pdev->phantom_stride )
>>>> +#endif
>>>> +    if ( rc < 0 || !pdev->phantom_stride )
>>>> +    {
>>>> +        if ( rc < 0 )
>>>> +            printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>>>> +                   &pdev->sbdf, rc);
>>>>          return rc;
>>>> +    }
>>>>
>>>>      for ( ; ; )
>>>>      {
>>>>          devfn += pdev->phantom_stride;
>>>>          if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
>>>>              return 0;
>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>> +        rc = iommu_add_dt_pci_device(devfn, pdev);
>>>> +#else
>>>>          rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>>> -        if ( rc )
>>>> +#endif
>>>> +        if ( rc < 0 )
>>>>              printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>>>>                     &PCI_SBDF(pdev->seg, pdev->bus, devfn), rc);
>>>>      }
>>>
>>> Such #ifdef-ary may be okay at the call site(s), but replacing a per-
>>> IOMMU hook with a system-wide DT function here looks wrong to me.
>>
>> Perhaps a better approach would be to rely on the existing iommu add_device call.
>>
>> This might look something like:
>>
>> #ifdef CONFIG_HAS_DEVICE_TREE
>>     rc = iommu_add_dt_pci_device(pdev);
>>     if ( !rc ) /* or rc >= 0, or something... */
>> #endif
>>         rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>
>> There would be no need to call iommu_add_dt_pci_device() in the loop iterating over phantom functions, as I will handle those inside iommu_add_dt_pci_device() in v2.
> 
> But that still leaves #ifdef-ary inside the function. If for whatever reason
> the hd->platform_ops hook isn't suitable (which I still don't understand),

There's nothing wrong with the existing hd->platform_ops hook. We just need to ensure we've translated RID to AXI stream ID sometime before it.

> then - as said - I'd view such #ifdef as possibly okay at the call site of
> iommu_add_device(), but not inside.

I'll move the #ifdef-ary.
Julien Grall May 11, 2023, 1:59 p.m. UTC | #5 
Hi,

Can you make sure to CC the Arm/SMMU maintainers as I feel it is 
important for them to also review the generic changes.

On 11/05/2023 14:49, Stewart Hildebrand wrote:
> On 5/8/23 10:51, Jan Beulich wrote:
>> On 08.05.2023 16:16, Stewart Hildebrand wrote:
>>> On 5/2/23 03:50, Jan Beulich wrote:
>>>> On 01.05.2023 22:03, Stewart Hildebrand wrote:
>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>> @@ -1305,7 +1305,7 @@ __initcall(setup_dump_pcidevs);
>>>>>
>>>>>   static int iommu_add_device(struct pci_dev *pdev)
>>>>>   {
>>>>> -    const struct domain_iommu *hd;
>>>>> +    const struct domain_iommu *hd __maybe_unused;
>>>>>       int rc;
>>>>>       unsigned int devfn = pdev->devfn;
>>>>>
>>>>> @@ -1318,17 +1318,30 @@ static int iommu_add_device(struct pci_dev *pdev)
>>>>>       if ( !is_iommu_enabled(pdev->domain) )
>>>>>           return 0;
>>>>>
>>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>>> +    rc = iommu_add_dt_pci_device(devfn, pdev);
>>>>> +#else
>>>>>       rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>>>> -    if ( rc || !pdev->phantom_stride )
>>>>> +#endif
>>>>> +    if ( rc < 0 || !pdev->phantom_stride )
>>>>> +    {
>>>>> +        if ( rc < 0 )
>>>>> +            printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>>>>> +                   &pdev->sbdf, rc);
>>>>>           return rc;
>>>>> +    }
>>>>>
>>>>>       for ( ; ; )
>>>>>       {
>>>>>           devfn += pdev->phantom_stride;
>>>>>           if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
>>>>>               return 0;
>>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>>> +        rc = iommu_add_dt_pci_device(devfn, pdev);
>>>>> +#else
>>>>>           rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>>>> -        if ( rc )
>>>>> +#endif
>>>>> +        if ( rc < 0 )
>>>>>               printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>>>>>                      &PCI_SBDF(pdev->seg, pdev->bus, devfn), rc);
>>>>>       }
>>>>
>>>> Such #ifdef-ary may be okay at the call site(s), but replacing a per-
>>>> IOMMU hook with a system-wide DT function here looks wrong to me.
>>>
>>> Perhaps a better approach would be to rely on the existing iommu add_device call.
>>>
>>> This might look something like:
>>>
>>> #ifdef CONFIG_HAS_DEVICE_TREE
>>>      rc = iommu_add_dt_pci_device(pdev);
>>>      if ( !rc ) /* or rc >= 0, or something... */
>>> #endif
>>>          rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>>
>>> There would be no need to call iommu_add_dt_pci_device() in the loop iterating over phantom functions, as I will handle those inside iommu_add_dt_pci_device() in v2.
>>
>> But that still leaves #ifdef-ary inside the function. If for whatever reason
>> the hd->platform_ops hook isn't suitable (which I still don't understand),
> 
> There's nothing wrong with the existing hd->platform_ops hook. We just need to ensure we've translated RID to AXI stream ID sometime before it.
> 
>> then - as said - I'd view such #ifdef as possibly okay at the call site of
>> iommu_add_device(), but not inside.
> 
> I'll move the #ifdef-ary.

I am not sure what #ifdef-ary you will have. However, at some point, we 
will also want to support ACPI on Arm. Both DT and ACPI co-exist and 
this would not work properly with the code you wrote.

If we need some DT specific call, then I think the call should happen 
with hd->platform_ops rather than in the generic infrastructure.

Cheers,
Jan Beulich May 11, 2023, 2:19 p.m. UTC | #6 
On 11.05.2023 15:59, Julien Grall wrote:
> On 11/05/2023 14:49, Stewart Hildebrand wrote:
>> On 5/8/23 10:51, Jan Beulich wrote:
>>> On 08.05.2023 16:16, Stewart Hildebrand wrote:
>>>> On 5/2/23 03:50, Jan Beulich wrote:
>>>>> On 01.05.2023 22:03, Stewart Hildebrand wrote:
>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>> @@ -1305,7 +1305,7 @@ __initcall(setup_dump_pcidevs);
>>>>>>
>>>>>>   static int iommu_add_device(struct pci_dev *pdev)
>>>>>>   {
>>>>>> -    const struct domain_iommu *hd;
>>>>>> +    const struct domain_iommu *hd __maybe_unused;
>>>>>>       int rc;
>>>>>>       unsigned int devfn = pdev->devfn;
>>>>>>
>>>>>> @@ -1318,17 +1318,30 @@ static int iommu_add_device(struct pci_dev *pdev)
>>>>>>       if ( !is_iommu_enabled(pdev->domain) )
>>>>>>           return 0;
>>>>>>
>>>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>>>> +    rc = iommu_add_dt_pci_device(devfn, pdev);
>>>>>> +#else
>>>>>>       rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>>>>> -    if ( rc || !pdev->phantom_stride )
>>>>>> +#endif
>>>>>> +    if ( rc < 0 || !pdev->phantom_stride )
>>>>>> +    {
>>>>>> +        if ( rc < 0 )
>>>>>> +            printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>>>>>> +                   &pdev->sbdf, rc);
>>>>>>           return rc;
>>>>>> +    }
>>>>>>
>>>>>>       for ( ; ; )
>>>>>>       {
>>>>>>           devfn += pdev->phantom_stride;
>>>>>>           if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
>>>>>>               return 0;
>>>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>>>> +        rc = iommu_add_dt_pci_device(devfn, pdev);
>>>>>> +#else
>>>>>>           rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>>>>> -        if ( rc )
>>>>>> +#endif
>>>>>> +        if ( rc < 0 )
>>>>>>               printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>>>>>>                      &PCI_SBDF(pdev->seg, pdev->bus, devfn), rc);
>>>>>>       }
>>>>>
>>>>> Such #ifdef-ary may be okay at the call site(s), but replacing a per-
>>>>> IOMMU hook with a system-wide DT function here looks wrong to me.
>>>>
>>>> Perhaps a better approach would be to rely on the existing iommu add_device call.
>>>>
>>>> This might look something like:
>>>>
>>>> #ifdef CONFIG_HAS_DEVICE_TREE
>>>>      rc = iommu_add_dt_pci_device(pdev);
>>>>      if ( !rc ) /* or rc >= 0, or something... */
>>>> #endif
>>>>          rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
>>>>
>>>> There would be no need to call iommu_add_dt_pci_device() in the loop iterating over phantom functions, as I will handle those inside iommu_add_dt_pci_device() in v2.
>>>
>>> But that still leaves #ifdef-ary inside the function. If for whatever reason
>>> the hd->platform_ops hook isn't suitable (which I still don't understand),
>>
>> There's nothing wrong with the existing hd->platform_ops hook. We just need to ensure we've translated RID to AXI stream ID sometime before it.
>>
>>> then - as said - I'd view such #ifdef as possibly okay at the call site of
>>> iommu_add_device(), but not inside.
>>
>> I'll move the #ifdef-ary.
> 
> I am not sure what #ifdef-ary you will have. However, at some point, we 
> will also want to support ACPI on Arm. Both DT and ACPI co-exist and 
> this would not work properly with the code you wrote.
> 
> If we need some DT specific call, then I think the call should happen 
> with hd->platform_ops rather than in the generic infrastructure.

I'm not sure about this, to be honest. platform_ops is about the particular
underlying IOMMU. I would expect that the kind of IOMMU in use has nothing
to do with where the configuration information comes from? Instead I would
view the proposed #ifdef (a layer up) as an intermediate step towards a
further level of indirection there. Then again I will admit I don't really
understand why special-casing DT here is necessary in the first place.
There's nothing ACPI-ish in this code, even if the IOMMU-specific
information comes from ACPI on x86. I therefore wonder whether the approach
chosen perhaps isn't the right one (which might mean going through
platform_op as we already do is the correct thing, and telling the DT case
from the [future] ACPI one ought to happen further down the call chain) ...

Jan
diff mbox series

Patch

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index b42acb8d7c09..ed5a6ede7847 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1305,7 +1305,7 @@  __initcall(setup_dump_pcidevs);
 
 static int iommu_add_device(struct pci_dev *pdev)
 {
-    const struct domain_iommu *hd;
+    const struct domain_iommu *hd __maybe_unused;
     int rc;
     unsigned int devfn = pdev->devfn;
 
@@ -1318,17 +1318,30 @@  static int iommu_add_device(struct pci_dev *pdev)
     if ( !is_iommu_enabled(pdev->domain) )
         return 0;
 
+#ifdef CONFIG_HAS_DEVICE_TREE
+    rc = iommu_add_dt_pci_device(devfn, pdev);
+#else
     rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
-    if ( rc || !pdev->phantom_stride )
+#endif
+    if ( rc < 0 || !pdev->phantom_stride )
+    {
+        if ( rc < 0 )
+            printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
+                   &pdev->sbdf, rc);
         return rc;
+    }
 
     for ( ; ; )
     {
         devfn += pdev->phantom_stride;
         if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
             return 0;
+#ifdef CONFIG_HAS_DEVICE_TREE
+        rc = iommu_add_dt_pci_device(devfn, pdev);
+#else
         rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
-        if ( rc )
+#endif
+        if ( rc < 0 )
             printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
                    &PCI_SBDF(pdev->seg, pdev->bus, devfn), rc);
     }