Re: [PATCH 1/2] arch/riscv: add semihosting support for RISC-V

Hi Sean,

Don't know about the DCSR.EBREAK option but it will be better for us to extend the existing trap vector functionality as you mentioned. Will handle this in v2. This also removes the need for us to implement our semihosting_enabled() in inline assembly as it will become a generic lib/semihosting.c function.

On Thu, Sep 15, 2022 at 9:32 PM Sean Anderson sean.anderson@seco.com wrote:

Hi Kautuk,

I've already noted my general remarks on this approach in response to your cover letter. This just has my comments on the RISC-V-specific parts.

On 9/15/22 8:45 AM, Kautuk Consul wrote:

...

We add RISC-V semihosting based serial console for JTAG based early debugging.

The RISC-V semihosting specification is available at: https://github.com/riscv/riscv-semihosting-spec/blob/main/riscv-semihosting-...

Signed-off-by: Anup Patel apatel@ventanamicro.com Signed-off-by: Kautuk Consul kconsul@ventanamicro.com

---

arch/riscv/Kconfig | 45 ++++++++ arch/riscv/include/asm/semihosting.h | 11 ++ arch/riscv/include/asm/spl.h | 1 + arch/riscv/lib/Makefile | 7 ++ arch/riscv/lib/semihosting.c | 166 +++++++++++++++++++++++++++ arch/riscv/lib/semihosting_mmode.c | 77 +++++++++++++ arch/riscv/lib/semihosting_smode.c | 77 +++++++++++++ 7 files changed, 384 insertions(+) create mode 100644 arch/riscv/include/asm/semihosting.h create mode 100644 arch/riscv/lib/semihosting.c create mode 100644 arch/riscv/lib/semihosting_mmode.c create mode 100644 arch/riscv/lib/semihosting_smode.c

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 78e964db12..1b23d1c6c1 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -371,4 +371,49 @@ config TPL_USE_ARCH_MEMSET

endmenu

+config SEMIHOSTING

•   bool "Support RISCV semihosting"
  

•   help
  

•     Semihosting is a method for a target to communicate with a host
  

•     debugger. It uses special instructions which the debugger will trap
  

•     on and interpret. This allows U-Boot to read/write files, print to
  

•     the console, and execute arbitrary commands on the host system.
  

•     Enabling this option will add support for reading and writing files
  

•     on the host system. If you don't have a debugger attached then trying
  

•     to do this will likely cause U-Boot to hang. Say 'n' if you are unsure.
  

+config SEMIHOSTING_FALLBACK

•   bool "Recover gracefully when semihosting fails"
  

•   depends on SEMIHOSTING && RISCV
  

•   default y
  

•   help
  

•     Normally, if U-Boot makes a semihosting call and no debugger is
  

•     attached, then it will panic due to a synchronous abort
  

•     exception. This config adds an exception handler which will allow
  

•     U-Boot to recover. Say 'y' if unsure.
  

+config SPL_SEMIHOSTING

•   bool "Support RISCV semihosting in SPL"
  

•   depends on SPL
  

•   help
  

•     Semihosting is a method for a target to communicate with a host
  

•     debugger. It uses special instructions which the debugger will trap
  

•     on and interpret. This allows U-Boot to read/write files, print to
  

•     the console, and execute arbitrary commands on the host system.
  

•     Enabling this option will add support for reading and writing files
  

•     on the host system. If you don't have a debugger attached then trying
  

•     to do this will likely cause U-Boot to hang. Say 'n' if you are unsure.
  

+config SPL_SEMIHOSTING_FALLBACK

•   bool "Recover gracefully when semihosting fails in SPL"
  

•   depends on SPL_SEMIHOSTING && RISCV
  

•   default y
  

•   help
  

•     Normally, if U-Boot makes a semihosting call and no debugger is
  

•     attached, then it will panic due to a synchronous abort
  

•     exception. This config adds an exception handler which will allow
  

•     U-Boot to recover. Say 'y' if unsure.
  

endmenu diff --git a/arch/riscv/include/asm/semihosting.h b/arch/riscv/include/asm/semihosting.h new file mode 100644 index 0000000000..7042821e00 --- /dev/null +++ b/arch/riscv/include/asm/semihosting.h @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Copyright (C) 2022 Ventana Micro Systems Inc.
• */

+#ifndef __ASM_RISCV_SEMIHOSTING_H +#define __ASM_RISCV_SEMIHOSTING_H

