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

[Clément Léger]

On 19/11/2024 05:16, Alistair Francis wrote:
> 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`

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 guess this still needs a few more explanations if that is not clear
enough, sorry for that.

Thanks,

Clément
> 
> 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
>>
>>