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