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
