Received: by 10.223.176.5 with SMTP id f5csp2772304wra; Thu, 1 Feb 2018 05:56:57 -0800 (PST) X-Google-Smtp-Source: AH8x224MRXktwNG2W/2j1f8yf+wq9wm9tAqwKGufviA9M/nzQf3hcI0g7NOUReHAC5Ks1mLCGXaV X-Received: by 2002:a17:902:ab85:: with SMTP id f5-v6mr32447015plr.199.1517493417628; Thu, 01 Feb 2018 05:56:57 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1517493417; cv=none; d=google.com; s=arc-20160816; b=qqL7LvNJp+D2/oHBSs/HNnjqHBdReAjsQFZzFUVIfPVIwMzu8Kd2J0Gb+St/Dk5hxg a/z3k3h7Zt6sxt+diIjZaO5/WDu+im9f6CU9dTOpwpMCTQOCEOL4/hDtTzFuVsqAvSJb rDIXAtUkcHZz75npjpT1Xw1xkQ4OzXNSjP0RnKZKIMYkU7UvoRAkQbtBfZLKekDYjA1P TeOEGMYuLWsRc+D8Ne41Os+tkLeCFrmrk9FgRPg7Yrs/MOX/bB76LU/kiftiEj15O30z 5uo2yWAM/+mZqQZrS79XEagYVad6UpCwdQ28hvAwuiWG4t5gFp/yizbypPMUnf73lqjv AiRw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:organization:from:references:cc:to:subject :arc-authentication-results; bh=ChJYGgdevGDHwQO3yajki35ij8RpEMI7JV+D1LLIDDw=; b=oG+FHay7H7KH4fmb40WpyuRFJG65n31MXA2TcLNoa03P112ZRznABsQJ8vm6bEEOeS aWiM8L+Krc4Xe/TPXFks+T9tpzCkJu/fCUBUgkpDWvVwKPmi/RZhFG6TQ1ckPqiri928 ZxXzhSbfOraQ2A9gFWMgwhxb9BUJ81PJiBAwGQbV+3Ty6WTgX6wdWv2FkOIokO4U2XeC CYnpYsEX7YhiPgreasHqFJ2fRgJEFaoIkc7X93WxatXfLsfNggihOePPsqbNJ+2eGYmn TJlrWn2F7hiVt1jFaC/A4Jb618qICLesRRpKIm0PpKEYFBT6yqsDK6oPztwIWJ5kOH/4 ivgg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id h7si12767282pgc.827.2018.02.01.05.56.30; Thu, 01 Feb 2018 05:56:57 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751569AbeBANy7 (ORCPT + 99 others); Thu, 1 Feb 2018 08:54:59 -0500 Received: from foss.arm.com ([217.140.101.70]:50620 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751529AbeBANy5 (ORCPT ); Thu, 1 Feb 2018 08:54:57 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id CF48980D; Thu, 1 Feb 2018 05:54:56 -0800 (PST) Received: from [10.1.207.62] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 20EAD3F25C; Thu, 1 Feb 2018 05:54:53 -0800 (PST) Subject: Re: [PATCH v3 16/18] arm/arm64: smccc: Implement SMCCC v1.1 inline primitive To: Robin Murphy , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu Cc: Catalin Marinas , Will Deacon , Peter Maydell , Christoffer Dall , Lorenzo Pieralisi , Mark Rutland , Ard Biesheuvel , Andrew Jones , Hanjun Guo , Jayachandran C , Jon Masters , Russell King - ARM Linux References: <20180201114657.7323-1-marc.zyngier@arm.com> <20180201114657.7323-17-marc.zyngier@arm.com> <6082f2bf-58be-8493-013e-e27f5a0d2570@arm.com> From: Marc Zyngier Organization: ARM Ltd Message-ID: <0af501c9-2481-cb42-e1da-a884f7379942@arm.com> Date: Thu, 1 Feb 2018 13:54:52 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <6082f2bf-58be-8493-013e-e27f5a0d2570@arm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/02/18 13:34, Robin Murphy wrote: > On 01/02/18 11:46, Marc Zyngier wrote: >> One of the major improvement of SMCCC v1.1 is that it only clobbers >> the first 4 registers, both on 32 and 64bit. This means that it >> becomes very easy to provide an inline version of the SMC call >> primitive, and avoid performing a function call to stash the >> registers that would otherwise be clobbered by SMCCC v1.0. >> >> Signed-off-by: Marc Zyngier >> --- >> include/linux/arm-smccc.h | 143 ++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 143 insertions(+) >> >> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h >> index dd44d8458c04..575aabe85905 100644 >> --- a/include/linux/arm-smccc.h >> +++ b/include/linux/arm-smccc.h >> @@ -150,5 +150,148 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1, >> >> #define arm_smccc_hvc_quirk(...) __arm_smccc_hvc(__VA_ARGS__) >> >> +/* SMCCC v1.1 implementation madness follows */ >> +#ifdef CONFIG_ARM64 >> + >> +#define SMCCC_SMC_INST "smc #0" >> +#define SMCCC_HVC_INST "hvc #0" > > Nit: Maybe the argument can go in the template and we just define the > instruction mnemonics here? > >> + >> +#endif >> + >> +#ifdef CONFIG_ARM > > #elif ? Sure, why not. > >> +#include >> +#include >> + >> +#define SMCCC_SMC_INST __SMC(0) >> +#define SMCCC_HVC_INST __HVC(0) > > Oh, I see, it was to line up with this :( > > I do wonder if we could just embed an asm(".arch armv7-a+virt\n") (if > even necessary) for ARM, then take advantage of the common mnemonics for > all 3 instruction sets instead of needing manual encoding tricks? I > don't think we should ever be pulling this file in for non-v7 builds. > > I suppose that strictly that appears to need binutils 2.21 rather than > the offical supported minimum of 2.20, but are people going to be > throwing SMCCC configs at antique toolchains in practice? It has been an issue in the past, back when we merged KVM. We settled on a hybrid solution where code outside of KVM would not rely on a newer toolchain, hence the macros that Dave introduced. Maybe we've moved on and we can take that bold step? > >> + >> +#endif >> + >> +#define ___count_args(_0, _1, _2, _3, _4, _5, _6, _7, _8, x, ...) x >> + >> +#define __count_args(...) \ >> + ___count_args(__VA_ARGS__, 7, 6, 5, 4, 3, 2, 1, 0) >> + >> +#define __constraint_write_0 \ >> + "+r" (r0), "=&r" (r1), "=&r" (r2), "=&r" (r3) >> +#define __constraint_write_1 \ >> + "+r" (r0), "+r" (r1), "=&r" (r2), "=&r" (r3) >> +#define __constraint_write_2 \ >> + "+r" (r0), "+r" (r1), "+r" (r2), "=&r" (r3) >> +#define __constraint_write_3 \ >> + "+r" (r0), "+r" (r1), "+r" (r2), "+r" (r3) >> +#define __constraint_write_4 __constraint_write_3 >> +#define __constraint_write_5 __constraint_write_4 >> +#define __constraint_write_6 __constraint_write_5 >> +#define __constraint_write_7 __constraint_write_6 >> + >> +#define __constraint_read_0 >> +#define __constraint_read_1 >> +#define __constraint_read_2 >> +#define __constraint_read_3 >> +#define __constraint_read_4 "r" (r4) >> +#define __constraint_read_5 __constraint_read_4, "r" (r5) >> +#define __constraint_read_6 __constraint_read_5, "r" (r6) >> +#define __constraint_read_7 __constraint_read_6, "r" (r7) >> + >> +#define __declare_arg_0(a0, res) \ >> + struct arm_smccc_res *___res = res; \ > > Looks like the declaration of ___res could simply be factored out to the > template... Tried that. But... > >> + register u32 r0 asm("r0") = a0; \ >> + register unsigned long r1 asm("r1"); \ >> + register unsigned long r2 asm("r2"); \ >> + register unsigned long r3 asm("r3") >> + >> +#define __declare_arg_1(a0, a1, res) \ >> + struct arm_smccc_res *___res = res; \ >> + register u32 r0 asm("r0") = a0; \ >> + register typeof(a1) r1 asm("r1") = a1; \ >> + register unsigned long r2 asm("r2"); \ >> + register unsigned long r3 asm("r3") >> + >> +#define __declare_arg_2(a0, a1, a2, res) \ >> + struct arm_smccc_res *___res = res; \ >> + register u32 r0 asm("r0") = a0; \ >> + register typeof(a1) r1 asm("r1") = a1; \ >> + register typeof(a2) r2 asm("r2") = a2; \ >> + register unsigned long r3 asm("r3") >> + >> +#define __declare_arg_3(a0, a1, a2, a3, res) \ >> + struct arm_smccc_res *___res = res; \ >> + register u32 r0 asm("r0") = a0; \ >> + register typeof(a1) r1 asm("r1") = a1; \ >> + register typeof(a2) r2 asm("r2") = a2; \ >> + register typeof(a3) r3 asm("r3") = a3 >> + >> +#define __declare_arg_4(a0, a1, a2, a3, a4, res) \ >> + __declare_arg_3(a0, a1, a2, a3, res); \ >> + register typeof(a4) r4 asm("r4") = a4 >> + >> +#define __declare_arg_5(a0, a1, a2, a3, a4, a5, res) \ >> + __declare_arg_4(a0, a1, a2, a3, a4, res); \ >> + register typeof(a5) r5 asm("r5") = a5 >> + >> +#define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res) \ >> + __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \ >> + register typeof(a6) r6 asm("r6") = a6 >> + >> +#define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res) \ >> + __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \ >> + register typeof(a7) r7 asm("r7") = a7 >> + >> +#define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__) >> +#define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__) >> + >> +#define ___constraints(count) \ >> + : __constraint_write_ ## count \ >> + : __constraint_read_ ## count \ >> + : "memory" >> +#define __constraints(count) ___constraints(count) >> + >> +/* >> + * We have an output list that is not necessarily used, and GCC feels >> + * entitled to optimise the whole sequence away. "volatile" is what >> + * makes it stick. >> + */ >> +#define __arm_smccc_1_1(inst, ...) \ >> + do { \ >> + __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \ >> + asm volatile(inst "\n" \ >> + __constraints(__count_args(__VA_ARGS__))); \ >> + if (___res) \ >> + *___res = (typeof(*___res)){r0, r1, r2, r3}; \ > > ...especially since there's no obvious indication of where it comes from > when you're looking here. ... we don't have the variable name at all here (it is the last parameter, and that doesn't quite work with the idea of variadic macros...). The alternative would be to add a set of macros that return the result parameter, based on the number of inputs. Not sure that's an improvement. Thoughts? > > Otherwise, though, this has already turned out pretty sleek; > > Reviewed-by: Robin Murphy Thanks, M. -- Jazz is not dead. It just smells funny...