[PATCHv5 15/16] ARM: hyp: initialize CNTVOFF to zero

[Dave Martin]

On Fri, Feb 01, 2013 at 11:46:41AM +0000, Mark Rutland wrote:
> On Fri, Feb 01, 2013 at 11:13:50AM +0000, Dave Martin wrote:
> > On Thu, Jan 31, 2013 at 12:15:38PM +0000, Mark Rutland wrote:
> > > From: Marc Zyngier <Marc.Zyngier at arm.com>
> > > 
> > > In order to be able to use the virtual counter in a safe way,
> > > make sure it is initialized to zero before dropping to SVC.
> > > 
> > > Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
> > > Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> > > Cc: Dave Martin <dave.martin at arm.com>
> > > ---
> > >  arch/arm/kernel/hyp-stub.S | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
> > > index 65b2417..455603a 100644
> > > --- a/arch/arm/kernel/hyp-stub.S
> > > +++ b/arch/arm/kernel/hyp-stub.S
> > > @@ -152,6 +152,9 @@ THUMB(	orr	r7, #(1 << 30)	)	@ HSCTLR.TE
> > >  	mrc	p15, 4, r7, c14, c1, 0	@ CNTHCTL
> > >  	orr	r7, r7, #3		@ PL1PCEN | PL1PCTEN
> > >  	mcr	p15, 4, r7, c14, c1, 0	@ CNTHCTL
> > > +	mov	r6, #0
> > > +	mov	r7, #0
> > > +	mcrr	p15, 4, r6, r7, c14	@ CNTVOFF
> > 
> > Is this required for safety, or is it more a sanity feature?
> 
> This makes more sense with the next patch, which makes the arch_timer
> driver always use the virtual counters (to avoid indirection in the fast
> path and messy races with the setup of function pointers otherwise).
> 
> It's required for safety when hyp mode is enabled, and the arch_timer
> driver uses the physical timers in combination with the virtual
> counters. Either the driver has to apply CNTVOFF manually when setting
> the physical timers, or the physical timers and virtual counters need
> the same view of time (i.e. CNTVOFF == 0).
> 
> It also brings us in line with arm64, which always uses the virtual
> counter for its vDSO.

OK.  This definitely sounds like the correct model.

> 
> > 
> > The architected timer counters are supposed to be monotonic time sources
> > only, so applying a random offset shouldn't really change anything.
> 
> This is true except when we want to use the physical timers as described above.
> 
> > 
> > The main thing I can think of is that it is easier for the host to
> > manage guests' virtual counter offsets if the host's offset is 0 (and
> > we don't really want to be changing the host offset after the host kernel
> > boots).
> 
> That's pretty much it. We don't want to have to further separate the handling
> of the timer for host and guest. By having CNTVOFF as zero for the host, we
> don't need to duplicate reading of the timers and/or incur an additional
> overhead on reading them.

That all sounds sensible.  FWIW:

Reviewed-by: Dave Martin <dave.martin at linaro.org>

Cheers
---Dave