[PATCH v5 1/9] target/riscv: fix henvcfg potentially containing stale bits

[Clément Léger]

With the current implementation, if we had the current scenario:
- set bit x in menvcfg
- set bit x in henvcfg
- clear bit x in menvcfg
then, the internal variable env->henvcfg would still contain bit x due
to both a wrong menvcfg mask used in write_henvcfg() as well as a
missing update of henvcfg upon menvcfg update.
This can lead to some wrong interpretation of the context. In order to
update henvcfg upon menvcfg writing, call write_henvcfg() after writing
menvcfg and fix the mask computation used in write_henvcfg() that is
used to mesk env->menvcfg value (which could still lead to some stale
bits). The same mechanism is also applied for henvcfgh writing.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/csr.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index b84b436151..73ac4d5449 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2345,6 +2345,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, 
int csrno,
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
+                                    target_ulong val);
 static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
                                     target_ulong val)
 {
@@ -2357,6 +2359,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, 
int csrno,
                 (cfg->ext_svadu ? MENVCFG_ADUE : 0);
     }
     env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
+    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
 
     return RISCV_EXCP_NONE;
 }
@@ -2368,6 +2371,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, 
int csrno,
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
+                                    target_ulong val);
 static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
                                      target_ulong val)
 {
@@ -2378,6 +2383,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, 
int csrno,
     uint64_t valh = (uint64_t)val << 32;
 
     env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
+    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
 
     return RISCV_EXCP_NONE;
 }
@@ -2435,6 +2441,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, 
int csrno,
                                     target_ulong val)
 {
     uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
+    uint64_t henvcfg_mask = mask, menvcfg_mask;
     RISCVException ret;
 
     ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
@@ -2443,10 +2450,24 @@ static RISCVException write_henvcfg(CPURISCVState *env, 
int csrno,
     }
 
     if (riscv_cpu_mxl(env) == MXL_RV64) {
-        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
+        /*
+         * Since henvcfg depends on a menvcfg subset, we want to clear all the
+         * menvcfg supported feature (whatever their state is) before enabling
+         * some new one using the provided value. Not doing so would result in
+         * keeping stale menvcfg bits in henvcfg value if a bit was enabled in
+         * menvcfg and then disabled before updating henvcfg for instance.
+         */
+        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
+        mask |= env->menvcfg & menvcfg_mask;
+        henvcfg_mask |= menvcfg_mask;
     }
 
-    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
+    /*
+     * 'henvcfg_mask' contains all supported bits (both in henvcfg and menvcfg
+     * common bits) and 'mask' contains henvcfg exclusive bits as well as
+     * menvcfg enabled bits only.
+     */
+    env->henvcfg = (env->henvcfg & ~henvcfg_mask) | (val & mask);
 
     return RISCV_EXCP_NONE;
 }
@@ -2469,8 +2490,13 @@ static RISCVException read_henvcfgh(CPURISCVState *env, 
int csrno,
 static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
                                      target_ulong val)
 {
-    uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
-                                    HENVCFG_ADUE);
+    /*
+     * Same comment than the one in write_henvcfg() applies here, we want to
+     * clear all previous menvcfg bits before enabling some new one to avoid
+     * stale menvcfg bits in henvcfg.
+     */
+    uint64_t henvcfg_mask = (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
+    uint64_t mask = env->menvcfg & henvcfg_mask;
     uint64_t valh = (uint64_t)val << 32;
     RISCVException ret;
 
@@ -2479,7 +2505,11 @@ static RISCVException write_henvcfgh(CPURISCVState *env, 
int csrno,
         return ret;
     }
 
-    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
+    /*
+     * 'henvcfg_mask' contains all menvcfg supported bits and 'mask' contains
+     * menvcfg enabled bits only.
+     */
+    env->henvcfg = (env->henvcfg & ~henvcfg_mask) | (valh & mask);
     return RISCV_EXCP_NONE;
 }
 
-- 
2.45.2