ARM: armv7: Add early stack for erratum workarounds

PATCH In review Andrew F. Davis 4 participants 6 messages Nov 19, 2018 – Nov 27, 2018 Arm (32-bit)

Share & tools

Andrew F. Davis

author

2018-11-19 20:47 UTC #0 lore raw

PATCH 1/1 ARM: armv7: Add early stack for erratum workarounds

Some erratum workarounds call into C code before the stack is setup, this can lead to values pushed onto the stack being lost, firewall exceptions, and other undefined behavior.

Setup a temporary stack to allow these functions to work correctly.

Signed-off-by Andrew F. Davis <[email protected]>

---
 arch/arm/cpu/armv7/start.S | 9 +++++++++
 1 file changed, 9 insertions(+)

arch/arm/cpu/armv7/start.S +9 -0

index 81edec01bf..0cb6dd39cc 100644
@@ -205,6 +205,15 @@ ENTRY(cpu_init_cp15)
 	mov	r2, r3, lsl #4		@ shift variant field for combined value
 	orr	r2, r4, r2		@ r2 has combined CPU variant + revision
 
+/* Early stack for ERRATA that needs into call C code */
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
+	ldr	r0, =(CONFIG_SPL_STACK)
+#else
+	ldr	r0, =(CONFIG_SYS_INIT_SP_ADDR)
+#endif
+	bic	r0, r0, #7	/* 8-byte alignment for ABI compliance */
+	mov	sp, r0
+
 #ifdef CONFIG_ARM_ERRATA_798870
 	cmp	r2, #0x30		@ Applies to lower than R3p0
 	bge	skip_errata_798870      @ skip if not affected rev

Signature · 1 line

2.19.1

Andreas Dannenberg

2018-11-20 14:48 UTC #1 lore raw

PATCH 1/1 ARM: armv7: Add early stack for erratum workarounds

Quoted 17 lines · Andrew F. Davis

Some erratum workarounds call into C code before the stack is setup, this can lead to values pushed onto the stack being lost, firewall exceptions, and other undefined behavior.
Setup a temporary stack to allow these functions to work correctly.
Signed-off-by Andrew F. Davis <[email protected]>
---
 arch/arm/cpu/armv7/start.S | 9 +++++++++
 1 file changed, 9 insertions(+)
arch/arm/cpu/armv7/start.S +0 -0
index 81edec01bf..0cb6dd39cc 100644

arch/arm/cpu/armv7/start.S ENTRY(cpu_init_cp15) · line 214

Quoted 10 lines

arch/arm/cpu/armv7/start.S +7 -0

@@ -205,6 +205,15 @@ ENTRY(cpu_init_cp15)
 	mov	r2, r3, lsl #4		@ shift variant field for combined value
 	orr	r2, r4, r2		@ r2 has combined CPU variant + revision
 
+/* Early stack for ERRATA that needs into call C code */
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
+	ldr	r0, =(CONFIG_SPL_STACK)
+#else
+	ldr	r0, =(CONFIG_SYS_INIT_SP_ADDR)
+#endif
+	bic	r0, r0, #7	/* 8-byte alignment for ABI compliance */

Perhaps use the same comment-style ('@') and alignment like the rest of that file?

Other than that looks good to me. Just wonder how this was noticed just now...

Acked-by Andreas Dannenberg <[email protected]>

Quoted 12 lines

arch/arm/cpu/armv7/start.S +2 -0

+	mov	sp, r0
+
 #ifdef CONFIG_ARM_ERRATA_798870
 	cmp	r2, #0x30		@ Applies to lower than R3p0
 	bge	skip_errata_798870      @ skip if not affected rev

Signature · 6 lines

2.19.1

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Andrew F. Davis

author

2018-11-20 16:20 UTC #2 lore raw

PATCH 1/1 ARM: armv7: Add early stack for erratum workarounds

Quoted 21 lines · Andreas Dannenberg

Quoted 20 lines · Andrew F. Davis
Some erratum workarounds call into C code before the stack is setup, this can lead to values pushed onto the stack being lost, firewall exceptions, and other undefined behavior.
Setup a temporary stack to allow these functions to work correctly.
Signed-off-by Andrew F. Davis <[email protected]>
---
 arch/arm/cpu/armv7/start.S | 9 +++++++++
 1 file changed, 9 insertions(+)
arch/arm/cpu/armv7/start.S +0 -0
index 81edec01bf..0cb6dd39cc 100644
@@ -205,6 +205,15 @@ ENTRY(cpu_init_cp15)
 	mov	r2, r3, lsl #4		@ shift variant field for combined value
 	orr	r2, r4, r2		@ r2 has combined CPU variant + revision
 

Quoted 11 lines

arch/arm/cpu/armv7/start.S ENTRY(cpu_init_cp15) · line 214
Quoted 7 lines
arch/arm/cpu/armv7/start.S +7 -0
+/* Early stack for ERRATA that needs into call C code */
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
+	ldr	r0, =(CONFIG_SPL_STACK)
+#else
+	ldr	r0, =(CONFIG_SYS_INIT_SP_ADDR)
+#endif
+	bic	r0, r0, #7	/* 8-byte alignment for ABI compliance */
Perhaps use the same comment-style ('@') and alignment like the rest of that file?

This code was borrowed from crt0.S where this is the comment style, so seems assembly comments are not aligned across U-Boot, but I agree they should be at least within a single file like here.

Quoted 3 lines

Other than that looks good to me. Just wonder how this was noticed just now...

