[PATCH 0/2] semihosting: use assembly conduit functions

[PATCH 0/2] semihosting: use assembly conduit functions

[Andre Przywara]

Hi,

to trigger the actual semihosting action in the debugger, we used some
carefully constructed inline assembly sequence. This was motivated by
the trigger being really just a single instruction, so originally this
could be neatly inlined by the compiler.
However we now have a separate function anyway, so inlining is no longer
happening. On top of that the inline assembly was really fragile and
hard to read.

To clean that up, just use actual assembly functions, which does away
with all the tricks to force the compiler into submission.
Patch 1 is for ARM, patch 2 for RISC-V.
Briefly tested on arm and aarch64 QEMU, and on an ARMv8 fastmodel, plus
compile-tested for RISC-V. Please test it on your system!

Cheers,
Andre

P.S. This idea came up before, but I don't remember if patches were
floating around already. If there were, apologies for my ignorance, and
I will be all too happy to use those patches instead of mine here, just
point me to them.

Andre Przywara (2):
  arm: semihosting: replace inline assembly with assembly file
  riscv: semihosting: replace inline assembly with assembly file

 arch/arm/lib/semihosting.S   | 31 ++++++++++++++++++++++++
 arch/arm/lib/semihosting.c   | 47 ------------------------------------
 arch/riscv/lib/semihosting.S | 22 +++++++++++++++++
 arch/riscv/lib/semihosting.c | 24 ------------------
 4 files changed, 53 insertions(+), 71 deletions(-)
 create mode 100644 arch/arm/lib/semihosting.S
 delete mode 100644 arch/arm/lib/semihosting.c
 create mode 100644 arch/riscv/lib/semihosting.S
 delete mode 100644 arch/riscv/lib/semihosting.c

-- 
2.25.1

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

[Andre Przywara]

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@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@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;
-}
-- 
2.25.1

Re: [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@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@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@seco.com>

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

On Tue, Feb 07, 2023 at 03:21:04PM +0000, 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@arm.com>
> Reviewed-by: Sean Anderson <sean.anderson@seco.com>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Andre Przywara]

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 a few instructions, 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 assembler.
This is much more readable and robust.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/riscv/lib/semihosting.S | 22 ++++++++++++++++++++++
 arch/riscv/lib/semihosting.c | 24 ------------------------
 2 files changed, 22 insertions(+), 24 deletions(-)
 create mode 100644 arch/riscv/lib/semihosting.S
 delete mode 100644 arch/riscv/lib/semihosting.c

diff --git a/arch/riscv/lib/semihosting.S b/arch/riscv/lib/semihosting.S
new file mode 100644
index 00000000000..5fff485b875
--- /dev/null
+++ b/arch/riscv/lib/semihosting.S
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2022 Ventana Micro Systems Inc.
+ */
+
+#include <asm/asm.h>
+#include <linux/linkage.h>
+
+.pushsection .text.smh_trap, "ax"
+ENTRY(smh_trap)
+	.align	2
+	.option	push
+	.option	norvc		/* semihosting sequence must be 32-bit wide */
+
+	slli zero, zero, 0x1f	/* Entry NOP to identify semihosting */
+	ebreak
+	srai zero, zero, 7	/* NOP encoding of semihosting call number */
+	.option pop
+
+	ret
+ENDPROC(smh_trap)
+.popsection
diff --git a/arch/riscv/lib/semihosting.c b/arch/riscv/lib/semihosting.c
deleted file mode 100644
index d6593b02a6f..00000000000
--- a/arch/riscv/lib/semihosting.c
+++ /dev/null
@@ -1,24 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * Copyright (C) 2022 Ventana Micro Systems Inc.
- */
-
-#include <common.h>
-
-long smh_trap(int sysnum, void *addr)
-{
-	register int ret asm ("a0") = sysnum;
-	register void *param0 asm ("a1") = addr;
-
-	asm volatile (".align 4\n"
-		".option push\n"
-		".option norvc\n"
-
-		"slli zero, zero, 0x1f\n"
-		"ebreak\n"
-		"srai zero, zero, 7\n"
-		".option pop\n"
-		: "+r" (ret) : "r" (param0) : "memory");
-
-	return ret;
-}
-- 
2.25.1

