From 8cec598ba3b689b86d9dfc58bca5610bdc48f55a Mon Sep 17 00:00:00 2001 From: Andrew Thoelke Date: Mon, 28 Apr 2014 12:28:39 +0100 Subject: [PATCH] Correct usage of data and instruction barriers The current code does not always use data and instruction barriers as required by the architecture and frequently uses barriers excessively due to their inclusion in all of the write_*() helper functions. Barriers should be used explicitly in assembler or C code when modifying processor state that requires the barriers in order to enable review of correctness of the code. This patch removes the barriers from the helper functions and introduces them as necessary elsewhere in the code. PORTING NOTE: check any port of Trusted Firmware for use of system register helper functions for reliance on the previous barrier behaviour and add explicit barriers as necessary. Fixes ARM-software/tf-issues#92 Change-Id: Ie63e187404ff10e0bdcb39292dd9066cb84c53bf --- bl1/aarch64/bl1_arch_setup.c | 1 + bl1/aarch64/bl1_entrypoint.S | 1 - bl1/aarch64/bl1_exceptions.S | 1 + bl2/aarch64/bl2_entrypoint.S | 1 - bl31/aarch64/bl31_entrypoint.S | 1 - bl31/bl31_main.c | 1 + drivers/arm/gic/aarch64/gic_v3_sysregs.S | 4 -- lib/aarch64/cache_helpers.S | 16 ------ lib/aarch64/cpu_helpers.S | 1 + lib/aarch64/misc_helpers.S | 6 --- lib/aarch64/sysreg_helpers.S | 60 +-------------------- lib/aarch64/tlb_helpers.S | 14 ----- plat/fvp/aarch64/plat_common.c | 21 ++++++++ plat/fvp/plat_gic.c | 3 ++ plat/fvp/plat_pm.c | 6 ++- services/std_svc/psci/psci_afflvl_off.c | 1 + services/std_svc/psci/psci_afflvl_suspend.c | 1 + services/std_svc/psci/psci_entry.S | 3 +- 18 files changed, 37 insertions(+), 105 deletions(-) diff --git a/bl1/aarch64/bl1_arch_setup.c b/bl1/aarch64/bl1_arch_setup.c index 758b8e8f..a1ebbdb2 100644 --- a/bl1/aarch64/bl1_arch_setup.c +++ b/bl1/aarch64/bl1_arch_setup.c @@ -44,6 +44,7 @@ void bl1_arch_setup(void) tmp_reg |= (SCTLR_A_BIT | SCTLR_SA_BIT); tmp_reg &= ~SCTLR_EE_BIT; write_sctlr_el3(tmp_reg); + isb(); /* * Enable HVCs, route FIQs to EL3, set the next EL to be AArch64, route diff --git a/bl1/aarch64/bl1_entrypoint.S b/bl1/aarch64/bl1_entrypoint.S index 012b779c..e25386f7 100644 --- a/bl1/aarch64/bl1_entrypoint.S +++ b/bl1/aarch64/bl1_entrypoint.S @@ -86,7 +86,6 @@ func bl1_entrypoint mrs x0, sctlr_el3 orr x0, x0, #SCTLR_I_BIT msr sctlr_el3, x0 - isb _wait_for_entrypoint: diff --git a/bl1/aarch64/bl1_exceptions.S b/bl1/aarch64/bl1_exceptions.S index 68d088b7..7f930d83 100644 --- a/bl1/aarch64/bl1_exceptions.S +++ b/bl1/aarch64/bl1_exceptions.S @@ -221,6 +221,7 @@ func process_exception bl read_sctlr_el3 bic x0, x0, x1 bl write_sctlr_el3 + isb mov x0, #DCCISW bl dcsw_op_all bl tlbialle3 diff --git a/bl2/aarch64/bl2_entrypoint.S b/bl2/aarch64/bl2_entrypoint.S index b8af9a55..cd07aa9d 100644 --- a/bl2/aarch64/bl2_entrypoint.S +++ b/bl2/aarch64/bl2_entrypoint.S @@ -73,7 +73,6 @@ func bl2_entrypoint mrs x0, sctlr_el1 orr x0, x0, #SCTLR_I_BIT msr sctlr_el1, x0 - isb /* --------------------------------------------- diff --git a/bl31/aarch64/bl31_entrypoint.S b/bl31/aarch64/bl31_entrypoint.S index 39fa605e..1b79421d 100644 --- a/bl31/aarch64/bl31_entrypoint.S +++ b/bl31/aarch64/bl31_entrypoint.S @@ -89,7 +89,6 @@ func bl31_entrypoint mrs x1, sctlr_el3 orr x1, x1, #SCTLR_I_BIT msr sctlr_el3, x1 - isb /* --------------------------------------------- diff --git a/bl31/bl31_main.c b/bl31/bl31_main.c index 01f00f23..755320d3 100644 --- a/bl31/bl31_main.c +++ b/bl31/bl31_main.c @@ -100,6 +100,7 @@ void bl31_main(void) assert(cm_get_context(mpidr, NON_SECURE)); cm_set_next_eret_context(NON_SECURE); write_vbar_el3((uint64_t) runtime_exceptions); + isb(); next_image_type = NON_SECURE; /* diff --git a/drivers/arm/gic/aarch64/gic_v3_sysregs.S b/drivers/arm/gic/aarch64/gic_v3_sysregs.S index 2a96da76..ddf85a8e 100644 --- a/drivers/arm/gic/aarch64/gic_v3_sysregs.S +++ b/drivers/arm/gic/aarch64/gic_v3_sysregs.S @@ -67,23 +67,19 @@ func read_icc_sre_el3 func write_icc_sre_el1 msr ICC_SRE_EL1, x0 - isb ret func write_icc_sre_el2 msr ICC_SRE_EL2, x0 - isb ret func write_icc_sre_el3 msr ICC_SRE_EL3, x0 - isb ret func write_icc_pmr_el1 msr ICC_PMR_EL1, x0 - isb ret diff --git a/lib/aarch64/cache_helpers.S b/lib/aarch64/cache_helpers.S index 2649ad0e..dc919751 100644 --- a/lib/aarch64/cache_helpers.S +++ b/lib/aarch64/cache_helpers.S @@ -46,57 +46,41 @@ func dcisw dc isw, x0 - dsb sy - isb ret func dccisw dc cisw, x0 - dsb sy - isb ret func dccsw dc csw, x0 - dsb sy - isb ret func dccvac dc cvac, x0 - dsb sy - isb ret func dcivac dc ivac, x0 - dsb sy - isb ret func dccivac dc civac, x0 - dsb sy - isb ret func dccvau dc cvau, x0 - dsb sy - isb ret func dczva dc zva, x0 - dsb sy - isb ret diff --git a/lib/aarch64/cpu_helpers.S b/lib/aarch64/cpu_helpers.S index 573d0b8b..4e5eb5bd 100644 --- a/lib/aarch64/cpu_helpers.S +++ b/lib/aarch64/cpu_helpers.S @@ -52,5 +52,6 @@ smp_setup_begin: bl read_cpuectlr orr x0, x0, #CPUECTLR_SMP_BIT bl write_cpuectlr + isb smp_setup_end: ret x19 diff --git a/lib/aarch64/misc_helpers.S b/lib/aarch64/misc_helpers.S index e7b23313..c33ade28 100644 --- a/lib/aarch64/misc_helpers.S +++ b/lib/aarch64/misc_helpers.S @@ -187,19 +187,16 @@ func write_spsr func write_spsr_el1 msr spsr_el1, x0 - isb ret func write_spsr_el2 msr spsr_el2, x0 - isb ret func write_spsr_el3 msr spsr_el3, x0 - isb ret @@ -240,19 +237,16 @@ func write_elr func write_elr_el1 msr elr_el1, x0 - isb ret func write_elr_el2 msr elr_el2, x0 - isb ret func write_elr_el3 msr elr_el3, x0 - isb ret diff --git a/lib/aarch64/sysreg_helpers.S b/lib/aarch64/sysreg_helpers.S index 61468f95..1d73ba9e 100644 --- a/lib/aarch64/sysreg_helpers.S +++ b/lib/aarch64/sysreg_helpers.S @@ -201,19 +201,16 @@ func read_vbar_el3 func write_vbar_el1 msr vbar_el1, x0 - isb ret func write_vbar_el2 msr vbar_el2, x0 - isb ret func write_vbar_el3 msr vbar_el3, x0 - isb ret @@ -238,19 +235,16 @@ func read_afsr0_el3 func write_afsr0_el1 msr afsr0_el1, x0 - isb ret func write_afsr0_el2 msr afsr0_el2, x0 - isb ret func write_afsr0_el3 msr afsr0_el3, x0 - isb ret @@ -275,19 +269,16 @@ func read_far_el3 func write_far_el1 msr far_el1, x0 - isb ret func write_far_el2 msr far_el2, x0 - isb ret func write_far_el3 msr far_el3, x0 - isb ret @@ -312,19 +303,16 @@ func read_mair_el3 func write_mair_el1 msr mair_el1, x0 - isb ret func write_mair_el2 msr mair_el2, x0 - isb ret func write_mair_el3 msr mair_el3, x0 - isb ret @@ -349,19 +337,16 @@ func read_amair_el3 func write_amair_el1 msr amair_el1, x0 - isb ret func write_amair_el2 msr amair_el2, x0 - isb ret func write_amair_el3 msr amair_el3, x0 - isb ret @@ -405,19 +390,16 @@ func read_rmr_el3 func write_rmr_el1 msr rmr_el1, x0 - isb ret func write_rmr_el2 msr rmr_el2, x0 - isb ret func write_rmr_el3 msr rmr_el3, x0 - isb ret @@ -442,19 +424,16 @@ func read_afsr1_el3 func write_afsr1_el1 msr afsr1_el1, x0 - isb ret func write_afsr1_el2 msr afsr1_el2, x0 - isb ret func write_afsr1_el3 msr afsr1_el3, x0 - isb ret @@ -479,22 +458,16 @@ func read_sctlr_el3 func write_sctlr_el1 msr sctlr_el1, x0 - dsb sy - isb ret func write_sctlr_el2 msr sctlr_el2, x0 - dsb sy - isb ret func write_sctlr_el3 msr sctlr_el3, x0 - dsb sy - isb ret @@ -519,22 +492,16 @@ func read_actlr_el3 func write_actlr_el1 msr actlr_el1, x0 - dsb sy - isb ret func write_actlr_el2 msr actlr_el2, x0 - dsb sy - isb ret func write_actlr_el3 msr actlr_el3, x0 - dsb sy - isb ret @@ -559,22 +526,16 @@ func read_esr_el3 func write_esr_el1 msr esr_el1, x0 - dsb sy - isb ret func write_esr_el2 msr esr_el2, x0 - dsb sy - isb ret func write_esr_el3 msr esr_el3, x0 - dsb sy - isb ret @@ -599,22 +560,16 @@ func read_tcr_el3 func write_tcr_el1 msr tcr_el1, x0 - dsb sy - isb ret func write_tcr_el2 msr tcr_el2, x0 - dsb sy - isb ret func write_tcr_el3 msr tcr_el3, x0 - dsb sy - isb ret @@ -643,15 +598,11 @@ func write_cptr_el1 func write_cptr_el2 msr cptr_el2, x0 - dsb sy - isb ret func write_cptr_el3 msr cptr_el3, x0 - dsb sy - isb ret @@ -676,19 +627,16 @@ func read_ttbr0_el3 func write_ttbr0_el1 msr ttbr0_el1, x0 - isb ret func write_ttbr0_el2 msr ttbr0_el2, x0 - isb ret func write_ttbr0_el3 msr ttbr0_el3, x0 - isb ret @@ -711,7 +659,6 @@ func read_ttbr1_el3 func write_ttbr1_el1 msr ttbr1_el1, x0 - isb ret @@ -730,8 +677,6 @@ func read_hcr func write_hcr msr hcr_el2, x0 - dsb sy - isb ret @@ -762,8 +707,6 @@ func read_cpuectlr func write_cpuectlr msr CPUECTLR_EL1, x0 - dsb sy - isb ret @@ -789,8 +732,6 @@ func write_cntfrq func write_scr msr scr_el3, x0 - dsb sy - isb ret @@ -818,6 +759,7 @@ func enable_vfp mov x1, #AARCH64_CPTR_TFP bic x0, x0, x1 msr cptr_el3, x0 + isb ret diff --git a/lib/aarch64/tlb_helpers.S b/lib/aarch64/tlb_helpers.S index ec1558b0..8dfae12e 100644 --- a/lib/aarch64/tlb_helpers.S +++ b/lib/aarch64/tlb_helpers.S @@ -41,47 +41,33 @@ func tlbialle1 tlbi alle1 - dsb sy - isb ret func tlbialle1is tlbi alle1is - dsb sy - isb ret func tlbialle2 tlbi alle2 - dsb sy - isb ret func tlbialle2is tlbi alle2is - dsb sy - isb ret func tlbialle3 tlbi alle3 - dsb sy - isb ret func tlbialle3is tlbi alle3is - dsb sy - isb ret func tlbivmalle1 tlbi vmalle1 - dsb sy - isb ret diff --git a/plat/fvp/aarch64/plat_common.c b/plat/fvp/aarch64/plat_common.c index c8e529d4..e2f23437 100644 --- a/plat/fvp/aarch64/plat_common.c +++ b/plat/fvp/aarch64/plat_common.c @@ -69,6 +69,8 @@ void enable_mmu() ttbr = (unsigned long) l1_xlation_table; if (GET_EL(current_el) == MODE_EL3) { + assert((read_sctlr_el3() & SCTLR_M_BIT) == 0); + write_mair_el3(mair); tcr |= TCR_EL3_RES1; /* Invalidate EL3 TLBs */ @@ -77,11 +79,19 @@ void enable_mmu() write_tcr_el3(tcr); write_ttbr0_el3(ttbr); + /* ensure all translation table writes have drained into memory, + * the TLB invalidation is complete, and translation register + * writes are committed before enabling the MMU + */ + dsb(); + isb(); + sctlr = read_sctlr_el3(); sctlr |= SCTLR_WXN_BIT | SCTLR_M_BIT | SCTLR_I_BIT; sctlr |= SCTLR_A_BIT | SCTLR_C_BIT; write_sctlr_el3(sctlr); } else { + assert((read_sctlr_el1() & SCTLR_M_BIT) == 0); write_mair_el1(mair); /* Invalidate EL1 TLBs */ @@ -90,11 +100,20 @@ void enable_mmu() write_tcr_el1(tcr); write_ttbr0_el1(ttbr); + /* ensure all translation table writes have drained into memory, + * the TLB invalidation is complete, and translation register + * writes are committed before enabling the MMU + */ + dsb(); + isb(); + sctlr = read_sctlr_el1(); sctlr |= SCTLR_WXN_BIT | SCTLR_M_BIT | SCTLR_I_BIT; sctlr |= SCTLR_A_BIT | SCTLR_C_BIT; write_sctlr_el1(sctlr); } + /* ensure the MMU enable takes effect immediately */ + isb(); return; } @@ -113,6 +132,8 @@ void disable_mmu(void) sctlr = sctlr & ~(SCTLR_M_BIT | SCTLR_C_BIT); write_sctlr_el1(sctlr); } + /* ensure the MMU disable takes effect immediately */ + isb(); /* Flush the caches */ dcsw_op_all(DCCISW); diff --git a/plat/fvp/plat_gic.c b/plat/fvp/plat_gic.c index 8457af1a..db3c9cf6 100644 --- a/plat/fvp/plat_gic.c +++ b/plat/fvp/plat_gic.c @@ -86,6 +86,7 @@ void gicv3_cpuif_setup(void) */ scr_val = read_scr(); write_scr(scr_val | SCR_NS_BIT); + isb(); /* ensure NS=1 takes effect before accessing ICC_SRE_EL2 */ /* * By default EL2 and NS-EL1 software should be able to enable GICv3 @@ -103,9 +104,11 @@ void gicv3_cpuif_setup(void) write_icc_sre_el2(val | ICC_SRE_EN | ICC_SRE_SRE); write_icc_pmr_el1(GIC_PRI_MASK); + isb(); /* commite ICC_* changes before setting NS=0 */ /* Restore SCR_EL3 */ write_scr(scr_val); + isb(); /* ensure NS=0 takes effect immediately */ } /******************************************************************************* diff --git a/plat/fvp/plat_pm.c b/plat/fvp/plat_pm.c index 5430fffd..f80e2d7b 100644 --- a/plat/fvp/plat_pm.c +++ b/plat/fvp/plat_pm.c @@ -54,7 +54,11 @@ int fvp_affinst_standby(unsigned int power_state) if (target_afflvl != MPIDR_AFFLVL0) return PSCI_E_INVALID_PARAMS; - /* Enter standby state */ + /* + * Enter standby state + * dsb is good practice before using wfi to enter low power states + */ + dsb(); wfi(); return PSCI_E_SUCCESS; diff --git a/services/std_svc/psci/psci_afflvl_off.c b/services/std_svc/psci/psci_afflvl_off.c index e007bc30..21a4d1a6 100644 --- a/services/std_svc/psci/psci_afflvl_off.c +++ b/services/std_svc/psci/psci_afflvl_off.c @@ -82,6 +82,7 @@ static int psci_afflvl0_off(unsigned long mpidr, aff_map_node_t *cpu_node) sctlr = read_sctlr_el3(); sctlr &= ~SCTLR_C_BIT; write_sctlr_el3(sctlr); + isb(); /* ensure MMU disable takes immediate effect */ /* * CAUTION: This flush to the level of unification makes an assumption diff --git a/services/std_svc/psci/psci_afflvl_suspend.c b/services/std_svc/psci/psci_afflvl_suspend.c index dc12f7a3..534e4a9f 100644 --- a/services/std_svc/psci/psci_afflvl_suspend.c +++ b/services/std_svc/psci/psci_afflvl_suspend.c @@ -198,6 +198,7 @@ static int psci_afflvl0_suspend(unsigned long mpidr, sctlr = read_sctlr_el3(); sctlr &= ~SCTLR_C_BIT; write_sctlr_el3(sctlr); + isb(); /* ensure MMU disable takes immediate effect */ /* * CAUTION: This flush to the level of unification makes an assumption diff --git a/services/std_svc/psci/psci_entry.S b/services/std_svc/psci/psci_entry.S index e2c690db..ec55a819 100644 --- a/services/std_svc/psci/psci_entry.S +++ b/services/std_svc/psci/psci_entry.S @@ -75,7 +75,6 @@ psci_aff_common_finish_entry: * --------------------------------------------- */ msr spsel, #0 - isb bl read_mpidr mov x19, x0 @@ -158,7 +157,7 @@ func __psci_cpu_suspend ret func final_wfi - dsb sy + dsb sy // ensure write buffer empty wfi wfi_spill: b wfi_spill -- 2.30.2