On 21/10/2024 02:46, Alistair Francis wrote:
> On Fri, Oct 18, 2024 at 12:55 AM 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 | 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
