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

[Julien Grall <julien@xxxxxxx>]

Hi,

Can you make sure to CC the Arm/SMMU maintainers as I feel it is 
important for them to also review the generic changes.
On 11/05/2023 14:49, Stewart Hildebrand wrote:
> On 5/8/23 10:51, Jan Beulich wrote:
> > 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),
> There's nothing wrong with the existing hd->platform_ops hook. We just need to 
> ensure we've translated RID to AXI stream ID sometime before it.
>
> > then - as said - I'd view such #ifdef as possibly okay at the call site of
> > iommu_add_device(), but not inside.
> I'll move the #ifdef-ary.
I am not sure what #ifdef-ary you will have. However, at some point, we 
will also want to support ACPI on Arm. Both DT and ACPI co-exist and 
this would not work properly with the code you wrote.
If we need some DT specific call, then I think the call should happen 
with hd->platform_ops rather than in the generic infrastructure.
Cheers,

--
Julien Grall