在 2022/7/28 下午2:15, Mayuresh Chitale 写道:
On Mon, 2022-07-25 at 15:23 +0800, Weiwei Li wrote:在 2022/7/24 下午11:49, Mayuresh Chitale 写道:On Fri, 2022-07-22 at 09:42 +0800, Weiwei Li wrote:在 2022/7/21 下午11:31, Mayuresh Chitale 写道:If smstateen is implemented and sstateen0.fcsr is clear then the floating point operations must return illegal instruction exception. Signed-off-by: Mayuresh Chitale <mchit...@ventanamicro.com> --- target/riscv/csr.c | 23 ++++++++++++++ target/riscv/insn_trans/trans_rvf.c.inc | 38 +++++++++++++++++++++-- target/riscv/insn_trans/trans_rvzfh.c.inc | 4 +++ 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/target/riscv/csr.c b/target/riscv/csr.c index ab06b117f9..a597b6cbc7 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -96,6 +96,10 @@ static RISCVException fs(CPURISCVState *env, int csrno) !RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { return RISCV_EXCP_ILLEGAL_INST; } + + if (!env->debugger && !riscv_cpu_fp_enabled(env)) { + return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR); + } #endif return RISCV_EXCP_NONE; } @@ -1876,6 +1880,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno, target_ulong new_val) { uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG; + if (!riscv_has_ext(env, RVF)) { + wr_mask |= SMSTATEEN0_FCSR; + }return write_mstateen(env, csrno, wr_mask, new_val);} @@ -1924,6 +1931,10 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno, { uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;+ if (!riscv_has_ext(env, RVF)) {+ wr_mask |= SMSTATEEN0_FCSR; + } + return write_mstateenh(env, csrno, wr_mask, new_val); }@@ -1973,6 +1984,10 @@ static RISCVExceptionwrite_hstateen0(CPURISCVState *env, int csrno, { uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;+ if (!riscv_has_ext(env, RVF)) {+ wr_mask |= SMSTATEEN0_FCSR; + } + return write_hstateen(env, csrno, wr_mask, new_val); }@@ -2024,6 +2039,10 @@ static RISCVExceptionwrite_hstateen0h(CPURISCVState *env, int csrno, { uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;+ if (!riscv_has_ext(env, RVF)) {+ wr_mask |= SMSTATEEN0_FCSR; + } + return write_hstateenh(env, csrno, wr_mask, new_val); }@@ -2083,6 +2102,10 @@ static RISCVExceptionwrite_sstateen0(CPURISCVState *env, int csrno, { uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;+ if (!riscv_has_ext(env, RVF)) {+ wr_mask |= SMSTATEEN0_FCSR; + } + return write_sstateen(env, csrno, wr_mask, new_val); }diff --git a/target/riscv/insn_trans/trans_rvf.c.incb/target/riscv/insn_trans/trans_rvf.c.inc index a1d3eb52ad..c43c48336b 100644 --- a/target/riscv/insn_trans/trans_rvf.c.inc +++ b/target/riscv/insn_trans/trans_rvf.c.inc @@ -24,9 +24,43 @@ return false; \ } while (0)+#ifndef CONFIG_USER_ONLY+#define SMSTATEEN_CHECK(ctx) do {\ + CPUState *cpu = ctx->cs; \ + CPURISCVState *env = cpu->env_ptr; \ + if (ctx->cfg_ptr->ext_smstateen && \ + (env->priv < PRV_M)) { \ + uint64_t stateen = env->mstateen[0]; \ + uint64_t hstateen = env->hstateen[0]; \ + uint64_t sstateen = env->sstateen[0]; \ + if (!(stateen & SMSTATEEN_STATEN)) {\ + hstateen = 0; \ + sstateen = 0; \ + } \ + if (ctx->virt_enabled) { \ + stateen &= hstateen; \ + if (!(hstateen & SMSTATEEN_STATEN)) {\ + sstateen = 0; \ + } \ + } \ + if (env->priv == PRV_U && has_ext(ctx, RVS)) {\eventually meaning + stateen &= sstateen; \ + } \ + if (!(stateen & SMSTATEEN0_FCSR)) { \ + return false; \ + } \ + } \ +} while (0)It's better to add a space before '\'.ok. will modify in the next version.+#else +#define SMSTATEEN_CHECK(ctx) +#endif + #define REQUIRE_ZFINX_OR_F(ctx) do {\ - if (!ctx->cfg_ptr->ext_zfinx) { \ - REQUIRE_EXT(ctx, RVF); \ + if (!has_ext(ctx, RVF)) { \ + SMSTATEEN_CHECK(ctx); \ + if (!ctx->cfg_ptr->ext_zfinx) { \ + return false; \ + } \ } \ } while (0)SMSTATEEN_CHECK is for CSR. and REQUIRE_ZFINX_OR_F is for Extension. I think It's better to separate them. By the way, if we want the smallest modification for current code, adding it to REQUIRE_FPU seems better.Actually REQUIRE_FPU is checking for mstatus.fs but as per smstateen spec we need to check for misa.f which is done in REQUIRE_ZFINX_OR_F.OK. It's acceptable to me even though I prefer separating them. However, I find another question in SMSTATEEN_CHECK: when access is disallowed by Xstateen.FCSR, it's always return false which will trigger illegal instruction exception finally. However, this exception is triggered by accessing fcsr CSR which may trigger illegal instruction trap and virtual instruction trap in different situation. "For convenience, when the stateen CSRs are implemented and misa.F = 0, then if bit 1 of a controlling stateen0 CSR is zero, all floating-point instructions cause an illegal instruction trap (or virtual instruction trap, if relevant), as though they all access fcsr, regardless of whether they really do." And stateen.fcsr is only work when zfinx is enabled, so it's better to SMSTATEEN_CHECK(ctx) after check for "!ctx->cfg_ptr->ext_zfinx"Actually the spec refers only to misa.F and not zfinx and regarding the fcsr its : "as though they all access fcsr, regardless of whether they really do"
Yeah, they are triggered by accessing fcsr. So they should take the same check as accessing fcsr here.
In above predicate fs for fcsr, if misa.F is zero and zfinx is not supported,illegal instruction fault is triggered.
Otherwise, stateen related check is done when misa.F is zero and may trigger illegal/virtual instruction fault.
both of this two cases are different in above check.I also sent related questions in https://github.com/riscv/riscv-state-enable/issues/9.
Any comments are welcome. Regards, Weiwei Li
Regards, Weiwei LiRegards, Weiwei Lidiff --git a/target/riscv/insn_trans/trans_rvzfh.c.incb/target/riscv/insn_trans/trans_rvzfh.c.inc index 5d07150cd0..b165ea9d58 100644 --- a/target/riscv/insn_trans/trans_rvzfh.c.inc +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc @@ -17,24 +17,28 @@ */#define REQUIRE_ZFH(ctx) do { \+ SMSTATEEN_CHECK(ctx); \ if (!ctx->cfg_ptr->ext_zfh) { \ return false; \ } \ } while (0)#define REQUIRE_ZHINX_OR_ZFH(ctx) do { \+ SMSTATEEN_CHECK(ctx); \ if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \ return false; \ } \ } while (0)#define REQUIRE_ZFH_OR_ZFHMIN(ctx) do { \+ SMSTATEEN_CHECK(ctx); \ if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \ return false; \ } \ } while (0)#define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do {\ + SMSTATEEN_CHECK(ctx); \ if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin || \ ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr-ext_zhinxmin)){ \ return false; \
