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

[Mark Rutland]

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.

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!

Thanks,
Mark.