| Message ID | 20180325180959.28008-4-stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25.03.2018 21:09, Stefan Agner wrote: > As documented in GCC naked functions should only use Basic asm > syntax. The Extended asm or mixture of Basic asm and "C" code is > not guaranteed. Currently this works because it was hard coded > to follow and check GCC behavior for arguments and register > placement. > > Furthermore with clang using parameters in Extended asm in a > naked function is not supported: > arch/arm/firmware/trusted_foundations.c:47:10: error: parameter > references not allowed in naked functions > : "r" (type), "r" (arg1), "r" (arg2) > ^ > > Use a regular function to be more portable. This aligns also with > the other smc call implementations e.g. in qcom_scm-32.c and > bcm_kona_smc.c. > > Cc: Dmitry Osipenko <digetx@gmail.com> > Cc: Stephen Warren <swarren@nvidia.com> > Cc: Thierry Reding <treding@nvidia.com> > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > Changes in v2: > - Keep stmfd/ldmfd to avoid potential ABI issues > > arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c > index 3fb1b5a1dce9..689e6565abfc 100644 > --- a/arch/arm/firmware/trusted_foundations.c > +++ b/arch/arm/firmware/trusted_foundations.c > @@ -31,21 +31,25 @@ > > static unsigned long cpu_boot_addr; > > -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) > +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) > { > + register u32 r0 asm("r0") = type; > + register u32 r1 asm("r1") = arg1; > + register u32 r2 asm("r2") = arg2; > + > asm volatile( > ".arch_extension sec\n\t" > - "stmfd sp!, {r4 - r11, lr}\n\t" > + "stmfd sp!, {r4 - r11}\n\t" > __asmeq("%0", "r0") > __asmeq("%1", "r1") > __asmeq("%2", "r2") > "mov r3, #0\n\t" > "mov r4, #0\n\t" > "smc #0\n\t" > - "ldmfd sp!, {r4 - r11, pc}" > + "ldmfd sp!, {r4 - r11}\n\t" > : > - : "r" (type), "r" (arg1), "r" (arg2) > - : "memory"); > + : "r" (r0), "r" (r1), "r" (r2) > + : "memory", "r3", "r12", "lr"); Although seems "lr" won't be affected by SMC invocation because it should be banked and hence could be omitted entirely from the code. Maybe somebody could confirm this.
On 26/03/18 22:20, Dmitry Osipenko wrote: > On 25.03.2018 21:09, Stefan Agner wrote: >> As documented in GCC naked functions should only use Basic asm >> syntax. The Extended asm or mixture of Basic asm and "C" code is >> not guaranteed. Currently this works because it was hard coded >> to follow and check GCC behavior for arguments and register >> placement. >> >> Furthermore with clang using parameters in Extended asm in a >> naked function is not supported: >> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter >> references not allowed in naked functions >> : "r" (type), "r" (arg1), "r" (arg2) >> ^ >> >> Use a regular function to be more portable. This aligns also with >> the other smc call implementations e.g. in qcom_scm-32.c and >> bcm_kona_smc.c. >> >> Cc: Dmitry Osipenko <digetx@gmail.com> >> Cc: Stephen Warren <swarren@nvidia.com> >> Cc: Thierry Reding <treding@nvidia.com> >> Signed-off-by: Stefan Agner <stefan@agner.ch> >> --- >> Changes in v2: >> - Keep stmfd/ldmfd to avoid potential ABI issues >> >> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c >> index 3fb1b5a1dce9..689e6565abfc 100644 >> --- a/arch/arm/firmware/trusted_foundations.c >> +++ b/arch/arm/firmware/trusted_foundations.c >> @@ -31,21 +31,25 @@ >> >> static unsigned long cpu_boot_addr; >> >> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) >> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) >> { >> + register u32 r0 asm("r0") = type; >> + register u32 r1 asm("r1") = arg1; >> + register u32 r2 asm("r2") = arg2; >> + >> asm volatile( >> ".arch_extension sec\n\t" >> - "stmfd sp!, {r4 - r11, lr}\n\t" >> + "stmfd sp!, {r4 - r11}\n\t" >> __asmeq("%0", "r0") >> __asmeq("%1", "r1") >> __asmeq("%2", "r2") >> "mov r3, #0\n\t" >> "mov r4, #0\n\t" >> "smc #0\n\t" >> - "ldmfd sp!, {r4 - r11, pc}" >> + "ldmfd sp!, {r4 - r11}\n\t" >> : >> - : "r" (type), "r" (arg1), "r" (arg2) >> - : "memory"); >> + : "r" (r0), "r" (r1), "r" (r2) >> + : "memory", "r3", "r12", "lr"); > > Although seems "lr" won't be affected by SMC invocation because it should be > banked and hence could be omitted entirely from the code. Maybe somebody could > confirm this. Strictly per the letter of the architecture, the SMC could be trapped to Hyp mode, and a hypervisor might clobber LR_usr in the process of forwarding the call to the firmware secure monitor (since Hyp doesn't have a banked LR of its own). Admittedly there are probably no real systems with the appropriate hardware/software combination to hit that, but on the other hand if this gets inlined where the compiler has already created a stack frame then an LR clobber is essentially free, so I reckon we're better off keeping it for reassurance. This isn't exactly a critical fast path anyway. Robin.
On 27.03.2018 14:54, Robin Murphy wrote: > On 26/03/18 22:20, Dmitry Osipenko wrote: >> On 25.03.2018 21:09, Stefan Agner wrote: >>> As documented in GCC naked functions should only use Basic asm >>> syntax. The Extended asm or mixture of Basic asm and "C" code is >>> not guaranteed. Currently this works because it was hard coded >>> to follow and check GCC behavior for arguments and register >>> placement. >>> >>> Furthermore with clang using parameters in Extended asm in a >>> naked function is not supported: >>> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter >>> references not allowed in naked functions >>> : "r" (type), "r" (arg1), "r" (arg2) >>> ^ >>> >>> Use a regular function to be more portable. This aligns also with >>> the other smc call implementations e.g. in qcom_scm-32.c and >>> bcm_kona_smc.c. >>> >>> Cc: Dmitry Osipenko <digetx@gmail.com> >>> Cc: Stephen Warren <swarren@nvidia.com> >>> Cc: Thierry Reding <treding@nvidia.com> >>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>> --- >>> Changes in v2: >>> - Keep stmfd/ldmfd to avoid potential ABI issues >>> >>> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- >>> 1 file changed, 9 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/arm/firmware/trusted_foundations.c >>> b/arch/arm/firmware/trusted_foundations.c >>> index 3fb1b5a1dce9..689e6565abfc 100644 >>> --- a/arch/arm/firmware/trusted_foundations.c >>> +++ b/arch/arm/firmware/trusted_foundations.c >>> @@ -31,21 +31,25 @@ >>> static unsigned long cpu_boot_addr; >>> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) >>> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) >>> { >>> + register u32 r0 asm("r0") = type; >>> + register u32 r1 asm("r1") = arg1; >>> + register u32 r2 asm("r2") = arg2; >>> + >>> asm volatile( >>> ".arch_extension sec\n\t" >>> - "stmfd sp!, {r4 - r11, lr}\n\t" >>> + "stmfd sp!, {r4 - r11}\n\t" >>> __asmeq("%0", "r0") >>> __asmeq("%1", "r1") >>> __asmeq("%2", "r2") >>> "mov r3, #0\n\t" >>> "mov r4, #0\n\t" >>> "smc #0\n\t" >>> - "ldmfd sp!, {r4 - r11, pc}" >>> + "ldmfd sp!, {r4 - r11}\n\t" >>> : >>> - : "r" (type), "r" (arg1), "r" (arg2) >>> - : "memory"); >>> + : "r" (r0), "r" (r1), "r" (r2) >>> + : "memory", "r3", "r12", "lr"); >> >> Although seems "lr" won't be affected by SMC invocation because it should be >> banked and hence could be omitted entirely from the code. Maybe somebody could >> confirm this. > Strictly per the letter of the architecture, the SMC could be trapped to Hyp > mode, and a hypervisor might clobber LR_usr in the process of forwarding the > call to the firmware secure monitor (since Hyp doesn't have a banked LR of its > own). Admittedly there are probably no real systems with the appropriate > hardware/software combination to hit that, but on the other hand if this gets > inlined where the compiler has already created a stack frame then an LR clobber > is essentially free, so I reckon we're better off keeping it for reassurance. > This isn't exactly a critical fast path anyway. Okay, thank you for the clarification.
On 27.03.2018 14:16, Dmitry Osipenko wrote: > On 27.03.2018 14:54, Robin Murphy wrote: >> On 26/03/18 22:20, Dmitry Osipenko wrote: >>> On 25.03.2018 21:09, Stefan Agner wrote: >>>> As documented in GCC naked functions should only use Basic asm >>>> syntax. The Extended asm or mixture of Basic asm and "C" code is >>>> not guaranteed. Currently this works because it was hard coded >>>> to follow and check GCC behavior for arguments and register >>>> placement. >>>> >>>> Furthermore with clang using parameters in Extended asm in a >>>> naked function is not supported: >>>> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter >>>> references not allowed in naked functions >>>> : "r" (type), "r" (arg1), "r" (arg2) >>>> ^ >>>> >>>> Use a regular function to be more portable. This aligns also with >>>> the other smc call implementations e.g. in qcom_scm-32.c and >>>> bcm_kona_smc.c. >>>> >>>> Cc: Dmitry Osipenko <digetx@gmail.com> >>>> Cc: Stephen Warren <swarren@nvidia.com> >>>> Cc: Thierry Reding <treding@nvidia.com> >>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>> --- >>>> Changes in v2: >>>> - Keep stmfd/ldmfd to avoid potential ABI issues >>>> >>>> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- >>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/arch/arm/firmware/trusted_foundations.c >>>> b/arch/arm/firmware/trusted_foundations.c >>>> index 3fb1b5a1dce9..689e6565abfc 100644 >>>> --- a/arch/arm/firmware/trusted_foundations.c >>>> +++ b/arch/arm/firmware/trusted_foundations.c >>>> @@ -31,21 +31,25 @@ >>>> static unsigned long cpu_boot_addr; >>>> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) >>>> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) >>>> { >>>> + register u32 r0 asm("r0") = type; >>>> + register u32 r1 asm("r1") = arg1; >>>> + register u32 r2 asm("r2") = arg2; >>>> + >>>> asm volatile( >>>> ".arch_extension sec\n\t" >>>> - "stmfd sp!, {r4 - r11, lr}\n\t" >>>> + "stmfd sp!, {r4 - r11}\n\t" >>>> __asmeq("%0", "r0") >>>> __asmeq("%1", "r1") >>>> __asmeq("%2", "r2") >>>> "mov r3, #0\n\t" >>>> "mov r4, #0\n\t" >>>> "smc #0\n\t" >>>> - "ldmfd sp!, {r4 - r11, pc}" >>>> + "ldmfd sp!, {r4 - r11}\n\t" >>>> : >>>> - : "r" (type), "r" (arg1), "r" (arg2) >>>> - : "memory"); >>>> + : "r" (r0), "r" (r1), "r" (r2) >>>> + : "memory", "r3", "r12", "lr"); >>> >>> Although seems "lr" won't be affected by SMC invocation because it should be >>> banked and hence could be omitted entirely from the code. Maybe somebody could >>> confirm this. >> Strictly per the letter of the architecture, the SMC could be trapped to Hyp >> mode, and a hypervisor might clobber LR_usr in the process of forwarding the >> call to the firmware secure monitor (since Hyp doesn't have a banked LR of its >> own). Admittedly there are probably no real systems with the appropriate >> hardware/software combination to hit that, but on the other hand if this gets >> inlined where the compiler has already created a stack frame then an LR clobber >> is essentially free, so I reckon we're better off keeping it for reassurance. >> This isn't exactly a critical fast path anyway. > > Okay, thank you for the clarification. So it seems this change is fine? Stephen, you picked up changes for this driver before, is this patch going through your tree? -- Stefan
On 04/16/2018 09:56 AM, Stefan Agner wrote: > On 27.03.2018 14:16, Dmitry Osipenko wrote: >> On 27.03.2018 14:54, Robin Murphy wrote: >>> On 26/03/18 22:20, Dmitry Osipenko wrote: >>>> On 25.03.2018 21:09, Stefan Agner wrote: >>>>> As documented in GCC naked functions should only use Basic asm >>>>> syntax. The Extended asm or mixture of Basic asm and "C" code is >>>>> not guaranteed. Currently this works because it was hard coded >>>>> to follow and check GCC behavior for arguments and register >>>>> placement. >>>>> >>>>> Furthermore with clang using parameters in Extended asm in a >>>>> naked function is not supported: >>>>> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter >>>>> references not allowed in naked functions >>>>> : "r" (type), "r" (arg1), "r" (arg2) >>>>> ^ >>>>> >>>>> Use a regular function to be more portable. This aligns also with >>>>> the other smc call implementations e.g. in qcom_scm-32.c and >>>>> bcm_kona_smc.c. >>>>> >>>>> Cc: Dmitry Osipenko <digetx@gmail.com> >>>>> Cc: Stephen Warren <swarren@nvidia.com> >>>>> Cc: Thierry Reding <treding@nvidia.com> >>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>> --- >>>>> Changes in v2: >>>>> - Keep stmfd/ldmfd to avoid potential ABI issues >>>>> >>>>> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- >>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/arch/arm/firmware/trusted_foundations.c >>>>> b/arch/arm/firmware/trusted_foundations.c >>>>> index 3fb1b5a1dce9..689e6565abfc 100644 >>>>> --- a/arch/arm/firmware/trusted_foundations.c >>>>> +++ b/arch/arm/firmware/trusted_foundations.c >>>>> @@ -31,21 +31,25 @@ >>>>> static unsigned long cpu_boot_addr; >>>>> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) >>>>> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) >>>>> { >>>>> + register u32 r0 asm("r0") = type; >>>>> + register u32 r1 asm("r1") = arg1; >>>>> + register u32 r2 asm("r2") = arg2; >>>>> + >>>>> asm volatile( >>>>> ".arch_extension sec\n\t" >>>>> - "stmfd sp!, {r4 - r11, lr}\n\t" >>>>> + "stmfd sp!, {r4 - r11}\n\t" >>>>> __asmeq("%0", "r0") >>>>> __asmeq("%1", "r1") >>>>> __asmeq("%2", "r2") >>>>> "mov r3, #0\n\t" >>>>> "mov r4, #0\n\t" >>>>> "smc #0\n\t" >>>>> - "ldmfd sp!, {r4 - r11, pc}" >>>>> + "ldmfd sp!, {r4 - r11}\n\t" >>>>> : >>>>> - : "r" (type), "r" (arg1), "r" (arg2) >>>>> - : "memory"); >>>>> + : "r" (r0), "r" (r1), "r" (r2) >>>>> + : "memory", "r3", "r12", "lr"); >>>> >>>> Although seems "lr" won't be affected by SMC invocation because it should be >>>> banked and hence could be omitted entirely from the code. Maybe somebody could >>>> confirm this. >>> Strictly per the letter of the architecture, the SMC could be trapped to Hyp >>> mode, and a hypervisor might clobber LR_usr in the process of forwarding the >>> call to the firmware secure monitor (since Hyp doesn't have a banked LR of its >>> own). Admittedly there are probably no real systems with the appropriate >>> hardware/software combination to hit that, but on the other hand if this gets >>> inlined where the compiler has already created a stack frame then an LR clobber >>> is essentially free, so I reckon we're better off keeping it for reassurance. >>> This isn't exactly a critical fast path anyway. >> >> Okay, thank you for the clarification. > > So it seems this change is fine? > > Stephen, you picked up changes for this driver before, is this patch > going through your tree? You had best ask Thierry; he's taken over Tegra maintenance upstream. But that said, don't files in arch/arm go through Russell?
On 16.04.2018 18:08, Stephen Warren wrote: > On 04/16/2018 09:56 AM, Stefan Agner wrote: >> On 27.03.2018 14:16, Dmitry Osipenko wrote: >>> On 27.03.2018 14:54, Robin Murphy wrote: >>>> On 26/03/18 22:20, Dmitry Osipenko wrote: >>>>> On 25.03.2018 21:09, Stefan Agner wrote: >>>>>> As documented in GCC naked functions should only use Basic asm >>>>>> syntax. The Extended asm or mixture of Basic asm and "C" code is >>>>>> not guaranteed. Currently this works because it was hard coded >>>>>> to follow and check GCC behavior for arguments and register >>>>>> placement. >>>>>> >>>>>> Furthermore with clang using parameters in Extended asm in a >>>>>> naked function is not supported: >>>>>> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter >>>>>> references not allowed in naked functions >>>>>> : "r" (type), "r" (arg1), "r" (arg2) >>>>>> ^ >>>>>> >>>>>> Use a regular function to be more portable. This aligns also with >>>>>> the other smc call implementations e.g. in qcom_scm-32.c and >>>>>> bcm_kona_smc.c. >>>>>> >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com> >>>>>> Cc: Stephen Warren <swarren@nvidia.com> >>>>>> Cc: Thierry Reding <treding@nvidia.com> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>>> --- >>>>>> Changes in v2: >>>>>> - Keep stmfd/ldmfd to avoid potential ABI issues >>>>>> >>>>>> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- >>>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c >>>>>> b/arch/arm/firmware/trusted_foundations.c >>>>>> index 3fb1b5a1dce9..689e6565abfc 100644 >>>>>> --- a/arch/arm/firmware/trusted_foundations.c >>>>>> +++ b/arch/arm/firmware/trusted_foundations.c >>>>>> @@ -31,21 +31,25 @@ >>>>>> static unsigned long cpu_boot_addr; >>>>>> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) >>>>>> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) >>>>>> { >>>>>> + register u32 r0 asm("r0") = type; >>>>>> + register u32 r1 asm("r1") = arg1; >>>>>> + register u32 r2 asm("r2") = arg2; >>>>>> + >>>>>> asm volatile( >>>>>> ".arch_extension sec\n\t" >>>>>> - "stmfd sp!, {r4 - r11, lr}\n\t" >>>>>> + "stmfd sp!, {r4 - r11}\n\t" >>>>>> __asmeq("%0", "r0") >>>>>> __asmeq("%1", "r1") >>>>>> __asmeq("%2", "r2") >>>>>> "mov r3, #0\n\t" >>>>>> "mov r4, #0\n\t" >>>>>> "smc #0\n\t" >>>>>> - "ldmfd sp!, {r4 - r11, pc}" >>>>>> + "ldmfd sp!, {r4 - r11}\n\t" >>>>>> : >>>>>> - : "r" (type), "r" (arg1), "r" (arg2) >>>>>> - : "memory"); >>>>>> + : "r" (r0), "r" (r1), "r" (r2) >>>>>> + : "memory", "r3", "r12", "lr"); >>>>> >>>>> Although seems "lr" won't be affected by SMC invocation because it should be >>>>> banked and hence could be omitted entirely from the code. Maybe somebody could >>>>> confirm this. >>>> Strictly per the letter of the architecture, the SMC could be trapped to Hyp >>>> mode, and a hypervisor might clobber LR_usr in the process of forwarding the >>>> call to the firmware secure monitor (since Hyp doesn't have a banked LR of its >>>> own). Admittedly there are probably no real systems with the appropriate >>>> hardware/software combination to hit that, but on the other hand if this gets >>>> inlined where the compiler has already created a stack frame then an LR clobber >>>> is essentially free, so I reckon we're better off keeping it for reassurance. >>>> This isn't exactly a critical fast path anyway. >>> >>> Okay, thank you for the clarification. >> >> So it seems this change is fine? >> >> Stephen, you picked up changes for this driver before, is this patch >> going through your tree? > > You had best ask Thierry; he's taken over Tegra maintenance upstream. > But that said, don't files in arch/arm go through Russell? I think the last patches applied to that file went through your tree. Thierry, Russel, any preferences? -- Stefan
On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote: > On 16.04.2018 18:08, Stephen Warren wrote: > > On 04/16/2018 09:56 AM, Stefan Agner wrote: > >> On 27.03.2018 14:16, Dmitry Osipenko wrote: > >>> On 27.03.2018 14:54, Robin Murphy wrote: > >>>> On 26/03/18 22:20, Dmitry Osipenko wrote: > >>>>> On 25.03.2018 21:09, Stefan Agner wrote: > >>>>>> As documented in GCC naked functions should only use Basic asm > >>>>>> syntax. The Extended asm or mixture of Basic asm and "C" code is > >>>>>> not guaranteed. Currently this works because it was hard coded > >>>>>> to follow and check GCC behavior for arguments and register > >>>>>> placement. > >>>>>> > >>>>>> Furthermore with clang using parameters in Extended asm in a > >>>>>> naked function is not supported: > >>>>>> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter > >>>>>> references not allowed in naked functions > >>>>>> : "r" (type), "r" (arg1), "r" (arg2) > >>>>>> ^ > >>>>>> > >>>>>> Use a regular function to be more portable. This aligns also with > >>>>>> the other smc call implementations e.g. in qcom_scm-32.c and > >>>>>> bcm_kona_smc.c. > >>>>>> > >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com> > >>>>>> Cc: Stephen Warren <swarren@nvidia.com> > >>>>>> Cc: Thierry Reding <treding@nvidia.com> > >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> > >>>>>> --- > >>>>>> Changes in v2: > >>>>>> - Keep stmfd/ldmfd to avoid potential ABI issues > >>>>>> > >>>>>> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- > >>>>>> 1 file changed, 9 insertions(+), 5 deletions(-) > >>>>>> > >>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c > >>>>>> b/arch/arm/firmware/trusted_foundations.c > >>>>>> index 3fb1b5a1dce9..689e6565abfc 100644 > >>>>>> --- a/arch/arm/firmware/trusted_foundations.c > >>>>>> +++ b/arch/arm/firmware/trusted_foundations.c > >>>>>> @@ -31,21 +31,25 @@ > >>>>>> static unsigned long cpu_boot_addr; > >>>>>> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) > >>>>>> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) > >>>>>> { > >>>>>> + register u32 r0 asm("r0") = type; > >>>>>> + register u32 r1 asm("r1") = arg1; > >>>>>> + register u32 r2 asm("r2") = arg2; > >>>>>> + > >>>>>> asm volatile( > >>>>>> ".arch_extension sec\n\t" > >>>>>> - "stmfd sp!, {r4 - r11, lr}\n\t" > >>>>>> + "stmfd sp!, {r4 - r11}\n\t" > >>>>>> __asmeq("%0", "r0") > >>>>>> __asmeq("%1", "r1") > >>>>>> __asmeq("%2", "r2") > >>>>>> "mov r3, #0\n\t" > >>>>>> "mov r4, #0\n\t" > >>>>>> "smc #0\n\t" > >>>>>> - "ldmfd sp!, {r4 - r11, pc}" > >>>>>> + "ldmfd sp!, {r4 - r11}\n\t" > >>>>>> : > >>>>>> - : "r" (type), "r" (arg1), "r" (arg2) > >>>>>> - : "memory"); > >>>>>> + : "r" (r0), "r" (r1), "r" (r2) > >>>>>> + : "memory", "r3", "r12", "lr"); > >>>>> > >>>>> Although seems "lr" won't be affected by SMC invocation because it should be > >>>>> banked and hence could be omitted entirely from the code. Maybe somebody could > >>>>> confirm this. > >>>> Strictly per the letter of the architecture, the SMC could be trapped to Hyp > >>>> mode, and a hypervisor might clobber LR_usr in the process of forwarding the > >>>> call to the firmware secure monitor (since Hyp doesn't have a banked LR of its > >>>> own). Admittedly there are probably no real systems with the appropriate > >>>> hardware/software combination to hit that, but on the other hand if this gets > >>>> inlined where the compiler has already created a stack frame then an LR clobber > >>>> is essentially free, so I reckon we're better off keeping it for reassurance. > >>>> This isn't exactly a critical fast path anyway. > >>> > >>> Okay, thank you for the clarification. > >> > >> So it seems this change is fine? > >> > >> Stephen, you picked up changes for this driver before, is this patch > >> going through your tree? > > > > You had best ask Thierry; he's taken over Tegra maintenance upstream. > > But that said, don't files in arch/arm go through Russell? > > I think the last patches applied to that file went through your tree. > > Thierry, Russel, any preferences? I don't mind picking this up into the Tegra tree. Might be a good idea to move this into drivers/firmware, though, since that's where all the other firmware-related drivers reside. Firmware code, such as the BPMP driver, usually goes through ARM-SoC these days. I think this is in the same category. Russell, any objections to me picking this patch up and moving it into drivers/firmware? Thanks, Thierry
On 16.04.2018 21:21, Stefan Agner wrote: > On 16.04.2018 18:08, Stephen Warren wrote: >> On 04/16/2018 09:56 AM, Stefan Agner wrote: >>> On 27.03.2018 14:16, Dmitry Osipenko wrote: >>>> On 27.03.2018 14:54, Robin Murphy wrote: >>>>> On 26/03/18 22:20, Dmitry Osipenko wrote: >>>>>> On 25.03.2018 21:09, Stefan Agner wrote: >>>>>>> As documented in GCC naked functions should only use Basic asm >>>>>>> syntax. The Extended asm or mixture of Basic asm and "C" code is >>>>>>> not guaranteed. Currently this works because it was hard coded >>>>>>> to follow and check GCC behavior for arguments and register >>>>>>> placement. >>>>>>> >>>>>>> Furthermore with clang using parameters in Extended asm in a >>>>>>> naked function is not supported: >>>>>>> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter >>>>>>> references not allowed in naked functions >>>>>>> : "r" (type), "r" (arg1), "r" (arg2) >>>>>>> ^ >>>>>>> >>>>>>> Use a regular function to be more portable. This aligns also with >>>>>>> the other smc call implementations e.g. in qcom_scm-32.c and >>>>>>> bcm_kona_smc.c. >>>>>>> >>>>>>> Cc: Dmitry Osipenko <digetx@gmail.com> >>>>>>> Cc: Stephen Warren <swarren@nvidia.com> >>>>>>> Cc: Thierry Reding <treding@nvidia.com> >>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>>>> --- >>>>>>> Changes in v2: >>>>>>> - Keep stmfd/ldmfd to avoid potential ABI issues >>>>>>> >>>>>>> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- >>>>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c >>>>>>> b/arch/arm/firmware/trusted_foundations.c >>>>>>> index 3fb1b5a1dce9..689e6565abfc 100644 >>>>>>> --- a/arch/arm/firmware/trusted_foundations.c >>>>>>> +++ b/arch/arm/firmware/trusted_foundations.c >>>>>>> @@ -31,21 +31,25 @@ >>>>>>> static unsigned long cpu_boot_addr; >>>>>>> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) >>>>>>> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) >>>>>>> { >>>>>>> + register u32 r0 asm("r0") = type; >>>>>>> + register u32 r1 asm("r1") = arg1; >>>>>>> + register u32 r2 asm("r2") = arg2; >>>>>>> + >>>>>>> asm volatile( >>>>>>> ".arch_extension sec\n\t" >>>>>>> - "stmfd sp!, {r4 - r11, lr}\n\t" >>>>>>> + "stmfd sp!, {r4 - r11}\n\t" >>>>>>> __asmeq("%0", "r0") >>>>>>> __asmeq("%1", "r1") >>>>>>> __asmeq("%2", "r2") >>>>>>> "mov r3, #0\n\t" >>>>>>> "mov r4, #0\n\t" >>>>>>> "smc #0\n\t" >>>>>>> - "ldmfd sp!, {r4 - r11, pc}" >>>>>>> + "ldmfd sp!, {r4 - r11}\n\t" >>>>>>> : >>>>>>> - : "r" (type), "r" (arg1), "r" (arg2) >>>>>>> - : "memory"); >>>>>>> + : "r" (r0), "r" (r1), "r" (r2) >>>>>>> + : "memory", "r3", "r12", "lr"); >>>>>> >>>>>> Although seems "lr" won't be affected by SMC invocation because it should be >>>>>> banked and hence could be omitted entirely from the code. Maybe somebody could >>>>>> confirm this. >>>>> Strictly per the letter of the architecture, the SMC could be trapped to Hyp >>>>> mode, and a hypervisor might clobber LR_usr in the process of forwarding the >>>>> call to the firmware secure monitor (since Hyp doesn't have a banked LR of its >>>>> own). Admittedly there are probably no real systems with the appropriate >>>>> hardware/software combination to hit that, but on the other hand if this gets >>>>> inlined where the compiler has already created a stack frame then an LR clobber >>>>> is essentially free, so I reckon we're better off keeping it for reassurance. >>>>> This isn't exactly a critical fast path anyway. >>>> >>>> Okay, thank you for the clarification. >>> >>> So it seems this change is fine? >>> >>> Stephen, you picked up changes for this driver before, is this patch >>> going through your tree? >> >> You had best ask Thierry; he's taken over Tegra maintenance upstream. >> But that said, don't files in arch/arm go through Russell? > > I think the last patches applied to that file went through your tree. > > Thierry, Russel, any preferences? I've been preparing patches for upstream to add initial support of L2 cache maintance to TF / Tegra30 and noticed that without this patch I'm getting a hang early in boot. That is because before this patch registers store / restore was incorrect, probably the premature return (lr -> pc) causes stack corruption. Not sure whether it's worth to backport this patch, but I want to see it at least in -next. Thierry, please take care of this patch. Thanks.
On 17.04.2018 10:11, Thierry Reding wrote: > On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote: >> On 16.04.2018 18:08, Stephen Warren wrote: >> > On 04/16/2018 09:56 AM, Stefan Agner wrote: >> >> On 27.03.2018 14:16, Dmitry Osipenko wrote: >> >>> On 27.03.2018 14:54, Robin Murphy wrote: >> >>>> On 26/03/18 22:20, Dmitry Osipenko wrote: >> >>>>> On 25.03.2018 21:09, Stefan Agner wrote: >> >>>>>> As documented in GCC naked functions should only use Basic asm >> >>>>>> syntax. The Extended asm or mixture of Basic asm and "C" code is >> >>>>>> not guaranteed. Currently this works because it was hard coded >> >>>>>> to follow and check GCC behavior for arguments and register >> >>>>>> placement. >> >>>>>> >> >>>>>> Furthermore with clang using parameters in Extended asm in a >> >>>>>> naked function is not supported: >> >>>>>> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter >> >>>>>> references not allowed in naked functions >> >>>>>> : "r" (type), "r" (arg1), "r" (arg2) >> >>>>>> ^ >> >>>>>> >> >>>>>> Use a regular function to be more portable. This aligns also with >> >>>>>> the other smc call implementations e.g. in qcom_scm-32.c and >> >>>>>> bcm_kona_smc.c. >> >>>>>> >> >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com> >> >>>>>> Cc: Stephen Warren <swarren@nvidia.com> >> >>>>>> Cc: Thierry Reding <treding@nvidia.com> >> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >> >>>>>> --- >> >>>>>> Changes in v2: >> >>>>>> - Keep stmfd/ldmfd to avoid potential ABI issues >> >>>>>> >> >>>>>> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- >> >>>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >> >>>>>> >> >>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c >> >>>>>> b/arch/arm/firmware/trusted_foundations.c >> >>>>>> index 3fb1b5a1dce9..689e6565abfc 100644 >> >>>>>> --- a/arch/arm/firmware/trusted_foundations.c >> >>>>>> +++ b/arch/arm/firmware/trusted_foundations.c >> >>>>>> @@ -31,21 +31,25 @@ >> >>>>>> static unsigned long cpu_boot_addr; >> >>>>>> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) >> >>>>>> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) >> >>>>>> { >> >>>>>> + register u32 r0 asm("r0") = type; >> >>>>>> + register u32 r1 asm("r1") = arg1; >> >>>>>> + register u32 r2 asm("r2") = arg2; >> >>>>>> + >> >>>>>> asm volatile( >> >>>>>> ".arch_extension sec\n\t" >> >>>>>> - "stmfd sp!, {r4 - r11, lr}\n\t" >> >>>>>> + "stmfd sp!, {r4 - r11}\n\t" >> >>>>>> __asmeq("%0", "r0") >> >>>>>> __asmeq("%1", "r1") >> >>>>>> __asmeq("%2", "r2") >> >>>>>> "mov r3, #0\n\t" >> >>>>>> "mov r4, #0\n\t" >> >>>>>> "smc #0\n\t" >> >>>>>> - "ldmfd sp!, {r4 - r11, pc}" >> >>>>>> + "ldmfd sp!, {r4 - r11}\n\t" >> >>>>>> : >> >>>>>> - : "r" (type), "r" (arg1), "r" (arg2) >> >>>>>> - : "memory"); >> >>>>>> + : "r" (r0), "r" (r1), "r" (r2) >> >>>>>> + : "memory", "r3", "r12", "lr"); >> >>>>> >> >>>>> Although seems "lr" won't be affected by SMC invocation because it should be >> >>>>> banked and hence could be omitted entirely from the code. Maybe somebody could >> >>>>> confirm this. >> >>>> Strictly per the letter of the architecture, the SMC could be trapped to Hyp >> >>>> mode, and a hypervisor might clobber LR_usr in the process of forwarding the >> >>>> call to the firmware secure monitor (since Hyp doesn't have a banked LR of its >> >>>> own). Admittedly there are probably no real systems with the appropriate >> >>>> hardware/software combination to hit that, but on the other hand if this gets >> >>>> inlined where the compiler has already created a stack frame then an LR clobber >> >>>> is essentially free, so I reckon we're better off keeping it for reassurance. >> >>>> This isn't exactly a critical fast path anyway. >> >>> >> >>> Okay, thank you for the clarification. >> >> >> >> So it seems this change is fine? >> >> >> >> Stephen, you picked up changes for this driver before, is this patch >> >> going through your tree? >> > >> > You had best ask Thierry; he's taken over Tegra maintenance upstream. >> > But that said, don't files in arch/arm go through Russell? >> >> I think the last patches applied to that file went through your tree. >> >> Thierry, Russel, any preferences? > > I don't mind picking this up into the Tegra tree. Might be a good idea > to move this into drivers/firmware, though, since that's where all the > other firmware-related drivers reside. > > Firmware code, such as the BPMP driver, usually goes through ARM-SoC > these days. I think this is in the same category. > > Russell, any objections to me picking this patch up and moving it into > drivers/firmware? Russel, I think Thierry is waiting for your ok on this. -- Stefan
On Tue, Apr 17, 2018 at 1:11 AM, Thierry Reding <treding@nvidia.com> wrote: > On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote: >> On 16.04.2018 18:08, Stephen Warren wrote: >> > On 04/16/2018 09:56 AM, Stefan Agner wrote: >> >> On 27.03.2018 14:16, Dmitry Osipenko wrote: >> >>> On 27.03.2018 14:54, Robin Murphy wrote: >> >>>> On 26/03/18 22:20, Dmitry Osipenko wrote: >> >>>>> On 25.03.2018 21:09, Stefan Agner wrote: >> >>>>>> As documented in GCC naked functions should only use Basic asm >> >>>>>> syntax. The Extended asm or mixture of Basic asm and "C" code is >> >>>>>> not guaranteed. Currently this works because it was hard coded >> >>>>>> to follow and check GCC behavior for arguments and register >> >>>>>> placement. >> >>>>>> >> >>>>>> Furthermore with clang using parameters in Extended asm in a >> >>>>>> naked function is not supported: >> >>>>>> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter >> >>>>>> references not allowed in naked functions >> >>>>>> : "r" (type), "r" (arg1), "r" (arg2) >> >>>>>> ^ >> >>>>>> >> >>>>>> Use a regular function to be more portable. This aligns also with >> >>>>>> the other smc call implementations e.g. in qcom_scm-32.c and >> >>>>>> bcm_kona_smc.c. >> >>>>>> >> >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com> >> >>>>>> Cc: Stephen Warren <swarren@nvidia.com> >> >>>>>> Cc: Thierry Reding <treding@nvidia.com> >> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >> >>>>>> --- >> >>>>>> Changes in v2: >> >>>>>> - Keep stmfd/ldmfd to avoid potential ABI issues >> >>>>>> >> >>>>>> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- >> >>>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >> >>>>>> >> >>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c >> >>>>>> b/arch/arm/firmware/trusted_foundations.c >> >>>>>> index 3fb1b5a1dce9..689e6565abfc 100644 >> >>>>>> --- a/arch/arm/firmware/trusted_foundations.c >> >>>>>> +++ b/arch/arm/firmware/trusted_foundations.c >> >>>>>> @@ -31,21 +31,25 @@ >> >>>>>> static unsigned long cpu_boot_addr; >> >>>>>> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) >> >>>>>> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) >> >>>>>> { >> >>>>>> + register u32 r0 asm("r0") = type; >> >>>>>> + register u32 r1 asm("r1") = arg1; >> >>>>>> + register u32 r2 asm("r2") = arg2; >> >>>>>> + >> >>>>>> asm volatile( >> >>>>>> ".arch_extension sec\n\t" >> >>>>>> - "stmfd sp!, {r4 - r11, lr}\n\t" >> >>>>>> + "stmfd sp!, {r4 - r11}\n\t" >> >>>>>> __asmeq("%0", "r0") >> >>>>>> __asmeq("%1", "r1") >> >>>>>> __asmeq("%2", "r2") >> >>>>>> "mov r3, #0\n\t" >> >>>>>> "mov r4, #0\n\t" >> >>>>>> "smc #0\n\t" >> >>>>>> - "ldmfd sp!, {r4 - r11, pc}" >> >>>>>> + "ldmfd sp!, {r4 - r11}\n\t" >> >>>>>> : >> >>>>>> - : "r" (type), "r" (arg1), "r" (arg2) >> >>>>>> - : "memory"); >> >>>>>> + : "r" (r0), "r" (r1), "r" (r2) >> >>>>>> + : "memory", "r3", "r12", "lr"); >> >>>>> >> >>>>> Although seems "lr" won't be affected by SMC invocation because it should be >> >>>>> banked and hence could be omitted entirely from the code. Maybe somebody could >> >>>>> confirm this. >> >>>> Strictly per the letter of the architecture, the SMC could be trapped to Hyp >> >>>> mode, and a hypervisor might clobber LR_usr in the process of forwarding the >> >>>> call to the firmware secure monitor (since Hyp doesn't have a banked LR of its >> >>>> own). Admittedly there are probably no real systems with the appropriate >> >>>> hardware/software combination to hit that, but on the other hand if this gets >> >>>> inlined where the compiler has already created a stack frame then an LR clobber >> >>>> is essentially free, so I reckon we're better off keeping it for reassurance. >> >>>> This isn't exactly a critical fast path anyway. >> >>> >> >>> Okay, thank you for the clarification. >> >> >> >> So it seems this change is fine? >> >> >> >> Stephen, you picked up changes for this driver before, is this patch >> >> going through your tree? >> > >> > You had best ask Thierry; he's taken over Tegra maintenance upstream. >> > But that said, don't files in arch/arm go through Russell? >> >> I think the last patches applied to that file went through your tree. >> >> Thierry, Russel, any preferences? > > I don't mind picking this up into the Tegra tree. Might be a good idea > to move this into drivers/firmware, though, since that's where all the > other firmware-related drivers reside. > > Firmware code, such as the BPMP driver, usually goes through ARM-SoC > these days. I think this is in the same category. > > Russell, any objections to me picking this patch up and moving it into > drivers/firmware? Please take this -- without it I'm seeing build failures on the arm allmodconfig under gcc 7.3.0: /tmp/ccKdsC59.s: Assembler messages: /tmp/ccKdsC59.s:35: Error: .err encountered /tmp/ccKdsC59.s:36: Error: .err encountered /tmp/ccKdsC59.s:37: Error: .err encountered scripts/Makefile.build:317: recipe for target 'arch/arm/firmware/trusted_foundations.o' failed The above patch fixes it for me. Thanks! -Kees
On Sun, Mar 25, 2018 at 08:09:56PM +0200, Stefan Agner wrote: > As documented in GCC naked functions should only use Basic asm > syntax. The Extended asm or mixture of Basic asm and "C" code is > not guaranteed. Currently this works because it was hard coded > to follow and check GCC behavior for arguments and register > placement. > > Furthermore with clang using parameters in Extended asm in a > naked function is not supported: > arch/arm/firmware/trusted_foundations.c:47:10: error: parameter > references not allowed in naked functions > : "r" (type), "r" (arg1), "r" (arg2) > ^ > > Use a regular function to be more portable. This aligns also with > the other smc call implementations e.g. in qcom_scm-32.c and > bcm_kona_smc.c. > > Cc: Dmitry Osipenko <digetx@gmail.com> > Cc: Stephen Warren <swarren@nvidia.com> > Cc: Thierry Reding <treding@nvidia.com> > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > Changes in v2: > - Keep stmfd/ldmfd to avoid potential ABI issues > > arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c > index 3fb1b5a1dce9..689e6565abfc 100644 > --- a/arch/arm/firmware/trusted_foundations.c > +++ b/arch/arm/firmware/trusted_foundations.c > @@ -31,21 +31,25 @@ > > static unsigned long cpu_boot_addr; > > -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) > +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) > { > + register u32 r0 asm("r0") = type; > + register u32 r1 asm("r1") = arg1; > + register u32 r2 asm("r2") = arg2; > + > asm volatile( > ".arch_extension sec\n\t" > - "stmfd sp!, {r4 - r11, lr}\n\t" > + "stmfd sp!, {r4 - r11}\n\t" > __asmeq("%0", "r0") > __asmeq("%1", "r1") > __asmeq("%2", "r2") > "mov r3, #0\n\t" > "mov r4, #0\n\t" > "smc #0\n\t" > - "ldmfd sp!, {r4 - r11, pc}" > + "ldmfd sp!, {r4 - r11}\n\t" > : > - : "r" (type), "r" (arg1), "r" (arg2) > - : "memory"); > + : "r" (r0), "r" (r1), "r" (r2) > + : "memory", "r3", "r12", "lr"); > } Does GCC try to inline this? It may just be better to switch to basic asm. We know that a naked function won't be inlined, and we already know (because we need the prologue/epilogue) what registers the 32-bit arguments will be in.
On Thu, Jul 12, 2018 at 03:43:10PM -0700, Kees Cook wrote: > On Tue, Apr 17, 2018 at 1:11 AM, Thierry Reding <treding@nvidia.com> wrote: > > On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote: > >> On 16.04.2018 18:08, Stephen Warren wrote: > >> > On 04/16/2018 09:56 AM, Stefan Agner wrote: > >> >> On 27.03.2018 14:16, Dmitry Osipenko wrote: > >> >>> On 27.03.2018 14:54, Robin Murphy wrote: > >> >>>> On 26/03/18 22:20, Dmitry Osipenko wrote: > >> >>>>> On 25.03.2018 21:09, Stefan Agner wrote: > >> >>>>>> As documented in GCC naked functions should only use Basic asm > >> >>>>>> syntax. The Extended asm or mixture of Basic asm and "C" code is > >> >>>>>> not guaranteed. Currently this works because it was hard coded > >> >>>>>> to follow and check GCC behavior for arguments and register > >> >>>>>> placement. > >> >>>>>> > >> >>>>>> Furthermore with clang using parameters in Extended asm in a > >> >>>>>> naked function is not supported: > >> >>>>>> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter > >> >>>>>> references not allowed in naked functions > >> >>>>>> : "r" (type), "r" (arg1), "r" (arg2) > >> >>>>>> ^ > >> >>>>>> > >> >>>>>> Use a regular function to be more portable. This aligns also with > >> >>>>>> the other smc call implementations e.g. in qcom_scm-32.c and > >> >>>>>> bcm_kona_smc.c. > >> >>>>>> > >> >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com> > >> >>>>>> Cc: Stephen Warren <swarren@nvidia.com> > >> >>>>>> Cc: Thierry Reding <treding@nvidia.com> > >> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> > >> >>>>>> --- > >> >>>>>> Changes in v2: > >> >>>>>> - Keep stmfd/ldmfd to avoid potential ABI issues > >> >>>>>> > >> >>>>>> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- > >> >>>>>> 1 file changed, 9 insertions(+), 5 deletions(-) > >> >>>>>> > >> >>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c > >> >>>>>> b/arch/arm/firmware/trusted_foundations.c > >> >>>>>> index 3fb1b5a1dce9..689e6565abfc 100644 > >> >>>>>> --- a/arch/arm/firmware/trusted_foundations.c > >> >>>>>> +++ b/arch/arm/firmware/trusted_foundations.c > >> >>>>>> @@ -31,21 +31,25 @@ > >> >>>>>> static unsigned long cpu_boot_addr; > >> >>>>>> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) > >> >>>>>> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) > >> >>>>>> { > >> >>>>>> + register u32 r0 asm("r0") = type; > >> >>>>>> + register u32 r1 asm("r1") = arg1; > >> >>>>>> + register u32 r2 asm("r2") = arg2; > >> >>>>>> + > >> >>>>>> asm volatile( > >> >>>>>> ".arch_extension sec\n\t" > >> >>>>>> - "stmfd sp!, {r4 - r11, lr}\n\t" > >> >>>>>> + "stmfd sp!, {r4 - r11}\n\t" > >> >>>>>> __asmeq("%0", "r0") > >> >>>>>> __asmeq("%1", "r1") > >> >>>>>> __asmeq("%2", "r2") > >> >>>>>> "mov r3, #0\n\t" > >> >>>>>> "mov r4, #0\n\t" > >> >>>>>> "smc #0\n\t" > >> >>>>>> - "ldmfd sp!, {r4 - r11, pc}" > >> >>>>>> + "ldmfd sp!, {r4 - r11}\n\t" > >> >>>>>> : > >> >>>>>> - : "r" (type), "r" (arg1), "r" (arg2) > >> >>>>>> - : "memory"); > >> >>>>>> + : "r" (r0), "r" (r1), "r" (r2) > >> >>>>>> + : "memory", "r3", "r12", "lr"); > >> >>>>> > >> >>>>> Although seems "lr" won't be affected by SMC invocation because it should be > >> >>>>> banked and hence could be omitted entirely from the code. Maybe somebody could > >> >>>>> confirm this. > >> >>>> Strictly per the letter of the architecture, the SMC could be trapped to Hyp > >> >>>> mode, and a hypervisor might clobber LR_usr in the process of forwarding the > >> >>>> call to the firmware secure monitor (since Hyp doesn't have a banked LR of its > >> >>>> own). Admittedly there are probably no real systems with the appropriate > >> >>>> hardware/software combination to hit that, but on the other hand if this gets > >> >>>> inlined where the compiler has already created a stack frame then an LR clobber > >> >>>> is essentially free, so I reckon we're better off keeping it for reassurance. > >> >>>> This isn't exactly a critical fast path anyway. > >> >>> > >> >>> Okay, thank you for the clarification. > >> >> > >> >> So it seems this change is fine? > >> >> > >> >> Stephen, you picked up changes for this driver before, is this patch > >> >> going through your tree? > >> > > >> > You had best ask Thierry; he's taken over Tegra maintenance upstream. > >> > But that said, don't files in arch/arm go through Russell? > >> > >> I think the last patches applied to that file went through your tree. > >> > >> Thierry, Russel, any preferences? > > > > I don't mind picking this up into the Tegra tree. Might be a good idea > > to move this into drivers/firmware, though, since that's where all the > > other firmware-related drivers reside. > > > > Firmware code, such as the BPMP driver, usually goes through ARM-SoC > > these days. I think this is in the same category. > > > > Russell, any objections to me picking this patch up and moving it into > > drivers/firmware? > > Please take this -- without it I'm seeing build failures on the arm > allmodconfig under gcc 7.3.0: Sorry, I'd completely missed this... now replied on the original patch.
On 13.07.2018 01:01, Russell King - ARM Linux wrote: > On Thu, Jul 12, 2018 at 03:43:10PM -0700, Kees Cook wrote: >> On Tue, Apr 17, 2018 at 1:11 AM, Thierry Reding <treding@nvidia.com> wrote: >> > On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote: >> >> On 16.04.2018 18:08, Stephen Warren wrote: >> >> > On 04/16/2018 09:56 AM, Stefan Agner wrote: >> >> >> On 27.03.2018 14:16, Dmitry Osipenko wrote: >> >> >>> On 27.03.2018 14:54, Robin Murphy wrote: >> >> >>>> On 26/03/18 22:20, Dmitry Osipenko wrote: >> >> >>>>> On 25.03.2018 21:09, Stefan Agner wrote: >> >> >>>>>> As documented in GCC naked functions should only use Basic asm >> >> >>>>>> syntax. The Extended asm or mixture of Basic asm and "C" code is >> >> >>>>>> not guaranteed. Currently this works because it was hard coded >> >> >>>>>> to follow and check GCC behavior for arguments and register >> >> >>>>>> placement. >> >> >>>>>> >> >> >>>>>> Furthermore with clang using parameters in Extended asm in a >> >> >>>>>> naked function is not supported: >> >> >>>>>> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter >> >> >>>>>> references not allowed in naked functions >> >> >>>>>> : "r" (type), "r" (arg1), "r" (arg2) >> >> >>>>>> ^ >> >> >>>>>> >> >> >>>>>> Use a regular function to be more portable. This aligns also with >> >> >>>>>> the other smc call implementations e.g. in qcom_scm-32.c and >> >> >>>>>> bcm_kona_smc.c. >> >> >>>>>> >> >> >>>>>> Cc: Dmitry Osipenko <digetx@gmail.com> >> >> >>>>>> Cc: Stephen Warren <swarren@nvidia.com> >> >> >>>>>> Cc: Thierry Reding <treding@nvidia.com> >> >> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >> >> >>>>>> --- >> >> >>>>>> Changes in v2: >> >> >>>>>> - Keep stmfd/ldmfd to avoid potential ABI issues >> >> >>>>>> >> >> >>>>>> arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- >> >> >>>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> >>>>>> >> >> >>>>>> diff --git a/arch/arm/firmware/trusted_foundations.c >> >> >>>>>> b/arch/arm/firmware/trusted_foundations.c >> >> >>>>>> index 3fb1b5a1dce9..689e6565abfc 100644 >> >> >>>>>> --- a/arch/arm/firmware/trusted_foundations.c >> >> >>>>>> +++ b/arch/arm/firmware/trusted_foundations.c >> >> >>>>>> @@ -31,21 +31,25 @@ >> >> >>>>>> static unsigned long cpu_boot_addr; >> >> >>>>>> -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) >> >> >>>>>> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) >> >> >>>>>> { >> >> >>>>>> + register u32 r0 asm("r0") = type; >> >> >>>>>> + register u32 r1 asm("r1") = arg1; >> >> >>>>>> + register u32 r2 asm("r2") = arg2; >> >> >>>>>> + >> >> >>>>>> asm volatile( >> >> >>>>>> ".arch_extension sec\n\t" >> >> >>>>>> - "stmfd sp!, {r4 - r11, lr}\n\t" >> >> >>>>>> + "stmfd sp!, {r4 - r11}\n\t" >> >> >>>>>> __asmeq("%0", "r0") >> >> >>>>>> __asmeq("%1", "r1") >> >> >>>>>> __asmeq("%2", "r2") >> >> >>>>>> "mov r3, #0\n\t" >> >> >>>>>> "mov r4, #0\n\t" >> >> >>>>>> "smc #0\n\t" >> >> >>>>>> - "ldmfd sp!, {r4 - r11, pc}" >> >> >>>>>> + "ldmfd sp!, {r4 - r11}\n\t" >> >> >>>>>> : >> >> >>>>>> - : "r" (type), "r" (arg1), "r" (arg2) >> >> >>>>>> - : "memory"); >> >> >>>>>> + : "r" (r0), "r" (r1), "r" (r2) >> >> >>>>>> + : "memory", "r3", "r12", "lr"); >> >> >>>>> >> >> >>>>> Although seems "lr" won't be affected by SMC invocation because it should be >> >> >>>>> banked and hence could be omitted entirely from the code. Maybe somebody could >> >> >>>>> confirm this. >> >> >>>> Strictly per the letter of the architecture, the SMC could be trapped to Hyp >> >> >>>> mode, and a hypervisor might clobber LR_usr in the process of forwarding the >> >> >>>> call to the firmware secure monitor (since Hyp doesn't have a banked LR of its >> >> >>>> own). Admittedly there are probably no real systems with the appropriate >> >> >>>> hardware/software combination to hit that, but on the other hand if this gets >> >> >>>> inlined where the compiler has already created a stack frame then an LR clobber >> >> >>>> is essentially free, so I reckon we're better off keeping it for reassurance. >> >> >>>> This isn't exactly a critical fast path anyway. >> >> >>> >> >> >>> Okay, thank you for the clarification. >> >> >> >> >> >> So it seems this change is fine? >> >> >> >> >> >> Stephen, you picked up changes for this driver before, is this patch >> >> >> going through your tree? >> >> > >> >> > You had best ask Thierry; he's taken over Tegra maintenance upstream. >> >> > But that said, don't files in arch/arm go through Russell? >> >> >> >> I think the last patches applied to that file went through your tree. >> >> >> >> Thierry, Russel, any preferences? >> > >> > I don't mind picking this up into the Tegra tree. Might be a good idea >> > to move this into drivers/firmware, though, since that's where all the >> > other firmware-related drivers reside. >> > >> > Firmware code, such as the BPMP driver, usually goes through ARM-SoC >> > these days. I think this is in the same category. >> > >> > Russell, any objections to me picking this patch up and moving it into >> > drivers/firmware? >> >> Please take this -- without it I'm seeing build failures on the arm >> allmodconfig under gcc 7.3.0: > > Sorry, I'd completely missed this... now replied on the original patch. Thierry merged this patch just two days ago, so it is already in -next. -- Stefan
diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c index 3fb1b5a1dce9..689e6565abfc 100644 --- a/arch/arm/firmware/trusted_foundations.c +++ b/arch/arm/firmware/trusted_foundations.c @@ -31,21 +31,25 @@ static unsigned long cpu_boot_addr; -static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2) +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2) { + register u32 r0 asm("r0") = type; + register u32 r1 asm("r1") = arg1; + register u32 r2 asm("r2") = arg2; + asm volatile( ".arch_extension sec\n\t" - "stmfd sp!, {r4 - r11, lr}\n\t" + "stmfd sp!, {r4 - r11}\n\t" __asmeq("%0", "r0") __asmeq("%1", "r1") __asmeq("%2", "r2") "mov r3, #0\n\t" "mov r4, #0\n\t" "smc #0\n\t" - "ldmfd sp!, {r4 - r11, pc}" + "ldmfd sp!, {r4 - r11}\n\t" : - : "r" (type), "r" (arg1), "r" (arg2) - : "memory"); + : "r" (r0), "r" (r1), "r" (r2) + : "memory", "r3", "r12", "lr"); } static int tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
As documented in GCC naked functions should only use Basic asm syntax. The Extended asm or mixture of Basic asm and "C" code is not guaranteed. Currently this works because it was hard coded to follow and check GCC behavior for arguments and register placement. Furthermore with clang using parameters in Extended asm in a naked function is not supported: arch/arm/firmware/trusted_foundations.c:47:10: error: parameter references not allowed in naked functions : "r" (type), "r" (arg1), "r" (arg2) ^ Use a regular function to be more portable. This aligns also with the other smc call implementations e.g. in qcom_scm-32.c and bcm_kona_smc.c. Cc: Dmitry Osipenko <digetx@gmail.com> Cc: Stephen Warren <swarren@nvidia.com> Cc: Thierry Reding <treding@nvidia.com> Signed-off-by: Stefan Agner <stefan@agner.ch> --- Changes in v2: - Keep stmfd/ldmfd to avoid potential ABI issues arch/arm/firmware/trusted_foundations.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)