[PATCH 0/5] ARM: clang support

[PATCH 0/5] ARM: clang support

[Stefan Agner]

This patchset fixes some remaining issues when building the ARM
architecture using LLVM/clang.  The patchset requires the following
kbuild change:
https://lkml.org/lkml/2018/3/19/1756

With that patch and this patchset applied and I can successfully
build the multi_v7_defconfig with 4.16-rc5 using clang 5.0.1.

So far I mainly tested with ARMv7 architectures. A brief test with
ARMv6 enabled (multi platform configuraiton) failed with an error
from the assembler:
/tmp/empty-96a4d6.s: Assembler messages:
/tmp/empty-96a4d6.s:4: Error: unknown cpu `arm1176j-s'

It seems that clang emits a non-existig cpu name:
https://reviews.llvm.org/D18086

Stefan Agner (5):
  bus: arm-cci: use asm unreachable
  efi/libstub/arm: add support for building with clang
  ARM: trusted_foundations: do not use naked function
  ARM: drop no-thumb-interwork in EABI mode
  ARM: add support for building ARM kernel with clang

 arch/arm/Makefile                       |  4 ++--
 arch/arm/boot/compressed/Makefile       |  2 +-
 arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
 drivers/bus/arm-cci.c                   |  3 +--
 drivers/firmware/efi/libstub/Makefile   |  3 ++-
 5 files changed, 13 insertions(+), 11 deletions(-)

-- 
2.16.2

[PATCH 1/5] bus: arm-cci: use asm unreachable

[Stefan Agner]

Mixing asm and C code is not recommended in a naked function by
gcc and leads to an error when using clang:
  drivers/bus/arm-cci.c:2107:2: error: non-ASM statement in naked
  function is not supported
        unreachable();
        ^

Instead of using the unreachable() macro use the assember variant
ASM_UNREACHABLE.  This will no longer emit __builtin_unreachable(),
but since the function is naked and its return type is void it seems
not to have aversive effects.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/bus/arm-cci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 5426c04fe24b..ee9da86fec47 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -2084,6 +2084,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
 
 "	mov	r0, #0 \n"
 "	bx	lr \n"
+	ASM_UNREACHABLE
 
 "	.align	2 \n"
 "5:	.word	cpu_port - . \n"
@@ -2103,8 +2104,6 @@ asmlinkage void __naked cci_enable_port_for_self(void)
 	[sizeof_struct_cpu_port] "i" (sizeof(struct cpu_port)),
 	[sizeof_struct_ace_port] "i" (sizeof(struct cci_ace_port)),
 	[offsetof_port_phys] "i" (offsetof(struct cci_ace_port, phys)) );
-
-	unreachable();
 }
 
 /**
-- 
2.16.2

[PATCH 2/5] efi/libstub/arm: add support for building with clang

[Stefan Agner]

Use cc-options call for -mno-single-pic-base since the option is
not available in clang. LLVM/clang always assumes a fixed
displacement between text/data and hence uses PC relative
addressing.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/firmware/efi/libstub/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 7b3ba40f0745..2bf69cca0d87 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -13,7 +13,8 @@ cflags-$(CONFIG_X86)		+= -m$(BITS) -D__KERNEL__ -O2 \
 
 cflags-$(CONFIG_ARM64)		:= $(subst -pg,,$(KBUILD_CFLAGS)) -fpie
 cflags-$(CONFIG_ARM)		:= $(subst -pg,,$(KBUILD_CFLAGS)) \
-				   -fno-builtin -fpic -mno-single-pic-base
+				   -fno-builtin -fpic \
+				   $(call cc-option,-mno-single-pic-base,)
 
 cflags-$(CONFIG_EFI_ARMSTUB)	+= -I$(srctree)/scripts/dtc/libfdt
 
-- 
2.16.2

[PATCH 3/5] ARM: trusted_foundations: do not use naked function

[Stefan Agner]

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
placement.

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
bcm_kona_smc.c.

Additionally also make sure all callee-saved registers get saved
as it has been done before.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
index 3fb1b5a1dce9..426d732e6591 100644
--- a/arch/arm/firmware/trusted_foundations.c
+++ b/arch/arm/firmware/trusted_foundations.c
@@ -31,21 +31,23 @@
 
 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"
 		__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}"
 		:
-		: "r" (type), "r" (arg1), "r" (arg2)
-		: "memory");
+		: "r" (r0), "r" (r1), "r" (r2)
+		: "memory", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10");
 }
 
 static int tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
-- 
2.16.2

[PATCH 4/5] ARM: drop no-thumb-interwork in EABI mode

[Stefan Agner]

According to GCC documentation -m(no-)thumb-interwork is
meaningless in AAPCS configurations. Also clang does not
support the flag:
  clang-5.0: error: unknown argument: '-mno-thumb-interwork'

Just drop -mno-thumb-interwork in AEABI configuration.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index e83f5161fdd8..e9e3fde3c657 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -106,7 +106,7 @@ tune-$(CONFIG_CPU_V6K)		=$(call cc-option,-mtune=arm1136j-s,-mtune=strongarm)
 tune-y := $(tune-y)
 
 ifeq ($(CONFIG_AEABI),y)
-CFLAGS_ABI	:=-mabi=aapcs-linux -mno-thumb-interwork -mfpu=vfp
+CFLAGS_ABI	:=-mabi=aapcs-linux -mfpu=vfp
 else
 CFLAGS_ABI	:=$(call cc-option,-mapcs-32,-mabi=apcs-gnu) $(call cc-option,-mno-thumb-interwork,)
 endif
-- 
2.16.2

[PATCH 5/5] ARM: add support for building ARM kernel with clang

[Stefan Agner]

Use cc-options call for compiler options which are not available
in clang. With this patch an ARMv7 multi platform kernel can be
successfully build using clang (tested with version 5.0.1).

Based-on-patches-by: Behan Webster <behanw@converseincode.com>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/Makefile                 | 2 +-
 arch/arm/boot/compressed/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index e9e3fde3c657..20e9fee1ccc5 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -39,7 +39,7 @@ KBUILD_CFLAGS	+= $(call cc-option,-mno-unaligned-access)
 endif
 
 ifeq ($(CONFIG_FRAME_POINTER),y)
-KBUILD_CFLAGS	+=-fno-omit-frame-pointer -mapcs -mno-sched-prolog
+KBUILD_CFLAGS	+=-fno-omit-frame-pointer $(call cc-option,-mapcs,) $(call cc-option,-mno-sched-prolog,)
 endif
 
 ifeq ($(CONFIG_CPU_BIG_ENDIAN),y)
diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index 45a6b9b7af2a..6a5c4ac97703 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -113,7 +113,7 @@ CFLAGS_fdt_ro.o := $(nossp_flags)
 CFLAGS_fdt_rw.o := $(nossp_flags)
 CFLAGS_fdt_wip.o := $(nossp_flags)
 
-ccflags-y := -fpic -mno-single-pic-base -fno-builtin -I$(obj)
+ccflags-y := -fpic $(call cc-option,-mno-single-pic-base,) -fno-builtin -I$(obj)
 asflags-y := -DZIMAGE
 
 # Supply kernel BSS size to the decompressor via a linker symbol.
-- 
2.16.2

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

[Russell King - ARM Linux]

On Wed, Mar 21, 2018 at 12:02:04AM +0100, 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
> placement.

Those checks have nothing to do with that at all.  The whole point of
__asmeq() is to catch situations where you use register variables,
specifying which register you want them in, and GCC then ends up
passing them to assembly code in some other random register(s).

This was found with older GCCs, and the problem was fixed.  It has
nothing to do with naked functions per se.

In fact, as you're introducing further register variables, these
checks become more important to have than they were with the
previous code.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH 5/5] ARM: add support for building ARM kernel with clang

[Russell King - ARM Linux]

On Wed, Mar 21, 2018 at 12:02:06AM +0100, Stefan Agner wrote:
> Use cc-options call for compiler options which are not available
> in clang. With this patch an ARMv7 multi platform kernel can be
> successfully build using clang (tested with version 5.0.1).
> 
> Based-on-patches-by: Behan Webster <behanw@converseincode.com>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  arch/arm/Makefile                 | 2 +-
>  arch/arm/boot/compressed/Makefile | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index e9e3fde3c657..20e9fee1ccc5 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -39,7 +39,7 @@ KBUILD_CFLAGS	+= $(call cc-option,-mno-unaligned-access)
>  endif
>  
>  ifeq ($(CONFIG_FRAME_POINTER),y)
> -KBUILD_CFLAGS	+=-fno-omit-frame-pointer -mapcs -mno-sched-prolog
> +KBUILD_CFLAGS	+=-fno-omit-frame-pointer $(call cc-option,-mapcs,) $(call cc-option,-mno-sched-prolog,)

Some of these options here are to ensure that we generate the following
code, so we can backtrace:

	mov	ip, sp
	stmfd	sp!, {fp, ip, lr, pc}
	sub	fp, ip, #4

If clang isn't producing that code at the start of functions with
CONFIG_FRAME_POINTER=y, then backtracing will not work, and arguably
CONFIG_FRAME_POINTER=y is useless there.  In that circumstance, it's
probably better to fail so the user can configure something more
debuggable, rather than having the kernel potentially producing
undebuggable oopses.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH 1/5] bus: arm-cci: use asm unreachable

[Russell King - ARM Linux]

On Wed, Mar 21, 2018 at 12:02:02AM +0100, Stefan Agner wrote:
> Mixing asm and C code is not recommended in a naked function by
> gcc and leads to an error when using clang:
>   drivers/bus/arm-cci.c:2107:2: error: non-ASM statement in naked
>   function is not supported
>         unreachable();
>         ^
> 
> Instead of using the unreachable() macro use the assember variant
> ASM_UNREACHABLE.  This will no longer emit __builtin_unreachable(),
> but since the function is naked and its return type is void it seems
> not to have aversive effects.

I think that unreachable() there is rather silly - this function
*does* return, and the comments say as much.  Just delete the silly
"unreachable()", there's no need to put an ASM_UNREACHABLE in there.

The function is not declared as not returning, and nothing in this
file uses it anyway - it's called from the mcpm code, which also
_does_ expect this function to return (if it doesn't, then we're
basically saying the CPU that called it is dead.)

> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/bus/arm-cci.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 5426c04fe24b..ee9da86fec47 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -2084,6 +2084,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>  
>  "	mov	r0, #0 \n"
>  "	bx	lr \n"
> +	ASM_UNREACHABLE
>  
>  "	.align	2 \n"
>  "5:	.word	cpu_port - . \n"
> @@ -2103,8 +2104,6 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>  	[sizeof_struct_cpu_port] "i" (sizeof(struct cpu_port)),
>  	[sizeof_struct_ace_port] "i" (sizeof(struct cci_ace_port)),
>  	[offsetof_port_phys] "i" (offsetof(struct cci_ace_port, phys)) );
> -
> -	unreachable();
>  }
>  
>  /**
> -- 
> 2.16.2
> 

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH 5/5] ARM: add support for building ARM kernel with clang

[Matthias Kaehlcke]

El Tue, Mar 20, 2018 at 11:18:33PM +0000 Russell King - ARM Linux ha dit:

> On Wed, Mar 21, 2018 at 12:02:06AM +0100, Stefan Agner wrote:
> > Use cc-options call for compiler options which are not available
> > in clang. With this patch an ARMv7 multi platform kernel can be
> > successfully build using clang (tested with version 5.0.1).
> > 
> > Based-on-patches-by: Behan Webster <behanw@converseincode.com>
> > Signed-off-by: Stefan Agner <stefan@agner.ch>

Great to see your work on bringing clang support for 32-bit ARM
upstream!

> > ---
> >  arch/arm/Makefile                 | 2 +-
> >  arch/arm/boot/compressed/Makefile | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> > index e9e3fde3c657..20e9fee1ccc5 100644
> > --- a/arch/arm/Makefile
> > +++ b/arch/arm/Makefile
> > @@ -39,7 +39,7 @@ KBUILD_CFLAGS	+= $(call cc-option,-mno-unaligned-access)
> >  endif
> >  
> >  ifeq ($(CONFIG_FRAME_POINTER),y)
> > -KBUILD_CFLAGS	+=-fno-omit-frame-pointer -mapcs -mno-sched-prolog
> > +KBUILD_CFLAGS	+=-fno-omit-frame-pointer $(call cc-option,-mapcs,) $(call cc-option,-mno-sched-prolog,)
> 
> Some of these options here are to ensure that we generate the following
> code, so we can backtrace:
> 
> 	mov	ip, sp
> 	stmfd	sp!, {fp, ip, lr, pc}
> 	sub	fp, ip, #4
> 
> If clang isn't producing that code at the start of functions with
> CONFIG_FRAME_POINTER=y, then backtracing will not work, and arguably
> CONFIG_FRAME_POINTER=y is useless there.  In that circumstance, it's
> probably better to fail so the user can configure something more
> debuggable, rather than having the kernel potentially producing
> undebuggable oopses.

Which option in particular is important to generate the above code for
backstracing?

According to the gcc doc -mapcs(-frame) is deprecated.

For -mno-sched-prolog the doc says:

"Prevent the reordering of instructions in the function prologue, or
the merging of those instruction with the instructions in the
function’s body. This means that all functions start with a
recognizable set of instructions (or in fact one of a choice from a
small set of different function prologues), and this information can
be used to locate the start of functions inside an executable piece of
code. The default is -msched-prolog."

Thanks

Matthias

Re: [PATCH 1/5] bus: arm-cci: use asm unreachable

[Stefan Agner]

On 21.03.2018 00:30, Russell King - ARM Linux wrote:
> On Wed, Mar 21, 2018 at 12:02:02AM +0100, Stefan Agner wrote:
>> Mixing asm and C code is not recommended in a naked function by
>> gcc and leads to an error when using clang:
>>   drivers/bus/arm-cci.c:2107:2: error: non-ASM statement in naked
>>   function is not supported
>>         unreachable();
>>         ^
>>
>> Instead of using the unreachable() macro use the assember variant
>> ASM_UNREACHABLE.  This will no longer emit __builtin_unreachable(),
>> but since the function is naked and its return type is void it seems
>> not to have aversive effects.
> 
> I think that unreachable() there is rather silly - this function
> *does* return, and the comments say as much.  Just delete the silly
> "unreachable()", there's no need to put an ASM_UNREACHABLE in there.
> 
> The function is not declared as not returning, and nothing in this
> file uses it anyway - it's called from the mcpm code, which also
> _does_ expect this function to return (if it doesn't, then we're
> basically saying the CPU that called it is dead.)
> 

Hm, that makes sense. Will just drop unreachable() in the next revision.

Thanks for reviewing!

--
Stefan

>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/bus/arm-cci.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>> index 5426c04fe24b..ee9da86fec47 100644
>> --- a/drivers/bus/arm-cci.c
>> +++ b/drivers/bus/arm-cci.c
>> @@ -2084,6 +2084,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>>
>>  "	mov	r0, #0 \n"
>>  "	bx	lr \n"
>> +	ASM_UNREACHABLE
>>
>>  "	.align	2 \n"
>>  "5:	.word	cpu_port - . \n"
>> @@ -2103,8 +2104,6 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>>  	[sizeof_struct_cpu_port] "i" (sizeof(struct cpu_port)),
>>  	[sizeof_struct_ace_port] "i" (sizeof(struct cci_ace_port)),
>>  	[offsetof_port_phys] "i" (offsetof(struct cci_ace_port, phys)) );
>> -
>> -	unreachable();
>>  }
>>
>>  /**
>> --
>> 2.16.2
>>

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

[Stefan Agner]

On 21.03.2018 00:13, Russell King - ARM Linux wrote:
> On Wed, Mar 21, 2018 at 12:02:04AM +0100, 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
>> placement.
> 
> Those checks have nothing to do with that at all.  The whole point of
> __asmeq() is to catch situations where you use register variables,
> specifying which register you want them in, and GCC then ends up
> passing them to assembly code in some other random register(s).
> 
> This was found with older GCCs, and the problem was fixed.  It has
> nothing to do with naked functions per se.
> 

Ok, will reword that part to something like:

As documented in GCC naked functions should only use Basic asm
syntax. The Extended asm or mixture of Basic asm and "C" code cannot
be depended upon.

Furthermore with clang using parameters in Extended asm in a
naked function is not supported:
...

> In fact, as you're introducing further register variables, these
> checks become more important to have than they were with the
> previous code.

Ok I see, so I definitely have to leave them in.

You generally agree with the change otherwise?

--
Stefan

Re: [PATCH 5/5] ARM: add support for building ARM kernel with clang

[Stefan Agner]

On 21.03.2018 01:20, Matthias Kaehlcke wrote:
> El Tue, Mar 20, 2018 at 11:18:33PM +0000 Russell King - ARM Linux ha dit:
> 
>> On Wed, Mar 21, 2018 at 12:02:06AM +0100, Stefan Agner wrote:
>> > Use cc-options call for compiler options which are not available
>> > in clang. With this patch an ARMv7 multi platform kernel can be
>> > successfully build using clang (tested with version 5.0.1).
>> >
>> > Based-on-patches-by: Behan Webster <behanw@converseincode.com>
>> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> 
> Great to see your work on bringing clang support for 32-bit ARM
> upstream!
> 
>> > ---
>> >  arch/arm/Makefile                 | 2 +-
>> >  arch/arm/boot/compressed/Makefile | 2 +-
>> >  2 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/arch/arm/Makefile b/arch/arm/Makefile
>> > index e9e3fde3c657..20e9fee1ccc5 100644
>> > --- a/arch/arm/Makefile
>> > +++ b/arch/arm/Makefile
>> > @@ -39,7 +39,7 @@ KBUILD_CFLAGS	+= $(call cc-option,-mno-unaligned-access)
>> >  endif
>> >
>> >  ifeq ($(CONFIG_FRAME_POINTER),y)
>> > -KBUILD_CFLAGS	+=-fno-omit-frame-pointer -mapcs -mno-sched-prolog
>> > +KBUILD_CFLAGS	+=-fno-omit-frame-pointer $(call cc-option,-mapcs,) $(call cc-option,-mno-sched-prolog,)
>>
>> Some of these options here are to ensure that we generate the following
>> code, so we can backtrace:
>>
>> 	mov	ip, sp
>> 	stmfd	sp!, {fp, ip, lr, pc}
>> 	sub	fp, ip, #4
>>
>> If clang isn't producing that code at the start of functions with
>> CONFIG_FRAME_POINTER=y, then backtracing will not work, and arguably
>> CONFIG_FRAME_POINTER=y is useless there.  In that circumstance, it's
>> probably better to fail so the user can configure something more
>> debuggable, rather than having the kernel potentially producing
>> undebuggable oopses.

With clang and -fno-omit-frame-pointer function prologue looks something
like this:

   0:   e92d4ff0        push    {r4, r5, r6, r7, r8, r9, sl, fp, lr}    
                                                                   
   4:   e28db01c        add     fp, sp, #28
...

This bug seems to be related:
https://bugs.llvm.org/show_bug.cgi?id=18505 

It seems that LLVM/clang does not plan to add APCS format/gcc
interoperability for the same frame layout.

I guess it would be possible to support the LLVM/clang frame layout?

But until then, I am with Russel here: Better just let clang fail on
CONFIG_FRAME_POINTER=y.

Most configs probably anyway use CONFIG_ARM_UNWIND. At least
multi_v7_defconfig does.

> 
> Which option in particular is important to generate the above code for
> backstracing?

For gcc, I guess really all of them.

--
Stefan

> 
> According to the gcc doc -mapcs(-frame) is deprecated.
> 
> For -mno-sched-prolog the doc says:
> 
> "Prevent the reordering of instructions in the function prologue, or
> the merging of those instruction with the instructions in the
> function’s body. This means that all functions start with a
> recognizable set of instructions (or in fact one of a choice from a
> small set of different function prologues), and this information can
> be used to locate the start of functions inside an executable piece of
> code. The default is -msched-prolog."
> 
> Thanks
> 
> Matthias

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

[Robin Murphy]

On 20/03/18 23:02, 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
> placement.
> 
> 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
> bcm_kona_smc.c.
> 
> Additionally also make sure all callee-saved registers get saved
> as it has been done before.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>   arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
> index 3fb1b5a1dce9..426d732e6591 100644
> --- a/arch/arm/firmware/trusted_foundations.c
> +++ b/arch/arm/firmware/trusted_foundations.c
> @@ -31,21 +31,23 @@
>   
>   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"
>   		__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}"
>   		:
> -		: "r" (type), "r" (arg1), "r" (arg2)
> -		: "memory");
> +		: "r" (r0), "r" (r1), "r" (r2)
> +		: "memory", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10");

I may be missing a subtlety, but it looks like we no longer have a 
guarantee that r11 will be caller-saved as it was previously. I don't 
know the Trusted Foundations ABI to say whether that matters or not, but 
if it is the case that it never needed preserving anyway, that might be 
worth calling out in the commit message.

Robin.

>   }
>   
>   static int tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
> 

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

[Stefan Agner]

On 21.03.2018 13:13, Robin Murphy wrote:
> On 20/03/18 23:02, 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
>> placement.
>>
>> 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
>> bcm_kona_smc.c.
>>
>> Additionally also make sure all callee-saved registers get saved
>> as it has been done before.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>   arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
>> index 3fb1b5a1dce9..426d732e6591 100644
>> --- a/arch/arm/firmware/trusted_foundations.c
>> +++ b/arch/arm/firmware/trusted_foundations.c
>> @@ -31,21 +31,23 @@
>>     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"
>>   		__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}"
>>   		:
>> -		: "r" (type), "r" (arg1), "r" (arg2)
>> -		: "memory");
>> +		: "r" (r0), "r" (r1), "r" (r2)
>> +		: "memory", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10");
> 
> I may be missing a subtlety, but it looks like we no longer have a
> guarantee that r11 will be caller-saved as it was previously. I don't
> know the Trusted Foundations ABI to say whether that matters or not,
> but if it is the case that it never needed preserving anyway, that
> might be worth calling out in the commit message.

Adding r11 (fp) to the clobber list causes an error when using gcc and
CONFIG_FRAME_POINTER=y:
arch/arm/firmware/trusted_foundations.c: In function ‘tf_generic_smc’:
arch/arm/firmware/trusted_foundations.c:51:1: error: fp cannot be used
in asm here

Not sure what ABI Trusted Foundations follow.

[adding Stephen, Thierry and Dmitry]
Maybe someone more familiar with NVIDIA Tegra SoCs can help?

When CONFIG_FRAME_POINTER=y fp gets saved anyway. So we could add r11 to
clobber list ifndef CONFIG_FRAME_POINTER...

--
Stefan

> 
> Robin.
> 
>>   }
>>     static int tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
>>

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

[Dmitry Osipenko]

On 21.03.2018 17:09, Stefan Agner wrote:
> On 21.03.2018 13:13, Robin Murphy wrote:
>> On 20/03/18 23:02, 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
>>> placement.
>>>
>>> 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
>>> bcm_kona_smc.c.
>>>
>>> Additionally also make sure all callee-saved registers get saved
>>> as it has been done before.
>>>
>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>> ---
>>>   arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
>>> index 3fb1b5a1dce9..426d732e6591 100644
>>> --- a/arch/arm/firmware/trusted_foundations.c
>>> +++ b/arch/arm/firmware/trusted_foundations.c
>>> @@ -31,21 +31,23 @@
>>>     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"
>>>   		__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}"
>>>   		:
>>> -		: "r" (type), "r" (arg1), "r" (arg2)
>>> -		: "memory");
>>> +		: "r" (r0), "r" (r1), "r" (r2)
>>> +		: "memory", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10");
>>
>> I may be missing a subtlety, but it looks like we no longer have a
>> guarantee that r11 will be caller-saved as it was previously. I don't
>> know the Trusted Foundations ABI to say whether that matters or not,
>> but if it is the case that it never needed preserving anyway, that
>> might be worth calling out in the commit message.
> 
> Adding r11 (fp) to the clobber list causes an error when using gcc and
> CONFIG_FRAME_POINTER=y:
> arch/arm/firmware/trusted_foundations.c: In function ‘tf_generic_smc’:
> arch/arm/firmware/trusted_foundations.c:51:1: error: fp cannot be used
> in asm here
> 
> Not sure what ABI Trusted Foundations follow.
> 
> [adding Stephen, Thierry and Dmitry]
> Maybe someone more familiar with NVIDIA Tegra SoCs can help?
> 
> When CONFIG_FRAME_POINTER=y fp gets saved anyway. So we could add r11 to
> clobber list ifndef CONFIG_FRAME_POINTER...

I have no idea about TF ABI either. Looking at the downstream kernel code, r4 -
r12 should be saved. I've CC'd Alexandre as he is the author of the original
patch and may still remember the details.

I'm also wondering why original code doesn't have r3 in the clobber list and why
r3 is set to '0', downstream sets it to the address of SP and on return from SMC
r3 contains the address of SP which should be restored. I'm now wondering how
SMC calling worked for me at all on T30, maybe it didn't..

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

[Stephen Warren]

On 03/21/2018 09:26 AM, Dmitry Osipenko wrote:
> On 21.03.2018 17:09, Stefan Agner wrote:
>> On 21.03.2018 13:13, Robin Murphy wrote:
>>> On 20/03/18 23:02, 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
>>>> placement.
>>>>
>>>> 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
>>>> bcm_kona_smc.c.
>>>>
>>>> Additionally also make sure all callee-saved registers get saved
>>>> as it has been done before.
>>>>
>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>> ---
>>>>    arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
>>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
>>>> index 3fb1b5a1dce9..426d732e6591 100644
>>>> --- a/arch/arm/firmware/trusted_foundations.c
>>>> +++ b/arch/arm/firmware/trusted_foundations.c
>>>> @@ -31,21 +31,23 @@
>>>>      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"
>>>>    		__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}"
>>>>    		:
>>>> -		: "r" (type), "r" (arg1), "r" (arg2)
>>>> -		: "memory");
>>>> +		: "r" (r0), "r" (r1), "r" (r2)
>>>> +		: "memory", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10");
>>>
>>> I may be missing a subtlety, but it looks like we no longer have a
>>> guarantee that r11 will be caller-saved as it was previously. I don't
>>> know the Trusted Foundations ABI to say whether that matters or not,
>>> but if it is the case that it never needed preserving anyway, that
>>> might be worth calling out in the commit message.
>>
>> Adding r11 (fp) to the clobber list causes an error when using gcc and
>> CONFIG_FRAME_POINTER=y:
>> arch/arm/firmware/trusted_foundations.c: In function ‘tf_generic_smc’:
>> arch/arm/firmware/trusted_foundations.c:51:1: error: fp cannot be used
>> in asm here
>>
>> Not sure what ABI Trusted Foundations follow.
>>
>> [adding Stephen, Thierry and Dmitry]
>> Maybe someone more familiar with NVIDIA Tegra SoCs can help?
>>
>> When CONFIG_FRAME_POINTER=y fp gets saved anyway. So we could add r11 to
>> clobber list ifndef CONFIG_FRAME_POINTER...
> 
> I have no idea about TF ABI either. Looking at the downstream kernel code, r4 -
> r12 should be saved. I've CC'd Alexandre as he is the author of the original
> patch and may still remember the details.
> 
> I'm also wondering why original code doesn't have r3 in the clobber list and why
> r3 is set to '0', downstream sets it to the address of SP and on return from SMC
> r3 contains the address of SP which should be restored. I'm now wondering how
> SMC calling worked for me at all on T30, maybe it didn't..

I don't know what the ABI for ATF is. I assume it's documented in the 
ATF, PSCI, or similar specification, or ATF source code. Hence, I don't 
know whether ATF restores fp/r11.

My guess is that r3/r4 are set to 0 because they're defined as inputs by 
the SMC/ATF ABI, yet nothing the kernel does needed that many 
parameters, so they're hard-coded to 0 (to ensure they're set to 
something predictable) rather than also being parameters to 
tf_generic_smc().

The original code used to save/restore a lot of registers, including 
r11/fp. Can't we side-step the issue of including/not-including r11/fp 
in the clobber list by not removing those stmfd/ldmfd assembly instructions?

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

[Robin Murphy]

On 21/03/18 16:40, Stephen Warren wrote:
> On 03/21/2018 09:26 AM, Dmitry Osipenko wrote:
>> On 21.03.2018 17:09, Stefan Agner wrote:
>>> On 21.03.2018 13:13, Robin Murphy wrote:
>>>> On 20/03/18 23:02, 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
>>>>> placement.
>>>>>
>>>>> 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
>>>>> bcm_kona_smc.c.
>>>>>
>>>>> Additionally also make sure all callee-saved registers get saved
>>>>> as it has been done before.
>>>>>
>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>> ---
>>>>>    arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
>>>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c 
>>>>> b/arch/arm/firmware/trusted_foundations.c
>>>>> index 3fb1b5a1dce9..426d732e6591 100644
>>>>> --- a/arch/arm/firmware/trusted_foundations.c
>>>>> +++ b/arch/arm/firmware/trusted_foundations.c
>>>>> @@ -31,21 +31,23 @@
>>>>>      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"
>>>>>            __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}"
>>>>>            :
>>>>> -        : "r" (type), "r" (arg1), "r" (arg2)
>>>>> -        : "memory");
>>>>> +        : "r" (r0), "r" (r1), "r" (r2)
>>>>> +        : "memory", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10");
>>>>
>>>> I may be missing a subtlety, but it looks like we no longer have a
>>>> guarantee that r11 will be caller-saved as it was previously. I don't
>>>> know the Trusted Foundations ABI to say whether that matters or not,
>>>> but if it is the case that it never needed preserving anyway, that
>>>> might be worth calling out in the commit message.
>>>
>>> Adding r11 (fp) to the clobber list causes an error when using gcc and
>>> CONFIG_FRAME_POINTER=y:
>>> arch/arm/firmware/trusted_foundations.c: In function ‘tf_generic_smc’:
>>> arch/arm/firmware/trusted_foundations.c:51:1: error: fp cannot be used
>>> in asm here
>>>
>>> Not sure what ABI Trusted Foundations follow.
>>>
>>> [adding Stephen, Thierry and Dmitry]
>>> Maybe someone more familiar with NVIDIA Tegra SoCs can help?
>>>
>>> When CONFIG_FRAME_POINTER=y fp gets saved anyway. So we could add r11 to
>>> clobber list ifndef CONFIG_FRAME_POINTER...
>>
>> I have no idea about TF ABI either. Looking at the downstream kernel 
>> code, r4 -
>> r12 should be saved. I've CC'd Alexandre as he is the author of the 
>> original
>> patch and may still remember the details.
>>
>> I'm also wondering why original code doesn't have r3 in the clobber 
>> list and why
>> r3 is set to '0', downstream sets it to the address of SP and on 
>> return from SMC
>> r3 contains the address of SP which should be restored. I'm now 
>> wondering how
>> SMC calling worked for me at all on T30, maybe it didn't..
> 
> I don't know what the ABI for ATF is. I assume it's documented in the 
> ATF, PSCI, or similar specification, or ATF source code. Hence, I don't 
> know whether ATF restores fp/r11.

Oops, I think we're starting to diverge here - "ATF" (as in "Arm Trusted 
Firmware") does implement the ARM SMCCC, which more or less just follows 
the regular procedure call standard in terms of register saving. The 
"TF" in question here is "Trusted Foundations" from Trusted Logic (who 
apparently don't exist any more) which is explicitly called out in the 
header as having its own nonstandard calling convention. I guess newer 
Tegras are using the former, whereas the older ones used the latter.

> My guess is that r3/r4 are set to 0 because they're defined as inputs by 
> the SMC/ATF ABI, yet nothing the kernel does needed that many 
> parameters, so they're hard-coded to 0 (to ensure they're set to 
> something predictable) rather than also being parameters to 
> tf_generic_smc().
> 
> The original code used to save/restore a lot of registers, including 
> r11/fp. Can't we side-step the issue of including/not-including r11/fp 
> in the clobber list by not removing those stmfd/ldmfd assembly 
> instructions?

That might be reasonable - fiddling with a C function's stack inside an 
asm is a bit grim, but for this case I can't see that it would mess with 
unwinding etc. or otherwise go wrong any more than the existing code, 
and I doubt the slight efficiency hit from having to change the "pop the 
LR straight into the PC" idiom matters much.

Robin.

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

[Stefan Agner]

On 21.03.2018 18:16, Robin Murphy wrote:
> On 21/03/18 16:40, Stephen Warren wrote:
>> On 03/21/2018 09:26 AM, Dmitry Osipenko wrote:
>>> On 21.03.2018 17:09, Stefan Agner wrote:
>>>> On 21.03.2018 13:13, Robin Murphy wrote:
>>>>> On 20/03/18 23:02, 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
>>>>>> placement.
>>>>>>
>>>>>> 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
>>>>>> bcm_kona_smc.c.
>>>>>>
>>>>>> Additionally also make sure all callee-saved registers get saved
>>>>>> as it has been done before.
>>>>>>
>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>>> ---
>>>>>>    arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
>>>>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
>>>>>> index 3fb1b5a1dce9..426d732e6591 100644
>>>>>> --- a/arch/arm/firmware/trusted_foundations.c
>>>>>> +++ b/arch/arm/firmware/trusted_foundations.c
>>>>>> @@ -31,21 +31,23 @@
>>>>>>      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"
>>>>>>            __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}"
>>>>>>            :
>>>>>> -        : "r" (type), "r" (arg1), "r" (arg2)
>>>>>> -        : "memory");
>>>>>> +        : "r" (r0), "r" (r1), "r" (r2)
>>>>>> +        : "memory", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10");
>>>>>
>>>>> I may be missing a subtlety, but it looks like we no longer have a
>>>>> guarantee that r11 will be caller-saved as it was previously. I don't
>>>>> know the Trusted Foundations ABI to say whether that matters or not,
>>>>> but if it is the case that it never needed preserving anyway, that
>>>>> might be worth calling out in the commit message.
>>>>
>>>> Adding r11 (fp) to the clobber list causes an error when using gcc and
>>>> CONFIG_FRAME_POINTER=y:
>>>> arch/arm/firmware/trusted_foundations.c: In function ‘tf_generic_smc’:
>>>> arch/arm/firmware/trusted_foundations.c:51:1: error: fp cannot be used
>>>> in asm here
>>>>
>>>> Not sure what ABI Trusted Foundations follow.
>>>>
>>>> [adding Stephen, Thierry and Dmitry]
>>>> Maybe someone more familiar with NVIDIA Tegra SoCs can help?
>>>>
>>>> When CONFIG_FRAME_POINTER=y fp gets saved anyway. So we could add r11 to
>>>> clobber list ifndef CONFIG_FRAME_POINTER...
>>>
>>> I have no idea about TF ABI either. Looking at the downstream kernel code, r4 -
>>> r12 should be saved. I've CC'd Alexandre as he is the author of the original
>>> patch and may still remember the details.
>>>
>>> I'm also wondering why original code doesn't have r3 in the clobber list and why
>>> r3 is set to '0', downstream sets it to the address of SP and on return from SMC
>>> r3 contains the address of SP which should be restored. I'm now wondering how
>>> SMC calling worked for me at all on T30, maybe it didn't..
>>
>> I don't know what the ABI for ATF is. I assume it's documented in the ATF, PSCI, or similar specification, or ATF source code. Hence, I don't know whether ATF restores fp/r11.
> 
> Oops, I think we're starting to diverge here - "ATF" (as in "Arm
> Trusted Firmware") does implement the ARM SMCCC, which more or less
> just follows the regular procedure call standard in terms of register
> saving. The "TF" in question here is "Trusted Foundations" from
> Trusted Logic (who apparently don't exist any more) which is
> explicitly called out in the header as having its own nonstandard
> calling convention. I guess newer Tegras are using the former, whereas
> the older ones used the latter.
> 

What do you mean by "called out in the header as having its own
nonstandard"?

It is unclear what ABI is used, I just inferred from the fact that
register have been saved before that it might use a nonstandard calling
convention. 

Tegra 4i/TK1 and newer seem to use something called Trusted Little
Kernel.

>> My guess is that r3/r4 are set to 0 because they're defined as inputs by the SMC/ATF ABI, yet nothing the kernel does needed that many parameters, so they're hard-coded to 0 (to ensure they're set to something predictable) rather than also being parameters to tf_generic_smc().
>>
>> The original code used to save/restore a lot of registers, including r11/fp. Can't we side-step the issue of including/not-including r11/fp in the clobber list by not removing those stmfd/ldmfd assembly instructions?
> 
> That might be reasonable - fiddling with a C function's stack inside
> an asm is a bit grim, but for this case I can't see that it would mess
> with unwinding etc. or otherwise go wrong any more than the existing
> code, and I doubt the slight efficiency hit from having to change the 
> "pop the LR straight into the PC" idiom matters much.

Sounds reasonable, I guess in that case we can also omit all the
additional register in the clobber list.

--
Stefan

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

[Robin Murphy]

On 21/03/18 21:41, Stefan Agner wrote:
> On 21.03.2018 18:16, Robin Murphy wrote:
>> On 21/03/18 16:40, Stephen Warren wrote:
>>> On 03/21/2018 09:26 AM, Dmitry Osipenko wrote:
>>>> On 21.03.2018 17:09, Stefan Agner wrote:
>>>>> On 21.03.2018 13:13, Robin Murphy wrote:
>>>>>> On 20/03/18 23:02, 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
>>>>>>> placement.
>>>>>>>
>>>>>>> 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
>>>>>>> bcm_kona_smc.c.
>>>>>>>
>>>>>>> Additionally also make sure all callee-saved registers get saved
>>>>>>> as it has been done before.
>>>>>>>
>>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>>>> ---
>>>>>>>     arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
>>>>>>>     1 file changed, 7 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
>>>>>>> index 3fb1b5a1dce9..426d732e6591 100644
>>>>>>> --- a/arch/arm/firmware/trusted_foundations.c
>>>>>>> +++ b/arch/arm/firmware/trusted_foundations.c
>>>>>>> @@ -31,21 +31,23 @@
>>>>>>>       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"
>>>>>>>             __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}"
>>>>>>>             :
>>>>>>> -        : "r" (type), "r" (arg1), "r" (arg2)
>>>>>>> -        : "memory");
>>>>>>> +        : "r" (r0), "r" (r1), "r" (r2)
>>>>>>> +        : "memory", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10");
>>>>>>
>>>>>> I may be missing a subtlety, but it looks like we no longer have a
>>>>>> guarantee that r11 will be caller-saved as it was previously. I don't
>>>>>> know the Trusted Foundations ABI to say whether that matters or not,
>>>>>> but if it is the case that it never needed preserving anyway, that
>>>>>> might be worth calling out in the commit message.
>>>>>
>>>>> Adding r11 (fp) to the clobber list causes an error when using gcc and
>>>>> CONFIG_FRAME_POINTER=y:
>>>>> arch/arm/firmware/trusted_foundations.c: In function ‘tf_generic_smc’:
>>>>> arch/arm/firmware/trusted_foundations.c:51:1: error: fp cannot be used
>>>>> in asm here
>>>>>
>>>>> Not sure what ABI Trusted Foundations follow.
>>>>>
>>>>> [adding Stephen, Thierry and Dmitry]
>>>>> Maybe someone more familiar with NVIDIA Tegra SoCs can help?
>>>>>
>>>>> When CONFIG_FRAME_POINTER=y fp gets saved anyway. So we could add r11 to
>>>>> clobber list ifndef CONFIG_FRAME_POINTER...
>>>>
>>>> I have no idea about TF ABI either. Looking at the downstream kernel code, r4 -
>>>> r12 should be saved. I've CC'd Alexandre as he is the author of the original
>>>> patch and may still remember the details.
>>>>
>>>> I'm also wondering why original code doesn't have r3 in the clobber list and why
>>>> r3 is set to '0', downstream sets it to the address of SP and on return from SMC
>>>> r3 contains the address of SP which should be restored. I'm now wondering how
>>>> SMC calling worked for me at all on T30, maybe it didn't..
>>>
>>> I don't know what the ABI for ATF is. I assume it's documented in the ATF, PSCI, or similar specification, or ATF source code. Hence, I don't know whether ATF restores fp/r11.
>>
>> Oops, I think we're starting to diverge here - "ATF" (as in "Arm
>> Trusted Firmware") does implement the ARM SMCCC, which more or less
>> just follows the regular procedure call standard in terms of register
>> saving. The "TF" in question here is "Trusted Foundations" from
>> Trusted Logic (who apparently don't exist any more) which is
>> explicitly called out in the header as having its own nonstandard
>> calling convention. I guess newer Tegras are using the former, whereas
>> the older ones used the latter.
>>
> 
> What do you mean by "called out in the header as having its own
> nonstandard"?

Specifically, the comment in arch/arm/include/asm/trusted_foundations.h 
which says:

   "The calls are completely specific to Trusted Foundations, and do
    *not* follow the SMC calling convention or the PSCI standard."

> It is unclear what ABI is used, I just inferred from the fact that
> register have been saved before that it might use a nonstandard calling
> convention.
> 
> Tegra 4i/TK1 and newer seem to use something called Trusted Little
> Kernel.
> 
>>> My guess is that r3/r4 are set to 0 because they're defined as inputs by the SMC/ATF ABI, yet nothing the kernel does needed that many parameters, so they're hard-coded to 0 (to ensure they're set to something predictable) rather than also being parameters to tf_generic_smc().
>>>
>>> The original code used to save/restore a lot of registers, including r11/fp. Can't we side-step the issue of including/not-including r11/fp in the clobber list by not removing those stmfd/ldmfd assembly instructions?
>>
>> That might be reasonable - fiddling with a C function's stack inside
>> an asm is a bit grim, but for this case I can't see that it would mess
>> with unwinding etc. or otherwise go wrong any more than the existing
>> code, and I doubt the slight efficiency hit from having to change the
>> "pop the LR straight into the PC" idiom matters much.
> 
> Sounds reasonable, I guess in that case we can also omit all the
> additional register in the clobber list.

Yeah, you should only need to specify clobbers for any registers which 
are neither used as arguments nor explicitly preserved - looking at the 
layout of the code, it seems unlikely that the compiler would have 
anything live in r3 or r12 across the call (since the scope for inlining 
is pretty trivial), but there's no harm in being strictly correct :)

Robin.

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

[Stefan Agner]

On 22.03.2018 12:48, Robin Murphy wrote:
> On 21/03/18 21:41, Stefan Agner wrote:
>> On 21.03.2018 18:16, Robin Murphy wrote:
>>> On 21/03/18 16:40, Stephen Warren wrote:
>>>> On 03/21/2018 09:26 AM, Dmitry Osipenko wrote:
>>>>> On 21.03.2018 17:09, Stefan Agner wrote:
>>>>>> On 21.03.2018 13:13, Robin Murphy wrote:
>>>>>>> On 20/03/18 23:02, 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
>>>>>>>> placement.
>>>>>>>>
>>>>>>>> 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
>>>>>>>> bcm_kona_smc.c.
>>>>>>>>
>>>>>>>> Additionally also make sure all callee-saved registers get saved
>>>>>>>> as it has been done before.
>>>>>>>>
>>>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>>>>> ---
>>>>>>>>     arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
>>>>>>>>     1 file changed, 7 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
>>>>>>>> index 3fb1b5a1dce9..426d732e6591 100644
>>>>>>>> --- a/arch/arm/firmware/trusted_foundations.c
>>>>>>>> +++ b/arch/arm/firmware/trusted_foundations.c
>>>>>>>> @@ -31,21 +31,23 @@
>>>>>>>>       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"
>>>>>>>>             __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}"
>>>>>>>>             :
>>>>>>>> -        : "r" (type), "r" (arg1), "r" (arg2)
>>>>>>>> -        : "memory");
>>>>>>>> +        : "r" (r0), "r" (r1), "r" (r2)
>>>>>>>> +        : "memory", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10");
>>>>>>>
>>>>>>> I may be missing a subtlety, but it looks like we no longer have a
>>>>>>> guarantee that r11 will be caller-saved as it was previously. I don't
>>>>>>> know the Trusted Foundations ABI to say whether that matters or not,
>>>>>>> but if it is the case that it never needed preserving anyway, that
>>>>>>> might be worth calling out in the commit message.
>>>>>>
>>>>>> Adding r11 (fp) to the clobber list causes an error when using gcc and
>>>>>> CONFIG_FRAME_POINTER=y:
>>>>>> arch/arm/firmware/trusted_foundations.c: In function ‘tf_generic_smc’:
>>>>>> arch/arm/firmware/trusted_foundations.c:51:1: error: fp cannot be used
>>>>>> in asm here
>>>>>>
>>>>>> Not sure what ABI Trusted Foundations follow.
>>>>>>
>>>>>> [adding Stephen, Thierry and Dmitry]
>>>>>> Maybe someone more familiar with NVIDIA Tegra SoCs can help?
>>>>>>
>>>>>> When CONFIG_FRAME_POINTER=y fp gets saved anyway. So we could add r11 to
>>>>>> clobber list ifndef CONFIG_FRAME_POINTER...
>>>>>
>>>>> I have no idea about TF ABI either. Looking at the downstream kernel code, r4 -
>>>>> r12 should be saved. I've CC'd Alexandre as he is the author of the original
>>>>> patch and may still remember the details.
>>>>>
>>>>> I'm also wondering why original code doesn't have r3 in the clobber list and why
>>>>> r3 is set to '0', downstream sets it to the address of SP and on return from SMC
>>>>> r3 contains the address of SP which should be restored. I'm now wondering how
>>>>> SMC calling worked for me at all on T30, maybe it didn't..
>>>>
>>>> I don't know what the ABI for ATF is. I assume it's documented in the ATF, PSCI, or similar specification, or ATF source code. Hence, I don't know whether ATF restores fp/r11.
>>>
>>> Oops, I think we're starting to diverge here - "ATF" (as in "Arm
>>> Trusted Firmware") does implement the ARM SMCCC, which more or less
>>> just follows the regular procedure call standard in terms of register
>>> saving. The "TF" in question here is "Trusted Foundations" from
>>> Trusted Logic (who apparently don't exist any more) which is
>>> explicitly called out in the header as having its own nonstandard
>>> calling convention. I guess newer Tegras are using the former, whereas
>>> the older ones used the latter.
>>>
>>
>> What do you mean by "called out in the header as having its own
>> nonstandard"?
> 
> Specifically, the comment in
> arch/arm/include/asm/trusted_foundations.h which says:
> 
>   "The calls are completely specific to Trusted Foundations, and do
>    *not* follow the SMC calling convention or the PSCI standard."
> 

Oh didn't notice that. Thanks for pointing out.

>> It is unclear what ABI is used, I just inferred from the fact that
>> register have been saved before that it might use a nonstandard calling
>> convention.
>>
>> Tegra 4i/TK1 and newer seem to use something called Trusted Little
>> Kernel.
>>
>>>> My guess is that r3/r4 are set to 0 because they're defined as inputs by the SMC/ATF ABI, yet nothing the kernel does needed that many parameters, so they're hard-coded to 0 (to ensure they're set to something predictable) rather than also being parameters to tf_generic_smc().
>>>>
>>>> The original code used to save/restore a lot of registers, including r11/fp. Can't we side-step the issue of including/not-including r11/fp in the clobber list by not removing those stmfd/ldmfd assembly instructions?
>>>
>>> That might be reasonable - fiddling with a C function's stack inside
>>> an asm is a bit grim, but for this case I can't see that it would mess
>>> with unwinding etc. or otherwise go wrong any more than the existing
>>> code, and I doubt the slight efficiency hit from having to change the
>>> "pop the LR straight into the PC" idiom matters much.
>>
>> Sounds reasonable, I guess in that case we can also omit all the
>> additional register in the clobber list.
> 
> Yeah, you should only need to specify clobbers for any registers which
> are neither used as arguments nor explicitly preserved - looking at
> the layout of the code, it seems unlikely that the compiler would have
> anything live in r3 or r12 across the call (since the scope for
> inlining is pretty trivial), but there's no harm in being strictly
> correct :)

So something like this?

-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"
         __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}"
         :
-        : "r" (type), "r" (arg1), "r" (arg2)
-        : "memory");
+        : "r" (r0), "r" (r1), "r" (r2)
+        : "memory", "r3");
 }


--
Stefan

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

[Dmitry Osipenko]

On 22.03.2018 15:43, Stefan Agner wrote:
> On 22.03.2018 12:48, Robin Murphy wrote:
>> On 21/03/18 21:41, Stefan Agner wrote:
>>> On 21.03.2018 18:16, Robin Murphy wrote:
>>>> On 21/03/18 16:40, Stephen Warren wrote:
>>>>> On 03/21/2018 09:26 AM, Dmitry Osipenko wrote:
>>>>>> On 21.03.2018 17:09, Stefan Agner wrote:
>>>>>>> On 21.03.2018 13:13, Robin Murphy wrote:
>>>>>>>> On 20/03/18 23:02, 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
>>>>>>>>> placement.
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>> bcm_kona_smc.c.
>>>>>>>>>
>>>>>>>>> Additionally also make sure all callee-saved registers get saved
>>>>>>>>> as it has been done before.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>>>>>> ---
>>>>>>>>>     arch/arm/firmware/trusted_foundations.c | 12 +++++++-----
>>>>>>>>>     1 file changed, 7 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
>>>>>>>>> index 3fb1b5a1dce9..426d732e6591 100644
>>>>>>>>> --- a/arch/arm/firmware/trusted_foundations.c
>>>>>>>>> +++ b/arch/arm/firmware/trusted_foundations.c
>>>>>>>>> @@ -31,21 +31,23 @@
>>>>>>>>>       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"
>>>>>>>>>             __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}"
>>>>>>>>>             :
>>>>>>>>> -        : "r" (type), "r" (arg1), "r" (arg2)
>>>>>>>>> -        : "memory");
>>>>>>>>> +        : "r" (r0), "r" (r1), "r" (r2)
>>>>>>>>> +        : "memory", "r3", "r4", "r5", "r6", "r7", "r8", "r9", "r10");
>>>>>>>>
>>>>>>>> I may be missing a subtlety, but it looks like we no longer have a
>>>>>>>> guarantee that r11 will be caller-saved as it was previously. I don't
>>>>>>>> know the Trusted Foundations ABI to say whether that matters or not,
>>>>>>>> but if it is the case that it never needed preserving anyway, that
>>>>>>>> might be worth calling out in the commit message.
>>>>>>>
>>>>>>> Adding r11 (fp) to the clobber list causes an error when using gcc and
>>>>>>> CONFIG_FRAME_POINTER=y:
>>>>>>> arch/arm/firmware/trusted_foundations.c: In function ‘tf_generic_smc’:
>>>>>>> arch/arm/firmware/trusted_foundations.c:51:1: error: fp cannot be used
>>>>>>> in asm here
>>>>>>>
>>>>>>> Not sure what ABI Trusted Foundations follow.
>>>>>>>
>>>>>>> [adding Stephen, Thierry and Dmitry]
>>>>>>> Maybe someone more familiar with NVIDIA Tegra SoCs can help?
>>>>>>>
>>>>>>> When CONFIG_FRAME_POINTER=y fp gets saved anyway. So we could add r11 to
>>>>>>> clobber list ifndef CONFIG_FRAME_POINTER...
>>>>>>
>>>>>> I have no idea about TF ABI either. Looking at the downstream kernel code, r4 -
>>>>>> r12 should be saved. I've CC'd Alexandre as he is the author of the original
>>>>>> patch and may still remember the details.
>>>>>>
>>>>>> I'm also wondering why original code doesn't have r3 in the clobber list and why
>>>>>> r3 is set to '0', downstream sets it to the address of SP and on return from SMC
>>>>>> r3 contains the address of SP which should be restored. I'm now wondering how
>>>>>> SMC calling worked for me at all on T30, maybe it didn't..
>>>>>
>>>>> I don't know what the ABI for ATF is. I assume it's documented in the ATF, PSCI, or similar specification, or ATF source code. Hence, I don't know whether ATF restores fp/r11.
>>>>
>>>> Oops, I think we're starting to diverge here - "ATF" (as in "Arm
>>>> Trusted Firmware") does implement the ARM SMCCC, which more or less
>>>> just follows the regular procedure call standard in terms of register
>>>> saving. The "TF" in question here is "Trusted Foundations" from
>>>> Trusted Logic (who apparently don't exist any more) which is
>>>> explicitly called out in the header as having its own nonstandard
>>>> calling convention. I guess newer Tegras are using the former, whereas
>>>> the older ones used the latter.
>>>>
>>>
>>> What do you mean by "called out in the header as having its own
>>> nonstandard"?
>>
>> Specifically, the comment in
>> arch/arm/include/asm/trusted_foundations.h which says:
>>
>>   "The calls are completely specific to Trusted Foundations, and do
>>    *not* follow the SMC calling convention or the PSCI standard."
>>
> 
> Oh didn't notice that. Thanks for pointing out.
> 
>>> It is unclear what ABI is used, I just inferred from the fact that
>>> register have been saved before that it might use a nonstandard calling
>>> convention.
>>>
>>> Tegra 4i/TK1 and newer seem to use something called Trusted Little
>>> Kernel.
>>>
>>>>> My guess is that r3/r4 are set to 0 because they're defined as inputs by the SMC/ATF ABI, yet nothing the kernel does needed that many parameters, so they're hard-coded to 0 (to ensure they're set to something predictable) rather than also being parameters to tf_generic_smc().
>>>>>
>>>>> The original code used to save/restore a lot of registers, including r11/fp. Can't we side-step the issue of including/not-including r11/fp in the clobber list by not removing those stmfd/ldmfd assembly instructions?
>>>>
>>>> That might be reasonable - fiddling with a C function's stack inside
>>>> an asm is a bit grim, but for this case I can't see that it would mess
>>>> with unwinding etc. or otherwise go wrong any more than the existing
>>>> code, and I doubt the slight efficiency hit from having to change the
>>>> "pop the LR straight into the PC" idiom matters much.
>>>
>>> Sounds reasonable, I guess in that case we can also omit all the
>>> additional register in the clobber list.
>>
>> Yeah, you should only need to specify clobbers for any registers which
>> are neither used as arguments nor explicitly preserved - looking at
>> the layout of the code, it seems unlikely that the compiler would have
>> anything live in r3 or r12 across the call (since the scope for
>> inlining is pretty trivial), but there's no harm in being strictly
>> correct :)
> 
> So something like this?
> 
> -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");

         : "memory", "r3", "r12", "lr");

>  }

Re: [PATCH 5/5] ARM: add support for building ARM kernel with clang

[Stefan Agner]

On 21.03.2018 00:18, Russell King - ARM Linux wrote:
> On Wed, Mar 21, 2018 at 12:02:06AM +0100, Stefan Agner wrote:
>> Use cc-options call for compiler options which are not available
>> in clang. With this patch an ARMv7 multi platform kernel can be
>> successfully build using clang (tested with version 5.0.1).
>>
>> Based-on-patches-by: Behan Webster <behanw@converseincode.com>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  arch/arm/Makefile                 | 2 +-
>>  arch/arm/boot/compressed/Makefile | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
>> index e9e3fde3c657..20e9fee1ccc5 100644
>> --- a/arch/arm/Makefile
>> +++ b/arch/arm/Makefile
>> @@ -39,7 +39,7 @@ KBUILD_CFLAGS	+= $(call cc-option,-mno-unaligned-access)
>>  endif
>>
>>  ifeq ($(CONFIG_FRAME_POINTER),y)
>> -KBUILD_CFLAGS	+=-fno-omit-frame-pointer -mapcs -mno-sched-prolog
>> +KBUILD_CFLAGS	+=-fno-omit-frame-pointer $(call cc-option,-mapcs,) $(call cc-option,-mno-sched-prolog,)
> 
> Some of these options here are to ensure that we generate the following
> code, so we can backtrace:
> 
> 	mov	ip, sp
> 	stmfd	sp!, {fp, ip, lr, pc}
> 	sub	fp, ip, #4
> 
> If clang isn't producing that code at the start of functions with
> CONFIG_FRAME_POINTER=y, then backtracing will not work, and arguably
> CONFIG_FRAME_POINTER=y is useless there.  In that circumstance, it's
> probably better to fail so the user can configure something more
> debuggable, rather than having the kernel potentially producing
> undebuggable oopses.

Just for the records, compiled with clang, this patchset applied and
CONFIG_FRAME_POINTER=y as expected leads to a non functional backtrace:

[    4.583711] ------------[ cut here ]------------
[    4.588347] WARNING: CPU: 0 PID: 1 at init/main.c:1012
kernel_init+0x60/0x238
[    4.595505] Modules linked in:
[    4.598590] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       
4.16.0-rc5-00019-g686ee524148e-dirty #42
[    4.608514] Hardware name: Freescale i.MX7 Dual (Device Tree)
[    4.614266] Backtrace: 
[    4.616728] [<dc063f34>] (0xdc063f34) from [<00000000>] (  (null))
[    4.622918] Backtrace aborted due to bad frame pointer <8c0165e4>
[    4.629046] ---[ end trace 200951c950497708 ]---

clang with CONFIG_FRAME_POINTER=y should really error out. Without the
cc-option calls building witih clang will print the following errors:

...
  CC      kernel/bounds.s
clang-6.0: error: unknown argument: '-mapcs'
clang-6.0: error: unknown argument: '-mno-sched-prolog'
...

Using CONFIG_ARM_UNWIND=y compiles fine and backtraces do work fine too:

[    4.630877] ------------[ cut here ]------------
[    4.635515] WARNING: CPU: 0 PID: 1 at init/main.c:1012
kernel_init+0x5c/0x234
[    4.642672] Modules linked in:
[    4.645742] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       
4.16.0-rc5-00019-g686ee524148e-dirty #41
[    4.655666] Hardware name: Freescale i.MX7 Dual (Device Tree)
[    4.661437] [<c0110a9c>] (unwind_backtrace) from [<c010ca6c>]
(show_stack+0x10/0x14)
[    4.669200] [<c010ca6c>] (show_stack) from [<c0959ebc>]
(dump_stack+0x9c/0xac)
[    4.676442] [<c0959ebc>] (dump_stack) from [<c01214c4>]
(__warn+0xb4/0x120)
[    4.683419] [<c01214c4>] (__warn) from [<c01215f4>]
(warn_slowpath_null+0x40/0x48)
[    4.691003] [<c01215f4>] (warn_slowpath_null) from [<c096e7b8>]
(kernel_init+0x5c/0x234)
[    4.699111] [<c096e7b8>] (kernel_init) from [<c01010e8>]
(ret_from_fork+0x14/0x2c)
[    4.706691] Exception stack(0xdc063fb0 to 0xdc063ff8)
[    4.711752] 3fa0:                                     00000000
00000000 00000000 00000000
[    4.719943] 3fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    4.728133] 3fe0: 00000000 00000000 00000000 00000000 00000013
00000000
[    4.734792] ---[ end trace 200951c950497708 ]---

--
Stefan