Re: [PATCH v3 1/8] arm64: KVM: Make VHE Stage-2 TLB invalidation operations non-interruptible

From: Christoffer Dall <christoffer.dall@arm.com>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	kvm@vger.kernel.org, Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	James Morse <james.morse@arm.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 1/8] arm64: KVM: Make VHE Stage-2 TLB invalidation operations non-interruptible
Date: Mon, 10 Dec 2018 11:49:17 +0100	[thread overview]
Message-ID: <20181210104917.GM30263@e113682-lin.lund.arm.com> (raw)
In-Reply-To: <d9219768-bbef-5d51-3bb4-84cf35714a4d@arm.com>

On Mon, Dec 10, 2018 at 10:24:31AM +0000, Marc Zyngier wrote:
> Hi Christoffer,
> 
> On 10/12/2018 10:03, Christoffer Dall wrote:
> > On Thu, Dec 06, 2018 at 05:31:19PM +0000, Marc Zyngier wrote:
> >> Contrary to the non-VHE version of the TLB invalidation helpers, the VHE
> >> code  has interrupts enabled, meaning that we can take an interrupt in
> >> the middle of such a sequence, and start running something else with
> >> HCR_EL2.TGE cleared.
> > 
> > Do we have to clear TGE to perform the TLB invalidation, or is that just
> > a side-effect of re-using code?
> 
> We really do need to clear TGE. From the description of TLBI VMALLE1IS:
> 
> <quote>
> When EL2 is implemented and enabled in the current Security state:
> — If HCR_EL2.{E2H, TGE} is not {1, 1}, the entry would be used with the
> current VMID and would be required to translate the specified VA using
> the EL1&0 translation regime.
> — If HCR_EL2.{E2H, TGE} is {1, 1}, the entry would be required to
> translate the specified VA using the EL2&0 translation regime.
> </quote>
> 
> > Also, do we generally perform TLB invalidations in the kernel with
> > interrupts disabled, or is this just a result of clearing TGE?
> 
> That's definitely a result of clearing TGE. We could be taking an
> interrupt here, and execute a user access on the back of it (perf will
> happily walk a user-space stack in that context, for example). Having
> TGE clear in that context. An alternative solution would be to
> save/restore TGE on interrupt entry/exit, but that's a bit overkill when
> you consider how rarely we issue such TLB invalidation.
> 
> > Somehow I feel like this should look more like just another TLB
> > invalidation in the kernel, but if there's a good reason why it can't
> > then this is fine.
> 
> The rest of the TLB invalidation in the kernel doesn't need to
> save/restore any context. They apply to a set of parameters that are
> already loaded on the CPU. What we have here is substantially different.
> 

Thanks for the explanation and Arm ARM quote.  I failed to find that on
my own this particular Monday morning.

Acked-by: Christoffer Dall <christoffer.dall@arm.com>

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