Re: [PATCH 2/2] riscv: 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 a few instructions, 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 assembler.
> This is much more readable and robust.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/riscv/lib/semihosting.S | 22 ++++++++++++++++++++++
>  arch/riscv/lib/semihosting.c | 24 ------------------------
>  2 files changed, 22 insertions(+), 24 deletions(-)
>  create mode 100644 arch/riscv/lib/semihosting.S
>  delete mode 100644 arch/riscv/lib/semihosting.c
> 
> diff --git a/arch/riscv/lib/semihosting.S b/arch/riscv/lib/semihosting.S
> new file mode 100644
> index 00000000000..5fff485b875
> --- /dev/null
> +++ b/arch/riscv/lib/semihosting.S
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2022 Ventana Micro Systems Inc.
> + */
> +
> +#include <asm/asm.h>
> +#include <linux/linkage.h>
> +
> +.pushsection .text.smh_trap, "ax"
> +ENTRY(smh_trap)
> +	.align	2
> +	.option	push
> +	.option	norvc		/* semihosting sequence must be 32-bit wide */
> +
> +	slli zero, zero, 0x1f	/* Entry NOP to identify semihosting */
> +	ebreak
> +	srai zero, zero, 7	/* NOP encoding of semihosting call number */
> +	.option pop
> +
> +	ret
> +ENDPROC(smh_trap)
> +.popsection
> diff --git a/arch/riscv/lib/semihosting.c b/arch/riscv/lib/semihosting.c
> deleted file mode 100644
> index d6593b02a6f..00000000000
> --- a/arch/riscv/lib/semihosting.c
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0+
> -/*
> - * Copyright (C) 2022 Ventana Micro Systems Inc.
> - */
> -
> -#include <common.h>
> -
> -long smh_trap(int sysnum, void *addr)
> -{
> -	register int ret asm ("a0") = sysnum;
> -	register void *param0 asm ("a1") = addr;
> -
> -	asm volatile (".align 4\n"
> -		".option push\n"
> -		".option norvc\n"
> -
> -		"slli zero, zero, 0x1f\n"
> -		"ebreak\n"
> -		"srai zero, zero, 7\n"
> -		".option pop\n"
> -		: "+r" (ret) : "r" (param0) : "memory");
> -
> -	return ret;
> -}

Reviewed-by: Sean Anderson <sean.anderson@seco.com>

Re: [PATCH 2/2] riscv: semihosting: replace inline assembly with assembly file

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

On Tue, Feb 07, 2023 at 03:21:05PM +0000, 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 a few instructions, 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 assembler.
> This is much more readable and robust.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Sean Anderson <sean.anderson@seco.com>

After correcting the style on the SPDX header of the new .S file,
applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/2] semihosting: use assembly conduit functions

[Sean Anderson]

On 2/7/23 10:21, Andre Przywara wrote:
> Hi,
> 
> to trigger the actual semihosting action in the debugger, we used some
> carefully constructed inline assembly sequence. This was motivated by
> the trigger being really just a single instruction, so originally this
> could be neatly inlined by the compiler.
> However we now have a separate function anyway, so inlining is no longer
> happening. On top of that the inline assembly was really fragile and
> hard to read.
> 
> To clean that up, just use actual assembly functions, which does away
> with all the tricks to force the compiler into submission.
> Patch 1 is for ARM, patch 2 for RISC-V.
> Briefly tested on arm and aarch64 QEMU, and on an ARMv8 fastmodel, plus
> compile-tested for RISC-V. Please test it on your system!
> 
> Cheers,
> Andre
> 
> P.S. This idea came up before, but I don't remember if patches were
> floating around already. If there were, apologies for my ignorance, and
> I will be all too happy to use those patches instead of mine here, just
> point me to them.
> 
> Andre Przywara (2):
>   arm: semihosting: replace inline assembly with assembly file
>   riscv: semihosting: replace inline assembly with assembly file
> 
>  arch/arm/lib/semihosting.S   | 31 ++++++++++++++++++++++++
>  arch/arm/lib/semihosting.c   | 47 ------------------------------------
>  arch/riscv/lib/semihosting.S | 22 +++++++++++++++++
>  arch/riscv/lib/semihosting.c | 24 ------------------
>  4 files changed, 53 insertions(+), 71 deletions(-)
>  create mode 100644 arch/arm/lib/semihosting.S
>  delete mode 100644 arch/arm/lib/semihosting.c
>  create mode 100644 arch/riscv/lib/semihosting.S
>  delete mode 100644 arch/riscv/lib/semihosting.c
> 

I would like to test this, but I don't think I'll have time for a bit.

--Sean