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

[Jan Beulich <jbeulich@xxxxxxxx>]

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