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);
return RISCV_EXCP_NONE;
}
@@ -2469,8 +2490,13 @@ 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 |
- HENVCFG_ADUE);
+ /*
+ * Same comment than the one in write_henvcfg() applies here, we want to
+ * clear all previous menvcfg bits before enabling some new one to avoid
+ * stale menvcfg bits in henvcfg.
+ */
+ uint64_t henvcfg_mask = (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
+ uint64_t mask = env->menvcfg & henvcfg_mask;
uint64_t valh = (uint64_t)val << 32;
RISCVException ret;
@@ -2479,7 +2505,11 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
return ret;
}
- env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
+ /*
+ * 'henvcfg_mask' contains all menvcfg supported bits and 'mask' contains
+ * menvcfg enabled bits only.
+ */
+ env->henvcfg = (env->henvcfg & ~henvcfg_mask) | (valh & mask);
return RISCV_EXCP_NONE;
}
--
2.45.2
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`
Alistair
>
> return RISCV_EXCP_NONE;
> }
> @@ -2469,8 +2490,13 @@ 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 |
> - HENVCFG_ADUE);
> + /*
> + * Same comment than the one in write_henvcfg() applies here, we want to
> + * clear all previous menvcfg bits before enabling some new one to avoid
> + * stale menvcfg bits in henvcfg.
> + */
> + uint64_t henvcfg_mask = (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
> + uint64_t mask = env->menvcfg & henvcfg_mask;
> uint64_t valh = (uint64_t)val << 32;
> RISCVException ret;
>
> @@ -2479,7 +2505,11 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> return ret;
> }
>
> - env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> + /*
> + * 'henvcfg_mask' contains all menvcfg supported bits and 'mask' contains
> + * menvcfg enabled bits only.
> + */
> + env->henvcfg = (env->henvcfg & ~henvcfg_mask) | (valh & mask);
> return RISCV_EXCP_NONE;
> }
>
> --
> 2.45.2
>
>
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 guess this still needs a few more explanations if that is not clear
enough, sorry for that.
Thanks,
Clément
>
> Alistair
>
>>
>> return RISCV_EXCP_NONE;
>> }
>> @@ -2469,8 +2490,13 @@ 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 |
>> - HENVCFG_ADUE);
>> + /*
>> + * Same comment than the one in write_henvcfg() applies here, we want to
>> + * clear all previous menvcfg bits before enabling some new one to avoid
>> + * stale menvcfg bits in henvcfg.
>> + */
>> + uint64_t henvcfg_mask = (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
>> + uint64_t mask = env->menvcfg & henvcfg_mask;
>> uint64_t valh = (uint64_t)val << 32;
>> RISCVException ret;
>>
>> @@ -2479,7 +2505,11 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>> return ret;
>> }
>>
>> - env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
>> + /*
>> + * 'henvcfg_mask' contains all menvcfg supported bits and 'mask' contains
>> + * menvcfg enabled bits only.
>> + */
>> + env->henvcfg = (env->henvcfg & ~henvcfg_mask) | (valh & mask);
>> return RISCV_EXCP_NONE;
>> }
>>
>> --
>> 2.45.2
>>
>>
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.
So clearing a bit in menvcfg will only not allow a bit to be set, but
not clear any existing bits
Alistair
>
> I guess this still needs a few more explanations if that is not clear
> enough, sorry for that.
>
> Thanks,
>
> Clément
> >
> > Alistair
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.
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
© 2016 - 2024 Red Hat, Inc.