[RFC PATCH 0/4] Smstateen FCSR implementation

[RFC PATCH 0/4] Smstateen FCSR implementation

[Mayuresh Chitale]

Patch 4 and 5 of the smstateen series need to be re-submitted with
changes described in the email below:
https://lists.nongnu.org/archive/html/qemu-riscv/2022-11/msg00155.html
Hence spliting the patch 4 of the original series into three patches and
re-submitting along with the original patch 5.

Mayuresh Chitale (4):
  target/riscv: smstateen check for fcsr
  target/riscv: Add fcsr field in tb->flags
  target/riscv: check smstateen fcsr flag
  target/riscv: smstateen knobs

 target/riscv/cpu.c                        |  3 ++-
 target/riscv/cpu.h                        |  4 ++++
 target/riscv/cpu_helper.c                 |  5 +++++
 target/riscv/csr.c                        | 25 ++++++++++++++++++++++-
 target/riscv/insn_trans/trans_rvf.c.inc   | 24 +++++++++++++++++++---
 target/riscv/insn_trans/trans_rvzfh.c.inc |  4 ++++
 target/riscv/translate.c                  |  2 ++
 7 files changed, 62 insertions(+), 5 deletions(-)

-- 
2.34.1

[RFC PATCH 1/4] target/riscv: smstateen check for fcsr

[Mayuresh Chitale]

If smstateen is implemented and sstateen0.fcsr is clear then the
floating point operations must return illegal instruction exception
or virtual instruction trap, if relevant.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/cpu.h |  3 +++
 target/riscv/csr.c | 25 ++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..132cf06ff2 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -613,6 +613,9 @@ void riscv_cpu_set_aia_ireg_rmw_fn(CPURISCVState *env, uint32_t priv,
                                                  target_ulong new_val,
                                                  target_ulong write_mask),
                                    void *rmw_fn_arg);
+RISCVException smstateen_acc_ok(CPURISCVState *env, int index, uint64_t bit);
+#else
+#define smstateen_acc_ok(env, index, bit) RISCV_EXCP_NONE
 #endif
 void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv);
 
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d522efc0b6..4979058434 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -43,7 +43,7 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
 
 /* Predicates */
 #if !defined(CONFIG_USER_ONLY)
-static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
+RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
                                        uint64_t bit)
 {
     bool virt = riscv_cpu_virt_enabled(env);
@@ -83,6 +83,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
         !riscv_cpu_cfg(env)->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;
 }
@@ -2056,6 +2060,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
                                       target_ulong new_val)
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
 
     return write_mstateen(env, csrno, wr_mask, new_val);
 }
@@ -2092,6 +2099,10 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_mstateenh(env, csrno, wr_mask, new_val);
 }
 
@@ -2129,6 +2140,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_hstateen(env, csrno, wr_mask, new_val);
 }
 
@@ -2168,6 +2183,10 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_hstateenh(env, csrno, wr_mask, new_val);
 }
 
@@ -2215,6 +2234,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_sstateen(env, csrno, wr_mask, new_val);
 }
 
-- 
2.34.1

[RFC PATCH 2/4] target/riscv: Add fcsr field in tb->flags

[Mayuresh Chitale]

The state of smstateen0.FCSR bit impacts the execution of floating point
instructions when misa.F==0. Add a field in the tb->flags which stores
the current state of smstateen0.fcsr and will be used by floating point
translation routines.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/cpu.h        | 1 +
 target/riscv/cpu_helper.c | 5 +++++
 target/riscv/translate.c  | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 132cf06ff2..9c6b10d29a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -653,6 +653,7 @@ FIELD(TB_FLAGS, VTA, 24, 1)
 FIELD(TB_FLAGS, VMA, 25, 1)
 /* Native debug itrigger */
 FIELD(TB_FLAGS, ITRIGGER, 26, 1)
+FIELD(TB_FLAGS, FCSR, 27, 1)
 
 #ifdef TARGET_RISCV32
 #define riscv_cpu_mxl(env)  ((void)(env), MXL_RV32)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..1590e6e480 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -117,6 +117,11 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
     if (env->cur_pmbase != 0) {
         flags = FIELD_DP32(flags, TB_FLAGS, PM_BASE_ENABLED, 1);
     }
