qemu-riscv
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Weiwei Li
Subject: Re: [PATCH v6 3/5] target/riscv: smstateen check for fcsr
Date: Thu, 28 Jul 2022 15:38:25 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 

在 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;                                        \
reply via email to
[Prev in Thread] Current Thread [Next in Thread]