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

[Jan Beulich <jbeulich@xxxxxxxx>]

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