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

From: Mark Rutland <mark.rutland@arm.com>
To: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>,
	Guillaume Tucker <guillaume.tucker@collabora.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	kernel-team@android.com, Ard Biesheuvel <ardb@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/3] arm64: Add missing ISB after invalidating TLB in __primary_switch
Date: Wed, 24 Feb 2021 11:45:40 +0000	[thread overview]
Message-ID: <20210224114540.GD50741@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20210224111648.GA11554@willie-the-truck>

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