Re: [PATCH 3/4] target/riscv: Set [m|s]tval for both illegal and virtual instruction traps

2022-05-09 Thread Alistair Francis 
On Fri, Apr 29, 2022 at 5:36 AM Anup Patel  wrote:>
> Currently, the [m|s]tval CSRs are set with trapping instruction encoding
> only for illegal instruction traps taken at the time of instruction
> decoding.
>
> In RISC-V world, a valid instructions might also trap as illegal or
> virtual instruction based to trapping bits in various CSRs (such as
> mstatus.TVM or hstatus.VTVM).
>
> We improve setting of [m|s]tval CSRs for all types of illegal and
> virtual instruction traps.
>
> Signed-off-by: Anup Patel 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c|  2 ++
>  target/riscv/cpu.h|  8 +++-
>  target/riscv/cpu_helper.c |  1 +
>  target/riscv/translate.c  | 17 +
>  4 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index dff4606585..f0a702fee6 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -406,6 +406,7 @@ void restore_state_to_opc(CPURISCVState *env, 
> TranslationBlock *tb,
>  } else {
>  env->pc = data[0];
>  }
> +env->bins = data[1];
>  }
>
>  static void riscv_cpu_reset(DeviceState *dev)
> @@ -445,6 +446,7 @@ static void riscv_cpu_reset(DeviceState *dev)
>  env->mcause = 0;
>  env->miclaim = MIP_SGEIP;
>  env->pc = env->resetvec;
> +env->bins = 0;
>  env->two_stage_lookup = false;
>
>  /* Initialized default priorities of local interrupts. */
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index fe6c9a2c92..a55c918274 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -30,6 +30,12 @@
>
>  #define TCG_GUEST_DEFAULT_MO 0
>
> +/*
> + * RISC-V-specific extra insn start words:
> + * 1: Original instruction opcode
> + */
> +#define TARGET_INSN_START_EXTRA_WORDS 1
> +
>  #define TYPE_RISCV_CPU "riscv-cpu"
>
>  #define RISCV_CPU_TYPE_SUFFIX "-" TYPE_RISCV_CPU
> @@ -140,7 +146,7 @@ struct CPUArchState {
>  target_ulong frm;
>
>  target_ulong badaddr;
> -uint32_t bins;
> +target_ulong bins;
>
>  target_ulong guest_phys_fault_addr;
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d83579accf..bba4fce777 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1371,6 +1371,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>  tval = env->badaddr;
>  break;
>  case RISCV_EXCP_ILLEGAL_INST:
> +case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
>  tval = env->bins;
>  break;
>  default:
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 0cd1d9ee94..55a4713af2 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -107,6 +107,8 @@ typedef struct DisasContext {
>  /* PointerMasking extension */
>  bool pm_mask_enabled;
>  bool pm_base_enabled;
> +/* TCG of the current insn_start */
> +TCGOp *insn_start;
>  } DisasContext;
>
>  static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> @@ -236,9 +238,6 @@ static void generate_exception_mtval(DisasContext *ctx, 
> int excp)
>
>  static void gen_exception_illegal(DisasContext *ctx)
>  {
> -tcg_gen_st_i32(tcg_constant_i32(ctx->opcode), cpu_env,
> -   offsetof(CPURISCVState, bins));
> -
>  generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
>  }
>
> @@ -1017,6 +1016,13 @@ static uint32_t opcode_at(DisasContextBase *dcbase, 
> target_ulong pc)
>  /* Include decoders for factored-out extensions */
>  #include "decode-XVentanaCondOps.c.inc"
>
> +static inline void decode_save_opc(DisasContext *ctx, target_ulong opc)
> +{
> +assert(ctx->insn_start != NULL);
> +tcg_set_insn_start_param(ctx->insn_start, 1, opc);
> +ctx->insn_start = NULL;
> +}
> +
>  static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t 
> opcode)
>  {
>  /*
> @@ -1033,6 +1039,7 @@ static void decode_opc(CPURISCVState *env, DisasContext 
> *ctx, uint16_t opcode)
>
>  /* Check for compressed insn */
>  if (extract16(opcode, 0, 2) != 3) {
> +decode_save_opc(ctx, opcode);
>  if (!has_ext(ctx, RVC)) {
>  gen_exception_illegal(ctx);
>  } else {
> @@ -1047,6 +1054,7 @@ static void decode_opc(CPURISCVState *env, DisasContext 
> *ctx, uint16_t opcode)
>  opcode32 = deposit32(opcode32, 16, 16,
>   translator_lduw(env, >base,
>   ctx->base.pc_next + 2));
> +decode_save_opc(ctx, opcode32);
>  ctx->opcode = opcode32;
>  ctx->pc_succ_insn = ctx->base.pc_next + 4;
>
> @@ -1113,7 +1121,8 @@ static void riscv_tr_insn_start(DisasContextBase 
> *dcbase, CPUState *cpu)
>  {
>  DisasContext *ctx = container_of(dcbase, DisasContext, base);
>
> -tcg_gen_insn_start(ctx->base.pc_next);
> +tcg_gen_insn_start(ctx->base.pc_next, 0);
> +ctx->insn_start = tcg_last_op();
>  }
>
>  static void

Re: [PATCH 3/4] target/riscv: Set [m|s]tval for both illegal and virtual instruction traps

2022-04-29 Thread Frank Chang 
Reviewed-by: Frank Chang 

On Fri, Apr 29, 2022 at 11:36 AM Anup Patel  wrote:

> Currently, the [m|s]tval CSRs are set with trapping instruction encoding
> only for illegal instruction traps taken at the time of instruction
> decoding.
>
> In RISC-V world, a valid instructions might also trap as illegal or
> virtual instruction based to trapping bits in various CSRs (such as
> mstatus.TVM or hstatus.VTVM).
>
> We improve setting of [m|s]tval CSRs for all types of illegal and
> virtual instruction traps.
>
> Signed-off-by: Anup Patel 
> ---
>  target/riscv/cpu.c|  2 ++
>  target/riscv/cpu.h|  8 +++-
>  target/riscv/cpu_helper.c |  1 +
>  target/riscv/translate.c  | 17 +
>  4 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index dff4606585..f0a702fee6 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -406,6 +406,7 @@ void restore_state_to_opc(CPURISCVState *env,
> TranslationBlock *tb,
>  } else {
>  env->pc = data[0];
>  }
> +env->bins = data[1];
>  }
>
>  static void riscv_cpu_reset(DeviceState *dev)
> @@ -445,6 +446,7 @@ static void riscv_cpu_reset(DeviceState *dev)
>  env->mcause = 0;
>  env->miclaim = MIP_SGEIP;
>  env->pc = env->resetvec;
> +env->bins = 0;
>  env->two_stage_lookup = false;
>
>  /* Initialized default priorities of local interrupts. */
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index fe6c9a2c92..a55c918274 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -30,6 +30,12 @@
>
>  #define TCG_GUEST_DEFAULT_MO 0
>
> +/*
> + * RISC-V-specific extra insn start words:
> + * 1: Original instruction opcode
> + */
> +#define TARGET_INSN_START_EXTRA_WORDS 1
> +
>  #define TYPE_RISCV_CPU "riscv-cpu"
>
>  #define RISCV_CPU_TYPE_SUFFIX "-" TYPE_RISCV_CPU
> @@ -140,7 +146,7 @@ struct CPUArchState {
>  target_ulong frm;
>
>  target_ulong badaddr;
> -uint32_t bins;
> +target_ulong bins;
>
>  target_ulong guest_phys_fault_addr;
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d83579accf..bba4fce777 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1371,6 +1371,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>  tval = env->badaddr;
>  break;
>  case RISCV_EXCP_ILLEGAL_INST:
> +case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
>  tval = env->bins;
>  break;
>  default:
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 0cd1d9ee94..55a4713af2 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -107,6 +107,8 @@ typedef struct DisasContext {
>  /* PointerMasking extension */
>  bool pm_mask_enabled;
>  bool pm_base_enabled;
> +/* TCG of the current insn_start */
> +TCGOp *insn_start;
>  } DisasContext;
>
>  static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> @@ -236,9 +238,6 @@ static void generate_exception_mtval(DisasContext
> *ctx, int excp)
>
>  static void gen_exception_illegal(DisasContext *ctx)
>  {
> -tcg_gen_st_i32(tcg_constant_i32(ctx->opcode), cpu_env,
> -   offsetof(CPURISCVState, bins));
> -
>  generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
>  }
>
> @@ -1017,6 +1016,13 @@ static uint32_t opcode_at(DisasContextBase *dcbase,
> target_ulong pc)
>  /* Include decoders for factored-out extensions */
>  #include "decode-XVentanaCondOps.c.inc"
>
> +static inline void decode_save_opc(DisasContext *ctx, target_ulong opc)
> +{
> +assert(ctx->insn_start != NULL);
> +tcg_set_insn_start_param(ctx->insn_start, 1, opc);
> +ctx->insn_start = NULL;
> +}
> +
>  static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t
> opcode)
>  {
>  /*
> @@ -1033,6 +1039,7 @@ static void decode_opc(CPURISCVState *env,
> DisasContext *ctx, uint16_t opcode)
>
>  /* Check for compressed insn */
>  if (extract16(opcode, 0, 2) != 3) {
> +decode_save_opc(ctx, opcode);
>  if (!has_ext(ctx, RVC)) {
>  gen_exception_illegal(ctx);
>  } else {
> @@ -1047,6 +1054,7 @@ static void decode_opc(CPURISCVState *env,
> DisasContext *ctx, uint16_t opcode)
>  opcode32 = deposit32(opcode32, 16, 16,
>   translator_lduw(env, >base,
>   ctx->base.pc_next + 2));
> +decode_save_opc(ctx, opcode32);
>  ctx->opcode = opcode32;
>  ctx->pc_succ_insn = ctx->base.pc_next + 4;
>
> @@ -1113,7 +1121,8 @@ static void riscv_tr_insn_start(DisasContextBase
> *dcbase, CPUState *cpu)
>  {
>  DisasContext *ctx = container_of(dcbase, DisasContext, base);
>
> -tcg_gen_insn_start(ctx->base.pc_next);
> +tcg_gen_insn_start(ctx->base.pc_next, 0);
> +ctx->insn_start = tcg_last_op();
>  }
>
>  static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState

[PATCH 3/4] target/riscv: Set [m|s]tval for both illegal and virtual instruction traps

2022-04-28 Thread Anup Patel 
Currently, the [m|s]tval CSRs are set with trapping instruction encoding
only for illegal instruction traps taken at the time of instruction
decoding.

In RISC-V world, a valid instructions might also trap as illegal or
virtual instruction based to trapping bits in various CSRs (such as
mstatus.TVM or hstatus.VTVM).

We improve setting of [m|s]tval CSRs for all types of illegal and
virtual instruction traps.

Signed-off-by: Anup Patel 
---
 target/riscv/cpu.c|  2 ++
 target/riscv/cpu.h|  8 +++-
 target/riscv/cpu_helper.c |  1 +
 target/riscv/translate.c  | 17 +
 4 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index dff4606585..f0a702fee6 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -406,6 +406,7 @@ void restore_state_to_opc(CPURISCVState *env, 
TranslationBlock *tb,
 } else {
 env->pc = data[0];
 }
+env->bins = data[1];
 }
 
 static void riscv_cpu_reset(DeviceState *dev)
@@ -445,6 +446,7 @@ static void riscv_cpu_reset(DeviceState *dev)
 env->mcause = 0;
 env->miclaim = MIP_SGEIP;
 env->pc = env->resetvec;
+env->bins = 0;
 env->two_stage_lookup = false;
 
 /* Initialized default priorities of local interrupts. */
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index fe6c9a2c92..a55c918274 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -30,6 +30,12 @@
 
 #define TCG_GUEST_DEFAULT_MO 0
 
+/*
+ * RISC-V-specific extra insn start words:
+ * 1: Original instruction opcode
+ */
+#define TARGET_INSN_START_EXTRA_WORDS 1
+
 #define TYPE_RISCV_CPU "riscv-cpu"
 
 #define RISCV_CPU_TYPE_SUFFIX "-" TYPE_RISCV_CPU
@@ -140,7 +146,7 @@ struct CPUArchState {
 target_ulong frm;
 
 target_ulong badaddr;
-uint32_t bins;
+target_ulong bins;
 
 target_ulong guest_phys_fault_addr;
 
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index d83579accf..bba4fce777 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1371,6 +1371,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
 tval = env->badaddr;
 break;
 case RISCV_EXCP_ILLEGAL_INST:
+case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
 tval = env->bins;
 break;
 default:
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 0cd1d9ee94..55a4713af2 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -107,6 +107,8 @@ typedef struct DisasContext {
 /* PointerMasking extension */
 bool pm_mask_enabled;
 bool pm_base_enabled;
+/* TCG of the current insn_start */
+TCGOp *insn_start;
 } DisasContext;
 
 static inline bool has_ext(DisasContext *ctx, uint32_t ext)
@@ -236,9 +238,6 @@ static void generate_exception_mtval(DisasContext *ctx, int 
excp)
 
 static void gen_exception_illegal(DisasContext *ctx)
 {
-tcg_gen_st_i32(tcg_constant_i32(ctx->opcode), cpu_env,
-   offsetof(CPURISCVState, bins));
-
 generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
 }
 
@@ -1017,6 +1016,13 @@ static uint32_t opcode_at(DisasContextBase *dcbase, 
target_ulong pc)
 /* Include decoders for factored-out extensions */
 #include "decode-XVentanaCondOps.c.inc"
 
+static inline void decode_save_opc(DisasContext *ctx, target_ulong opc)
+{
+assert(ctx->insn_start != NULL);
+tcg_set_insn_start_param(ctx->insn_start, 1, opc);
+ctx->insn_start = NULL;
+}
+
 static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
 {
 /*
@@ -1033,6 +1039,7 @@ static void decode_opc(CPURISCVState *env, DisasContext 
*ctx, uint16_t opcode)
 
 /* Check for compressed insn */
 if (extract16(opcode, 0, 2) != 3) {
+decode_save_opc(ctx, opcode);
 if (!has_ext(ctx, RVC)) {
 gen_exception_illegal(ctx);
 } else {
@@ -1047,6 +1054,7 @@ static void decode_opc(CPURISCVState *env, DisasContext 
*ctx, uint16_t opcode)
 opcode32 = deposit32(opcode32, 16, 16,
  translator_lduw(env, >base,
  ctx->base.pc_next + 2));
+decode_save_opc(ctx, opcode32);
 ctx->opcode = opcode32;
 ctx->pc_succ_insn = ctx->base.pc_next + 4;
 
@@ -1113,7 +1121,8 @@ static void riscv_tr_insn_start(DisasContextBase *dcbase, 
CPUState *cpu)
 {
 DisasContext *ctx = container_of(dcbase, DisasContext, base);
 
-tcg_gen_insn_start(ctx->base.pc_next);
+tcg_gen_insn_start(ctx->base.pc_next, 0);
+ctx->insn_start = tcg_last_op();
 }
 
 static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
-- 
2.34.1