Re: [PATCH v5 1/9] target/riscv: fix henvcfg potentially containing stale bits

[Alistair Francis]

On Thu, Nov 14, 2024 at 7:14 PM 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 | 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
>
>