Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Martin <Dave.Martin@arm.com>
To: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: linux-arch@vger.kernel.org,
	Shijith Thotton <sthotton@marvell.com>,
	Peter Collingbourne <pcc@google.com>,
	Arnd Bergmann <arnd@arndb.de>, Huw Davies <huw@codeweavers.com>,
	Andre Przywara <andre.przywara@arm.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Will Deacon <will.deacon@arm.com>,
	linux-kernel@vger.kernel.org, Ralf Baechle <ralf@linux-mips.org>,
	linux-mips@vger.kernel.org, Paul Burton <paul.burton@mips.com>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	linux-kselftest@vger.kernel.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Russell King <linux@armlinux.org.uk>,
	Dmitry Safonov <0x7f454c46@gmail.com>,
	Mark Salyzyn <salyzyn@android.com>, Shuah Khan <shuah@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation
Date: Wed, 26 Jun 2019 17:14:16 +0100
Message-ID: <20190626161413.GA2790@e103592.cambridge.arm.com> (raw)
In-Reply-To: <f5ac379a-731d-0662-2f5b-bd046e3bd1c5@arm.com>

On Wed, Jun 26, 2019 at 02:27:59PM +0100, Vincenzo Frascino wrote:
> Hi Dave,
> 
> On 25/06/2019 16:33, Dave Martin wrote:
> > 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@arm.com>
> >> Cc: Will Deacon <will.deacon@arm.com>
> >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> >> Tested-by: Shijith Thotton <sthotton@marvell.com>
> >> Tested-by: Andre Przywara <andre.przywara@arm.com>
> > 
> > [...]
> > 
> >> 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?
> > 
> 
> It is a design choice. Philosophically, I prefer to control and reduce the scope
> of the decisions the compiler has to make in order to not have surprises.
> 
> >> +{
> >> +	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.
> >
> 
> I took very seriously what you are mentioning here because I think
> that robustness of the code comes before than everything especially
> in the kernel and I carried on some experiments to try to verify if
> in this case is safe to assume that the compiler is doing the right
> thing.
> 
> Based on my investigation and on previous observations of the
> generation of the vDSO library, I can conclude that the approach
> seems safe due to the fact that the usage of this code is very
> limited, the code itself is simple enough and that gcc would inline
> this code anyway based on the current compilation options.

I'd caution about "seems safe".  A lot of subtly wrong code not only
seems safe, but _is_ safe in its original context, in practice.  Add
some code to the vdso over time though, or tweak the compilation options
at some point in the future, or use a different compiler, and things
could still go wrong.

(Further comments below.)

> The experiment that I did was to define some self-contained code that
> tries to mimic what you are describing and compile it with 3
> different versions of gcc (6.4, 8.1 and 8.3) and in all the tree
> cases the behavior seems correct.
> 
> Code:
> =====
> 
> typedef int ssize_t;
> typedef int size_t;
> 
> static int my_strlen(const char *s)
> {
> 	int i = 0;
> 
> 	while (s[i] == '\0')
> 		i++;
> 
> 	return i;
> }
> 
> static inline ssize_t my_syscall(int fd, const void *buf, size_t count)
> {
> 	register ssize_t arg1 asm ("x0") = fd;
> 	register const void *arg2 asm ("x1") = buf;
> 	register size_t arg3 asm ("x2") = count;
> 
> 	__asm__ volatile (
> 		"mov x8, #64\n"
> 		"svc #0\n"
> 		: "=&r" (arg1)
> 		: "r" (arg2), "r" (arg3)
> 		: "x8"
>         );
> 
>         return arg1;
> }
> 
> void sys_caller(const char *s)
> {
> 	my_syscall(1, s, my_strlen(s));
> }
> 
> 
> GCC 8.3.0:
> ==========
> 
> main.8.3.0.o:     file format elf64-littleaarch64
> 
> 
> Disassembly of section .text:
> 
> 0000000000000000 <sys_caller>:
>    0:	39400001 	ldrb	w1, [x0]
>    4:	35000161 	cbnz	w1, 30 <sys_caller+0x30>
>    8:	d2800023 	mov	x3, #0x1                   	// #1
>    c:	d1000404 	sub	x4, x0, #0x1
>   10:	2a0303e2 	mov	w2, w3
>   14:	91000463 	add	x3, x3, #0x1
>   18:	38636881 	ldrb	w1, [x4, x3]
>   1c:	34ffffa1 	cbz	w1, 10 <sys_caller+0x10>
>   20:	aa0003e1 	mov	x1, x0
>   24:	d2800808 	mov	x8, #0x40                  	// #64
>   28:	d4000001 	svc	#0x0
>   2c:	d65f03c0 	ret
>   30:	52800002 	mov	w2, #0x0                   	// #0
>   34:	17fffffb 	b	20 <sys_caller+0x20>
> 
> 
> GCC 8.1.0:
> ==========
> 
> main.8.1.0.o:     file format elf64-littleaarch64
> 
> 
> Disassembly of section .text:
> 
> 0000000000000000 <sys_caller>:
>    0:	39400001 	ldrb	w1, [x0]
>    4:	35000161 	cbnz	w1, 30 <sys_caller+0x30>
>    8:	d2800023 	mov	x3, #0x1                   	// #1
>    c:	d1000404 	sub	x4, x0, #0x1
>   10:	2a0303e2 	mov	w2, w3
>   14:	91000463 	add	x3, x3, #0x1
>   18:	38636881 	ldrb	w1, [x4, x3]
>   1c:	34ffffa1 	cbz	w1, 10 <sys_caller+0x10>
>   20:	aa0003e1 	mov	x1, x0
>   24:	d2800808 	mov	x8, #0x40                  	// #64
>   28:	d4000001 	svc	#0x0
>   2c:	d65f03c0 	ret
>   30:	52800002 	mov	w2, #0x0                   	// #0
>   34:	17fffffb 	b	20 <sys_caller+0x20>
> 
> 
> 
> GCC 6.4.0:
> ==========
> 
> main.6.4.0.o:     file format elf64-littleaarch64
> 
> 
> Disassembly of section .text:
> 
> 0000000000000000 <sys_caller>:
>    0:	39400001 	ldrb	w1, [x0]
>    4:	35000161 	cbnz	w1, 30 <sys_caller+0x30>
>    8:	d2800023 	mov	x3, #0x1                   	// #1
>    c:	d1000404 	sub	x4, x0, #0x1
>   10:	2a0303e2 	mov	w2, w3
>   14:	91000463 	add	x3, x3, #0x1
>   18:	38636881 	ldrb	w1, [x4, x3]
>   1c:	34ffffa1 	cbz	w1, 10 <sys_caller+0x10>
>   20:	aa0003e1 	mov	x1, x0
>   24:	d2800808 	mov	x8, #0x40                  	// #64
>   28:	d4000001 	svc	#0x0
>   2c:	d65f03c0 	ret
>   30:	52800002 	mov	w2, #0x0                   	// #0
>   34:	17fffffb 	b	20 <sys_caller+0x20>

Thanks for having a go at this.  If the compiler can show the
problematic behaviour, it looks like your could could probably trigger
it, and as you observe, it doesn't trigger.

I am sure I have seen it in the past, but today I am struggling
to tickle the compiler in the right way.  My original reproducer may
have involved LTO, but either way I don't still have it :(


The classic example of this (triggered directly and not due to inlining)
would be something like:

int bar(int, int);

void foo(int x, int y)
{
	register int x_ asm("r0") = x;
	register int y_ asm("r1") = bar(x, y);

	asm volatile (
		"svc	#0"
		:: "r" (x_), "r" (y_)
		: "memory"
	);
}

->

0000000000000000 <foo>:
   0:   a9bf7bfd        stp     x29, x30, [sp, #-16]!
   4:   910003fd        mov     x29, sp
   8:   94000000        bl      0 <bar>
   c:   2a0003e1        mov     w1, w0
  10:   d4000001        svc     #0x0
  14:   a8c17bfd        ldp     x29, x30, [sp], #16
  18:   d65f03c0        ret


The gcc documentation is vague and ambiguous about precisely whan this
can happen and about how to avoid it.

The case where this behaviour is triggered by inlining an expression
that involves a (possibly implicit) function call seems hard to
reproduce.


However, the workaround is cheap, and to avoid the chance of subtle
intermittent code gen bugs it may be worth it:

void foo(int x, int y)
{
	asm volatile (
		"mov	x0, %0\n\t"
		"mov	x1, %1\n\t"
		"svc	#0"
		:: "r" (x), "r" (bar(x, y))
		: "r0", "r1", "memory"
	);
}

->

0000000000000000 <foo>:
   0:   a9be7bfd        stp     x29, x30, [sp, #-32]!
   4:   910003fd        mov     x29, sp
   8:   f9000bf3        str     x19, [sp, #16]
   c:   2a0003f3        mov     w19, w0
  10:   94000000        bl      0 <bar>
  14:   2a0003e2        mov     w2, w0
  18:   aa1303e0        mov     x0, x19
  1c:   aa0203e1        mov     x1, x2
  20:   d4000001        svc     #0x0
  24:   f9400bf3        ldr     x19, [sp, #16]
  28:   a8c27bfd        ldp     x29, x30, [sp], #32
  2c:   d65f03c0        ret


What do you think?

Cheers
---Dave

  reply index

Thread overview: 108+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-21  9:52 [PATCH v7 00/25] Unify vDSOs across more architectures Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 01/25] kernel: Standardize vdso_datapage Vincenzo Frascino
2019-06-24 13:56   ` Catalin Marinas
2019-06-21  9:52 ` [PATCH v7 02/25] kernel: Define gettimeofday vdso common code Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 03/25] kernel: Unify update_vsyscall implementation Vincenzo Frascino
2019-06-21 10:49   ` Huw Davies
2019-06-21  9:52 ` [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation Vincenzo Frascino
2019-06-24 13:36   ` Will Deacon
2019-06-24 13:59     ` Vincenzo Frascino
2019-06-25 16:18     ` [PATCH 1/3] lib/vdso: Delay mask application in do_hres() Vincenzo Frascino
2019-06-25 16:18       ` [PATCH 2/3] arm64: Fix __arch_get_hw_counter() implementation Vincenzo Frascino
2019-06-25 16:18       ` [PATCH 3/3] arm64: compat: " Vincenzo Frascino
2019-06-25 17:02       ` [PATCH 1/3] lib/vdso: Delay mask application in do_hres() Thomas Gleixner
2019-06-25 18:27         ` Thomas Gleixner
2019-06-25 20:15           ` Andy Lutomirski
2019-06-25 22:24             ` Thomas Gleixner
2019-06-26  6:38         ` Thomas Gleixner
2019-06-26  9:25           ` Vincenzo Frascino
2019-06-26 10:02             ` lib/vdso: Make delta calculation work correctly Thomas Gleixner
2019-06-26 11:08               ` Vincenzo Frascino
2019-06-24 13:58   ` [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation Catalin Marinas
2019-06-25 15:33   ` Dave Martin
2019-06-26 13:27     ` Vincenzo Frascino
2019-06-26 16:14       ` Dave Martin [this message]
2019-06-26 19:01         ` Vincenzo Frascino
2019-06-27 10:01           ` Dave Martin
2019-06-27 10:57             ` Vincenzo Frascino
2019-06-27 11:27               ` Dave Martin
2019-06-27 11:59                 ` Vincenzo Frascino
2019-06-27 14:38                   ` Dave Martin
2019-06-27 15:34                     ` Vincenzo Frascino
2019-06-25 17:43   ` [PATCH] arm64: vdso: Fix compilation with clang < 8 Vincenzo Frascino
2019-06-26 11:36   ` [PATCH v2] arm64: vdso: Fix compilation with clang older then 8 Vincenzo Frascino
     [not found]   ` <CGME20190628130921eucas1p239935b0771032c331911eacc1a69dd2e@eucas1p2.samsung.com>
2019-06-28 13:09     ` [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation Marek Szyprowski
2019-06-28 14:32       ` Vincenzo Frascino
2019-06-28 16:50         ` Sylwester Nawrocki
2019-06-29  6:58           ` Vincenzo Frascino
2019-07-08 12:57             ` Sylwester Nawrocki
2019-07-08 13:09               ` Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 05/25] arm64: Build vDSO with -ffixed-x18 Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 06/25] arm64: compat: Add missing syscall numbers Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 07/25] arm64: compat: Expose signal related structures Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 08/25] arm64: compat: Generate asm offsets for signals Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 09/25] lib: vdso: Add compat support Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 10/25] arm64: compat: Add vDSO Vincenzo Frascino
2019-06-24 14:00   ` Catalin Marinas
2019-07-10  4:02   ` John Stultz
2019-07-10  6:12     ` Thomas Gleixner
2019-07-10  9:48       ` Vincenzo Frascino
2019-07-10  8:27     ` Will Deacon
2019-07-10  8:58       ` Thomas Gleixner
2019-07-10  9:12         ` Will Deacon
2019-07-10  9:47     ` Vincenzo Frascino
2019-07-10 13:41       ` Vincenzo Frascino
2019-07-10 13:04   ` [PATCH] arm64: vdso: Fix ABI regression in compat vdso Vincenzo Frascino
2019-07-10 13:25     ` Will Deacon
2019-07-10 13:42       ` Vincenzo Frascino
2019-07-10 14:01   ` [PATCH v2] " Vincenzo Frascino
2019-07-10 15:44     ` John Stultz
2019-07-10 15:53       ` Vincenzo Frascino
2019-07-11  9:45     ` Will Deacon
2019-07-11 10:34       ` Thomas Gleixner
2019-07-11 11:32         ` Will Deacon
2019-06-21  9:52 ` [PATCH v7 11/25] arm64: Refactor vDSO code Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 12/25] arm64: compat: vDSO setup for compat layer Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 13/25] arm64: elf: vDSO code page discovery Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 14/25] arm64: compat: Get sigreturn trampolines from vDSO Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 15/25] arm64: Add vDSO compat support Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 16/25] arm: Add support for generic vDSO Vincenzo Frascino
2019-12-04 13:51   ` [PATCH v7 16/25] arm: Add support for generic vDSO (causing crash) Guenter Roeck
2019-12-04 13:58     ` Vincenzo Frascino
2019-12-04 16:16       ` Guenter Roeck
2019-12-04 17:15         ` Vincenzo Frascino
2019-12-04 19:39           ` Guenter Roeck
2019-12-05  9:42           ` Philippe Mathieu-Daudé
2019-12-05 10:00             ` Vincenzo Frascino
2019-12-05 11:02               ` Arnd Bergmann
2019-12-05 14:56                 ` Philippe Mathieu-Daudé
2019-06-21  9:52 ` [PATCH v7 17/25] arm: Add clock_getres entry point Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 18/25] arm: Add clock_gettime64 " Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 19/25] mips: Add support for generic vDSO Vincenzo Frascino
2019-07-26  5:15   ` Paul Burton
2019-07-26 16:29     ` [PATCH 0/2] mips: vdso: Fix Makefile Vincenzo Frascino
2019-07-26 16:29       ` [PATCH 1/2] mips: vdso: Fix source path Vincenzo Frascino
2019-07-26 16:29       ` [PATCH 2/2] mips: vdso: Fix flip/flop vdso building bug Vincenzo Frascino
2019-07-28 22:20       ` [PATCH 0/2] mips: vdso: Fix Makefile Paul Burton
2019-06-21  9:52 ` [PATCH v7 20/25] mips: Add clock_getres entry point Vincenzo Frascino
2019-07-26  5:15   ` Paul Burton
2019-06-21  9:52 ` [PATCH v7 21/25] mips: Add clock_gettime64 " Vincenzo Frascino
2019-07-26  5:15   ` Paul Burton
2019-06-21  9:52 ` [PATCH v7 22/25] x86: Add support for generic vDSO Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 23/25] x86: Add clock_getres entry point Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 24/25] x86: Add clock_gettime64 " Vincenzo Frascino
2019-06-21  9:52 ` [PATCH v7 25/25] kselftest: Extend vDSO selftest Vincenzo Frascino
2019-06-24  0:34 ` [PATCH v7 00/25] Unify vDSOs across more architectures Thomas Gleixner
2019-06-24  1:15   ` Andy Lutomirski
2019-06-24  7:42     ` Thomas Gleixner
2019-06-24 13:21   ` Vincenzo Frascino
2019-06-24 14:18   ` Thomas Gleixner
2019-06-24 14:23     ` Russell King - ARM Linux admin
2019-06-24 14:49       ` Catalin Marinas
2019-06-24 16:20         ` Vincenzo Frascino
2019-10-25 11:42         ` Geert Uytterhoeven
2019-06-24 18:41   ` Paul Burton
2019-06-24 23:16     ` Vincenzo Frascino
2019-06-25 17:11       ` Paul Burton
2019-06-25 17:17         ` Vincenzo Frascino
2019-06-24 12:50 ` Andre Przywara

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190626161413.GA2790@e103592.cambridge.arm.com \
    --to=dave.martin@arm.com \
    --cc=0x7f454c46@gmail.com \
    --cc=andre.przywara@arm.com \
    --cc=arnd@arndb.de \
    --cc=catalin.marinas@arm.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=huw@codeweavers.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=linux@rasmusvillemoes.dk \
    --cc=paul.burton@mips.com \
    --cc=pcc@google.com \
    --cc=ralf@linux-mips.org \
    --cc=salyzyn@android.com \
    --cc=shuah@kernel.org \
    --cc=sthotton@marvell.com \
    --cc=tglx@linutronix.de \
    --cc=vincenzo.frascino@arm.com \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git