| Message ID | 20240403070823.80897-1-yumin686@andestech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] target/riscv: raise an exception when CSRRS/CSRRC writes a read-only CSR | expand |
On Wed, Apr 3, 2024 at 5:10 PM Yu-Ming Chang via <qemu-devel@nongnu.org> wrote: > > Both CSRRS and CSRRC always read the addressed CSR and cause any read side > effects regardless of rs1 and rd fields. Note that if rs1 specifies a register > holding a zero value other than x0, the instruction will still attempt to write > the unmodified value back to the CSR and will cause any attendant side effects. > > So if CSRRS or CSRRC tries to write a read-only CSR with rs1 which specifies > a register holding a zero value, an illegal instruction exception should be > raised. > > Signed-off-by: Yu-Ming Chang <yumin686@andestech.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > Hi maintainers, > Do I need to make any further improvements to this patch? > > Best regards, > Yuming > > target/riscv/cpu.h | 4 ++++ > target/riscv/csr.c | 51 ++++++++++++++++++++++++++++++++++++---- > target/riscv/op_helper.c | 6 ++--- > 3 files changed, 53 insertions(+), 8 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 3b1a02b944..99006bdb45 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -710,6 +710,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc, > void riscv_cpu_update_mask(CPURISCVState *env); > bool riscv_cpu_is_32bit(RISCVCPU *cpu); > > +RISCVException riscv_csrr(CPURISCVState *env, int csrno, > + target_ulong *ret_value); > RISCVException riscv_csrrw(CPURISCVState *env, int csrno, > target_ulong *ret_value, > target_ulong new_value, target_ulong write_mask); > @@ -742,6 +744,8 @@ typedef RISCVException (*riscv_csr_op_fn)(CPURISCVState *env, int csrno, > target_ulong new_value, > target_ulong write_mask); > > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, > + Int128 *ret_value); > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, > Int128 *ret_value, > Int128 new_value, Int128 write_mask); > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index 726096444f..35662e1777 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -4312,7 +4312,7 @@ static RISCVException rmw_seed(CPURISCVState *env, int csrno, > > static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > int csrno, > - bool write_mask) > + bool write) > { > /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */ > bool read_only = get_field(csrno, 0xC00) == 3; > @@ -4334,7 +4334,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > } > > /* read / write check */ > - if (write_mask && read_only) { > + if (write && read_only) { > return RISCV_EXCP_ILLEGAL_INST; > } > > @@ -4421,11 +4421,22 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > +RISCVException riscv_csrr(CPURISCVState *env, int csrno, > + target_ulong *ret_value) > +{ > + RISCVException ret = riscv_csrrw_check(env, csrno, false); > + if (ret != RISCV_EXCP_NONE) { > + return ret; > + } > + > + return riscv_csrrw_do64(env, csrno, ret_value, 0, 0); > +} > + > RISCVException riscv_csrrw(CPURISCVState *env, int csrno, > target_ulong *ret_value, > target_ulong new_value, target_ulong write_mask) > { > - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask); > + RISCVException ret = riscv_csrrw_check(env, csrno, true); > if (ret != RISCV_EXCP_NONE) { > return ret; > } > @@ -4473,13 +4484,45 @@ static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, > + Int128 *ret_value) > +{ > + RISCVException ret; > + > + ret = riscv_csrrw_check(env, csrno, false); > + if (ret != RISCV_EXCP_NONE) { > + return ret; > + } > + > + if (csr_ops[csrno].read128) { > + return riscv_csrrw_do128(env, csrno, ret_value, > + int128_zero(), int128_zero()); > + } > + > + /* > + * Fall back to 64-bit version for now, if the 128-bit alternative isn't > + * at all defined. > + * Note, some CSRs don't need to extend to MXLEN (64 upper bits non > + * significant), for those, this fallback is correctly handling the > + * accesses > + */ > + target_ulong old_value; > + ret = riscv_csrrw_do64(env, csrno, &old_value, > + (target_ulong)0, > + (target_ulong)0); > + if (ret == RISCV_EXCP_NONE && ret_value) { > + *ret_value = int128_make64(old_value); > + } > + return ret; > +} > + > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, > Int128 *ret_value, > Int128 new_value, Int128 write_mask) > { > RISCVException ret; > > - ret = riscv_csrrw_check(env, csrno, int128_nz(write_mask)); > + ret = riscv_csrrw_check(env, csrno, true); > if (ret != RISCV_EXCP_NONE) { > return ret; > } > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index f414aaebdb..b95d47e9ac 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -51,7 +51,7 @@ target_ulong helper_csrr(CPURISCVState *env, int csr) > } > > target_ulong val = 0; > - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0); > + RISCVException ret = riscv_csrr(env, csr, &val); > > if (ret != RISCV_EXCP_NONE) { > riscv_raise_exception(env, ret, GETPC()); > @@ -84,9 +84,7 @@ target_ulong helper_csrrw(CPURISCVState *env, int csr, > target_ulong helper_csrr_i128(CPURISCVState *env, int csr) > { > Int128 rv = int128_zero(); > - RISCVException ret = riscv_csrrw_i128(env, csr, &rv, > - int128_zero(), > - int128_zero()); > + RISCVException ret = riscv_csrr_i128(env, csr, &rv); > > if (ret != RISCV_EXCP_NONE) { > riscv_raise_exception(env, ret, GETPC()); > -- > 2.34.1 > >
On Wed, Apr 3, 2024 at 5:10 PM Yu-Ming Chang via <qemu-devel@nongnu.org> wrote: > > Both CSRRS and CSRRC always read the addressed CSR and cause any read side > effects regardless of rs1 and rd fields. Note that if rs1 specifies a register > holding a zero value other than x0, the instruction will still attempt to write > the unmodified value back to the CSR and will cause any attendant side effects. > > So if CSRRS or CSRRC tries to write a read-only CSR with rs1 which specifies > a register holding a zero value, an illegal instruction exception should be > raised. > > Signed-off-by: Yu-Ming Chang <yumin686@andestech.com> Thanks! Applied to riscv-to-apply.next Alistair > --- > Hi maintainers, > Do I need to make any further improvements to this patch? > > Best regards, > Yuming > > target/riscv/cpu.h | 4 ++++ > target/riscv/csr.c | 51 ++++++++++++++++++++++++++++++++++++---- > target/riscv/op_helper.c | 6 ++--- > 3 files changed, 53 insertions(+), 8 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 3b1a02b944..99006bdb45 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -710,6 +710,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc, > void riscv_cpu_update_mask(CPURISCVState *env); > bool riscv_cpu_is_32bit(RISCVCPU *cpu); > > +RISCVException riscv_csrr(CPURISCVState *env, int csrno, > + target_ulong *ret_value); > RISCVException riscv_csrrw(CPURISCVState *env, int csrno, > target_ulong *ret_value, > target_ulong new_value, target_ulong write_mask); > @@ -742,6 +744,8 @@ typedef RISCVException (*riscv_csr_op_fn)(CPURISCVState *env, int csrno, > target_ulong new_value, > target_ulong write_mask); > > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, > + Int128 *ret_value); > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, > Int128 *ret_value, > Int128 new_value, Int128 write_mask); > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index 726096444f..35662e1777 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -4312,7 +4312,7 @@ static RISCVException rmw_seed(CPURISCVState *env, int csrno, > > static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > int csrno, > - bool write_mask) > + bool write) > { > /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */ > bool read_only = get_field(csrno, 0xC00) == 3; > @@ -4334,7 +4334,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > } > > /* read / write check */ > - if (write_mask && read_only) { > + if (write && read_only) { > return RISCV_EXCP_ILLEGAL_INST; > } > > @@ -4421,11 +4421,22 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > +RISCVException riscv_csrr(CPURISCVState *env, int csrno, > + target_ulong *ret_value) > +{ > + RISCVException ret = riscv_csrrw_check(env, csrno, false); > + if (ret != RISCV_EXCP_NONE) { > + return ret; > + } > + > + return riscv_csrrw_do64(env, csrno, ret_value, 0, 0); > +} > + > RISCVException riscv_csrrw(CPURISCVState *env, int csrno, > target_ulong *ret_value, > target_ulong new_value, target_ulong write_mask) > { > - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask); > + RISCVException ret = riscv_csrrw_check(env, csrno, true); > if (ret != RISCV_EXCP_NONE) { > return ret; > } > @@ -4473,13 +4484,45 @@ static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, > + Int128 *ret_value) > +{ > + RISCVException ret; > + > + ret = riscv_csrrw_check(env, csrno, false); > + if (ret != RISCV_EXCP_NONE) { > + return ret; > + } > + > + if (csr_ops[csrno].read128) { > + return riscv_csrrw_do128(env, csrno, ret_value, > + int128_zero(), int128_zero()); > + } > + > + /* > + * Fall back to 64-bit version for now, if the 128-bit alternative isn't > + * at all defined. > + * Note, some CSRs don't need to extend to MXLEN (64 upper bits non > + * significant), for those, this fallback is correctly handling the > + * accesses > + */ > + target_ulong old_value; > + ret = riscv_csrrw_do64(env, csrno, &old_value, > + (target_ulong)0, > + (target_ulong)0); > + if (ret == RISCV_EXCP_NONE && ret_value) { > + *ret_value = int128_make64(old_value); > + } > + return ret; > +} > + > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, > Int128 *ret_value, > Int128 new_value, Int128 write_mask) > { > RISCVException ret; > > - ret = riscv_csrrw_check(env, csrno, int128_nz(write_mask)); > + ret = riscv_csrrw_check(env, csrno, true); > if (ret != RISCV_EXCP_NONE) { > return ret; > } > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index f414aaebdb..b95d47e9ac 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -51,7 +51,7 @@ target_ulong helper_csrr(CPURISCVState *env, int csr) > } > > target_ulong val = 0; > - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0); > + RISCVException ret = riscv_csrr(env, csr, &val); > > if (ret != RISCV_EXCP_NONE) { > riscv_raise_exception(env, ret, GETPC()); > @@ -84,9 +84,7 @@ target_ulong helper_csrrw(CPURISCVState *env, int csr, > target_ulong helper_csrr_i128(CPURISCVState *env, int csr) > { > Int128 rv = int128_zero(); > - RISCVException ret = riscv_csrrw_i128(env, csr, &rv, > - int128_zero(), > - int128_zero()); > + RISCVException ret = riscv_csrr_i128(env, csr, &rv); > > if (ret != RISCV_EXCP_NONE) { > riscv_raise_exception(env, ret, GETPC()); > -- > 2.34.1 > >
On Wed, Apr 3, 2024 at 5:10 PM Yu-Ming Chang via <qemu-devel@nongnu.org> wrote: > > Both CSRRS and CSRRC always read the addressed CSR and cause any read side > effects regardless of rs1 and rd fields. Note that if rs1 specifies a register > holding a zero value other than x0, the instruction will still attempt to write > the unmodified value back to the CSR and will cause any attendant side effects. > > So if CSRRS or CSRRC tries to write a read-only CSR with rs1 which specifies > a register holding a zero value, an illegal instruction exception should be > raised. > > Signed-off-by: Yu-Ming Chang <yumin686@andestech.com> This fails the GitLab CI tests https://gitlab.com/qemu-project/qemu/-/jobs/6953349448 ERROR:../tests/plugin/insn.c:58:vcpu_init: assertion failed: (count > 0) timeout: the monitored command dumped core Aborted make[1]: *** [Makefile:178: run-plugin-catch-syscalls-with-libinsn.so] Error 134 make: *** [/builds/qemu-project/qemu/tests/Makefile.include:56: run-tcg-tests-riscv64-linux-user] Error 2 #0 riscv_gdb_get_csr (cs=<optimized out>, buf=0x5555558e7f50, n=3072) at ../src/target/riscv/gdbstub.c:183 #1 0x00007ffff7fb7841 in vcpu_init (id=<optimized out>, vcpu_index=<optimized out>) at ../src/tests/plugin/insn.c:57 #2 0x000055555569ef1a in plugin_vcpu_cb__simple (cpu=0x5555558fb820, ev=<optimized out>) at ../src/plugins/core.c:111 After 182 result = riscv_csrrw_debug(env, n, &val, 0, 0); result == 2. I haven't had much luck reproducing this locally, so I don't have a great idea of why it isn't working. I suspect you need to ignore the checks for debug accesses Alistair > --- > Hi maintainers, > Do I need to make any further improvements to this patch? > > Best regards, > Yuming > > target/riscv/cpu.h | 4 ++++ > target/riscv/csr.c | 51 ++++++++++++++++++++++++++++++++++++---- > target/riscv/op_helper.c | 6 ++--- > 3 files changed, 53 insertions(+), 8 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 3b1a02b944..99006bdb45 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -710,6 +710,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc, > void riscv_cpu_update_mask(CPURISCVState *env); > bool riscv_cpu_is_32bit(RISCVCPU *cpu); > > +RISCVException riscv_csrr(CPURISCVState *env, int csrno, > + target_ulong *ret_value); > RISCVException riscv_csrrw(CPURISCVState *env, int csrno, > target_ulong *ret_value, > target_ulong new_value, target_ulong write_mask); > @@ -742,6 +744,8 @@ typedef RISCVException (*riscv_csr_op_fn)(CPURISCVState *env, int csrno, > target_ulong new_value, > target_ulong write_mask); > > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, > + Int128 *ret_value); > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, > Int128 *ret_value, > Int128 new_value, Int128 write_mask); > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index 726096444f..35662e1777 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -4312,7 +4312,7 @@ static RISCVException rmw_seed(CPURISCVState *env, int csrno, > > static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > int csrno, > - bool write_mask) > + bool write) > { > /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */ > bool read_only = get_field(csrno, 0xC00) == 3; > @@ -4334,7 +4334,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > } > > /* read / write check */ > - if (write_mask && read_only) { > + if (write && read_only) { > return RISCV_EXCP_ILLEGAL_INST; > } > > @@ -4421,11 +4421,22 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > +RISCVException riscv_csrr(CPURISCVState *env, int csrno, > + target_ulong *ret_value) > +{ > + RISCVException ret = riscv_csrrw_check(env, csrno, false); > + if (ret != RISCV_EXCP_NONE) { > + return ret; > + } > + > + return riscv_csrrw_do64(env, csrno, ret_value, 0, 0); > +} > + > RISCVException riscv_csrrw(CPURISCVState *env, int csrno, > target_ulong *ret_value, > target_ulong new_value, target_ulong write_mask) > { > - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask); > + RISCVException ret = riscv_csrrw_check(env, csrno, true); > if (ret != RISCV_EXCP_NONE) { > return ret; > } > @@ -4473,13 +4484,45 @@ static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, > + Int128 *ret_value) > +{ > + RISCVException ret; > + > + ret = riscv_csrrw_check(env, csrno, false); > + if (ret != RISCV_EXCP_NONE) { > + return ret; > + } > + > + if (csr_ops[csrno].read128) { > + return riscv_csrrw_do128(env, csrno, ret_value, > + int128_zero(), int128_zero()); > + } > + > + /* > + * Fall back to 64-bit version for now, if the 128-bit alternative isn't > + * at all defined. > + * Note, some CSRs don't need to extend to MXLEN (64 upper bits non > + * significant), for those, this fallback is correctly handling the > + * accesses > + */ > + target_ulong old_value; > + ret = riscv_csrrw_do64(env, csrno, &old_value, > + (target_ulong)0, > + (target_ulong)0); > + if (ret == RISCV_EXCP_NONE && ret_value) { > + *ret_value = int128_make64(old_value); > + } > + return ret; > +} > + > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, > Int128 *ret_value, > Int128 new_value, Int128 write_mask) > { > RISCVException ret; > > - ret = riscv_csrrw_check(env, csrno, int128_nz(write_mask)); > + ret = riscv_csrrw_check(env, csrno, true); > if (ret != RISCV_EXCP_NONE) { > return ret; > } > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index f414aaebdb..b95d47e9ac 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -51,7 +51,7 @@ target_ulong helper_csrr(CPURISCVState *env, int csr) > } > > target_ulong val = 0; > - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0); > + RISCVException ret = riscv_csrr(env, csr, &val); > > if (ret != RISCV_EXCP_NONE) { > riscv_raise_exception(env, ret, GETPC()); > @@ -84,9 +84,7 @@ target_ulong helper_csrrw(CPURISCVState *env, int csr, > target_ulong helper_csrr_i128(CPURISCVState *env, int csr) > { > Int128 rv = int128_zero(); > - RISCVException ret = riscv_csrrw_i128(env, csr, &rv, > - int128_zero(), > - int128_zero()); > + RISCVException ret = riscv_csrr_i128(env, csr, &rv); > > if (ret != RISCV_EXCP_NONE) { > riscv_raise_exception(env, ret, GETPC()); > -- > 2.34.1 > >
Hi Alistair, I think we need the following patch to fix this issue: From 6175c9aee103e40b5a5da587f659563de93b3d85 Mon Sep 17 00:00:00 2001 From: Alvin Chang <alvinga@andestech.com> Date: Thu, 18 Apr 2024 14:52:36 +0800 Subject: [PATCH] target/riscv: Fix GDB can not read the read-only CSR From commit 563581cb60, use riscv_csrrw() to read a read-only CSR will lead to exception. Fix it by calling riscv_csrr() when GDB wants to read a read-only CSR. Signed-off-by: Alvin Chang <alvinga@andestech.com> --- target/riscv/csr.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 7aab267916..96accc1549 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -4625,7 +4625,11 @@ RISCVException riscv_csrrw_debug(CPURISCVState *env, int csrno, #if !defined(CONFIG_USER_ONLY) env->debugger = true; #endif - ret = riscv_csrrw(env, csrno, ret_value, new_value, write_mask); + if (!write_mask) { + ret = riscv_csrr(env, csrno, ret_value); + } else { + ret = riscv_csrrw(env, csrno, ret_value, new_value, write_mask); + } #if !defined(CONFIG_USER_ONLY) env->debugger = false; #endif -- 2.34.1 Best regards, Yuming -----Original Message----- From: Alistair Francis <alistair23@gmail.com> Sent: Monday, June 3, 2024 1:39 PM To: Yuming Yu-Ming Chang(張育銘) <yumin686@andestech.com> Cc: qemu-riscv@nongnu.org; qemu-devel@nongnu.org; palmer@dabbelt.com; alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com; dbarboza@ventanamicro.com; zhiwei_liu@linux.alibaba.com Subject: Re: [PATCH v3] target/riscv: raise an exception when CSRRS/CSRRC writes a read-only CSR [EXTERNAL MAIL] On Wed, Apr 3, 2024 at 5:10 PM Yu-Ming Chang via <qemu-devel@nongnu.org> wrote: > > Both CSRRS and CSRRC always read the addressed CSR and cause any read side > effects regardless of rs1 and rd fields. Note that if rs1 specifies a register > holding a zero value other than x0, the instruction will still attempt to write > the unmodified value back to the CSR and will cause any attendant side effects. > > So if CSRRS or CSRRC tries to write a read-only CSR with rs1 which specifies > a register holding a zero value, an illegal instruction exception should be > raised. > > Signed-off-by: Yu-Ming Chang <yumin686@andestech.com> This fails the GitLab CI tests https://gitlab.com/qemu-project/qemu/-/jobs/6953349448 ERROR:../tests/plugin/insn.c:58:vcpu_init: assertion failed: (count > 0) timeout: the monitored command dumped core Aborted make[1]: *** [Makefile:178: run-plugin-catch-syscalls-with-libinsn.so] Error 134 make: *** [/builds/qemu-project/qemu/tests/Makefile.include:56: run-tcg-tests-riscv64-linux-user] Error 2 #0 riscv_gdb_get_csr (cs=<optimized out>, buf=0x5555558e7f50, n=3072) at ../src/target/riscv/gdbstub.c:183 #1 0x00007ffff7fb7841 in vcpu_init (id=<optimized out>, vcpu_index=<optimized out>) at ../src/tests/plugin/insn.c:57 #2 0x000055555569ef1a in plugin_vcpu_cb__simple (cpu=0x5555558fb820, ev=<optimized out>) at ../src/plugins/core.c:111 After 182 result = riscv_csrrw_debug(env, n, &val, 0, 0); result == 2. I haven't had much luck reproducing this locally, so I don't have a great idea of why it isn't working. I suspect you need to ignore the checks for debug accesses Alistair > --- > Hi maintainers, > Do I need to make any further improvements to this patch? > > Best regards, > Yuming > > target/riscv/cpu.h | 4 ++++ > target/riscv/csr.c | 51 ++++++++++++++++++++++++++++++++++++---- > target/riscv/op_helper.c | 6 ++--- > 3 files changed, 53 insertions(+), 8 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 3b1a02b944..99006bdb45 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -710,6 +710,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc, > void riscv_cpu_update_mask(CPURISCVState *env); > bool riscv_cpu_is_32bit(RISCVCPU *cpu); > > +RISCVException riscv_csrr(CPURISCVState *env, int csrno, > + target_ulong *ret_value); > RISCVException riscv_csrrw(CPURISCVState *env, int csrno, > target_ulong *ret_value, > target_ulong new_value, target_ulong write_mask); > @@ -742,6 +744,8 @@ typedef RISCVException (*riscv_csr_op_fn)(CPURISCVState *env, int csrno, > target_ulong new_value, > target_ulong write_mask); > > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, > + Int128 *ret_value); > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, > Int128 *ret_value, > Int128 new_value, Int128 write_mask); > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index 726096444f..35662e1777 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -4312,7 +4312,7 @@ static RISCVException rmw_seed(CPURISCVState *env, int csrno, > > static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > int csrno, > - bool write_mask) > + bool write) > { > /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */ > bool read_only = get_field(csrno, 0xC00) == 3; > @@ -4334,7 +4334,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > } > > /* read / write check */ > - if (write_mask && read_only) { > + if (write && read_only) { > return RISCV_EXCP_ILLEGAL_INST; > } > > @@ -4421,11 +4421,22 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > +RISCVException riscv_csrr(CPURISCVState *env, int csrno, > + target_ulong *ret_value) > +{ > + RISCVException ret = riscv_csrrw_check(env, csrno, false); > + if (ret != RISCV_EXCP_NONE) { > + return ret; > + } > + > + return riscv_csrrw_do64(env, csrno, ret_value, 0, 0); > +} > + > RISCVException riscv_csrrw(CPURISCVState *env, int csrno, > target_ulong *ret_value, > target_ulong new_value, target_ulong write_mask) > { > - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask); > + RISCVException ret = riscv_csrrw_check(env, csrno, true); > if (ret != RISCV_EXCP_NONE) { > return ret; > } > @@ -4473,13 +4484,45 @@ static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, > + Int128 *ret_value) > +{ > + RISCVException ret; > + > + ret = riscv_csrrw_check(env, csrno, false); > + if (ret != RISCV_EXCP_NONE) { > + return ret; > + } > + > + if (csr_ops[csrno].read128) { > + return riscv_csrrw_do128(env, csrno, ret_value, > + int128_zero(), int128_zero()); > + } > + > + /* > + * Fall back to 64-bit version for now, if the 128-bit alternative isn't > + * at all defined. > + * Note, some CSRs don't need to extend to MXLEN (64 upper bits non > + * significant), for those, this fallback is correctly handling the > + * accesses > + */ > + target_ulong old_value; > + ret = riscv_csrrw_do64(env, csrno, &old_value, > + (target_ulong)0, > + (target_ulong)0); > + if (ret == RISCV_EXCP_NONE && ret_value) { > + *ret_value = int128_make64(old_value); > + } > + return ret; > +} > + > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, > Int128 *ret_value, > Int128 new_value, Int128 write_mask) > { > RISCVException ret; > > - ret = riscv_csrrw_check(env, csrno, int128_nz(write_mask)); > + ret = riscv_csrrw_check(env, csrno, true); > if (ret != RISCV_EXCP_NONE) { > return ret; > } > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index f414aaebdb..b95d47e9ac 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -51,7 +51,7 @@ target_ulong helper_csrr(CPURISCVState *env, int csr) > } > > target_ulong val = 0; > - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0); > + RISCVException ret = riscv_csrr(env, csr, &val); > > if (ret != RISCV_EXCP_NONE) { > riscv_raise_exception(env, ret, GETPC()); > @@ -84,9 +84,7 @@ target_ulong helper_csrrw(CPURISCVState *env, int csr, > target_ulong helper_csrr_i128(CPURISCVState *env, int csr) > { > Int128 rv = int128_zero(); > - RISCVException ret = riscv_csrrw_i128(env, csr, &rv, > - int128_zero(), > - int128_zero()); > + RISCVException ret = riscv_csrr_i128(env, csr, &rv); > > if (ret != RISCV_EXCP_NONE) { > riscv_raise_exception(env, ret, GETPC()); > -- > 2.34.1 > > CONFIDENTIALITY NOTICE: This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation. Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
On Mon, Jun 3, 2024 at 4:00 PM Yuming Yu-Ming Chang(張育銘) <yumin686@andestech.com> wrote: > > Hi Alistair, > > I think we need the following patch to fix this issue: I have dropped the original patch from my tree. Please fix the issue and send a new patch with the fix incorporated. Alistair > > From 6175c9aee103e40b5a5da587f659563de93b3d85 Mon Sep 17 00:00:00 2001 > From: Alvin Chang <alvinga@andestech.com> > Date: Thu, 18 Apr 2024 14:52:36 +0800 > Subject: [PATCH] target/riscv: Fix GDB can not read the read-only CSR > > From commit 563581cb60, use riscv_csrrw() to read a read-only CSR will > lead to exception. Fix it by calling riscv_csrr() when GDB wants to read > a read-only CSR. > > Signed-off-by: Alvin Chang <alvinga@andestech.com> > --- > target/riscv/csr.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index 7aab267916..96accc1549 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -4625,7 +4625,11 @@ RISCVException riscv_csrrw_debug(CPURISCVState *env, int csrno, > #if !defined(CONFIG_USER_ONLY) > env->debugger = true; > #endif > - ret = riscv_csrrw(env, csrno, ret_value, new_value, write_mask); > + if (!write_mask) { > + ret = riscv_csrr(env, csrno, ret_value); > + } else { > + ret = riscv_csrrw(env, csrno, ret_value, new_value, write_mask); > + } > #if !defined(CONFIG_USER_ONLY) > env->debugger = false; > #endif > -- > 2.34.1 > > Best regards, > Yuming > > -----Original Message----- > From: Alistair Francis <alistair23@gmail.com> > Sent: Monday, June 3, 2024 1:39 PM > To: Yuming Yu-Ming Chang(張育銘) <yumin686@andestech.com> > Cc: qemu-riscv@nongnu.org; qemu-devel@nongnu.org; palmer@dabbelt.com; alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com; dbarboza@ventanamicro.com; zhiwei_liu@linux.alibaba.com > Subject: Re: [PATCH v3] target/riscv: raise an exception when CSRRS/CSRRC writes a read-only CSR > > [EXTERNAL MAIL] > > On Wed, Apr 3, 2024 at 5:10 PM Yu-Ming Chang via <qemu-devel@nongnu.org> wrote: > > > > Both CSRRS and CSRRC always read the addressed CSR and cause any read side > > effects regardless of rs1 and rd fields. Note that if rs1 specifies a register > > holding a zero value other than x0, the instruction will still attempt to write > > the unmodified value back to the CSR and will cause any attendant side effects. > > > > So if CSRRS or CSRRC tries to write a read-only CSR with rs1 which specifies > > a register holding a zero value, an illegal instruction exception should be > > raised. > > > > Signed-off-by: Yu-Ming Chang <yumin686@andestech.com> > > This fails the GitLab CI tests > > https://gitlab.com/qemu-project/qemu/-/jobs/6953349448 > > ERROR:../tests/plugin/insn.c:58:vcpu_init: assertion failed: (count > 0) > timeout: the monitored command dumped core > Aborted > make[1]: *** [Makefile:178: run-plugin-catch-syscalls-with-libinsn.so] Error 134 > make: *** [/builds/qemu-project/qemu/tests/Makefile.include:56: > run-tcg-tests-riscv64-linux-user] Error 2 > > #0 riscv_gdb_get_csr (cs=<optimized out>, buf=0x5555558e7f50, n=3072) > at ../src/target/riscv/gdbstub.c:183 > #1 0x00007ffff7fb7841 in vcpu_init (id=<optimized out>, > vcpu_index=<optimized out>) at ../src/tests/plugin/insn.c:57 > #2 0x000055555569ef1a in plugin_vcpu_cb__simple (cpu=0x5555558fb820, > ev=<optimized out>) at ../src/plugins/core.c:111 > > > After > > 182 result = riscv_csrrw_debug(env, n, &val, 0, 0); > > result == 2. > > I haven't had much luck reproducing this locally, so I don't have a > great idea of why it isn't working. I suspect you need to ignore the > checks for debug accesses > > Alistair > > > --- > > Hi maintainers, > > Do I need to make any further improvements to this patch? > > > > Best regards, > > Yuming > > > > target/riscv/cpu.h | 4 ++++ > > target/riscv/csr.c | 51 ++++++++++++++++++++++++++++++++++++---- > > target/riscv/op_helper.c | 6 ++--- > > 3 files changed, 53 insertions(+), 8 deletions(-) > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > > index 3b1a02b944..99006bdb45 100644 > > --- a/target/riscv/cpu.h > > +++ b/target/riscv/cpu.h > > @@ -710,6 +710,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc, > > void riscv_cpu_update_mask(CPURISCVState *env); > > bool riscv_cpu_is_32bit(RISCVCPU *cpu); > > > > +RISCVException riscv_csrr(CPURISCVState *env, int csrno, > > + target_ulong *ret_value); > > RISCVException riscv_csrrw(CPURISCVState *env, int csrno, > > target_ulong *ret_value, > > target_ulong new_value, target_ulong write_mask); > > @@ -742,6 +744,8 @@ typedef RISCVException (*riscv_csr_op_fn)(CPURISCVState *env, int csrno, > > target_ulong new_value, > > target_ulong write_mask); > > > > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, > > + Int128 *ret_value); > > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, > > Int128 *ret_value, > > Int128 new_value, Int128 write_mask); > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > index 726096444f..35662e1777 100644 > > --- a/target/riscv/csr.c > > +++ b/target/riscv/csr.c > > @@ -4312,7 +4312,7 @@ static RISCVException rmw_seed(CPURISCVState *env, int csrno, > > > > static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > > int csrno, > > - bool write_mask) > > + bool write) > > { > > /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */ > > bool read_only = get_field(csrno, 0xC00) == 3; > > @@ -4334,7 +4334,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > > } > > > > /* read / write check */ > > - if (write_mask && read_only) { > > + if (write && read_only) { > > return RISCV_EXCP_ILLEGAL_INST; > > } > > > > @@ -4421,11 +4421,22 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno, > > return RISCV_EXCP_NONE; > > } > > > > +RISCVException riscv_csrr(CPURISCVState *env, int csrno, > > + target_ulong *ret_value) > > +{ > > + RISCVException ret = riscv_csrrw_check(env, csrno, false); > > + if (ret != RISCV_EXCP_NONE) { > > + return ret; > > + } > > + > > + return riscv_csrrw_do64(env, csrno, ret_value, 0, 0); > > +} > > + > > RISCVException riscv_csrrw(CPURISCVState *env, int csrno, > > target_ulong *ret_value, > > target_ulong new_value, target_ulong write_mask) > > { > > - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask); > > + RISCVException ret = riscv_csrrw_check(env, csrno, true); > > if (ret != RISCV_EXCP_NONE) { > > return ret; > > } > > @@ -4473,13 +4484,45 @@ static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno, > > return RISCV_EXCP_NONE; > > } > > > > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, > > + Int128 *ret_value) > > +{ > > + RISCVException ret; > > + > > + ret = riscv_csrrw_check(env, csrno, false); > > + if (ret != RISCV_EXCP_NONE) { > > + return ret; > > + } > > + > > + if (csr_ops[csrno].read128) { > > + return riscv_csrrw_do128(env, csrno, ret_value, > > + int128_zero(), int128_zero()); > > + } > > + > > + /* > > + * Fall back to 64-bit version for now, if the 128-bit alternative isn't > > + * at all defined. > > + * Note, some CSRs don't need to extend to MXLEN (64 upper bits non > > + * significant), for those, this fallback is correctly handling the > > + * accesses > > + */ > > + target_ulong old_value; > > + ret = riscv_csrrw_do64(env, csrno, &old_value, > > + (target_ulong)0, > > + (target_ulong)0); > > + if (ret == RISCV_EXCP_NONE && ret_value) { > > + *ret_value = int128_make64(old_value); > > + } > > + return ret; > > +} > > + > > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, > > Int128 *ret_value, > > Int128 new_value, Int128 write_mask) > > { > > RISCVException ret; > > > > - ret = riscv_csrrw_check(env, csrno, int128_nz(write_mask)); > > + ret = riscv_csrrw_check(env, csrno, true); > > if (ret != RISCV_EXCP_NONE) { > > return ret; > > } > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > > index f414aaebdb..b95d47e9ac 100644 > > --- a/target/riscv/op_helper.c > > +++ b/target/riscv/op_helper.c > > @@ -51,7 +51,7 @@ target_ulong helper_csrr(CPURISCVState *env, int csr) > > } > > > > target_ulong val = 0; > > - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0); > > + RISCVException ret = riscv_csrr(env, csr, &val); > > > > if (ret != RISCV_EXCP_NONE) { > > riscv_raise_exception(env, ret, GETPC()); > > @@ -84,9 +84,7 @@ target_ulong helper_csrrw(CPURISCVState *env, int csr, > > target_ulong helper_csrr_i128(CPURISCVState *env, int csr) > > { > > Int128 rv = int128_zero(); > > - RISCVException ret = riscv_csrrw_i128(env, csr, &rv, > > - int128_zero(), > > - int128_zero()); > > + RISCVException ret = riscv_csrr_i128(env, csr, &rv); > > > > if (ret != RISCV_EXCP_NONE) { > > riscv_raise_exception(env, ret, GETPC()); > > -- > > 2.34.1 > > > > > CONFIDENTIALITY NOTICE: > > This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation. > > Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 3b1a02b944..99006bdb45 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -710,6 +710,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc, void riscv_cpu_update_mask(CPURISCVState *env); bool riscv_cpu_is_32bit(RISCVCPU *cpu); +RISCVException riscv_csrr(CPURISCVState *env, int csrno, + target_ulong *ret_value); RISCVException riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value, target_ulong new_value, target_ulong write_mask); @@ -742,6 +744,8 @@ typedef RISCVException (*riscv_csr_op_fn)(CPURISCVState *env, int csrno, target_ulong new_value, target_ulong write_mask); +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, + Int128 *ret_value); RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, Int128 *ret_value, Int128 new_value, Int128 write_mask); diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 726096444f..35662e1777 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -4312,7 +4312,7 @@ static RISCVException rmw_seed(CPURISCVState *env, int csrno, static inline RISCVException riscv_csrrw_check(CPURISCVState *env, int csrno, - bool write_mask) + bool write) { /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */ bool read_only = get_field(csrno, 0xC00) == 3; @@ -4334,7 +4334,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, } /* read / write check */ - if (write_mask && read_only) { + if (write && read_only) { return RISCV_EXCP_ILLEGAL_INST; } @@ -4421,11 +4421,22 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno, return RISCV_EXCP_NONE; } +RISCVException riscv_csrr(CPURISCVState *env, int csrno, + target_ulong *ret_value) +{ + RISCVException ret = riscv_csrrw_check(env, csrno, false); + if (ret != RISCV_EXCP_NONE) { + return ret; + } + + return riscv_csrrw_do64(env, csrno, ret_value, 0, 0); +} + RISCVException riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value, target_ulong new_value, target_ulong write_mask) { - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask); + RISCVException ret = riscv_csrrw_check(env, csrno, true); if (ret != RISCV_EXCP_NONE) { return ret; } @@ -4473,13 +4484,45 @@ static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno, return RISCV_EXCP_NONE; } +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno, + Int128 *ret_value) +{ + RISCVException ret; + + ret = riscv_csrrw_check(env, csrno, false); + if (ret != RISCV_EXCP_NONE) { + return ret; + } + + if (csr_ops[csrno].read128) { + return riscv_csrrw_do128(env, csrno, ret_value, + int128_zero(), int128_zero()); + } + + /* + * Fall back to 64-bit version for now, if the 128-bit alternative isn't + * at all defined. + * Note, some CSRs don't need to extend to MXLEN (64 upper bits non + * significant), for those, this fallback is correctly handling the + * accesses + */ + target_ulong old_value; + ret = riscv_csrrw_do64(env, csrno, &old_value, + (target_ulong)0, + (target_ulong)0); + if (ret == RISCV_EXCP_NONE && ret_value) { + *ret_value = int128_make64(old_value); + } + return ret; +} + RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno, Int128 *ret_value, Int128 new_value, Int128 write_mask) { RISCVException ret; - ret = riscv_csrrw_check(env, csrno, int128_nz(write_mask)); + ret = riscv_csrrw_check(env, csrno, true); if (ret != RISCV_EXCP_NONE) { return ret; } diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c index f414aaebdb..b95d47e9ac 100644 --- a/target/riscv/op_helper.c +++ b/target/riscv/op_helper.c @@ -51,7 +51,7 @@ target_ulong helper_csrr(CPURISCVState *env, int csr) } target_ulong val = 0; - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0); + RISCVException ret = riscv_csrr(env, csr, &val); if (ret != RISCV_EXCP_NONE) { riscv_raise_exception(env, ret, GETPC()); @@ -84,9 +84,7 @@ target_ulong helper_csrrw(CPURISCVState *env, int csr, target_ulong helper_csrr_i128(CPURISCVState *env, int csr) { Int128 rv = int128_zero(); - RISCVException ret = riscv_csrrw_i128(env, csr, &rv, - int128_zero(), - int128_zero()); + RISCVException ret = riscv_csrr_i128(env, csr, &rv); if (ret != RISCV_EXCP_NONE) { riscv_raise_exception(env, ret, GETPC());
Both CSRRS and CSRRC always read the addressed CSR and cause any read side effects regardless of rs1 and rd fields. Note that if rs1 specifies a register holding a zero value other than x0, the instruction will still attempt to write the unmodified value back to the CSR and will cause any attendant side effects. So if CSRRS or CSRRC tries to write a read-only CSR with rs1 which specifies a register holding a zero value, an illegal instruction exception should be raised. Signed-off-by: Yu-Ming Chang <yumin686@andestech.com> --- Hi maintainers, Do I need to make any further improvements to this patch? Best regards, Yuming target/riscv/cpu.h | 4 ++++ target/riscv/csr.c | 51 ++++++++++++++++++++++++++++++++++++---- target/riscv/op_helper.c | 6 ++--- 3 files changed, 53 insertions(+), 8 deletions(-)