Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

Mon, 10 Oct 2022 08:11:16 -0700 


On 2022/10/7 01:06, mchit...@ventanamicro.com wrote:

On Tue, 2022-10-04 at 21:23 +0800, weiwei wrote:



On 2022/10/4 14:51, mchit...@ventanamicro.com wrote:

On Mon, 2022-10-03 at 21:02 +0800, weiwei wrote:

On 2022/10/3 19:47, 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.
Signed-off-by: Mayuresh Chitale
<mchit...@ventanamicro.com>
---
   target/riscv/csr.c                        | 23 ++++++++++++
   target/riscv/insn_trans/trans_rvf.c.inc   | 43
+++++++++++++++++++++--
   target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++++++
   3 files changed, 75 insertions(+), 3 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 71236f2b5d..8b25f885ec 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -84,6 +84,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;
   }
@@ -2023,6 +2027,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);

   }
@@ -2059,6 +2066,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);
   }

   
@@ -2096,6 +2107,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);
   }

   
@@ -2135,6 +2150,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);
   }

   
@@ -2182,6 +2201,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);
   }

   
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc

b/target/riscv/insn_trans/trans_rvf.c.inc
index a1d3eb52ad..93657680c6 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -24,9 +24,46 @@
               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
+static inline bool smstateen_fcsr_check(DisasContext *ctx, int
index)
+{
+    CPUState *cpu = ctx->cs;
+    CPURISCVState *env = cpu->env_ptr;
+    uint64_t stateen = env->mstateen[index];
+
+    if (!ctx->cfg_ptr->ext_smstateen || env->priv == PRV_M) {
+        return true;
+    }
+
+    if (ctx->virt_enabled) {
+        stateen &= env->hstateen[index];
+    }
+
+    if (env->priv == PRV_U && has_ext(ctx, RVS)) {
+        stateen &= env->sstateen[index];
+    }
+
+    if (!(stateen & SMSTATEEN0_FCSR)) {
+        if (ctx->virt_enabled) {
+            ctx->virt_inst_excp = true;
+        }

Are there any considerations for adding  virt_inst_excp instead of
directly
"generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)" here?
Regards,
Weiwei Li

I had considered it but I think this is a simpler approach as the rest
of the code path stays the same as generating an illegal instruction
exception, for e.g setting the bins value (tval).



OK,  we did need to set bins value for virt instruction exception. 
However I prefer directly call a


new gen_exception_virt function(similar togen_exception_illegal) here.


  Also we need to
return true from all the caller trans_* functions even if the smstateen
check has failed.


False is returned when smstateen check fails currently.


Yes, however if we make this change then should return true if the 
check fails so that the decode_opc doesnt fall through and generate 
another exception. It can be done but it would be contrary to the 
general convention.


OK.  Acceptable to me.

Reviewed-by: Weiwei Li <liwei...@iscas.ac.cn>

Regards,

Weiwei Li


Regards,

Weiwei Li


+        return false;
+    }
+
+    return true;
+}
+#else
+#define smstateen_fcsr_check(ctx, index) (true)
+#endif
+
+#define REQUIRE_ZFINX_OR_F(ctx) do { \
+    if (!has_ext(ctx, RVF)) { \
+        if (!ctx->cfg_ptr->ext_zfinx) { \
+            return false; \
+        } \
+        if (!smstateen_fcsr_check(ctx, 0)) { \
+            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..6c2e338c0a 100644
--- a/target/riscv/insn_trans/trans_rvzfh.c.inc
+++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
@@ -20,18 +20,27 @@
       if (!ctx->cfg_ptr->ext_zfh) {      \
           return false;         \
       }                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)

   
   #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \

       if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
           return false;                  \
       }                                  \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)

   
   #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \

       if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
           return false;                         \
       }                                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)

   
   #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \

@@ -39,6 +48,9 @@
             ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr->ext_zhinxmin))
{     \
           return false;                                        \
       }                                                        \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)

   
   static bool trans_flh(DisasContext *ctx, arg_flh *a)






















					Reply via email to