Re: [PATCH v2] ARM: document arm_setup_stack() pitfalls

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- 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