[Qemu-devel] [PATCH for-2.12 0/2] RISC-V: Mark FP status dirty

[Qemu-devel] [PATCH for-2.12 0/2] RISC-V: Mark FP status dirty

[Richard Henderson]

Since it was my patch that broke FP state tracking in the
first place, I feel obligated to fix it again.

Mark mstatus[fs] as dirty whenever we write to the file.
This can be optimized by only doing so once within a TB
which initially began with a clean file.

I have not yet put together an environment that can test
this, so I'll need someone else to give it a go.


r~


Richard Henderson (2):
  target/riscv: Split out mstatus_fs from tb_flags during translation
  target/riscv: Mark MSTATUS_FS dirty

 target/riscv/cpu.h       |  6 +++---
 target/riscv/op_helper.c | 25 ++++++++++++++++--------
 target/riscv/translate.c | 50 ++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 64 insertions(+), 17 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] target/riscv: Split out mstatus_fs from tb_flags during translation

[Richard Henderson]

We will want to track changes to mstatus_fs through the TB.
As there is nothing else in tb_flags at the moment, remove
the variable from DisasContext.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/cpu.h       |  6 +++---
 target/riscv/translate.c | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 41e06ac0f9..d201dd3e90 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -269,8 +269,8 @@ void QEMU_NORETURN do_raise_exception_err(CPURISCVState *env,
 target_ulong cpu_riscv_get_fflags(CPURISCVState *env);
 void cpu_riscv_set_fflags(CPURISCVState *env, target_ulong);
 
-#define TB_FLAGS_MMU_MASK  3
-#define TB_FLAGS_FP_ENABLE MSTATUS_FS
+#define TB_FLAGS_MMU_MASK   3
+#define TB_FLAGS_MSTATUS_FS MSTATUS_FS
 
 static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
                                         target_ulong *cs_base, uint32_t *flags)
@@ -278,7 +278,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
     *pc = env->pc;
     *cs_base = 0;
 #ifdef CONFIG_USER_ONLY