+    if (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) == RISCV_EXCP_NONE) {
+        flags = FIELD_DP32(flags, TB_FLAGS, FCSR, 1);
+    } else {
+        flags = FIELD_DP32(flags, TB_FLAGS, FCSR, 0);
+    }
 
     *pflags = flags;
 }
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 0ee8ee147d..4880eaeb89 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -77,6 +77,7 @@ typedef struct DisasContext {
     int frm;
     RISCVMXL ol;
     bool virt_inst_excp;
+    bool smstateen_fcsr_ok;
     bool virt_enabled;
     const RISCVCPUConfig *cfg_ptr;
     bool hlsx;
@@ -1187,6 +1188,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
     ctx->zero = tcg_constant_tl(0);
     ctx->virt_inst_excp = false;
+    ctx->smstateen_fcsr_ok = FIELD_EX32(tb_flags, TB_FLAGS, FCSR);
 }
 
 static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
-- 
2.34.1

[RFC PATCH 3/4] target/riscv: check smstateen fcsr flag

[Mayuresh Chitale]

If misa.F and smstateen_fcsr_ok flag are clear then all the floating
point instructions must generate an appropriate exception.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/insn_trans/trans_rvf.c.inc   | 24 ++++++++++++++++++++---
 target/riscv/insn_trans/trans_rvzfh.c.inc |  4 ++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
index 052408f45c..6173dace46 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -24,9 +24,27 @@
             return false; \
 } while (0)
 
-#define REQUIRE_ZFINX_OR_F(ctx) do {\
-    if (!ctx->cfg_ptr->ext_zfinx) { \
-        REQUIRE_EXT(ctx, RVF); \
+#ifndef CONFIG_USER_ONLY
+#define smstateen_fcsr_check(ctx) do { \
+    if (!ctx->smstateen_fcsr_ok) { \
+        if (ctx->virt_enabled) { \
+            generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT); \
+        } else { \
+            generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); \
+        } \
+        return true; \
+    } \
+} while (0)
+#else
+#define smstateen_fcsr_check(ctx)
+#endif
+
+#define REQUIRE_ZFINX_OR_F(ctx) do { \
+    if (!has_ext(ctx, RVF)) { \
+        if (!ctx->cfg_ptr->ext_zfinx) { \
+            return false; \
+        } \
+        smstateen_fcsr_check(ctx); \
     } \
 } while (0)
 
diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc
index 74dde37ff7..304bee1002 100644
--- a/target/riscv/insn_trans/trans_rvzfh.c.inc
+++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
@@ -20,24 +20,28 @@
     if (!ctx->cfg_ptr->ext_zfh) {      \
         return false;         \
     }                         \
+    smstateen_fcsr_check(ctx); \
 } while (0)
 
 #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
     if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
         return false;                  \
     }                                  \
+    smstateen_fcsr_check(ctx); \
 } while (0)
 
 #define REQUIRE_ZFHMIN(ctx) do {              \
     if (!ctx->cfg_ptr->ext_zfhmin) {          \
         return false;                         \
     }                                         \
+    smstateen_fcsr_check(ctx); \
 } while (0)
 
 #define REQUIRE_ZFHMIN_OR_ZHINXMIN(ctx) do {                 \
     if (!(ctx->cfg_ptr->ext_zfhmin || ctx->cfg_ptr->ext_zhinxmin)) { \
         return false;                                        \
     }                                                        \
+    smstateen_fcsr_check(ctx); \
 } while (0)
 
 static bool trans_flh(DisasContext *ctx, arg_flh *a)
-- 
2.34.1

[RFC PATCH 4/4] target/riscv: smstateen knobs

[Mayuresh Chitale]

Add knobs to allow users to enable smstateen and also export it via the
ISA extension string.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
Reviewed-by: Weiwei Li<liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1e97473af2..4e926b0143 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -112,6 +112,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zhinx, true, PRIV_VERSION_1_12_0, ext_zhinx),
     ISA_EXT_DATA_ENTRY(zhinxmin, true, PRIV_VERSION_1_12_0, ext_zhinxmin),
     ISA_EXT_DATA_ENTRY(smaia, true, PRIV_VERSION_1_12_0, ext_smaia),
