Re: [PATCH 08/13] clocksource/arm_arch_timer: Work around broken CVAL implementations

[Marc Zyngier]

On Tue, 10 Aug 2021 13:34:07 +0100,
Mark Rutland <mark.rutland@xxxxxxx> wrote:
> 
> On Mon, Aug 09, 2021 at 04:26:46PM +0100, Marc Zyngier wrote:
> > The Applied Micro XGene-1 SoC has a busted implementation of the
> > CVAL register: it looks like it is based on TVAL instead of the
> > other way around. The net effect of this implementation blunder
> > is that the maximum deadline you can program in the timer is
> > 32bit wide.
> > 
> > Detect the problematic case and limit the timer to 32bit deltas.
> > Note that we don't tie this bug to XGene specifically, as it may
> > also catch similar defects on other high-quality implementations.
> 
> Do we know of any other implementations that have a similar bug?
> 
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> >  drivers/clocksource/arm_arch_timer.c | 38 +++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > index 895844c33351..1c596cd3cc5c 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -778,9 +778,42 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt,
> >  	return 0;
> >  }
> >  
> > +static u64 __arch_timer_check_delta(void)
> > +{
> > +#ifdef CONFIG_ARM64
> > +	u64 tmp;
> > +
> > +	/*
> > +	 * XGene-1 implements CVAL in terms of TVAL, meaning that the
> > +	 * maximum timer range is 32bit. Shame on them. Detect the
> > +	 * issue by setting a timer to now+(1<<32), which will
> > +	 * immediately fire on the duff CPU.
> > +	 */
> > +	write_sysreg(0, cntv_ctl_el0);
> > +	isb();
> > +	tmp = read_sysreg(cntvct_el0) | BIT(32);
> > +	write_sysreg(tmp, cntv_cval_el0);
> 
> This will fire on legitimate implementations fairly often. Consider if
> we enter this function at a time where CNTCVT_EL0[32] == 1, where:
> 
> * At 100MHz, bit 32 flips every ~42.95
> * At 200MHz, bit 32 flips every ~21.47
> * At 1GHz, bit 32 flips every ~4.29s
> 
> ... and ThunderX2 has a 200MHz frequency today, with SBSA recommending
> 100MHz.

Yup, you're right, this is silly. Orr-ing the bit is a bad enough bug
(it really should be a +), but also preemption in a guest will add
another set of false positives.

> What does XGene-1 return upon a read of CVAL? If it always returns 0 for
> the high bits, we could do a timing-insensitive check for truncation of
> CVAL, e.g.
> 
> | 	/* CVAL must be at least 56 bits wide, as with CNT */
> | 	u64 mask = GENMASK(55, 0);
> | 	u64 val;
> | 
> | 	write_sysreg(mask, cntv_cval_el0);
> | 	val = read_sysread(cnt_cval_el0);
> | 
> | 	if (val != mask) {
> | 		/* What a great CPU */
> | 	}

No, the register itself returns what has been written. But only the
low 32bits of the delta trickle into TVAL on write, which is then used
as a countdown. I guess I could play the same trick as above with a
higher bit, but it still is pretty unreliable, as it could then wrap
through 0.

Maybe I'll just check the MIDR in the end...

	M.

-- 
Without deviation from the norm, progress is not possible.