On Tue, Jun 4, 2024 at 7:15 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 <yumin...@andestech.com> > Signed-off-by: Alvin Chang <alvi...@andestech.com>
Thanks! Applied to riscv-to-apply.next Alistair > --- > Hi Alistair, > This fixed the issue of riscv_csrrw_debug(). > > Best regards, > Yuming > > target/riscv/cpu.h | 4 +++ > target/riscv/csr.c | 57 ++++++++++++++++++++++++++++++++++++---- > target/riscv/op_helper.c | 6 ++--- > 3 files changed, 58 insertions(+), 9 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 2d0c02c35b..72921bafc0 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -708,6 +708,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); > @@ -740,6 +742,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..aa765678b9 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; > } > @@ -4518,7 +4561,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 > 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 > >