Re: [PATCH rc] iommu/arm-smmu-v3: Do not use GFP_KERNEL under as spinlock

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Feb 14, 2024 at 05:00:30PM +0000, Will Deacon wrote:

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 0ffb1cf17e0b..e48f2b46f25e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1019,7 +1019,10 @@ static void arm_smmu_write_cd_l1_desc(__le64 *dst,
>  	WRITE_ONCE(*dst, cpu_to_le64(val));
>  }
>  
> -static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid)
> +static __le64 *
> +__arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid,
> +		      int (*cd_alloc)(struct arm_smmu_device *,
> +				      struct arm_smmu_l1_ctx_desc *))
>  {
>  	__le64 *l1ptr;
>  	unsigned int idx;
> @@ -1033,7 +1036,7 @@ static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid)
>  	idx = ssid >> CTXDESC_SPLIT;
>  	l1_desc = &cd_table->l1_desc[idx];
>  	if (!l1_desc->l2ptr) {
> -		if (arm_smmu_alloc_cd_leaf_table(smmu, l1_desc))
> +		if (!cd_alloc || cd_alloc(smmu, l1_desc))
>  			return NULL;
>  
>  		l1ptr = cd_table->cdtab + idx * CTXDESC_L1_DESC_DWORDS;
> @@ -1045,6 +1048,19 @@ static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid)
>  	return l1_desc->l2ptr + idx * CTXDESC_CD_DWORDS;
>  }
>  
> +static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid)
> +{
> +	return __arm_smmu_get_cd_ptr(master, ssid, NULL);
> +}

I think this will break arm_smmu_write_ctx_desc() which requires
allocation behavior to install the SSID0 in a two level table?

> +int arm_smmu_init_cd_table(struct arm_smmu_master *master, int ssid)
> +{
> +	if (!__arm_smmu_get_cd_ptr(master, ssid, arm_smmu_alloc_cd_leaf_table))
> +		return -ENOMEM;
> +
> +	return 0;
> +}

And this does not compose well with the rest of where we are going. At
the end there are 7 calls to arm_smmu_get_cd_ptr(), all need
allocation except for two (clear and mmu notifier release)

Better to add a noalloc version. Also 'bool noalloc' would be clearer
than a function pointer without obfuscating the control flow.

Also I don't think this should go to -rc, it is not fixing any
bug. The preallocation is enough. Making more clarity for this
confused code is a nice to have at best.

How about this instead as a -next patch:

--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -288,10 +288,11 @@ static const struct mmu_notifier_ops arm_smmu_mmu_notifier_ops = {
 
 /* Allocate or get existing MMU notifier for this {domain, mm} pair */
 static struct arm_smmu_mmu_notifier *
-arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
+arm_smmu_mmu_notifier_get(struct arm_smmu_master *true_master,
+                         struct arm_smmu_domain *smmu_domain,
                          struct mm_struct *mm)
 {
-       int ret;
+       int ret = 0;
        unsigned long flags;
        struct arm_smmu_ctx_desc *cd;
        struct arm_smmu_mmu_notifier *smmu_mn;
@@ -327,8 +328,15 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
 
        spin_lock_irqsave(&smmu_domain->devices_lock, flags);
        list_for_each_entry(master, &smmu_domain->devices, domain_head) {
-               ret = arm_smmu_write_ctx_desc(master, mm_get_enqcmd_pasid(mm),
-                                             cd);
+               /*
+                * A limitation of the current SVA code requires the RID
+                * smmu_domain to be unique to the actual master.
+                */
+               if (WARN_ON(master != true_master))
+                       ret = -EINVAL;
+               if (!ret)
+                       ret = arm_smmu_write_ctx_desc(
+                               master, mm_get_enqcmd_pasid(mm), cd);
                if (ret) {
                        list_for_each_entry_from_reverse(
                                master, &smmu_domain->devices, domain_head)
@@ -398,7 +406,7 @@ static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 
        bond->mm = mm;
 
-       bond->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain, mm);
+       bond->smmu_mn = arm_smmu_mmu_notifier_get(master, smmu_domain, mm);
        if (IS_ERR(bond->smmu_mn)) {
                ret = PTR_ERR(bond->smmu_mn);
                goto err_free_bond;

Thanks,
Jason