[PATCH 1/2] arm64: psci: move psci firmware calls out of line

[PATCH 1/2] arm64: psci: move psci firmware calls out of line

[Will Deacon]

An arm64 allmodconfig fails to build with GCC 5 due to __asmeq
assertions in the PSCI firmware calling code firing due to mcount
preambles breaking our assumptions about register allocation of function
arguments:

  /tmp/ccDqJsJ6.s: Assembler messages:
  /tmp/ccDqJsJ6.s:60: Error: .err encountered
  /tmp/ccDqJsJ6.s:61: Error: .err encountered
  /tmp/ccDqJsJ6.s:62: Error: .err encountered
  /tmp/ccDqJsJ6.s:99: Error: .err encountered
  /tmp/ccDqJsJ6.s:100: Error: .err encountered
  /tmp/ccDqJsJ6.s:101: Error: .err encountered

This patch fixes the issue by moving the PSCI calls out-of-line into
their own assembly files, which are safe from the compiler's meddling
fingers.

Reported-by: Andy Whitcroft <apw@canonical.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/Makefile    |  5 +++--
 arch/arm64/kernel/psci-call.S | 28 ++++++++++++++++++++++++++++
 arch/arm64/kernel/psci.c      | 37 +++----------------------------------
 3 files changed, 34 insertions(+), 36 deletions(-)
 create mode 100644 arch/arm64/kernel/psci-call.S

diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index bef04afd6031..5ee07eee80c2 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -15,8 +15,9 @@ CFLAGS_REMOVE_return_address.o = -pg
 arm64-obj-y		:= cputable.o debug-monitors.o entry.o irq.o fpsimd.o	\
 			   entry-fpsimd.o process.o ptrace.o setup.o signal.o	\
 			   sys.o stacktrace.o time.o traps.o io.o vdso.o	\
-			   hyp-stub.o psci.o cpu_ops.o insn.o return_address.o	\
-			   cpuinfo.o cpu_errata.o alternative.o cacheinfo.o
+			   hyp-stub.o psci.o psci-call.o cpu_ops.o insn.o	\
+			   return_address.o cpuinfo.o cpu_errata.o		\
+			   alternative.o cacheinfo.o
 
 arm64-obj-$(CONFIG_COMPAT)		+= sys32.o kuser32.o signal32.o 	\
 					   sys_compat.o entry32.o		\
diff --git a/arch/arm64/kernel/psci-call.S b/arch/arm64/kernel/psci-call.S
new file mode 100644
index 000000000000..cf83e61cd3b5
--- /dev/null
+++ b/arch/arm64/kernel/psci-call.S
@@ -0,0 +1,28 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Copyright (C) 2015 ARM Limited
+ *
+ * Author: Will Deacon <will.deacon@arm.com>
+ */
+
+#include <linux/linkage.h>
+
+/* int __invoke_psci_fn_hvc(u64 function_id, u64 arg0, u64 arg1, u64 arg2) */
+ENTRY(__invoke_psci_fn_hvc)
+	hvc	#0
+	ret
+ENDPROC(__invoke_psci_fn_hvc)
+
+/* int __invoke_psci_fn_smc(u64 function_id, u64 arg0, u64 arg1, u64 arg2) */
+ENTRY(__invoke_psci_fn_smc)
+	smc	#0
+	ret
+ENDPROC(__invoke_psci_fn_smc)
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index 3425f311c49e..9b8a70ae64a1 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -57,6 +57,9 @@ static struct psci_operations psci_ops;
 static int (*invoke_psci_fn)(u64, u64, u64, u64);
 typedef int (*psci_initcall_t)(const struct device_node *);
 
