Re: [PATCH RFC] arm64: vdso: Use __arch_counter_get_cntvct()

[Marc Zyngier <maz@kernel.org>]

On Wed, 02 Apr 2025 23:04:41 +0100,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Wed, 02 Apr 2025 20:22:47 +0100,
> Breno Leitao <leitao@debian.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@debian.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.