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

[Will Deacon]

On Wed, Feb 14, 2024 at 03:09:54PM -0400, Jason Gunthorpe wrote:
> 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?

Ah yes, you're quite right. We'd need to add the allocation to
arm_smmu_attach_dev() as well.

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

This is what I originally had in mind, but for some reason I thought you
weren't happy with it. Mind if I fold it into your -rc patch? I'd really
like them to go in together.

Will