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>
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com>
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
>
>
