On Thu, Nov 21, 2024 at 6:28 PM Clément Léger <[email protected]> wrote: > > > > On 20/11/2024 06:02, Alistair Francis wrote: > > On Tue, Nov 19, 2024 at 9:27 PM Clément Léger <[email protected]> wrote: > >> > >> > >> > >> On 19/11/2024 05:16, Alistair Francis wrote: > >>> On Thu, Nov 14, 2024 at 7:14 PM Clément Léger <[email protected]> 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 <[email protected]> > >>>> --- > >>>> 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 >
