Hi Julien, > On 27 Apr 2022, at 6:42 pm, Julien Grall <[email protected]> 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 <[email protected]> >> --- >> 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. 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); } @@ -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
