[PATCH 0/2] RISC-V: Populate mtval and stval

[PATCH 0/2] RISC-V: Populate mtval and stval

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

Populate mtval and stval when taking an illegal instruction exception.

The RISC-V spec states that "The stval register can optionally also be
used to return the faulting instruction bits on an illegal instruction
exception...". In this case we are always writing the value on an
illegal instruction.

This doesn't match all CPUs (some CPUs won't write the data), but in
QEMU let's just populate the value on illegal instructions. This won't
break any guest software, but will provide more information to guests.

*** BLURB HERE ***

Alistair Francis (2):
  target/riscv: Set the opcode in DisasContext
  target/riscv: Implement the stval/mtval illegal instruction

 target/riscv/cpu.h        |  2 ++
 target/riscv/cpu_helper.c | 25 +++++++++++--------------
 target/riscv/translate.c  |  5 +++++
 3 files changed, 18 insertions(+), 14 deletions(-)

-- 
2.31.1

[PATCH 1/2] target/riscv: Set the opcode in DisasContext

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 1d57bc97b5..24251bc8cc 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -586,6 +586,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
         if (!has_ext(ctx, RVC)) {
             gen_exception_illegal(ctx);
         } else {
+            ctx->opcode = opcode;
             ctx->pc_succ_insn = ctx->base.pc_next + 2;
             if (!decode_insn16(ctx, opcode)) {
                 gen_exception_illegal(ctx);
@@ -596,6 +597,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
         opcode32 = deposit32(opcode32, 16, 16,
                              translator_lduw(env, &ctx->base,
                                              ctx->base.pc_next + 2));
+        ctx->opcode = opcode32;
         ctx->pc_succ_insn = ctx->base.pc_next + 4;
         if (!decode_insn32(ctx, opcode32)) {
             gen_exception_illegal(ctx);
-- 
2.31.1

[PATCH 2/2] target/riscv: Implement the stval/mtval illegal instruction

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

The stval and mtval registers can optionally contain the faulting
instruction on an illegal instruction exception. This patch adds support
for setting the stval and mtval registers based on the CPU feature.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.h        |  2 ++
 target/riscv/cpu_helper.c | 25 +++++++++++--------------
 target/riscv/translate.c  |  3 +++
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0760c0af93..3a163b57ed 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -127,6 +127,8 @@ struct CPURISCVState {
     target_ulong frm;
 
     target_ulong badaddr;
+    uint32_t bins;
+
     target_ulong guest_phys_fault_addr;
 
     target_ulong priv_ver;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 9eeed38c7e..cb5833a6d2 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -975,7 +975,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
     bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG);
     target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
     target_ulong deleg = async ? env->mideleg : env->medeleg;
-    bool write_tval = false;
     target_ulong tval = 0;
     target_ulong htval = 0;
     target_ulong mtval2 = 0;
@@ -1004,9 +1003,11 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         case RISCV_EXCP_INST_PAGE_FAULT:
         case RISCV_EXCP_LOAD_PAGE_FAULT:
         case RISCV_EXCP_STORE_PAGE_FAULT:
-            write_tval  = true;
             tval = env->badaddr;
             break;
+        case RISCV_EXCP_ILLEGAL_INST:
+            tval = env->bins;
+            break;
         default:
             break;
         }
@@ -1041,17 +1042,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         if (riscv_has_ext(env, RVH)) {
             target_ulong hdeleg = async ? env->hideleg : env->hedeleg;
 
-            if (env->two_stage_lookup && write_tval) {
-                /*
-                 * If we are writing a guest virtual address to stval, set
-                 * this to 1. If we are trapping to VS we will set this to 0
-                 * later.
-                 */
-                env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1);
-            } else {
-                /* For other HS-mode traps, we set this to 0. */
-                env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
-            }
+            env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
 
             if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) {
                 /* Trap to VS mode */
@@ -1063,7 +1054,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
                     cause == IRQ_VS_EXT) {
                     cause = cause - 1;
                 }
-                env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
             } else if (riscv_cpu_virt_enabled(env)) {
                 /* Trap into HS mode, from virt */
                 riscv_cpu_swap_hypervisor_regs(env);
@@ -1071,6 +1061,13 @@ void riscv_cpu_do_interrupt(CPUState *cs)
                                          env->priv);
                 env->hstatus = set_field(env->hstatus, HSTATUS_SPV,
                                          riscv_cpu_virt_enabled(env));
