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`
Alistair
>
> 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
>
>