-    *flags = TB_FLAGS_FP_ENABLE;
+    *flags = TB_FLAGS_MSTATUS_FS;
 #else
     *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
 #endif
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 808eab7f50..a30724aa90 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -43,7 +43,7 @@ typedef struct DisasContext {
     target_ulong pc;
     target_ulong next_pc;
     uint32_t opcode;
-    uint32_t flags;
+    uint32_t mstatus_fs;
     uint32_t mem_idx;
     int singlestep_enabled;
     int bstate;
@@ -665,7 +665,7 @@ static void gen_fp_load(DisasContext *ctx, uint32_t opc, int rd,
 {
     TCGv t0;
 
-    if (!(ctx->flags & TB_FLAGS_FP_ENABLE)) {
+    if (ctx->mstatus_fs == 0) {
         gen_exception_illegal(ctx);
         return;
     }
@@ -695,7 +695,7 @@ static void gen_fp_store(DisasContext *ctx, uint32_t opc, int rs1,
 {
     TCGv t0;
 
-    if (!(ctx->flags & TB_FLAGS_FP_ENABLE)) {
+    if (ctx->mstatus_fs == 0) {
         gen_exception_illegal(ctx);
         return;
     }
@@ -986,7 +986,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
 {
     TCGv t0 = NULL;
 
-    if (!(ctx->flags & TB_FLAGS_FP_ENABLE)) {
+    if (ctx->mstatus_fs == 0) {
         goto do_illegal;
     }
 
@@ -1862,8 +1862,8 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
 
     ctx.tb = tb;
     ctx.bstate = BS_NONE;
-    ctx.flags = tb->flags;
     ctx.mem_idx = tb->flags & TB_FLAGS_MMU_MASK;
+    ctx.mstatus_fs = tb->flags & TB_FLAGS_MSTATUS_FS;
     ctx.frm = -1;  /* unknown rounding mode */
 
     num_insns = 0;
-- 
2.14.3

[Qemu-devel] [PATCH 2/2] target/riscv: Mark MSTATUS_FS dirty

[Richard Henderson]

Writes to the FP register file mark the register file as dirty.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/op_helper.c | 25 +++++++++++++++++--------
 target/riscv/translate.c | 40 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index e34715df4e..74eeef0be8 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -72,11 +72,20 @@ void helper_raise_exception(CPURISCVState *env, uint32_t exception)
     do_raise_exception_err(env, exception, 0);
 }
 
-static void validate_mstatus_fs(CPURISCVState *env, uintptr_t ra)
+static void validate_mstatus_fs(CPURISCVState *env, uintptr_t ra, bool write)
 {
 #ifndef CONFIG_USER_ONLY
-    if (!(env->mstatus & MSTATUS_FS)) {
+    switch (get_field(env->mstatus, MSTATUS_FS)) {
+    case 0: /* disabled */
         do_raise_exception_err(env, RISCV_EXCP_ILLEGAL_INST, ra);
+        g_assert_not_reached();
+    case 1: /* initial */
+    case 2: /* clean */
+        if (write) {
+            /* Mark fp status as dirty.  */
+            env->mstatus = MSTATUS_FS;
+        }
+        break;
     }
 #endif
 }
@@ -96,15 +105,15 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
 
     switch (csrno) {
     case CSR_FFLAGS:
-        validate_mstatus_fs(env, GETPC());
+        validate_mstatus_fs(env, GETPC(), true);
         cpu_riscv_set_fflags(env, val_to_write & (FSR_AEXC >> FSR_AEXC_SHIFT));
         break;
     case CSR_FRM:
-        validate_mstatus_fs(env, GETPC());
+        validate_mstatus_fs(env, GETPC(), true);
         env->frm = val_to_write & (FSR_RD >> FSR_RD_SHIFT);
         break;
     case CSR_FCSR:
-        validate_mstatus_fs(env, GETPC());
+        validate_mstatus_fs(env, GETPC(), true);
         env->frm = (val_to_write & FSR_RD) >> FSR_RD_SHIFT;
         cpu_riscv_set_fflags(env, (val_to_write & FSR_AEXC) >> FSR_AEXC_SHIFT);
         break;
@@ -379,13 +388,13 @@ target_ulong csr_read_helper(CPURISCVState *env, target_ulong csrno)
 
     switch (csrno) {
     case CSR_FFLAGS:
-        validate_mstatus_fs(env, GETPC());
+        validate_mstatus_fs(env, GETPC(), false);
         return cpu_riscv_get_fflags(env);
     case CSR_FRM:
-        validate_mstatus_fs(env, GETPC());
+        validate_mstatus_fs(env, GETPC(), false);
         return env->frm;
     case CSR_FCSR:
-        validate_mstatus_fs(env, GETPC());
+        validate_mstatus_fs(env, GETPC(), false);
         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 */
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index a30724aa90..08fc42a679 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -660,6 +660,31 @@ static void gen_store(DisasContext *ctx, uint32_t opc, int rs1, int rs2,
     tcg_temp_free(dat);
 }
 
+#ifndef CONFIG_USER_ONLY
+/* The states of mstatus_fs are:
+ * 0 = disabled, 1 = initial, 2 = clean, 3 = dirty
+ * We will have already diagnosed disabled state,
+ * and need to turn initial/clean into dirty.
+ */
+static void mark_fs_dirty(DisasContext *ctx)
+{
+    TCGv tmp;
+    if (ctx->mstatus_fs == MSTATUS_FS) {
+        return;
+    }
+    /* Remember the state change for the rest of the TB.  */
+    ctx->mstatus_fs = MSTATUS_FS;
+
+    tmp = tcg_temp_new();
+    tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
+    tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS);
+    tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
+    tcg_temp_free(tmp);
+}
+#else
+static inline void mark_fs_dirty(DisasContext *ctx) { }
+#endif
+
 static void gen_fp_load(DisasContext *ctx, uint32_t opc, int rd,
         int rs1, target_long imm)
 {
@@ -688,6 +713,8 @@ static void gen_fp_load(DisasContext *ctx, uint32_t opc, int rd,
         break;
     }
     tcg_temp_free(t0);
+
+    mark_fs_dirty(ctx);
 }
 
 static void gen_fp_store(DisasContext *ctx, uint32_t opc, int rs1,
@@ -985,6 +1012,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
                          int rs1, int rs2, int rm)
 {
     TCGv t0 = NULL;
+    bool fp_output = true;
 
     if (ctx->mstatus_fs == 0) {
         goto do_illegal;
@@ -1047,6 +1075,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
         }
         gen_set_gpr(rd, t0);
         tcg_temp_free(t0);
+        fp_output = false;
         break;
 
     case OPC_RISC_FCVT_W_S:
@@ -1076,6 +1105,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
         }
         gen_set_gpr(rd, t0);
         tcg_temp_free(t0);
+        fp_output = false;
         break;
 
     case OPC_RISC_FCVT_S_W:
@@ -1126,6 +1156,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
         }
         gen_set_gpr(rd, t0);
         tcg_temp_free(t0);
+        fp_output = false;
         break;
 
     case OPC_RISC_FMV_S_X:
@@ -1218,6 +1249,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
         }
         gen_set_gpr(rd, t0);
         tcg_temp_free(t0);
+        fp_output = false;
         break;
 
     case OPC_RISC_FCVT_W_D:
@@ -1247,6 +1279,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
         }
         gen_set_gpr(rd, t0);
         tcg_temp_free(t0);
+        fp_output = false;
         break;
 
     case OPC_RISC_FCVT_D_W:
@@ -1294,6 +1327,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
         default:
             goto do_illegal;
         }
+        fp_output = false;
         break;
 
     case OPC_RISC_FMV_D_X:
@@ -1310,7 +1344,11 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
             tcg_temp_free(t0);
         }
         gen_exception_illegal(ctx);
-        break;
+        return;
+    }
+
+    if (fp_output) {
+        mark_fs_dirty(ctx);
     }
 }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.12 0/2] RISC-V: Mark FP status dirty

[Michael Clark]

Hi Richard,

Thanks! I'll test this tomorrow morning and we can choose whether to
include your proper fix or the workaround.

I think we have time assuming we send out PRs tomorrow.

Given our important fixes have review including either this fix by tomorrow
or the workaround, and Philippe has reviewed our other important bugs
fixes, then we should be fine.

Then after getting the critical and important fixes out of the way, then I
perhaps make a PR for the other reviewed changes, although these might best
wait until QEMU 2.13 opens.

Thanks again,
Michael.


On Tue, Mar 27, 2018 at 7:22 PM, Richard Henderson <
richard.henderson@linaro.org> wrote:

> Since it was my patch that broke FP state tracking in the
> first place, I feel obligated to fix it again.
>
> Mark mstatus[fs] as dirty whenever we write to the file.
> This can be optimized by only doing so once within a TB
> which initially began with a clean file.
>
> I have not yet put together an environment that can test
> this, so I'll need someone else to give it a go.
>
>
> r~
>
>
> Richard Henderson (2):
>   target/riscv: Split out mstatus_fs from tb_flags during translation
>   target/riscv: Mark MSTATUS_FS dirty
>
>  target/riscv/cpu.h       |  6 +++---
>  target/riscv/op_helper.c | 25 ++++++++++++++++--------
>  target/riscv/translate.c | 50 ++++++++++++++++++++++++++++++
> ++++++++++++------
>  3 files changed, 64 insertions(+), 17 deletions(-)
>
> --
> 2.14.3
>
>

Re: [Qemu-devel] [PATCH 2/2] target/riscv: Mark MSTATUS_FS dirty

[Richard Henderson]

On 03/28/2018 10:22 AM, Richard Henderson wrote:
> +            /* Mark fp status as dirty.  */
> +            env->mstatus = MSTATUS_FS;

Bah.  This should of course be |=.


r~

Re: [Qemu-devel] [PATCH 2/2] target/riscv: Mark MSTATUS_FS dirty

[Michael Clark]

On Tue, Mar 27, 2018 at 7:22 PM, Richard Henderson <
richard.henderson@linaro.org> wrote:

> Writes to the FP register file mark the register file as dirty.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/riscv/op_helper.c | 25 +++++++++++++++++--------
>  target/riscv/translate.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 56 insertions(+), 9 deletions(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index e34715df4e..74eeef0be8 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -72,11 +72,20 @@ void helper_raise_exception(CPURISCVState *env,
> uint32_t exception)
>      do_raise_exception_err(env, exception, 0);
>  }
>
> -static void validate_mstatus_fs(CPURISCVState *env, uintptr_t ra)
> +static void validate_mstatus_fs(CPURISCVState *env, uintptr_t ra, bool
> write)
>  {
>  #ifndef CONFIG_USER_ONLY
> -    if (!(env->mstatus & MSTATUS_FS)) {
> +    switch (get_field(env->mstatus, MSTATUS_FS)) {
> +    case 0: /* disabled */
>          do_raise_exception_err(env, RISCV_EXCP_ILLEGAL_INST, ra);
> +        g_assert_not_reached();
> +    case 1: /* initial */
> +    case 2: /* clean */
> +        if (write) {
> +            /* Mark fp status as dirty.  */
> +            env->mstatus = MSTATUS_FS;
> +        }
> +        break;
>      }
>  #endif
>  }
> @@ -96,15 +105,15 @@ void csr_write_helper(CPURISCVState *env,
> target_ulong val_to_write,
>
>      switch (csrno) {
>      case CSR_FFLAGS:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), true);
>          cpu_riscv_set_fflags(env, val_to_write & (FSR_AEXC >>
> FSR_AEXC_SHIFT));
>          break;
>      case CSR_FRM:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), true);
>          env->frm = val_to_write & (FSR_RD >> FSR_RD_SHIFT);
>          break;
>      case CSR_FCSR:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), true);
>          env->frm = (val_to_write & FSR_RD) >> FSR_RD_SHIFT;
>          cpu_riscv_set_fflags(env, (val_to_write & FSR_AEXC) >>
> FSR_AEXC_SHIFT);
>          break;
> @@ -379,13 +388,13 @@ target_ulong csr_read_helper(CPURISCVState *env,
> target_ulong csrno)
>
>      switch (csrno) {
>      case CSR_FFLAGS:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), false);
>          return cpu_riscv_get_fflags(env);
>      case CSR_FRM:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), false);
>          return env->frm;
>      case CSR_FCSR:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), false);
>          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 */
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index a30724aa90..08fc42a679 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -660,6 +660,31 @@ static void gen_store(DisasContext *ctx, uint32_t
> opc, int rs1, int rs2,
>      tcg_temp_free(dat);
>  }
>
> +#ifndef CONFIG_USER_ONLY
> +/* The states of mstatus_fs are:
> + * 0 = disabled, 1 = initial, 2 = clean, 3 = dirty
> + * We will have already diagnosed disabled state,
> + * and need to turn initial/clean into dirty.
> + */
> +static void mark_fs_dirty(DisasContext *ctx)
> +{
> +    TCGv tmp;
> +    if (ctx->mstatus_fs == MSTATUS_FS) {
> +        return;
> +    }
> +    /* Remember the state change for the rest of the TB.  */
> +    ctx->mstatus_fs = MSTATUS_FS;
> +
> +    tmp = tcg_temp_new();
> +    tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
> +    tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS);
> +    tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
> +    tcg_temp_free(tmp);
> +}
> +#else
> +static inline void mark_fs_dirty(DisasContext *ctx) { }
> +#endif
> +
>  static void gen_fp_load(DisasContext *ctx, uint32_t opc, int rd,
>          int rs1, target_long imm)
>  {
> @@ -688,6 +713,8 @@ static void gen_fp_load(DisasContext *ctx, uint32_t
> opc, int rd,
>          break;
>      }
>      tcg_temp_free(t0);
> +
> +    mark_fs_dirty(ctx);
>  }
>

