target/riscv/csr.c | 46 ++++++++++++++++++++++++++++------------ target/riscv/op_helper.c | 7 +++++- 2 files changed, 39 insertions(+), 14 deletions(-)
For csrrs and csrrc, if rs1 specifies a register other than x0, holding
a zero value, the instruction will still attempt to write the unmodified
value back to the csr and will cause side effects
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
target/riscv/csr.c | 46 ++++++++++++++++++++++++++++------------
target/riscv/op_helper.c | 7 +++++-
2 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index aea82dff4a..f4774ca07b 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2872,7 +2872,7 @@ static RISCVException write_upmbase(CPURISCVState *env, int csrno,
static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
int csrno,
- bool write_mask,
+ bool write_csr,
RISCVCPU *cpu)
{
/* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
@@ -2895,7 +2895,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
return RISCV_EXCP_ILLEGAL_INST;
}
#endif
- if (write_mask && read_only) {
+ if (write_csr && read_only) {
return RISCV_EXCP_ILLEGAL_INST;
}
@@ -2915,7 +2915,8 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
target_ulong *ret_value,
target_ulong new_value,
- target_ulong write_mask)
+ target_ulong write_mask,
+ bool write_csr)
{
RISCVException ret;
target_ulong old_value;
@@ -2935,8 +2936,8 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
return ret;
}
- /* write value if writable and write mask set, otherwise drop writes */
- if (write_mask) {
+ /* write value if needed, otherwise drop writes */
+ if (write_csr) {
new_value = (old_value & ~write_mask) | (new_value & write_mask);
if (csr_ops[csrno].write) {
ret = csr_ops[csrno].write(env, csrno, new_value);
@@ -2960,18 +2961,27 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
{
RISCVCPU *cpu = env_archcpu(env);
- RISCVException ret = riscv_csrrw_check(env, csrno, write_mask, cpu);
+ /*
+ * write value when write_mask is set or rs1 is not x0 but holding zero
+ * value for csrrc(new_value is zero) and csrrs(new_value is all-ones)
+ */
+ bool write_csr = write_mask || ((write_mask == 0) &&
+ ((new_value == 0) ||
+ (new_value == (target_ulong)-1)));
+
+ RISCVException ret = riscv_csrrw_check(env, csrno, write_csr, cpu);
if (ret != RISCV_EXCP_NONE) {
return ret;
}
- return riscv_csrrw_do64(env, csrno, ret_value, new_value, write_mask);
+ return riscv_csrrw_do64(env, csrno, ret_value, new_value, write_mask,
+ write_csr);
}
static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno,
Int128 *ret_value,
Int128 new_value,
- Int128 write_mask)
+ Int128 write_mask, bool write_csr)
{
RISCVException ret;
Int128 old_value;
@@ -2982,8 +2992,8 @@ static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno,
return ret;
}
- /* write value if writable and write mask set, otherwise drop writes */
- if (int128_nz(write_mask)) {
+ /* write value if needed, otherwise drop writes */
+ if (write_csr) {
new_value = int128_or(int128_and(old_value, int128_not(write_mask)),
int128_and(new_value, write_mask));
if (csr_ops[csrno].write128) {
@@ -3015,13 +3025,22 @@ RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno,
RISCVException ret;
RISCVCPU *cpu = env_archcpu(env);
- ret = riscv_csrrw_check(env, csrno, int128_nz(write_mask), cpu);
+ /*
+ * write value when write_mask is set or rs1 is not x0 but holding zero
+ * value for csrrc(new_value is zero) and csrrs(new_value is all-ones)
+ */
+ bool write_csr = write_mask || ((write_mask == 0) &&
+ ((new_value == 0) ||
+ (new_value == UINT128_MAX)));
+
+ ret = riscv_csrrw_check(env, csrno, write_csr, cpu);
if (ret != RISCV_EXCP_NONE) {
return ret;
}
if (csr_ops[csrno].read128) {
- return riscv_csrrw_do128(env, csrno, ret_value, new_value, write_mask);
+ return riscv_csrrw_do128(env, csrno, ret_value, new_value, write_mask,
+ write_csr);
}
/*
@@ -3033,7 +3052,8 @@ RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno,
target_ulong old_value;
ret = riscv_csrrw_do64(env, csrno, &old_value,
int128_getlo(new_value),
- int128_getlo(write_mask));
+ int128_getlo(write_mask),
+ write_csr);
if (ret == RISCV_EXCP_NONE && ret_value) {
*ret_value = int128_make64(old_value);
}
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 1a75ba11e6..b2ad465533 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -40,7 +40,12 @@ void helper_raise_exception(CPURISCVState *env, uint32_t exception)
target_ulong helper_csrr(CPURISCVState *env, int csr)
{
target_ulong val = 0;
- RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
+
+ /*
+ * new_value here should be none-zero or none-all-ones here to
+ * distinguish with csrrc/csrrs with rs1 is not x0 but holding zero value
+ */
+ RISCVException ret = riscv_csrrw(env, csr, &val, 1, 0);
if (ret != RISCV_EXCP_NONE) {
riscv_raise_exception(env, ret, GETPC());
--
2.17.1On Wed, Mar 2, 2022 at 11:50 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> For csrrs and csrrc, if rs1 specifies a register other than x0, holding
> a zero value, the instruction will still attempt to write the unmodified
> value back to the csr and will cause side effects
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/csr.c | 46 ++++++++++++++++++++++++++++------------
> target/riscv/op_helper.c | 7 +++++-
> 2 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index aea82dff4a..f4774ca07b 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2872,7 +2872,7 @@ static RISCVException write_upmbase(CPURISCVState *env, int csrno,
>
> static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> int csrno,
> - bool write_mask,
> + bool write_csr,
> RISCVCPU *cpu)
> {
> /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
> @@ -2895,7 +2895,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> return RISCV_EXCP_ILLEGAL_INST;
> }
> #endif
> - if (write_mask && read_only) {
> + if (write_csr && read_only) {
> return RISCV_EXCP_ILLEGAL_INST;
> }
>
> @@ -2915,7 +2915,8 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
> target_ulong new_value,
> - target_ulong write_mask)
> + target_ulong write_mask,
> + bool write_csr)
> {
> RISCVException ret;
> target_ulong old_value;
> @@ -2935,8 +2936,8 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
> return ret;
> }
>
> - /* write value if writable and write mask set, otherwise drop writes */
> - if (write_mask) {
> + /* write value if needed, otherwise drop writes */
> + if (write_csr) {
> new_value = (old_value & ~write_mask) | (new_value & write_mask);
> if (csr_ops[csrno].write) {
> ret = csr_ops[csrno].write(env, csrno, new_value);
> @@ -2960,18 +2961,27 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
> {
> RISCVCPU *cpu = env_archcpu(env);
>
> - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask, cpu);
> + /*
> + * write value when write_mask is set or rs1 is not x0 but holding zero
> + * value for csrrc(new_value is zero) and csrrs(new_value is all-ones)
I don't understand this. Won't write_mask also be zero and when reading?
Alistair
> + */
> + bool write_csr = write_mask || ((write_mask == 0) &&
> + ((new_value == 0) ||
> + (new_value == (target_ulong)-1)));
> +
> + RISCVException ret = riscv_csrrw_check(env, csrno, write_csr, cpu);
> if (ret != RISCV_EXCP_NONE) {
> return ret;
> }
>
> - return riscv_csrrw_do64(env, csrno, ret_value, new_value, write_mask);
> + return riscv_csrrw_do64(env, csrno, ret_value, new_value, write_mask,
> + write_csr);
> }
>
> static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno,
> Int128 *ret_value,
> Int128 new_value,
> - Int128 write_mask)
> + Int128 write_mask, bool write_csr)
> {
> RISCVException ret;
> Int128 old_value;
> @@ -2982,8 +2992,8 @@ static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno,
> return ret;
> }
>
> - /* write value if writable and write mask set, otherwise drop writes */
> - if (int128_nz(write_mask)) {
> + /* write value if needed, otherwise drop writes */
> + if (write_csr) {
> new_value = int128_or(int128_and(old_value, int128_not(write_mask)),
> int128_and(new_value, write_mask));
> if (csr_ops[csrno].write128) {
> @@ -3015,13 +3025,22 @@ RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno,
> RISCVException ret;
> RISCVCPU *cpu = env_archcpu(env);
>
> - ret = riscv_csrrw_check(env, csrno, int128_nz(write_mask), cpu);
> + /*
> + * write value when write_mask is set or rs1 is not x0 but holding zero
> + * value for csrrc(new_value is zero) and csrrs(new_value is all-ones)
> + */
> + bool write_csr = write_mask || ((write_mask == 0) &&
> + ((new_value == 0) ||
> + (new_value == UINT128_MAX)));
> +
> + ret = riscv_csrrw_check(env, csrno, write_csr, cpu);
> if (ret != RISCV_EXCP_NONE) {
> return ret;
> }
>
> if (csr_ops[csrno].read128) {
> - return riscv_csrrw_do128(env, csrno, ret_value, new_value, write_mask);
> + return riscv_csrrw_do128(env, csrno, ret_value, new_value, write_mask,
> + write_csr);
> }
>
> /*
> @@ -3033,7 +3052,8 @@ RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno,
> target_ulong old_value;
> ret = riscv_csrrw_do64(env, csrno, &old_value,
> int128_getlo(new_value),
> - int128_getlo(write_mask));
> + int128_getlo(write_mask),
> + write_csr);
> if (ret == RISCV_EXCP_NONE && ret_value) {
> *ret_value = int128_make64(old_value);
> }
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 1a75ba11e6..b2ad465533 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -40,7 +40,12 @@ void helper_raise_exception(CPURISCVState *env, uint32_t exception)
> target_ulong helper_csrr(CPURISCVState *env, int csr)
> {
> target_ulong val = 0;
> - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
> +
> + /*
> + * new_value here should be none-zero or none-all-ones here to
> + * distinguish with csrrc/csrrs with rs1 is not x0 but holding zero value
> + */
> + RISCVException ret = riscv_csrrw(env, csr, &val, 1, 0);
>
> if (ret != RISCV_EXCP_NONE) {
> riscv_raise_exception(env, ret, GETPC());
> --
> 2.17.1
>
>
在 2022/3/11 上午10:58, Alistair Francis 写道:
> On Wed, Mar 2, 2022 at 11:50 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>> For csrrs and csrrc, if rs1 specifies a register other than x0, holding
>> a zero value, the instruction will still attempt to write the unmodified
>> value back to the csr and will cause side effects
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>> target/riscv/csr.c | 46 ++++++++++++++++++++++++++++------------
>> target/riscv/op_helper.c | 7 +++++-
>> 2 files changed, 39 insertions(+), 14 deletions(-)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index aea82dff4a..f4774ca07b 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -2872,7 +2872,7 @@ static RISCVException write_upmbase(CPURISCVState *env, int csrno,
>>
>> static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>> int csrno,
>> - bool write_mask,
>> + bool write_csr,
>> RISCVCPU *cpu)
>> {
>> /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
>> @@ -2895,7 +2895,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>> return RISCV_EXCP_ILLEGAL_INST;
>> }
>> #endif
>> - if (write_mask && read_only) {
>> + if (write_csr && read_only) {
>> return RISCV_EXCP_ILLEGAL_INST;
>> }
>>
>> @@ -2915,7 +2915,8 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>> static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
>> target_ulong *ret_value,
>> target_ulong new_value,
>> - target_ulong write_mask)
>> + target_ulong write_mask,
>> + bool write_csr)
>> {
>> RISCVException ret;
>> target_ulong old_value;
>> @@ -2935,8 +2936,8 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
>> return ret;
>> }
>>
>> - /* write value if writable and write mask set, otherwise drop writes */
>> - if (write_mask) {
>> + /* write value if needed, otherwise drop writes */
>> + if (write_csr) {
>> new_value = (old_value & ~write_mask) | (new_value & write_mask);
>> if (csr_ops[csrno].write) {
>> ret = csr_ops[csrno].write(env, csrno, new_value);
>> @@ -2960,18 +2961,27 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
>> {
>> RISCVCPU *cpu = env_archcpu(env);
>>
>> - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask, cpu);
>> + /*
>> + * write value when write_mask is set or rs1 is not x0 but holding zero
>> + * value for csrrc(new_value is zero) and csrrs(new_value is all-ones)
> I don't understand this. Won't write_mask also be zero and when reading?
>
> Alistair
>
Yeah. It's true. To distinguish only-read operation with the special
write case(write_mask = 0), I also modified the new_value of riscv_csrrw
from 0 to 1 in helper_csrr :
target_ulong helper_csrr(CPURISCVState *env, int csr)
{
target_ulong val = 0;
- RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
+
+ /*
+ * new_value here should be none-zero or none-all-ones here to
+ * distinguish with csrrc/csrrs with rs1 is not x0 but holding zero value
+ */
+ RISCVException ret = riscv_csrrw(env, csr, &val, 1, 0);
if (ret != RISCV_EXCP_NONE) {
riscv_raise_exception(env, ret, GETPC());
After modification, the cases for all csr related instructions is as follows:
index instruction helper write_mask
new_value Read/Write write_csr
1 csrrw csrrw/csrw all-ones
src1 (R)W true
2 csrrs(rs1=0) csrr zero
1 R false
3 csrrs(rs1!=0) csrrw src1
all-ones RW true
4 csrrs(rs1=0) csrr zero
1 R false
5 csrrc(rs1!=0) csrrw src1
zero RW true
6 csrrc(rs1=0) csrr zero
1 R false
7 csrrwi csrrw/csrw
all-ones rs1 (R)W true
8 csrrsi(rs1=0) csrr zero
1 R false
9 csrrsi(rs1!=0) csrrw rs1
all-ones RW true
10 csrrci(rs1=0) csrr zero
1 R false
11 csrrci(rs1!=0) csrrw rs1
zero RW true
Only row 3 and 5 can be Write-operation with write_mask = 0 when src1 =
0. And it's the special case will be identified by :
((write_mask == 0) && ((new_value == 0) || (new_value == (target_ulong)-1)));
for other only-read instructions, the write_mask is zero, but the new_value is changed to 1 (none-zero and none-all-ones), so they will make write_csr to be false.
Regards,
Weiwei Li
>> + */
>> + bool write_csr = write_mask || ((write_mask == 0) &&
>> + ((new_value == 0) ||
>> + (new_value == (target_ulong)-1)));
>> +
>>
>> --
>> 2.17.1
>>
>>
On Fri, Mar 11, 2022 at 2:58 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
> 在 2022/3/11 上午10:58, Alistair Francis 写道:
> > On Wed, Mar 2, 2022 at 11:50 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> >> For csrrs and csrrc, if rs1 specifies a register other than x0, holding
> >> a zero value, the instruction will still attempt to write the unmodified
> >> value back to the csr and will cause side effects
> >>
> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> >> ---
> >> target/riscv/csr.c | 46 ++++++++++++++++++++++++++++------------
> >> target/riscv/op_helper.c | 7 +++++-
> >> 2 files changed, 39 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index aea82dff4a..f4774ca07b 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -2872,7 +2872,7 @@ static RISCVException write_upmbase(CPURISCVState *env, int csrno,
> >>
> >> static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> >> int csrno,
> >> - bool write_mask,
> >> + bool write_csr,
> >> RISCVCPU *cpu)
> >> {
> >> /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
> >> @@ -2895,7 +2895,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> >> return RISCV_EXCP_ILLEGAL_INST;
> >> }
> >> #endif
> >> - if (write_mask && read_only) {
> >> + if (write_csr && read_only) {
> >> return RISCV_EXCP_ILLEGAL_INST;
> >> }
> >>
> >> @@ -2915,7 +2915,8 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> >> static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
> >> target_ulong *ret_value,
> >> target_ulong new_value,
> >> - target_ulong write_mask)
> >> + target_ulong write_mask,
> >> + bool write_csr)
> >> {
> >> RISCVException ret;
> >> target_ulong old_value;
> >> @@ -2935,8 +2936,8 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
> >> return ret;
> >> }
> >>
> >> - /* write value if writable and write mask set, otherwise drop writes */
> >> - if (write_mask) {
> >> + /* write value if needed, otherwise drop writes */
> >> + if (write_csr) {
> >> new_value = (old_value & ~write_mask) | (new_value & write_mask);
> >> if (csr_ops[csrno].write) {
> >> ret = csr_ops[csrno].write(env, csrno, new_value);
> >> @@ -2960,18 +2961,27 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
> >> {
> >> RISCVCPU *cpu = env_archcpu(env);
> >>
> >> - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask, cpu);
> >> + /*
> >> + * write value when write_mask is set or rs1 is not x0 but holding zero
> >> + * value for csrrc(new_value is zero) and csrrs(new_value is all-ones)
> > I don't understand this. Won't write_mask also be zero and when reading?
> >
> > Alistair
> >
> Yeah. It's true. To distinguish only-read operation with the special
> write case(write_mask = 0), I also modified the new_value of riscv_csrrw
> from 0 to 1 in helper_csrr :
>
> target_ulong helper_csrr(CPURISCVState *env, int csr)
> {
> target_ulong val = 0;
> - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
> +
> + /*
> + * new_value here should be none-zero or none-all-ones here to
> + * distinguish with csrrc/csrrs with rs1 is not x0 but holding zero value
> + */
> + RISCVException ret = riscv_csrrw(env, csr, &val, 1, 0);
This is confusing though and I worry a future change will break this.
I think we should be explicit instead of using special combinations of
masks. What if a write operation occurred that wanted to write 1 with
a mark of 0?
The two options seem to either add a check for the seed CSR in
helper_csrr() to fault if the address matches. That's not the best as
then we have specific code, but this requirement seems pretty specific
as well so it's probably ok.
The other option would be to modify riscv_csrrw() to explicitly pass
in a `bool write_op` that you check against.
Alistair
>
> if (ret != RISCV_EXCP_NONE) {
> riscv_raise_exception(env, ret, GETPC());
>
>
> After modification, the cases for all csr related instructions is as follows:
>
> index instruction helper write_mask
> new_value Read/Write write_csr
>
> 1 csrrw csrrw/csrw all-ones
> src1 (R)W true
>
> 2 csrrs(rs1=0) csrr zero
> 1 R false
>
> 3 csrrs(rs1!=0) csrrw src1
> all-ones RW true
>
> 4 csrrs(rs1=0) csrr zero
> 1 R false
>
> 5 csrrc(rs1!=0) csrrw src1
> zero RW true
>
> 6 csrrc(rs1=0) csrr zero
> 1 R false
>
> 7 csrrwi csrrw/csrw
> all-ones rs1 (R)W true
>
> 8 csrrsi(rs1=0) csrr zero
> 1 R false
>
> 9 csrrsi(rs1!=0) csrrw rs1
> all-ones RW true
>
> 10 csrrci(rs1=0) csrr zero
> 1 R false
>
> 11 csrrci(rs1!=0) csrrw rs1
> zero RW true
>
>
> Only row 3 and 5 can be Write-operation with write_mask = 0 when src1 =
> 0. And it's the special case will be identified by :
>
> ((write_mask == 0) && ((new_value == 0) || (new_value == (target_ulong)-1)));
>
> for other only-read instructions, the write_mask is zero, but the new_value is changed to 1 (none-zero and none-all-ones), so they will make write_csr to be false.
>
> Regards,
> Weiwei Li
>
> >> + */
> >> + bool write_csr = write_mask || ((write_mask == 0) &&
> >> + ((new_value == 0) ||
> >> + (new_value == (target_ulong)-1)));
> >> +
> >>
> >> --
> >> 2.17.1
> >>
> >>
>
在 2022/3/11 下午3:54, Alistair Francis 写道:
> On Fri, Mar 11, 2022 at 2:58 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>>
>> 在 2022/3/11 上午10:58, Alistair Francis 写道:
>>> On Wed, Mar 2, 2022 at 11:50 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>>>> For csrrs and csrrc, if rs1 specifies a register other than x0, holding
>>>> a zero value, the instruction will still attempt to write the unmodified
>>>> value back to the csr and will cause side effects
>>>>
>>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>>> ---
>>>> target/riscv/csr.c | 46 ++++++++++++++++++++++++++++------------
>>>> target/riscv/op_helper.c | 7 +++++-
>>>> 2 files changed, 39 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>> index aea82dff4a..f4774ca07b 100644
>>>> --- a/target/riscv/csr.c
>>>> +++ b/target/riscv/csr.c
>>>> @@ -2872,7 +2872,7 @@ static RISCVException write_upmbase(CPURISCVState *env, int csrno,
>>>>
>>>> static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>>>> int csrno,
>>>> - bool write_mask,
>>>> + bool write_csr,
>>>> RISCVCPU *cpu)
>>>> {
>>>> /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
>>>> @@ -2895,7 +2895,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>>>> return RISCV_EXCP_ILLEGAL_INST;
>>>> }
>>>> #endif
>>>> - if (write_mask && read_only) {
>>>> + if (write_csr && read_only) {
>>>> return RISCV_EXCP_ILLEGAL_INST;
>>>> }
>>>>
>>>> @@ -2915,7 +2915,8 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>>>> static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
>>>> target_ulong *ret_value,
>>>> target_ulong new_value,
>>>> - target_ulong write_mask)
>>>> + target_ulong write_mask,
>>>> + bool write_csr)
>>>> {
>>>> RISCVException ret;
>>>> target_ulong old_value;
>>>> @@ -2935,8 +2936,8 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
>>>> return ret;
>>>> }
>>>>
>>>> - /* write value if writable and write mask set, otherwise drop writes */
>>>> - if (write_mask) {
>>>> + /* write value if needed, otherwise drop writes */
>>>> + if (write_csr) {
>>>> new_value = (old_value & ~write_mask) | (new_value & write_mask);
>>>> if (csr_ops[csrno].write) {
>>>> ret = csr_ops[csrno].write(env, csrno, new_value);
>>>> @@ -2960,18 +2961,27 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
>>>> {
>>>> RISCVCPU *cpu = env_archcpu(env);
>>>>
>>>> - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask, cpu);
>>>> + /*
>>>> + * write value when write_mask is set or rs1 is not x0 but holding zero
>>>> + * value for csrrc(new_value is zero) and csrrs(new_value is all-ones)
>>> I don't understand this. Won't write_mask also be zero and when reading?
>>>
>>> Alistair
>>>
>> Yeah. It's true. To distinguish only-read operation with the special
>> write case(write_mask = 0), I also modified the new_value of riscv_csrrw
>> from 0 to 1 in helper_csrr :
>>
>> target_ulong helper_csrr(CPURISCVState *env, int csr)
>> {
>> target_ulong val = 0;
>> - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
>> +
>> + /*
>> + * new_value here should be none-zero or none-all-ones here to
>> + * distinguish with csrrc/csrrs with rs1 is not x0 but holding zero value
>> + */
>> + RISCVException ret = riscv_csrrw(env, csr, &val, 1, 0);
> This is confusing though and I worry a future change will break this.
> I think we should be explicit instead of using special combinations of
> masks. What if a write operation occurred that wanted to write 1 with
> a mark of 0?
When we write csr, if the mask is zero, the new_value will be ignored
and write back the original value.
So choose any none-zero and none-all-ones value here is OK. and a new
instruction to write 1 with a mark of 0
seems unnecessary. I have no idea what future change may break this
currently.
>
> The two options seem to either add a check for the seed CSR in
> helper_csrr() to fault if the address matches. That's not the best as
> then we have specific code, but this requirement seems pretty specific
> as well so it's probably ok.
The side effect of writing CSR with original value is ignored in
previous code. So I think it's a missing function,
not only the requirement of seed csr.
>
> The other option would be to modify riscv_csrrw() to explicitly pass
> in a `bool write_op` that you check against.
I agree that it may be more intuitive and easy to understand if we
explicitly pass a new "write_op" argument.
I'll try this. Maybe we can judge which one is better later.
>
> Alistair
>
>> if (ret != RISCV_EXCP_NONE) {
>> riscv_raise_exception(env, ret, GETPC());
>>
>>
>> After modification, the cases for all csr related instructions is as follows:
>>
>> index instruction helper write_mask
>> new_value Read/Write write_csr
>>
>> 1 csrrw csrrw/csrw all-ones
>> src1 (R)W true
>>
>> 2 csrrs(rs1=0) csrr zero
>> 1 R false
>>
>> 3 csrrs(rs1!=0) csrrw src1
>> all-ones RW true
>>
>> 4 csrrs(rs1=0) csrr zero
>> 1 R false
>>
>> 5 csrrc(rs1!=0) csrrw src1
>> zero RW true
>>
>> 6 csrrc(rs1=0) csrr zero
>> 1 R false
>>
>> 7 csrrwi csrrw/csrw
>> all-ones rs1 (R)W true
>>
>> 8 csrrsi(rs1=0) csrr zero
>> 1 R false
>>
>> 9 csrrsi(rs1!=0) csrrw rs1
>> all-ones RW true
>>
>> 10 csrrci(rs1=0) csrr zero
>> 1 R false
>>
>> 11 csrrci(rs1!=0) csrrw rs1
>> zero RW true
>>
>>
>> Only row 3 and 5 can be Write-operation with write_mask = 0 when src1 =
>> 0. And it's the special case will be identified by :
>>
>> ((write_mask == 0) && ((new_value == 0) || (new_value == (target_ulong)-1)));
>>
>> for other only-read instructions, the write_mask is zero, but the new_value is changed to 1 (none-zero and none-all-ones), so they will make write_csr to be false.
>>
>> Regards,
>> Weiwei Li
>>
>>>> + */
>>>> + bool write_csr = write_mask || ((write_mask == 0) &&
>>>> + ((new_value == 0) ||
>>>> + (new_value == (target_ulong)-1)));
>>>> +
>>>>
>>>> --
>>>> 2.17.1
>>>>
>>>>
© 2016 - 2024 Red Hat, Inc.