|
From: | Weiwei Li |
Subject: | Re: [PATCH v6 3/5] target/riscv: smstateen check for fcsr |
Date: | Mon, 25 Jul 2022 15:23:48 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 |
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 <mchitale@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 RISCVException write_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 RISCVException write_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 RISCVException write_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.inc b/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" Regards, Weiwei Li
Regards, Weiwei Lidiff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/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; \
[Prev in Thread] | Current Thread | [Next in Thread] |