From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Andr=c3=a9_Przywara?= Date: Wed, 24 Jun 2020 13:11:41 +0100 Subject: [PATCH v2] arm64: issue ISB after updating system registers In-Reply-To: <20200624010508.668635-1-volodymyr_babchuk@epam.com> References: <20200624010508.668635-1-volodymyr_babchuk@epam.com> Message-ID: <1fd3b137-cbdb-b8dd-1558-38cbd9a08655@arm.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 24/06/2020 02:05, Volodymyr Babchuk wrote: Hi Volodymyr, thanks for the find and for taking care! And thanks Julien for forwarding! > ARM Architecture reference manual clearly states that PE pipeline > should be flushed after any change to system registers. Don't want to be pedantic here, but this does not affect all system registers. Some clearly don't need an ISB, some are self-synchronising. The manual speaks of "*Examples* of context-changing operations ..." Could you change this to "... after changes to some system registers."? > Refer to > paragraph "B2.3.5 Memory Barriers" at page B2-92 of "Arm Architecture > Reference Manual ARMv8 for ARMv8-A Architecture Profile" (ARM DDI > 0487B.a). > > Failing to issue instruction memory synchronization barrier can lead It's just "instruction synchronization barrier". This is not really about memory, it's about making sure that the processor re-fetches the next instructions and "re-evaluates" them again, to take the changed system state into account. (Flushing the pipeline is just one possible way to implement this, btw.) > to spurious errors, like synchronous exception when accessing FPU > registers. This is very prominent on CPUs with long instruction > pipeline, like ARM Cortex A72. > > This change fixes the following U-Boot panic: > > "Synchronous Abort" handler, esr 0x1fe00000 > elr: 00000000800948cc lr : 0000000080091e04 > x0 : 00000000801ffdc8 x1 : 00000000000000c8 > x2 : 00000000800979d4 x3 : 00000000801ffc60 > x4 : 00000000801ffd40 x5 : ffffff80ffffffd8 > x6 : 00000000801ffd70 x7 : 00000000801ffd70 > x8 : 000000000000000a x9 : 0000000000000000 > x10: 0000000000000044 x11: 0000000000000000 > x12: 0000000000000000 x13: 0000000000000000 > x14: 0000000000000000 x15: 0000000000000000 > x16: 000000008008b2e0 x17: 0000000000000000 > x18: 00000000801ffec0 x19: 00000000800957b0 > x20: 00000000000000c8 x21: 00000000801ffdc8 > x22: 000000008009909e x23: 0000000000000000 > x24: 0000000000000000 x25: 0000000000000000 > x26: 0000000000000000 x27: 0000000000000000 > x28: 0000000000000000 x29: 00000000801ffc50 > > Code: a94417e4 a90217e4 a9051fe6 a90617e4 (3d801fe0) > > While executing instruction > > str q0, [sp, #112] > > in vsnprintf() prologue. This panic was observed only on Cortex A72 so > far. > > This patch places ISBs on other strategic places as well. > > Also, this probably is the right fix for the issue workarounded in the > commit 45f41c13 ("ARM: uniphier: add weird workaround code for LD20") > > Reported-by: Oleksandr Andrushchenko > Suggested-by: Julien Grall > Signed-off-by: Volodymyr Babchuk > CC: Tom Rini > CC: Masahiro Yamada > CC: Stefano Stabellini I checked the A57 ACTLR_EL1 and ECTLR_EL1 bits affected below, and indeed an ISB seems to be in order here. Since this is the initialization code path, it wouldn't hurt anyway ;-) Reviewed-by: Andre Przywara Cheers, Andre > -- > > Changes from v1: > - Added ISBs under CONFIG_ARMV8_SET_SMPEN and erratas. > - Added Stefano, Julien and Oleksandr > --- > arch/arm/cpu/armv8/start.S | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S > index 99d126660d..002698b501 100644 > --- a/arch/arm/cpu/armv8/start.S > +++ b/arch/arm/cpu/armv8/start.S > @@ -120,6 +120,7 @@ pie_fixup_done: > mov x0, #3 << 20 > msr cpacr_el1, x0 /* Enable FP/SIMD */ > 0: > + isb > > /* > * Enable SMPEN bit for coherency. > @@ -132,6 +133,7 @@ pie_fixup_done: > mrs x0, S3_1_c15_c2_1 /* cpuectlr_el1 */ > orr x0, x0, #0x40 > msr S3_1_c15_c2_1, x0 > + isb > 1: > #endif > > @@ -233,6 +235,7 @@ apply_a53_core_errata: > /* Enable data cache clean as data cache clean/invalidate */ > orr x0, x0, #1 << 44 > msr S3_1_c15_c2_0, x0 /* cpuactlr_el1 */ > + isb > #endif > b 0b > > @@ -247,6 +250,7 @@ apply_a57_core_errata: > /* Disable write streaming no-allocate threshold */ > orr x0, x0, #3 << 27 > msr S3_1_c15_c2_0, x0 /* cpuactlr_el1 */ > + isb > #endif > > #ifdef CONFIG_ARM_ERRATA_826974 > @@ -254,6 +258,7 @@ apply_a57_core_errata: > /* Disable speculative load execution ahead of a DMB */ > orr x0, x0, #1 << 59 > msr S3_1_c15_c2_0, x0 /* cpuactlr_el1 */ > + isb > #endif > > #ifdef CONFIG_ARM_ERRATA_833471 > @@ -263,6 +268,7 @@ apply_a57_core_errata: > could impact performance. */ > orr x0, x0, #1 << 38 > msr S3_1_c15_c2_0, x0 /* cpuactlr_el1 */ > + isb > #endif > > #ifdef CONFIG_ARM_ERRATA_829520 > @@ -273,6 +279,7 @@ apply_a57_core_errata: > could impact performance. */ > orr x0, x0, #1 << 4 > msr S3_1_c15_c2_0, x0 /* cpuactlr_el1 */ > + isb > #endif > > #ifdef CONFIG_ARM_ERRATA_833069 > @@ -280,6 +287,7 @@ apply_a57_core_errata: > /* Disable Enable Invalidates of BTB bit */ > and x0, x0, #0xE > msr S3_1_c15_c2_0, x0 /* cpuactlr_el1 */ > + isb > #endif > b 0b > ENDPROC(apply_core_errata) >