mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH v2] ARM: document arm_setup_stack() pitfalls
@ 2021-09-16  9:37 Ahmad Fatoum
  2021-09-16  9:46 ` Jules Maselbas
  2021-09-16 10:48 ` Uwe Kleine-König
  0 siblings, 2 replies; 3+ messages in thread
From: Ahmad Fatoum @ 2021-09-16  9:37 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum, Jules Maselbas

Many arm32 board entry points use arm_setup_stack() to set up
the stack from C code. This necessitates using __naked, which
probably has been our most frequent cause of misscompiled C code.

GCC is quite clear that:

  Only basic asm statements can safely be included in naked functions
  While using extended asm or a mixture of basic asm and C code may
  appear to work, they cannot be depended upon to work reliably and
  are not supported.

But some boards use it anyway, because it's nice to avoid writing
assembly. Reading generated assembly to spot compiler miscompilation
isn't that nice though, so add some documentation, comments
and compiler diagnostics to hopefully reduce future porting effort.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - fix commit message title

Cc: Jules Maselbas <jmaselbas@kalray.eu>
---
 Documentation/devel/porting.rst | 21 +++++++++++++++++++++
 arch/arm/include/asm/common.h   | 22 ++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/Documentation/devel/porting.rst b/Documentation/devel/porting.rst
index 97b787327c9a..699badbec672 100644
--- a/Documentation/devel/porting.rst
+++ b/Documentation/devel/porting.rst
@@ -169,6 +169,27 @@ Looking at other boards you might see some different patterns:
    needs to be done at start. If a board similar to yours does this, you probably
    want to do likewise.
 
+ - ``__naked``: All functions called before stack is correctly initialized must be
+   marked with this attribute. Otherwise, function prologue and epilogue may access
+   the uninitialized stack. If the compiler for the target architecture doesn't
+   support the attribute, stack must be set up in non-inline assembly:
+   either a barebox assembly entry point or in earlier firmware.
+   The compiler may still spill excess local C variables used in a naked function
+   to the stack before it was initialized.
+   A naked function should thus preferably only contain inline assembly, set up a
+   stack and jump directly after to a ``noinline`` non naked function where the
+   stack is then normally usable.
+
+ - ``noinline``: Compiler code inlining is oblivious to stack modification.
+   If you want to ensure a new function has its own stack frame (e.g. after setting
+   up the stack in a ``__naked`` function), you must jump to a ``__noreturn noinline``
+   function.
+
+ - ``arm_setup_stack``: For 32-bit ARM, ``arm_setup_stack`` initialized the stack
+   top when called from a naked C function, which allows to write the entry point
+   directly in C. The stack pointer will be decremented before pushing values.
+   Avoid interleaving with C-code. See ``__naked`` above for more details.
+
  - ``__dtb_z_my_board_start[];``: Because the PBL normally doesn't parse anything out
    of the device tree blob, boards can benefit from keeping the device tree blob
    compressed and only unpack it in barebox proper. Such LZO-compressed device trees
diff --git a/arch/arm/include/asm/common.h b/arch/arm/include/asm/common.h
index d03ee6273fe5..88e2991c9324 100644
--- a/arch/arm/include/asm/common.h
+++ b/arch/arm/include/asm/common.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_ARM_COMMON_H
 #define __ASM_ARM_COMMON_H
 
+#include <linux/compiler.h>
+
 static inline unsigned long get_pc(void)
 {
 	unsigned long pc;
@@ -46,8 +48,28 @@ static inline unsigned long get_sp(void)
 	return sp;
 }
 
+extern void __compiletime_error(
+	"arm_setup_stack() called outside of naked function. On ARM64, "
+	"stack should be setup in non-inline assembly before branching to C entry point."
+) __unsafe_stack_setup(void);
+
+/*
+ * Sets up new stack growing down from top within a naked C function.
+ * The first stack word will be top - sizeof(word).
+ *
+ * Avoid interleaving with C code as much as possible and and jump
+ * ASAP to a noinline function.
+ */
 static inline void arm_setup_stack(unsigned long top)
 {
+	if (IS_ENABLED(CONFIG_CPU_V8)) {
+		/*
+		 * There are no naked functions on ARM64 and thus it's never
+		 * safe to call arm_setup_stack().
+		 */
+		__unsafe_stack_setup();
+	}
+
 	__asm__ __volatile__("mov sp, %0"
 			     :
 			     : "r"(top));
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] ARM: document arm_setup_stack() pitfalls
  2021-09-16  9:37 [PATCH v2] ARM: document arm_setup_stack() pitfalls Ahmad Fatoum
@ 2021-09-16  9:46 ` Jules Maselbas
  2021-09-16 10:48 ` Uwe Kleine-König
  1 sibling, 0 replies; 3+ messages in thread
From: Jules Maselbas @ 2021-09-16  9:46 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

Thanks Ahmad !





_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] ARM: document arm_setup_stack() pitfalls
  2021-09-16  9:37 [PATCH v2] ARM: document arm_setup_stack() pitfalls Ahmad Fatoum
  2021-09-16  9:46 ` Jules Maselbas
@ 2021-09-16 10:48 ` Uwe Kleine-König
  1 sibling, 0 replies; 3+ messages in thread
