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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. The
!= 0 => < 0 changes also would want, after respective auditing,
clarifying that they're indeed no functional change to existing code.

Jan