[PATCH 0/2] arm64/timer: trival fix for sync inside counter

[PATCH 0/2] arm64/timer: trival fix for sync inside counter

[Pingfan Liu]

These two patches are indpendent, but related so pack them into a
series.

Pingfan Liu (2):
  arm64/gettimeofday: correct the note about isb in
    __arch_get_hw_counter()
  arm64/arch_timer: replace arch_counter_enforce_ordering() with isb

 arch/arm64/include/asm/arch_timer.h           | 34 +++++--------------
 .../include/asm/vdso/compat_gettimeofday.h    |  9 +++--
 arch/arm64/include/asm/vdso/gettimeofday.h    |  9 +++--
 3 files changed, 21 insertions(+), 31 deletions(-)

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

-- 
2.29.2


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

[PATCH 1/2] arm64/gettimeofday: correct the note about isb in __arch_get_hw_counter()

[Pingfan Liu]

The seq lock in vdso_read_retry() is behind smp_rmb(), and can not be
speculated before the counter value due to the read dependency. Hence
the original note is misleading.

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

Hence this isb is still required to protect against backward speculation.
Correct the note around the code to show the motivation.

[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/vdso/compat_gettimeofday.h | 9 ++++++---
 arch/arm64/include/asm/vdso/gettimeofday.h        | 9 ++++++---
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/vdso/compat_gettimeofday.h b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
index 7508b0ac1d21..b5dfda25a5dc 100644
--- a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
+++ b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
@@ -123,10 +123,13 @@ static __always_inline u64 __arch_get_hw_counter(s32 clock_mode,
 	isb();
 	asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (res));
 	/*
-	 * This isb() is required to prevent that the seq lock is
-	 * speculated.
+	 * Getting count value may execute out of program order, either forward
+	 * or backward. Although the caller has a read dependency on @res, but
+	 * it can not protect backward speculation against no dependency
+	 * instruction. Beside this purpose, this isb also severs as a
+	 * compiler barrier for this __always_inline function.
 	 */
-	isb();
+	 isb();
 
 	return res;
 }
diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h
index 631ab1281633..6988a730b878 100644
--- a/arch/arm64/include/asm/vdso/gettimeofday.h
+++ b/arch/arm64/include/asm/vdso/gettimeofday.h
@@ -84,10 +84,13 @@ static __always_inline u64 __arch_get_hw_counter(s32 clock_mode,
 	isb();
 	asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
 	/*
-	 * This isb() is required to prevent that the seq lock is
-	 * speculated.#
+	 * Getting count value may execute out of program order, either forward
+	 * or backward. Although the caller has a read dependency on @res, but
+	 * it can not protect backward speculation against no dependency
+	 * instruction. Beside this purpose, this isb also severs as a
+	 * compiler barrier for this __always_inline function.
 	 */
-	isb();
+	 isb();
 
 	return res;
 }
-- 
2.29.2


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

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

[Pingfan Liu]

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

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

[Will Deacon]

On Tue, Mar 30, 2021 at 06:57:19PM +0800, Pingfan Liu wrote:
> 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.

Which "no dependency instructions"?

> 
> Replacing arch_counter_enforce_ordering() with isb to achieve the aim.
> 
> [1]: AArch64 Programmer's Guides Generic Timer:  3.1. Count and frequency

I wouldn't trust that guide as far as I can throw it.

Please describe the problem you're trying to solve, and hopefully I can
help. ISB is an expensive instruction so we need a good justification to
add it here (i.e. an example of why the current scheme is not correct).

Thanks,

Will

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

Re: [PATCH 1/2] arm64/gettimeofday: correct the note about isb in __arch_get_hw_counter()

[Will Deacon]

On Tue, Mar 30, 2021 at 06:57:18PM +0800, Pingfan Liu wrote:
> The seq lock in vdso_read_retry() is behind smp_rmb(), and can not be
> speculated before the counter value due to the read dependency. Hence
> the original note is misleading.
> 
> The description of getting counter value is not very clear. [1]
> 'mrs Xt, cntpct' may execute out of program order, either forward or
> backward.
> 
> Hence this isb is still required to protect against backward speculation.
> Correct the note around the code to show the motivation.
> 
> [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/vdso/compat_gettimeofday.h | 9 ++++++---
>  arch/arm64/include/asm/vdso/gettimeofday.h        | 9 ++++++---
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/vdso/compat_gettimeofday.h b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
> index 7508b0ac1d21..b5dfda25a5dc 100644
> --- a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
> +++ b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
> @@ -123,10 +123,13 @@ static __always_inline u64 __arch_get_hw_counter(s32 clock_mode,
>  	isb();
>  	asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (res));
>  	/*
> -	 * This isb() is required to prevent that the seq lock is
> -	 * speculated.
> +	 * Getting count value may execute out of program order, either forward
> +	 * or backward. Although the caller has a read dependency on @res, but
> +	 * it can not protect backward speculation against no dependency
> +	 * instruction. Beside this purpose, this isb also severs as a
> +	 * compiler barrier for this __always_inline function.

I agree that the existing comment is pretty rubbish, but I don't think this
is really much better.

> diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h
> index 631ab1281633..6988a730b878 100644
> --- a/arch/arm64/include/asm/vdso/gettimeofday.h
> +++ b/arch/arm64/include/asm/vdso/gettimeofday.h
> @@ -84,10 +84,13 @@ static __always_inline u64 __arch_get_hw_counter(s32 clock_mode,
>  	isb();
>  	asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
>  	/*
> -	 * This isb() is required to prevent that the seq lock is
> -	 * speculated.#
> +	 * Getting count value may execute out of program order, either forward
> +	 * or backward. Although the caller has a read dependency on @res, but
> +	 * it can not protect backward speculation against no dependency
> +	 * instruction. Beside this purpose, this isb also severs as a
> +	 * compiler barrier for this __always_inline function.
>  	 */
> -	isb();
> +	 isb();

This ISB doesn't exist in linux-next (I've changed it to use the dependency
trick, which you seem to have doubts about).

