* [PATCH 0/2] arm64/timer: trival fix for sync inside counter
@ 2021-03-30 10:57 Pingfan Liu
2021-03-30 10:57 ` [PATCH 1/2] arm64/gettimeofday: correct the note about isb in __arch_get_hw_counter() Pingfan Liu
2021-03-30 10:57 ` [PATCH 2/2] arm64/arch_timer: replace arch_counter_enforce_ordering() with isb Pingfan Liu
0 siblings, 2 replies; 6+ messages in thread
From: Pingfan Liu @ 2021-03-30 10:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Pingfan Liu, Catalin Marinas, Will Deacon, Vincenzo Frascino,
Thomas Gleixner, Mark Rutland, Andrei Vagin, Marc Zyngier
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] arm64/gettimeofday: correct the note about isb in __arch_get_hw_counter() 2021-03-30 10:57 [PATCH 0/2] arm64/timer: trival fix for sync inside counter Pingfan Liu @ 2021-03-30 10:57 ` Pingfan Liu 2021-03-30 11:08 ` Will Deacon 2021-03-30 10:57 ` [PATCH 2/2] arm64/arch_timer: replace arch_counter_enforce_ordering() with isb Pingfan Liu 1 sibling, 1 reply; 6+ messages in thread From: Pingfan Liu @ 2021-03-30 10:57 UTC (permalink / raw) To: linux-arm-kernel Cc: Pingfan Liu, Catalin Marinas, Will Deacon, Vincenzo Frascino, Thomas Gleixner, Mark Rutland, Andrei Vagin, Marc Zyngier 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] arm64/gettimeofday: correct the note about isb in __arch_get_hw_counter() 2021-03-30 10:57 ` [PATCH 1/2] arm64/gettimeofday: correct the note about isb in __arch_get_hw_counter() Pingfan Liu @ 2021-03-30 11:08 ` Will Deacon 0 siblings, 0 replies; 6+ messages in thread From: Will Deacon @ 2021-03-30 11:08 UTC (permalink / raw) To: Pingfan Liu Cc: linux-arm-kernel, Catalin Marinas, Vincenzo Frascino, Thomas Gleixner, Mark Rutland, Andrei Vagin, Marc Zyngier 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] arm64/arch_timer: replace arch_counter_enforce_ordering() with isb 2021-03-30 10:57 [PATCH 0/2] arm64/timer: trival fix for sync inside counter Pingfan Liu 2021-03-30 10:57 ` [PATCH 1/2] arm64/gettimeofday: correct the note about isb in __arch_get_hw_counter() Pingfan Liu @ 2021-03-30 10:57 ` Pingfan Liu 2021-03-30 11:05 ` Will Deacon 1 sibling, 1 reply; 6+ messages in thread From: Pingfan Liu @ 2021-03-30 10:57 UTC (permalink / raw) To: linux-arm-kernel Cc: Pingfan Liu, Catalin Marinas, Will Deacon, Vincenzo Frascino, Thomas Gleixner, Mark Rutland, Andrei Vagin, Marc Zyngier 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] arm64/arch_timer: replace arch_counter_enforce_ordering() with isb 2021-03-30 10:57 ` [PATCH 2/2] arm64/arch_timer: replace arch_counter_enforce_ordering() with isb Pingfan Liu @ 2021-03-30 11:05 ` Will Deacon 2021-03-31 9:20 ` Pingfan Liu 0 siblings, 1 reply; 6+ messages in thread From: Will Deacon @ 2021-03-30 11:05 UTC (permalink / raw) To: Pingfan Liu Cc: linux-arm-kernel, Catalin Marinas, Vincenzo Frascino, Thomas Gleixner, Mark Rutland, Andrei Vagin, Marc Zyngier 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] arm64/arch_timer: replace arch_counter_enforce_ordering() with isb 2021-03-30 11:05 ` Will Deacon @ 2021-03-31 9:20 ` Pingfan Liu 0 siblings, 0 replies; 6+ messages in thread From: Pingfan Liu @ 2021-03-31 9:20 UTC (permalink / raw) To: Will Deacon Cc: linux-arm-kernel, Catalin Marinas, Vincenzo Frascino, Thomas Gleixner, Mark Rutland, Andrei Vagin, Marc Zyngier 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-31 9:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-30 10:57 [PATCH 0/2] arm64/timer: trival fix for sync inside counter Pingfan Liu 2021-03-30 10:57 ` [PATCH 1/2] arm64/gettimeofday: correct the note about isb in __arch_get_hw_counter() Pingfan Liu 2021-03-30 11:08 ` Will Deacon 2021-03-30 10:57 ` [PATCH 2/2] arm64/arch_timer: replace arch_counter_enforce_ordering() with isb Pingfan Liu 2021-03-30 11:05 ` Will Deacon 2021-03-31 9:20 ` Pingfan Liu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.