[PATCH 2/2] arm64/arch_timer: replace arch_counter_enforce_ordering() with isb

[Pingfan Liu <kernelfans@gmail.com>]

The description of getting counter value is not very clear. [1]
'mrs Xt, cntpct' may execute out of program order, either forward or
backward.

Now taking a look at this group of getting counter routines. All of them
are called from sched_clock(). And there is an isb to protect forward
speculation. But there is no isb for the backward speculation.

The current code enforces read dependency instructions anchored on
getting counter. But it is not enough to protect against other no
dependency instructions, and even function call can not prevent the
speculation between getting counter and them.

Replacing arch_counter_enforce_ordering() with isb to achieve the aim.

[1]: AArch64 Programmer's Guides Generic Timer:  3.1. Count and frequency

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andrei Vagin <avagin@gmail.com>
Cc: Marc Zyngier <maz@kernel.org>
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm64/include/asm/arch_timer.h | 34 ++++++++---------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 9f0ec21d6327..233ade46390f 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -165,32 +165,18 @@ static inline void arch_timer_set_cntkctl(u32 cntkctl)
 	isb();
 }
 
-/*
- * Ensure that reads of the counter are treated the same as memory reads
- * for the purposes of ordering by subsequent memory barriers.
- *
- * This insanity brought to you by speculative system register reads,
- * out-of-order memory accesses, sequence locks and Thomas Gleixner.
- *
- * http://lists.infradead.org/pipermail/linux-arm-kernel/2019-February/631195.html
- */
-#define arch_counter_enforce_ordering(val) do {				\
-	u64 tmp, _val = (val);						\
-									\
-	asm volatile(							\
-	"	eor	%0, %1, %1\n"					\
-	"	add	%0, sp, %0\n"					\
-	"	ldr	xzr, [%0]"					\
-	: "=r" (tmp) : "r" (_val));					\
-} while (0)
-
 static __always_inline u64 __arch_counter_get_cntpct_stable(void)
 {
 	u64 cnt;
 
 	isb();
 	cnt = arch_timer_reg_read_stable(cntpct_el0);
-	arch_counter_enforce_ordering(cnt);
+	/*
+	 * read dependency does not help against the sepculation between getting counter
+	 * and no dependency instructions, which cause the overwritten of register value.
+	 * So here needs isb.
+	 */
+	isb();
 	return cnt;
 }
 
@@ -200,7 +186,7 @@ static __always_inline u64 __arch_counter_get_cntpct(void)
 
 	isb();
 	cnt = read_sysreg(cntpct_el0);
-	arch_counter_enforce_ordering(cnt);
+	isb();
 	return cnt;
 }
 
@@ -210,7 +196,7 @@ static __always_inline u64 __arch_counter_get_cntvct_stable(void)
 
 	isb();
 	cnt = arch_timer_reg_read_stable(cntvct_el0);
-	arch_counter_enforce_ordering(cnt);
+	isb();
 	return cnt;
 }
 
@@ -220,12 +206,10 @@ static __always_inline u64 __arch_counter_get_cntvct(void)
 
 	isb();
 	cnt = read_sysreg(cntvct_el0);
-	arch_counter_enforce_ordering(cnt);
+	isb();
 	return cnt;
 }
 
-#undef arch_counter_enforce_ordering
-
 static inline int arch_timer_arch_init(void)
 {
 	return 0;
-- 
2.29.2


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