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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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