Don't we want the mark_fs_dirty(ctx) to be at the end of gen_fp_store
instead of gen_fp_load?



>  static void gen_fp_store(DisasContext *ctx, uint32_t opc, int rs1,
> @@ -985,6 +1012,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>                           int rs1, int rs2, int rm)
>  {
>      TCGv t0 = NULL;
> +    bool fp_output = true;
>
>      if (ctx->mstatus_fs == 0) {
>          goto do_illegal;
> @@ -1047,6 +1075,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          }
>          gen_set_gpr(rd, t0);
>          tcg_temp_free(t0);
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FCVT_W_S:
> @@ -1076,6 +1105,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          }
>          gen_set_gpr(rd, t0);
>          tcg_temp_free(t0);
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FCVT_S_W:
> @@ -1126,6 +1156,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          }
>          gen_set_gpr(rd, t0);
>          tcg_temp_free(t0);
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FMV_S_X:
> @@ -1218,6 +1249,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          }
>          gen_set_gpr(rd, t0);
>          tcg_temp_free(t0);
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FCVT_W_D:
> @@ -1247,6 +1279,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          }
>          gen_set_gpr(rd, t0);
>          tcg_temp_free(t0);
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FCVT_D_W:
> @@ -1294,6 +1327,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          default:
>              goto do_illegal;
>          }
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FMV_D_X:
> @@ -1310,7 +1344,11 @@ static void gen_fp_arith(DisasContext *ctx,
> uint32_t opc, int rd,
>              tcg_temp_free(t0);
>          }
>          gen_exception_illegal(ctx);
> -        break;
> +        return;
> +    }
> +
> +    if (fp_output) {
> +        mark_fs_dirty(ctx);
>      }
>  }
>
> --
> 2.14.3
>
>

