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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 23 Jul 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/07/2021 00:36, Stefano Stabellini wrote:
> > If both legacy IOMMU bindings and generic bindings are present,
> > iommu_add_dt_device can be called twice. Do not return error in that
> > case, that way there is no need to check for -EEXIST at the call sites.
> > Remove the one existing -EEXIT check, now unneeded.
> 
> The commit message implies that we already support both legacy and generic
> bindings. However, this is not yet implemented.
> 
> So how about:
> 
> "
> iommu_add_dt_device() will returns -EEXIST if the device was already
> registered.
> 
> At the moment, this can only happen if the device was already assigned to a
> domain (either dom0 at boot or via XEN_DOMCTL_assign_device).
> 
> In a follow-up patch, we will convert the SMMU driver to use the FW spec. When
> the legacy bindings are used, all the devices will be registered at probe.
> Therefore, iommu_add_dt_device() will always returns -EEXIST.
> 
> Currently, one caller (XEN_DOMCTL_assign_device) will check the return and
> ignore -EEXIST. All the other will fail because it was technically a
> programming error.
> 
> However, there is no harm to call iommu_add_dt_device() twice, so we can
> simply return 0.
> 
> With that in place the caller doesn't need to check -EEXIST anymore, so remove
> the check.
> "

This is a lot better, thank you!


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


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


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