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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Mykyta,

> On 16 Jun 2022, at 8:48 am, Mykyta Poturai <mykyta.poturai@xxxxxxxxx> wrote:
> 
> Hi Julien, Rahul
> I've encountered a similar problem with IMX8 GPU recently. It wasn't probing
> properly after the domain reboot.  After some digging, I came to the same
> solution as Rahul and found this thread. I also encountered the occasional
> "Unexpected global fault, this could be serious" error message when destroying
> a domain with an actively-working GPU.
> 
>> 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?
> Indeed after removing the arm_smmu_master_free_smes() call, both reboot and 
> global
> fault issues are gone. If I understand correctly, device removing is not yet
> supported, so I can't find a proper place for the arm_smmu_master_free_smes() 
> call.
> Should we remove the function completely or just left it commented for later 
> or
> something else?
> 
> Rahul, are you still working on this or could I send my patch?

Yes, I have this on my to-do list but I was busy with other work and it got 
delayed. 

I created another solution for this issue, in which we don’t need to call 
arm_smmu_master_free_smes() 
in arm_smmu_detach_dev() but we can configure the s2cr value to type fault in 
detach function.

Will call new function arm_smmu_domain_remove_master() in detach function that 
will revert the changes done 
by arm_smmu_domain_add_master()  in attach function.

I don’t have any board to test the patch. If it is okay, Could you please test 
the patch and let me know the result.

diff --git a/xen/drivers/passthrough/arm/smmu.c 
b/xen/drivers/passthrough/arm/smmu.c
index 69511683b4..da3adf8e7f 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1598,21 +1598,6 @@ out_err:
        return ret;
 }
 
-static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg)
-{
-    struct arm_smmu_device *smmu = cfg->smmu;
-       int i, idx;
-       struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
-
-       spin_lock(&smmu->stream_map_lock);
-       for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
-               if (arm_smmu_free_sme(smmu, idx))
-                       arm_smmu_write_sme(smmu, idx);
-               cfg->smendx[i] = INVALID_SMENDX;
-       }
-       spin_unlock(&smmu->stream_map_lock);
-}
-
 static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
                                      struct arm_smmu_master_cfg *cfg)
 {
@@ -1635,6 +1620,20 @@ static int arm_smmu_domain_add_master(struct 
arm_smmu_domain *smmu_domain,
        return 0;
 }
 
+static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
+                                     struct arm_smmu_master_cfg *cfg)
+{
+       struct arm_smmu_device *smmu = smmu_domain->smmu;
+       struct arm_smmu_s2cr *s2cr = smmu->s2crs;
+       struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
+       int i, idx;
+
+       for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
+               s2cr[idx] = s2cr_init_val;
+               arm_smmu_write_s2cr(smmu, idx);
+       }
+}
+
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
        int ret;
@@ -1684,10 +1683,11 @@ static int arm_smmu_attach_dev(struct iommu_domain 
*domain, struct device *dev)
 
 static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device 
*dev)
 {
+       struct arm_smmu_domain *smmu_domain = domain->priv;
        struct arm_smmu_master_cfg *cfg = find_smmu_master_cfg(dev);
 
        if (cfg)
-               arm_smmu_master_free_smes(cfg);
+               return arm_smmu_domain_remove_master(smmu_domain, cfg);
 
 }

Regards,
Rahul