Re: [Qemu-riscv] [Qemu-devel] [PATCH 4/5 v2] RISC-V: Add debug support for accessing CSRs.

[Alistair Francis]

On Fri, Dec 28, 2018 at 2:23 PM Jim Wilson <address@hidden> wrote:
>
> Adds a debugger parameter to csr_read_helper and csr_write_helper.  When
> this is true, we disable illegal instruction checks.
>
> Signed-off-by: Jim Wilson <address@hidden>

Reviewed-by: Alistair Francis <address@hidden>

Alistair

> ---
>  linux-user/riscv/signal.c |  5 ++-
>  target/riscv/cpu.h        |  7 +++-
>  target/riscv/cpu_helper.c |  4 +-
>  target/riscv/gdbstub.c    |  4 +-
>  target/riscv/op_helper.c  | 93 
> ++++++++++++++++++++++++++++++++---------------
>  5 files changed, 76 insertions(+), 37 deletions(-)
>
> diff --git a/linux-user/riscv/signal.c b/linux-user/riscv/signal.c
> index f598d41..ed76f23 100644
> --- a/linux-user/riscv/signal.c
> +++ b/linux-user/riscv/signal.c
> @@ -83,7 +83,8 @@ static void setup_sigcontext(struct target_sigcontext *sc, 
> CPURISCVState *env)
>          __put_user(env->fpr[i], &sc->fpr[i]);
>      }
>
> -    uint32_t fcsr = csr_read_helper(env, CSR_FCSR); /*riscv_get_fcsr(env);*/
> +    /*riscv_get_fcsr(env);*/
> +    uint32_t fcsr = csr_read_helper(env, CSR_FCSR, false);
>      __put_user(fcsr, &sc->fcsr);
>  }
>
> @@ -159,7 +160,7 @@ static void restore_sigcontext(CPURISCVState *env, struct 
> target_sigcontext *sc)
>
>      uint32_t fcsr;
>      __get_user(fcsr, &sc->fcsr);
> -    csr_write_helper(env, fcsr, CSR_FCSR);
> +    csr_write_helper(env, fcsr, CSR_FCSR, false);
>  }
>
>  static void restore_ucontext(CPURISCVState *env, struct target_ucontext *uc)
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 4ee09b9..29361ca 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -290,8 +290,11 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState 
> *env, target_ulong *pc,
>  }
>
>  void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
> -        target_ulong csrno);
> -target_ulong csr_read_helper(CPURISCVState *env, target_ulong csrno);
> +                      target_ulong csrno, bool debugger);
> +target_ulong csr_read_helper(CPURISCVState *env, target_ulong csrno,
> +                             bool debugger);
> +
> +void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
>
>  #include "exec/cpu-all.h"
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 86f9f47..1abad94 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -526,7 +526,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>              get_field(s, MSTATUS_SIE) : get_field(s, MSTATUS_UIE << 
> env->priv));
>          s = set_field(s, MSTATUS_SPP, env->priv);
>          s = set_field(s, MSTATUS_SIE, 0);
> -        csr_write_helper(env, s, CSR_MSTATUS);
> +        csr_write_helper(env, s, CSR_MSTATUS, false);
>          riscv_set_mode(env, PRV_S);
>      } else {
>          /* No need to check MTVEC for misaligned - lower 2 bits cannot be 
> set */
> @@ -551,7 +551,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>              get_field(s, MSTATUS_MIE) : get_field(s, MSTATUS_UIE << 
> env->priv));
>          s = set_field(s, MSTATUS_MPP, env->priv);
>          s = set_field(s, MSTATUS_MIE, 0);
> -        csr_write_helper(env, s, CSR_MSTATUS);
> +        csr_write_helper(env, s, CSR_MSTATUS, false);
>          riscv_set_mode(env, PRV_M);
>      }
>      /* TODO yield load reservation  */
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 71c3eb1..b06f0fa 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -34,7 +34,7 @@ int riscv_cpu_gdb_read_register(CPUState *cs, uint8_t 
> *mem_buf, int n)
>      } else if (n < 65) {
>          return gdb_get_reg64(mem_buf, env->fpr[n - 33]);
>      } else if (n < 4096 + 65) {
> -        return gdb_get_regl(mem_buf, csr_read_helper(env, n - 65));
> +        return gdb_get_regl(mem_buf, csr_read_helper(env, n - 65, true));
>      }
>      return 0;
>  }
> @@ -57,7 +57,7 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t 
> *mem_buf, int n)
>          env->fpr[n - 33] = ldq_p(mem_buf); /* always 64-bit */
>          return sizeof(uint64_t);
>      } else if (n < 4096 + 65) {
> -        csr_write_helper(env, ldtul_p(mem_buf), n - 65);
> +        csr_write_helper(env, ldtul_p(mem_buf), n - 65, true);
>      }
>      return 0;
>  }
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 3726299..3c5641e 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -87,7 +87,7 @@ static void validate_mstatus_fs(CPURISCVState *env, 
> uintptr_t ra)
>   * Adapted from Spike's processor_t::set_csr
>   */
>  void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
> -        target_ulong csrno)
> +                      target_ulong csrno, bool debugger)
>  {
>  #ifndef CONFIG_USER_ONLY
>      uint64_t delegable_ints = MIP_SSIP | MIP_STIP | MIP_SEIP;
> @@ -96,15 +96,21 @@ void csr_write_helper(CPURISCVState *env, target_ulong 
> val_to_write,
>
>      switch (csrno) {
>      case CSR_FFLAGS:
> -        validate_mstatus_fs(env, GETPC());
> +        if (!debugger) {
> +            validate_mstatus_fs(env, GETPC());
> +        }
>          cpu_riscv_set_fflags(env, val_to_write & (FSR_AEXC >> 
> FSR_AEXC_SHIFT));
>          break;
>      case CSR_FRM:
> -        validate_mstatus_fs(env, GETPC());
> +        if (!debugger) {
> +            validate_mstatus_fs(env, GETPC());
> +        }
>          env->frm = val_to_write & (FSR_RD >> FSR_RD_SHIFT);
>          break;
>      case CSR_FCSR:
> -        validate_mstatus_fs(env, GETPC());
> +        if (!debugger) {
> +            validate_mstatus_fs(env, GETPC());
> +        }
>          env->frm = (val_to_write & FSR_RD) >> FSR_RD_SHIFT;
>          cpu_riscv_set_fflags(env, (val_to_write & FSR_AEXC) >> 
> FSR_AEXC_SHIFT);
>          break;
> @@ -169,7 +175,9 @@ void csr_write_helper(CPURISCVState *env, target_ulong 
> val_to_write,
>           * CLINT, no additional locking is needed for read-modifiy-write
>           * CSR operations
>           */
> -        qemu_mutex_lock_iothread();
> +        if (!debugger) {
> +            qemu_mutex_lock_iothread();
> +        }
>          RISCVCPU *cpu = riscv_env_get_cpu(env);
>          riscv_cpu_update_mip(cpu, MIP_SSIP | MIP_STIP,
>                                    (val_to_write & (MIP_SSIP | MIP_STIP)));
> @@ -177,7 +185,9 @@ void csr_write_helper(CPURISCVState *env, target_ulong 
> val_to_write,
>           * csrs, csrc on mip.SEIP is not decomposable into separate read and
>           * write steps, so a different implementation is needed
>           */
> -        qemu_mutex_unlock_iothread();
> +        if (!debugger) {
> +            qemu_mutex_unlock_iothread();
> +        }
>          break;
>      }
>      case CSR_MIE: {
> @@ -247,21 +257,25 @@ void csr_write_helper(CPURISCVState *env, target_ulong 
> val_to_write,
>              mask |= SSTATUS_MXR;
>          }
>          ms = (ms & ~mask) | (val_to_write & mask);
> -        csr_write_helper(env, ms, CSR_MSTATUS);
> +        csr_write_helper(env, ms, CSR_MSTATUS, debugger);
>          break;
>      }
>      case CSR_SIP: {
> -        qemu_mutex_lock_iothread();
> +        if (!debugger) {
> +            qemu_mutex_lock_iothread();
> +        }
>          target_ulong next_mip = (env->mip & ~env->mideleg)
>                                  | (val_to_write & env->mideleg);
> -        qemu_mutex_unlock_iothread();
> -        csr_write_helper(env, next_mip, CSR_MIP);
> +        if (!debugger) {
> +            qemu_mutex_unlock_iothread();
> +        }
> +        csr_write_helper(env, next_mip, CSR_MIP, debugger);
>          break;
>      }
>      case CSR_SIE: {
>          target_ulong next_mie = (env->mie & ~env->mideleg)
>                                  | (val_to_write & env->mideleg);
> -        csr_write_helper(env, next_mie, CSR_MIE);
> +        csr_write_helper(env, next_mie, CSR_MIE, debugger);
>          break;
>      }
>      case CSR_SATP: /* CSR_SPTBR */ {
> @@ -371,7 +385,9 @@ void csr_write_helper(CPURISCVState *env, target_ulong 
> val_to_write,
>      do_illegal:
>  #endif
>      default:
> -        do_raise_exception_err(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> +        if (!debugger) {
> +            do_raise_exception_err(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> +        }
>      }
>  }
>
> @@ -380,7 +396,8 @@ void csr_write_helper(CPURISCVState *env, target_ulong 
> val_to_write,
>   *
>   * Adapted from Spike's processor_t::get_csr
>   */
> -target_ulong csr_read_helper(CPURISCVState *env, target_ulong csrno)
> +target_ulong csr_read_helper(CPURISCVState *env, target_ulong csrno,
> +                             bool debugger)
>  {
>  #ifndef CONFIG_USER_ONLY
>      target_ulong ctr_en = env->priv == PRV_U ? env->scounteren :
> @@ -416,13 +433,19 @@ target_ulong csr_read_helper(CPURISCVState *env, 
> target_ulong csrno)
>
>      switch (csrno) {
>      case CSR_FFLAGS:
> -        validate_mstatus_fs(env, GETPC());
> +        if (!debugger) {
> +            validate_mstatus_fs(env, GETPC());
> +        }
>          return cpu_riscv_get_fflags(env);
>      case CSR_FRM:
> -        validate_mstatus_fs(env, GETPC());
> +        if (!debugger) {
> +            validate_mstatus_fs(env, GETPC());
> +        }
>          return env->frm;
>      case CSR_FCSR:
> -        validate_mstatus_fs(env, GETPC());
> +        if (!debugger) {
> +            validate_mstatus_fs(env, GETPC());
> +        }
>          return (cpu_riscv_get_fflags(env) << FSR_AEXC_SHIFT)
>                  | (env->frm << FSR_RD_SHIFT);
>      /* rdtime/rdtimeh is trapped and emulated by bbl in system mode */
> @@ -504,9 +527,13 @@ target_ulong csr_read_helper(CPURISCVState *env, 
> target_ulong csrno)
>          return env->mstatus & mask;
>      }
>      case CSR_SIP: {
> -        qemu_mutex_lock_iothread();
> +        if (!debugger) {
> +            qemu_mutex_lock_iothread();
> +        }
>          target_ulong tmp = env->mip & env->mideleg;
> -        qemu_mutex_unlock_iothread();
> +        if (!debugger) {
> +            qemu_mutex_unlock_iothread();
> +        }
>          return tmp;
>      }
>      case CSR_SIE:
> @@ -539,9 +566,13 @@ target_ulong csr_read_helper(CPURISCVState *env, 
> target_ulong csrno)
>      case CSR_MSTATUS:
>          return env->mstatus;
>      case CSR_MIP: {
> -        qemu_mutex_lock_iothread();
> +        if (!debugger) {
> +            qemu_mutex_lock_iothread();
> +        }
>          target_ulong tmp = env->mip;
> -        qemu_mutex_unlock_iothread();
> +        if (!debugger) {
> +            qemu_mutex_unlock_iothread();
> +        }
>          return tmp;
>      }
>      case CSR_MIE:
> @@ -601,7 +632,11 @@ target_ulong csr_read_helper(CPURISCVState *env, 
> target_ulong csrno)
>  #endif
>      }
>      /* used by e.g. MTIME read */
> -    do_raise_exception_err(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> +    if (!debugger) {
> +        do_raise_exception_err(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> +    } else {
> +        return 0;
> +    }
>  }
>
>  /*
> @@ -625,8 +660,8 @@ target_ulong helper_csrrw(CPURISCVState *env, 
> target_ulong src,
>          target_ulong csr)
>  {
>      validate_csr(env, csr, 1, GETPC());
> -    uint64_t csr_backup = csr_read_helper(env, csr);
> -    csr_write_helper(env, src, csr);
> +    uint64_t csr_backup = csr_read_helper(env, csr, false);
> +    csr_write_helper(env, src, csr, false);
>      return csr_backup;
>  }
>
> @@ -634,9 +669,9 @@ target_ulong helper_csrrs(CPURISCVState *env, 
> target_ulong src,
>          target_ulong csr, target_ulong rs1_pass)
>  {
>      validate_csr(env, csr, rs1_pass != 0, GETPC());
> -    uint64_t csr_backup = csr_read_helper(env, csr);
> +    uint64_t csr_backup = csr_read_helper(env, csr, false);
>      if (rs1_pass != 0) {
> -        csr_write_helper(env, src | csr_backup, csr);
> +        csr_write_helper(env, src | csr_backup, csr, false);
>      }
>      return csr_backup;
>  }
> @@ -645,9 +680,9 @@ target_ulong helper_csrrc(CPURISCVState *env, 
> target_ulong src,
>          target_ulong csr, target_ulong rs1_pass)
>  {
>      validate_csr(env, csr, rs1_pass != 0, GETPC());
> -    uint64_t csr_backup = csr_read_helper(env, csr);
> +    uint64_t csr_backup = csr_read_helper(env, csr, false);
>      if (rs1_pass != 0) {
> -        csr_write_helper(env, (~src) & csr_backup, csr);
> +        csr_write_helper(env, (~src) & csr_backup, csr, false);
>      }
>      return csr_backup;
>  }
> @@ -674,7 +709,7 @@ target_ulong helper_sret(CPURISCVState *env, target_ulong 
> cpu_pc_deb)
>      mstatus = set_field(mstatus, MSTATUS_SPIE, 0);
>      mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
>      riscv_set_mode(env, prev_priv);
> -    csr_write_helper(env, mstatus, CSR_MSTATUS);
> +    csr_write_helper(env, mstatus, CSR_MSTATUS, false);
>
>      return retpc;
>  }
> @@ -699,7 +734,7 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong 
> cpu_pc_deb)
>      mstatus = set_field(mstatus, MSTATUS_MPIE, 0);
>      mstatus = set_field(mstatus, MSTATUS_MPP, PRV_U);
>      riscv_set_mode(env, prev_priv);
> -    csr_write_helper(env, mstatus, CSR_MSTATUS);
> +    csr_write_helper(env, mstatus, CSR_MSTATUS, false);
>
>      return retpc;
>  }
> --
> 2.7.4
>
>