Re: [RESEND PATCH] arm64: arch_timer: Workaround for Cortex-A73 erratum 858921

[Leo Yan <leo.yan@linaro.org>]

Hi Marc,

On Wed, Aug 30, 2017 at 09:02:47AM +0100, Marc Zyngier wrote:
> + Mark
> 
> On 30/08/17 06:06, Leo Yan wrote:
> > commit fa8d815fac96e7c9247783d5a1f8fa4685b3c543 upstream.
> > 
> > Cortex-A73 (all versions) counter read can return a wrong value
> > when the counter crosses a 32bit boundary.
> > 
> > The workaround involves performing the read twice, and to return
> > one or the other depending on whether a transition has taken place.
> > 
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  arch/arm64/Kconfig                  | 12 ++++++++++++
> >  arch/arm64/include/asm/arch_timer.h | 11 +++++++++++
> >  2 files changed, 23 insertions(+)
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 14cdc6d..68e7c98 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -374,6 +374,18 @@ config ARM64_ERRATUM_843419
> >  
> >  	  If unsure, say Y.
> >  
> > +config ARM64_ERRATUM_858921
> > +	bool "Cortex-A73: 858921: arch timer counter read can return a wrong value"
> > +	default y
> > +	depends on ARM_ARCH_TIMER && ARM64
> > +	help
> > +	  This option enables a workaround applicable to Cortex-A73
> > +	  (all versions), whose counter may return incorrect values.
> > +	  The workaround will be dynamically enabled when an affected
> > +	  core is detected.
> > +
> > +	  If unsure, say Y.
> > +
> >  config CAVIUM_ERRATUM_22375
> >  	bool "Cavium erratum 22375, 24313"
> >  	default y
> > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> > index fbe0ca3..9b2b0f5 100644
> > --- a/arch/arm64/include/asm/arch_timer.h
> > +++ b/arch/arm64/include/asm/arch_timer.h
> > @@ -114,6 +114,16 @@ static inline u64 arch_counter_get_cntpct(void)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_ARM64_ERRATUM_858921
> > +static inline u64 arch_counter_get_cntvct(void)
> > +{
> > +	u64 old, new;
> > +
> > +	asm volatile("mrs %0, cntvct_el0" : "=r" (old));
> > +	asm volatile("mrs %0, cntvct_el0" : "=r" (new));
> > +	return (((old ^ new) >> 32) & 1) ? old : new;
> > +}
> > +#else
> >  static inline u64 arch_counter_get_cntvct(void)
> >  {
> >  	u64 cval;
> > @@ -123,6 +133,7 @@ static inline u64 arch_counter_get_cntvct(void)
> >  
> >  	return cval;
> >  }
> > +#endif
> >  
> >  static inline int arch_timer_arch_init(void)
> >  {
> > 
> 
> This is completely busted:
> 
> - You are only addressing the kernel side, and ignore userspace (which
> is just as broken as the kernel).

Thanks for quick reviewing.

Yeah, I remembered there have some code directly reads arch timer
virtual counter from userspace. But can you remind for userspace
broken issue, is the code in libc or vdso?

Here I have another question is: after applied the whole workaround
infrastructure, can it also fix userspace broken issue?

> - You lie in the description of the option (this is in no way dynamic,
> since you didn't backport the whole workaround infrastructure).

I should fix it.

> So I'm afraid I'm NAKing this. Please refrain from blindly backporting
> random patches. fa8d815fac96e7c9 only makes sense in the context of the
> whole series, and on its own gives you a very false sense of having
> properly addressed it.

IIUC, at least fa8d815fac96e7c9 can fix issue in kernel side, such like
for sched_clock() roll back issue [1].

So for this issue, are you suggesting we need backport whole workaround
infrastructure onto kernel 4.4 and 4.9? Many ARM devices are working
with these two kernels.

[1] https://lists.linaro.org/pipermail/eas-dev/2017-August/000941.html

Thanks,
Leo Yan