On 30/08/17 09:20, Leo Yan wrote:

Hi Marc,

On Wed, Aug 30, 2017 at 09:02:47AM +0100, Marc Zyngier wrote:

quoted

+ Mark

On 30/08/17 06:06, Leo Yan wrote:

quoted

commit fa8d815fac96e7c9247783d5a1f8fa4685b3c543 upstream.

Cortex-A73 (all versions) counter read can return a wrong value
when the counter crosses a 32bit boundary.

The workaround involves performing the read twice, and to return
one or the other depending on whether a transition has taken place.

Signed-off-by: Marc Zyngier <redacted>
Signed-off-by: Leo Yan <redacted>
---
 arch/arm64/Kconfig                  | 12 ++++++++++++
 arch/arm64/include/asm/arch_timer.h | 11 +++++++++++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 14cdc6d..68e7c98 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig

@@ -374,6 +374,18 @@ config ARM64_ERRATUM_843419
 
 	  If unsure, say Y.
 
+config ARM64_ERRATUM_858921
+	bool "Cortex-A73: 858921: arch timer counter read can return a wrong value"
+	default y
+	depends on ARM_ARCH_TIMER && ARM64
+	help
+	  This option enables a workaround applicable to Cortex-A73
+	  (all versions), whose counter may return incorrect values.
+	  The workaround will be dynamically enabled when an affected
+	  core is detected.
+
+	  If unsure, say Y.
+
 config CAVIUM_ERRATUM_22375
 	bool "Cavium erratum 22375, 24313"
 	default y

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index fbe0ca3..9b2b0f5 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h

@@ -114,6 +114,16 @@ static inline u64 arch_counter_get_cntpct(void)
 	return 0;
 }
 
+#ifdef CONFIG_ARM64_ERRATUM_858921
+static inline u64 arch_counter_get_cntvct(void)
+{
+	u64 old, new;
+
+	asm volatile("mrs %0, cntvct_el0" : "=r" (old));
+	asm volatile("mrs %0, cntvct_el0" : "=r" (new));
+	return (((old ^ new) >> 32) & 1) ? old : new;
+}
+#else
 static inline u64 arch_counter_get_cntvct(void)
 {
 	u64 cval;

@@ -123,6 +133,7 @@ static inline u64 arch_counter_get_cntvct(void)
 
 	return cval;
 }
+#endif
 
 static inline int arch_timer_arch_init(void)
 {

This is completely busted:

- You are only addressing the kernel side, and ignore userspace (which
is just as broken as the kernel).

Thanks for quick reviewing.

Yeah, I remembered there have some code directly reads arch timer
virtual counter from userspace. But can you remind for userspace
broken issue, is the code in libc or vdso?

You're missing the point. The virtual counter is freely available to
userspace to play with (this is a de-facto ABI). Since it can return bad
values, it needs to be trapped to be correctly emulated (together with
cntfrq_el0), and the VDSO disabled.

Here I have another question is: after applied the whole workaround
infrastructure, can it also fix userspace broken issue?

Yup. That's why I ended-up with a 18 patches series, and not just this
single one. Trust me, I'm lazy. There is nothing I hate more than doing
useless work.

quoted

- You lie in the description of the option (this is in no way dynamic,
since you didn't backport the whole workaround infrastructure).

I should fix it.

quoted

So I'm afraid I'm NAKing this. Please refrain from blindly backporting
random patches. fa8d815fac96e7c9 only makes sense in the context of the
whole series, and on its own gives you a very false sense of having
properly addressed it.

IIUC, at least fa8d815fac96e7c9 can fix issue in kernel side, such like
for sched_clock() roll back issue [1].

What's the point of fixing the kernel if userspace is just as likely to
fail?

So for this issue, are you suggesting we need backport whole workaround
infrastructure onto kernel 4.4 and 4.9? Many ARM devices are working
with these two kernels.

Then these systems are completely broken if they use a Cortex-A73.
Either they run mainline (which will be just fine), or they get a fully
backported workaround infrastructure.

Thanks,

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