[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <878qoiyzic.wl-maz@kernel.org>
Date: Wed, 02 Apr 2025 23:22:51 +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 23:04:41 +0100,
Marc Zyngier <maz@...nel.org> wrote:
>
> 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.
Ah, I appear to have misread this patch, and
__arch_counter_get_cntvct() does have the same ordering requirements.
Apologies for the noise.
M.
--
Jazz isn't dead. It just smells funny.
Powered by blists - more mailing lists