Re: [PATCH v2 3/6] ARM: trusted_foundations: do not use naked function
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
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
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
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
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
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
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
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
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
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
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
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
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
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