On Wed, Oct 30, 2024 at 6:57 PM Clément Léger <cle...@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 <cle...@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 <cle...@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 <cle...@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
Alistair
