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

[Dave Martin]

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>

[...]

> diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h
> new file mode 100644
> index 000000000000..bc3cb6738051
> --- /dev/null
> +++ b/arch/arm64/include/asm/vdso/gettimeofday.h
> @@ -0,0 +1,86 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 ARM Limited
> + */
> +#ifndef __ASM_VDSO_GETTIMEOFDAY_H
> +#define __ASM_VDSO_GETTIMEOFDAY_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <asm/unistd.h>
> +#include <uapi/linux/time.h>
> +
> +#define VDSO_HAS_CLOCK_GETRES		1
> +
> +static __always_inline int gettimeofday_fallback(
> +					struct __kernel_old_timeval *_tv,
> +					struct timezone *_tz)

Out of interest, does this need to be __always_inline?

> +{
> +	register struct timezone *tz asm("x1") = _tz;
> +	register struct __kernel_old_timeval *tv asm("x0") = _tv;
> +	register long ret asm ("x0");
> +	register long nr asm("x8") = __NR_gettimeofday;
> +
> +	asm volatile(
> +	"       svc #0\n"

Can inlining of this function result in non-trivial expressions being
substituted for _tz or _tv?

A function call can clobber register asm vars that are assigned to the
caller-save registers or that the PCS uses for function arguments, and
the situations where this can happen are poorly defined AFAICT.  There's
also no reliable way to detect at build time whether the compiler has
done this, and no robust way to stop if happening.

(IMHO the compiler is wrong to do this, but it's been that way for ever,
and I think I saw GCC 9 show this behaviour recently when I was
investigating something related.)


To be safe, it's better to put this out of line, or remove the reg asm()
specifiers, mark x0-x18 and lr as clobbered here (so that the compiler
doesn't map arguments to them), and put movs in the asm to move things
into the right registers.  The syscall number can be passed with an "i"
constraint.  (And yes, this sucks.)

If the code this is inlined in is simple enough though, we can be fairly
confident of getting away with it.

[...]

Cheers
---Dave