+                if (tval) {
+                    /*
+                     * If we are writing a guest virtual address to stval, set
+                     * this to 1.
+                     */
+                    env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1);
+                }
 
                 htval = env->guest_phys_fault_addr;
 
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 24251bc8cc..921ca06bf9 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -167,6 +167,9 @@ 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);
 }
 
-- 
2.31.1

Re: [PATCH 2/2] target/riscv: Implement the stval/mtval illegal instruction

[Richard Henderson]

On 12/9/21 10:26 PM, Alistair Francis wrote:
> @@ -975,7 +975,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>       bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG);
>       target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
>       target_ulong deleg = async ? env->mideleg : env->medeleg;
> -    bool write_tval = false;
>       target_ulong tval = 0;
>       target_ulong htval = 0;
>       target_ulong mtval2 = 0;
> @@ -1004,9 +1003,11 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>           case RISCV_EXCP_INST_PAGE_FAULT:
>           case RISCV_EXCP_LOAD_PAGE_FAULT:
>           case RISCV_EXCP_STORE_PAGE_FAULT:
> -            write_tval  = true;
>               tval = env->badaddr;
>               break;
...
> @@ -1041,17 +1042,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>           if (riscv_has_ext(env, RVH)) {
>               target_ulong hdeleg = async ? env->hideleg : env->hedeleg;
>   
> -            if (env->two_stage_lookup && write_tval) {
> -                /*
> -                 * If we are writing a guest virtual address to stval, set
> -                 * this to 1. If we are trapping to VS we will set this to 0
> -                 * later.
> -                 */
> -                env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1);
> -            } else {
> -                /* For other HS-mode traps, we set this to 0. */
> -                env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
> -            }
> +            env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
>   
>               if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) {
>                   /* Trap to VS mode */
> @@ -1063,7 +1054,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>                       cause == IRQ_VS_EXT) {
>                       cause = cause - 1;
>                   }
> -                env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
>               } else if (riscv_cpu_virt_enabled(env)) {
>                   /* Trap into HS mode, from virt */
>                   riscv_cpu_swap_hypervisor_regs(env);
> @@ -1071,6 +1061,13 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>                                            env->priv);
>                   env->hstatus = set_field(env->hstatus, HSTATUS_SPV,
>                                            riscv_cpu_virt_enabled(env));
> +                if (tval) {
> +                    /*
> +                     * If we are writing a guest virtual address to stval, set
> +                     * this to 1.
> +                     */
> +                    env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1);
> +                }

The thing that concerns me here is the very legitimate badaddr of NULL.  I think you need 
to keep some out-of-band flag for that.

In addition, the out-of-band flag should probably be called write_gva, because tval from 
ILLEGAL_INST is not a Guest Virtual Address, and so should not set GVA.

You could even push the setting of the bit down so that it's only done once, e.g.

     hstatus_gva = true;
     tval = env->badaddr;

     if (trap to vs) {
        ...
        hstatus_gva = false;
     } else if (virt enabled) {
        ...
     } else {
        trap into hs
        hstatus_gva = false;
     }
     env->hstatus(set_field(env->hstatus, HSTATUS_GVA, hstatus_gva);


The actual handling of ILLEGAL_INST looks fine.
You may well want to split the GVA handling to a separate patch.


r~

Re: [PATCH 1/2] target/riscv: Set the opcode in DisasContext

[Richard Henderson]

On 12/9/21 10:26 PM, Alistair Francis wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/translate.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~