From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AC356C48BD3 for ; Wed, 26 Jun 2019 13:28:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 828802147A for ; Wed, 26 Jun 2019 13:28:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727741AbfFZN2F (ORCPT ); Wed, 26 Jun 2019 09:28:05 -0400 Received: from foss.arm.com ([217.140.110.172]:32928 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726104AbfFZN2E (ORCPT ); Wed, 26 Jun 2019 09:28:04 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9A6DE360; Wed, 26 Jun 2019 06:28:03 -0700 (PDT) Received: from [10.1.196.72] (e119884-lin.cambridge.arm.com [10.1.196.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D00BA3F71E; Wed, 26 Jun 2019 06:28:00 -0700 (PDT) Subject: Re: [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation To: Dave Martin Cc: linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org, linux-kselftest@vger.kernel.org, Shuah Khan , Andre Przywara , Arnd Bergmann , Huw Davies , Catalin Marinas , Daniel Lezcano , Will Deacon , Russell King , Ralf Baechle , Mark Salyzyn , Paul Burton , Dmitry Safonov <0x7f454c46@gmail.com>, Rasmus Villemoes , Thomas Gleixner , Shijith Thotton , Peter Collingbourne References: <20190621095252.32307-1-vincenzo.frascino@arm.com> <20190621095252.32307-5-vincenzo.frascino@arm.com> <20190625153336.GZ2790@e103592.cambridge.arm.com> From: Vincenzo Frascino Message-ID: Date: Wed, 26 Jun 2019 14:27:59 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <20190625153336.GZ2790@e103592.cambridge.arm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> Cc: Will Deacon >> Signed-off-by: Vincenzo Frascino >> Tested-by: Shijith Thotton >> Tested-by: Andre Przywara > > [...] > >> 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 >> +#include >> + >> +#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. 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 : 0: 39400001 ldrb w1, [x0] 4: 35000161 cbnz w1, 30 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 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 GCC 8.1.0: ========== main.8.1.0.o: file format elf64-littleaarch64 Disassembly of section .text: 0000000000000000 : 0: 39400001 ldrb w1, [x0] 4: 35000161 cbnz w1, 30 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 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 GCC 6.4.0: ========== main.6.4.0.o: file format elf64-littleaarch64 Disassembly of section .text: 0000000000000000 : 0: 39400001 ldrb w1, [x0] 4: 35000161 cbnz w1, 30 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 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 > [...] > > Cheers > ---Dave > -- Regards, Vincenzo From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 96561C48BD6 for ; Wed, 26 Jun 2019 13:28:11 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5EBA62147A for ; Wed, 26 Jun 2019 13:28:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="dD5GKQzL" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5EBA62147A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=0uDBuHOb9mlxMyw+FFcKKpMOW5vsxkOAmXovrt98WOs=; b=dD5GKQzL3qZGMq gHT5nz7RLwmyYQS8exNDW6W0dE5hYYxSaMfKP5yVNV29a4D+imk62oy7yemaDhTvRfOLDY/E9PI3Z Y3pA/cz949SogtxToM/9/1VCt7lkBU3CZHiig9spmLc3fkjp+s5UNEPBX8X/AJi0J/NttkU5+7uez vFMbaZD+q9FcJG5C+Mm8vMF5vuYPMqMMc6/Bg6oS6ximqUn9z8SL3yubO5AAbPKGmdParEaIV8fc9 81pt1rwA3fJNEhkqAK+UVmCpjhk4Ffn25QEovEkuYnoSnHYvFvUlBU9P59twVZFXhuZq2EmX19+i8 fnnwcWg2Q7l6c6YJlKww==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hg7y4-0006f4-4b; Wed, 26 Jun 2019 13:28:08 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hg7y0-0006ea-Ug for linux-arm-kernel@lists.infradead.org; Wed, 26 Jun 2019 13:28:06 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9A6DE360; Wed, 26 Jun 2019 06:28:03 -0700 (PDT) Received: from [10.1.196.72] (e119884-lin.cambridge.arm.com [10.1.196.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D00BA3F71E; Wed, 26 Jun 2019 06:28:00 -0700 (PDT) Subject: Re: [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation To: Dave Martin References: <20190621095252.32307-1-vincenzo.frascino@arm.com> <20190621095252.32307-5-vincenzo.frascino@arm.com> <20190625153336.GZ2790@e103592.cambridge.arm.com> From: Vincenzo Frascino Message-ID: Date: Wed, 26 Jun 2019 14:27:59 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <20190625153336.GZ2790@e103592.cambridge.arm.com> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190626_062805_086115_E18FF431 X-CRM114-Status: GOOD ( 24.31 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-arch@vger.kernel.org, Shijith Thotton , Peter Collingbourne , Arnd Bergmann , Huw Davies , Andre Przywara , Daniel Lezcano , Will Deacon , linux-kernel@vger.kernel.org, Ralf Baechle , linux-mips@vger.kernel.org, Paul Burton , Rasmus Villemoes , linux-kselftest@vger.kernel.org, Catalin Marinas , Russell King , Dmitry Safonov <0x7f454c46@gmail.com>, Mark Salyzyn , Shuah Khan , Thomas Gleixner , linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 >> Cc: Will Deacon >> Signed-off-by: Vincenzo Frascino >> Tested-by: Shijith Thotton >> Tested-by: Andre Przywara > > [...] > >> 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 >> +#include >> + >> +#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. 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 : 0: 39400001 ldrb w1, [x0] 4: 35000161 cbnz w1, 30 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 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 GCC 8.1.0: ========== main.8.1.0.o: file format elf64-littleaarch64 Disassembly of section .text: 0000000000000000 : 0: 39400001 ldrb w1, [x0] 4: 35000161 cbnz w1, 30 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 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 GCC 6.4.0: ========== main.6.4.0.o: file format elf64-littleaarch64 Disassembly of section .text: 0000000000000000 : 0: 39400001 ldrb w1, [x0] 4: 35000161 cbnz w1, 30 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 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 > [...] > > Cheers > ---Dave > -- Regards, Vincenzo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel