Re: [PATCH] xen/arm: smmu: Set/clear IOMMU domain for device

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

Hi, Julien!

On 17.08.21 20:21, Julien Grall wrote:
> Hi Oleksandr,
>
> Apologies for the late answer.
>
> On 11/08/2021 14:03, Oleksandr Andrushchenko wrote:
> > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >
> > When a device is assigned/de-assigned it is required to properly set
> > IOMMU domain used to protect the device. This assignment was missing,
> > thus it was not possible to de-assign the device:
> >
> > (XEN) Deassigning device 0000:03:00.0 from dom2
> > (XEN) smmu: 0000:03:00.0:  not attached to domain 2
> > (XEN) d2: deassign (0000:03:00.0) failed (-3)
> >
> > Fix this by assigning IOMMU domain on arm_smmu_assign_dev and reset it
> > to NULL on arm_smmu_deassign_dev.
> I think this was introduced by commit 06d1f7a278dd "xen/arm: smmuv1: Keep track of 
> S2CR state". If so, please add:
>
> Fixes: 06d1f7a278dd ("xen/arm: smmuv1: Keep track of S2CR state")
Will do
> Looking at the commit message, the IOMMU domain used to be set/unset in 
> arm_smmu_{attach, detach}_dev() but Linux drop it because they now rely in the 
> core IOMMU framework to track the domain.
>
> So I agree with the new position for...
>
> > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > ---
> >   xen/drivers/passthrough/arm/smmu.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/xen/drivers/passthrough/arm/smmu.c 
> > b/xen/drivers/passthrough/arm/smmu.c
> > index c234ad9c7f1e..373d9d4d123a 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -2768,6 +2768,7 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
> >               arm_smmu_destroy_iommu_domain(domain);
> >       } else {
> >           atomic_inc(&domain->ref);
> > +        dev_iommu_domain(dev) = domain;
> ... this one. However...
>
> >       }
> >     out:
> > @@ -2794,7 +2795,10 @@ static int arm_smmu_deassign_dev(struct domain *d, 
> > struct device *dev)
> >       atomic_dec(&domain->ref);
> >         if (domain->ref.counter == 0)
> > +    {
> >           arm_smmu_destroy_iommu_domain(domain);
> > +        dev_iommu_domain(dev) = NULL;
> > +    }
> ... I think this one is incorrect because you would only unset 
> dev_iommu_domain() is the refcount drop to 0. You can have multiple device in 
> the same domain, so the ref.counter may not be 0.
>
> So I think this needs to be moved outside of the if.
Yes, absolutely
> Note that it is also a good practice to remove any reference (e.g. set to NULL) 
> before freeing even if it doesn't much matter here.
>
> Lastly, the file is using the Linux coding style. So { needs to be on the same 
> line as the if.
>
> Cheers,
Thanks,

Oleksandr