Re: [PATCH] xen/arm: smmuv1: remove iommu group when deassign a device

[Julien Grall <julien@xxxxxxx>]

On 29/04/2022 15:33, Rahul Singh wrote:
> Hi Julien,
Hi Rahul,

> > On 27 Apr 2022, at 6:42 pm, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi,
> >
> > On 27/04/2022 17:15, Rahul Singh wrote:
> > > When a device is deassigned from the domain it is required to remove the
> > > iommu group.
> > This read wrong to me. We should not need to re-create the IOMMU group (and 
> > call arm_smmu_add_device()) every time a device is re-assigned.
> Ok.
> > > If we don't remove the group, the next time when we assign
> > > a device, SME and S2CR will not be setup correctly for the device
> > > because of that SMMU fault will be observed.
> > I think this is a bug fix for 0435784cc75dcfef3b5f59c29deb1dbb84265ddb. If so, 
> > please add a Fixes tag.
> Ok Let me add the Fixes tag in next version.
> > > Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> > > ---
> > > xen/drivers/passthrough/arm/smmu.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > > diff --git a/xen/drivers/passthrough/arm/smmu.c 
> > > b/xen/drivers/passthrough/arm/smmu.c
> > > index 5cacb2dd99..9a31c332d0 100644
> > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > @@ -1690,6 +1690,8 @@ static void arm_smmu_detach_dev(struct iommu_domain 
> > > *domain, struct device *dev)
> > >         if (cfg)
> > >                 arm_smmu_master_free_smes(cfg);
> > > +       iommu_group_put(dev_iommu_group(dev));
> > > +       dev_iommu_group(dev) = NULL;
> > > }
> > The goal of arm_smmu_detach_dev() is to revert the change made in 
> > arm_smmu_attach_dev(). But looking at the code, neither the IOMMU group nor the 
> > smes are allocated in arm_smmu_attach_dev().
> >
> > Are the SMES meant to be re-allocated everytime we assign to a different 
> > domain? If yes, the allocation should be done in arm_smmu_attach_dev().
> Yes SMES have to be re-allocated every time a device is assigned.
Hmmmm.... Looking at the code, arm_smmu_alloc_smes() doesn't seem to use 
the domain information. So why would it need to be done every time it is 
assigned?
> Is that okay if I will move the function arm_smmu_master_alloc_smes() from 
> arm_smmu_add_device() to arm_smmu_attach_dev().
> In this case we don’t need to remove the IOMMU group and also 
> arm_smmu_detach_dev() will also revert the  change made in arm_smmu_attach_dev().
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index 5cacb2dd99..ff1b73d3d8 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -1680,6 +1680,10 @@ static int arm_smmu_attach_dev(struct iommu_domain 
> *domain, struct device *dev)
>          if (!cfg)
>                  return -ENODEV;
>   
> +       ret = arm_smmu_master_alloc_smes(dev);
> +       if (ret)
> +               return ret;
> +
>          return arm_smmu_domain_add_master(smmu_domain, cfg);
If we go down this route, then you will likely need to revert the change 
made by arm_smmu_master_alloc_smes().
>   }
>   
> @@ -2075,7 +2079,7 @@ static int arm_smmu_add_device(struct device *dev)
>          iommu_group_add_device(group, dev);
>          iommu_group_put(group);
>   
> -       return arm_smmu_master_alloc_smes(dev);
> +       return 0;
>   }
>
> Regards,
> Rahul
> > If not, then we should not free the SMES here
> >
> > IIUC, the SMES have to be re-allocated every time a device is assigned. 
> > Therefore, I think we should move the call to arm_smmu_master_alloc_smes() out 
> > of the detach callback and in a helper that would be used when removing a 
> > device (not yet supported by Xen).
> >
> > Cheers,
> >
> > --
> > Julien Grall
--
Julien Grall