Re: [Qemu-devel] [PATCH 2/2] target/riscv: Mark MSTATUS_FS dirty

[Michael Clark]

On Wed, Mar 28, 2018 at 10:36 AM, Michael Clark <mjc@sifive.com> wrote:

>
>
> On Tue, Mar 27, 2018 at 7:22 PM, Richard Henderson <
> richard.henderson@linaro.org> wrote:
>
>> Writes to the FP register file mark the register file as dirty.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/riscv/op_helper.c | 25 +++++++++++++++++--------
>>  target/riscv/translate.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 56 insertions(+), 9 deletions(-)
>>
>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>> index e34715df4e..74eeef0be8 100644
>> --- a/target/riscv/op_helper.c
>> +++ b/target/riscv/op_helper.c
>> @@ -72,11 +72,20 @@ void helper_raise_exception(CPURISCVState *env,
>> uint32_t exception)
>>      do_raise_exception_err(env, exception, 0);
>>  }
>>
>> -static void validate_mstatus_fs(CPURISCVState *env, uintptr_t ra)
>> +static void validate_mstatus_fs(CPURISCVState *env, uintptr_t ra, bool
>> write)
>>  {
>>  #ifndef CONFIG_USER_ONLY
>> -    if (!(env->mstatus & MSTATUS_FS)) {
>> +    switch (get_field(env->mstatus, MSTATUS_FS)) {
>> +    case 0: /* disabled */
>>          do_raise_exception_err(env, RISCV_EXCP_ILLEGAL_INST, ra);
>> +        g_assert_not_reached();
>> +    case 1: /* initial */
>> +    case 2: /* clean */
>> +        if (write) {
>> +            /* Mark fp status as dirty.  */
>> +            env->mstatus = MSTATUS_FS;
>> +        }
>> +        break;
>>      }
>>  #endif
>>  }
>> @@ -96,15 +105,15 @@ void csr_write_helper(CPURISCVState *env,
>> target_ulong val_to_write,
>>
>>      switch (csrno) {
>>      case CSR_FFLAGS:
>> -        validate_mstatus_fs(env, GETPC());
>> +        validate_mstatus_fs(env, GETPC(), true);
>>          cpu_riscv_set_fflags(env, val_to_write & (FSR_AEXC >>
>> FSR_AEXC_SHIFT));
>>          break;
>>      case CSR_FRM:
>> -        validate_mstatus_fs(env, GETPC());
>> +        validate_mstatus_fs(env, GETPC(), true);
>>          env->frm = val_to_write & (FSR_RD >> FSR_RD_SHIFT);
>>          break;
>>      case CSR_FCSR:
>> -        validate_mstatus_fs(env, GETPC());
>> +        validate_mstatus_fs(env, GETPC(), true);
>>          env->frm = (val_to_write & FSR_RD) >> FSR_RD_SHIFT;
>>          cpu_riscv_set_fflags(env, (val_to_write & FSR_AEXC) >>
>> FSR_AEXC_SHIFT);
>>          break;
>> @@ -379,13 +388,13 @@ target_ulong csr_read_helper(CPURISCVState *env,
>> target_ulong csrno)
>>
>>      switch (csrno) {
>>      case CSR_FFLAGS:
>> -        validate_mstatus_fs(env, GETPC());
>> +        validate_mstatus_fs(env, GETPC(), false);
>>          return cpu_riscv_get_fflags(env);
>>      case CSR_FRM:
>> -        validate_mstatus_fs(env, GETPC());
>> +        validate_mstatus_fs(env, GETPC(), false);
>>          return env->frm;
>>      case CSR_FCSR:
>> -        validate_mstatus_fs(env, GETPC());
>> +        validate_mstatus_fs(env, GETPC(), false);
>>          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 */
>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>> index a30724aa90..08fc42a679 100644
>> --- a/target/riscv/translate.c
>> +++ b/target/riscv/translate.c
>> @@ -660,6 +660,31 @@ static void gen_store(DisasContext *ctx, uint32_t
>> opc, int rs1, int rs2,
>>      tcg_temp_free(dat);
>>  }
>>
>> +#ifndef CONFIG_USER_ONLY
>> +/* The states of mstatus_fs are:
>> + * 0 = disabled, 1 = initial, 2 = clean, 3 = dirty
>> + * We will have already diagnosed disabled state,
>> + * and need to turn initial/clean into dirty.
>> + */
>> +static void mark_fs_dirty(DisasContext *ctx)
>> +{
>> +    TCGv tmp;
>> +    if (ctx->mstatus_fs == MSTATUS_FS) {
>> +        return;
>> +    }
>> +    /* Remember the state change for the rest of the TB.  */
>> +    ctx->mstatus_fs = MSTATUS_FS;
>> +
>> +    tmp = tcg_temp_new();
>> +    tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
>> +    tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS);
>> +    tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
>> +    tcg_temp_free(tmp);
>> +}
>> +#else
>> +static inline void mark_fs_dirty(DisasContext *ctx) { }
>> +#endif
>> +
>>  static void gen_fp_load(DisasContext *ctx, uint32_t opc, int rd,
>>          int rs1, target_long imm)
>>  {
>> @@ -688,6 +713,8 @@ static void gen_fp_load(DisasContext *ctx, uint32_t
>> opc, int rd,
>>          break;
>>      }
>>      tcg_temp_free(t0);
>> +
>> +    mark_fs_dirty(ctx);
>>  }
>>
>
> Don't we want the mark_fs_dirty(ctx) to be at the end of gen_fp_store
> instead of gen_fp_load?
>

Sorry I was thinking of storing to the fp register file vs storing to
memory. The code is of course correct.


>  static void gen_fp_store(DisasContext *ctx, uint32_t opc, int rs1,
>> @@ -985,6 +1012,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
>> opc, int rd,
>>                           int rs1, int rs2, int rm)
>>  {
>>      TCGv t0 = NULL;
>> +    bool fp_output = true;
>>
>>      if (ctx->mstatus_fs == 0) {
>>          goto do_illegal;
>> @@ -1047,6 +1075,7 @@ static void gen_fp_arith(DisasContext *ctx,
>> uint32_t opc, int rd,
>>          }
>>          gen_set_gpr(rd, t0);
>>          tcg_temp_free(t0);
>> +        fp_output = false;
>>          break;
>>
>>      case OPC_RISC_FCVT_W_S:
>> @@ -1076,6 +1105,7 @@ static void gen_fp_arith(DisasContext *ctx,
>> uint32_t opc, int rd,
>>          }
>>          gen_set_gpr(rd, t0);
>>          tcg_temp_free(t0);
>> +        fp_output = false;
>>          break;
>>
>>      case OPC_RISC_FCVT_S_W:
>> @@ -1126,6 +1156,7 @@ static void gen_fp_arith(DisasContext *ctx,
>> uint32_t opc, int rd,
>>          }
>>          gen_set_gpr(rd, t0);
>>          tcg_temp_free(t0);
>> +        fp_output = false;
>>          break;
>>
>>      case OPC_RISC_FMV_S_X:
>> @@ -1218,6 +1249,7 @@ static void gen_fp_arith(DisasContext *ctx,
>> uint32_t opc, int rd,
>>          }
>>          gen_set_gpr(rd, t0);
>>          tcg_temp_free(t0);
>> +        fp_output = false;
>>          break;
>>
>>      case OPC_RISC_FCVT_W_D:
>> @@ -1247,6 +1279,7 @@ static void gen_fp_arith(DisasContext *ctx,
>> uint32_t opc, int rd,
>>          }
>>          gen_set_gpr(rd, t0);
>>          tcg_temp_free(t0);
>> +        fp_output = false;
>>          break;
>>
>>      case OPC_RISC_FCVT_D_W:
>> @@ -1294,6 +1327,7 @@ static void gen_fp_arith(DisasContext *ctx,
>> uint32_t opc, int rd,
>>          default:
>>              goto do_illegal;
>>          }
>> +        fp_output = false;
>>          break;
>>
>>      case OPC_RISC_FMV_D_X:
>> @@ -1310,7 +1344,11 @@ static void gen_fp_arith(DisasContext *ctx,
>> uint32_t opc, int rd,
>>              tcg_temp_free(t0);
>>          }
>>          gen_exception_illegal(ctx);
>> -        break;
>> +        return;
>> +    }
>> +
>> +    if (fp_output) {
>> +        mark_fs_dirty(ctx);
>>      }
>>  }
>>
>> --
>> 2.14.3
>>
>>
>

Re: [Qemu-devel] [PATCH for-2.12 0/2] RISC-V: Mark FP status dirty

[Michael Clark]

On Tue, Mar 27, 2018 at 7:22 PM, Richard Henderson <
richard.henderson@linaro.org> wrote:

> Since it was my patch that broke FP state tracking in the
> first place, I feel obligated to fix it again.
>
> Mark mstatus[fs] as dirty whenever we write to the file.
> This can be optimized by only doing so once within a TB
> which initially began with a clean file.
>
> I have not yet put together an environment that can test
> this, so I'll need someone else to give it a go.
>

I have tested it with the simple test case running SMP Linux and it appears
okay (Note it must be compiled with -O2):

 http://oirase.annexia.org/tmp/sched.c

It is clearly broken without your two patches.

Re: [Qemu-devel] [PATCH 1/2] target/riscv: Split out mstatus_fs from tb_flags during translation

[Michael Clark]

On Tue, Mar 27, 2018 at 7:22 PM, Richard Henderson <
richard.henderson@linaro.org> wrote:

> We will want to track changes to mstatus_fs through the TB.
> As there is nothing else in tb_flags at the moment, remove
> the variable from DisasContext.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>

Reviewed-by: Michael Clark <mjc@sifive.com>

---
>  target/riscv/cpu.h       |  6 +++---
>  target/riscv/translate.c | 10 +++++-----
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 41e06ac0f9..d201dd3e90 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -269,8 +269,8 @@ void QEMU_NORETURN do_raise_exception_err(CPURISCVState
> *env,
>  target_ulong cpu_riscv_get_fflags(CPURISCVState *env);
>  void cpu_riscv_set_fflags(CPURISCVState *env, target_ulong);
>
> -#define TB_FLAGS_MMU_MASK  3
> -#define TB_FLAGS_FP_ENABLE MSTATUS_FS
> +#define TB_FLAGS_MMU_MASK   3
> +#define TB_FLAGS_MSTATUS_FS MSTATUS_FS
>
>  static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong
> *pc,
>                                          target_ulong *cs_base, uint32_t
> *flags)
> @@ -278,7 +278,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState
> *env, target_ulong *pc,
>      *pc = env->pc;
>      *cs_base = 0;
>  #ifdef CONFIG_USER_ONLY
> -    *flags = TB_FLAGS_FP_ENABLE;
> +    *flags = TB_FLAGS_MSTATUS_FS;
>  #else
>      *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
>  #endif
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 808eab7f50..a30724aa90 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -43,7 +43,7 @@ typedef struct DisasContext {
>      target_ulong pc;
>      target_ulong next_pc;
>      uint32_t opcode;
> -    uint32_t flags;
> +    uint32_t mstatus_fs;
>      uint32_t mem_idx;
>      int singlestep_enabled;
>      int bstate;
> @@ -665,7 +665,7 @@ static void gen_fp_load(DisasContext *ctx, uint32_t
> opc, int rd,
>  {
>      TCGv t0;
>
> -    if (!(ctx->flags & TB_FLAGS_FP_ENABLE)) {
> +    if (ctx->mstatus_fs == 0) {
>          gen_exception_illegal(ctx);
>          return;
>      }
> @@ -695,7 +695,7 @@ static void gen_fp_store(DisasContext *ctx, uint32_t
> opc, int rs1,
>  {
>      TCGv t0;
>
> -    if (!(ctx->flags & TB_FLAGS_FP_ENABLE)) {
> +    if (ctx->mstatus_fs == 0) {
>          gen_exception_illegal(ctx);
>          return;
>      }
> @@ -986,7 +986,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>  {
>      TCGv t0 = NULL;
>
> -    if (!(ctx->flags & TB_FLAGS_FP_ENABLE)) {
> +    if (ctx->mstatus_fs == 0) {
>          goto do_illegal;
>      }
>
> @@ -1862,8 +1862,8 @@ void gen_intermediate_code(CPUState *cs,
> TranslationBlock *tb)
>
>      ctx.tb = tb;
>      ctx.bstate = BS_NONE;
> -    ctx.flags = tb->flags;
>      ctx.mem_idx = tb->flags & TB_FLAGS_MMU_MASK;
> +    ctx.mstatus_fs = tb->flags & TB_FLAGS_MSTATUS_FS;
>      ctx.frm = -1;  /* unknown rounding mode */
>
>      num_insns = 0;
> --
> 2.14.3
>
>

Re: [Qemu-devel] [PATCH 2/2] target/riscv: Mark MSTATUS_FS dirty

[Michael Clark]

On Tue, Mar 27, 2018 at 7:22 PM, Richard Henderson <
richard.henderson@linaro.org> wrote:

> Writes to the FP register file mark the register file as dirty.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>

Reviewed-by: Michael Clark <mjc@sifive.com>
Tested-by: Michael Clark <mjc@sifive.com>


> ---
>  target/riscv/op_helper.c | 25 +++++++++++++++++--------
>  target/riscv/translate.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 56 insertions(+), 9 deletions(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index e34715df4e..74eeef0be8 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -72,11 +72,20 @@ void helper_raise_exception(CPURISCVState *env,
> uint32_t exception)
>      do_raise_exception_err(env, exception, 0);
>  }
>
> -static void validate_mstatus_fs(CPURISCVState *env, uintptr_t ra)
> +static void validate_mstatus_fs(CPURISCVState *env, uintptr_t ra, bool
> write)
>  {
>  #ifndef CONFIG_USER_ONLY
> -    if (!(env->mstatus & MSTATUS_FS)) {
> +    switch (get_field(env->mstatus, MSTATUS_FS)) {
> +    case 0: /* disabled */
>          do_raise_exception_err(env, RISCV_EXCP_ILLEGAL_INST, ra);
> +        g_assert_not_reached();
> +    case 1: /* initial */
> +    case 2: /* clean */
> +        if (write) {
> +            /* Mark fp status as dirty.  */
> +            env->mstatus = MSTATUS_FS;
> +        }
> +        break;
>      }
>  #endif
>  }
> @@ -96,15 +105,15 @@ void csr_write_helper(CPURISCVState *env,
> target_ulong val_to_write,
>
>      switch (csrno) {
>      case CSR_FFLAGS:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), true);
>          cpu_riscv_set_fflags(env, val_to_write & (FSR_AEXC >>
> FSR_AEXC_SHIFT));
>          break;
>      case CSR_FRM:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), true);
>          env->frm = val_to_write & (FSR_RD >> FSR_RD_SHIFT);
>          break;
>      case CSR_FCSR:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), true);
>          env->frm = (val_to_write & FSR_RD) >> FSR_RD_SHIFT;
>          cpu_riscv_set_fflags(env, (val_to_write & FSR_AEXC) >>
> FSR_AEXC_SHIFT);
>          break;
> @@ -379,13 +388,13 @@ target_ulong csr_read_helper(CPURISCVState *env,
> target_ulong csrno)
>
>      switch (csrno) {
>      case CSR_FFLAGS:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), false);
>          return cpu_riscv_get_fflags(env);
>      case CSR_FRM:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), false);
>          return env->frm;
>      case CSR_FCSR:
> -        validate_mstatus_fs(env, GETPC());
> +        validate_mstatus_fs(env, GETPC(), false);
>          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 */
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index a30724aa90..08fc42a679 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -660,6 +660,31 @@ static void gen_store(DisasContext *ctx, uint32_t
> opc, int rs1, int rs2,
>      tcg_temp_free(dat);
>  }
>
> +#ifndef CONFIG_USER_ONLY
> +/* The states of mstatus_fs are:
> + * 0 = disabled, 1 = initial, 2 = clean, 3 = dirty
> + * We will have already diagnosed disabled state,
> + * and need to turn initial/clean into dirty.
> + */
> +static void mark_fs_dirty(DisasContext *ctx)
> +{
> +    TCGv tmp;
> +    if (ctx->mstatus_fs == MSTATUS_FS) {
> +        return;
> +    }
> +    /* Remember the state change for the rest of the TB.  */
> +    ctx->mstatus_fs = MSTATUS_FS;
> +
> +    tmp = tcg_temp_new();
> +    tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
> +    tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS);
> +    tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
> +    tcg_temp_free(tmp);
> +}
> +#else
> +static inline void mark_fs_dirty(DisasContext *ctx) { }
> +#endif
> +
>  static void gen_fp_load(DisasContext *ctx, uint32_t opc, int rd,
>          int rs1, target_long imm)
>  {
> @@ -688,6 +713,8 @@ static void gen_fp_load(DisasContext *ctx, uint32_t
> opc, int rd,
>          break;
>      }
>      tcg_temp_free(t0);
> +
> +    mark_fs_dirty(ctx);
>  }
>
>  static void gen_fp_store(DisasContext *ctx, uint32_t opc, int rs1,
> @@ -985,6 +1012,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>                           int rs1, int rs2, int rm)
>  {
>      TCGv t0 = NULL;
> +    bool fp_output = true;
>
>      if (ctx->mstatus_fs == 0) {
>          goto do_illegal;
> @@ -1047,6 +1075,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          }
>          gen_set_gpr(rd, t0);
>          tcg_temp_free(t0);
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FCVT_W_S:
> @@ -1076,6 +1105,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          }
>          gen_set_gpr(rd, t0);
>          tcg_temp_free(t0);
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FCVT_S_W:
> @@ -1126,6 +1156,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          }
>          gen_set_gpr(rd, t0);
>          tcg_temp_free(t0);
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FMV_S_X:
> @@ -1218,6 +1249,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          }
>          gen_set_gpr(rd, t0);
>          tcg_temp_free(t0);
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FCVT_W_D:
> @@ -1247,6 +1279,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          }
>          gen_set_gpr(rd, t0);
>          tcg_temp_free(t0);
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FCVT_D_W:
> @@ -1294,6 +1327,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t
> opc, int rd,
>          default:
>              goto do_illegal;
>          }
> +        fp_output = false;
>          break;
>
>      case OPC_RISC_FMV_D_X:
> @@ -1310,7 +1344,11 @@ static void gen_fp_arith(DisasContext *ctx,
> uint32_t opc, int rd,
>              tcg_temp_free(t0);
>          }
>          gen_exception_illegal(ctx);
> -        break;
> +        return;
> +    }
> +
> +    if (fp_output) {
> +        mark_fs_dirty(ctx);
>      }
>  }
>
> --
> 2.14.3
>
>

Re: [Qemu-devel] [PATCH for-2.12 0/2] RISC-V: Mark FP status dirty

[Richard W.M. Jones]

On Wed, Mar 28, 2018 at 10:22:31AM +0800, Richard Henderson wrote:
> Since it was my patch that broke FP state tracking in the
> first place, I feel obligated to fix it again.

I missed this patch, thanks Michael Clark for pointing it out to me.

I've just tried it now using my test reproducer of the bug
(http://oirase.annexia.org/tmp/sched.c) and Fedora/RISC-V and it
appears to fix the problem.

> Mark mstatus[fs] as dirty whenever we write to the file.
> This can be optimized by only doing so once within a TB
> which initially began with a clean file.
> 
> I have not yet put together an environment that can test
> this, so I'll need someone else to give it a go.

This is the Fedora stage4 disk image which works fine under qemu from
git (make sure you read the readme.txt file first):

  https://fedorapeople.org/groups/risc-v/disk-images/

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org