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 | 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;
}
- env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
+ env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask);
return RISCV_EXCP_NONE;
}
@@ -2469,8 +2477,9 @@ 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 |
+ uint64_t menvcfg_mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
HENVCFG_ADUE);
+ uint64_t mask = env->menvcfg & menvcfg_mask;
uint64_t valh = (uint64_t)val << 32;
RISCVException ret;
@@ -2479,7 +2488,7 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
return ret;
}
- env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
+ env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (valh & mask);
return RISCV_EXCP_NONE;
}
--
2.45.2
On Fri, Oct 18, 2024 at 12:55 AM Clément Léger <cleger@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 <cleger@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
On 21/10/2024 02:46, Alistair Francis wrote: > On Fri, Oct 18, 2024 at 12:55 AM Clément Léger <cleger@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 <cleger@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 That is what I did before but that didn't work, henvcfg still contained some stale bits. > > 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 The problem is that if you use: mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE) Then, if a bit was cleared in menvcfg before writing henvcfg (let's say HENVCFG_ADUE), then env->henvcfg will be masked with mask = HENVCFG_PBMTE | HENVCFG_STCE, leaving the HENVCFG_ADUE stale bit in env->henvcfg which is wrong for the internal state. The idea here is to actually clear any menvcfg related bit (the 1:1 bits) using the raw mask (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE) to clear everything and then OR it with the value to be written (which is masked with raw bits + menvcfg content) to avoid any stale bits. Thanks, Clément > > Alistair
© 2016 - 2024 Red Hat, Inc.