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

[Julien Grall <julien@xen.org>]

Hi Jan,

On 03/08/2021 07:57, Jan Beulich wrote:
> 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".

I meant in the IOMMU firmware table, sorry. IOW, when a new IOMMU is 
added we walk its configuration to figure out which device is attached 
to it.

>  Otoh to "discover a new device" means the device wasn't (supposed
> to be) known before, which to me means -EEXIST is appropriate.

Right. The problem is after patch #1 all callers would need to cope with 
-EEXIST because the legacy binding register the device up front.

That's why I think returning 0 here is better.

Cheers,

-- 
Julien Grall