|
| From: | Weiwei Li |
| Subject: | Re: [PATCH] target/riscv: write back unmodified value for csrrc/csrrs with rs1 is not x0 but holding zero |
| Date: | Fri, 11 Mar 2022 16:39:26 +0800 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 |
在 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? AlistairYeah. 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.
Alistairif (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
| [Prev in Thread] | Current Thread | [Next in Thread] |