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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.05.2023 15:59, Julien Grall wrote:
> 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.

I'm not sure about this, to be honest. platform_ops is about the particular
underlying IOMMU. I would expect that the kind of IOMMU in use has nothing
to do with where the configuration information comes from? Instead I would
view the proposed #ifdef (a layer up) as an intermediate step towards a
further level of indirection there. Then again I will admit I don't really
understand why special-casing DT here is necessary in the first place.
There's nothing ACPI-ish in this code, even if the IOMMU-specific
information comes from ACPI on x86. I therefore wonder whether the approach
chosen perhaps isn't the right one (which might mean going through
platform_op as we already do is the correct thing, and telling the DT case
from the [future] ACPI one ought to happen further down the call chain) ...

Jan