From c8afc789196cdd568e29aa1e54cb5d24be5adf70 Mon Sep 17 00:00:00 2001 From: Achin Gupta Date: Mon, 25 Nov 2013 18:45:02 +0000 Subject: [PATCH] psci: fix error due to a non zero context id In the previous psci implementation, the psci_afflvl_power_on_finish() function would run into an error condition if the value of the context id parameter in the cpu_on and cpu_suspend psci calls was != 0. The parameter was being restored as the return value of the affinity level 0 finisher function. A non zero context id would be treated as an error condition. This would prevent successful wake up of the cpu from a power down state. Also, the contents of the general purpose registers were not being cleared upon return to the non-secure world after a cpu power up. This could potentially allow the non-secure world to view secure data. This patch ensures that all general purpose registers are set to ~0 prior to the final eret that drops the execution to the non-secure world. The context id is used to initialize the general purpose register x0 prior to re-entry into the non-secure world and is no longer restored as a function return value. A platform helper (platform_get_stack()) has been introduced to facilitate this change. Change-Id: I2454911ffd75705d6aa8609a5d250d9b26fa097c --- common/psci/psci_afflvl_on.c | 2 +- common/psci/psci_afflvl_suspend.c | 2 +- common/psci/psci_common.c | 20 +++++++++++++++++--- common/psci/psci_entry.S | 9 ++++++--- common/psci/psci_private.h | 2 +- common/runtime_svc.c | 1 + docs/change-log.md | 4 ++++ docs/porting-guide.md | 19 +++++++++++++++++-- plat/common/aarch64/platform_helpers.S | 20 +++++++++++++++----- plat/fvp/platform.h | 1 + 10 files changed, 64 insertions(+), 16 deletions(-) diff --git a/common/psci/psci_afflvl_on.c b/common/psci/psci_afflvl_on.c index 7126b98d..c118cab6 100644 --- a/common/psci/psci_afflvl_on.c +++ b/common/psci/psci_afflvl_on.c @@ -337,7 +337,7 @@ static unsigned int psci_afflvl0_on_finish(unsigned long mpidr, * for restoring the re-entry info */ index = cpu_node->data; - rc = psci_get_ns_entry_info(index); + psci_get_ns_entry_info(index); /* Clean caches before re-entering normal world */ dcsw_op_louis(DCCSW); diff --git a/common/psci/psci_afflvl_suspend.c b/common/psci/psci_afflvl_suspend.c index b68b4df7..05b0aad0 100644 --- a/common/psci/psci_afflvl_suspend.c +++ b/common/psci/psci_afflvl_suspend.c @@ -389,7 +389,7 @@ static unsigned int psci_afflvl0_suspend_finish(unsigned long mpidr, * information that we had stashed away during the suspend * call to set this cpu on it's way. */ - rc = psci_get_ns_entry_info(index); + psci_get_ns_entry_info(index); /* Clean caches before re-entering normal world */ dcsw_op_louis(DCCSW); diff --git a/common/psci/psci_common.c b/common/psci/psci_common.c index 59126baa..8eb02133 100644 --- a/common/psci/psci_common.c +++ b/common/psci/psci_common.c @@ -36,6 +36,7 @@ #include #include #include +#include /******************************************************************************* * Arrays that contains information needs to resume a cpu's execution when woken @@ -280,9 +281,10 @@ unsigned int psci_calculate_affinity_state(aff_map_node *aff_node) * resume a cpu's execution in the non-secure state after it has been physically * powered on i.e. turned ON or resumed from SUSPEND ******************************************************************************/ -unsigned int psci_get_ns_entry_info(unsigned int index) +void psci_get_ns_entry_info(unsigned int index) { unsigned long sctlr = 0, scr, el_status, id_aa64pfr0; + gp_regs *ns_gp_regs; scr = read_scr(); @@ -318,10 +320,22 @@ unsigned int psci_get_ns_entry_info(unsigned int index) /* Fulfill the cpu_on entry reqs. as per the psci spec */ write_scr(scr); - write_spsr(psci_ns_entry_info[index].eret_info.spsr); write_elr(psci_ns_entry_info[index].eret_info.entrypoint); - return psci_ns_entry_info[index].context_id; + /* + * Set the general purpose registers to ~0 upon entry into the + * non-secure world except for x0 which should contain the + * context id & spsr. This is done directly on the "would be" + * stack pointer. Prior to entry into the non-secure world, an + * offset equivalent to the size of the 'gp_regs' structure is + * added to the sp. This general purpose register context is + * retrieved then. + */ + ns_gp_regs = (gp_regs *) platform_get_stack(read_mpidr()); + ns_gp_regs--; + memset(ns_gp_regs, ~0, sizeof(*ns_gp_regs)); + ns_gp_regs->x0 = psci_ns_entry_info[index].context_id; + ns_gp_regs->spsr = psci_ns_entry_info[index].eret_info.spsr; } /******************************************************************************* diff --git a/common/psci/psci_entry.S b/common/psci/psci_entry.S index 2f39f360..74cdf956 100644 --- a/common/psci/psci_entry.S +++ b/common/psci/psci_entry.S @@ -32,6 +32,7 @@ #include #include #include +#include #include .globl psci_aff_on_finish_entry @@ -77,7 +78,6 @@ psci_aff_common_finish_entry: mov x0, x19 mov x1, #MPIDR_AFFLVL0 blr x22 - mov x21, x0 /* -------------------------------------------- * Give ourselves a stack allocated in Normal @@ -88,10 +88,13 @@ psci_aff_common_finish_entry: bl platform_set_stack /* -------------------------------------------- - * Restore the context id. value + * Use the size of the general purpose register + * context to restore the register state + * stashed by earlier code * -------------------------------------------- */ - mov x0, x21 + sub sp, sp, #SIZEOF_GPREGS + exception_exit restore_regs /* -------------------------------------------- * Jump back to the non-secure world assuming diff --git a/common/psci/psci_private.h b/common/psci/psci_private.h index 4741397c..e2100f83 100644 --- a/common/psci/psci_private.h +++ b/common/psci/psci_private.h @@ -108,7 +108,7 @@ extern int get_max_afflvl(void); extern unsigned int psci_get_phys_state(unsigned int); extern unsigned int psci_get_aff_phys_state(aff_map_node *); extern unsigned int psci_calculate_affinity_state(aff_map_node *); -extern unsigned int psci_get_ns_entry_info(unsigned int index); +extern void psci_get_ns_entry_info(unsigned int index); extern unsigned long mpidr_set_aff_inst(unsigned long,unsigned char, int); extern int psci_change_state(unsigned long, int, int, unsigned int); extern int psci_validate_mpidr(unsigned long, int); diff --git a/common/runtime_svc.c b/common/runtime_svc.c index 0aa4460f..431cfa3c 100644 --- a/common/runtime_svc.c +++ b/common/runtime_svc.c @@ -38,6 +38,7 @@ #include #include #include +#include /******************************************************************************* * Perform initialization of runtime services possibly across exception levels diff --git a/docs/change-log.md b/docs/change-log.md index 73d7e7a1..8be7a514 100644 --- a/docs/change-log.md +++ b/docs/change-log.md @@ -52,6 +52,10 @@ Detailed changes since last release * Definitions of some constants related to the PSCI api calls AFFINITY_INFO and CPU_SUSPEND have been corrected. +* A bug which triggered an error condition in the code executed after a cpu + is powered on, if a non zero context id parameter was passed in the PSCI + CPU_ON and CPU_SUSPEND api calls has been corrected. + ARM Trusted Firmware - version 0.2 ================================== diff --git a/docs/porting-guide.md b/docs/porting-guide.md index 0b071118..c0e6acea 100644 --- a/docs/porting-guide.md +++ b/docs/porting-guide.md @@ -188,8 +188,9 @@ the implementer chooses. In the ARM FVP port, they are implemented in bytes) of the largest cache line amongst all caches implemented in the system. A pointer to this memory should be exported with the name `platform_normal_stacks`. This pointer is used by the common platform helper - function `platform_set_stack()` to allocate a stack to each CPU in the - platform (see [../plat/common/aarch64/platform_helpers.S]). + functions `platform_set_stack()` (to allocate a stack for each CPU in the + platform) & `platform_get_stack()` (to return the base address of that + stack) (see [../plat/common/aarch64/platform_helpers.S]). 2.2 Common optional modifications @@ -262,6 +263,20 @@ The size of the stack allocated to each CPU is specified by the platform defined constant `PLATFORM_STACK_SIZE`. +### Function : platform_get_stack() + + Argument : unsigned long + Return : unsigned long + +This function uses the `platform_normal_stacks` pointer variable to return the +base address of the stack memory reserved for a CPU. Further details are given +in the description of the `platform_normal_stacks` variable below. A CPU is +identified by its `MPIDR`, which is passed as the argument. + +The size of the stack allocated to each CPU is specified by the platform defined +constant `PLATFORM_STACK_SIZE`. + + ### Function : plat_report_exception() Argument : unsigned int diff --git a/plat/common/aarch64/platform_helpers.S b/plat/common/aarch64/platform_helpers.S index 8a6f4931..770c1e23 100644 --- a/plat/common/aarch64/platform_helpers.S +++ b/plat/common/aarch64/platform_helpers.S @@ -35,6 +35,7 @@ .globl pcpu_dv_mem_stack .weak platform_get_core_pos .weak platform_set_stack + .weak platform_get_stack .weak platform_is_primary_cpu .weak platform_set_coherent_stack .weak platform_check_mpidr @@ -95,19 +96,28 @@ platform_is_primary_cpu:; .type platform_is_primary_cpu, %function cset x0, eq ret - /* ----------------------------------------------------- - * void platform_set_stack (int mpidr) + * void platform_get_stack (unsigned long mpidr) * ----------------------------------------------------- */ -platform_set_stack:; .type platform_set_stack, %function - mov x9, x30 // lr +platform_get_stack:; .type platform_get_stack, %function + mov x10, x30 // lr bl platform_get_core_pos add x0, x0, #1 mov x1, #PLATFORM_STACK_SIZE mul x0, x0, x1 ldr x1, =platform_normal_stacks - add sp, x1, x0 + add x0, x1, x0 + ret x10 + + /* ----------------------------------------------------- + * void platform_set_stack (unsigned long mpidr) + * ----------------------------------------------------- + */ +platform_set_stack:; .type platform_set_stack, %function + mov x9, x30 // lr + bl platform_get_stack + mov sp, x0 ret x9 /* ----------------------------------------------------- diff --git a/plat/fvp/platform.h b/plat/fvp/platform.h index 399a772a..38bd5580 100644 --- a/plat/fvp/platform.h +++ b/plat/fvp/platform.h @@ -331,6 +331,7 @@ extern unsigned long platform_get_cfgvar(unsigned int); extern int platform_config_setup(void); extern void plat_report_exception(unsigned long); extern unsigned long plat_get_ns_image_entrypoint(void); +extern unsigned long platform_get_stack(unsigned long mpidr); /* Declarations for fvp_topology.c */ extern int plat_setup_topology(void); -- 2.30.2