Re: [PATCH v6 3/5] target/riscv: smstateen check for fcsr

[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"

> 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;                                        \