Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled

[Marc Zyngier]

On 24/04/17 08:59, Hanjun Guo wrote:
> Hi Marc,
> 
> On 2017/4/5 1:18, Marc Zyngier wrote:
>> Userspace being allowed to use read CNTVCT_EL0 anytime (and not
>> only in the VDSO), we need to enable trapping whenever a cntvct
>> workaround is enabled on a given CPU.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 45 +++++++++++++++++++++++++-----------
>>  1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> [...]
>>  #else
>>  #define arch_timer_check_ool_workaround(t,a)		do { } while(0)
>>  #define erratum_set_next_event_tval_virt(...)		({BUG(); 0;})
>>  #define erratum_set_next_event_tval_phys(...)		({BUG(); 0;})
>>  #define erratum_handler(fn, r, ...)			({false;})
>> +#define arch_timer_this_cpu_has_cntvct_wa()		({false;})
>>  #endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */
>>  
>>  static __always_inline irqreturn_t timer_handler(const int access,
>> @@ -660,15 +680,23 @@ static void arch_counter_set_user_access(void)
>>  {
>>  	u32 cntkctl = arch_timer_get_cntkctl();
>>  
>> -	/* Disable user access to the timers and the physical counter */
>> +	/* Disable user access to the timers and both counters */
>>  	/* Also disable virtual event stream */
>>  	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
>>  			| ARCH_TIMER_USR_VT_ACCESS_EN
>> +		        | ARCH_TIMER_USR_VCT_ACCESS_EN
>>  			| ARCH_TIMER_VIRT_EVT_EN
>>  			| ARCH_TIMER_USR_PCT_ACCESS_EN);
>>  
>> -	/* Enable user access to the virtual counter */
>> -	cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
>> +	/*
>> +	 * Enable user access to the virtual counter if it doesn't
>> +	 * need to be workaround. The vdso may have been already
>> +	 * disabled though.
>> +	 */
>> +	if (arch_timer_this_cpu_has_cntvct_wa())
>> +		pr_info("CPU%d: Trapping CNTVCT access\n", smp_processor_id());
>> +	else
>> +		cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
> 
> Since CNTVCT_EL0 and CNTFRQ_EL0 share the same control register,then we
> need to trap CNTFRQ_EL0 as well.
> 
> We hit an "undefined instruction" when running Open MPI, which
> read CNTFRQ_EL0 in the user space:
> 
> https://github.com/open-mpi/ompi/blob/5b40fd267f9ddc6463051e402382a988637c3bb3/opal/include/opal/sys/arm64/timer.h
> 
> static inline opal_timer_t
> opal_sys_timer_freq(void) { opal_timer_t freq; __asm__ __volatile__
> ("mrs %0, CNTFRQ_EL0" : "=r" (freq)); return (opal_timer_t)(freq); }
> Please take look :) Thanks Hanjun

Ah, nice :-/ ... Can you please give the patch below a shot?

Thanks,

	M.