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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 23/07/2021 21:16, Stefano Stabellini wrote:
> On Fri, 23 Jul 2021, Julien Grall wrote:
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > > ---
> > > Changes in v5:
> > > - new patch
> > > ---
> > >    xen/drivers/passthrough/device_tree.c | 9 +++++++--
> > >    1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/drivers/passthrough/device_tree.c
> > > b/xen/drivers/passthrough/device_tree.c
> > > index 999b831d90..32526ecabb 100644
> > > --- 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.
> > Wouldn't this also happen when there is just generic bindings? If so,
> > shouldn't this patch be first in the series to avoid breaking bisection?
> No, both need to be present; if there is just the generic bindings we
> don't need this change. I can still move it to the beginning of the
> series anyway if you prefer.
Sorry but I am having some trouble to understand why this is not a 
problem for just the legacy binding.
If I look at patch #1, the dev->iommu_fspec will be allocated in 
register_smmu_master(). If I am not mistaken, this is called when the 
SMMU is initialized.
So the call to iommu_add_dt_device() in handle_device() should return 
-EEXIST (dev_iommu_fwspec_get() will return a non-NULL pointer).
What did I miss?

> > > +     */
> > My point on the previous version is this is not the only reasons why
> > dev_iommu_fwspec_get(). So either we want to write all the reasons (AFAICT,
> > there is only two) or we want to write a generic message.
> I see. Maybe:
>
>    * In some circumstances iommu_add_dt_device() can genuinly be called
>    * twice. As there is no harm in it just return success early.
Sound good to me.

> > >        if ( dev_iommu_fwspec_get(dev) )
> > > -        return -EEXIST;
> > > +        return 0;
> > >          /*
> > >         * According to the Documentation/devicetree/bindings/iommu/iommu.txt
> > > @@ -254,7 +259,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct
> > > domain *d,
> > >             * already added to the IOMMU (positive result). Such happens
> > > after
> > >             * re-creating guest domain.
> > >             */
> > This comment on top is now stale.
> I missed it somehow; yes definitely it should be removed. I can do it in
> the next version of this patch.
>
>
> > > -        if ( ret < 0 && ret != -EEXIST )
> > > +        if ( ret < 0 )
> > >            {
> > >                printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n",
> > >                       dt_node_full_name(dev));
Cheers,

--
Julien Grall