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

[Ben Horgan]

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 at 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?

> 
>> +				    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
> 
> 

Thanks,

Ben