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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Mykyta,

> On 17 Jun 2022, at 12:15 pm, Mykyta Poturai <mykyta.poturai@xxxxxxxxx> wrote:
> 
>> 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
> 
> Hello Rahul,
> 
> For me, this patch fixed the issue with the GPU not probing after domain 
> reboot.

Thanks for testing the patch.
> But not fixed the "Unexpected Global fault" that occasionally happens when 
> destroying
> the domain with an actively working GPU. Although, I am not sure if this issue
> is relevant here.

Can you please if possible share the more details and logs so that I can look 
if this issue is relevant here ?

Thanks in advance.

Regards,
Rahul