[PATCH 11/27] iommu/arm-smmu-v3: Lift CD programming out of the SVA notifier code

[Michael Shavit]

On Thu, Oct 12, 2023 at 7:26 AM Jason Gunthorpe <jgg at nvidia.com> wrote:
> [...]
> -static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
> +static struct arm_smmu_ctx_desc *
> +arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
>  {
>         struct mm_struct *mm = smmu_mn->mn.mm;
>         struct arm_smmu_ctx_desc *cd = smmu_mn->cd;
>         struct arm_smmu_domain *smmu_domain = smmu_mn->domain;
> -       struct arm_smmu_master *master;
> -       unsigned long flags;
>
>         if (!refcount_dec_and_test(&smmu_mn->refs))
> -               return;
> +               return cd;
>
>         list_del(&smmu_mn->list);
>
> -       spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -       list_for_each_entry(master, &smmu_domain->devices, domain_head)
> -               arm_smmu_clear_cd(master, mm->pasid);
> -       spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> -
>         /*
>          * If we went through clear(), we've already invalidated, and no
>          * new TLB entry can have been formed.

This re-orders the TLB invalidation before the CD entry is cleared.
Couldn't a misbehaving device form TLB entries in this time interval
that we'd want to avoid?

> @@ -434,11 +403,12 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
>
>         /* Frees smmu_mn */
>         mmu_notifier_put(&smmu_mn->mn);
> -       arm_smmu_free_shared_cd(cd);
> +       return cd;
>  }
>
> -static struct iommu_sva *
> -__arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
> +static struct iommu_sva *__arm_smmu_sva_bind(struct device *dev,
> +                                            struct mm_struct *mm,
> +                                            struct arm_smmu_cd *target)
>  {
>         int ret;
>         struct arm_smmu_bond *bond;
> @@ -456,6 +426,8 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
>         list_for_each_entry(bond, &master->bonds, list) {
>                 if (bond->mm == mm) {
>                         refcount_inc(&bond->refs);
> +                       arm_smmu_make_sva_cd(target, master, mm,
> +                                            bond->smmu_mn->cd->asid);
>                         return &bond->sva;
>                 }
>         }
> @@ -475,6 +447,7 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
>         }
>
>         list_add(&bond->list, &master->bonds);
> +       arm_smmu_make_sva_cd(target, master, mm, bond->smmu_mn->cd->asid);
>         return &bond->sva;
>
>  err_free_bond:
> @@ -646,9 +619,15 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
>         }
>
>         if (!WARN_ON(!bond) && refcount_dec_and_test(&bond->refs)) {
> +               struct arm_smmu_ctx_desc *cd;
> +
>                 list_del(&bond->list);
> -               arm_smmu_mmu_notifier_put(bond->smmu_mn);
> +               cd = arm_smmu_mmu_notifier_put(bond->smmu_mn);
> +               arm_smmu_remove_pasid(master, to_smmu_domain(domain), id);
> +               arm_smmu_free_shared_cd(cd);
>                 kfree(bond);

arm_smmu_mmu_notifier_put was previously only calling
free_shared_cd(cd) when smmu_mn's refcount reached 0. IIRC, the
arm_smmu_mmu_notifier refcount can be greater than 1 if an MM/SVA
domain is attached to devices with distinct SMMU instances.

> +       } else {
> +               arm_smmu_remove_pasid(master, to_smmu_domain(domain), id);
>         }

I don't think we ever expect this else condition to be hit. Patch
"iommu/arm-smmu-v3-sva: Remove bond refcount" in Joerg's next tree
removes the refcount, and bond shouldn't ever be NULL for a "valid"
pasid either.

> [...]
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2576,6 +2576,30 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>         return 0;
>  }
>
> +int arm_smmu_set_pasid(struct arm_smmu_master *master,
> +                      struct arm_smmu_domain *smmu_domain, ioasid_t id,
> +                      const struct arm_smmu_cd *cd)
> +{
> +       struct arm_smmu_domain *old_smmu_domain =
> +               to_smmu_domain_safe(iommu_get_domain_for_dev(master->dev));

nit: The name old_smmu_domain sounds to me like it's being replaced
with a newer domain.