+long smh_trap(int sysnum, void *addr);

+#endif /* __ASM_RISCV_SEMIHOSTING_H */ diff --git a/arch/riscv/include/asm/spl.h b/arch/riscv/include/asm/spl.h index e8a94fcb1f..2898a770ee 100644 --- a/arch/riscv/include/asm/spl.h +++ b/arch/riscv/include/asm/spl.h @@ -25,6 +25,7 @@ enum { BOOT_DEVICE_DFU, BOOT_DEVICE_XIP, BOOT_DEVICE_BOOTROM,

•   BOOT_DEVICE_SMH,
    BOOT_DEVICE_NONE
  

};

diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 06020fcc2a..2c89c3a2fa 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -42,3 +42,10 @@ extra-$(CONFIG_EFI) += $(EFI_CRT0) $(EFI_RELOC) obj-$(CONFIG_$(SPL_TPL_)USE_ARCH_MEMSET) += memset.o obj-$(CONFIG_$(SPL_TPL_)USE_ARCH_MEMMOVE) += memmove.o obj-$(CONFIG_$(SPL_TPL_)USE_ARCH_MEMCPY) += memcpy.o

+obj-$(CONFIG_$(SPL_TPL_)SEMIHOSTING) += semihosting.o +ifeq ($(CONFIG_$(SPL_)RISCV_MMODE),y) +obj-$(CONFIG_$(SPL_TPL_)SEMIHOSTING) += semihosting_mmode.o +else +obj-$(CONFIG_$(SPL_TPL_)SEMIHOSTING) += semihosting_smode.o +endif diff --git a/arch/riscv/lib/semihosting.c b/arch/riscv/lib/semihosting.c new file mode 100644 index 0000000000..504c9a1ddc --- /dev/null +++ b/arch/riscv/lib/semihosting.c @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2022 Ventana Micro Systems Inc.

Please retain the original copyright notices when copying files.

...

• */

+#include <common.h> +#include <log.h> +#include <semihosting.h> +#include <asm/semihosting.h>

+#define SYSOPEN 0x01 +#define SYSCLOSE 0x02 +#define SYSWRITEC 0x03 +#define SYSWRITE0 0x04 +#define SYSWRITE 0x05 +#define SYSREAD 0x06 +#define SYSREADC 0x07 +#define SYSISERROR 0x08 +#define SYSSEEK 0x0A +#define SYSFLEN 0x0C +#define SYSERRNO 0x13

+/**

• • smh_errno() - Read the host's errno
• • This gets the value of the host's errno and negates it. The host's errno may
• • or may not be set, so only call this function if a previous semihosting call
• • has failed.
• • Return: a negative error value
• */

+static int smh_errno(void) +{

•   long ret = smh_trap(SYSERRNO, NULL);
  

•   if (ret > 0 && ret < INT_MAX)
  

•           return -ret;
  

•   return -EIO;
  

+}

+long smh_open(const char *fname, enum smh_open_mode mode) +{

•   long fd;
  

•   struct smh_open_s {
  

•           const char *fname;
  

•           unsigned long mode;
  

•           size_t len;
  

•   } open;
  

•   debug("%s: file \'%s\', mode \'%u\'\n", __func__, fname, mode);
  

•   open.fname = fname;
  

•   open.len = strlen(fname);
  

•   open.mode = mode;
  

•   /* Open the file on the host */
  

•   fd = smh_trap(SYSOPEN, &open);
  

•   if (fd == -1)
  

•           return smh_errno();
  

•   return fd;
  

+}

+/**

• • struct smg_rdwr_s - Arguments for read and write
• • @fd: A file descriptor returned from smh_open()
• • @memp: Pointer to a buffer of memory of at least @len bytes
• • @len: The number of bytes to read or write
• */

+struct smh_rdwr_s {

•   long fd;
  

•   void *memp;
  

•   size_t len;
  

+};

+long smh_read(long fd, void *memp, size_t len) +{

•   long ret;
  

•   struct smh_rdwr_s read;
  

•   debug("%s: fd %ld, memp %p, len %zu\n", __func__, fd, memp, len);
  

•   read.fd = fd;
  

•   read.memp = memp;
  

•   read.len = len;
  

•   ret = smh_trap(SYSREAD, &read);
  

•   if (ret < 0)
  

•           return smh_errno();
  

•   return len - ret;
  

+}