+    ISA_EXT_DATA_ENTRY(smstateen, true, PRIV_VERSION_1_12_0, ext_smstateen),
     ISA_EXT_DATA_ENTRY(ssaia, true, PRIV_VERSION_1_12_0, ext_ssaia),
     ISA_EXT_DATA_ENTRY(sscofpmf, true, PRIV_VERSION_1_12_0, ext_sscofpmf),
     ISA_EXT_DATA_ENTRY(sstc, true, PRIV_VERSION_1_12_0, ext_sstc),
@@ -1395,8 +1396,8 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
     DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
 
+    DEFINE_PROP_BOOL("smstateen", RISCVCPU, cfg.ext_smstateen, false),
     DEFINE_PROP_BOOL("svadu", RISCVCPU, cfg.ext_svadu, true),
-
     DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
     DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
     DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
-- 
2.34.1

Re: [RFC PATCH 3/4] target/riscv: check smstateen fcsr flag

[liweiwei]

On 2023/4/10 22:13, Mayuresh Chitale wrote:
> If misa.F and smstateen_fcsr_ok flag are clear then all the floating
> point instructions must generate an appropriate exception.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>   target/riscv/insn_trans/trans_rvf.c.inc   | 24 ++++++++++++++++++++---
>   target/riscv/insn_trans/trans_rvzfh.c.inc |  4 ++++
>   2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
> index 052408f45c..6173dace46 100644
> --- a/target/riscv/insn_trans/trans_rvf.c.inc
> +++ b/target/riscv/insn_trans/trans_rvf.c.inc
> @@ -24,9 +24,27 @@
>               return false; \
>   } while (0)
>   
> -#define REQUIRE_ZFINX_OR_F(ctx) do {\
> -    if (!ctx->cfg_ptr->ext_zfinx) { \
> -        REQUIRE_EXT(ctx, RVF); \
> +#ifndef CONFIG_USER_ONLY
> +#define smstateen_fcsr_check(ctx) do { \
> +    if (!ctx->smstateen_fcsr_ok) { \
> +        if (ctx->virt_enabled) { \
> +            generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT); \
> +        } else { \
> +            generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); \
> +        } \

We can setctx->virt_inst_excp = ctx->virt_enabledand return false here.

Or we need store current opcode to bins before generate_exception.

>   
> +        return true; \
> +    } \
> +} while (0)
> +#else
> +#define smstateen_fcsr_check(ctx)
> +#endif
> +
> +#define REQUIRE_ZFINX_OR_F(ctx) do { \
> +    if (!has_ext(ctx, RVF)) { \
> +        if (!ctx->cfg_ptr->ext_zfinx) { \
> +            return false; \
> +        } \
> +        smstateen_fcsr_check(ctx); \
>       } \
>   } while (0)
>   
> diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc
> index 74dde37ff7..304bee1002 100644
> --- a/target/riscv/insn_trans/trans_rvzfh.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
> @@ -20,24 +20,28 @@
>       if (!ctx->cfg_ptr->ext_zfh) {      \
>           return false;         \
>       }                         \
> +    smstateen_fcsr_check(ctx); \
>   } while (0)
>   
>   #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
>       if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
>           return false;                  \
>       }                                  \
> +    smstateen_fcsr_check(ctx); \

It's better to remain "\" alignment here.

Similar to following cases.

Regards,

Weiwei Li

>   } while (0)
>   
>   #define REQUIRE_ZFHMIN(ctx) do {              \
>       if (!ctx->cfg_ptr->ext_zfhmin) {          \
>           return false;                         \
>       }                                         \
> +    smstateen_fcsr_check(ctx); \
>   } while (0)
>   
>   #define REQUIRE_ZFHMIN_OR_ZHINXMIN(ctx) do {                 \
>       if (!(ctx->cfg_ptr->ext_zfhmin || ctx->cfg_ptr->ext_zhinxmin)) { \
>           return false;                                        \
>       }                                                        \
> +    smstateen_fcsr_check(ctx); \
>   } while (0)
>   
>   static bool trans_flh(DisasContext *ctx, arg_flh *a)

Re: [RFC PATCH 1/4] target/riscv: smstateen check for fcsr

[liweiwei]

On 2023/4/10 22:13, Mayuresh Chitale wrote:
> If smstateen is implemented and sstateen0.fcsr is clear then the
> floating point operations must return illegal instruction exception
> or virtual instruction trap, if relevant.

