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

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:

quoted

On Wed, Feb 24, 2021 at 09:37:37AM +0000, Marc Zyngier wrote:

quoted

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.

quoted

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.

quoted

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@kernel.org>
Signed-off-by: Marc Zyngier <maz@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@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.

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