Re: [PATCH v2 1/3] [sim/riscv] Add basic semi-hosting support

Andrew Burgess
2023-12-12T17:58:01Z

<jaydeep.patil@imgtec.com> writes:

> From: Jaydeep Patil <jaydeep.patil@imgtec.com>
>
> Added support for basic semi-hosting calls OPEN, EXIT and GET_CMDLINE.
> Added gdb.arch/riscv-exit-getcmd.c to test it.
> ---
> gdb/testsuite/gdb.arch/riscv-exit-getcmd.c | 26 +++
> gdb/testsuite/gdb.arch/riscv-exit-getcmd.exp | 27 +++
> sim/riscv/riscv-sim.h | 32 +++
> sim/riscv/sim-main.c | 230 ++++++++++++++++++-
> 4 files changed, 310 insertions(+), 5 deletions(-)
> create mode 100644 gdb/testsuite/gdb.arch/riscv-exit-getcmd.c
> create mode 100644 gdb/testsuite/gdb.arch/riscv-exit-getcmd.exp
>
> diff --git a/gdb/testsuite/gdb.arch/riscv-exit-getcmd.c b/gdb/testsuite/gdb.arch/riscv-exit-getcmd.c
> new file mode 100644
> index 00000000000..02a97da8643
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/riscv-exit-getcmd.c
> @@ -0,0 +1,26 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> + Copyright 2023 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +/* Test basic semi-hosting calls SYS_GET_CMDLINE and SYS_EXIT. */
> +
> +int
> +main (int argc, char **argv)
> +{
> + if (argc != 4)
> + return 1;
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/riscv-exit-getcmd.exp b/gdb/testsuite/gdb.arch/riscv-exit-getcmd.exp
> new file mode 100644
> index 00000000000..7f5670200c2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/riscv-exit-getcmd.exp
> @@ -0,0 +1,27 @@
> +# Copyright 2023 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test basic semi-hosting calls SYS_GET_CMDLINE and SYS_EXIT.
> +
> +require {istarget "riscv*-*-*"}
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> + {debug quiet}] } {
> + return -1
> +}
> +
> +gdb_test "run 1 2 3" ".*Inferior.*process.*exited normally.*"
> diff --git a/sim/riscv/riscv-sim.h b/sim/riscv/riscv-sim.h
> index 1bc9aa12156..bb296fa6e62 100644
> --- a/sim/riscv/riscv-sim.h
> +++ b/sim/riscv/riscv-sim.h
> @@ -75,4 +75,36 @@ extern void initialize_env (SIM_DESC, const char * const *argv,
>
> #define RISCV_XLEN(cpu) MACH_WORD_BITSIZE (CPU_MACH (cpu))
>
> +#define APPLICATION_EXIT 0x20026
> +
> +#define SYS_OPEN 0x01
> +#define SYS_GET_CMDLINE 0x15
> +#define SYS_EXIT 0x18

I wonder if we should avoid the SYS_ prefix here? And instead go with
something that binds these more tightly to semi-hosting? SEMIHOST_
maybe? e.g. SEMIHOST_OPEN

> +
> +#define GDB_O_RDONLY 0x000
> +#define GDB_O_WRONLY 0x001
> +#define GDB_O_RDWR 0x002
> +#define GDB_O_APPEND 0x008
> +#define GDB_O_CREAT 0x200
> +#define GDB_O_TRUNC 0x400
> +#define GDB_O_BINARY 0

The GDB_ prefix seems weird. I haven't tried to figure this out. Is
this in some way GDB specific?

> +
> +#define SEMI_HOSTING_SLLI 0x01f01013
> +#define SEMI_HOSTING_SRAI 0x40705013
> +
> +static int gdb_open_modeflags[12] = {
> + GDB_O_RDONLY,
> + GDB_O_RDONLY | GDB_O_BINARY,
> + GDB_O_RDWR,
> + GDB_O_RDWR | GDB_O_BINARY,
> + GDB_O_WRONLY | GDB_O_CREAT | GDB_O_TRUNC,
> + GDB_O_WRONLY | GDB_O_CREAT | GDB_O_TRUNC | GDB_O_BINARY,
> + GDB_O_RDWR | GDB_O_CREAT | GDB_O_TRUNC,
> + GDB_O_RDWR | GDB_O_CREAT | GDB_O_TRUNC | GDB_O_BINARY,
> + GDB_O_WRONLY | GDB_O_CREAT | GDB_O_APPEND,
> + GDB_O_WRONLY | GDB_O_CREAT | GDB_O_APPEND | GDB_O_BINARY,
> + GDB_O_RDWR | GDB_O_CREAT | GDB_O_APPEND,
> + GDB_O_RDWR | GDB_O_CREAT | GDB_O_APPEND | GDB_O_BINARY
> +};

So, my instinct would be that all of these should be moved into
sim-main.c. That's the only place they are used, and is likely the only
place they are going to be used ... so I see no reason why they need to
live in the header file.

Additionally, I think we need to comment some/all of these defines; e.g.
APPLICATION_EXIT needs a comment, SEMI_HOSTING_{SLLI,SRAI} need
comments, and gdb_open_modeflags would need comments. You should
describe where these values come from, what they mean, where should I go
to read/understand more (e.g. name the spec or reference implementation).

> +
> #endif
> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
> index afdfcf50656..8e08eb9fc4e 100644
> --- a/sim/riscv/sim-main.c
> +++ b/sim/riscv/sim-main.c
> @@ -136,6 +136,176 @@ store_csr (SIM_CPU *cpu, const char *name, int csr, unsigned_word *reg,
> TRACE_REGISTER (cpu, "wrote CSR %s = %#" PRIxTW, name, val);
> }
>
> +/* Read 4 or 8 bytes of data from the core memory. ADDR and (INDEX * XLEN)
> + form the base address. */
> +static uintptr_t

I would have thought unsigned_8 would be a better return type. The
requirement for this function is that it return something unsigned, and
at least 8-bytes in length.

Also the comment should say that 4-byte values are zero extended.

> +get_core_data (SIM_CPU *cpu, unsigned_word addr, unsigned_word index)
> +{
> + uintptr_t param;
> + int xlen = RISCV_XLEN (cpu);
> + struct riscv_sim_cpu *riscv_cpu = RISCV_SIM_CPU (cpu);
> +
> + if (xlen == 64)
> + param = sim_core_read_unaligned_8 (cpu, riscv_cpu->pc, read_map,
> + addr + (index * 8));
> + else
> + param = sim_core_read_unaligned_4 (cpu, riscv_cpu->pc, read_map,
> + addr + (index * 4));
> +
> + return param;
> +}
> +
> +/* Write string in HOST_BUF at CORE_ADDR. The length of string is LEN. */
> +static void
> +set_core_string (SIM_CPU *cpu, unsigned_word core_addr, char *host_buf,
> + int len)
> +{
> + struct riscv_sim_cpu *riscv_cpu = RISCV_SIM_CPU (cpu);
> + for (int i = 0; i < len; i++)
> + {
> + sim_core_write_unaligned_1 (cpu, riscv_cpu->pc, write_map,
> + core_addr + i, host_buf[i]);
> + }

GNU style it to drop the { ... } around single statement blocks.

> +}
> +
> +/* Read string of length LEN at address ADDR. */
> +static char *
> +get_core_string_with_len (SIM_CPU *cpu, unsigned_word addr,
> + unsigned_word len)
> +{
> + struct riscv_sim_cpu *riscv_cpu = RISCV_SIM_CPU (cpu);
> + char * str;
> + str = (char *) malloc (len + 1);

Use xmalloc. It will handle (abort) for failure to allocate memory.

> +
> + for (int i = 0; i < len; i++)
> + {
> + str[i] = sim_core_read_unaligned_1 (cpu, riscv_cpu->pc, read_map,
> + addr + i);
> + }

Same with the single statement block.

> + str[len] = 0;

Better I think to write:

str[len] = '\0';

> +
> + return str;
> +}
> +
> +/* SYS_OPEN
> + Register a1 points to a buffer containing:
> + Index 0: Pointer: Address of the file name string.
> + Index 1: Integer: File open flags.
> + Index 2: Integer: Length of the file name string.
> + File handle is returned through register a0. */
> +static void
> +semihosting_open (SIM_CPU *cpu)
> +{
> + uintptr_t fname_addr;
> + uintptr_t flags;
> + uintptr_t fname_len;
> + char *name;
> + struct riscv_sim_cpu *riscv_cpu = RISCV_SIM_CPU (cpu);
> +
> + fname_addr = (uintptr_t) get_core_data (cpu, riscv_cpu->a1, 0);
> + flags = (uintptr_t) get_core_data (cpu, riscv_cpu->a1, 1);
> + fname_len = (uintptr_t) get_core_data (cpu, riscv_cpu->a1, 2);

These casts all seem redundant given the return type of get_core_data.

> +
> + if (fname_len <= 0)
> + {
> + riscv_cpu->a0 = -1;
> + return;
> + }

I wonder if we should place some (arbitrary) cap on fname_len. A badly
behaved application could cause the simulator to (try and) allocate 2^64
bytes of memory.

> +
> + name = get_core_string_with_len (cpu, fname_addr, fname_len);
> + riscv_cpu->a0 = sim_io_open (CPU_STATE (cpu), name,
> + gdb_open_modeflags[flags]);
> + free (name);
> +}
> +
> +/* SYS_EXIT. In case of RV32, register a1 holds the application stop reason
> + and the exit code is decided based on it. Otherwise, register a1 points to
> + a buffer containing:
> + Index 0: Integer: Application stop reason (ignored)
> + Index 1: Integer: Exit code. */
> +static void
> +semihosting_exit (SIM_CPU *cpu)
> +{
> + uintptr_t app_code, exit_code;
> + SIM_DESC sd = CPU_STATE (cpu);
> + struct riscv_sim_cpu *riscv_cpu = RISCV_SIM_CPU (cpu);
> + if (RISCV_XLEN (cpu) == 32)
> + {
> + app_code = riscv_cpu->a1;
> + if (app_code == APPLICATION_EXIT)
> + exit_code = 0;
> + else
> + exit_code = 1;
> + }
> + else
> + exit_code = (uintptr_t) get_core_data (cpu, riscv_cpu->a1, 1);
> + riscv_cpu->a0 = exit_code;
> + sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc, sim_exited, exit_code);

I wonder if we should try to marshal these calls through sim_syscall
where possible? I know we're going to need some syscall specific stubs
for each syscall as fetching the arguments is not as simple as just
reading the registers as for the ecall syscall case. But it feels like,
once we've got the arguments we should try to get back onto the "normal"
syscall handling path if possible. Thoughts?

> +}
> +
> +/* SYS_GET_CMDLINE. Write command line arguments to a buffer. Arguments
> + passed to "run" command are stored in STATE_PROG_ARGV member of SIM_DESC.
> + Register a1 points to a buffer containing:
> + Index 0: Pointer: Buffer to store command line arguments
> + Index 1: Integer: Maximum length of the buffer. */
> +static void
> +semihosting_get_cmdline (SIM_CPU *cpu)
> +{

So this one, I think, doesn't have a "normal" syscall equivalent (or
maybe I missed it?), so this is going to have to be handled separately,
but that's OK I think.

