| Message ID | 20230428165212.2800669-3-mchitale@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Smstateen FCSR | expand |
On 2023/4/29 00:52, Mayuresh Chitale wrote: > When misa.F is 0 tb->flags.FS field is unused and can be used to save > the current state of smstateen0.FCSR check which is needed by the > floating point translation routines. > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> > --- Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn> Weiwei Li > target/riscv/cpu_helper.c | 9 +++++++++ > target/riscv/translate.c | 12 +++++++++++- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index b68dcfe7b6..126ac221a0 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -119,6 +119,15 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc, > vs = MIN(vs, get_field(env->mstatus_hs, MSTATUS_VS)); > } > > + /* > + * If misa.F is 0 then the FS field of the tb->flags can be used to pass > + * the current state of the smstateen.FCSR bit which must be checked for > + * in the floating point translation routines. > + */ > + if (!riscv_has_ext(env, RVF)) { > + fs = (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) == RISCV_EXCP_NONE); > + } > + > if (cpu->cfg.debug && !icount_enabled()) { > flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled); > } > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 928da0d3f0..74f624aa62 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -78,6 +78,7 @@ typedef struct DisasContext { > int frm; > RISCVMXL ol; > bool virt_inst_excp; > + bool smstateen_fcsr_ok; > bool virt_enabled; > const RISCVCPUConfig *cfg_ptr; > /* vector extension */ > @@ -1155,7 +1156,11 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > ctx->pc_succ_insn = ctx->base.pc_first; > ctx->priv = FIELD_EX32(tb_flags, TB_FLAGS, PRIV); > ctx->mem_idx = FIELD_EX32(tb_flags, TB_FLAGS, MEM_IDX); > - ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS); > + if (has_ext(ctx, RVF)) { > + ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS); > + } else { > + ctx->mstatus_fs = 0; > + } > ctx->mstatus_vs = FIELD_EX32(tb_flags, TB_FLAGS, VS); > ctx->priv_ver = env->priv_ver; > ctx->virt_enabled = FIELD_EX32(tb_flags, TB_FLAGS, VIRT_ENABLED); > @@ -1178,6 +1183,11 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER); > ctx->zero = tcg_constant_tl(0); > ctx->virt_inst_excp = false; > + if (has_ext(ctx, RVF) || !cpu->cfg.ext_smstateen) { > + ctx->smstateen_fcsr_ok = 1; > + } else { > + ctx->smstateen_fcsr_ok = FIELD_EX32(tb_flags, TB_FLAGS, FS); > + } > } > > static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
On 4/28/23 17:52, Mayuresh Chitale wrote: > When misa.F is 0 tb->flags.FS field is unused and can be used to save > the current state of smstateen0.FCSR check which is needed by the > floating point translation routines. > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> > --- > target/riscv/cpu_helper.c | 9 +++++++++ > target/riscv/translate.c | 12 +++++++++++- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index b68dcfe7b6..126ac221a0 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -119,6 +119,15 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc, > vs = MIN(vs, get_field(env->mstatus_hs, MSTATUS_VS)); > } > > + /* > + * If misa.F is 0 then the FS field of the tb->flags can be used to pass > + * the current state of the smstateen.FCSR bit which must be checked for > + * in the floating point translation routines. > + */ > + if (!riscv_has_ext(env, RVF)) { > + fs = (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) == RISCV_EXCP_NONE); > + } You have misunderstood my suggestion: /* With Zfinx, floating point is enabled/disabled by Smstateen. */ if (!riscv_has_ext(env, RVF)) { fs = (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED); } > + bool smstateen_fcsr_ok; Not needed. > - ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS); > + if (has_ext(ctx, RVF)) { > + ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS); > + } else { > + ctx->mstatus_fs = 0; > + } Not needed. In patch 3, which should be merged with this, there are no changes to REQUIRE_ZFINX_OR_F, no additional smstateen_fcsr_check, and REQUIRE_FPU reduces to #define REQUIRE_FPU do { \ if (ctx->mstatus_fs == EXT_STATUS_DISABLED) { \ return false; \ } \ } while (0) This makes the DisasContext version of fs be the single gate for floating point. No extra checks required. r~
On 2023/4/29 16:54, Richard Henderson wrote: > On 4/28/23 17:52, Mayuresh Chitale wrote: >> When misa.F is 0 tb->flags.FS field is unused and can be used to save >> the current state of smstateen0.FCSR check which is needed by the >> floating point translation routines. >> >> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> >> --- >> target/riscv/cpu_helper.c | 9 +++++++++ >> target/riscv/translate.c | 12 +++++++++++- >> 2 files changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >> index b68dcfe7b6..126ac221a0 100644 >> --- a/target/riscv/cpu_helper.c >> +++ b/target/riscv/cpu_helper.c >> @@ -119,6 +119,15 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, >> target_ulong *pc, >> vs = MIN(vs, get_field(env->mstatus_hs, MSTATUS_VS)); >> } >> + /* >> + * If misa.F is 0 then the FS field of the tb->flags can be used >> to pass >> + * the current state of the smstateen.FCSR bit which must be >> checked for >> + * in the floating point translation routines. >> + */ >> + if (!riscv_has_ext(env, RVF)) { >> + fs = (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) == >> RISCV_EXCP_NONE); >> + } > > You have misunderstood my suggestion: > > /* With Zfinx, floating point is enabled/disabled by Smstateen. */ > if (!riscv_has_ext(env, RVF)) { > fs = (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) > ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED); > } > >> + bool smstateen_fcsr_ok; > > Not needed. > >> - ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS); >> + if (has_ext(ctx, RVF)) { >> + ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS); >> + } else { >> + ctx->mstatus_fs = 0; >> + } > > Not needed. > > In patch 3, which should be merged with this, there are no changes to > REQUIRE_ZFINX_OR_F, no additional smstateen_fcsr_check, and > REQUIRE_FPU reduces to > > #define REQUIRE_FPU do { \ > if (ctx->mstatus_fs == EXT_STATUS_DISABLED) { \ > return false; \ > } \ > } while (0) > > This makes the DisasContext version of fs be the single gate for > floating point. > No extra checks required. Yeah. It's better to merge with REQUIRE_FPU. However, virtual instruction exception will be triggered in VS/VU mode if smstateen check fails, which is different from normal REQUIRE_FPU. So, we may need to modify REQUIRE_FPU to distinguish them: #define REQUIRE_FPU do { \ if (ctx->mstatus_fs == EXT_STATUS_DISABLED) { \ ctx->virt_inst_excp = ctx->virt_enabled && ctx->cfg_ptr->ext_zfinx; \ return false; \ } \ } while (0) Regards, Weiwei Li > > > r~
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index b68dcfe7b6..126ac221a0 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -119,6 +119,15 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc, vs = MIN(vs, get_field(env->mstatus_hs, MSTATUS_VS)); } + /* + * If misa.F is 0 then the FS field of the tb->flags can be used to pass + * the current state of the smstateen.FCSR bit which must be checked for + * in the floating point translation routines. + */ + if (!riscv_has_ext(env, RVF)) { + fs = (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) == RISCV_EXCP_NONE); + } + if (cpu->cfg.debug && !icount_enabled()) { flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled); } diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 928da0d3f0..74f624aa62 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -78,6 +78,7 @@ typedef struct DisasContext { int frm; RISCVMXL ol; bool virt_inst_excp; + bool smstateen_fcsr_ok; bool virt_enabled; const RISCVCPUConfig *cfg_ptr; /* vector extension */ @@ -1155,7 +1156,11 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) ctx->pc_succ_insn = ctx->base.pc_first; ctx->priv = FIELD_EX32(tb_flags, TB_FLAGS, PRIV); ctx->mem_idx = FIELD_EX32(tb_flags, TB_FLAGS, MEM_IDX); - ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS); + if (has_ext(ctx, RVF)) { + ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS); + } else { + ctx->mstatus_fs = 0; + } ctx->mstatus_vs = FIELD_EX32(tb_flags, TB_FLAGS, VS); ctx->priv_ver = env->priv_ver; ctx->virt_enabled = FIELD_EX32(tb_flags, TB_FLAGS, VIRT_ENABLED); @@ -1178,6 +1183,11 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER); ctx->zero = tcg_constant_tl(0); ctx->virt_inst_excp = false; + if (has_ext(ctx, RVF) || !cpu->cfg.ext_smstateen) { + ctx->smstateen_fcsr_ok = 1; + } else { + ctx->smstateen_fcsr_ok = FIELD_EX32(tb_flags, TB_FLAGS, FS); + } } static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
When misa.F is 0 tb->flags.FS field is unused and can be used to save the current state of smstateen0.FCSR check which is needed by the floating point translation routines. Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> --- target/riscv/cpu_helper.c | 9 +++++++++ target/riscv/translate.c | 12 +++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-)