Re: [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation

[Will Deacon]

Hi Vincenzo,

On Fri, Jun 21, 2019 at 10:52:31AM +0100, Vincenzo Frascino wrote:
> To take advantage of the commonly defined vdso interface for
> gettimeofday the architectural code requires an adaptation.
> 
> Re-implement the gettimeofday vdso in C in order to use lib/vdso.
> 
> With the new implementation arm64 gains support for CLOCK_BOOTTIME
> and CLOCK_TAI.
> 
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
> Tested-by: Shijith Thotton <sthotton@xxxxxxxxxxx>
> Tested-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  arch/arm64/Kconfig                         |   2 +
>  arch/arm64/include/asm/vdso/gettimeofday.h |  86 ++++++
>  arch/arm64/include/asm/vdso/vsyscall.h     |  53 ++++
>  arch/arm64/include/asm/vdso_datapage.h     |  48 ---
>  arch/arm64/kernel/asm-offsets.c            |  33 +-
>  arch/arm64/kernel/vdso.c                   |  51 +---
>  arch/arm64/kernel/vdso/Makefile            |  34 ++-
>  arch/arm64/kernel/vdso/gettimeofday.S      | 334 ---------------------
>  arch/arm64/kernel/vdso/vgettimeofday.c     |  28 ++

I'm concerned about an apparent semantic change introduced by your patch:

> +static __always_inline u64 __arch_get_hw_counter(s32 clock_mode)
> +{
> +	u64 res;
> +
> +	asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
> +
> +	return res;
> +}

vs:

> -	.macro	get_clock_shifted_nsec res, cycle_last, mult
> -	/* Read the virtual counter. */
> -	isb
> -	mrs	x_tmp, cntvct_el0
> -	/* Calculate cycle delta and convert to ns. */
> -	sub	\res, x_tmp, \cycle_last
> -	/* We can only guarantee 56 bits of precision. */
> -	movn	x_tmp, #0xff00, lsl #48
> -	and	\res, x_tmp, \res
> -	mul	\res, \res, \mult
> -	/*
> -	 * Fake address dependency from the value computed from the counter
> -	 * register to subsequent data page accesses so that the sequence
> -	 * locking also orders the read of the counter.
> -	 */
> -	and	x_tmp, \res, xzr
> -	add	vdso_data, vdso_data, x_tmp
> -	.endm

It looks like you're dropping both the preceding ISB (allowing the counter
value to be speculated) and also the subsequent dependency (allowing the
seq lock to be speculated). If I've missed them, apologies, but I couldn't
spot them elsewhere in this patch.

__arch_get_hw_counter should probably be identical to __arch_counter_get_cntvct
to avoid these problems. I guess we don't need to care about the case where
the counter is unstable, since we'll just disable the vDSO altogether on
such systems?

Will