[PATCH 2/3] arm64: Add missing ISB after invalidating TLB in __primary_switch

[Will Deacon]

On Wed, Feb 24, 2021 at 11:45:40AM +0000, Mark Rutland wrote:
> On Wed, Feb 24, 2021 at 11:16:50AM +0000, Will Deacon wrote:
> > On Wed, Feb 24, 2021 at 11:06:43AM +0000, Mark Rutland wrote:
> > > On Wed, Feb 24, 2021 at 09:37:37AM +0000, Marc Zyngier wrote:
> > > > Although there has been a bit of back and forth on the subject,
> > > > it appears that invalidating TLBs requires an ISB instruction
> > > > after the TLBI/DSB sequence, as documented in d0b7a302d58a
> > > > ("Revert "arm64: Remove unnecessary ISBs from set_{pte,pmd,pud}"").
> > > 
> > > That commit describes a different scenario (going faulting->valid
> > > without TLB maintenance), and I don't think that implies anything about
> > > the behaviour in the presence of a TLBI, which is quite different.
> > 
> > Sorry, that was my fault. I remember this all happened around the same
> > time.
> > 
> > > Howerver, I do see that commits:
> > > 
> > >   7f0b1bf045113489 ("arm64: Fix barriers used for page table modifications")
> > >   51696d346c49c6cf ("arm64: tlb: Ensure we execute an ISB following walk cache invalidation")
> > > 
> > > ... assume that we need an ISB after a TLBI+DSB, so I think it would be
> > > better to refer to those, to avoid conflating the distinct cases.
> > > 
> > > > Add the missing ISB in __primary_switch, just in case.
> > > > 
> > > > Fixes: 3c5e9f238bc4 ("arm64: head.S: move KASLR processing out of __enable_mmu()")
> > > > Suggested-by: Will Deacon <will at kernel.org>
> > > > Signed-off-by: Marc Zyngier <maz at kernel.org>
> > > 
> > > For consistency with the other kernel TLBI paths, I'm fine with this
> > > (assuming we update the commit message accordingly):
> > > 
> > > Acked-by: Mark Rutland <mark.rutland at arm.com>
> > > 
> > > My understanding is that we don't need an ISB after invalidation, and if
> > > we align on that understanding we can follow up and update all of the
> > > TLBI paths in one go.
> > 
> > Without FEAT_ETS, I think the ISB is required to complete TLBI:
> > 
> >  | In an implementation that does not implement FEAT_ETS, a TLB maintenance
> >  | instruction executed by a PE, PEx, can complete at any time after it is
> >  | issued, but is only guaranteed to be finished for a PE, PEx, after the
> >  | execution of DSB by the PEx followed by a Context synchronization event.
> 
> Ah; given that quote, I agree.
> 
> Can we put that in the commit message? I think that's clearer than
> referring to any commit, and with that my ack definitely stands.

Done!

> I /hope/ that quote means the same PEx in both cases (i.e. only the
> issuing PE needs the context synchronization event), or we have much
> greater problems!

Yeah, that's definitely what it's supposed to mean (I remember working on
this), but the wording ain't great! Other PEs are covered in a separate
statement:

 | A TLB maintenance instruction executed by a PE, PEx, can complete at any time
 | after it is issued, but is only guaranteed to be finished for a PE other than
 | PEx after the execution of DSB by the PEx.

which would make for an interesting tutorial exercise for somebody studying
temporal logic.

Will