Re: [Qemu-devel] [PATCH v1 12/30] RISC-V: Mark mstatus.fs dirty

[Alistair Francis]

On Tue, May 22, 2018 at 5:14 PM, Michael Clark <address@hidden> wrote:
> From: Richard Henderson <address@hidden>
>
> Modifed from Richard Henderson's patch [1] to integrate
> with the new control and status register implementation.
>
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg07034.html
>
> Note: the f* CSRs already mark mstatus.FS dirty using
> env->mstatus |= mstatus.FS so the bug in the first
> spin of this patch has been fixed in a prior commit.
>
> Cc: Sagar Karandikar <address@hidden>
> Cc: Bastian Koppelmann <address@hidden>
> Cc: Palmer Dabbelt <address@hidden>
> Cc: Alistair Francis <address@hidden>
> Cc: Richard Henderson <address@hidden>
> Signed-off-by: Michael Clark <address@hidden>
> Reviewed-by: Michael Clark <address@hidden>

It sounds like this should still have Richard's SOB line.

Once that is fixed:

Reviewed-by: Alistair Francis <address@hidden>

Alistair

>
> Co-authored-by: Richard Henderson <address@hidden>
> Co-authored-by: Michael Clark <address@hidden>
> ---
>  target/riscv/csr.c       | 12 ------------
>  target/riscv/translate.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0e6c0c365154..b4452388ff02 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -325,18 +325,6 @@ static int write_mstatus(CPURISCVState *env, int csrno, 
> target_ulong val)
>
>      mstatus = (mstatus & ~mask) | (val & mask);
>
> -    /* Note: this is a workaround for an issue where mstatus.FS
> -       does not report dirty after floating point operations
> -       that modify floating point state. This workaround is
> -       technically compliant with the RISC-V Privileged
> -       specification as it is legal to return only off, or dirty.
> -       at the expense of extra floating point save/restore. */
> -
> -    /* FP is always dirty or off */
> -    if (mstatus & MSTATUS_FS) {
> -        mstatus |= MSTATUS_FS;
> -    }
> -
>      int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
>                  ((mstatus & MSTATUS_XS) == MSTATUS_XS);
>      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 466b9551cbd9..a980611eb611 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -651,6 +651,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)
>  {
> @@ -679,6 +704,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,
> @@ -944,6 +971,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;
> @@ -1006,6 +1034,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:
> @@ -1035,6 +1064,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:
> @@ -1085,6 +1115,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:
> @@ -1177,6 +1208,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:
> @@ -1206,6 +1238,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:
> @@ -1253,6 +1286,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:
> @@ -1269,7 +1303,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.7.0
>
>