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

[Ben Horgan <ben.horgan@arm.com>]

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?

> +				    arch_counter_get_cntvct_stable :
> +				    arch_counter_get_cntvct);
>  	clocksource_counter.vdso_clock_mode = vdso_default;
>  
>  	width = arch_counter_get_width();
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 2eda895f19f54..5bfd6a5db75aa 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -88,6 +88,7 @@ struct arch_timer_mem {
>  
>  extern u32 arch_timer_get_rate(void);
>  extern u64 (*arch_timer_read_counter)(void);
> +extern u64 (*arch_timer_read_vcounter)(void);
>  extern struct arch_timer_kvm_info *arch_timer_get_kvm_info(void);
>  extern bool arch_timer_evtstrm_available(void);
>  

Thanks,

Ben