| Message ID | 20180925172043.20248-7-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: SMCCC fixup and improvement | expand |
On Tue, 25 Sep 2018, Julien Grall wrote: > call_smc is a subset of arm_smccc_smc. Rather than having 2 methods to > do SMCCC call, replace all call to the former by the later. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/Makefile | 1 - > xen/arch/arm/platforms/exynos5.c | 3 ++- > xen/arch/arm/platforms/seattle.c | 4 ++-- > xen/arch/arm/psci.c | 37 +++++++++++++++++++++++++------------ > xen/arch/arm/smc.S | 21 --------------------- > xen/include/asm-arm/processor.h | 3 --- > 6 files changed, 29 insertions(+), 40 deletions(-) > delete mode 100644 xen/arch/arm/smc.S > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index b9b141dc84..37fa8268b3 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -39,7 +39,6 @@ obj-y += processor.o > obj-y += psci.o > obj-y += setup.o > obj-y += shutdown.o > -obj-y += smc.o > obj-y += smp.o > obj-y += smpboot.o > obj-y += sysctl.o > diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c > index c15ecf80f5..e2c0b7b878 100644 > --- a/xen/arch/arm/platforms/exynos5.c > +++ b/xen/arch/arm/platforms/exynos5.c > @@ -26,6 +26,7 @@ > #include <asm/platforms/exynos5.h> > #include <asm/platform.h> > #include <asm/io.h> > +#include <asm/smccc.h> > > static bool secure_firmware; > > @@ -249,7 +250,7 @@ static int exynos5_cpu_up(int cpu) > iounmap(power); > > if ( secure_firmware ) > - call_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); > + arm_smccc_smc(SMC_CMD_CPU1BOOT, cpu, NULL); > > return cpu_up_send_sgi(cpu); > } > diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c > index 893cc17972..64cc1868c2 100644 > --- a/xen/arch/arm/platforms/seattle.c > +++ b/xen/arch/arm/platforms/seattle.c > @@ -33,12 +33,12 @@ static const char * const seattle_dt_compat[] __initconst = > */ > static void seattle_system_reset(void) > { > - call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0); > + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL); > } > > static void seattle_system_off(void) > { > - call_smc(PSCI_0_2_FN32_SYSTEM_OFF, 0, 0, 0); > + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL); > } > > PLATFORM_START(seattle, "SEATTLE") > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c > index 941eec921b..02737e6caa 100644 > --- a/xen/arch/arm/psci.c > +++ b/xen/arch/arm/psci.c > @@ -42,42 +42,53 @@ uint32_t smccc_ver; > > static uint32_t psci_cpu_on_nr; > > +#define PSCI_RET(res) ((int32_t)(res).a0) > + > int call_psci_cpu_on(int cpu) > { > - return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), 0); > + struct arm_smccc_res res; > + > + arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), > + &res); > + > + return (int32_t)res.a0; > } Can't we use PSCI_RET(res) here? Also in general, should we care about the type mismatch int32_t vs. int which is the return type of this function? > void call_psci_cpu_off(void) > { > if ( psci_ver > PSCI_VERSION(0, 1) ) > { > - int errno; > + struct arm_smccc_res res; > > /* If successfull the PSCI cpu_off call doesn't return */ > - errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0); > + arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF, &res); > panic("PSCI cpu off failed for CPU%d err=%d\n", smp_processor_id(), > - errno); > + PSCI_RET(res)); > } > } > > void call_psci_system_off(void) > { > if ( psci_ver > PSCI_VERSION(0, 1) ) > - call_smc(PSCI_0_2_FN32_SYSTEM_OFF, 0, 0, 0); > + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL); > } > > void call_psci_system_reset(void) > { > if ( psci_ver > PSCI_VERSION(0, 1) ) > - call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0); > + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL); > } > > static int __init psci_features(uint32_t psci_func_id) > { > + struct arm_smccc_res res; > + > if ( psci_ver < PSCI_VERSION(1, 0) ) > return PSCI_NOT_SUPPORTED; > > - return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0); > + arm_smccc_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, NULL); > + > + return PSCI_RET(res); > } > > static int __init psci_is_smc_method(const struct dt_device_node *psci) > @@ -112,11 +123,11 @@ static void __init psci_init_smccc(void) > > if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED ) > { > - uint32_t ret; > + struct arm_smccc_res res; > > - ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0); > - if ( ret != ARM_SMCCC_NOT_SUPPORTED ) > - smccc_ver = ret; > + arm_smccc_smc(ARM_SMCCC_VERSION_FID, &res); > + if ( PSCI_RET(res) != ARM_SMCCC_NOT_SUPPORTED ) > + smccc_ver = PSCI_RET(res); > } > > if ( smccc_ver >= SMCCC_VERSION(1, 1) ) > @@ -165,6 +176,7 @@ static int __init psci_init_0_2(void) > { /* sentinel */ }, > }; > int ret; > + struct arm_smccc_res res; > > if ( acpi_disabled ) > { > @@ -186,7 +198,8 @@ static int __init psci_init_0_2(void) > } > } > > - psci_ver = call_smc(PSCI_0_2_FN32_PSCI_VERSION, 0, 0, 0); > + arm_smccc_smc(PSCI_0_2_FN32_PSCI_VERSION, &res); > + psci_ver = PSCI_RET(res); > > /* For the moment, we only support PSCI 0.2 and PSCI 1.x */ > if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_VERSION_MAJOR(psci_ver) != 1 ) > diff --git a/xen/arch/arm/smc.S b/xen/arch/arm/smc.S > deleted file mode 100644 > index b8f182272a..0000000000 > --- a/xen/arch/arm/smc.S > +++ /dev/null > @@ -1,21 +0,0 @@ > -/* > - * xen/arch/arm/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. > - */ > - > -#include <asm/macros.h> > - > -ENTRY(call_smc) > - smc #0 > - ret > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index 222a02dd99..8016cf306f 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -812,9 +812,6 @@ 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 call_smc(register_t function_id, register_t arg0, register_t arg1, > - register_t arg2); > - > void do_trap_hyp_serror(struct cpu_user_regs *regs); > > void do_trap_guest_serror(struct cpu_user_regs *regs); > -- > 2.11.0 >
Hi Stefano, On 09/26/2018 12:57 AM, Stefano Stabellini wrote: > On Tue, 25 Sep 2018, Julien Grall wrote: >> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c >> index 941eec921b..02737e6caa 100644 >> --- a/xen/arch/arm/psci.c >> +++ b/xen/arch/arm/psci.c >> @@ -42,42 +42,53 @@ uint32_t smccc_ver; >> >> static uint32_t psci_cpu_on_nr; >> >> +#define PSCI_RET(res) ((int32_t)(res).a0) >> + >> int call_psci_cpu_on(int cpu) >> { >> - return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), 0); >> + struct arm_smccc_res res; >> + >> + arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), >> + &res); >> + >> + return (int32_t)res.a0; >> } > > Can't we use PSCI_RET(res) here? I missed that one. I will update it. > > Also in general, should we care about the type mismatch int32_t vs. int > which is the return type of this function? The only issue I could see is if sizeof(int) < sizeof(int32_t). If that happen, then psci.c would be our least concern as we always assume int would at least 32-bit wide. I would prefer to keep the return of the function int and casting the result with (int32_t). The latter is helpful to know what is the size of the result (a0 is 64-bit). Cheers,
On Wed, 26 Sep 2018, Julien Grall wrote: > Hi Stefano, > > On 09/26/2018 12:57 AM, Stefano Stabellini wrote: > > On Tue, 25 Sep 2018, Julien Grall wrote: > > > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c > > > index 941eec921b..02737e6caa 100644 > > > --- a/xen/arch/arm/psci.c > > > +++ b/xen/arch/arm/psci.c > > > @@ -42,42 +42,53 @@ uint32_t smccc_ver; > > > static uint32_t psci_cpu_on_nr; > > > +#define PSCI_RET(res) ((int32_t)(res).a0) > > > + > > > int call_psci_cpu_on(int cpu) > > > { > > > - return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), > > > __pa(init_secondary), 0); > > > + struct arm_smccc_res res; > > > + > > > + arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), > > > __pa(init_secondary), > > > + &res); > > > + > > > + return (int32_t)res.a0; > > > } > > > > Can't we use PSCI_RET(res) here? > > I missed that one. I will update it. > > > > > Also in general, should we care about the type mismatch int32_t vs. int > > which is the return type of this function? > > The only issue I could see is if sizeof(int) < sizeof(int32_t). If that > happen, then psci.c would be our least concern as we always assume int would > at least 32-bit wide. > > I would prefer to keep the return of the function int and casting the result > with (int32_t). The latter is helpful to know what is the size of the result > (a0 is 64-bit). It is a good idea to keep the cast. I don't have a strong opinion on whether the functions should return int or int32_t. The only question is whether we want to cast to (int32_t) or to (int). I would prefer to cast to the same type of the return of the function. So if we keep int as return type, then casting to (int). However, given that in practice it makes no difference, I can ack the patch in any case.
Hi Stefano, On 09/27/2018 11:41 PM, Stefano Stabellini wrote: > On Wed, 26 Sep 2018, Julien Grall wrote: >> Hi Stefano, >> >> On 09/26/2018 12:57 AM, Stefano Stabellini wrote: >>> On Tue, 25 Sep 2018, Julien Grall wrote: >>>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c >>>> index 941eec921b..02737e6caa 100644 >>>> --- a/xen/arch/arm/psci.c >>>> +++ b/xen/arch/arm/psci.c >>>> @@ -42,42 +42,53 @@ uint32_t smccc_ver; >>>> static uint32_t psci_cpu_on_nr; >>>> +#define PSCI_RET(res) ((int32_t)(res).a0) >>>> + >>>> int call_psci_cpu_on(int cpu) >>>> { >>>> - return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), >>>> __pa(init_secondary), 0); >>>> + struct arm_smccc_res res; >>>> + >>>> + arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), >>>> __pa(init_secondary), >>>> + &res); >>>> + >>>> + return (int32_t)res.a0; >>>> } >>> >>> Can't we use PSCI_RET(res) here? >> >> I missed that one. I will update it. >> >>> >>> Also in general, should we care about the type mismatch int32_t vs. int >>> which is the return type of this function? >> >> The only issue I could see is if sizeof(int) < sizeof(int32_t). If that >> happen, then psci.c would be our least concern as we always assume int would >> at least 32-bit wide. >> >> I would prefer to keep the return of the function int and casting the result >> with (int32_t). The latter is helpful to know what is the size of the result >> (a0 is 64-bit). > > It is a good idea to keep the cast. I don't have a strong opinion on > whether the functions should return int or int32_t. The only question is > whether we want to cast to (int32_t) or to (int). I would prefer to cast > to the same type of the return of the function. So if we keep int as > return type, then casting to (int). However, given that in practice it > makes no difference, I can ack the patch in any case. I would prefer to stick with the current approach. Cheers,
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index b9b141dc84..37fa8268b3 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -39,7 +39,6 @@ obj-y += processor.o obj-y += psci.o obj-y += setup.o obj-y += shutdown.o -obj-y += smc.o obj-y += smp.o obj-y += smpboot.o obj-y += sysctl.o diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c index c15ecf80f5..e2c0b7b878 100644 --- a/xen/arch/arm/platforms/exynos5.c +++ b/xen/arch/arm/platforms/exynos5.c @@ -26,6 +26,7 @@ #include <asm/platforms/exynos5.h> #include <asm/platform.h> #include <asm/io.h> +#include <asm/smccc.h> static bool secure_firmware; @@ -249,7 +250,7 @@ static int exynos5_cpu_up(int cpu) iounmap(power); if ( secure_firmware ) - call_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); + arm_smccc_smc(SMC_CMD_CPU1BOOT, cpu, NULL); return cpu_up_send_sgi(cpu); } diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c index 893cc17972..64cc1868c2 100644 --- a/xen/arch/arm/platforms/seattle.c +++ b/xen/arch/arm/platforms/seattle.c @@ -33,12 +33,12 @@ static const char * const seattle_dt_compat[] __initconst = */ static void seattle_system_reset(void) { - call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0); + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL); } static void seattle_system_off(void) { - call_smc(PSCI_0_2_FN32_SYSTEM_OFF, 0, 0, 0); + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL); } PLATFORM_START(seattle, "SEATTLE") diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c index 941eec921b..02737e6caa 100644 --- a/xen/arch/arm/psci.c +++ b/xen/arch/arm/psci.c @@ -42,42 +42,53 @@ uint32_t smccc_ver; static uint32_t psci_cpu_on_nr; +#define PSCI_RET(res) ((int32_t)(res).a0) + int call_psci_cpu_on(int cpu) { - return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), 0); + struct arm_smccc_res res; + + arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), + &res); + + return (int32_t)res.a0; } void call_psci_cpu_off(void) { if ( psci_ver > PSCI_VERSION(0, 1) ) { - int errno; + struct arm_smccc_res res; /* If successfull the PSCI cpu_off call doesn't return */ - errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0); + arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF, &res); panic("PSCI cpu off failed for CPU%d err=%d\n", smp_processor_id(), - errno); + PSCI_RET(res)); } } void call_psci_system_off(void) { if ( psci_ver > PSCI_VERSION(0, 1) ) - call_smc(PSCI_0_2_FN32_SYSTEM_OFF, 0, 0, 0); + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL); } void call_psci_system_reset(void) { if ( psci_ver > PSCI_VERSION(0, 1) ) - call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0); + arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL); } static int __init psci_features(uint32_t psci_func_id) { + struct arm_smccc_res res; + if ( psci_ver < PSCI_VERSION(1, 0) ) return PSCI_NOT_SUPPORTED; - return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0); + arm_smccc_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, NULL); + + return PSCI_RET(res); } static int __init psci_is_smc_method(const struct dt_device_node *psci) @@ -112,11 +123,11 @@ static void __init psci_init_smccc(void) if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED ) { - uint32_t ret; + struct arm_smccc_res res; - ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0); - if ( ret != ARM_SMCCC_NOT_SUPPORTED ) - smccc_ver = ret; + arm_smccc_smc(ARM_SMCCC_VERSION_FID, &res); + if ( PSCI_RET(res) != ARM_SMCCC_NOT_SUPPORTED ) + smccc_ver = PSCI_RET(res); } if ( smccc_ver >= SMCCC_VERSION(1, 1) ) @@ -165,6 +176,7 @@ static int __init psci_init_0_2(void) { /* sentinel */ }, }; int ret; + struct arm_smccc_res res; if ( acpi_disabled ) { @@ -186,7 +198,8 @@ static int __init psci_init_0_2(void) } } - psci_ver = call_smc(PSCI_0_2_FN32_PSCI_VERSION, 0, 0, 0); + arm_smccc_smc(PSCI_0_2_FN32_PSCI_VERSION, &res); + psci_ver = PSCI_RET(res); /* For the moment, we only support PSCI 0.2 and PSCI 1.x */ if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_VERSION_MAJOR(psci_ver) != 1 ) diff --git a/xen/arch/arm/smc.S b/xen/arch/arm/smc.S deleted file mode 100644 index b8f182272a..0000000000 --- a/xen/arch/arm/smc.S +++ /dev/null @@ -1,21 +0,0 @@ -/* - * xen/arch/arm/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. - */ - -#include <asm/macros.h> - -ENTRY(call_smc) - smc #0 - ret diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index 222a02dd99..8016cf306f 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -812,9 +812,6 @@ 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 call_smc(register_t function_id, register_t arg0, register_t arg1, - register_t arg2); - void do_trap_hyp_serror(struct cpu_user_regs *regs); void do_trap_guest_serror(struct cpu_user_regs *regs);
call_smc is a subset of arm_smccc_smc. Rather than having 2 methods to do SMCCC call, replace all call to the former by the later. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/Makefile | 1 - xen/arch/arm/platforms/exynos5.c | 3 ++- xen/arch/arm/platforms/seattle.c | 4 ++-- xen/arch/arm/psci.c | 37 +++++++++++++++++++++++++------------ xen/arch/arm/smc.S | 21 --------------------- xen/include/asm-arm/processor.h | 3 --- 6 files changed, 29 insertions(+), 40 deletions(-) delete mode 100644 xen/arch/arm/smc.S