typo. sstateen0 -> smstateen

And fcsr bit only work when F is not enabled.

>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>   target/riscv/cpu.h |  3 +++
>   target/riscv/csr.c | 25 ++++++++++++++++++++++++-
>   2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 638e47c75a..132cf06ff2 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -613,6 +613,9 @@ void riscv_cpu_set_aia_ireg_rmw_fn(CPURISCVState *env, uint32_t priv,
>                                                    target_ulong new_val,
>                                                    target_ulong write_mask),
>                                      void *rmw_fn_arg);
> +RISCVException smstateen_acc_ok(CPURISCVState *env, int index, uint64_t bit);
This have been added in the latest riscv-to-apply.next.
> +#else
> +#define smstateen_acc_ok(env, index, bit) RISCV_EXCP_NONE

This seems unnecessary.  smstateen_acc_ok in this patch works only in 
system mode.

Regards,

Weiwei Li

>   #endif
>   void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv);
>   
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d522efc0b6..4979058434 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -43,7 +43,7 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
>   
>   /* Predicates */
>   #if !defined(CONFIG_USER_ONLY)
> -static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
> +RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
>                                          uint64_t bit)
>   {
>       bool virt = riscv_cpu_virt_enabled(env);
> @@ -83,6 +83,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
>           !riscv_cpu_cfg(env)->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;
>   }
> @@ -2056,6 +2060,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
>                                         target_ulong new_val)
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
>   
>       return write_mstateen(env, csrno, wr_mask, new_val);
>   }
> @@ -2092,6 +2099,10 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
>   
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
> +
>       return write_mstateenh(env, csrno, wr_mask, new_val);
>   }
>   
> @@ -2129,6 +2140,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
>   
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
> +
>       return write_hstateen(env, csrno, wr_mask, new_val);
>   }
>   
> @@ -2168,6 +2183,10 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
>   
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
> +
>       return write_hstateenh(env, csrno, wr_mask, new_val);
>   }
>   
> @@ -2215,6 +2234,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
>   
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
> +
>       return write_sstateen(env, csrno, wr_mask, new_val);
>   }
>   

Re: [RFC PATCH 2/4] target/riscv: Add fcsr field in tb->flags

[Richard Henderson]

On 4/10/23 07:13, Mayuresh Chitale wrote:
> The state of smstateen0.FCSR bit impacts the execution of floating point
> instructions when misa.F==0. Add a field in the tb->flags which stores
> the current state of smstateen0.fcsr and will be used by floating point
> translation routines.

Are you certain that you require a new bit?

Could the same effect be achieved by forcing one or more of the existing 
TB_FLAGS.{FS,HS_FS} fields to 0 within cpu_get_tb_cpu_state?  I.e. for the purposes of 
translation, pretend the FS state is DISABLED?

These bits are scarce, are we are nearly out.


r~

Re: [RFC PATCH 3/4] target/riscv: check smstateen fcsr flag

[Richard Henderson]

On 4/10/23 07:13, Mayuresh Chitale wrote:
> +#ifndef CONFIG_USER_ONLY
> +#define smstateen_fcsr_check(ctx) do { \
> +    if (!ctx->smstateen_fcsr_ok) { \
> +        if (ctx->virt_enabled) { \
> +            generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT); \
> +        } else { \
> +            generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); \
> +        } \
> +        return true; \
> +    } \
> +} while (0)
> +#else
> +#define smstateen_fcsr_check(ctx)
> +#endif
> +
> +#define REQUIRE_ZFINX_OR_F(ctx) do { \
> +    if (!has_ext(ctx, RVF)) { \
> +        if (!ctx->cfg_ptr->ext_zfinx) { \
> +            return false; \
> +        } \
> +        smstateen_fcsr_check(ctx); \
>       } \
>   } while (0)

As a matter of style, I strongly object to a *nested* macro returning from the calling 
function.  These should all be changed to normal functions of the form

     if (!require_xyz(ctx) || !require_abc(ctx)) {
         return something;
     }

etc.  insn_trans/trans_rvv.c.inc is much much cleaner in this respect.


r~

Re: [RFC PATCH 1/4] target/riscv: smstateen check for fcsr

[Mayuresh Chitale]

