Re: [PATCH v3] target/riscv: raise an exception when CSRRS/CSRRC writes a read-only CSR

[Alistair Francis <alistair23@gmail.com>]

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
>
>