xen/arm: smmuv1: Set s2cr to type fault when the devices are deassigned

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

Posted by Rahul Singh 1 year, 8 months ago

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@arm.com>
---
 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)
-{
-    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;
+	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
+	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
+
+	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);
 
 }
 
-- 
2.25.1

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

Posted by Julien Grall 1 year, 8 months ago

Hi Rahul,

title: The driver is for both smmuv1 and v2. Are you suggesting the 
issue only occurs on v1?

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@arm.com>

AFAICT, this is fixing 0435784cc75d ("xen/arm: smmuv1: Intelligent SMR 
allocation"). If I am correct, can you add a 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.

> -{
> -    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 ...

> +	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...

> +
> +	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?

Cheers,

-- 
Julien Grall

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

Posted by Rahul Singh 1 year, 8 months ago

Hi Julien,

> On 9 Aug 2022, at 7:19 pm, Julien Grall <julien@xen.org> 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@arm.com>
> 
> 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