On 2/7/23 10:21, Andre Przywara wrote: > So far we used inline assembly to inject the actual instruction that > triggers the semihosting service. While this sounds elegant, as it's > really only about one instruction, it has some serious downsides: > - We need some barriers in place to force the compiler to issue writes > to a data structure before issuing the trap instruction. > - We need to convince the compiler to actually fill the structures that > we use pointers to. > - We need a memory clobber to avoid the compiler caching the data in > those structures, when semihosting writes data back. > - We need register arguments to make sure the function ID and the > pointer land in the right registers. > > This is all doable, but fragile and somewhat cumbersome. Since we now > have a separate function in an extra file anyway, we can do away with > all the magic and just write that in an actual assembly file. > This is much more readable and robust. > > Signed-off-by: Andre Przywara <andre.przyw...@arm.com> > --- > arch/arm/lib/semihosting.S | 31 +++++++++++++++++++++++++ > arch/arm/lib/semihosting.c | 47 -------------------------------------- > 2 files changed, 31 insertions(+), 47 deletions(-) > create mode 100644 arch/arm/lib/semihosting.S > delete mode 100644 arch/arm/lib/semihosting.c > > diff --git a/arch/arm/lib/semihosting.S b/arch/arm/lib/semihosting.S > new file mode 100644 > index 00000000000..393aade94a5 > --- /dev/null > +++ b/arch/arm/lib/semihosting.S > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * (C) 2022 Arm Ltd. > + */ > + > +#include <config.h> > +#include <asm/macro.h> > +#include <linux/linkage.h> > + > +.pushsection .text.smh_trap, "ax" > +/* long smh_trap(unsigned int sysnum, void *addr); */ > +ENTRY(smh_trap) > + > +#if defined(CONFIG_ARM64) > + hlt #0xf000 > +#elif defined(CONFIG_CPU_V7M) > + bkpt #0xab > +#elif defined(CONFIG_SYS_THUMB_BUILD) > + svc #0xab > +#else > + svc #0x123456 > +#endif > + > +#if defined(CONFIG_ARM64) > + ret > +#else > + bx lr > +#endif > + > +ENDPROC(smh_trap) > +.popsection > diff --git a/arch/arm/lib/semihosting.c b/arch/arm/lib/semihosting.c > deleted file mode 100644 > index 7b7669bed06..00000000000 > --- a/arch/arm/lib/semihosting.c > +++ /dev/null > @@ -1,47 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0+ > -/* > - * Copyright (C) 2022 Sean Anderson <sean.ander...@seco.com> > - * Copyright 2014 Broadcom Corporation > - */ > - > -#include <common.h> > - > -/* > - * Macro to force the compiler to *populate* memory (for an array or struct) > - * before passing the pointer to an inline assembly call. > - */ > -#define USE_PTR(ptr) *(const char (*)[]) (ptr) > - > -#if defined(CONFIG_ARM64) > - #define SMH_TRAP "hlt #0xf000" > -#elif defined(CONFIG_CPU_V7M) > - #define SMH_TRAP "bkpt #0xAB" > -#elif defined(CONFIG_SYS_THUMB_BUILD) > - #define SMH_TRAP "svc #0xab" > -#else > - #define SMH_TRAP "svc #0x123456" > -#endif > - > -/* > - * Call the handler > - */ > -long smh_trap(unsigned int sysnum, void *addr) > -{ > - register long result asm("r0"); > - register void *_addr asm("r1") = addr; > - > - /* > - * We need a memory clobber (aka compiler barrier) for two reasons: > - * - The compiler needs to populate any data structures pointed to > - * by "addr" *before* the trap instruction is called. > - * - At least the SYSREAD function puts the result into memory pointed > - * to by "addr", so the compiler must not use a cached version of > - * the previous content, after the call has finished. > - */ > - asm volatile (SMH_TRAP > - : "=r" (result) > - : "0"(sysnum), "r"(USE_PTR(_addr)) > - : "memory"); > - > - return result; > -}
Reviewed-by: Sean Anderson <sean.ander...@seco.com>