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

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Oct 26, 2023 at 03:31:41PM +0800, Michael Shavit wrote:
> On Wed, Oct 25, 2023 at 7:46 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > 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'
> 
> Just to confirm, why "No for the 'inv_asid'"? My best guess:
> 1. Transactions don't hit the TLB entries unless there's a valid CD
> configured with that ASID
> 2. You're relying on those TLB entries being cleared elsewhere in the
> code, when freeing/reclaiming the ASID from the domain.

Ahhh I was too thoughtless to say that!

Yes, we need to have the CD removed before doing the IOTLB
invalidation too because we are trying to clear the IOTLB of that ASID
so the ASID is clean before going back to the allocator.

I got a bit confused because later on in the series that specific
invalidation is moved into arm_smmu_domain_free_id() and moved over to
domain deallocation, but at this point we don't have that yet so it is
still doing something important. Regardless it is fixed!

> But this also makes me curious why we bother with an ASID invalidation
> in the first place if it's not required for correctness.

ASIDs put back into the xarray for allocation must be clean in the
IOTLB as we don't do an invalidation when we allocate an unused ASID
from the xarray. An ASID that is not clean could have stale
translations that are not valid. See patch 22

Jason