Re: [RFC PATCH v2 02/19] arm64: Use the physical counter when available for read_cycles

[Christoffer Dall <christoffer.dall@linaro.org>]

On Tue, Jul 25, 2017 at 10:43:08AM +0100, Will Deacon wrote:
> Hi Christoffer,
> 
> On Mon, Jul 17, 2017 at 04:27:01PM +0200, Christoffer Dall wrote:
> > Currently get_cycles() is hardwired to arch_counter_get_cntvct() on
> > arm64, but as we move to using the physical timer for the in-kernel
> > time-keeping, we need to make that more flexible.
> > 
> > First, we need to make sure the physical counter can be read on equal
> > terms to the virtual counter, which includes adding physical counter
> > read functions for timers that require errata.
> > 
> > Second, we need to make a choice between reading the physical vs virtual
> > counter, depending on which timer is used for time keeping in the kernel
> > otherwise.  We can do this using a static key to avoid a performance
> > penalty during runtime when reading the counter.
> > 
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  arch/arm64/include/asm/arch_timer.h  | 18 ++++++++++++------
> >  arch/arm64/include/asm/timex.h       |  2 +-
> >  drivers/clocksource/arm_arch_timer.c | 32 ++++++++++++++++++++++++++++++--
> >  3 files changed, 43 insertions(+), 9 deletions(-)
> 
> [...]
> 
> > @@ -886,10 +912,12 @@ static void __init arch_counter_register(unsigned type)
> >  
> >  	/* Register the CP15 based counter if we have one */
> >  	if (type & ARCH_TIMER_TYPE_CP15) {
> > -		if (arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI)
> > +		if (arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI) {
> >  			arch_timer_read_counter = arch_counter_get_cntvct;
> > -		else
> > +		} else {
> >  			arch_timer_read_counter = arch_counter_get_cntpct;
> > +			static_branch_enable(&arch_timer_phys_counter_available);
> > +		}
> 
> I'm a bit worried about this change, although I can't put my finger on
> exactly the problematic scenario. My concern is that if we have a system
> where the host kernel is entered at NS-EL1 (because, e.g. EL2 is used for
> something else or the bootloader just didn't load us there) then the booting
> protocol doesn't mandate a zero-initialised CNTVOFF value. If we can
> subsequently end up using the physical counter in the kernel and the virtual
> counter in userspace, the vDSO will get confused because the datapage values
> will not correspond to the values it actually ends up reading. 

Just so I'm sure I'm understanding correctly, vDSO always reads the
virtual counter?

> There's also
> the likelihood that existing EL2 init code simply isn't setting up
> CNTHCTL_EL2 and CNTVOFF correctly, so we probably need a way to force
> virtual counter on the cmdline.

My understanding was that we only ever use the physical counter when we
boot at EL2 and therefore the kernel is in control of CNTVOFF and can
set that to 0.  Is this not the case, or are you asking for a way to
mandate this relationship or make it abundantly clear?

Also, are you fine with arch_timer_read_counter changing to using the
physical counter on arm64, but you're merely worried about
read_cycles()?

Does it make sense to have read_cycles() still read the virtual counter
whereas arch timers (timers, not counters) use the physical counter?  I
somewhat convinced myself that this doesn't work though.

> 
> In practice it looks like we always end up with ARCH_TIMER_VIRT_PPI out of
> arch_timer_select_ppi, but that's not guaranteed and I haven't thought at
> all about the 32-bit case, which has other quirks/complexities.
> 

How does this change affect the 32-bit side?  All this should do is
enable a static branch which is unused on the 32-bit side; what am I
missing?

Thanks,
-Christoffer