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