| Message ID | 20220721153136.377578-4-mchitale@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V Smstateen support | expand |
在 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)) {\ > + stateen &= sstateen; \ > + } \ > + if (!(stateen & SMSTATEEN0_FCSR)) { \ > + return false; \ > + } \ > + } \ > +} while (0) It's better to add a space before '\'. > +#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. Regards, Weiwei Li > > diff --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; \
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. > Regards, > Weiwei Li > > > > > diff --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; \
在 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 <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 Li >> >>> >>> diff --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; \
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 <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" 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" > Regards, > Weiwei Li > > > Regards, > > > Weiwei Li > > > > > > > > > > > diff --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; \
在 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 <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" > 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 Li >>>> Regards, >>>> Weiwei Li >>>> >>>>> >>>>> diff --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; \
On 28/07/2022 07:15, Mayuresh Chitale wrote: > 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 <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; \ >>>>> + } \ >>>>> + } \ given the size of that I would have thought an "static inline" function would be easier to write and maintain for SMSTATEEN_CHECK
On Thu, 2022-07-28 at 09:09 +0100, Ben Dooks wrote: > On 28/07/2022 07:15, Mayuresh Chitale wrote: > > 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 <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; \ > > > > > > + } \ > > > > > > + } \ > > given the size of that I would have thought an "static inline" > function would be easier to write and maintain for SMSTATEEN_CHECK Ok. I will update in the next version. > >
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)) {\ + stateen &= sstateen; \ + } \ + if (!(stateen & SMSTATEEN0_FCSR)) { \ + return false; \ + } \ + } \ +} while (0) +#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) diff --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; \
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(-)