Re: [PATCH v2 3/6] ARM: trusted_foundations: do not use naked function

From: Stephen Warren
Date: Mon Apr 16 2018 - 12:08:12 EST

On 04/16/2018 09:56 AM, Stefan Agner wrote:
On 27.03.2018 14:16, Dmitry Osipenko wrote:
On 27.03.2018 14:54, Robin Murphy wrote:
On 26/03/18 22:20, Dmitry Osipenko wrote:
On 25.03.2018 21:09, Stefan Agner wrote:
As documented in GCC naked functions should only use Basic asm
syntax. The Extended asm or mixture of Basic asm and "C" code is
not guaranteed. Currently this works because it was hard coded
to follow and check GCC behavior for arguments and register

Furthermore with clang using parameters in Extended asm in a
naked function is not supported:
ÂÂ arch/arm/firmware/trusted_foundations.c:47:10: error: parameter
ÂÂÂÂÂÂÂÂÂÂ references not allowed in naked functions
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ : "r" (type), "r" (arg1), "r" (arg2)

Use a regular function to be more portable. This aligns also with
the other smc call implementations e.g. in qcom_scm-32.c and

Cc: Dmitry Osipenko <digetx@xxxxxxxxx>
Cc: Stephen Warren <swarren@xxxxxxxxxx>
Cc: Thierry Reding <treding@xxxxxxxxxx>
Signed-off-by: Stefan Agner <stefan@xxxxxxxx>
Changes in v2:
- Keep stmfd/ldmfd to avoid potential ABI issues

 arch/arm/firmware/trusted_foundations.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/arm/firmware/trusted_foundations.c
index 3fb1b5a1dce9..689e6565abfc 100644
--- a/arch/arm/firmware/trusted_foundations.c
+++ b/arch/arm/firmware/trusted_foundations.c
@@ -31,21 +31,25 @@
  static unsigned long cpu_boot_addr;
 -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2)
+static void tf_generic_smc(u32 type, u32 arg1, u32 arg2)
+ÂÂÂ register u32 r0 asm("r0") = type;
+ÂÂÂ register u32 r1 asm("r1") = arg1;
+ÂÂÂ register u32 r2 asm("r2") = arg2;
ÂÂÂÂÂ asm volatile(
ÂÂÂÂÂÂÂÂÂ ".arch_extensionÂÂÂ sec\n\t"
-ÂÂÂÂÂÂÂ "stmfdÂÂÂ sp!, {r4 - r11, lr}\n\t"
+ÂÂÂÂÂÂÂ "stmfdÂÂÂ sp!, {r4 - r11}\n\t"
ÂÂÂÂÂÂÂÂÂ __asmeq("%0", "r0")
ÂÂÂÂÂÂÂÂÂ __asmeq("%1", "r1")
ÂÂÂÂÂÂÂÂÂ __asmeq("%2", "r2")
ÂÂÂÂÂÂÂÂÂ "movÂÂÂ r3, #0\n\t"
ÂÂÂÂÂÂÂÂÂ "movÂÂÂ r4, #0\n\t"
ÂÂÂÂÂÂÂÂÂ "smcÂÂÂ #0\n\t"
-ÂÂÂÂÂÂÂ "ldmfdÂÂÂ sp!, {r4 - r11, pc}"
+ÂÂÂÂÂÂÂ "ldmfdÂÂÂ sp!, {r4 - r11}\n\t"
-ÂÂÂÂÂÂÂ : "r" (type), "r" (arg1), "r" (arg2)
-ÂÂÂÂÂÂÂ : "memory");
+ÂÂÂÂÂÂÂ : "r" (r0), "r" (r1), "r" (r2)
+ÂÂÂÂÂÂÂ : "memory", "r3", "r12", "lr");

Although seems "lr" won't be affected by SMC invocation because it should be
banked and hence could be omitted entirely from the code. Maybe somebody could
confirm this.
Strictly per the letter of the architecture, the SMC could be trapped to Hyp
mode, and a hypervisor might clobber LR_usr in the process of forwarding the
call to the firmware secure monitor (since Hyp doesn't have a banked LR of its
own). Admittedly there are probably no real systems with the appropriate
hardware/software combination to hit that, but on the other hand if this gets
inlined where the compiler has already created a stack frame then an LR clobber
is essentially free, so I reckon we're better off keeping it for reassurance.
This isn't exactly a critical fast path anyway.

Okay, thank you for the clarification.

So it seems this change is fine?

Stephen, you picked up changes for this driver before, is this patch
going through your tree?

You had best ask Thierry; he's taken over Tegra maintenance upstream. But that said, don't files in arch/arm go through Russell?