On Fri, Oct 18, 2024 at 12:55 AM Clément Léger <cle...@rivosinc.com> wrote: > > 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 <cle...@rivosinc.com> > --- > target/riscv/csr.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index b84b436151..9e832e0b39 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 menvcfg_mask = 0; > RISCVException ret; > > ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG); > @@ -2443,10 +2450,11 @@ static RISCVException write_henvcfg(CPURISCVState > *env, int csrno, > } > > if (riscv_cpu_mxl(env) == MXL_RV64) { > - mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE); > + menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE; > + mask |= env->menvcfg & menvcfg_mask;
This doesn't seem right. Should it be something like if (riscv_cpu_mxl(env) == MXL_RV64) { mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE); } mask = env->menvcfg & mask; > } > > - env->henvcfg = (env->henvcfg & ~mask) | (val & mask); > + env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask); Using both menvcfg_mask and mask seems wrong here Alistair