On Mon, Apr 10, 2023 at 8:14 PM liweiwei <liweiwei@iscas.ac.cn> wrote:
>
>
> On 2023/4/10 22:13, Mayuresh Chitale wrote:
> > If smstateen is implemented and sstateen0.fcsr is clear then the
> > floating point operations must return illegal instruction exception
> > or virtual instruction trap, if relevant.
>
> typo. sstateen0 -> smstateen
Ok.
>
> And fcsr bit only work when F is not enabled.
Will fix it.
>
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >   target/riscv/cpu.h |  3 +++
> >   target/riscv/csr.c | 25 ++++++++++++++++++++++++-
> >   2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 638e47c75a..132cf06ff2 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -613,6 +613,9 @@ void riscv_cpu_set_aia_ireg_rmw_fn(CPURISCVState *env, uint32_t priv,
> >                                                    target_ulong new_val,
> >                                                    target_ulong write_mask),
> >                                      void *rmw_fn_arg);
> > +RISCVException smstateen_acc_ok(CPURISCVState *env, int index, uint64_t bit);
> This have been added in the latest riscv-to-apply.next.
> > +#else
> > +#define smstateen_acc_ok(env, index, bit) RISCV_EXCP_NONE
>
> This seems unnecessary.  smstateen_acc_ok in this patch works only in
> system mode.
Ok.
>
> Regards,
>
> Weiwei Li
>
> >   #endif
> >   void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv);
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index d522efc0b6..4979058434 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -43,7 +43,7 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
> >
> >   /* Predicates */
> >   #if !defined(CONFIG_USER_ONLY)
> > -static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
> > +RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
> >                                          uint64_t bit)
> >   {
> >       bool virt = riscv_cpu_virt_enabled(env);
> > @@ -83,6 +83,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
> >           !riscv_cpu_cfg(env)->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;
> >   }
> > @@ -2056,6 +2060,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
> >                                         target_ulong new_val)
> >   {
> >       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> >
> >       return write_mstateen(env, csrno, wr_mask, new_val);
> >   }
> > @@ -2092,6 +2099,10 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
> >   {
> >       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> > +
> >       return write_mstateenh(env, csrno, wr_mask, new_val);
> >   }
> >
> > @@ -2129,6 +2140,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
> >   {
> >       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> > +
> >       return write_hstateen(env, csrno, wr_mask, new_val);
> >   }
> >
> > @@ -2168,6 +2183,10 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
> >   {
> >       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> > +
> >       return write_hstateenh(env, csrno, wr_mask, new_val);
> >   }
> >
> > @@ -2215,6 +2234,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
> >   {
> >       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> > +
> >       return write_sstateen(env, csrno, wr_mask, new_val);
> >   }
> >
>

Re: [RFC PATCH 3/4] target/riscv: check smstateen fcsr flag

[Mayuresh Chitale]

On Mon, Apr 10, 2023 at 8:00 PM liweiwei <liweiwei@iscas.ac.cn> wrote:
>
>
> On 2023/4/10 22:13, Mayuresh Chitale wrote:
> > If misa.F and smstateen_fcsr_ok flag are clear then all the floating
> > point instructions must generate an appropriate exception.
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >   target/riscv/insn_trans/trans_rvf.c.inc   | 24 ++++++++++++++++++++---
> >   target/riscv/insn_trans/trans_rvzfh.c.inc |  4 ++++
> >   2 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
> > index 052408f45c..6173dace46 100644
> > --- a/target/riscv/insn_trans/trans_rvf.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvf.c.inc
> > @@ -24,9 +24,27 @@
> >               return false; \
> >   } while (0)
> >
> > -#define REQUIRE_ZFINX_OR_F(ctx) do {\
> > -    if (!ctx->cfg_ptr->ext_zfinx) { \
> > -        REQUIRE_EXT(ctx, RVF); \
> > +#ifndef CONFIG_USER_ONLY
> > +#define smstateen_fcsr_check(ctx) do { \
> > +    if (!ctx->smstateen_fcsr_ok) { \
> > +        if (ctx->virt_enabled) { \
> > +            generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT); \
> > +        } else { \
> > +            generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); \
> > +        } \
>
> We can setctx->virt_inst_excp = ctx->virt_enabledand return false here.
Ok.
>
> Or we need store current opcode to bins before generate_exception.
>
> >
> > +        return true; \
> > +    } \
> > +} while (0)
> > +#else
> > +#define smstateen_fcsr_check(ctx)
> > +#endif
> > +
> > +#define REQUIRE_ZFINX_OR_F(ctx) do { \
> > +    if (!has_ext(ctx, RVF)) { \
> > +        if (!ctx->cfg_ptr->ext_zfinx) { \
> > +            return false; \
> > +        } \
> > +        smstateen_fcsr_check(ctx); \
> >       } \
> >   } while (0)
> >
> > diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc
> > index 74dde37ff7..304bee1002 100644
> > --- a/target/riscv/insn_trans/trans_rvzfh.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
> > @@ -20,24 +20,28 @@
> >       if (!ctx->cfg_ptr->ext_zfh) {      \
> >           return false;         \
> >       }                         \
> > +    smstateen_fcsr_check(ctx); \
> >   } while (0)
> >
> >   #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
> >       if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
> >           return false;                  \
> >       }                                  \
> > +    smstateen_fcsr_check(ctx); \
>
> It's better to remain "\" alignment here.
Ok.

