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  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 
>> >> >> Cc: Stephen Warren 
>> >> >> Cc: Thierry Reding 
>> >> >> Signed-off-by: Stefan Agner 
>> >> >> ---
>> >> >> 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_extensionsec\n\t"
>> >> >> -"stmfdsp!, {r4 - r11, lr}\n\t"
>> >> >> +"stmfdsp!, {r4 - r11}\n\t"
>> >> >>__asmeq("%0", "r0")
>> >> >>__asmeq("%1", "r1")
>> >> >>__asmeq("%2", "r2")
>> >> >>"movr3, #0\n\t"
>> >> >>"movr4, #0\n\t"
>> >> >>"smc#0\n\t"
>> >> >> -"ldmfdsp!, {r4 - r11, pc}"
>> >> >> +"ldmfdsp!, {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 

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  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 
>> >> >> Cc: Stephen Warren 
>> >> >> Cc: Thierry Reding 
>> >> >> Signed-off-by: Stefan Agner 
>> >> >> ---
>> >> >> 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_extensionsec\n\t"
>> >> >> -"stmfdsp!, {r4 - r11, lr}\n\t"
>> >> >> +"stmfdsp!, {r4 - r11}\n\t"
>> >> >>__asmeq("%0", "r0")
>> >> >>__asmeq("%1", "r1")
>> >> >>__asmeq("%2", "r2")
>> >> >>"movr3, #0\n\t"
>> >> >>"movr4, #0\n\t"
>> >> >>"smc#0\n\t"
>> >> >> -"ldmfdsp!, {r4 - r11, pc}"
>> >> >> +"ldmfdsp!, {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 

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  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 
> >> >> Cc: Stephen Warren 
> >> >> Cc: Thierry Reding 
> >> >> Signed-off-by: Stefan Agner 
> >> >> ---
> >> >> 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_extensionsec\n\t"
> >> >> -"stmfdsp!, {r4 - r11, lr}\n\t"
> >> >> +"stmfdsp!, {r4 - r11}\n\t"
> >> >>__asmeq("%0", "r0")
> >> >>__asmeq("%1", "r1")
> >> >>__asmeq("%2", "r2")
> >> >>"movr3, #0\n\t"
> >> >>"movr4, #0\n\t"
> >> >>"smc#0\n\t"
> >> >> -"ldmfdsp!, {r4 - r11, pc}"
> >> >> +"ldmfdsp!, {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 

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  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 
> >> >> Cc: Stephen Warren 
> >> >> Cc: Thierry Reding 
> >> >> Signed-off-by: Stefan Agner 
> >> >> ---
> >> >> 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_extensionsec\n\t"
> >> >> -"stmfdsp!, {r4 - r11, lr}\n\t"
> >> >> +"stmfdsp!, {r4 - r11}\n\t"
> >> >>__asmeq("%0", "r0")
> >> >>__asmeq("%1", "r1")
> >> >>__asmeq("%2", "r2")
> >> >>"movr3, #0\n\t"
> >> >>"movr4, #0\n\t"
> >> >>"smc#0\n\t"
> >> >> -"ldmfdsp!, {r4 - r11, pc}"
> >> >> +"ldmfdsp!, {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 

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 
> Cc: Stephen Warren 
> Cc: Thierry Reding 
> Signed-off-by: Stefan Agner 
> ---
> 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_extensionsec\n\t"
> - "stmfd  sp!, {r4 - r11, lr}\n\t"
> + "stmfd  sp!, {r4 - r11}\n\t"
>   __asmeq("%0", "r0")
>   __asmeq("%1", "r1")
>   __asmeq("%2", "r2")
>   "movr3, #0\n\t"
>   "movr4, #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

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 
> Cc: Stephen Warren 
> Cc: Thierry Reding 
> Signed-off-by: Stefan Agner 
> ---
> 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_extensionsec\n\t"
> - "stmfd  sp!, {r4 - r11, lr}\n\t"
> + "stmfd  sp!, {r4 - r11}\n\t"
>   __asmeq("%0", "r0")
>   __asmeq("%1", "r1")
>   __asmeq("%2", "r2")
>   "movr3, #0\n\t"
>   "movr4, #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

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  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 
>> >> Cc: Stephen Warren 
>> >> Cc: Thierry Reding 
>> >> Signed-off-by: Stefan Agner 
>> >> ---
>> >> 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_extensionsec\n\t"
>> >> -"stmfdsp!, {r4 - r11, lr}\n\t"
>> >> +"stmfdsp!, {r4 - r11}\n\t"
>> >>__asmeq("%0", "r0")
>> >>__asmeq("%1", "r1")
>> >>__asmeq("%2", "r2")
>> >>"movr3, #0\n\t"
>> >>"movr4, #0\n\t"
>> >>"smc#0\n\t"
>> >> -"ldmfdsp!, {r4 - r11, pc}"
>> >> +"ldmfdsp!, {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:

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  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 
>> >> Cc: Stephen Warren 
>> >> Cc: Thierry Reding 
>> >> Signed-off-by: Stefan Agner 
>> >> ---
>> >> 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_extensionsec\n\t"
>> >> -"stmfdsp!, {r4 - r11, lr}\n\t"
>> >> +"stmfdsp!, {r4 - r11}\n\t"
>> >>__asmeq("%0", "r0")
>> >>__asmeq("%1", "r1")
>> >>__asmeq("%2", "r2")
>> >>"movr3, #0\n\t"
>> >>"movr4, #0\n\t"
>> >>"smc#0\n\t"
>> >> -"ldmfdsp!, {r4 - r11, pc}"
>> >> +"ldmfdsp!, {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:

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 
>> >> Cc: Stephen Warren 
>> >> Cc: Thierry Reding 
>> >> Signed-off-by: Stefan Agner 
>> >> ---
>> >> 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

