[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: |
Alistair Francis |
|
Subject: |
Re: [PATCH v5 1/9] target/riscv: fix henvcfg potentially containing stale bits |
|
Date: |
Thu, 28 Nov 2024 11:37:14 +1000 |
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?
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()
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
>
[PATCH v5 3/9] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior, Clément Léger, 2024/11/14
[PATCH v5 2/9] target/riscv: Add Ssdbltrp CSRs handling, Clément Léger, 2024/11/14
[PATCH v5 5/9] target/riscv: Add Ssdbltrp ISA extension enable switch, Clément Léger, 2024/11/14
[PATCH v5 4/9] target/riscv: Implement Ssdbltrp exception handling, Clément Léger, 2024/11/14
[PATCH v5 8/9] target/riscv: Implement Smdbltrp behavior, Clément Léger, 2024/11/14
[PATCH v5 7/9] target/riscv: Implement Smdbltrp sret, mret and mnret behavior, Clément Léger, 2024/11/14
[PATCH v5 6/9] target/riscv: Add Smdbltrp CSRs handling, Clément Léger, 2024/11/14
[PATCH v5 9/9] target/riscv: Add Smdbltrp ISA extension enable switch, Clément Léger, 2024/11/14