[PATCH 1/2] arm: semihosting: replace inline assembly with assembly file

[Sean Anderson]

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.przywara at 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.anderson at 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.anderson at seco.com>