[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 
>> >> Cc: Stephen Warren 
>> >> Cc: Thierry Reding 
>> >> Signed-off-by: Stefan Agner 
>> >> ---
>> >> 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

[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 
>>> Cc: Stephen Warren 
>>> Cc: Thierry Reding 
>>> Signed-off-by: Stefan Agner 
>>> ---
>>> 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

[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 
>>> Cc: Stephen Warren 
>>> Cc: Thierry Reding 
>>> Signed-off-by: Stefan Agner 
>>> ---
>>> 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

[Thierry Reding]

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 
> >> Cc: Stephen Warren 
> >> Cc: Thierry Reding 
> >> Signed-off-by: Stefan Agner 
> >> ---
> >> 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


signature.asc
Description: PGP signature

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

[Thierry Reding]

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 
> >> Cc: Stephen Warren 
> >> Cc: Thierry Reding 
> >> Signed-off-by: Stefan Agner 
> >> ---
> >> 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


signature.asc
Description: PGP signature

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 
>> Cc: Stephen Warren 
>> Cc: Thierry Reding 
>> Signed-off-by: Stefan Agner 
>> ---
>> 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

[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 
>> Cc: Stephen Warren 
>> Cc: Thierry Reding 
>> Signed-off-by: Stefan Agner 
>> ---
>> 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

[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 
Cc: Stephen Warren 
Cc: Thierry Reding 
Signed-off-by: Stefan Agner 
---
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

[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 
Cc: Stephen Warren 
Cc: Thierry Reding 
Signed-off-by: Stefan Agner 
---
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 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 
 Cc: Stephen Warren 
 Cc: Thierry Reding 
 Signed-off-by: Stefan Agner 
 ---
 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

[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 
 Cc: Stephen Warren 
 Cc: Thierry Reding 
 Signed-off-by: Stefan Agner 
 ---
 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

[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 
>>> Cc: Stephen Warren 
>>> Cc: Thierry Reding 
>>> Signed-off-by: Stefan Agner 
>>> ---
>>> 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

[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 
>>> Cc: Stephen Warren 
>>> Cc: Thierry Reding 
>>> Signed-off-by: Stefan Agner 
>>> ---
>>> 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

[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 
Cc: Stephen Warren 
Cc: Thierry Reding 
Signed-off-by: Stefan Agner 
---
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

[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 
Cc: Stephen Warren 
Cc: Thierry Reding 
Signed-off-by: Stefan Agner 
---
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 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 
> Cc: Stephen Warren 
> Cc: Thierry Reding 
> Signed-off-by: Stefan Agner 
> ---
> 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_extensionsec\n\t"
> - "stmfd  sp!, {r4 - r11, lr}\n\t"
> + "stmfd  sp!, {r4 - r11}\n\t"
>   __asmeq("%0", "r0")
>   __asmeq("%1", "r1")
>   __asmeq("%2", "r2")
>   "movr3, #0\n\t"
>   "movr4, #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

[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 
> Cc: Stephen Warren 
> Cc: Thierry Reding 
> Signed-off-by: Stefan Agner 
> ---
> 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_extensionsec\n\t"
> - "stmfd  sp!, {r4 - r11, lr}\n\t"
> + "stmfd  sp!, {r4 - r11}\n\t"
>   __asmeq("%0", "r0")
>   __asmeq("%1", "r1")
>   __asmeq("%2", "r2")
>   "movr3, #0\n\t"
>   "movr4, #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.

[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 
Cc: Stephen Warren 
Cc: Thierry Reding 
Signed-off-by: Stefan Agner 
---
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_extensionsec\n\t"
-   "stmfd  sp!, {r4 - r11, lr}\n\t"
+   "stmfd  sp!, {r4 - r11}\n\t"
__asmeq("%0", "r0")
__asmeq("%1", "r1")
__asmeq("%2", "r2")
"movr3, #0\n\t"
"movr4, #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

[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 
Cc: Stephen Warren 
Cc: Thierry Reding 
Signed-off-by: Stefan Agner 
---
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_extensionsec\n\t"
-   "stmfd  sp!, {r4 - r11, lr}\n\t"
+   "stmfd  sp!, {r4 - r11}\n\t"
__asmeq("%0", "r0")
__asmeq("%1", "r1")
__asmeq("%2", "r2")
"movr3, #0\n\t"
"movr4, #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