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

[Clément Léger]

On 20/11/2024 06:02, Alistair Francis wrote:
> On Tue, Nov 19, 2024 at 9:27 PM Clément Léger <cleger@rivosinc.com> wrote:
>>
>>
>>
>> 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'm not sure I follow...
> 
> The commit message says:
> 
> """
> - set bit x in menvcfg
> - set bit x in henvcfg
> - clear bit x in menvcfg
> """
> 
> Which to me means henvcfg should be cleared when a bit in menvcfg is
> cleared. But env->henvcfg is instead cleared based on `henvcfg_mask`
> which isn't affected by menvcfg.

Hey Alistair,

Let's take some real example (MENVCFG_PBMTE for instance.) Let's assume
menvcfg/henvcfg are 0 and the following sequence:

- Set MENVCFG_PBMTE in menvcfg (menvcfg = MENVCFG_PBMTE)
- Set HENVCFG_PBMTE in henvcfg (henvcfg = HENVCFG_PBMTE)
- Clear MENVCFG_PBMTE in menvcfg (menvcfg = 0)

On such sequence, we should clear HENVCFG_PBMTE in henvcfg. When using
the existing code, henvcfg_write() does so:

mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);

So our mask = (HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE |
HENVCFG_CBZE) and does not contains HENVCFG_PBMTE.

Finally:

env->henvcfg = (env->henvcfg & ~mask) | (val & mask);

Then env->henvcfg & ~(HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE |
HENVCFG_CBZE | HENVCFG_STCE | HENVCFG_ADUE) will yield henvcfg =
HENVCFG_PBMTE (which is obviously not what we want) | val & mask.

> 
> So clearing a bit in menvcfg will only not allow a bit to be set, but
> not clear any existing bits

Let's take again the current patch and what it does with the same sequence:

- Set MENVCFG_PBMTE in menvcfg (menvcfg = MENVCFG_PBMTE)
- Set HENVCFG_PBMTE in henvcfg (henvcfg = HENVCFG_PBMTE)
- Clear MENVCFG_PBMTE in menvcfg (menvcfg = 0)

henvcfg_mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;

henvcfg_mask |= (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
/* Only keep the enabled bits from menvcfg */
mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);

So mask = (HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE)
which rightfully does not contain HENVCFG_PBMTE so the value to be
written will be  correctly cleared from that bit.

Finally, when it comes to write the final value we'll have the following:

env->henvcfg = (env->henvcfg & ~henvcfg_mask) | (val & mask);

Which yield

henvcfg & ~(HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE |
HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE) | val & (HENVCFG_FIOM |
HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE | HENVCFG_STCE | HENVCFG_ADUE)

So henvcfg HENVCFG_PBMTE bit is correctly cleared and not allowed to be
set by the written value. But I might be missing something.

Thanks,

Clément

> 
> Alistair
> 
>>
>> I guess this still needs a few more explanations if that is not clear
>> enough, sorry for that.
>>
>> Thanks,
>>
>> Clément
>>>
>>> Alistair