On Tue, Nov 19, 2024 at 9:27 PM Clément Léger <cle...@rivosinc.com> wrote:
>
>
>
> On 19/11/2024 05:16, Alistair Francis wrote:
> > On Thu, Nov 14, 2024 at 7:14 PM 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 | 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);
> >
> > Won't `env->henvcfg & ~henvcfg_mask` still contain the stale data?
> > `henvcfg_mask` isn't based on the current value of `env->menvcfg`
>
> Hey Alistair,
>
> That's the point, env->henvcfg is cleared with henvcfg_mask which
> contains the set of HENVCFG_* and MENVCFG_* "raw" bits so that the new
> value that is written does not contain any menvcfg stale bits. "mask"
> however is actually masked with menvcfg value to ensure the new bits
> that are going to be written won't contain any incoherent bits.
I'm not sure I follow...
The commit message says:
"""
- set bit x in menvcfg
- set bit x in henvcfg
- clear bit x in menvcfg
"""
Which to me means henvcfg should be cleared when a bit in menvcfg is
cleared. But env->henvcfg is instead cleared based on `henvcfg_mask`
which isn't affected by menvcfg.
So clearing a bit in menvcfg will only not allow a bit to be set, but
not clear any existing bits
Alistair
>
> I guess this still needs a few more explanations if that is not clear
> enough, sorry for that.
>
> Thanks,
>
> Clément
> >
> > Alistair
