| Message ID | 1413214141-370-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 10/13/2014 04:29 PM, Julien Grall wrote: > "xen: arm: Add support for the Exynos secure firmware" introduced code > assuming that exynos_smc() would get called with arguments in certain > registers. While the "noinline" attribute guarantees the function to > not get inlined, it does not guarantee that all arguments arrive in the > assumed registers: gcc's interprocedural analysis can result in clone > functions to be created where some of the incoming arguments (commonly > when they have constant values) get replaced by putting in place the > respective values inside the clone. > > Xen contains in multiple place of this SMC function: consolidate the function > in a single place and write it in assembly. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- > This is a fix for Xen 4.5 to compile the hypervisor with GCC 4.9.1, used by > Fedora & co. > > Jan: I kept your Signed-off-by for the commit message. > > Changes in v2: > - Write the SMC call in assembly > - Consolidate the code in a single place I forgot to add that this patch has been build tested on both arm32 and arm64 and successfully boot on both midway and the Foundation Model. Regards,
>>> On 13.10.14 at 17:29, <julien.grall@linaro.org> wrote: > "xen: arm: Add support for the Exynos secure firmware" introduced code > assuming that exynos_smc() would get called with arguments in certain > registers. While the "noinline" attribute guarantees the function to > not get inlined, it does not guarantee that all arguments arrive in the > assumed registers: gcc's interprocedural analysis can result in clone > functions to be created where some of the incoming arguments (commonly > when they have constant values) get replaced by putting in place the > respective values inside the clone. > > Xen contains in multiple place of this SMC function: consolidate the > function > in a single place and write it in assembly. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- > This is a fix for Xen 4.5 to compile the hypervisor with GCC 4.9.1, used by > Fedora & co. > > Jan: I kept your Signed-off-by for the commit message. I don't think you should have - nothing of my original fix got retained afaict. IMO you should convert this to a Reported-by. Jan
>>> On 13.10.14 at 17:29, <julien.grall@linaro.org> wrote: > --- /dev/null > +++ b/xen/arch/arm/arm32/smc.S > @@ -0,0 +1,19 @@ > +/* > + * xen/arch/arm/arm32/smc.S > + * > + * Wrapper for Secure Monitors Calls > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +GLOBAL(do_smc) > + smc #0 > + mov pc, lr If you defined a little "ret" helper macro there wouldn't be a need for separate files for 32- and 64-bit. Jan
Hi Jan, On 10/13/2014 04:49 PM, Jan Beulich wrote: >>>> On 13.10.14 at 17:29, <julien.grall@linaro.org> wrote: >> "xen: arm: Add support for the Exynos secure firmware" introduced code >> assuming that exynos_smc() would get called with arguments in certain >> registers. While the "noinline" attribute guarantees the function to >> not get inlined, it does not guarantee that all arguments arrive in the >> assumed registers: gcc's interprocedural analysis can result in clone >> functions to be created where some of the incoming arguments (commonly >> when they have constant values) get replaced by putting in place the >> respective values inside the clone. >> >> Xen contains in multiple place of this SMC function: consolidate the >> function >> in a single place and write it in assembly. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- >> This is a fix for Xen 4.5 to compile the hypervisor with GCC 4.9.1, used by >> Fedora & co. >> >> Jan: I kept your Signed-off-by for the commit message. > > I don't think you should have - nothing of my original fix got retained > afaict. IMO you should convert this to a Reported-by. I wasn't sure if Signed-off-by was required when copying the commit message. I will replace the Signed-off-by by Reported-by. Regards,
Hi Jan, On 10/13/2014 04:51 PM, Jan Beulich wrote: >>>> On 13.10.14 at 17:29, <julien.grall@linaro.org> wrote: >> --- /dev/null >> +++ b/xen/arch/arm/arm32/smc.S >> @@ -0,0 +1,19 @@ >> +/* >> + * xen/arch/arm/arm32/smc.S >> + * >> + * Wrapper for Secure Monitors Calls >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +GLOBAL(do_smc) >> + smc #0 >> + mov pc, lr > > If you defined a little "ret" helper macro there wouldn't be a need > for separate files for 32- and 64-bit. Good point. I will give a look to this solution. Regards,
On Mon, 13 Oct 2014, Julien Grall wrote: > "xen: arm: Add support for the Exynos secure firmware" introduced code > assuming that exynos_smc() would get called with arguments in certain > registers. While the "noinline" attribute guarantees the function to > not get inlined, it does not guarantee that all arguments arrive in the > assumed registers: gcc's interprocedural analysis can result in clone > functions to be created where some of the incoming arguments (commonly > when they have constant values) get replaced by putting in place the > respective values inside the clone. > > Xen contains in multiple place of this SMC function: consolidate the function > in a single place and write it in assembly. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- > This is a fix for Xen 4.5 to compile the hypervisor with GCC 4.9.1, used by > Fedora & co. > > Jan: I kept your Signed-off-by for the commit message. > > Changes in v2: > - Write the SMC call in assembly > - Consolidate the code in a single place > --- > xen/arch/arm/arm32/Makefile | 1 + > xen/arch/arm/arm32/smc.S | 19 +++++++++++++++++++ > xen/arch/arm/arm64/Makefile | 1 + > xen/arch/arm/arm64/smc.S | 19 +++++++++++++++++++ > xen/arch/arm/platforms/exynos5.c | 15 +-------------- > xen/arch/arm/platforms/seattle.c | 12 ++---------- > xen/arch/arm/psci.c | 29 ++--------------------------- > xen/include/asm-arm/processor.h | 2 ++ > 8 files changed, 47 insertions(+), 51 deletions(-) > create mode 100644 xen/arch/arm/arm32/smc.S > create mode 100644 xen/arch/arm/arm64/smc.S > > diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile > index df0e7de..51b64f9 100644 > --- a/xen/arch/arm/arm32/Makefile > +++ b/xen/arch/arm/arm32/Makefile > @@ -8,5 +8,6 @@ obj-y += domain.o > obj-y += vfp.o > obj-y += smpboot.o > obj-y += domctl.o > +obj-y += smc.o > > obj-$(EARLY_PRINTK) += debug.o > diff --git a/xen/arch/arm/arm32/smc.S b/xen/arch/arm/arm32/smc.S > new file mode 100644 > index 0000000..ea1dba5 > --- /dev/null > +++ b/xen/arch/arm/arm32/smc.S > @@ -0,0 +1,19 @@ > +/* > + * xen/arch/arm/arm32/smc.S > + * > + * Wrapper for Secure Monitors Calls > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +GLOBAL(do_smc) > + smc #0 > + mov pc, lr > diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile > index c7243f5..4debaf4 100644 > --- a/xen/arch/arm/arm64/Makefile > +++ b/xen/arch/arm/arm64/Makefile > @@ -1,6 +1,7 @@ > subdir-y += lib > > obj-y += entry.o > +obj-y += smc.o > > obj-y += traps.o > obj-y += domain.o > diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S > new file mode 100644 > index 0000000..dfe3f02 > --- /dev/null > +++ b/xen/arch/arm/arm64/smc.S > @@ -0,0 +1,19 @@ > +/* > + * xen/arch/arm/arm64/smc.S > + * > + * Wrapper for Secure Monitors Calls > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +GLOBAL(do_smc) > + smc #0 > + ret [...] > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index e719c26..5b7385c 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -614,6 +614,8 @@ void vcpu_regs_hyp_to_user(const struct vcpu *vcpu, > void vcpu_regs_user_to_hyp(struct vcpu *vcpu, > const struct vcpu_guest_core_regs *regs); > > +int do_smc(register_t function_id, ...); I am not sure whether this is safe: the smc calling convention on arm64 doesn't promise to save the x0-x17 registers. The smc calling convention on arm32 only promises to save r4-r15. That means that after issuing an smc call with just two arguments, you could still find r3 to be changed afterwards. I think you'll have to manually save/restore all the registers outside the safety guarantees of the smc protocol. See: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0028a/index.html
Hi Stefano, On 10/13/2014 05:06 PM, Stefano Stabellini wrote: >> +int do_smc(register_t function_id, ...); > > I am not sure whether this is safe: the smc calling convention on arm64 > doesn't promise to save the x0-x17 registers. The smc calling convention > on arm32 only promises to save r4-r15. That means that after issuing an > smc call with just two arguments, you could still find r3 to be changed > afterwards. > I think you'll have to manually save/restore all the registers outside > the safety guarantees of the smc protocol. The calling convention specifies: - r0-r3 as argument/scratch register on ARM32 - x0-x17 as argument/temporary register on ARM64 So we don't need to save/restore them before/after calling the SMC function. Regards,
On Mon, 13 Oct 2014, Julien Grall wrote: > Hi Stefano, > > On 10/13/2014 05:06 PM, Stefano Stabellini wrote: > >> +int do_smc(register_t function_id, ...); > > > > I am not sure whether this is safe: the smc calling convention on arm64 > > doesn't promise to save the x0-x17 registers. The smc calling convention > > on arm32 only promises to save r4-r15. That means that after issuing an > > smc call with just two arguments, you could still find r3 to be changed > > afterwards. > > I think you'll have to manually save/restore all the registers outside > > the safety guarantees of the smc protocol. > > The calling convention specifies: > - r0-r3 as argument/scratch register on ARM32 > - x0-x17 as argument/temporary register on ARM64 > > So we don't need to save/restore them before/after calling the SMC function. I think you are right.
On 10/13/2014 06:01 PM, Stefano Stabellini wrote: > On Mon, 13 Oct 2014, Julien Grall wrote: >> Hi Stefano, >> >> On 10/13/2014 05:06 PM, Stefano Stabellini wrote: >>>> +int do_smc(register_t function_id, ...); >>> >>> I am not sure whether this is safe: the smc calling convention on arm64 >>> doesn't promise to save the x0-x17 registers. The smc calling convention >>> on arm32 only promises to save r4-r15. That means that after issuing an >>> smc call with just two arguments, you could still find r3 to be changed >>> afterwards. >>> I think you'll have to manually save/restore all the registers outside >>> the safety guarantees of the smc protocol. >> >> The calling convention specifies: >> - r0-r3 as argument/scratch register on ARM32 >> - x0-x17 as argument/temporary register on ARM64 >> >> So we don't need to save/restore them before/after calling the SMC function. > > I think you are right. I will resend a patch with Jan comments.
On Mon, 2014-10-13 at 16:29 +0100, Julien Grall wrote: > +GLOBAL(do_smc) > +GLOBAL(do_smc) These should both be ENTRY. > +int do_smc(register_t function_id, ...); Are you sure that the variadic function calling convention is the same as for a regular function call? I'm not entirely clear having read AAPCS, it says they are marshalled according to "the standard base". I think it would probably be safer to declare this guy as taking 3-4 arguments and pass in 0 for the unused one. You could wrap in do_smc<N>() helpers if you really wanted. I'd prefer to name this call_smc, do_foo tends to be used on the callee side of exception things (e.g. do_hypervisor_trap etc, do_psci_blah). Ian.
On 10/14/2014 10:15 AM, Ian Campbell wrote: > On Mon, 2014-10-13 at 16:29 +0100, Julien Grall wrote: > >> +GLOBAL(do_smc) > >> +GLOBAL(do_smc) > > These should both be ENTRY. Why? Is it because GLOBAL should be used for variable and ENTRY for function? >> +int do_smc(register_t function_id, ...); > > Are you sure that the variadic function calling convention is the same > as for a regular function call? I'm not entirely clear having read > AAPCS, it says they are marshalled according to "the standard base". All the parameters fits in a register, so the compiler will effectively use the first registers to pass arguments. This may be an issue if the user decides to pass an uint64_t on ARM32. > I think it would probably be safer to declare this guy as taking 3-4 > arguments and pass in 0 for the unused one. You could wrap in > do_smc<N>() helpers if you really wanted. I will introduce helpers. It will be easier if we decide to extend the number of parameters (SMC64 supports up to 5 parameters). > I'd prefer to name this call_smc, do_foo tends to be used on the callee > side of exception things (e.g. do_hypervisor_trap etc, do_psci_blah). Will make the change in the next version. Regards,
On Tue, 2014-10-14 at 13:43 +0100, Julien Grall wrote: > On 10/14/2014 10:15 AM, Ian Campbell wrote: > > On Mon, 2014-10-13 at 16:29 +0100, Julien Grall wrote: > > > >> +GLOBAL(do_smc) > > > >> +GLOBAL(do_smc) > > > > These should both be ENTRY. > > Why? Is it because GLOBAL should be used for variable and ENTRY for > function? Exactly. The practical difference is that ENTRY makes sure the code entry point is suitably aligned, but semantically ENTRY is the correct name. > >> +int do_smc(register_t function_id, ...); > > > > Are you sure that the variadic function calling convention is the same > > as for a regular function call? I'm not entirely clear having read > > AAPCS, it says they are marshalled according to "the standard base". > > All the parameters fits in a register, so the compiler will effectively > use the first registers to pass arguments. Does it? Even with variadic functions? It's not unheard of for an ABI to fallback to pushing things onto the stack for such cases, since it works out far easier in stdargs.h. > This may be an issue if the user decides to pass an uint64_t on ARM32. > > > I think it would probably be safer to declare this guy as taking 3-4 > > arguments and pass in 0 for the unused one. You could wrap in > > do_smc<N>() helpers if you really wanted. > > I will introduce helpers. It will be easier if we decide to extend the > number of parameters (SMC64 supports up to 5 parameters). Great. Ian.
On 10/14/2014 01:57 PM, Ian Campbell wrote: > On Tue, 2014-10-14 at 13:43 +0100, Julien Grall wrote: >> On 10/14/2014 10:15 AM, Ian Campbell wrote: >>> On Mon, 2014-10-13 at 16:29 +0100, Julien Grall wrote: >>> >>>> +GLOBAL(do_smc) >>> >>>> +GLOBAL(do_smc) >>> >>> These should both be ENTRY. >> >> Why? Is it because GLOBAL should be used for variable and ENTRY for >> function? > > Exactly. The practical difference is that ENTRY makes sure the code > entry point is suitably aligned, but semantically ENTRY is the correct > name. Thanks for the explanation. So arm*/debug.S are buggy. I will send a patch to replace GLOBAL by ENTRY. >>>> +int do_smc(register_t function_id, ...); >>> >>> Are you sure that the variadic function calling convention is the same >>> as for a regular function call? I'm not entirely clear having read >>> AAPCS, it says they are marshalled according to "the standard base". >> >> All the parameters fits in a register, so the compiler will effectively >> use the first registers to pass arguments. > > Does it? Even with variadic functions? It's not unheard of for an ABI to > fallback to pushing things onto the stack for such cases, since it works > out far easier in stdargs.h. You are right, it looks like it's compiler depend how variadic function will be called. Regards,
>>> On 14.10.14 at 15:06, <julien.grall@linaro.org> wrote: > On 10/14/2014 01:57 PM, Ian Campbell wrote: >> On Tue, 2014-10-14 at 13:43 +0100, Julien Grall wrote: >>> On 10/14/2014 10:15 AM, Ian Campbell wrote: >>>> On Mon, 2014-10-13 at 16:29 +0100, Julien Grall wrote: >>>>> +int do_smc(register_t function_id, ...); >>>> >>>> Are you sure that the variadic function calling convention is the same >>>> as for a regular function call? I'm not entirely clear having read >>>> AAPCS, it says they are marshalled according to "the standard base". >>> >>> All the parameters fits in a register, so the compiler will effectively >>> use the first registers to pass arguments. >> >> Does it? Even with variadic functions? It's not unheard of for an ABI to >> fallback to pushing things onto the stack for such cases, since it works >> out far easier in stdargs.h. > > You are right, it looks like it's compiler depend how variadic function > will be called. Now that should never happen - there ought to be an ABI that all compilers abide by. Jan
On Tue, 2014-10-14 at 14:13 +0100, Jan Beulich wrote: > >>> On 14.10.14 at 15:06, <julien.grall@linaro.org> wrote: > > On 10/14/2014 01:57 PM, Ian Campbell wrote: > >> On Tue, 2014-10-14 at 13:43 +0100, Julien Grall wrote: > >>> On 10/14/2014 10:15 AM, Ian Campbell wrote: > >>>> On Mon, 2014-10-13 at 16:29 +0100, Julien Grall wrote: > >>>>> +int do_smc(register_t function_id, ...); > >>>> > >>>> Are you sure that the variadic function calling convention is the same > >>>> as for a regular function call? I'm not entirely clear having read > >>>> AAPCS, it says they are marshalled according to "the standard base". > >>> > >>> All the parameters fits in a register, so the compiler will effectively > >>> use the first registers to pass arguments. > >> > >> Does it? Even with variadic functions? It's not unheard of for an ABI to > >> fallback to pushing things onto the stack for such cases, since it works > >> out far easier in stdargs.h. > > > > You are right, it looks like it's compiler depend how variadic function > > will be called. > > Now that should never happen - there ought to be an ABI that all > compilers abide by. Agreed. Ian.
At 14:13 +0100 on 14 Oct (1413292415), Jan Beulich wrote: > >>> On 14.10.14 at 15:06, <julien.grall@linaro.org> wrote: > > On 10/14/2014 01:57 PM, Ian Campbell wrote: > >> On Tue, 2014-10-14 at 13:43 +0100, Julien Grall wrote: > >>> On 10/14/2014 10:15 AM, Ian Campbell wrote: > >>>> On Mon, 2014-10-13 at 16:29 +0100, Julien Grall wrote: > >>>>> +int do_smc(register_t function_id, ...); > >>>> > >>>> Are you sure that the variadic function calling convention is the same > >>>> as for a regular function call? I'm not entirely clear having read > >>>> AAPCS, it says they are marshalled according to "the standard base". > >>> > >>> All the parameters fits in a register, so the compiler will effectively > >>> use the first registers to pass arguments. > >> > >> Does it? Even with variadic functions? It's not unheard of for an ABI to > >> fallback to pushing things onto the stack for such cases, since it works > >> out far easier in stdargs.h. > > > > You are right, it looks like it's compiler depend how variadic function > > will be called. > > Now that should never happen - there ought to be an ABI that all > compilers abide by. Indeed. It may depend on compiler _flags_ but it muct be one of the standard layouts. By my reading of the AAPCS, calls with up to four word-sized integer/pointer arguments (even variadic) will use registers -- "marshalled as for the base standard" just means "not using any of the optional rules for FP arguments". All of which reminds me of this (from last year): https://www.mikeash.com/pyblog/friday-qa-2013-06-28-anatomy-of-a-compiler-bug.html Tim.
On Tue, 2014-10-14 at 21:09 +0200, Tim Deegan wrote: > At 14:13 +0100 on 14 Oct (1413292415), Jan Beulich wrote: > > >>> On 14.10.14 at 15:06, <julien.grall@linaro.org> wrote: > > > On 10/14/2014 01:57 PM, Ian Campbell wrote: > > >> On Tue, 2014-10-14 at 13:43 +0100, Julien Grall wrote: > > >>> On 10/14/2014 10:15 AM, Ian Campbell wrote: > > >>>> On Mon, 2014-10-13 at 16:29 +0100, Julien Grall wrote: > > >>>>> +int do_smc(register_t function_id, ...); > > >>>> > > >>>> Are you sure that the variadic function calling convention is the same > > >>>> as for a regular function call? I'm not entirely clear having read > > >>>> AAPCS, it says they are marshalled according to "the standard base". > > >>> > > >>> All the parameters fits in a register, so the compiler will effectively > > >>> use the first registers to pass arguments. > > >> > > >> Does it? Even with variadic functions? It's not unheard of for an ABI to > > >> fallback to pushing things onto the stack for such cases, since it works > > >> out far easier in stdargs.h. > > > > > > You are right, it looks like it's compiler depend how variadic function > > > will be called. > > > > Now that should never happen - there ought to be an ABI that all > > compilers abide by. > > Indeed. It may depend on compiler _flags_ but it muct be one of the > standard layouts. By my reading of the AAPCS, calls with up to four > word-sized integer/pointer arguments (even variadic) will use > registers -- "marshalled as for the base standard" just means "not > using any of the optional rules for FP arguments". OK, so that's good. I think smc calls can potentially take more than 4 arguments, but I don't think we have any such right now. Using call_smc<N> will help protect us against stumbling over this in the future... > All of which reminds me of this (from last year): It reminded me of the stack misalignment issue we used to have on one of the arm subarchs, which broke variadics iirc... > https://www.mikeash.com/pyblog/friday-qa-2013-06-28-anatomy-of-a-compiler-bug.html Interesting read thanks! Ian.
diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile index df0e7de..51b64f9 100644 --- a/xen/arch/arm/arm32/Makefile +++ b/xen/arch/arm/arm32/Makefile @@ -8,5 +8,6 @@ obj-y += domain.o obj-y += vfp.o obj-y += smpboot.o obj-y += domctl.o +obj-y += smc.o obj-$(EARLY_PRINTK) += debug.o diff --git a/xen/arch/arm/arm32/smc.S b/xen/arch/arm/arm32/smc.S new file mode 100644 index 0000000..ea1dba5 --- /dev/null +++ b/xen/arch/arm/arm32/smc.S @@ -0,0 +1,19 @@ +/* + * xen/arch/arm/arm32/smc.S + * + * Wrapper for Secure Monitors Calls + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +GLOBAL(do_smc) + smc #0 + mov pc, lr diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile index c7243f5..4debaf4 100644 --- a/xen/arch/arm/arm64/Makefile +++ b/xen/arch/arm/arm64/Makefile @@ -1,6 +1,7 @@ subdir-y += lib obj-y += entry.o +obj-y += smc.o obj-y += traps.o obj-y += domain.o diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S new file mode 100644 index 0000000..dfe3f02 --- /dev/null +++ b/xen/arch/arm/arm64/smc.S @@ -0,0 +1,19 @@ +/* + * xen/arch/arm/arm64/smc.S + * + * Wrapper for Secure Monitors Calls + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +GLOBAL(do_smc) + smc #0 + ret diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c index ac556cb..0b6e884 100644 --- a/xen/arch/arm/platforms/exynos5.c +++ b/xen/arch/arm/platforms/exynos5.c @@ -37,19 +37,6 @@ static bool_t secure_firmware; #define SMC_CMD_CPU1BOOT (-4) -static noinline void exynos_smc(register_t function_id, register_t arg0, - register_t arg1, register_t arg2) -{ - asm volatile( - __asmeq("%0", "r0") - __asmeq("%1", "r1") - __asmeq("%2", "r2") - __asmeq("%3", "r3") - "smc #0" - : - : "r" (function_id), "r" (arg0), "r" (arg1), "r" (arg2)); -} - static int exynos5_init_time(void) { uint32_t reg; @@ -263,7 +250,7 @@ static int exynos5_cpu_up(int cpu) iounmap(power); if ( secure_firmware ) - exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); + do_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); return cpu_up_send_sgi(cpu); } diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c index edfc391..91b3c47 100644 --- a/xen/arch/arm/platforms/seattle.c +++ b/xen/arch/arm/platforms/seattle.c @@ -31,22 +31,14 @@ static const char * const seattle_dt_compat[] __initconst = * This is temporary until full PSCI-0.2 is supported. * Then, these function will be removed. */ -static noinline void seattle_smc_psci(register_t func_id) -{ - asm volatile( - "smc #0" - : "+r" (func_id) - :); -} - static void seattle_system_reset(void) { - seattle_smc_psci(PSCI_0_2_FN_SYSTEM_RESET); + do_smc(PSCI_0_2_FN_SYSTEM_RESET); } static void seattle_system_off(void) { - seattle_smc_psci(PSCI_0_2_FN_SYSTEM_OFF); + do_smc(PSCI_0_2_FN_SYSTEM_OFF); } PLATFORM_START(seattle, "SEATTLE") diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c index b6360d5..7f1f628 100644 --- a/xen/arch/arm/psci.c +++ b/xen/arch/arm/psci.c @@ -25,37 +25,12 @@ bool_t psci_available; -#ifdef CONFIG_ARM_32 -#define REG_PREFIX "r" -#else -#define REG_PREFIX "x" -#endif - -static noinline int __invoke_psci_fn_smc(register_t function_id, - register_t arg0, - register_t arg1, - register_t arg2) -{ - asm volatile( - __asmeq("%0", REG_PREFIX"0") - __asmeq("%1", REG_PREFIX"1") - __asmeq("%2", REG_PREFIX"2") - __asmeq("%3", REG_PREFIX"3") - "smc #0" - : "+r" (function_id) - : "r" (arg0), "r" (arg1), "r" (arg2)); - - return function_id; -} - -#undef REG_PREFIX - static uint32_t psci_cpu_on_nr; int call_psci_cpu_on(int cpu) { - return __invoke_psci_fn_smc(psci_cpu_on_nr, - cpu_logical_map(cpu), __pa(init_secondary), 0); + return do_smc(psci_cpu_on_nr, cpu_logical_map(cpu), + __pa(init_secondary), 0); } int __init psci_init(void) diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index e719c26..5b7385c 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -614,6 +614,8 @@ void vcpu_regs_hyp_to_user(const struct vcpu *vcpu, void vcpu_regs_user_to_hyp(struct vcpu *vcpu, const struct vcpu_guest_core_regs *regs); +int do_smc(register_t function_id, ...); + #endif /* __ASSEMBLY__ */ #endif /* __ASM_ARM_PROCESSOR_H */ /*