+asmlinkage int __invoke_psci_fn_hvc(u64, u64, u64, u64);
+asmlinkage int __invoke_psci_fn_smc(u64, u64, u64, u64);
+
 enum psci_function {
 	PSCI_FN_CPU_SUSPEND,
 	PSCI_FN_CPU_ON,
@@ -109,40 +112,6 @@ static void psci_power_state_unpack(u32 power_state,
 			PSCI_0_2_POWER_STATE_AFFL_SHIFT;
 }
 
-/*
- * The following two functions are invoked via the invoke_psci_fn pointer
- * and will not be inlined, allowing us to piggyback on the AAPCS.
- */
-static noinline int __invoke_psci_fn_hvc(u64 function_id, u64 arg0, u64 arg1,
-					 u64 arg2)
-{
-	asm volatile(
-			__asmeq("%0", "x0")
-			__asmeq("%1", "x1")
-			__asmeq("%2", "x2")
-			__asmeq("%3", "x3")
-			"hvc	#0\n"
-		: "+r" (function_id)
-		: "r" (arg0), "r" (arg1), "r" (arg2));
-
-	return function_id;
-}
-
-static noinline int __invoke_psci_fn_smc(u64 function_id, u64 arg0, u64 arg1,
-					 u64 arg2)
-{
-	asm volatile(
-			__asmeq("%0", "x0")
-			__asmeq("%1", "x1")
-			__asmeq("%2", "x2")
-			__asmeq("%3", "x3")
-			"smc	#0\n"
-		: "+r" (function_id)
-		: "r" (arg0), "r" (arg1), "r" (arg2));
-
-	return function_id;
-}
-
 static int psci_get_version(void)
 {
 	int err;
-- 
2.1.4

[PATCH 2/2] ARM: psci: move psci firmware calls out of line

[Will Deacon]

From: Mark Rutland <mark.rutland@arm.com>

arm64 builds with GCC 5 have caused the __asmeq assertions in the PSCI
calling code to fire, so move the ARM PSCI calls out of line into their
own assembly file for consistency and to safeguard against the same
issue occuring with the 32-bit toolchain.

Reported-by: Andy Whitcroft <apw@canonical.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
[will: brought into line with arm64 implementation]
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/Makefile    |  2 +-
 arch/arm/kernel/psci-call.S | 31 +++++++++++++++++++++++++++++++
 arch/arm/kernel/psci.c      | 39 +++------------------------------------
 3 files changed, 35 insertions(+), 37 deletions(-)
 create mode 100644 arch/arm/kernel/psci-call.S

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 902397dd1000..1c1cdfa566ac 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -86,7 +86,7 @@ obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
 
 obj-$(CONFIG_ARM_VIRT_EXT)	+= hyp-stub.o
 ifeq ($(CONFIG_ARM_PSCI),y)
-obj-y				+= psci.o
+obj-y				+= psci.o psci-call.o
 obj-$(CONFIG_SMP)		+= psci_smp.o
 endif
 
diff --git a/arch/arm/kernel/psci-call.S b/arch/arm/kernel/psci-call.S
new file mode 100644
index 000000000000..a78e9e1e206d
--- /dev/null
+++ b/arch/arm/kernel/psci-call.S
@@ -0,0 +1,31 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Copyright (C) 2015 ARM Limited
+ *
+ * Author: Mark Rutland <mark.rutland@arm.com>
+ */
+
+#include <linux/linkage.h>
+
+#include <asm/opcodes-sec.h>
+#include <asm/opcodes-virt.h>
+
+/* int __invoke_psci_fn_hvc(u32 function_id, u32 arg0, u32 arg1, u32 arg2) */
+ENTRY(__invoke_psci_fn_hvc)
+	__HVC(0)
+	bx	lr
+ENDPROC(__invoke_psci_fn_hvc)
+
+/* int __invoke_psci_fn_smc(u32 function_id, u32 arg0, u32 arg1, u32 arg2) */
+ENTRY(__invoke_psci_fn_smc)
+	__SMC(0)
+	bx	lr
+ENDPROC(__invoke_psci_fn_smc)
diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index f73891b6b730..f90fdf4ce7c7 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -23,8 +23,6 @@
 
 #include <asm/compiler.h>
 #include <asm/errno.h>
-#include <asm/opcodes-sec.h>
-#include <asm/opcodes-virt.h>
 #include <asm/psci.h>
 #include <asm/system_misc.h>
 
@@ -33,6 +31,9 @@ struct psci_operations psci_ops;
 static int (*invoke_psci_fn)(u32, u32, u32, u32);
 typedef int (*psci_initcall_t)(const struct device_node *);
 
+asmlinkage int __invoke_psci_fn_hvc(u32, u32, u32, u32);
+asmlinkage int __invoke_psci_fn_smc(u32, u32, u32, u32);
+
 enum psci_function {
 	PSCI_FN_CPU_SUSPEND,
 	PSCI_FN_CPU_ON,
@@ -71,40 +72,6 @@ static u32 psci_power_state_pack(struct psci_power_state state)
 		 & PSCI_0_2_POWER_STATE_AFFL_MASK);
 }
 
-/*
- * The following two functions are invoked via the invoke_psci_fn pointer
- * and will not be inlined, allowing us to piggyback on the AAPCS.
- */
-static noinline int __invoke_psci_fn_hvc(u32 function_id, u32 arg0, u32 arg1,
-					 u32 arg2)
-{
-	asm volatile(
-			__asmeq("%0", "r0")
-			__asmeq("%1", "r1")
-			__asmeq("%2", "r2")
-			__asmeq("%3", "r3")
-			__HVC(0)
-		: "+r" (function_id)
-		: "r" (arg0), "r" (arg1), "r" (arg2));
-
-	return function_id;
-}
-
-static noinline int __invoke_psci_fn_smc(u32 function_id, u32 arg0, u32 arg1,
-					 u32 arg2)
-{
-	asm volatile(
-			__asmeq("%0", "r0")
-			__asmeq("%1", "r1")
-			__asmeq("%2", "r2")
-			__asmeq("%3", "r3")
-			__SMC(0)
-		: "+r" (function_id)
-		: "r" (arg0), "r" (arg1), "r" (arg2));
-
-	return function_id;
-}
-
 static int psci_get_version(void)
 {
 	int err;
-- 
2.1.4

[PATCH 2/2] ARM: psci: move psci firmware calls out of line

[Russell King - ARM Linux]

On Wed, Feb 25, 2015 at 12:10:36PM +0000, Will Deacon wrote:
> -/*
> - * The following two functions are invoked via the invoke_psci_fn pointer
> - * and will not be inlined, allowing us to piggyback on the AAPCS.
> - */
> -static noinline int __invoke_psci_fn_hvc(u32 function_id, u32 arg0, u32 arg1,
> -					 u32 arg2)
> -{
> -	asm volatile(
> -			__asmeq("%0", "r0")
> -			__asmeq("%1", "r1")
> -			__asmeq("%2", "r2")
> -			__asmeq("%3", "r3")
> -			__HVC(0)
> -		: "+r" (function_id)
> -		: "r" (arg0), "r" (arg1), "r" (arg2));
> -
> -	return function_id;
> -}

Why not convert these to:

static int __naked __invoke_psci_fn_hvc(u32 function_id, u32 arg0, u32 arg1,
					u32 arg2)
{
	asm(
		__HVC(0)
		"bx lr");
}

?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/2] ARM: psci: move psci firmware calls out of line

[Will Deacon]

On Wed, Mar 18, 2015 at 10:20:29AM +0000, Russell King - ARM Linux wrote:
> On Wed, Feb 25, 2015 at 12:10:36PM +0000, Will Deacon wrote:
> > -/*
> > - * The following two functions are invoked via the invoke_psci_fn pointer
> > - * and will not be inlined, allowing us to piggyback on the AAPCS.
> > - */
> > -static noinline int __invoke_psci_fn_hvc(u32 function_id, u32 arg0, u32 arg1,
> > -					 u32 arg2)
> > -{
> > -	asm volatile(
> > -			__asmeq("%0", "r0")
> > -			__asmeq("%1", "r1")
> > -			__asmeq("%2", "r2")
> > -			__asmeq("%3", "r3")
> > -			__HVC(0)
> > -		: "+r" (function_id)
> > -		: "r" (arg0), "r" (arg1), "r" (arg2));
> > -
> > -	return function_id;
> > -}
> 
> Why not convert these to:
> 
> static int __naked __invoke_psci_fn_hvc(u32 function_id, u32 arg0, u32 arg1,
> 					u32 arg2)
> {
> 	asm(
> 		__HVC(0)
> 		"bx lr");
> }
> 
> ?

I tried this, but the compiler printed a diagnostic along the lines of
"Ignoring __naked attribute", so we moved the functions out-of-line like
we have on arm64 instead. Assuming it worked reliably, what's the
advantage of __naked over having these out-of-line?

Will

[PATCH 2/2] ARM: psci: move psci firmware calls out of line

[Russell King - ARM Linux]

On Wed, Mar 18, 2015 at 10:30:14AM +0000, Will Deacon wrote:
> On Wed, Mar 18, 2015 at 10:20:29AM +0000, Russell King - ARM Linux wrote:
> > Why not convert these to:
> > 
> > static int __naked __invoke_psci_fn_hvc(u32 function_id, u32 arg0, u32 arg1,
> > 					u32 arg2)
> > {
> > 	asm(
> > 		__HVC(0)
> > 		"bx lr");
> > }
> > 
> > ?
> 
> I tried this, but the compiler printed a diagnostic along the lines of
> "Ignoring __naked attribute", so we moved the functions out-of-line like
> we have on arm64 instead. Assuming it worked reliably, what's the
> advantage of __naked over having these out-of-line?

Note that the above isn't using any asm constraints/arguments.  The GCC
manual states what can be safely included:

`naked'
     Use this attribute on the ARM, AVR, IP2K, RX and SPU ports to
     indicate that the specified function does not need
     prologue/epilogue sequences generated by the compiler.  It is up
     to the programmer to provide these sequences. The only statements

     that can be safely included in naked functions are `asm'

     statements that do not have operands.  All other statements,
                ^^^^^^^^^^^^^^^^^^^^^^^^^^

     including declarations of local variables, `if' statements, and so
     forth, should be avoided.  Naked functions should be used to
     implement the body of an assembly function, while allowing the
     compiler to construct the requisite function declaration for the
     assembler.

The advantage is that you have locality of code, and we don't need to
spring up lots of assembly files along side their .c files.

I'd guess that if you try to declare local variables etc, the compiler
will just ignore the naked attribute, because it then can't give the
guarantees that the function will be truely naked.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/2] ARM: psci: move psci firmware calls out of line

[Will Deacon]

On Wed, Mar 18, 2015 at 10:35:57AM +0000, Russell King - ARM Linux wrote:
> On Wed, Mar 18, 2015 at 10:30:14AM +0000, Will Deacon wrote:
> > On Wed, Mar 18, 2015 at 10:20:29AM +0000, Russell King - ARM Linux wrote:
> > > Why not convert these to:
> > > 
> > > static int __naked __invoke_psci_fn_hvc(u32 function_id, u32 arg0, u32 arg1,
> > > 					u32 arg2)
> > > {
> > > 	asm(
> > > 		__HVC(0)
> > > 		"bx lr");
> > > }
> > > 
> > > ?
> > 
> > I tried this, but the compiler printed a diagnostic along the lines of
> > "Ignoring __naked attribute", so we moved the functions out-of-line like
> > we have on arm64 instead. Assuming it worked reliably, what's the
> > advantage of __naked over having these out-of-line?
> 
> Note that the above isn't using any asm constraints/arguments.  The GCC
> manual states what can be safely included:
> 
> `naked'
>      Use this attribute on the ARM, AVR, IP2K, RX and SPU ports to
>      indicate that the specified function does not need
>      prologue/epilogue sequences generated by the compiler.  It is up
>      to the programmer to provide these sequences. The only statements
> 
>      that can be safely included in naked functions are `asm'
> 
>      statements that do not have operands.  All other statements,
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>      including declarations of local variables, `if' statements, and so
>      forth, should be avoided.  Naked functions should be used to
>      implement the body of an assembly function, while allowing the
>      compiler to construct the requisite function declaration for the
>      assembler.
> 
> The advantage is that you have locality of code, and we don't need to
> spring up lots of assembly files along side their .c files.
> 
> I'd guess that if you try to declare local variables etc, the compiler
> will just ignore the naked attribute, because it then can't give the
> guarantees that the function will be truely naked.

In which case, I can reinvestigate when I get some spare cycles. IIRC, the
problem only cropped up with allmodconfig, because we compile with -pg
but it's all a bit hazy...

Will