[RFC PATCH 1/7] arm64: Use physical counter for in-kernel reads

[Christoffer Dall]

On Fri, Jan 06, 2017 at 10:38:49AM +0000, Marc Zyngier wrote:
> On 06/01/17 10:00, Christoffer Dall wrote:
> > Hi Marc,
> > 
> > On Thu, Jan 05, 2017 at 06:11:14PM +0000, Marc Zyngier wrote:
> >> [adding the arm64 maintainers, plus Mark as arch timer maintainer]
> > 
> > Right, sorry, I should have done that already.
> > 
> >>
> >> On 10/12/16 20:47, Christoffer Dall wrote:
> >>> Using the physical counter allows KVM to retain the offset between the
> >>> virtual and physical counter as long as it is actively running a VCPU.
> >>>
> >>> As soon as a VCPU is released, another thread is scheduled or we start
> >>> running userspace applications, we reset the offset to 0, so that VDSO
> >>> operations can still read the virtual counter and get the same view of
> >>> time as the kernel.
> >>>
> >>> This opens up potential improvements for KVM performance.
> >>>
> >>> VHE kernels or kernels using the virtual timer are unaffected by this.
> >>>
> >>> Signed-off-by: Christoffer Dall <christoffer.dall at linaro.org>
> >>> ---
> >>>  arch/arm64/include/asm/arch_timer.h  | 6 ++++--
> >>>  drivers/clocksource/arm_arch_timer.c | 2 +-
> >>>  2 files changed, 5 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> >>> index eaa5bbe..cec2549 100644
> >>> --- a/arch/arm64/include/asm/arch_timer.h
> >>> +++ b/arch/arm64/include/asm/arch_timer.h
> >>> @@ -139,11 +139,13 @@ static inline void arch_timer_set_cntkctl(u32 cntkctl)
> >>>  
> >>>  static inline u64 arch_counter_get_cntpct(void)
> >>>  {
> >>> +	u64 cval;
> >>>  	/*
> >>>  	 * AArch64 kernel and user space mandate the use of CNTVCT.
> >>>  	 */
> >>> -	BUG();
> >>> -	return 0;
> >>> +	isb();
> >>> +	asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
> >>> +	return cval;
> >>>  }
> >>>  
> >>>  static inline u64 arch_counter_get_cntvct(void)
> >>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> >>> index 73c487d..a5b0789 100644
> >>> --- a/drivers/clocksource/arm_arch_timer.c
> >>> +++ b/drivers/clocksource/arm_arch_timer.c
> >>> @@ -597,7 +597,7 @@ static void __init arch_counter_register(unsigned type)
> >>>  
> >>>  	/* Register the CP15 based counter if we have one */
> >>>  	if (type & ARCH_CP15_TIMER) {
> >>> -		if (IS_ENABLED(CONFIG_ARM64) || arch_timer_uses_ppi == VIRT_PPI)
> >>> +		if (arch_timer_uses_ppi == VIRT_PPI || is_kernel_in_hyp_mode())
> >>
> >> Why do we have this is_kernel_in_hyp_mode clause? I can't think of any
> >> reason for a VHE kernel to use the virtual counter at all...
> >>
> > 
> > Good question.  I think I just didn't want to change behavior from the
> > existing functionality mre than necessary.
> > 
> > Note that on a VHE kernel this will be the EL2 virtual counter, not the
> > EL1 virtual counter, due to the register redirection.  Are the virtual
> > and physical EL2 counters always equivalent on a VHE system?
> 
> Yes, they are. CNTVOFF_EL2 is ignored in that case, and you get an extra
> interrupt for the new EL2 virtual timer as well.
> 

ok, in that case I suppose I can just check for arch_timer_uses_ppi ==
VIRT_PPI and be done with it.

Thanks,
-Christoffer