Re: Query regarding ERRATUM_1418040

[Marc Zyngier <maz@kernel.org>]

On 2020-07-01 17:27, Will Deacon wrote:
> On Wed, Jun 17, 2020 at 03:29:46PM +0100, Marc Zyngier wrote:
>> On 2020-06-17 12:25, Will Deacon wrote:
>> > On Wed, Jun 17, 2020 at 12:19:16PM +0100, Marc Zyngier wrote:
>> > > On 2020-06-17 09:55, Neeraj Upadhyay wrote:
>> > > > I have query regarding the errata 1418040 [1]. Here, on kernel exit to
>> > > > EL0 64 bit mode, will it always enable ARCH_TIMER_USR_VCT_ACCESS_EN;
>> > > > and override any other erratas, which might require EL0 direct vct
>> > > > access to be disabled for 64 bit also?
>> > >
>> > > So far, I am not aware of any erratum that would require traps of
>> > > CNTVCT_EL0
>> > > to EL1 when running AArch64 userspace for CPUs that are affected by
>> > > ARM-1418040. If such an erratum exists, it would have to be handled
>> > > separately.
>> > >
>> > > > Also, this errata applies
>> > > > mitigation for all CPUs (on return to 32 bit EL0), even if, not all
>> > > > cpus are impacted by the errata?
>> > >
>> > > Indeed. There isn't much we can do to avoid it here, unless you want
>> > > to read
>> > > some per-CPU variable that tells you whether the CPU is affected.
>> > > This would
>> > > add to the cost of the mitigation , and isn't an appealing prospect.
>> >
>> > Hmm, but in conjunction with the previous point, doesn't this mean if
>> > some CPUs are affected by an erratum which requires CNTVCT trapping for
>> > AArch64 and others are affected by 1418040, then the former won't
>> > actually
>> > be trapped?
>> 
>> Indeed. Having CPUs that require opposite workarounds is one of the 
>> many
>> fascinating aspects of BL systems... :-/ Does such a system exist 
>> today?
> 
> I don't know, but it feels like we should either address the issue of 
> scream
> loudly if we detect it!

I have no idea how you plan to detect that.

>> > Maybe we should preserve ARCH_TIMER_USR_VCT_ACCESS_EN for AArch64 tasks
>> > instead of setting it unconditionally?
>> 
>> We'd still need something when switching from an AArch32 task to an 
>> AArch64
>> one. I guess we'd either need to re-enable it on entry from a 32bit 
>> task, or
>> implement some sort of per-CPU, per-ISA state to be restored on return 
>> to
>> userspace.
> 
> I think re-enabling on entry from a 32-bit task would be the easiest 
> thing to
> do. Since you're playing with 32-bit timer bugs atm, do you fancy 
> taking a
> look ;)

I came up with this, tested in a guest only by fudging the detection
code (I don't have the offending HW at hand). The use of branches vs
NOPs is debatable, no strong opinion here.

         M.

 From 81126933d6c990dac7213d0ec66c4a9df21fe8b8 Mon Sep 17 00:00:00 2001
 From: Marc Zyngier <maz@kernel.org>
Date: Wed, 1 Jul 2020 21:29:24 +0100
Subject: [PATCH] arm64: Rework ARM_ERRATUM_1414080 handling

The current handling of erratum 1414080 has the side effect that
cntkctl_el1 can get changed for both 32 and 64bit tasks.

This isn't a problem so far, but if we ever need to mitigate another
of these errata on the 64bit side, we'd better keep the messing with
cntkctl_el1 local to 32bit tasks.

For that, make sure that on entering the kernel from a 32bit tasks,
userspace access to cntvct gets enabled, and disabled returning to
userspace, while it never gets changed for 64bit tasks.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
  arch/arm64/kernel/entry.S | 44 +++++++++++++++++++++++++--------------
  1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 5304d193c79d..357bce62c31e 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -167,6 +167,19 @@ alternative_cb_end
  	stp	x28, x29, [sp, #16 * 14]

  	.if	\el == 0
+	.if	\regsize == 32
+	// If we come back from a 32bit task on a system affected by
+	// 1418040, let's reenable userspace access to the virtual counter.
+#ifdef CONFIG_ARM64_ERRATUM_1418040
+alternative_if_not ARM64_WORKAROUND_1418040
+	b	.L__entry_wa_1418040\@
+alternative_else_nop_endif
+	mrs	x0, cntkctl_el1
+	orr	x0, x0, #2	// ARCH_TIMER_USR_VCT_ACCESS_EN
+	msr	cntkctl_el1, x0
+.L__entry_wa_1418040\@:
+#endif
+	.endif
  	clear_gp_regs
  	mrs	x21, sp_el0
  	ldr_this_cpu	tsk, __entry_task, x20
@@ -318,7 +331,21 @@ alternative_else_nop_endif
  	ldr	x23, [sp, #S_SP]		// load return stack pointer
  	msr	sp_el0, x23
  	tst	x22, #PSR_MODE32_BIT		// native task?
-	b.eq	3f
+	b.eq	4f
+
+#ifdef CONFIG_ARM64_ERRATUM_1418040
+alternative_if_not ARM64_WORKAROUND_1418040
+	b	3f
+alternative_else_nop_endif
+	/*
+	 * if (x22.mode32 == 1)
+	 *     cntkctl_el1.el0vcten = 0
+	 */
+	mrs	x1, cntkctl_el1
+	bfi	x1, xzr, #1, #1	// ARCH_TIMER_USR_VCT_ACCESS_EN
+	msr	cntkctl_el1, x1
+3:
+#endif

  #ifdef CONFIG_ARM64_ERRATUM_845719
  alternative_if ARM64_WORKAROUND_845719
@@ -330,22 +357,7 @@ alternative_if ARM64_WORKAROUND_845719
  #endif
  alternative_else_nop_endif
  #endif
-3:
-#ifdef CONFIG_ARM64_ERRATUM_1418040
-alternative_if_not ARM64_WORKAROUND_1418040
-	b	4f
-alternative_else_nop_endif
-	/*
-	 * if (x22.mode32 == cntkctl_el1.el0vcten)
-	 *     cntkctl_el1.el0vcten = ~cntkctl_el1.el0vcten
-	 */
-	mrs	x1, cntkctl_el1
-	eon	x0, x1, x22, lsr #3
-	tbz	x0, #1, 4f
-	eor	x1, x1, #2	// ARCH_TIMER_USR_VCT_ACCESS_EN
-	msr	cntkctl_el1, x1
  4:
-#endif
  	scs_save tsk, x0

  	/* No kernel C function calls after this as user keys are set. */
-- 
2.27.0


-- 
Jazz is not dead. It just smells funny...

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