Will

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

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

[Pingfan Liu]

On Tue, Mar 30, 2021 at 12:05:53PM +0100, Will Deacon wrote:
Hi Will,

Thank you for kindly review.

I have several questions haunting on this issue. Hope further help from you.

> On Tue, Mar 30, 2021 at 06:57:19PM +0800, Pingfan Liu wrote:
> > 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.
> 
> Which "no dependency instructions"?
> 
In fact, I have two questions here. 

-1. let me start with the backward speculation.
No such instructions inside this function, but combining the caller and callee,
it will look like:
00000000000002e0 <sched_clock>:
...
 318:   f9400e60        ldr     x0, [x19, #24]
 31c:   d63f0000        blr     x0
					0000000000000030 <arch_counter_get_cntpct>:
					      30:       d503233f        paciasp
					      34:       d5033fdf        isb
					      38:       d53be020        mrs     x0, cntpct_el0
					      3c:       ca000001        eor     x1, x0, x0
					      40:       8b2163e1        add     x1, sp, x1
					      44:       f940003f        ldr     xzr, [x1]
					      48:       d50323bf        autiasp
					      4c:       d65f03c0        ret
		
 320:   29441261        ldp     w1, w4, [x19, #32]
...
 33c:   cb060000        sub     x0, x0, x6

There is no 'instruction sync semantics' applied on 'ret', so
arch_counter_get_cntpct() may return to 320 before its instrutions are finished
in pipeline, and then new instructions are fetched into pipeline.

In theory (no idea about better material than [1]), any instruction touching
x0, but have no data dependency anchored on getting cntpct will take the risk
of speculation with getting cntpct. Can this happen?  (I admit there is no such
instruction in disasemble, but does it vary due to compiler?)

Making an analogy to isb at the entry. If this speculation is not possible, could
it be eliminated?


-2. Is arch_counter_enforce_ordering() needed?
Since the return value of arch_counter_get_cntpct() is always read, and
naturally, this read op will observe the read-barrier.
Here this instruction is "33c:   cb060000        sub     x0, x0, x6"


> > 
> > Replacing arch_counter_enforce_ordering() with isb to achieve the aim.
> > 
> > [1]: AArch64 Programmer's Guides Generic Timer:  3.1. Count and frequency
> 
> I wouldn't trust that guide as far as I can throw it.
> 
> Please describe the problem you're trying to solve, and hopefully I can
> help. ISB is an expensive instruction so we need a good justification to
> add it here (i.e. an example of why the current scheme is not correct).
> 
Yes, ISB is expensive, and original, I consider whether it can be eliminated
totally, but finally run to this reverse conclusion.

On the contrary, if this kind of speculation is not real, plus the fact that
there is already a natural read on getting cntvct. Can the second isb in
__arch_get_hw_counter() just be replaced with compiler barrier,


Thanks,

Pingfan

---
In case of reference

00000000000002e0 <sched_clock>:
 2e0:   d503233f        paciasp
 2e4:   a9bc7bfd        stp     x29, x30, [sp, #-64]!
 2e8:   910003fd        mov     x29, sp
 2ec:   a90153f3        stp     x19, x20, [sp, #16]
 2f0:   90000014        adrp    x20, 0 <jiffy_sched_clock_read>
 2f4:   91000294        add     x20, x20, #0x0
 2f8:   a90363f7        stp     x23, x24, [sp, #48]
 2fc:   91002297        add     x23, x20, #0x8
 300:   52800518        mov     w24, #0x28                      // #40
 304:   a9025bf5        stp     x21, x22, [sp, #32]
 308:   b9400296        ldr     w22, [x20]
 30c:   120002d5        and     w21, w22, #0x1
 310:   9bb87eb5        umull   x21, w21, w24
 314:   8b1502f3        add     x19, x23, x21
 318:   f9400e60        ldr     x0, [x19, #24]
 31c:   d63f0000        blr     x0
 320:   29441261        ldp     w1, w4, [x19, #32]
 324:   f8756ae3        ldr     x3, [x23, x21]
 328:   a9409666        ldp     x6, x5, [x19, #8]
 32c:   d50339bf        dmb     ishld
 330:   b9400282        ldr     w2, [x20]
 334:   6b16005f        cmp     w2, w22
 338:   54fffe81        b.ne    308 <sched_clock+0x28>  // b.any
 33c:   cb060000        sub     x0, x0, x6
 340:   2a0103e1        mov     w1, w1
 344:   8a050000        and     x0, x0, x5
 348:   a94153f3        ldp     x19, x20, [sp, #16]
 34c:   9b017c00        mul     x0, x0, x1
 350:   a9425bf5        ldp     x21, x22, [sp, #32]
 354:   9ac42400        lsr     x0, x0, x4
 358:   8b030000        add     x0, x0, x3
 35c:   a94363f7        ldp     x23, x24, [sp, #48]
 360:   a8c47bfd        ldp     x29, x30, [sp], #64
 364:   d50323bf        autiasp
 368:   d65f03c0        ret


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