+long smh_write(long fd, const void *memp, size_t len, ulong *written) +{

•   long ret;
  

•   struct smh_rdwr_s write;
  

•   debug("%s: fd %ld, memp %p, len %zu\n", __func__, fd, memp, len);
  

•   write.fd = fd;
  

•   write.memp = (void *)memp;
  

•   write.len = len;
  

•   ret = smh_trap(SYSWRITE, &write);
  

•   *written = len - ret;
  

•   if (ret)
  

•           return smh_errno();
  

•   return 0;
  

+}

+long smh_close(long fd) +{

•   long ret;
  

•   debug("%s: fd %ld\n", __func__, fd);
  

•   ret = smh_trap(SYSCLOSE, &fd);
  

•   if (ret == -1)
  

•           return smh_errno();
  

•   return 0;
  

+}

+long smh_flen(long fd) +{

•   long ret;
  

•   debug("%s: fd %ld\n", __func__, fd);
  

•   ret = smh_trap(SYSFLEN, &fd);
  

•   if (ret == -1)
  

•           return smh_errno();
  

•   return ret;
  

+}

+long smh_seek(long fd, long pos) +{

•   long ret;
  

•   struct smh_seek_s {
  

•           long fd;
  

•           long pos;
  

•   } seek;
  

•   debug("%s: fd %ld pos %ld\n", __func__, fd, pos);
  

•   seek.fd = fd;
  

•   seek.pos = pos;
  

•   ret = smh_trap(SYSSEEK, &seek);
  

•   if (ret)
  

•           return smh_errno();
  

•   return 0;
  

+}

+int smh_getc(void) +{

•   return smh_trap(SYSREADC, NULL);
  

+}

+void smh_putc(char ch) +{

•   smh_trap(SYSWRITEC, &ch);
  

+}

+void smh_puts(const char *s) +{

•   smh_trap(SYSWRITE0, (char *)s);
  

+} diff --git a/arch/riscv/lib/semihosting_mmode.c b/arch/riscv/lib/semihosting_mmode.c new file mode 100644 index 0000000000..7bc97a1a66 --- /dev/null +++ b/arch/riscv/lib/semihosting_mmode.c @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2022 Ventana Micro Systems Inc.
• */

+#include <common.h>

+#define SYSERRNO 0x13

Maybe we should move these defines to semihosting.h?

...

+long smh_trap(int sysnum, void *addr) +{

•   register int ret asm ("a0") = sysnum;
  

•   register void *param0 asm ("a1") = addr;
  

•   asm volatile ("       .option push\n"
  

•           "     .option norvc\n"
  

•           "     j 1f\n"
  

•           "     .align 4\n"
  

•           "     1: slli zero, zero, 0x1f\n"
  

•           "     ebreak\n"
  

•           "     srai zero, zero, 7\n"
  

•           "     .option pop\n"
  

•           : "+r" (ret) : "r" (param0) : "memory");
  

•   return ret;
  

+}

I wonder if we should just do this in a .S file. I was thinking about this for arm; I think it will be cleaner than inline assembly.

...

+#if CONFIG_IS_ENABLED(SEMIHOSTING_FALLBACK) +static bool _semihosting_enabled = true; +static bool try_semihosting = true;

