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

[Andrew Burgess <aburgess@redhat.com>]

Jaydeep Patil <Jaydeep.Patil@imgtec.com> writes:

> 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   |   6 +
> gdb/testsuite/gdb.arch/riscv-exit-getcmd.exp |  31 +++
> sim/riscv/riscv-sim.h                        |  29 +++
> sim/riscv/sim-main.c                         | 205 ++++++++++++++++++-
> 4 files changed, 266 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..2d396e530f6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/riscv-exit-getcmd.c
> @@ -0,0 +1,6 @@

New test source files should include a copyright header -- all but the
oldest files will have one you can copy.

> +
> +int main (int argc, char **argv) {

You should follow the GDB coding standard wherever possible when writing
tests, so function name should start a new line, and '{' should be on
its own line.

> +  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..672b3c4aa10
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/riscv-exit-getcmd.exp
> @@ -0,0 +1,31 @@
> +# Copyright 2020-2022 Free Software Foundation, Inc.

Unless this patch was posted to the list back in 2020, the date here
should be just '2023'.

> +#
> +# 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

Comments should be complete sentences, so end with a period.

> +
> +if {![istarget "riscv*-*-*"]} {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}

This should be replaced with:

  require {istarget "riscv*-*-*"}

> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> +                {debug quiet}] } {
> +    unsupported "failed to compile"

I believe this unsupported is not required.  prepare_for_testing should
already report if this test fails to prepare.

> +    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..c9e78a756c4 100644
> --- a/sim/riscv/riscv-sim.h
> +++ b/sim/riscv/riscv-sim.h
> @@ -75,4 +75,33 @@ extern void initialize_env (SIM_DESC, const char * const *argv,
>
> #define RISCV_XLEN(cpu) MACH_WORD_BITSIZE (CPU_MACH (cpu))
>
> +#define ApplicationExit 0x20026
> +
> +#define SYS_OPEN 0x01
> +#define SYS_GET_CMDLINE 0x15
> +#define SYS_EXIT 0x18
> +
> +#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
> +
> +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
> +};
> +
> #endif
> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
> index 250791634a1..2b184aea554 100644
> --- a/sim/riscv/sim-main.c
> +++ b/sim/riscv/sim-main.c
> @@ -136,6 +136,155 @@ store_csr (SIM_CPU *cpu, const char *name, int csr, unsigned_word *reg,
>    TRACE_REGISTER (cpu, "wrote CSR %s = %#" PRIxTW, name, val);
> }
>
> +static uintptr_t
> +get_core_data (SIM_CPU *cpu, unsigned_word addr, unsigned_word index)

All these new static functions should ideally have a header comment
explaining what they do, and what the parameters are for.

> +{
> +  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;
> +}
> +
> +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]);
> +    }
> +}
> +
> +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);
> +
> +  for (int i = 0; i < len; i++)
> +    {
> +      str[i] = sim_core_read_unaligned_1 (cpu, riscv_cpu->pc, read_map,
> +                                                                                addr + i);
> +    }
> +  str[len] = 0;
> +
> +  return str;
> +}
> +
> +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);
> +
> +  if (fname_len <= 0)
> +    {
> +      riscv_cpu->a0 = -1;
> +      return;
> +    }
> +
> +  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);
> +}
> +
> +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;
> +  else
> +    {
> +      app_code = (uintptr_t) get_core_data (cpu, riscv_cpu->a1, 0);
> +      exit_code = (uintptr_t) get_core_data (cpu, riscv_cpu->a1, 1);
> +    }
> +  if (app_code == ApplicationExit)

GNU style doesn't use camel case.  Read-only constants can be
APPLICATION_EXIT, or just 'application_exit'.

> +    exit_code = 0;
> +  else
> +    exit_code = 1;
> +  riscv_cpu->a0 = exit_code;
> +  sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc, sim_exited, exit_code);
> +}
> +
> +static void
> +semihosting_get_cmdline (SIM_CPU *cpu)
> +{
> +  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

GNU style is for /* ... */ comments, not // style.

> +      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);
> +
> +  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
> +      buf_addr += len;
> +      total_len += len;
> +      i++;
> +    }
> +  riscv_cpu->a0 = 0;       // no error
> +}
> +
> +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 +772,56 @@ 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;
> +              {
> +                /* RISC-V semi-hosting call is flagged using these three
> +                 * instructions
> +                 *  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.  */

Comments should follow GNU style; so drop all the extra '*'.

> +                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 == 0x01f01013)

At a minimum, these magic numbers needs a good comment.  But I wonder if
we can do better, maybe we can make use of MASK_* and MATCH_* macros
from include/opcode/riscv-opc.h?

> +                  {
> +                    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);

GNU style places the opening '(' onto the new line, rather than trailing
at the end of the previous line.

> +
> +                    if (post_iw == 0x40705013)
> +                              {

The indentation looks wrong here, but I'm not sure if that's from your
original patch, or just a result of how the email was sent.

Thanks,
Andrew

> +                                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 +1184,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:
>        return execute_i (cpu, iw, op);
>      case INSN_CLASS_M:
>      case INSN_CLASS_ZMMUL:
> --
> 2.25.1