| Message ID | 20210330105719.47760-2-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/timer: trival fix for sync inside counter | expand |
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
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; }
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(-)