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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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