I'm wondering that too, took me a while to believe I was the first to hit this and just thought that I was missing something.

The reason is probably that these errata fixes are not enabled for any of our production builds by default. Just now with this Spectre workaround do we enable one by default. Plus on non-HS platforms null pointer dereferences and pushing to a null stack this early only silently drop values it seems, only on HS does a firewall violation alert one of such a problem.

Andrew

Quoted 16 lines

Acked-by Andreas Dannenberg <[email protected]>

Quoted 12 lines

arch/arm/cpu/armv7/start.S +2 -0

+	mov	sp, r0
+
 #ifdef CONFIG_ARM_ERRATA_798870
 	cmp	r2, #0x30		@ Applies to lower than R3p0
 	bge	skip_errata_798870      @ skip if not affected rev

Signature · 6 lines

2.19.1

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Nishanth Menon

2018-11-20 23:02 UTC #3 lore raw

PATCH 1/1 ARM: armv7: Add early stack for erratum workarounds

In reply to Andrew F. Davis

Quoted 19 lines · Andrew F. Davis

Some erratum workarounds call into C code before the stack is setup, this can lead to values pushed onto the stack being lost, firewall exceptions, and other undefined behavior.
Setup a temporary stack to allow these functions to work correctly.
Signed-off-by Andrew F. Davis <[email protected]>
---
 arch/arm/cpu/armv7/start.S | 9 +++++++++
 1 file changed, 9 insertions(+)
arch/arm/cpu/armv7/start.S +0 -0
index 81edec01bf..0cb6dd39cc 100644
@@ -205,6 +205,15 @@ ENTRY(cpu_init_cp15)
 	mov	r2, r3, lsl #4		@ shift variant field for combined value
 	orr	r2, r4, r2		@ r2 has combined CPU variant + revision

arch/arm/cpu/armv7/start.S ENTRY(cpu_init_cp15) · line 216

Quoted 10 lines

arch/arm/cpu/armv7/start.S +9 -0

 
+/* Early stack for ERRATA that needs into call C code */
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
+	ldr	r0, =(CONFIG_SPL_STACK)
+#else
+	ldr	r0, =(CONFIG_SYS_INIT_SP_ADDR)
+#endif
+	bic	r0, r0, #7	/* 8-byte alignment for ABI compliance */
+	mov	sp, r0
+

Dumb q:
Is this a little too late? bl cpu_init_cp15 gets invoked from reset.. doesnt sp need to be setup prior to the call in?

Quoted 6 lines

arch/arm/cpu/armv7/start.S +0 -0

 #ifdef CONFIG_ARM_ERRATA_798870
 	cmp	r2, #0x30		@ Applies to lower than R3p0
 	bge	skip_errata_798870      @ skip if not affected rev

Signature · 1 line

2.19.1

Signature · 2 lines

Regards,
Nishanth Menon

Nishanth Menon

2018-11-20 23:14 UTC #4 lore raw

PATCH 1/1 ARM: armv7: Add early stack for erratum workarounds

Quoted 21 lines · Nishanth Menon

Quoted 20 lines · Andrew F. Davis
Some erratum workarounds call into C code before the stack is setup, this can lead to values pushed onto the stack being lost, firewall exceptions, and other undefined behavior.
Setup a temporary stack to allow these functions to work correctly.
Signed-off-by Andrew F. Davis <[email protected]>
---
 arch/arm/cpu/armv7/start.S | 9 +++++++++
 1 file changed, 9 insertions(+)
arch/arm/cpu/armv7/start.S +0 -0
index 81edec01bf..0cb6dd39cc 100644
@@ -205,6 +205,15 @@ ENTRY(cpu_init_cp15)
 	mov	r2, r3, lsl #4		@ shift variant field for combined value
 	orr	r2, r4, r2		@ r2 has combined CPU variant + revision
 

Quoted 13 lines

arch/arm/cpu/armv7/start.S ENTRY(cpu_init_cp15) · line 216
Quoted 9 lines
arch/arm/cpu/armv7/start.S +9 -0
+/* Early stack for ERRATA that needs into call C code */
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
+	ldr	r0, =(CONFIG_SPL_STACK)
+#else
+	ldr	r0, =(CONFIG_SYS_INIT_SP_ADDR)
+#endif
+	bic	r0, r0, #7	/* 8-byte alignment for ABI compliance */
+	mov	sp, r0
+
Dumb q:
Is this a little too late? bl cpu_init_cp15 gets invoked from reset.. doesnt sp need to be setup prior to the call in?

OK. I guess I will answer my own dumb question -> we don't push till we get here! Sigh.. I should have figured that when i introduced the series!

I guess I introduced the bug in c616a0df297e ("ARM: Introduce erratum workaround for 798870", just a mere 3 years ago :(...

Apologies, but not sure if intermediate stack setup should be something folks prefer earlier or not.. will let ARM folks comment.

Reviewed-by Nishanth Menon <[email protected]>

Signature · 2 lines

Regards,
Nishanth Menon

Tom Rini

2018-11-27 18:47 UTC #5 lore raw

In reply to Andrew F. Davis

Quoted 10 lines · Andrew F. Davis

Some erratum workarounds call into C code before the stack is setup, this can lead to values pushed onto the stack being lost, firewall exceptions, and other undefined behavior.
Setup a temporary stack to allow these functions to work correctly.
Signed-off-by Andrew F. Davis <[email protected]>
Acked-by Andreas Dannenberg <[email protected]>
Reviewed-by Nishanth Menon <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181127/e9da838d/attachment.sig>