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
