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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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.