[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87a58yz0cm.wl-maz@kernel.org>
Date: Wed, 02 Apr 2025 23:04:41 +0100
From: Marc Zyngier <maz@...nel.org>
To: Breno Leitao <leitao@...ian.org>
Cc: Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org,
arnd@...db.de,
kernel-team@...a.com,
vincenzo.frascino@....com,
anders.roxell@...aro.org
Subject: Re: [PATCH RFC] arm64: vdso: Use __arch_counter_get_cntvct()
On Wed, 02 Apr 2025 20:22:47 +0100,
Breno Leitao <leitao@...ian.org> wrote:
>
> While reading how `cntvct_el0` was read in the kernel, I found that
> __arch_get_hw_counter() is doing something very similar to what
> __arch_counter_get_cntvct() is already doing.
>
> Use the existing __arch_counter_get_cntvct() function instead of
> duplicating similar inline assembly code in __arch_get_hw_counter().
>
> Both functions were performing nearly identical operations to read the
> cntvct_el0 register. The only difference was that
> __arch_get_hw_counter() included a memory clobber in its inline
> assembly, which appears unnecessary in this context.
>
> This change simplifies the code by eliminating duplicate functionality
> and improves maintainability by centralizing the counter access logic in
> a single implementation.
>
> Signed-off-by: Breno Leitao <leitao@...ian.org>
> ---
> I'm sharing this code as an RFC since I'm not intimately familiar with
> different arm platforms, and I want to double-check that I haven't
> missed anything subtle.
> ---
> arch/arm64/include/asm/vdso/gettimeofday.h | 22 ++--------------------
> 1 file changed, 2 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h
> index 92a2b59a9f3df..417b5b41b877d 100644
> --- a/arch/arm64/include/asm/vdso/gettimeofday.h
> +++ b/arch/arm64/include/asm/vdso/gettimeofday.h
> @@ -11,6 +11,7 @@
> #include <asm/barrier.h>
> #include <asm/unistd.h>
> #include <asm/sysreg.h>
> +#include <asm/arch_timer.h>
>
> #define VDSO_HAS_CLOCK_GETRES 1
>
> @@ -69,8 +70,6 @@ int clock_getres_fallback(clockid_t _clkid, struct __kernel_timespec *_ts)
> static __always_inline u64 __arch_get_hw_counter(s32 clock_mode,
> const struct vdso_time_data *vd)
> {
> - u64 res;
> -
> /*
> * Core checks for mode already, so this raced against a concurrent
> * update. Return something. Core will do another round and then
> @@ -79,24 +78,7 @@ static __always_inline u64 __arch_get_hw_counter(s32 clock_mode,
> if (clock_mode == VDSO_CLOCKMODE_NONE)
> return 0;
>
> - /*
> - * If FEAT_ECV is available, use the self-synchronizing counter.
> - * Otherwise the isb is required to prevent that the counter value
> - * is speculated.
> - */
> - asm volatile(
> - ALTERNATIVE("isb\n"
> - "mrs %0, cntvct_el0",
> - "nop\n"
> - __mrs_s("%0", SYS_CNTVCTSS_EL0),
> - ARM64_HAS_ECV)
> - : "=r" (res)
> - :
> - : "memory");
> -
> - arch_counter_enforce_ordering(res);
> -
> - return res;
> + return __arch_counter_get_cntvct();
I won't pretend I understand it all, but you really want to have a
look at the link just above the arch_counter_enforce_ordering()
definition, pasted below for your convenience:
https://lore.kernel.org/r/alpine.DEB.2.21.1902081950260.1662@nanos.tec.linutronix.de/
Dropping this ordering enforcement seems pretty adventurous unless you
have very strong guarantees about the context this executes in.
M.
--
Jazz isn't dead. It just smells funny.
Powered by blists - more mailing lists