+bool semihosting_enabled(void) +{

•   unsigned long tmp = 0;
  

•   register int ret asm ("a0") = SYSERRNO;
  

•   register void *param0 asm ("a1") = NULL;
  

•   if (!try_semihosting)
  

•           return _semihosting_enabled;
  

•   asm volatile ("       .option push\n"
  

•           "     .option norvc\n"
  

•           "     j _semihost_test_vector_next\n"
  

•           "     .align 4\n"
  

•           "     _semihost_test_vector:\n"
  

•           "             csrr %[en], mepc\n"
  

•           "             addi %[en], %[en], 4\n"
  

•           "             csrw mepc, %[en]\n"
  

•           "             add %[en], zero, zero\n"
  

•           "             mret\n"
  

•           "     _semihost_test_vector_next:\n"
  

•           "     la %[tmp], _semihost_test_vector\n"
  

•           "     csrrw %[tmp], mtvec, %[tmp]\n"
  

•           "     j 1f\n"
  

•           "     .align 4\n"
  

•           "     1: slli zero, zero, 0x1f\n"
  

•           "     ebreak\n"
  

•           "     srai zero, zero, 7\n"
  

•           "     csrw mtvec, %[tmp]\n"
  

•           "     .option pop\n"
  

•           : [tmp] "+r" (tmp), [en] "+r" (_semihosting_enabled),
  

•             [ret] "+r" (ret)
  

•           : "r" (param0) : "memory");
  

Can you comment on why you chose this approach (setting a new trap vector when testing for semihosting) as opposed to the approach used on arm (extending the existing trap vector) or reading DCSR.EBREAKx? The advantage of extending the existing trap vector is that the user can disconnect the debugger and U-Boot will continue running (albeit perhaps in a state of reduced functionality). There is also never a window where we can crash with no output (since the original trap handler remains in place). The disadvantage is that we have to add some extra code so we are very sure that we have a semihosting halt (instead of some other illegal instruction).

The other option is to just read DCSR.EBREAKx directly [1]. I think this could be the cleanest approach, since we can add a check in smh_trap, avoiding the ebreak completely if there's no debugger attached. I'm not sure if this csr is readable by regular code (maybe you have to be in D-mode). There's also a small race condition if the debugger is detached in between when we check DCSR.EBREAKx and when we execute ebreak.

[1] https://tomverbeure.github.io/2021/12/30/Semihosting-on-RISCV.html#when-does...

...

•   try_semihosting = false;
  

•   return _semihosting_enabled;
  

+}

+void disable_semihosting(void) +{

•   _semihosting_enabled = false;
  

+} +#endif diff --git a/arch/riscv/lib/semihosting_smode.c b/arch/riscv/lib/semihosting_smode.c new file mode 100644 index 0000000000..d7ce07985b --- /dev/null +++ b/arch/riscv/lib/semihosting_smode.c @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2022 Ventana Micro Systems Inc.
• */

+#include <common.h>

+#define SYSERRNO 0x13

+long smh_trap(int sysnum, void *addr) +{

•   register int ret asm ("a0") = sysnum;
  

•   register void *param0 asm ("a1") = addr;
  

•   asm volatile ("       .option push\n"
  

•           "     .option norvc\n"
  

•           "     j 1f\n"
  

•           "     .align 4\n"
  

•           "     1: slli zero, zero, 0x1f\n"
  

•           "     ebreak\n"
  

•           "     srai zero, zero, 7\n"
  

•           "     .option pop\n"
  

•           : "+r" (ret) : "r" (param0) : "memory");
  

•   return ret;
  

+}

This looks identical to the M-mode version.

...

+#if CONFIG_IS_ENABLED(SEMIHOSTING_FALLBACK) +static bool _semihosting_enabled = true; +static bool try_semihosting = true;

+bool semihosting_enabled(void) +{

•   unsigned long tmp = 0;
  

•   register int ret asm ("a0") = SYSERRNO;
  

•   register void *param0 asm ("a1") = NULL;
  

•   if (!try_semihosting)
  

•           return _semihosting_enabled;
  

•   asm volatile ("       .option push\n"
  

•           "     .option norvc\n"
  

•           "     j _semihost_test_vector_next\n"
  

•           "     .align 4\n"
  

•           "     _semihost_test_vector:\n"
  

•           "             csrr %[en], sepc\n"
  

•           "             addi %[en], %[en], 4\n"
  

•           "             csrw sepc, %[en]\n"
  

•           "             add %[en], zero, zero\n"
  

•           "             sret\n"
  

As does this, with the exception of the csrs and rets. I think we have some macros to abstract this sort of thing away.

...

•           "     _semihost_test_vector_next:\n"
  

•           "     la %[tmp], _semihost_test_vector\n"
  

•           "     csrrw %[tmp], stvec, %[tmp]\n"
  

•           "     j 1f\n"
  

•           "     .align 4\n"
  

•           "     1: slli zero, zero, 0x1f\n"
  

•           "     ebreak\n"
  

•           "     srai zero, zero, 7\n"
  

•           "     csrw stvec, %[tmp]\n"
  

•           "     .option pop\n"
  

•           : [tmp] "+r" (tmp), [en] "+r" (_semihosting_enabled),
  

•             [ret] "+r" (ret)
  

•           : "r" (param0) : "memory");
  

•   try_semihosting = false;
  

•   return _semihosting_enabled;
  

+}

+void disable_semihosting(void) +{

•   _semihosting_enabled = false;
  

+}

+#endif

2.34.1

--Sean