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