Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.07.2021 17:45, Julien Grall wrote:
> On 23/07/2021 14:02, Jan Beulich wrote:
>> On 23.07.2021 11:28, Julien Grall wrote:
>>> On 23/07/2021 07:31, Jan Beulich wrote:
>>>> On 23.07.2021 01:36, Stefano Stabellini wrote:
>>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>>>        if ( !ops )
>>>>>            return -EINVAL;
>>>>>    
>>>>> +    /*
>>>>> +     * Some Device Trees may expose both legacy SMMU and generic
>>>>> +     * IOMMU bindings together. If both are present, the device
>>>>> +     * can be already added.
>>>>> +     */
>>>>>        if ( dev_iommu_fwspec_get(dev) )
>>>>> -        return -EEXIST;
>>>>> +        return 0;
>>>>
>>>> Since the xen: prefix in the subject made me go look (I wouldn't have
>>>> if it had been e.g. dt: ), I may as well ask: Since previously there
>>>> was concern about bogus duplicate entries, does this concern go away
>>>> no altogether?
>>>
>>> The check wasn't originally added because of legacy vs generic binding.
>>>
>>> It was added because in some circumstances iommu_add_dt_device() could
>>> genuinely be called twice (for instance if the device is re-assigned).
>>> This was returning -EEXIST rather than 0 so the caller can decide
>>> whether it is normal that the device is already added.
>>
>> Okay. If that distinction is of no interest anymore, then I can see
>> this wanting dropping.
>>
>>> Calling iommu_add_dt_device() twice doesn't hurt but after patch #1
>>> (this patch should really be first), dev_iommu_fwspec_get() will return
>>> a non-NULL pointer as the legacy devices are added when the IOMMU is probed.
>>>
>>>> It's one thing for there to be a legacy and a generic
>>>> binding, but another if you found two legacy or two generic ones, I
>>>> would think.
>>>
>>> I am not quite too sure what you mean by "two legacy" and "two generic".
>>> Can you clarify it?
>>
>> Well, I'm having trouble describing it in different terms. I mean
>> two entries of the same kind (both legacy or both generic) referring
>> to the same device, thus leading to the function recognizing the 2nd > time 
>> round that the device is already there.
> 
> I think you are misunderstanding the purpose of this function. It is 
> called when we discover a new device rather than discovering a new entry 
> in the IOMMU. The function will then sort out what to do for the device.

I'm struggling with assigning meaning to "discovering a new entry in the
IOMMU". Otoh to "discover a new device" means the device wasn't (supposed
to be) known before, which to me means -EEXIST is appropriate.

> The legacy binding is somewhat specific because it bypass the function 
> as the discovering is done per IOMMU rather than per device.

Well, I then guess I'm lacking too much context here.

Jan