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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Oct 24, 2023 at 02:34:28PM +0800, Michael Shavit wrote:
> On Thu, Oct 12, 2023 at 7:26 AM Jason Gunthorpe <jgg@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?

Hum.. No for the 'inv_asid', but yes for the 'atc_inv_domain'

This actually looks like something I was not fully careful with even
in the end. The SID and PASID attach paths do have an ATC flush when
changing the translation. The SID detach paths indirectly do because
they turn off ATS, which flushes.

It is missing for the PASID detach and SID detach when ATS is left on.
Those need fixes in other patches

This specific code gets deleted pretty soon, but we can make it
better.

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

I can no longer remember why this hunk moving
arm_smmu_free_shared_cd() is here. I think it may have been a left
over from a discarded direction.

So, like this:

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index d643c8634467c5..29469073fc53fe 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -378,15 +378,14 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
 	return ERR_PTR(ret);
 }
 
-static struct arm_smmu_ctx_desc *
-arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
+static void 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;
 
 	if (!refcount_dec_and_test(&smmu_mn->refs))
-		return cd;
+		return;
 
 	list_del(&smmu_mn->list);
 
@@ -401,11 +400,11 @@ arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 
 	/* Frees smmu_mn */
 	mmu_notifier_put(&smmu_mn->mn);
-	return cd;
+	arm_smmu_free_shared_cd(cd);
 }
 
 static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
-					     struct arm_smmu_cd *target)
+			       struct arm_smmu_cd *target)
 {
 	int ret;
 	struct arm_smmu_bond *bond;
@@ -595,6 +594,8 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 	struct arm_smmu_bond *bond = NULL, *t;
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 
+	arm_smmu_remove_pasid(master, to_smmu_domain(domain), id);
+
 	mutex_lock(&sva_lock);
 	list_for_each_entry(t, &master->bonds, list) {
 		if (t->mm == mm) {
@@ -604,15 +605,9 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 	}
 
 	if (!WARN_ON(!bond)) {
-		struct arm_smmu_ctx_desc *cd;
-
 		list_del(&bond->list);
-		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);
+		arm_smmu_mmu_notifier_put(bond->smmu_mn);
 		kfree(bond);
-	} else {
-		arm_smmu_remove_pasid(master, to_smmu_domain(domain), id);
 	}
 	mutex_unlock(&sva_lock);
 }

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

Sure, a later patch eventually changes this to be 'sid_domain'
(without the arm_smmu_domain type) so lets just call this
sid_smmu_domain here.

Thanks,
Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel