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

[Clément Léger]

On 06/11/2024 01:16, Alistair Francis wrote:
> On Wed, Oct 30, 2024 at 6:57 PM Clément Léger <cleger@rivosinc.com> wrote:
>>
>>
>>
>> On 23/10/2024 04:51, Alistair Francis wrote:
>>> On Mon, Oct 21, 2024 at 7:30 PM Clément Léger <cleger@rivosinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 21/10/2024 02:46, Alistair Francis wrote:
>>>>> On Fri, Oct 18, 2024 at 12:55 AM 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 | 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.
>>>
>>> When calling write_henvcfg() can't you just do:
>>>
>>> write_henvcfg(env, CSR_HENVCFG, env->henvcfg & env->menvcfg)
>>
>> Yeah it's clearer and I thought of that but you'll end up with the same
>> result since the problem does not come from the value you supply but
>> rather by the old masked henvcfg value used at the end of
>> write_henvcg()(and the mask is computed independently of the new value),
>> ie this line:
>>
>> env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
> 
> Yeah ok.
> 
> Maybe a comment or two to describe what is going on would be enough
> then, or even splitting the single line ops into multiple lines would
> be clearer

Yep, that could be enough. I'll do that,

Thanks !

> 
> Alistair