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.
Jan