From: Uwe Kleine-König @ 2021-09-16 10:48 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox, Jules Maselbas


[-- Attachment #1.1: Type: text/plain, Size: 4895 bytes --]

On Thu, Sep 16, 2021 at 11:37:53AM +0200, Ahmad Fatoum wrote:
> Many arm32 board entry points use arm_setup_stack() to set up
> the stack from C code. This necessitates using __naked, which
> probably has been our most frequent cause of misscompiled C code.
> 
> GCC is quite clear that:
> 
>   Only basic asm statements can safely be included in naked functions
>   While using extended asm or a mixture of basic asm and C code may
>   appear to work, they cannot be depended upon to work reliably and
>   are not supported.
> 
> But some boards use it anyway, because it's nice to avoid writing
> assembly. Reading generated assembly to spot compiler miscompilation
> isn't that nice though, so add some documentation, comments
> and compiler diagnostics to hopefully reduce future porting effort.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> v1 -> v2:
>   - fix commit message title
> 
> Cc: Jules Maselbas <jmaselbas@kalray.eu>
> ---
>  Documentation/devel/porting.rst | 21 +++++++++++++++++++++
>  arch/arm/include/asm/common.h   | 22 ++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/Documentation/devel/porting.rst b/Documentation/devel/porting.rst
> index 97b787327c9a..699badbec672 100644
> --- a/Documentation/devel/porting.rst
> +++ b/Documentation/devel/porting.rst
> @@ -169,6 +169,27 @@ Looking at other boards you might see some different patterns:
>     needs to be done at start. If a board similar to yours does this, you probably
>     want to do likewise.
>  
> + - ``__naked``: All functions called before stack is correctly initialized must be
> +   marked with this attribute. Otherwise, function prologue and epilogue may access
> +   the uninitialized stack. If the compiler for the target architecture doesn't
> +   support the attribute, stack must be set up in non-inline assembly:
> +   either a barebox assembly entry point or in earlier firmware.

s/either/Either/

> +   The compiler may still spill excess local C variables used in a naked function
> +   to the stack before it was initialized.
> +   A naked function should thus preferably only contain inline assembly, set up a
> +   stack and jump directly after to a ``noinline`` non naked function where the
> +   stack is then normally usable.
> +
> + - ``noinline``: Compiler code inlining is oblivious to stack modification.
> +   If you want to ensure a new function has its own stack frame (e.g. after setting
> +   up the stack in a ``__naked`` function), you must jump to a ``__noreturn noinline``
> +   function.
> +
> + - ``arm_setup_stack``: For 32-bit ARM, ``arm_setup_stack`` initialized the stack

s/initialized/initializes/

> +   top when called from a naked C function, which allows to write the entry point
> +   directly in C. The stack pointer will be decremented before pushing values.
> +   Avoid interleaving with C-code. See ``__naked`` above for more details.
> +
>   - ``__dtb_z_my_board_start[];``: Because the PBL normally doesn't parse anything out
>     of the device tree blob, boards can benefit from keeping the device tree blob
>     compressed and only unpack it in barebox proper. Such LZO-compressed device trees
> diff --git a/arch/arm/include/asm/common.h b/arch/arm/include/asm/common.h
> index d03ee6273fe5..88e2991c9324 100644
> --- a/arch/arm/include/asm/common.h
> +++ b/arch/arm/include/asm/common.h
> @@ -1,6 +1,8 @@
>  #ifndef __ASM_ARM_COMMON_H
>  #define __ASM_ARM_COMMON_H
>  
> +#include <linux/compiler.h>
> +
>  static inline unsigned long get_pc(void)
>  {
>  	unsigned long pc;
> @@ -46,8 +48,28 @@ static inline unsigned long get_sp(void)
>  	return sp;
>  }
>  
> +extern void __compiletime_error(
> +	"arm_setup_stack() called outside of naked function. On ARM64, "
> +	"stack should be setup in non-inline assembly before branching to C entry point."
> +) __unsafe_stack_setup(void);
> +
> +/*
> + * Sets up new stack growing down from top within a naked C function.
> + * The first stack word will be top - sizeof(word).
> + *
> + * Avoid interleaving with C code as much as possible and and jump

s/and and/and/

> + * ASAP to a noinline function.
> + */
>  static inline void arm_setup_stack(unsigned long top)
>  {
> +	if (IS_ENABLED(CONFIG_CPU_V8)) {

For configs that have CONFIG_CPU_V8 and CONFIG_CPU_V7 it might be legal
to call arm_setup_stack when running on a v7 SoC. Not sure how relevant
this is though, maybe just add a comment?

> +		/*
> +		 * There are no naked functions on ARM64 and thus it's never
> +		 * safe to call arm_setup_stack().
> +		 */
> +		__unsafe_stack_setup();
> +	}
> +

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-09-16 10:53 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16  9:37 [PATCH v2] ARM: document arm_setup_stack() pitfalls Ahmad Fatoum
2021-09-16  9:46 ` Jules Maselbas
2021-09-16 10:48 ` Uwe Kleine-König

mail archive of the barebox mailing list

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lore.barebox.org/barebox/0 barebox/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 barebox barebox/ https://lore.barebox.org/barebox \
		barebox@lists.infradead.org
	public-inbox-index barebox

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git