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 <[email protected]> --- 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
