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 | 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;
}
- env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
+ env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask);
return RISCV_EXCP_NONE;
}
@@ -2469,8 +2477,9 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
target_ulong val)
{
- uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
+ uint64_t menvcfg_mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
HENVCFG_ADUE);
+ uint64_t mask = env->menvcfg & menvcfg_mask;
uint64_t valh = (uint64_t)val << 32;
RISCVException ret;
@@ -2479,7 +2488,7 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
return ret;
}
- env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
+ env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (valh & mask);
return RISCV_EXCP_NONE;
}
--
2.45.2
On Fri, Oct 18, 2024 at 12:55 AM 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 | 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
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
Alistair
On 21/10/2024 02:46, Alistair Francis wrote:
> On Fri, Oct 18, 2024 at 12:55 AM 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 | 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.
Thanks,
Clément
>
> Alistair
On Mon, Oct 21, 2024 at 7:30 PM Clément Léger <cleger@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 <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 | 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)
I feel like that's clearer then the current approach
Alistair
>
> Thanks,
>
> Clément
>
> >
> > Alistair
>
On 23/10/2024 04:51, Alistair Francis wrote:
> On Mon, Oct 21, 2024 at 7:30 PM Clément Léger <cleger@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 <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 | 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);
The mask is actually not containing the bits that were cleared in menvcfg.
We could actually use
write_henvcfg(env, CSR_HENVCFG, env->henvcfg & env->menvcfg)
But maybe we could call it prior to menvcfg update in order to clear all
currently set bit in menvcfg and mask env->henvcfg with the new value to
be written. That seems a bit off as well. But I'll take whatever you
think is clearer.
Clément
>
> I feel like that's clearer then the current approach
>
> Alistair
>
>>
>> Thanks,
>>
>> Clément
>>
>>>
>>> Alistair
>>
On Wed, Oct 30, 2024 at 6:57 PM Clément Léger <cleger@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 <cleger@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 <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 | 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
On 06/11/2024 01:16, Alistair Francis wrote:
> On Wed, Oct 30, 2024 at 6:57 PM Clément Léger <cleger@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 <cleger@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 <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 | 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
Yep, that could be enough. I'll do that,
Thanks !
>
> Alistair
© 2016 - 2026 Red Hat, Inc.