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

[PATCH v2 0/3] 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 if
the features are set for the CPU.



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

 target/riscv/cpu.h        |  6 +++++-
 target/riscv/cpu.c        |  6 +++++-
 target/riscv/cpu_helper.c | 10 +++++++++
 target/riscv/translate.c  | 43 +++++++++++++++++++++------------------
 4 files changed, 43 insertions(+), 22 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] 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 | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index e356fc6c46..25670be435 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -485,20 +485,20 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
 /* Include the auto-generated decoder for 16 bit insn */
 #include "decode-insn16.c.inc"
 
-static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
+static void decode_opc(CPURISCVState *env, DisasContext *ctx)
 {
     /* check for compressed insn */
-    if (extract16(opcode, 0, 2) != 3) {
+    if (extract16(ctx->opcode, 0, 2) != 3) {
         if (!has_ext(ctx, RVC)) {
             gen_exception_illegal(ctx);
         } else {
             ctx->pc_succ_insn = ctx->base.pc_next + 2;
-            if (!decode_insn16(ctx, opcode)) {
+            if (!decode_insn16(ctx, ctx->opcode)) {
                 gen_exception_illegal(ctx);
             }
         }
     } else {
-        uint32_t opcode32 = opcode;
+        uint32_t opcode32 = ctx->opcode;
         opcode32 = deposit32(opcode32, 16, 16,
                              translator_lduw(env, ctx->base.pc_next + 2));
         ctx->pc_succ_insn = ctx->base.pc_next + 4;
@@ -561,9 +561,9 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPURISCVState *env = cpu->env_ptr;
-    uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
+    ctx->opcode = translator_lduw(env, ctx->base.pc_next);
 
-    decode_opc(env, ctx, opcode16);
+    decode_opc(env, ctx);
     ctx->base.pc_next = ctx->pc_succ_insn;
     ctx->w = false;
 
-- 
2.31.1

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

[Bin Meng]

On Wed, Sep 8, 2021 at 12:54 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/translate.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

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

[Richard Henderson]

On 9/8/21 6:54 AM, Alistair Francis wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/translate.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index e356fc6c46..25670be435 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -485,20 +485,20 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
>   /* Include the auto-generated decoder for 16 bit insn */
>   #include "decode-insn16.c.inc"
>   
> -static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> +static void decode_opc(CPURISCVState *env, DisasContext *ctx)
>   {
>       /* check for compressed insn */
> -    if (extract16(opcode, 0, 2) != 3) {
> +    if (extract16(ctx->opcode, 0, 2) != 3) {
>           if (!has_ext(ctx, RVC)) {
>               gen_exception_illegal(ctx);
>           } else {
>               ctx->pc_succ_insn = ctx->base.pc_next + 2;
> -            if (!decode_insn16(ctx, opcode)) {
> +            if (!decode_insn16(ctx, ctx->opcode)) {
>                   gen_exception_illegal(ctx);
>               }
>           }
>       } else {
> -        uint32_t opcode32 = opcode;
> +        uint32_t opcode32 = ctx->opcode;
>           opcode32 = deposit32(opcode32, 16, 16,
>                                translator_lduw(env, ctx->base.pc_next + 2));

You needed to write back to ctx->opcode here.

I think that all of the other changes are less than ideal -- let the value stay in a 
register as long as possible and drop them to memory immediately before calling 
decode_insn{16,32}, just before the write to pc_succ_insn in both cases.


r~


>           ctx->pc_succ_insn = ctx->base.pc_next + 4;
> @@ -561,9 +561,9 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>   {
>       DisasContext *ctx = container_of(dcbase, DisasContext, base);
>       CPURISCVState *env = cpu->env_ptr;
> -    uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
> +    ctx->opcode = translator_lduw(env, ctx->base.pc_next);
>   
> -    decode_opc(env, ctx, opcode16);
> +    decode_opc(env, ctx);
>       ctx->base.pc_next = ctx->pc_succ_insn;
>       ctx->w = false;
>   
> 

[PATCH v2 2/3] 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        |  5 ++++-
 target/riscv/cpu_helper.c | 10 ++++++++++
 target/riscv/translate.c  | 31 +++++++++++++++++--------------
 3 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index bf1c899c00..d11db1f031 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -77,7 +77,8 @@ enum {
     RISCV_FEATURE_MMU,
     RISCV_FEATURE_PMP,
     RISCV_FEATURE_EPMP,
-    RISCV_FEATURE_MISA
+    RISCV_FEATURE_MISA,
+    RISCV_FEATURE_MTVAL_INST,
 };
 
 #define PRIV_VERSION_1_10_0 0x00011000
@@ -130,6 +131,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 968cb8046f..65832967e1 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -967,6 +967,16 @@ void riscv_cpu_do_interrupt(CPUState *cs)
             write_tval  = true;
             tval = env->badaddr;
             break;
+        case RISCV_EXCP_ILLEGAL_INST:
+            if (riscv_feature(env, RISCV_FEATURE_MTVAL_INST)) {
+                /*
+                 * The stval/mtval register can optionally also be used to
+                 * return the faulting instruction bits on an illegal
+                 * instruction exception.
+                 */
+                tval = env->bins;
+            }
+            break;
         default:
             break;
         }
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 25670be435..99810db57d 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -173,8 +173,25 @@ static void lookup_and_goto_ptr(DisasContext *ctx)
     }
 }
 
+/*
+ * Wrappers for getting reg values.
+ *
+ * The $zero register does not have cpu_gpr[0] allocated -- we supply the
+ * constant zero as a source, and an uninitialized sink as destination.
+ *
+ * Further, we may provide an extension for word operations.
+ */
+static TCGv temp_new(DisasContext *ctx)
+{
+    assert(ctx->ntemp < ARRAY_SIZE(ctx->temp));
+    return ctx->temp[ctx->ntemp++] = tcg_temp_new();
+}
+
 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);
 }
 
@@ -195,20 +212,6 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
     }
 }
 
-/*
- * Wrappers for getting reg values.
- *
- * The $zero register does not have cpu_gpr[0] allocated -- we supply the
- * constant zero as a source, and an uninitialized sink as destination.
- *
- * Further, we may provide an extension for word operations.
- */
-static TCGv temp_new(DisasContext *ctx)
-{
-    assert(ctx->ntemp < ARRAY_SIZE(ctx->temp));
-    return ctx->temp[ctx->ntemp++] = tcg_temp_new();
-}
-
 static TCGv get_gpr(DisasContext *ctx, int reg_num, DisasExtend ext)
 {
     TCGv t;
-- 
2.31.1

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

[Bin Meng]

On Wed, Sep 8, 2021 at 12:54 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> 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        |  5 ++++-
>  target/riscv/cpu_helper.c | 10 ++++++++++
>  target/riscv/translate.c  | 31 +++++++++++++++++--------------
>  3 files changed, 31 insertions(+), 15 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

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

[Richard Henderson]

On 9/8/21 6:54 AM, Alistair Francis wrote:
> @@ -967,6 +967,16 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>               write_tval  = true;
>               tval = env->badaddr;
>               break;
> +        case RISCV_EXCP_ILLEGAL_INST:
> +            if (riscv_feature(env, RISCV_FEATURE_MTVAL_INST)) {
> +                /*
> +                 * The stval/mtval register can optionally also be used to
> +                 * return the faulting instruction bits on an illegal
> +                 * instruction exception.
> +                 */
> +                tval = env->bins;
> +            }
> +            break;

I'll note that write_tval should probably be renamed, and/or eliminated, because it looks 
like it's incorrectly unset here.  If you move the adjustment to cause above this switch, 
then you can move the RVH code that needed write_tval into this switch (just the 
HSTATUS_GVA update?).

But... more specific to this case.  Prior to this, was the exception handler allowed to 
assume anything about the contents of stval?  Should the value have been zero?  Would it 
be wrong to write to stval unconditionally?  How does the guest OS know that it can rely 
on stval being set?

I simply wonder whether it's worthwhile to add the feature and feature test.


r~

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

[Alistair Francis]

On Wed, Sep 8, 2021 at 4:48 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 9/8/21 6:54 AM, Alistair Francis wrote:
> > @@ -967,6 +967,16 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >               write_tval  = true;
> >               tval = env->badaddr;
> >               break;
> > +        case RISCV_EXCP_ILLEGAL_INST:
> > +            if (riscv_feature(env, RISCV_FEATURE_MTVAL_INST)) {
> > +                /*
> > +                 * The stval/mtval register can optionally also be used to
> > +                 * return the faulting instruction bits on an illegal
> > +                 * instruction exception.
> > +                 */
> > +                tval = env->bins;
> > +            }
> > +            break;
>
> I'll note that write_tval should probably be renamed, and/or eliminated, because it looks
> like it's incorrectly unset here.  If you move the adjustment to cause above this switch,
> then you can move the RVH code that needed write_tval into this switch (just the
> HSTATUS_GVA update?).
>
> But... more specific to this case.  Prior to this, was the exception handler allowed to
> assume anything about the contents of stval?  Should the value have been zero?  Would it
> be wrong to write to stval unconditionally?  How does the guest OS know that it can rely
> on stval being set?

As we didn't support writing the illegal instruction stval should be
zero before this patch.

>
> I simply wonder whether it's worthwhile to add the feature and feature test.

Do you just mean have it enabled all the time?

Alistair

>
>
> r~

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

[Richard Henderson]

On 9/24/21 2:48 AM, Alistair Francis wrote:
>> But... more specific to this case.  Prior to this, was the exception handler allowed to
>> assume anything about the contents of stval?  Should the value have been zero?  Would it
>> be wrong to write to stval unconditionally?  How does the guest OS know that it can rely
>> on stval being set?
> 
> As we didn't support writing the illegal instruction stval should be
> zero before this patch.

Ok, that didn't quite answer the question...

If *wasn't* zero before this patch: we didn't write anything at all, and so keep whatever 
previous value the previous exception wrote.

Is that a bug that needs fixing?  Because you're still not writing anything to stval if 
!MTVAL_INST...

>> I simply wonder whether it's worthwhile to add the feature and feature test.
> 
> Do you just mean have it enabled all the time?

Yes, if without this feature the value of stval was undefined.


r~

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

[Alistair Francis]

On Fri, Sep 24, 2021 at 10:57 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 9/24/21 2:48 AM, Alistair Francis wrote:
> >> But... more specific to this case.  Prior to this, was the exception handler allowed to
> >> assume anything about the contents of stval?  Should the value have been zero?  Would it
> >> be wrong to write to stval unconditionally?  How does the guest OS know that it can rely
> >> on stval being set?
> >
> > As we didn't support writing the illegal instruction stval should be
> > zero before this patch.
>
> Ok, that didn't quite answer the question...
>
> If *wasn't* zero before this patch: we didn't write anything at all, and so keep whatever
> previous value the previous exception wrote.
>
> Is that a bug that needs fixing?  Because you're still not writing anything to stval if
> !MTVAL_INST...

Yeah, that sounds like a bug then.

>
> >> I simply wonder whether it's worthwhile to add the feature and feature test.
> >
> > Do you just mean have it enabled all the time?
>
> Yes, if without this feature the value of stval was undefined.

Ok, I'll have another look at this. Thanks for pointing this out.

Alistair

>
>
> r~

[PATCH v2 3/3] target/riscv: Set mtval and stval support

[Alistair Francis]

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

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

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index d11db1f031..5b0bbf2fca 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -309,6 +309,7 @@ struct RISCVCPU {
         bool mmu;
         bool pmp;
         bool epmp;
+        bool mtval_inst;
         uint64_t resetvec;
     } cfg;
 };
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1a2b03d579..537f2af341 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -437,6 +437,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         }
     }
 
