Re: [PATCH] xen/arm: smmuv1: Set s2cr to type fault when the devices are deassigned

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 9 Aug 2022, at 7:19 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> title: The driver is for both smmuv1 and v2. Are you suggesting the issue 
> only occurs on v1?

Issue occurs on both v1 & v2. I will fix this in next version.
> 
> On 05/08/2022 16:43, Rahul Singh wrote:
>> When devices are deassigned/assigned, SMMU global fault is observed
>> because SMEs are freed in detach function and not allocated again when
>> the device is assigned back to the guest.
>> Don't free the SMEs when devices are deassigned, set the s2cr to type
>> fault. This way the SMMU will generate a fault if a DMA access is done
>> by a device not assigned to a guest
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> 
> AFAICT, this is fixing 0435784cc75d ("xen/arm: smmuv1: Intelligent SMR 
> allocation"). If I am correct, can you add a Fixes tag?

Yes, I will add the fixes tag.
> 
>> ---
>>  xen/drivers/passthrough/arm/smmu.c | 32 +++++++++++++++---------------
>>  1 file changed, 16 insertions(+), 16 deletions(-)
>> diff --git a/xen/drivers/passthrough/arm/smmu.c 
>> b/xen/drivers/passthrough/arm/smmu.c
>> index 69511683b4..141948decd 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)
> 
> IIUC, the function needs to be moved because you need to use 
> arm_smmu_write_s2cr(). If so, I would suggest to mention in the commit 
> message because at first it seems unwarranted.

Ack. I will add that in commit msg.
> 
>> -{
>> -    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)
>> +{
>> +    int i, idx;
> 
> NIT: I would suggest to take the opportunity to switch to "unsigned int" and 
> ...

Ack. 
> 
>> +    struct arm_smmu_device *smmu = smmu_domain->smmu;
>> +    struct arm_smmu_s2cr *s2cr = smmu->s2crs;
>> +    struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
> 
> ... use const here. "cfg" and "smmu" can't be consistent but "smmu_domain" 
> technically could (thanks to how C works). That said, I quite dislike it as 
> the code ends up to be confusing...

Ack. 
> 
>> +
>> +    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);
> 
> Why are you using adding a 'return' here?

Not required. I will remove “return”.

Regards,
Rahul