> + int i = 0, len = 0, total_len = 0;
> + uintptr_t buf_addr, max_buf_len;
> + SIM_DESC sd = CPU_STATE (cpu);
> + char *space = " ";
> + struct riscv_sim_cpu *riscv_cpu = RISCV_SIM_CPU (cpu);
> +
> + char **prog_argv = STATE_PROG_ARGV (sd);
> + if (prog_argv == NULL)
> + {
> + riscv_cpu->a0 = 1; /* Return non-zero to indicate error. */

Comments before the line of code please, not at the end of the line.

> + return;
> + }
> +
> + buf_addr = (uintptr_t) get_core_data (cpu, riscv_cpu->a1, 0);
> + max_buf_len = (uintptr_t) get_core_data (cpu, riscv_cpu->a1, 1);

Unnecessary casts again.

> +
> + while (prog_argv[i])
> + {
> + len = strlen (prog_argv[i]);
> + if ((total_len + len) > max_buf_len)
> + break;
> + set_core_string (cpu, buf_addr, prog_argv[i], len);
> + set_core_string (cpu, buf_addr + len, space, 1);
> + len++; /* Terminate it with space. */

Comment before the line of code. And I don't see how that line is
adding a space, so the comment doesn't help me understand ... it's just
a clue that I need to go and investigate this code.

> + buf_addr += len;
> + total_len += len;
> + i++;
> + }
> + riscv_cpu->a0 = 0; /* No error. */
> +}
> +
> +/* Handle semi-hosting calls. Register a0 contains semi-hosting call number
> + and a1 contains pointer to a buffer containing additional arguments. */
> +static int
> +do_semihosting (SIM_CPU *cpu)
> +{
> + struct riscv_sim_cpu *riscv_cpu = RISCV_SIM_CPU (cpu);
> + switch (riscv_cpu->a0)
> + {
> + case SYS_OPEN:
> + semihosting_open (cpu);
> + break;
> + case SYS_GET_CMDLINE:
> + semihosting_get_cmdline (cpu);
> + break;
> + case SYS_EXIT:
> + semihosting_exit (cpu);
> + break;
> + default:
> + return -1; /* Semi-hosting call not supported. */
> + }
> +
> + return 0;
> +}
> +
> static inline unsigned_word
> ashiftrt (unsigned_word val, unsigned_word shift)
> {
> @@ -623,11 +793,60 @@ execute_i (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
> TRACE_INSN (cpu, "fence.i;");
> break;
> case MATCH_EBREAK:
> - TRACE_INSN (cpu, "ebreak;");
> - /* GDB expects us to step over EBREAK. */
> - sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc + 4, sim_stopped,
> - SIM_SIGTRAP);
> - break;
> + {

I think this whole semi-hosting logic should be moved out of this switch
statement into a separate function. While reviewing I played around
with this, and ended up with this code in the switch statement:

case MATCH_EBREAK:
if (check_and_handle_riscv_semihosting (cpu, &pc))
break;

/* Not a RISC-V semihosting syscall. */
TRACE_INSN (cpu, "ebreak;");
sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc, sim_stopped,
SIM_SIGTRAP);
break;

And then check_and_handle_riscv_semihosting was declared as:

/* The instruction at *PC_PTR is assumed to be an ebreak instruction.
Check if this ebreak is surrounded by the required slli/srai
instructions that indicate this is a semihosting call.

If this is a semihosting call then extract the argument data and
perform the syscall, return 1 (true) to indicate a call was handled. In
this case, *PC_PTR is updated to point at the next instruction to
execute, this will be the instruction after the srai.

If no semihosting call was handled then return 0 (false). */

static int
check_and_handle_riscv_semihosting (SIM_CPU *cpu, sim_cia *pc_ptr)
{ ... }

The implementation was what you wrote, just moved to the function. I
think this makes it much easier, when scanning the original switch
statement, to understand what happens for an EBREAK.

> + /* If ebreak is at PC 0 then do not check for semi-hosting. */
> + if (riscv_cpu->pc != 0)
> + {
> + /* RISC-V semi-hosting call is flagged using these three
> + instructions

I think this comment should explain that the random 32-bit hex numbers
in the following are the instruction encodings. Also, I'd be very
tempted to move the SEMI_HOSTING_{SLLI,SRAI} definitions to somewhere
just after this comment. They are only needed in this function, this
comment explains exactly what's going on ... feels like the perfect
location.

> + slli zero, zero, 0x1f 0x01f01013
> + ebreak 0x00100073
> + srai zero, zero, 0x7 0x40705013
> + Register a0 holds the system call number and a1 holds the
> + pointer to parameter buffer. Do not read 4 bytes in one go
> + as we might read malformed 4 byte instruction. */

This whole "Do not read 4 bytes in one go ..." business, is this just
an optimisation. I don't see the problem with reading 4 bytes in one
go, and then checking the instruction length, and discarding if it is
wrong. It feels like the double read is more complicated than the naive
approach....

> + int iw_len;
> + sim_cia pre_pc = riscv_cpu->pc - 4;
> + unsigned_word pre_iw;
> + pre_iw = sim_core_read_aligned_2 (cpu, pre_pc, exec_map, pre_pc);
> + iw_len = riscv_insn_length (pre_iw);
> + if (iw_len == 4)
> + pre_iw |= ((unsigned_word) sim_core_read_aligned_2
> + (cpu, pre_pc, exec_map, pre_pc + 2) << 16);
> +
> + if (pre_iw == SEMI_HOSTING_SLLI)
> + {
> + sim_cia post_pc = riscv_cpu->pc + 4;
> + unsigned_word post_iw;
> + post_iw = sim_core_read_aligned_2 (cpu, post_pc, exec_map,
> + post_pc);
> + iw_len = riscv_insn_length (post_iw);
> + if (iw_len == 4)
> + post_iw |= ((unsigned_word) sim_core_read_aligned_2
> + (cpu, post_pc, exec_map, post_pc + 2) << 16);

Same double read that I don't understand the need for.

> +
> + if (post_iw == SEMI_HOSTING_SRAI)
> + {
> + TRACE_INSN (cpu, "semi-hosting a0=%lx,a1=%lx;",
> + riscv_cpu->a0, riscv_cpu->a1);
> + if (do_semihosting (cpu))
> + {
> + /* Invalid semi-hosting call. */
> + TRACE_INSN (cpu, "ebreak;");
> + sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc,
> + sim_stopped, SIM_SIGTRAP);
> + }
> + else
> + pc = pc + 4; /* post srai. */
> + break;
> + }
> + }
> + }
> + TRACE_INSN (cpu, "ebreak;");
> + sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc, sim_stopped,
> + SIM_SIGTRAP);
> + break;
> + }
> case MATCH_ECALL:
> TRACE_INSN (cpu, "ecall;");
> riscv_cpu->a0 = sim_syscall (cpu, riscv_cpu->a7, riscv_cpu->a0,
> @@ -990,6 +1209,7 @@ execute_one (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
> case INSN_CLASS_A:
> return execute_a (cpu, iw, op);
> case INSN_CLASS_I:
> + case INSN_CLASS_ZICSR:

Revert this change?

Thanks,
Andrew

> return execute_i (cpu, iw, op);
> case INSN_CLASS_M:
> case INSN_CLASS_ZMMUL:
> --
> 2.25.1