qemu-riscv
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Clément Léger
Subject: Re: [PATCH v5 1/9] target/riscv: fix henvcfg potentially containing stale bits
Date: Thu, 28 Nov 2024 08:43:36 +0100
User-agent: Mozilla Thunderbird 

On 28/11/2024 02:37, Alistair Francis wrote:
> On Thu, Nov 21, 2024 at 6:28 PM Clément Léger <cleger@rivosinc.com> wrote:
>>
>>
>>
>> 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.
> 
> Ah ok!
> 
> That makes sense, sorry I took so long to wrap my head around this one.
> 
> Although now that we are basically clearing henvcfg on each write, do
> we ever need the ` (env->henvcfg & ~henvcfg_mask) | ` can't we just
> overwrite it each time?

Oh god, I basically wrote the most complicated way to clear a
register... so yeah indeed, this can be reduced to just 'env->henvcfg =
val & mask'.

> 
> Also, this will need a rebase on:
> https://github.com/alistair23/qemu/tree/riscv-to-apply.next as there
> are now more operations in write_henvcfg()

Acked,

I'll submit another version.

Thanks,

Clément

> 
> Alistair
> 
>>
>> 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
>>
reply via email to
[Prev in Thread] Current Thread [Next in Thread]