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 <[email protected]> > --- > v1 -> v2: > - fix commit message title > > Cc: Jules Maselbas <[email protected]> > --- > 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/ |
signature.asc
Description: PGP signature
_______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