+    if (cpu->cfg.mtval_inst) {
+        set_feature(env, RISCV_FEATURE_MTVAL_INST);
+    }
+
     set_resetvec(env, cpu->cfg.resetvec);
 
     /* If only XLEN is set for misa, then set misa from properties */
@@ -600,7 +604,7 @@ static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
     DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
     DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
-
+    DEFINE_PROP_BOOL("tval-inst", RISCVCPU, cfg.mtval_inst, true),
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC),
     DEFINE_PROP_END_OF_LIST(),
 };
-- 
2.31.1

Re: [PATCH v2 3/3] target/riscv: Set mtval and stval support

[Bin Meng]

On Wed, Sep 8, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.h | 1 +
>  target/riscv/cpu.c | 6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index d11db1f031..5b0bbf2fca 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -309,6 +309,7 @@ struct RISCVCPU {
>          bool mmu;
>          bool pmp;
>          bool epmp;
> +        bool mtval_inst;
>          uint64_t resetvec;
>      } cfg;
>  };
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 1a2b03d579..537f2af341 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -437,6 +437,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>          }
>      }
>
> +    if (cpu->cfg.mtval_inst) {
> +        set_feature(env, RISCV_FEATURE_MTVAL_INST);
> +    }
> +
>      set_resetvec(env, cpu->cfg.resetvec);
>
>      /* If only XLEN is set for misa, then set misa from properties */
> @@ -600,7 +604,7 @@ static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
>      DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
>      DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
> -
> +    DEFINE_PROP_BOOL("tval-inst", RISCVCPU, cfg.mtval_inst, true),

Should we tweak such on a per-CPU basis instead of globally enabling
it, e.g.: update sifive_u54 per the real hardware implementation?

>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>

Regards,
Bin