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

[Weiwei Li]

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