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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxx>
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