Re: [PATCH v2 5/8] pci/arm: Use iommu_add_dt_pci_device()

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/15/23 03:30, Jan Beulich wrote:
> On 12.05.2023 23:03, Stewart Hildebrand wrote:
>> On 5/12/23 03:25, Jan Beulich wrote:
>>> On 11.05.2023 21:16, Stewart Hildebrand wrote:
>>>> @@ -762,9 +767,20 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>>              pdev->domain = NULL;
>>>>              goto out;
>>>>          }
>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>> +        ret = iommu_add_dt_pci_device(pdev);
>>>> +        if ( ret < 0 )
>>>> +        {
>>>> +            printk(XENLOG_ERR "pci-iommu translation failed: %d\n", ret);
>>>> +            goto out;
>>>> +        }
>>>> +#endif
>>>>          ret = iommu_add_device(pdev);
>>>
>>> Hmm, am I misremembering that in the earlier patch you had #else to
>>> invoke the alternative behavior?
>>
>> You are remembering correctly. v1 had an #else, v2 does not.
>>
>>> Now you end up calling both functions;
>>> if that's indeed intended,
>>
>> Yes, this is intentional.
>>
>>> this may still want doing differently.
>>> Looking at the earlier patch introducing the function, I can't infer
>>> though whether that's intended: iommu_add_dt_pci_device() checks that
>>> the add_device hook is present, but then I didn't find any use of this
>>> hook. The revlog there suggests the check might be stale.
>>
>> Ah, right, the ops->add_device check is stale in the other patch. Good 
>> catch, I'll remove it there.
>>
>>> If indeed the function does only preparatory work, I don't see why it
>>> would need naming "iommu_..."; I'd rather consider pci_add_dt_device()
>>> then.
>>
>> The function has now been reduced to reading SMMU configuration data from DT 
>> and mapping RID/BDF -> AXI stream ID. However, it is still SMMU related, and 
>> it is still invoking another iommu_ops hook function, dt_xlate (which is yet 
>> another AXI stream ID translation, separate from what is being discussed 
>> here). Does this justify keeping "iommu_..." in the name? I'm not convinced 
>> pci_add_dt_device() is a good name for it either (more on this below).
> 
> The function being SMMU-related pretty strongly suggests it wants to be
> invoked via a hook. If the add_device() one isn't suitable, perhaps we
> need a new (optional) prepare_device() one? With pci_add_device() then
> calling iommu_prepare_device(), wrapping the hook invocation?
> 
> But just to be clear: A new hook would need enough justification as to
> the existing one being unsuitable.

I'll move it to the add_device hook in v3