| Message ID | 20180925172043.20248-4-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: SMCCC fixup and improvement | expand |
On Tue, 25 Sep 2018, Julien Grall wrote: > From: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > Existing SMC wrapper call_smc() allows only 4 parameters and > returns only one value. This is enough for existing > use in PSCI code, but TEE mediator will need a call that is > fully compatible with ARM SMCCC v1.0. > > This patch adds a wrapper for both arm32 and arm64. In the case of > arm32, the wrapper is just an alias to the ARM SMCCC v1.1 as the > convention is the same. > > CC: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > [julien: Rework the wrapper to make it closer to SMCC 1.1 wrapper] > Signed-off-by: Julien Grall <julien.grall@arm.com> I have been struggling to find the old doc for SMCCC v1.0, all the references have been updated to v1.1 online now. Do you have a link? > --- > xen/arch/arm/arm64/Makefile | 1 + > xen/arch/arm/arm64/asm-offsets.c | 5 ++++ > xen/arch/arm/arm64/smc.S | 32 +++++++++++++++++++++++++ > xen/include/asm-arm/smccc.h | 51 +++++++++++++++++++++++++++++++++++++++- > 4 files changed, 88 insertions(+), 1 deletion(-) > create mode 100644 xen/arch/arm/arm64/smc.S > > diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile > index bb5c610b2a..c4f3a28a0d 100644 > --- a/xen/arch/arm/arm64/Makefile > +++ b/xen/arch/arm/arm64/Makefile > @@ -8,6 +8,7 @@ obj-y += domain.o > obj-y += entry.o > obj-y += insn.o > obj-$(CONFIG_LIVEPATCH) += livepatch.o > +obj-y += smc.o > obj-y += smpboot.o > obj-y += traps.o > obj-y += vfp.o > diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c > index 62833d8c8b..280ddb55bf 100644 > --- a/xen/arch/arm/arm64/asm-offsets.c > +++ b/xen/arch/arm/arm64/asm-offsets.c > @@ -10,6 +10,7 @@ > #include <xen/bitops.h> > #include <public/xen.h> > #include <asm/current.h> > +#include <asm/smccc.h> > > #define DEFINE(_sym, _val) \ > asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ > @@ -51,6 +52,10 @@ void __dummy__(void) > > BLANK(); > OFFSET(INITINFO_stack, struct init_info, stack); > + > + BLANK(); > + OFFSET(SMCCC_RES_a0, struct arm_smccc_res, a0); > + OFFSET(SMCCC_RES_a2, struct arm_smccc_res, a2); > } > > /* > diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S > new file mode 100644 > index 0000000000..b0752be57e > --- /dev/null > +++ b/xen/arch/arm/arm64/smc.S > @@ -0,0 +1,32 @@ > +/* > + * 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. > + * > + * 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. > + */ > + > +#include <asm/asm_defns.h> > +#include <asm/macros.h> > + > +/* > + * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2, > + * register_t a3, register_t a4, register_t a5, > + * register_t a6, register_t a7, > + * struct arm_smccc_res *res) > + */ > +ENTRY(__arm_smccc_1_0_smc) > + smc #0 > + ldr x4, [sp] > + cbz x4, 1f /* No need to store the result */ > + stp x0, x1, [x4, #SMCCC_RES_a0] > + stp x2, x3, [x4, #SMCCC_RES_a2] > +1: > + ret As I mentioned, I couldn't find the doc, but it looks like the Linux implementation always copies back the results (arch/arm64/kernel/smccc-call.S)? Shouldn't we zero x0-x3 at least? Other than that, it looks fine. > diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h > index 648bef28bd..1ed6cbaa48 100644 > --- a/xen/include/asm-arm/smccc.h > +++ b/xen/include/asm-arm/smccc.h > @@ -207,7 +207,56 @@ struct arm_smccc_res { > *___res = (typeof(*___res)){r0, r1, r2, r3}; \ > } while ( 0 ) > > -#endif > +/* > + * The calling convention for arm32 is the same for both SMCCC v1.0 and > + * v1.1. > + */ > +#ifdef CONFIG_ARM_32 > +#define arm_smccc_1_0_smc(...) arm_smccc_1_1_smc(__VA_ARGS__) > +#else > + > +void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2, > + register_t a3, register_t a4, register_t a5, > + register_t a6, register_t a7, > + struct arm_smccc_res *res); > + > +/* Macros to handle variadic parameter for SMCCC v1.0 helper */ > +#define __arm_smccc_1_0_smc_7(a0, a1, a2, a3, a4, a5, a6, a7, res) \ > + __arm_smccc_1_0_smc(a0, a1, a2, a3, a4, a5, a6, a7, res) > + > +#define __arm_smccc_1_0_smc_6(a0, a1, a2, a3, a4, a5, a6, res) \ > + __arm_smccc_1_0_smc_7(a0, a1, a2, a3, a4, a5, a6, 0, res) > + > +#define __arm_smccc_1_0_smc_5(a0, a1, a2, a3, a4, a5, res) \ > + __arm_smccc_1_0_smc_6(a0, a1, a2, a3, a4, a5, 0, res) > + > +#define __arm_smccc_1_0_smc_4(a0, a1, a2, a3, a4, res) \ > + __arm_smccc_1_0_smc_5(a0, a1, a2, a3, a4, 0, res) > + > +#define __arm_smccc_1_0_smc_3(a0, a1, a2, a3, res) \ > + __arm_smccc_1_0_smc_4(a0, a1, a2, a3, 0, res) > + > +#define __arm_smccc_1_0_smc_2(a0, a1, a2, res) \ > + __arm_smccc_1_0_smc_3(a0, a1, a2, 0, res) > + > +#define __arm_smccc_1_0_smc_1(a0, a1, res) \ > + __arm_smccc_1_0_smc_2(a0, a1, 0, res) > + > +#define __arm_smccc_1_0_smc_0(a0, res) \ > + __arm_smccc_1_0_smc_1(a0, 0, res) > + > +#define ___arm_smccc_1_0_smc_count(count, ...) \ > + __arm_smccc_1_0_smc_ ## count(__VA_ARGS__) > + > +#define __arm_smccc_1_0_smc_count(count, ...) \ > + ___arm_smccc_1_0_smc_count(count, __VA_ARGS__) > + > +#define arm_smccc_1_0_smc(...) \ > + __arm_smccc_1_0_smc_count(__count_args(__VA_ARGS__), __VA_ARGS__) > + > +#endif /* CONFIG_ARM_64 */ > + > +#endif /* __ASSEMBLY__ */ > > /* > * Construct function identifier from call type (fast or standard), > -- > 2.11.0 >
Hi Stefano, On 09/26/2018 12:50 AM, Stefano Stabellini wrote: > On Tue, 25 Sep 2018, Julien Grall wrote: >> From: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >> >> Existing SMC wrapper call_smc() allows only 4 parameters and >> returns only one value. This is enough for existing >> use in PSCI code, but TEE mediator will need a call that is >> fully compatible with ARM SMCCC v1.0. >> >> This patch adds a wrapper for both arm32 and arm64. In the case of >> arm32, the wrapper is just an alias to the ARM SMCCC v1.1 as the >> convention is the same. >> >> CC: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> >> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >> [julien: Rework the wrapper to make it closer to SMCC 1.1 wrapper] >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > I have been struggling to find the old doc for SMCCC v1.0, all the > references have been updated to v1.1 online now. Do you have a link? Are you sure? All the references are still to v1.0 (DEN 0028B). See [1]. >> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S >> new file mode 100644 >> index 0000000000..b0752be57e >> --- /dev/null >> +++ b/xen/arch/arm/arm64/smc.S >> @@ -0,0 +1,32 @@ >> +/* >> + * 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. >> + * >> + * 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. >> + */ >> + >> +#include <asm/asm_defns.h> >> +#include <asm/macros.h> >> + >> +/* >> + * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2, >> + * register_t a3, register_t a4, register_t a5, >> + * register_t a6, register_t a7, >> + * struct arm_smccc_res *res) >> + */ >> +ENTRY(__arm_smccc_1_0_smc) >> + smc #0 >> + ldr x4, [sp] >> + cbz x4, 1f /* No need to store the result */ >> + stp x0, x1, [x4, #SMCCC_RES_a0] >> + stp x2, x3, [x4, #SMCCC_RES_a2] >> +1: >> + ret > > As I mentioned, I couldn't find the doc, but it looks like the Linux > implementation always copies back the results > (arch/arm64/kernel/smccc-call.S)? Shouldn't we zero x0-x3 at least? Could you provide more details on what looks wrong? The results are copied in the array res using stp instructions. The only difference with Linux implementation is we don't handle quirk. Cheers, [1] https://developer.arm.com/products/architecture/cpu-architecture/a-profile/docs/den0028/latest/smc-calling-convention-system-software-on-arm-platforms
On Wed, 26 Sep 2018, Julien Grall wrote: > Hi Stefano, > > On 09/26/2018 12:50 AM, Stefano Stabellini wrote: > > On Tue, 25 Sep 2018, Julien Grall wrote: > > > From: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > > > > > Existing SMC wrapper call_smc() allows only 4 parameters and > > > returns only one value. This is enough for existing > > > use in PSCI code, but TEE mediator will need a call that is > > > fully compatible with ARM SMCCC v1.0. > > > > > > This patch adds a wrapper for both arm32 and arm64. In the case of > > > arm32, the wrapper is just an alias to the ARM SMCCC v1.1 as the > > > convention is the same. > > > > > > CC: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > > [julien: Rework the wrapper to make it closer to SMCC 1.1 wrapper] > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > I have been struggling to find the old doc for SMCCC v1.0, all the > > references have been updated to v1.1 online now. Do you have a link? > > Are you sure? All the references are still to v1.0 (DEN 0028B). See [1]. The lack of version in the doc confused me. > > > diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S > > > new file mode 100644 > > > index 0000000000..b0752be57e > > > --- /dev/null > > > +++ b/xen/arch/arm/arm64/smc.S > > > @@ -0,0 +1,32 @@ > > > +/* > > > + * 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. > > > + * > > > + * 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. > > > + */ > > > + > > > +#include <asm/asm_defns.h> > > > +#include <asm/macros.h> > > > + > > > +/* > > > + * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2, > > > + * register_t a3, register_t a4, register_t a5, > > > + * register_t a6, register_t a7, > > > + * struct arm_smccc_res *res) > > > + */ > > > +ENTRY(__arm_smccc_1_0_smc) > > > + smc #0 > > > + ldr x4, [sp] > > > + cbz x4, 1f /* No need to store the result */ > > > + stp x0, x1, [x4, #SMCCC_RES_a0] > > > + stp x2, x3, [x4, #SMCCC_RES_a2] > > > +1: > > > + ret > > > > As I mentioned, I couldn't find the doc, but it looks like the Linux > > implementation always copies back the results > > (arch/arm64/kernel/smccc-call.S)? Shouldn't we zero x0-x3 at least? > Could you provide more details on what looks wrong? > > The results are copied in the array res using stp instructions. The only > difference with Linux implementation is we don't handle quirk. The only difference is that in this implementation we handle `res' being NULL, which I think is a good idea: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Hi Stefano, On 09/28/2018 12:02 AM, Stefano Stabellini wrote: > On Wed, 26 Sep 2018, Julien Grall wrote: >> Hi Stefano, >> >> On 09/26/2018 12:50 AM, Stefano Stabellini wrote: >>> On Tue, 25 Sep 2018, Julien Grall wrote: >>>> From: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >>>> >>>> Existing SMC wrapper call_smc() allows only 4 parameters and >>>> returns only one value. This is enough for existing >>>> use in PSCI code, but TEE mediator will need a call that is >>>> fully compatible with ARM SMCCC v1.0. >>>> >>>> This patch adds a wrapper for both arm32 and arm64. In the case of >>>> arm32, the wrapper is just an alias to the ARM SMCCC v1.1 as the >>>> convention is the same. >>>> >>>> CC: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> >>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >>>> [julien: Rework the wrapper to make it closer to SMCC 1.1 wrapper] >>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> >>> I have been struggling to find the old doc for SMCCC v1.0, all the >>> references have been updated to v1.1 online now. Do you have a link? >> >> Are you sure? All the references are still to v1.0 (DEN 0028B). See [1]. > > The lack of version in the doc confused me. > > >>>> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S >>>> new file mode 100644 >>>> index 0000000000..b0752be57e >>>> --- /dev/null >>>> +++ b/xen/arch/arm/arm64/smc.S >>>> @@ -0,0 +1,32 @@ >>>> +/* >>>> + * 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. >>>> + * >>>> + * 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. >>>> + */ >>>> + >>>> +#include <asm/asm_defns.h> >>>> +#include <asm/macros.h> >>>> + >>>> +/* >>>> + * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2, >>>> + * register_t a3, register_t a4, register_t a5, >>>> + * register_t a6, register_t a7, >>>> + * struct arm_smccc_res *res) >>>> + */ >>>> +ENTRY(__arm_smccc_1_0_smc) >>>> + smc #0 >>>> + ldr x4, [sp] >>>> + cbz x4, 1f /* No need to store the result */ >>>> + stp x0, x1, [x4, #SMCCC_RES_a0] >>>> + stp x2, x3, [x4, #SMCCC_RES_a2] >>>> +1: >>>> + ret >>> >>> As I mentioned, I couldn't find the doc, but it looks like the Linux >>> implementation always copies back the results >>> (arch/arm64/kernel/smccc-call.S)? Shouldn't we zero x0-x3 at least? >> Could you provide more details on what looks wrong? >> >> The results are copied in the array res using stp instructions. The only >> difference with Linux implementation is we don't handle quirk. > > The only difference is that in this implementation we handle `res' being > NULL, which I think is a good idea: Oh yeah I forgot that difference :/. I added it to keep the behavior similar to the SMCCC v1.1 implementation. > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > Thank you! Cheers,
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile index bb5c610b2a..c4f3a28a0d 100644 --- a/xen/arch/arm/arm64/Makefile +++ b/xen/arch/arm/arm64/Makefile @@ -8,6 +8,7 @@ obj-y += domain.o obj-y += entry.o obj-y += insn.o obj-$(CONFIG_LIVEPATCH) += livepatch.o +obj-y += smc.o obj-y += smpboot.o obj-y += traps.o obj-y += vfp.o diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c index 62833d8c8b..280ddb55bf 100644 --- a/xen/arch/arm/arm64/asm-offsets.c +++ b/xen/arch/arm/arm64/asm-offsets.c @@ -10,6 +10,7 @@ #include <xen/bitops.h> #include <public/xen.h> #include <asm/current.h> +#include <asm/smccc.h> #define DEFINE(_sym, _val) \ asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ @@ -51,6 +52,10 @@ void __dummy__(void) BLANK(); OFFSET(INITINFO_stack, struct init_info, stack); + + BLANK(); + OFFSET(SMCCC_RES_a0, struct arm_smccc_res, a0); + OFFSET(SMCCC_RES_a2, struct arm_smccc_res, a2); } /* diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S new file mode 100644 index 0000000000..b0752be57e --- /dev/null +++ b/xen/arch/arm/arm64/smc.S @@ -0,0 +1,32 @@ +/* + * 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. + * + * 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. + */ + +#include <asm/asm_defns.h> +#include <asm/macros.h> + +/* + * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2, + * register_t a3, register_t a4, register_t a5, + * register_t a6, register_t a7, + * struct arm_smccc_res *res) + */ +ENTRY(__arm_smccc_1_0_smc) + smc #0 + ldr x4, [sp] + cbz x4, 1f /* No need to store the result */ + stp x0, x1, [x4, #SMCCC_RES_a0] + stp x2, x3, [x4, #SMCCC_RES_a2] +1: + ret diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h index 648bef28bd..1ed6cbaa48 100644 --- a/xen/include/asm-arm/smccc.h +++ b/xen/include/asm-arm/smccc.h @@ -207,7 +207,56 @@ struct arm_smccc_res { *___res = (typeof(*___res)){r0, r1, r2, r3}; \ } while ( 0 ) -#endif +/* + * The calling convention for arm32 is the same for both SMCCC v1.0 and + * v1.1. + */ +#ifdef CONFIG_ARM_32 +#define arm_smccc_1_0_smc(...) arm_smccc_1_1_smc(__VA_ARGS__) +#else + +void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2, + register_t a3, register_t a4, register_t a5, + register_t a6, register_t a7, + struct arm_smccc_res *res); + +/* Macros to handle variadic parameter for SMCCC v1.0 helper */ +#define __arm_smccc_1_0_smc_7(a0, a1, a2, a3, a4, a5, a6, a7, res) \ + __arm_smccc_1_0_smc(a0, a1, a2, a3, a4, a5, a6, a7, res) + +#define __arm_smccc_1_0_smc_6(a0, a1, a2, a3, a4, a5, a6, res) \ + __arm_smccc_1_0_smc_7(a0, a1, a2, a3, a4, a5, a6, 0, res) + +#define __arm_smccc_1_0_smc_5(a0, a1, a2, a3, a4, a5, res) \ + __arm_smccc_1_0_smc_6(a0, a1, a2, a3, a4, a5, 0, res) + +#define __arm_smccc_1_0_smc_4(a0, a1, a2, a3, a4, res) \ + __arm_smccc_1_0_smc_5(a0, a1, a2, a3, a4, 0, res) + +#define __arm_smccc_1_0_smc_3(a0, a1, a2, a3, res) \ + __arm_smccc_1_0_smc_4(a0, a1, a2, a3, 0, res) + +#define __arm_smccc_1_0_smc_2(a0, a1, a2, res) \ + __arm_smccc_1_0_smc_3(a0, a1, a2, 0, res) + +#define __arm_smccc_1_0_smc_1(a0, a1, res) \ + __arm_smccc_1_0_smc_2(a0, a1, 0, res) + +#define __arm_smccc_1_0_smc_0(a0, res) \ + __arm_smccc_1_0_smc_1(a0, 0, res) + +#define ___arm_smccc_1_0_smc_count(count, ...) \ + __arm_smccc_1_0_smc_ ## count(__VA_ARGS__) + +#define __arm_smccc_1_0_smc_count(count, ...) \ + ___arm_smccc_1_0_smc_count(count, __VA_ARGS__) + +#define arm_smccc_1_0_smc(...) \ + __arm_smccc_1_0_smc_count(__count_args(__VA_ARGS__), __VA_ARGS__) + +#endif /* CONFIG_ARM_64 */ + +#endif /* __ASSEMBLY__ */ /* * Construct function identifier from call type (fast or standard),