>
> Similar to following cases.
>
> Regards,
>
> Weiwei Li
>
> >   } while (0)
> >
> >   #define REQUIRE_ZFHMIN(ctx) do {              \
> >       if (!ctx->cfg_ptr->ext_zfhmin) {          \
> >           return false;                         \
> >       }                                         \
> > +    smstateen_fcsr_check(ctx); \
> >   } while (0)
> >
> >   #define REQUIRE_ZFHMIN_OR_ZHINXMIN(ctx) do {                 \
> >       if (!(ctx->cfg_ptr->ext_zfhmin || ctx->cfg_ptr->ext_zhinxmin)) { \
> >           return false;                                        \
> >       }                                                        \
> > +    smstateen_fcsr_check(ctx); \
> >   } while (0)
> >
> >   static bool trans_flh(DisasContext *ctx, arg_flh *a)
>

Re: [RFC PATCH 3/4] target/riscv: check smstateen fcsr flag

[Mayuresh Chitale]

On Tue, Apr 11, 2023 at 7:23 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/10/23 07:13, Mayuresh Chitale wrote:
> > +#ifndef CONFIG_USER_ONLY
> > +#define smstateen_fcsr_check(ctx) do { \
> > +    if (!ctx->smstateen_fcsr_ok) { \
> > +        if (ctx->virt_enabled) { \
> > +            generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT); \
> > +        } else { \
> > +            generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); \
> > +        } \
> > +        return true; \
> > +    } \
> > +} while (0)
> > +#else
> > +#define smstateen_fcsr_check(ctx)
> > +#endif
> > +
> > +#define REQUIRE_ZFINX_OR_F(ctx) do { \
> > +    if (!has_ext(ctx, RVF)) { \
> > +        if (!ctx->cfg_ptr->ext_zfinx) { \
> > +            return false; \
> > +        } \
> > +        smstateen_fcsr_check(ctx); \
> >       } \
> >   } while (0)
>
> As a matter of style, I strongly object to a *nested* macro returning from the calling
> function.  These should all be changed to normal functions of the form
>
>      if (!require_xyz(ctx) || !require_abc(ctx)) {
>          return something;
>      }
>
> etc.  insn_trans/trans_rvv.c.inc is much much cleaner in this respect.
Ok. I will change smstateen_fcsr_check to a function.
>
>
> r~

Re: [RFC PATCH 2/4] target/riscv: Add fcsr field in tb->flags

[Mayuresh Chitale]

On Tue, Apr 11, 2023 at 7:17 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/10/23 07:13, Mayuresh Chitale wrote:
> > The state of smstateen0.FCSR bit impacts the execution of floating point
> > instructions when misa.F==0. Add a field in the tb->flags which stores
> > the current state of smstateen0.fcsr and will be used by floating point
> > translation routines.
>
> Are you certain that you require a new bit?
>
> Could the same effect be achieved by forcing one or more of the existing
> TB_FLAGS.{FS,HS_FS} fields to 0 within cpu_get_tb_cpu_state?  I.e. for the purposes of
> translation, pretend the FS state is DISABLED?
Yes, that is correct.
>
> These bits are scarce, are we are nearly out.
>
>
> r~
>