[PATCH v2 0/6] ARM: clang support

[PATCH v2 0/6] 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 (and boot) the multi_v7_defconfig with 4.16-rc5 using clang
5.0.1 and 6.0.0.

This version also adds a patch to mitigate a often printed warning
about duplicate 'const' declaration specifier when using get_user().

Stefan Agner (6):
  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
  ARM: uaccess: remove const to avoid duplicate specifier

 arch/arm/Makefile                       |  2 +-
 arch/arm/boot/compressed/Makefile       |  2 +-
 arch/arm/firmware/trusted_foundations.c | 14 +++++++++-----
 arch/arm/include/asm/uaccess.h          |  2 +-
 drivers/bus/arm-cci.c                   |  2 --
 drivers/firmware/efi/libstub/Makefile   |  3 ++-
 6 files changed, 14 insertions(+), 11 deletions(-)

-- 
2.16.2

[PATCH v2 1/6] 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();
        ^

While the function is marked __naked it actually properly return
in asm. There is no need for the unreachable() call.

Suggested-by: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Changes in v2:
- Don't add assembly ASM_UNREACHABLE, just drop unreachable()

 drivers/bus/arm-cci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 5426c04fe24b..fc2da3a617ac 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -2103,8 +2103,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 v2 1/6] bus: arm-cci: use asm unreachable

[Nicolas Pitre]

On Sun, 25 Mar 2018, 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();
>         ^
> 
> While the function is marked __naked it actually properly return
> in asm. There is no need for the unreachable() call.
> 
> Suggested-by: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Stefan Agner <stefan@agner.ch>

If that doesn't introduce any warning with gcc then I'm fine with it.

Acked-by: Nicolas Pitre <nico@linaro.org>


> ---
> Changes in v2:
> - Don't add assembly ASM_UNREACHABLE, just drop unreachable()
> 
>  drivers/bus/arm-cci.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 5426c04fe24b..fc2da3a617ac 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -2103,8 +2103,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 v2 1/6] bus: arm-cci: use asm unreachable

[Stefan Agner]

On 25.03.2018 20:14, Nicolas Pitre wrote:
> On Sun, 25 Mar 2018, 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();
>>         ^
>>
>> While the function is marked __naked it actually properly return
>> in asm. There is no need for the unreachable() call.
>>
>> Suggested-by: Russell King <linux@arm.linux.org.uk>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
> 
> If that doesn't introduce any warning with gcc then I'm fine with it.
> 

At least with GCC 6.2 it did not introduce a new warning with gcc.

> Acked-by: Nicolas Pitre <nico@linaro.org>

Thanks!

--
Stefan

> 
> 
>> ---
>> Changes in v2:
>> - Don't add assembly ASM_UNREACHABLE, just drop unreachable()
>>
>>  drivers/bus/arm-cci.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>> index 5426c04fe24b..fc2da3a617ac 100644
>> --- a/drivers/bus/arm-cci.c
>> +++ b/drivers/bus/arm-cci.c
>> @@ -2103,8 +2103,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 v2 1/6] bus: arm-cci: use asm unreachable

[Stefan Agner]

On 25.03.2018 20:09, 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();
>         ^
> 
> While the function is marked __naked it actually properly return
> in asm. There is no need for the unreachable() call.

Hi Arnd,

I think previously changes to this driver went through one of your
trees, can you pick this up?

--
Stefan


