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 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.
 */

Cheers,

--
Julien Grall