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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

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.
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?
> And what if legacy and generic representation differ in some way?
That would be a firmware table issue. It is not Xen business to check 
whether the two representation agree.
> Shouldn't you limit processing to just one of the two categories,
> such that no legitimate "already present" case could be encountered
> here in the first place?
There are legitimate "already present" case. This can happen when a 
device is re-assigned. Arguably the caller could check if the device was 
already added, however it would involve more code in each caller. So it 
is much easier to add in iommu_add_dt_device().
Cheers,

--
Julien Grall