[U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
From: Stefan Agner stefan.agner@toradex.com
A proper stack is required to safely use C code in psci_arch_cpu_entry.
Fixes: 486daaa618e1 ("arm: psci: add a weak function psci_arch_cpu_entry") Cc: Patrick Delaunay patrick.delaunay@st.com Signed-off-by: Stefan Agner stefan.agner@toradex.com ---
arch/arm/cpu/armv7/psci.S | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S index 08b5088675..983cd90442 100644 --- a/arch/arm/cpu/armv7/psci.S +++ b/arch/arm/cpu/armv7/psci.S @@ -331,6 +331,8 @@ ENTRY(psci_cpu_entry)
bl _nonsec_init
+ bl psci_stack_setup + bl psci_arch_cpu_entry
bl psci_get_cpu_id @ CPU ID => r0
From: Stefan Agner stefan.agner@toradex.com
There is no need for assembly in the platform specific part of the PSCI implementation.
Note that this does not make it a complete PSCI 1.0 implementation yet but aids to do so in upcoming patches.
Signed-off-by: Stefan Agner stefan.agner@toradex.com ---
arch/arm/mach-imx/mx7/Makefile | 5 +-- arch/arm/mach-imx/mx7/psci-mx7.c | 29 +++++++++++---- arch/arm/mach-imx/mx7/psci.S | 60 -------------------------------- 3 files changed, 24 insertions(+), 70 deletions(-) delete mode 100644 arch/arm/mach-imx/mx7/psci.S
diff --git a/arch/arm/mach-imx/mx7/Makefile b/arch/arm/mach-imx/mx7/Makefile index 7a1ee7a944..94a0e6464f 100644 --- a/arch/arm/mach-imx/mx7/Makefile +++ b/arch/arm/mach-imx/mx7/Makefile @@ -4,7 +4,4 @@ #
obj-y := soc.o clock.o clock_slice.o ddr.o snvs.o - -ifdef CONFIG_ARMV7_PSCI -obj-y += psci-mx7.o psci.o -endif +obj-$(CONFIG_ARMV7_PSCI) += psci-mx7.o diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c index 7dc49bd444..95f8cb4def 100644 --- a/arch/arm/mach-imx/mx7/psci-mx7.c +++ b/arch/arm/mach-imx/mx7/psci-mx7.c @@ -67,23 +67,34 @@ __secure void imx_enable_cpu_ca7(int cpu, bool enable) writel(val, SRC_BASE_ADDR + SRC_A7RCR1); }
-__secure int imx_cpu_on(int fn, int cpu, int pc) +__secure s32 psci_cpu_on(u32 __always_unused function_id, u32 mpidr, u32 ep, + u32 context_id) { - writel(pc, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D); + u32 cpu = (mpidr & 0x1); + + psci_save(cpu, ep, context_id); + + writel((u32)psci_cpu_entry, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D); imx_gpcv2_set_core1_power(true); imx_enable_cpu_ca7(cpu, true); return 0; }
-__secure int imx_cpu_off(int cpu) +__secure s32 psci_cpu_off(void) { + int cpu; + + psci_cpu_off_common(); + cpu = psci_get_cpu_id(); imx_enable_cpu_ca7(cpu, false); imx_gpcv2_set_core1_power(false); writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4); - return 0; + + while (1) + wfi(); }
-__secure void imx_system_reset(void) +__secure void psci_system_reset(void) { struct wdog_regs *wdog = (struct wdog_regs *)WDOG1_BASE_ADDR;
@@ -91,9 +102,12 @@ __secure void imx_system_reset(void) writel(0x1 << 28, CCM_BASE_ADDR + CCM_ROOT_WDOG); writel(0x3, CCM_BASE_ADDR + CCM_CCGR_WDOG1); writew(WCR_WDE, &wdog->wcr); + + while (1) + wfi(); }
-__secure void imx_system_off(void) +__secure void psci_system_off(void) { u32 val;
@@ -103,4 +117,7 @@ __secure void imx_system_off(void) val = readl(SNVS_BASE_ADDR + SNVS_LPCR); val |= BP_SNVS_LPCR_DP_EN | BP_SNVS_LPCR_TOP; writel(val, SNVS_BASE_ADDR + SNVS_LPCR); + + while (1) + wfi(); } diff --git a/arch/arm/mach-imx/mx7/psci.S b/arch/arm/mach-imx/mx7/psci.S deleted file mode 100644 index 89dcf880e8..0000000000 --- a/arch/arm/mach-imx/mx7/psci.S +++ /dev/null @@ -1,60 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0+ */ -/* - * Copyright (C) 2015-2016 Freescale Semiconductor, Inc. - * Copyright 2017 NXP - */ - -#include <config.h> -#include <linux/linkage.h> - -#include <asm/armv7.h> -#include <asm/arch-armv7/generictimer.h> -#include <asm/psci.h> - - .pushsection ._secure.text, "ax" - - .arch_extension sec - -.globl psci_cpu_on -psci_cpu_on: - push {r4, r5, lr} - - mov r4, r0 - mov r5, r1 - mov r0, r1 - mov r1, r2 - mov r2, r3 - bl psci_save - - mov r0, r4 - mov r1, r5 - ldr r2, =psci_cpu_entry - bl imx_cpu_on - - pop {r4, r5, pc} - -.globl psci_cpu_off -psci_cpu_off: - - bl psci_cpu_off_common - bl psci_get_cpu_id - bl imx_cpu_off - -1: wfi - b 1b - -.globl psci_system_reset -psci_system_reset: - bl imx_system_reset - -2: wfi - b 2b - -.globl psci_system_off -psci_system_off: - bl imx_system_off - -3: wfi - b 3b - - .popsection
From: Stefan Agner stefan.agner@toradex.com
PSCI 1.0 require PSCI_VERSION, PSCI_FEATURES, AFFINITY_INFO and CPU_SUSPEND to be implemented. Commit 0ec3d98f7692 ("mx7_common: use psci 1.0 instead of 0.1") marked the i.MX 7 implementation to be PSCI 1.0 compliant but failed to implement those functions. Especially the missing PSCI version callback was noticeable when booting Linux:
[ 0.000000] psci: probing for conduit method from DT. [ 0.000000] psci: PSCIv65535.65535 detected in firmware. [ 0.000000] psci: Using standard PSCI v0.2 function IDs [ 0.000000] psci: MIGRATE_INFO_TYPE not supported. [ 0.000000] psci: SMC Calling Convention v1.0
This patch provides a minimal implementation thereof. With this patch applied Linux detects PSCI 1.0:
[ 0.000000] psci: probing for conduit method from DT. [ 0.000000] psci: PSCIv1.0 detected in firmware. [ 0.000000] psci: Using standard PSCI v0.2 function IDs [ 0.000000] psci: MIGRATE_INFO_TYPE not supported. [ 0.000000] psci: SMC Calling Convention v1.0
Fixes: 0ec3d98f7692 ("mx7_common: use psci 1.0 instead of 0.1") Suggested-by: Mark Rutland mark.rutland@arm.com Signed-off-by: Stefan Agner stefan.agner@toradex.com ---
arch/arm/mach-imx/mx7/psci-mx7.c | 96 +++++++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c index 95f8cb4def..8b839d37a9 100644 --- a/arch/arm/mach-imx/mx7/psci-mx7.c +++ b/arch/arm/mach-imx/mx7/psci-mx7.c @@ -8,6 +8,7 @@ #include <asm/psci.h> #include <asm/secure.h> #include <asm/arch/imx-regs.h> +#include <linux/bitops.h> #include <common.h> #include <fsl_wdog.h>
@@ -34,6 +35,24 @@ #define CCM_ROOT_WDOG 0xbb80 #define CCM_CCGR_WDOG1 0x49c0
+#define MPIDR_AFF0 GENMASK(7, 0) + +#define IMX7D_PSCI_NR_CPUS 2 +#if IMX7D_PSCI_NR_CPUS > CONFIG_ARMV7_PSCI_NR_CPUS +#error "invalid value for CONFIG_ARMV7_PSCI_NR_CPUS" +#endif + +u8 psci_state[IMX7D_PSCI_NR_CPUS] __secure_data = { + PSCI_AFFINITY_LEVEL_ON, + PSCI_AFFINITY_LEVEL_OFF}; + +static inline void psci_set_state(int cpu, u8 state) +{ + psci_state[cpu] = state; + dsb(); + isb(); +} + static inline void imx_gpcv2_set_m_core_pgc(bool enable, u32 offset) { writel(enable, GPC_IPS_BASE_ADDR + offset); @@ -67,25 +86,51 @@ __secure void imx_enable_cpu_ca7(int cpu, bool enable) writel(val, SRC_BASE_ADDR + SRC_A7RCR1); }
+__secure void psci_arch_cpu_entry(void) +{ + u32 cpu = psci_get_cpu_id(); + + psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON); +} + __secure s32 psci_cpu_on(u32 __always_unused function_id, u32 mpidr, u32 ep, u32 context_id) { - u32 cpu = (mpidr & 0x1); + u32 cpu = mpidr & MPIDR_AFF0; + + if (mpidr & ~MPIDR_AFF0) + return ARM_PSCI_RET_INVAL; + + if (cpu >= IMX7D_PSCI_NR_CPUS) + return ARM_PSCI_RET_INVAL; + + if (psci_state[cpu] == PSCI_AFFINITY_LEVEL_ON) + return ARM_PSCI_RET_ALREADY_ON; + + if (psci_state[cpu] == PSCI_AFFINITY_LEVEL_ON_PENDING) + return ARM_PSCI_RET_ON_PENDING;
psci_save(cpu, ep, context_id);
writel((u32)psci_cpu_entry, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D); + + psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON_PENDING); + imx_gpcv2_set_core1_power(true); imx_enable_cpu_ca7(cpu, true); - return 0; + + return ARM_PSCI_RET_SUCCESS; }
__secure s32 psci_cpu_off(void) { int cpu;
- psci_cpu_off_common(); cpu = psci_get_cpu_id(); + + psci_cpu_off_common(); + psci_set_state(cpu, PSCI_AFFINITY_LEVEL_OFF); + imx_enable_cpu_ca7(cpu, false); imx_gpcv2_set_core1_power(false); writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4); @@ -121,3 +166,48 @@ __secure void psci_system_off(void) while (1) wfi(); } + +__secure u32 psci_version(void) +{ + return ARM_PSCI_VER_1_0; +} + +__secure s32 psci_cpu_suspend(u32 __always_unused function_id, u32 power_state, + u32 entry_point_address, + u32 context_id) +{ + return ARM_PSCI_RET_INVAL; +} + +__secure s32 psci_affinity_info(u32 __always_unused function_id, + u32 target_affinity, + u32 lowest_affinity_level) +{ + u32 cpu = target_affinity & MPIDR_AFF0; + + if (lowest_affinity_level > 0) + return ARM_PSCI_RET_INVAL; + + if (target_affinity & ~MPIDR_AFF0) + return ARM_PSCI_RET_INVAL; + + if (cpu >= IMX7D_PSCI_NR_CPUS) + return ARM_PSCI_RET_INVAL; + + return psci_state[cpu]; +} + +__secure s32 psci_features(u32 __always_unused function_id, u32 psci_fid) +{ + switch (psci_fid) { + case ARM_PSCI_0_2_FN_PSCI_VERSION: + case ARM_PSCI_0_2_FN_CPU_OFF: + case ARM_PSCI_0_2_FN_CPU_ON: + case ARM_PSCI_0_2_FN_AFFINITY_INFO: + case ARM_PSCI_0_2_FN_SYSTEM_OFF: + case ARM_PSCI_0_2_FN_SYSTEM_RESET: + case ARM_PSCI_1_0_FN_PSCI_FEATURES: + return 0x0; + } + return ARM_PSCI_RET_NI; +}
From: Stefan Agner stefan.agner@toradex.com
So far psci_cpu_(on|off) only worked for CPU1. Allow to control CPU0 too. This allows to run the Linux PSCI checker successfully: [ 2.213447] psci_checker: PSCI checker started using 2 CPUs [ 2.219107] psci_checker: Starting hotplug tests [ 2.223859] psci_checker: Trying to turn off and on again all CPUs [ 2.267191] IRQ21 no longer affine to CPU0 [ 2.293266] Retrying again to check for CPU kill [ 2.302269] CPU0 killed. [ 2.311648] psci_checker: Trying to turn off and on again group 0 (CPUs 0-1) [ 2.354354] IRQ21 no longer affine to CPU0 [ 2.383222] Retrying again to check for CPU kill [ 2.392148] CPU0 killed. [ 2.398063] psci_checker: Hotplug tests passed OK [ 2.402910] psci_checker: Starting suspend tests (10 cycles per state) [ 2.410019] psci_checker: cpuidle not available on CPU 0, ignoring [ 2.416452] psci_checker: cpuidle not available on CPU 1, ignoring [ 2.422757] psci_checker: Could not start suspend tests on any CPU [ 2.429370] psci_checker: PSCI checker completed
Signed-off-by: Stefan Agner stefan.agner@toradex.com ---
arch/arm/mach-imx/mx7/psci-mx7.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c index 8b839d37a9..cd72449d9b 100644 --- a/arch/arm/mach-imx/mx7/psci-mx7.c +++ b/arch/arm/mach-imx/mx7/psci-mx7.c @@ -14,8 +14,10 @@
#define GPC_CPU_PGC_SW_PDN_REQ 0xfc #define GPC_CPU_PGC_SW_PUP_REQ 0xf0 +#define GPC_PGC_C0 0x800 #define GPC_PGC_C1 0x840
+#define BM_CPU_PGC_SW_PDN_PUP_REQ_CORE0_A7 0x1 #define BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7 0x2
/* below is for i.MX7D */ @@ -58,22 +60,24 @@ static inline void imx_gpcv2_set_m_core_pgc(bool enable, u32 offset) writel(enable, GPC_IPS_BASE_ADDR + offset); }
-__secure void imx_gpcv2_set_core1_power(bool pdn) +__secure void imx_gpcv2_set_core_power(int cpu, bool pdn) { u32 reg = pdn ? GPC_CPU_PGC_SW_PUP_REQ : GPC_CPU_PGC_SW_PDN_REQ; + u32 pgc = cpu ? GPC_PGC_C1 : GPC_PGC_C0; + u32 pdn_pup_req = cpu ? BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7 : + BM_CPU_PGC_SW_PDN_PUP_REQ_CORE0_A7; u32 val;
- imx_gpcv2_set_m_core_pgc(true, GPC_PGC_C1); + imx_gpcv2_set_m_core_pgc(true, pgc);
val = readl(GPC_IPS_BASE_ADDR + reg); - val |= BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7; + val |= pdn_pup_req; writel(val, GPC_IPS_BASE_ADDR + reg);
- while ((readl(GPC_IPS_BASE_ADDR + reg) & - BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7) != 0) + while ((readl(GPC_IPS_BASE_ADDR + reg) & pdn_pup_req) != 0) ;
- imx_gpcv2_set_m_core_pgc(false, GPC_PGC_C1); + imx_gpcv2_set_m_core_pgc(false, pgc); }
__secure void imx_enable_cpu_ca7(int cpu, bool enable) @@ -116,7 +120,7 @@ __secure s32 psci_cpu_on(u32 __always_unused function_id, u32 mpidr, u32 ep,
psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON_PENDING);
- imx_gpcv2_set_core1_power(true); + imx_gpcv2_set_core_power(cpu, true); imx_enable_cpu_ca7(cpu, true);
return ARM_PSCI_RET_SUCCESS; @@ -132,7 +136,7 @@ __secure s32 psci_cpu_off(void) psci_set_state(cpu, PSCI_AFFINITY_LEVEL_OFF);
imx_enable_cpu_ca7(cpu, false); - imx_gpcv2_set_core1_power(false); + imx_gpcv2_set_core_power(cpu, false); writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4);
while (1)
From: Stefan Agner stefan.agner@toradex.com
Implement MIGRATE_INFO_TYPE. This informs Linux that no migration for the trusted operating system is necessary: [ 0.000000] psci: Trusted OS migration not required
Signed-off-by: Stefan Agner stefan.agner@toradex.com ---
arch/arm/mach-imx/mx7/psci-mx7.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c index cd72449d9b..aae96c553f 100644 --- a/arch/arm/mach-imx/mx7/psci-mx7.c +++ b/arch/arm/mach-imx/mx7/psci-mx7.c @@ -201,6 +201,12 @@ __secure s32 psci_affinity_info(u32 __always_unused function_id, return psci_state[cpu]; }
+__secure s32 psci_migrate_info_type(u32 function_id) +{ + /* Trusted OS is either not present or does not require migration */ + return 2; +} + __secure s32 psci_features(u32 __always_unused function_id, u32 psci_fid) { switch (psci_fid) { @@ -208,6 +214,7 @@ __secure s32 psci_features(u32 __always_unused function_id, u32 psci_fid) case ARM_PSCI_0_2_FN_CPU_OFF: case ARM_PSCI_0_2_FN_CPU_ON: case ARM_PSCI_0_2_FN_AFFINITY_INFO: + case ARM_PSCI_0_2_FN_MIGRATE_INFO_TYPE: case ARM_PSCI_0_2_FN_SYSTEM_OFF: case ARM_PSCI_0_2_FN_SYSTEM_RESET: case ARM_PSCI_1_0_FN_PSCI_FEATURES:
On 24.06.2018 21:09, Stefan Agner wrote:
From: Stefan Agner stefan.agner@toradex.com
A proper stack is required to safely use C code in psci_arch_cpu_entry.
Patrick, I prefer to have your ack on this since you introduced psci_arch_cpu_entry.
As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same function in i.MX 7's PSCI implementation the compiler actually pushed stuff on the (uninitialized) stack, which caused the newly brought up CPU to immediately crash.
Not sure if in your case the stack pointer is already setup by some other means or your compiler does not use the stack.
In any case, I think it is better to just setup the stack properly as done in this patch...
Stefano, I think we really want patch 2/3 applied before the release since it fixes i.MX 7 PSCI. Right now the implementation is really broken and not PSCI 1.0 conformant. But patch 2/3 require this patch to be applied... Not sure how we should handle this.
-- Stefan
Fixes: 486daaa618e1 ("arm: psci: add a weak function psci_arch_cpu_entry") Cc: Patrick Delaunay patrick.delaunay@st.com Signed-off-by: Stefan Agner stefan.agner@toradex.com
arch/arm/cpu/armv7/psci.S | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S index 08b5088675..983cd90442 100644 --- a/arch/arm/cpu/armv7/psci.S +++ b/arch/arm/cpu/armv7/psci.S @@ -331,6 +331,8 @@ ENTRY(psci_cpu_entry)
bl _nonsec_init
bl psci_stack_setup
bl psci_arch_cpu_entry
bl psci_get_cpu_id @ CPU ID => r0
Hi Stefan
From: Stefan Agner [mailto:stefan@agner.ch] Sent: mercredi 27 juin 2018 10:36 Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
On 24.06.2018 21:09, Stefan Agner wrote:
From: Stefan Agner stefan.agner@toradex.com
A proper stack is required to safely use C code in psci_arch_cpu_entry.
Patrick, I prefer to have your ack on this since you introduced psci_arch_cpu_entry.
As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same function in i.MX 7's PSCI implementation the compiler actually pushed stuff on the (uninitialized) stack, which caused the newly brought up CPU to immediately crash.
Not sure if in your case the stack pointer is already setup by some other means or your compiler does not use the stack.
In any case, I think it is better to just setup the stack properly as done in this patch...
I expected that the secure stack is initialized by bootROM, but after check on 2018.07-rc2, I got a crash also with the stm32mp1 platform.
After code review, my behavior is clearly not safe: I don't sure that the initial BootROM stack is not overlapping the installed PSCI monitor code or data. So I agree: it is needed to initialize the stack in psci_cpu_entry.
Moreover after your patch the crash is solved for my platform stm32mp1.
Stefano, I think we really want patch 2/3 applied before the release since it fixes i.MX 7 PSCI. Right now the implementation is really broken and not PSCI 1.0 conformant. But patch 2/3 require this patch to be applied... Not sure how we should handle this.
-- Stefan
Acked-by: Patrick DELAUNAY Patrick.delaunay@st.com Tested-by: Patrick DELAUNAY Patrick.delaunay@st.com
Test done on stm32mp1 platform
Thanks! Patrick
Hi Tom, Hi Stefano,
On 02.07.2018 11:08, Patrick DELAUNAY wrote:
Hi Stefan
From: Stefan Agner [mailto:stefan@agner.ch] Sent: mercredi 27 juin 2018 10:36 Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
On 24.06.2018 21:09, Stefan Agner wrote:
From: Stefan Agner stefan.agner@toradex.com
A proper stack is required to safely use C code in psci_arch_cpu_entry.
Patrick, I prefer to have your ack on this since you introduced psci_arch_cpu_entry.
As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same function in i.MX 7's PSCI implementation the compiler actually pushed stuff on the (uninitialized) stack, which caused the newly brought up CPU to immediately crash.
Not sure if in your case the stack pointer is already setup by some other means or your compiler does not use the stack.
In any case, I think it is better to just setup the stack properly as done in this patch...
I expected that the secure stack is initialized by bootROM, but after check on 2018.07-rc2, I got a crash also with the stm32mp1 platform.
After code review, my behavior is clearly not safe: I don't sure that the initial BootROM stack is not overlapping the installed PSCI monitor code or data. So I agree: it is needed to initialize the stack in psci_cpu_entry.
Moreover after your patch the crash is solved for my platform stm32mp1.
Given that this fixes two platforms I guess it definitely should go into v2018.07.
Technically, for i.MX 7 only patch 1, 2 and 3 are necessary, but 4 and 5 are fairly straight forward and seem to work fine here.
Patch 1 is generic, so not sure through which trees the patchset should go...
-- Stefan
Stefano, I think we really want patch 2/3 applied before the release since it fixes i.MX 7 PSCI. Right now the implementation is really broken and not PSCI 1.0 conformant. But patch 2/3 require this patch to be applied... Not sure how we should handle this.
-- Stefan
Acked-by: Patrick DELAUNAY Patrick.delaunay@st.com Tested-by: Patrick DELAUNAY Patrick.delaunay@st.com
Test done on stm32mp1 platform
Thanks! Patrick
On Tue, Jul 03, 2018 at 02:32:28PM +0200, Stefan Agner wrote:
Hi Tom, Hi Stefano,
On 02.07.2018 11:08, Patrick DELAUNAY wrote:
Hi Stefan
From: Stefan Agner [mailto:stefan@agner.ch] Sent: mercredi 27 juin 2018 10:36 Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
On 24.06.2018 21:09, Stefan Agner wrote:
From: Stefan Agner stefan.agner@toradex.com
A proper stack is required to safely use C code in psci_arch_cpu_entry.
Patrick, I prefer to have your ack on this since you introduced psci_arch_cpu_entry.
As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same function in i.MX 7's PSCI implementation the compiler actually pushed stuff on the (uninitialized) stack, which caused the newly brought up CPU to immediately crash.
Not sure if in your case the stack pointer is already setup by some other means or your compiler does not use the stack.
In any case, I think it is better to just setup the stack properly as done in this patch...
I expected that the secure stack is initialized by bootROM, but after check on 2018.07-rc2, I got a crash also with the stm32mp1 platform.
After code review, my behavior is clearly not safe: I don't sure that the initial BootROM stack is not overlapping the installed PSCI monitor code or data. So I agree: it is needed to initialize the stack in psci_cpu_entry.
Moreover after your patch the crash is solved for my platform stm32mp1.
Given that this fixes two platforms I guess it definitely should go into v2018.07.
Technically, for i.MX 7 only patch 1, 2 and 3 are necessary, but 4 and 5 are fairly straight forward and seem to work fine here.
Patch 1 is generic, so not sure through which trees the patchset should go...
I'm OK with the series coming in if Stefano is.
On 03/07/2018 14:32, Stefan Agner wrote:
Hi Tom, Hi Stefano,
On 02.07.2018 11:08, Patrick DELAUNAY wrote:
Hi Stefan
From: Stefan Agner [mailto:stefan@agner.ch] Sent: mercredi 27 juin 2018 10:36 Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
On 24.06.2018 21:09, Stefan Agner wrote:
From: Stefan Agner stefan.agner@toradex.com
A proper stack is required to safely use C code in psci_arch_cpu_entry.
Patrick, I prefer to have your ack on this since you introduced psci_arch_cpu_entry.
As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same function in i.MX 7's PSCI implementation the compiler actually pushed stuff on the (uninitialized) stack, which caused the newly brought up CPU to immediately crash.
Not sure if in your case the stack pointer is already setup by some other means or your compiler does not use the stack.
In any case, I think it is better to just setup the stack properly as done in this patch...
I expected that the secure stack is initialized by bootROM, but after check on 2018.07-rc2, I got a crash also with the stm32mp1 platform.
After code review, my behavior is clearly not safe: I don't sure that the initial BootROM stack is not overlapping the installed PSCI monitor code or data. So I agree: it is needed to initialize the stack in psci_cpu_entry.
Moreover after your patch the crash is solved for my platform stm32mp1.
Given that this fixes two platforms I guess it definitely should go into v2018.07.
Technically, for i.MX 7 only patch 1, 2 and 3 are necessary, but 4 and 5 are fairly straight forward and seem to work fine here.
Patch 1 is generic, so not sure through which trees the patchset should go...
Sorry for late answer, but benner late as never...;-)
I have applied the whole patchset to u-boot-imx. I will push it in a couple of hours on the server.
Regards, Stefano
participants (4)
-
Patrick DELAUNAY -
Stefan Agner -
Stefano Babic -
Tom Rini