Re: [PATCH 2/3] clocksource/drivers/arm_arch_timer: Expose a direct accessor for the virtual counter

[Marc Zyngier <maz@kernel.org>]

On Thu, 26 Feb 2026 14:03:36 +0000,
Ben Horgan <ben.horgan@arm.com> wrote:
> 
> 
> 
> On 2/26/26 13:48, Ben Horgan wrote:
> > Hi Marc,
> > 
> > On 2/26/26 08:22, Marc Zyngier wrote:
> >> We allow access to the architected counter via arch_timer_read_counter().
> >> However, this accessor can either be the virtual or the physical
> >> view of the counter, depending on how the kernel has been booted.
> >>
> >> At the same time, we have some architectural features (such as WFIT,
> >> WFET) that rely on the virtual counter, and nothing else.
> >>
> >> If implementations were perfect, we'd rely on reading CNTVCT_EL0,
> >> and be done with it. However, we have a bunch of broken implementations
> >> in the wild, which rely on preemption being disabled and other
> >> costly workarounds.
> >>
> >> In order to provide decent performance on non-broken HW while still
> >> supporting the legacy horrors, expose arch_timer_read_vcounter() as
> >> a new helper that hides this complexity.
> >>
> >> Signed-off-by: Marc Zyngier <maz@kernel.org>
> >> ---
> >>  drivers/clocksource/arm_arch_timer.c | 5 +++++
> >>  include/clocksource/arm_arch_timer.h | 1 +
> >>  2 files changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> >> index 90aeff44a2764..4e4a62e1c9439 100644
> >> --- a/drivers/clocksource/arm_arch_timer.c
> >> +++ b/drivers/clocksource/arm_arch_timer.c
> >> @@ -137,6 +137,8 @@ static noinstr u64 arch_counter_get_cntvct(void)
> >>  u64 (*arch_timer_read_counter)(void) __ro_after_init = arch_counter_get_cntvct;
> >>  EXPORT_SYMBOL_GPL(arch_timer_read_counter);
> >>  
> >> +u64 (*arch_timer_read_vcounter)(void) __ro_after_init = arch_counter_get_cntvct;
> >> +
> >>  static u64 arch_counter_read(struct clocksource *cs)
> >>  {
> >>  	return arch_timer_read_counter();
> >> @@ -931,6 +933,9 @@ static void __init arch_counter_register(void)
> >>  	}
> >>  
> >>  	arch_timer_read_counter = rd;
> >> +	arch_timer_read_vcounter = (arch_timer_counter_has_wa() ?
> > 
> > This matches what is done for arch_timer_read_counter but it seems a bit
> > surprising to me that arch_timer_counter_has_wa() is checking that the
> > workaround is in use and not whether the workaround should be in use. Do
> > we need to worry about what happens if the workaround fails to be enabled?
> 
> Or is the point that if you haven't enabled a relevant workaround then
> all cores are treated the same and so there is no need to disable
> preemption?

There are multiple things at play here:

- we cannot fail to enable a workaround. If we find one, we enable it.

- if no workaround are available, then there is no need to disable
  preemption, because the read of the counter is the same on all CPUs.

However, this code is a bug nest, and I just re-discovered an
interesting failure mode (boot on a sane CPU, keeping the broken CPUs
offline, online a broken CPU late, enjoy the fireworks).

Plus the fact that we don't indirect sched_clock(), which means we
never really enable a workaround if the boot CPU is not affected.

I have a small pile of hacks to address all of this, but I need to
convince myself that this is actually correct.

Stay tuned...

	M.

-- 
Without deviation from the norm, progress is not possible.