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
diff --git a/common/psci/psci_afflvl_on.c b/common/psci/psci_afflvl_on.c
index 7126b98..c118cab 100644
--- a/common/psci/psci_afflvl_on.c
+++ b/common/psci/psci_afflvl_on.c
@@ -337,7 +337,7 @@
* 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 b68b4df..05b0aad 100644
--- a/common/psci/psci_afflvl_suspend.c
+++ b/common/psci/psci_afflvl_suspend.c
@@ -389,7 +389,7 @@
* 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 59126ba..8eb0213 100644
--- a/common/psci/psci_common.c
+++ b/common/psci/psci_common.c
@@ -36,6 +36,7 @@
#include <platform.h>
#include <psci.h>
#include <psci_private.h>
+#include <runtime_svc.h>
/*******************************************************************************
* Arrays that contains information needs to resume a cpu's execution when woken
@@ -280,9 +281,10 @@
* 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 @@
/* 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 2f39f36..74cdf95 100644
--- a/common/psci/psci_entry.S
+++ b/common/psci/psci_entry.S
@@ -32,6 +32,7 @@
#include <platform.h>
#include <psci.h>
#include <psci_private.h>
+#include <runtime_svc.h>
#include <asm_macros.S>
.globl psci_aff_on_finish_entry
@@ -77,7 +78,6 @@
mov x0, x19
mov x1, #MPIDR_AFFLVL0
blr x22
- mov x21, x0
/* --------------------------------------------
* Give ourselves a stack allocated in Normal
@@ -88,10 +88,13 @@
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 4741397..e2100f8 100644
--- a/common/psci/psci_private.h
+++ b/common/psci/psci_private.h
@@ -108,7 +108,7 @@
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 0aa4460..431cfa3 100644
--- a/common/runtime_svc.c
+++ b/common/runtime_svc.c
@@ -38,6 +38,7 @@
#include <semihosting.h>
#include <bl_common.h>
#include <psci.h>
+#include <runtime_svc.h>
/*******************************************************************************
* Perform initialization of runtime services possibly across exception levels
diff --git a/docs/change-log.md b/docs/change-log.md
index 73d7e7a..8be7a51 100644
--- a/docs/change-log.md
+++ b/docs/change-log.md
@@ -52,6 +52,10 @@
* 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 0b07111..c0e6ace 100644
--- a/docs/porting-guide.md
+++ b/docs/porting-guide.md
@@ -188,8 +188,9 @@
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 @@
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 8a6f493..770c1e2 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 @@
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 399a772..38bd558 100644
--- a/plat/fvp/platform.h
+++ b/plat/fvp/platform.h
@@ -331,6 +331,7 @@
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);