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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 2 Aug 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 30/07/2021 22:57, Stefano Stabellini wrote:
> > On Mon, 26 Jul 2021, Julien Grall wrote:
> > > 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?
> > 
> > I checked. It is true that we need this check with the legacy bindings,
> > even when alone.
> > 
> > Like you said, dev->iommu_fspec is allocated early by
> > register_smmu_master. Then, handle_device, or handle_passthrough_prop
> > for dom0less guests, calls iommu_add_dt_device a second time.
> > 
> > On the other hand with only the generic bindings register_smmu_master
> > doesn't call iommu_add_dt_device, so iommu_add_dt_device is called only
> > once by handle_device or handle_passthrough_prop. >
> > 
> > The comment I proposed is not correct. What about this one?
> > 
> >      /*
> >       * In case of legacy SMMU bindings, register_smmu_master might have
> >       * already initialized struct iommu_fwspec for this device.
> >       */
> As I may have mentionned in a previous version of the series, this check is
> not specific to the SMMU. We also need it for other cases (e.g. when the
> device is (re-)assigned to a guest).
> 
> So I think a specialized comment is unsuitable here. Instead, we should
> provide a generic comment. Something like:
> 
> /*
>  * The device may already have been registered. As there is no harm in
>  * it just return success early.
>  */

Thanks, I'll use it in the next version