Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Will Deacon <will@kernel.org>
To: Pingfan Liu <kernelfans@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Mark Rutland <mark.rutland@arm.com>,
	Andrei Vagin <avagin@gmail.com>, Marc Zyngier <maz@kernel.org>
Subject: Re: [PATCH 1/2] arm64/gettimeofday: correct the note about isb in __arch_get_hw_counter()
Date: Tue, 30 Mar 2021 12:08:27 +0100	[thread overview]
Message-ID: <20210330110826.GB5707@willie-the-truck> (raw)
In-Reply-To: <20210330105719.47760-2-kernelfans@gmail.com>

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

  reply	other threads:[~2021-03-30 11:10 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210330110826.GB5707@willie-the-truck \
    --to=will@kernel.org \
    --cc=avagin@gmail.com \
    --cc=catalin.marinas@arm.com \
    --cc=kernelfans@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=vincenzo.frascino@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox