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