> 
> Suggested-by: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> Changes in v2:
> - Don't add assembly ASM_UNREACHABLE, just drop unreachable()
> 
>  drivers/bus/arm-cci.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 5426c04fe24b..fc2da3a617ac 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -2103,8 +2103,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();
>  }
>  
>  /**

[PATCH v2 2/6] 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 v2 3/6] 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.

Cc: Dmitry Osipenko <digetx@gmail.com>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
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 b/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");
 }
 
 static int tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
-- 
2.16.2

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

[Dmitry Osipenko]

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
> 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.
> 
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> 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 b/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.

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

[Robin Murphy]

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
>> 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.
>>
>> Cc: Dmitry Osipenko <digetx@gmail.com>
>> Cc: Stephen Warren <swarren@nvidia.com>
>> Cc: Thierry Reding <treding@nvidia.com>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>> 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 b/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.

Robin.

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

[Dmitry Osipenko]

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
>>> 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.
>>>
>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>> Cc: Stephen Warren <swarren@nvidia.com>
>>> Cc: Thierry Reding <treding@nvidia.com>
>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>> ---
>>> 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
>>> b/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.

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

[Stefan Agner]

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
>>>> 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.
>>>>
>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>> Cc: Thierry Reding <treding@nvidia.com>
>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>> ---
>>>> 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
>>>> b/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?

--
Stefan

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

[Stephen Warren]

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
>>>>> 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.
>>>>>
>>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>>> Cc: Thierry Reding <treding@nvidia.com>
>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>> ---
>>>>> 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
>>>>> b/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?

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

[Stefan Agner]

On 16.04.2018 18:08, Stephen Warren wrote:
> 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
>>>>>> 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.
>>>>>>
>>>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>>>> Cc: Thierry Reding <treding@nvidia.com>
>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>>> ---
>>>>>> 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
>>>>>> b/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?

I think the last patches applied to that file went through your tree.

Thierry, Russel, any preferences?

--
Stefan

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5082 bytes --]

On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote:
> On 16.04.2018 18:08, Stephen Warren wrote:
> > 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
> >>>>>> 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.
> >>>>>>
> >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
> >>>>>> Cc: Stephen Warren <swarren@nvidia.com>
> >>>>>> Cc: Thierry Reding <treding@nvidia.com>
> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >>>>>> ---
> >>>>>> 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
> >>>>>> b/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?
> 
> I think the last patches applied to that file went through your tree.
> 
> Thierry, Russel, any preferences?

I don't mind picking this up into the Tegra tree. Might be a good idea
to move this into drivers/firmware, though, since that's where all the
other firmware-related drivers reside.

Firmware code, such as the BPMP driver, usually goes through ARM-SoC
these days. I think this is in the same category.

Russell, any objections to me picking this patch up and moving it into
drivers/firmware?

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Stefan Agner]

On 17.04.2018 10:11, Thierry Reding wrote:
> On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote:
>> On 16.04.2018 18:08, Stephen Warren wrote:
>> > 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
>> >>>>>> 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.
>> >>>>>>
>> >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>> >>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>> >>>>>> Cc: Thierry Reding <treding@nvidia.com>
>> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >>>>>> ---
>> >>>>>> 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
>> >>>>>> b/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?
>>
>> I think the last patches applied to that file went through your tree.
>>
>> Thierry, Russel, any preferences?
> 
> I don't mind picking this up into the Tegra tree. Might be a good idea
> to move this into drivers/firmware, though, since that's where all the
> other firmware-related drivers reside.
> 
> Firmware code, such as the BPMP driver, usually goes through ARM-SoC
> these days. I think this is in the same category.
> 
> Russell, any objections to me picking this patch up and moving it into
> drivers/firmware?

Russel, I think Thierry is waiting for your ok on this.

--
Stefan

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

[Kees Cook]

On Tue, Apr 17, 2018 at 1:11 AM, Thierry Reding <treding@nvidia.com> wrote:
> On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote:
>> On 16.04.2018 18:08, Stephen Warren wrote:
>> > 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
>> >>>>>> 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.
>> >>>>>>
>> >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>> >>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>> >>>>>> Cc: Thierry Reding <treding@nvidia.com>
>> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >>>>>> ---
>> >>>>>> 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
>> >>>>>> b/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?
>>
>> I think the last patches applied to that file went through your tree.
>>
>> Thierry, Russel, any preferences?
>
> I don't mind picking this up into the Tegra tree. Might be a good idea
> to move this into drivers/firmware, though, since that's where all the
> other firmware-related drivers reside.
>
> Firmware code, such as the BPMP driver, usually goes through ARM-SoC
> these days. I think this is in the same category.
>
> Russell, any objections to me picking this patch up and moving it into
> drivers/firmware?

Please take this -- without it I'm seeing build failures on the arm
allmodconfig under gcc 7.3.0:

/tmp/ccKdsC59.s: Assembler messages:
/tmp/ccKdsC59.s:35: Error: .err encountered
/tmp/ccKdsC59.s:36: Error: .err encountered
/tmp/ccKdsC59.s:37: Error: .err encountered
scripts/Makefile.build:317: recipe for target
'arch/arm/firmware/trusted_foundations.o' failed

The above patch fixes it for me.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

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

[Russell King - ARM Linux]

On Thu, Jul 12, 2018 at 03:43:10PM -0700, Kees Cook wrote:
> On Tue, Apr 17, 2018 at 1:11 AM, Thierry Reding <treding@nvidia.com> wrote:
> > On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote:
> >> On 16.04.2018 18:08, Stephen Warren wrote:
> >> > 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
> >> >>>>>> 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.
> >> >>>>>>
> >> >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
> >> >>>>>> Cc: Stephen Warren <swarren@nvidia.com>
> >> >>>>>> Cc: Thierry Reding <treding@nvidia.com>
> >> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> >>>>>> ---
> >> >>>>>> 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
> >> >>>>>> b/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?
> >>
> >> I think the last patches applied to that file went through your tree.
> >>
> >> Thierry, Russel, any preferences?
> >
> > I don't mind picking this up into the Tegra tree. Might be a good idea
> > to move this into drivers/firmware, though, since that's where all the
> > other firmware-related drivers reside.
> >
> > Firmware code, such as the BPMP driver, usually goes through ARM-SoC
> > these days. I think this is in the same category.
> >
> > Russell, any objections to me picking this patch up and moving it into
> > drivers/firmware?
> 
> Please take this -- without it I'm seeing build failures on the arm
> allmodconfig under gcc 7.3.0:

Sorry, I'd completely missed this... now replied on the original patch.

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

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

[Stefan Agner]

On 13.07.2018 01:01, Russell King - ARM Linux wrote:
> On Thu, Jul 12, 2018 at 03:43:10PM -0700, Kees Cook wrote:
>> On Tue, Apr 17, 2018 at 1:11 AM, Thierry Reding <treding@nvidia.com> wrote:
>> > On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote:
>> >> On 16.04.2018 18:08, Stephen Warren wrote:
>> >> > 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
>> >> >>>>>> 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.
>> >> >>>>>>
>> >> >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>> >> >>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>> >> >>>>>> Cc: Thierry Reding <treding@nvidia.com>
>> >> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >> >>>>>> ---
>> >> >>>>>> 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
>> >> >>>>>> b/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?
>> >>
>> >> I think the last patches applied to that file went through your tree.
>> >>
>> >> Thierry, Russel, any preferences?
>> >
>> > I don't mind picking this up into the Tegra tree. Might be a good idea
>> > to move this into drivers/firmware, though, since that's where all the
>> > other firmware-related drivers reside.
>> >
>> > Firmware code, such as the BPMP driver, usually goes through ARM-SoC
>> > these days. I think this is in the same category.
>> >
>> > Russell, any objections to me picking this patch up and moving it into
>> > drivers/firmware?
>>
>> Please take this -- without it I'm seeing build failures on the arm
>> allmodconfig under gcc 7.3.0:
> 
> Sorry, I'd completely missed this... now replied on the original patch.

Thierry merged this patch just two days ago, so it is already in -next.

--
Stefan

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

[Dmitry Osipenko]

On 16.04.2018 21:21, Stefan Agner wrote:
> On 16.04.2018 18:08, Stephen Warren wrote:
>> 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
>>>>>>> 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.
>>>>>>>
>>>>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>>>>> Cc: Thierry Reding <treding@nvidia.com>
>>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>>>> ---
>>>>>>> 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
>>>>>>> b/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?
> 
> I think the last patches applied to that file went through your tree.
> 
> Thierry, Russel, any preferences?

I've been preparing patches for upstream to add initial support of L2 cache
maintance to TF / Tegra30 and noticed that without this patch I'm getting a hang
early in boot. That is because before this patch registers store / restore was
incorrect, probably the premature return (lr -> pc) causes stack corruption. Not
sure whether it's worth to backport this patch, but I want to see it at least in
-next.

Thierry, please take care of this patch. Thanks.

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

[Russell King - ARM Linux]

On Sun, Mar 25, 2018 at 08:09:56PM +0200, 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.
> 
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> 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 b/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");
>  }

Does GCC try to inline this?

It may just be better to switch to basic asm.  We know that a naked
function won't be inlined, and we already know (because we need the
prologue/epilogue) what registers the 32-bit arguments will be in.

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

[PATCH v2 4/6] 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

Re: [v2,4/6] ARM: drop no-thumb-interwork in EABI mode

[Guenter Roeck]

On Sun, Mar 25, 2018 at 08:09:57PM +0200, Stefan Agner wrote:
> According to GCC documentation -m(no-)thumb-interwork is
> meaningless in AAPCS configurations. Also clang does not

It appears that this is only correct for recent versions of gcc.

With gcc 4.9.2, this patch causes the qemu collie emulation
to fail with collie_defconfig+CONFIG_AEABI.

qemu-system-arm: Trying to execute code outside RAM or ROM at 0x02000000
This usually means one of the following happened:
...

With gcc 7.3.0, the emulation works as expected. Reverting the patch
fixes the problem with gcc 4.9.2. Not that it matters much to me - I can
and will switch to gcc 7.3.0 for my testing - but effectively this means
that older versions of gcc are no longer supported for all configurations.

Maybe $(call cc-option,-mno-thumb-interwork,) would have been safer ?

Guenter

> 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

Re: [v2,4/6] ARM: drop no-thumb-interwork in EABI mode

[Stefan Agner]

On 12.06.2018 19:19, Guenter Roeck wrote:
> On Sun, Mar 25, 2018 at 08:09:57PM +0200, Stefan Agner wrote:
>> According to GCC documentation -m(no-)thumb-interwork is
>> meaningless in AAPCS configurations. Also clang does not
> 
> It appears that this is only correct for recent versions of gcc.
> 
> With gcc 4.9.2, this patch causes the qemu collie emulation
> to fail with collie_defconfig+CONFIG_AEABI.

Hm, interesting. However, even 4.9.0 claims this option is meaningless
when using AAPCS configurations:
https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/ARM-Options.html#ARM-Options

> 
> qemu-system-arm: Trying to execute code outside RAM or ROM at 0x02000000
> This usually means one of the following happened:
> ...
> 
> With gcc 7.3.0, the emulation works as expected. Reverting the patch
> fixes the problem with gcc 4.9.2. Not that it matters much to me - I can
> and will switch to gcc 7.3.0 for my testing - but effectively this means
> that older versions of gcc are no longer supported for all configurations.
> 
> Maybe $(call cc-option,-mno-thumb-interwork,) would have been safer ?

I used to have call cc-option in place, but I removed that when I
realized that gcc claims it is meaningless with AAPCS configurations.

--
Stefan

> 
> Guenter
> 
>> 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

[PATCH v2 5/6] 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>
---
Changes in v2:
- Drop cc-options in CONFIG_FRAME_POINTER=y case

 arch/arm/boot/compressed/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

[PATCH v2 6/6] ARM: uaccess: remove const to avoid duplicate specifier

[Stefan Agner]

Some users of get_user use the macro with an argument p which
is already specified as static. When using clang this leads to
a duplicate specifier:
    CC      arch/arm/kernel/process.o
  In file included from init/do_mounts.c:15:
  In file included from ./include/linux/tty.h:7:
  In file included from ./include/uapi/linux/termios.h:6:
  In file included from ./arch/arm/include/generated/uapi/asm/termios.h:1:
  ./include/asm-generic/termios.h:25:6: warning: duplicate 'const' declaration
          specifier [-Wduplicate-decl-specifier]
          if (get_user(tmp, &termio->c_iflag) < 0)
              ^
  ./arch/arm/include/asm/uaccess.h:195:3: note: expanded from macro 'get_user'
                  __get_user_check(x, p);                                 \
                  ^
  ./arch/arm/include/asm/uaccess.h:155:12: note: expanded from macro
          '__get_user_check'
                  register const typeof(*(p)) __user *__p asm("r0") = (p);\

Remove the const attribute from the register declaration
to avoid the duplicate const specifier. In a test with ptrace.c
and traps.c (both using get_user with non-const arguments for p)
the generated code was exactly the same.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Similar issue has already been discussed here:
https://patchwork.kernel.org/patch/9693821/

 arch/arm/include/asm/uaccess.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 0bf2347495f1..3d614e90c19f 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -152,7 +152,7 @@ extern int __get_user_64t_4(void *);
 #define __get_user_check(x, p)						\
 	({								\
 		unsigned long __limit = current_thread_info()->addr_limit - 1; \
-		register const typeof(*(p)) __user *__p asm("r0") = (p);\
+		register typeof(*(p)) __user *__p asm("r0") = (p);	\
 		register typeof(x) __r2 asm("r2");			\
 		register unsigned long __l asm("r1") = __limit;		\
 		register int __e asm("r0");				\
-- 
2.16.2

Re: [PATCH v2 0/6] ARM: clang support

[Stefan Agner]

On 25.03.2018 20:09, Stefan Agner wrote:
> 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 (and boot) the multi_v7_defconfig with 4.16-rc5 using clang
> 5.0.1 and 6.0.0.

Russel, Arnd, any comment on this patch series? How can we get it
merged?

I was thinking patch 1 through armsoc since that is the way previous
patches have been merged.

Note sure about patch 2, Russel can you comment on Thierry's email?

And patch 3 through 6 through Russel's tree?

--
Stefan

> 
> This version also adds a patch to mitigate a often printed warning
> about duplicate 'const' declaration specifier when using get_user().
> 
> Stefan Agner (6):
>   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
>   ARM: uaccess: remove const to avoid duplicate specifier
> 
>  arch/arm/Makefile                       |  2 +-
>  arch/arm/boot/compressed/Makefile       |  2 +-
>  arch/arm/firmware/trusted_foundations.c | 14 +++++++++-----
>  arch/arm/include/asm/uaccess.h          |  2 +-
>  drivers/bus/arm-cci.c                   |  2 --
>  drivers/firmware/efi/libstub/Makefile   |  3 ++-
>  6 files changed, 14 insertions(+), 11 deletions(-)

Re: [PATCH v2 0/6] ARM: clang support

[Arnd Bergmann]

On Mon, May 7, 2018 at 4:24 PM, Stefan Agner <stefan@agner.ch> wrote:
> On 25.03.2018 20:09, Stefan Agner wrote:
>> 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 (and boot) the multi_v7_defconfig with 4.16-rc5 using clang
>> 5.0.1 and 6.0.0.
>
> Russel, Arnd, any comment on this patch series? How can we get it
> merged?
>
> I was thinking patch 1 through armsoc since that is the way previous
> patches have been merged.

Please resend it to arm@kernel.org.

Lorenzo, Robin, Will, Suzuki: could one of you look at the patch and
provide an Ack
and maybe send a patch to add a MAINTAINERS file entry for the the
arm-cci driver to make it less ambiguous to who's responsible for it?

I know very little about the code and don't want to be the main person having
to decide if a patch can go in or not.

> Note sure about patch 2,

This should be up to Ard to pick up or comment on. I think I had
previously suggested a similar patch to him, but don't remember
what happened to that.

> Russel can you comment on Thierry's email?
>
> And patch 3 through 6 through Russel's tree?

Sounds good to me. Please submit those through
http://www.arm.linux.org.uk/developer/patches/ once they are
ready. I think patches 4, 5 and 6 are good to go in, while patch
3 has an unfinished discussion.

It might also be a good idea to pick up the CONFIG_FRAME_POINTER
problem and disable that option for clang based builds now, using
the new infrastructure we have for detecting compiler features at
Kconfig time.

       Arnd