[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 at gmail.com>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> Cc: Will Deacon <will at kernel.org>
> Cc: Vincenzo Frascino <vincenzo.frascino at arm.com>
> Cc: Thomas Gleixner <tglx at linutronix.de>
> Cc: Mark Rutland <mark.rutland at arm.com>
> Cc: Andrei Vagin <avagin at gmail.com>
> Cc: Marc Zyngier <maz at kernel.org>
> To: linux-arm-kernel at 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