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@xxxxxxxxxxxxxx>
> ---
> v1 -> v2:
> - fix commit message title
>
> Cc: Jules Maselbas <jmaselbas@xxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
_______________________________________________ barebox mailing list barebox@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox
