diff mbox series

[09/17] target/riscv: Reorg csr instructions

Message ID 20210709042608.883256-10-richard.henderson@linaro.org
State Superseded
Headers show
Series target/riscv: Use tcg_constant_* | expand

Commit Message

Richard Henderson July 9, 2021, 4:26 a.m. UTC 
Introduce csrr and csrw helpers, for read-only and write-only insns.

Note that we do not properly implement this in riscv_csrrw, in that
we cannot distinguish true read-only (rs1 == 0) from any other zero
write_mask another source register -- this should still raise an
exception for read-only registers.

Only issue gen_io_start for CF_USE_ICOUNT.
Use ctx->zero for csrrc.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/riscv/helper.h                   |   6 +-
 target/riscv/op_helper.c                |  18 +--
 target/riscv/insn_trans/trans_rvi.c.inc | 170 +++++++++++++++++-------
 3 files changed, 129 insertions(+), 65 deletions(-)

-- 
2.25.1

Comments

Alistair Francis July 23, 2021, 5 a.m. UTC | #1 
On Fri, Jul 9, 2021 at 2:46 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Introduce csrr and csrw helpers, for read-only and write-only insns.

>

> Note that we do not properly implement this in riscv_csrrw, in that

> we cannot distinguish true read-only (rs1 == 0) from any other zero

> write_mask another source register -- this should still raise an

> exception for read-only registers.

>

> Only issue gen_io_start for CF_USE_ICOUNT.

> Use ctx->zero for csrrc.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


Reviewed-by: Alistair Francis <alistair.francis@wdc.com>


Alistair

> ---

>  target/riscv/helper.h                   |   6 +-

>  target/riscv/op_helper.c                |  18 +--

>  target/riscv/insn_trans/trans_rvi.c.inc | 170 +++++++++++++++++-------

>  3 files changed, 129 insertions(+), 65 deletions(-)

>

> diff --git a/target/riscv/helper.h b/target/riscv/helper.h

> index 415e37bc37..460eee9988 100644

> --- a/target/riscv/helper.h

> +++ b/target/riscv/helper.h

> @@ -65,9 +65,9 @@ DEF_HELPER_FLAGS_2(gorc, TCG_CALL_NO_RWG_SE, tl, tl, tl)

>  DEF_HELPER_FLAGS_2(gorcw, TCG_CALL_NO_RWG_SE, tl, tl, tl)

>

>  /* Special functions */

> -DEF_HELPER_3(csrrw, tl, env, tl, tl)

> -DEF_HELPER_4(csrrs, tl, env, tl, tl, tl)

> -DEF_HELPER_4(csrrc, tl, env, tl, tl, tl)

> +DEF_HELPER_2(csrr, tl, env, int)

> +DEF_HELPER_3(csrw, void, env, int, tl)

> +DEF_HELPER_4(csrrw, tl, env, int, tl, tl)

>  #ifndef CONFIG_USER_ONLY

>  DEF_HELPER_2(sret, tl, env, tl)

>  DEF_HELPER_2(mret, tl, env, tl)

> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c

> index 3c48e739ac..ee7c24efe7 100644

> --- a/target/riscv/op_helper.c

> +++ b/target/riscv/op_helper.c

> @@ -37,11 +37,10 @@ void helper_raise_exception(CPURISCVState *env, uint32_t exception)

>      riscv_raise_exception(env, exception, 0);

>  }

>

> -target_ulong helper_csrrw(CPURISCVState *env, target_ulong src,

> -        target_ulong csr)

> +target_ulong helper_csrr(CPURISCVState *env, int csr)

>  {

>      target_ulong val = 0;

> -    RISCVException ret = riscv_csrrw(env, csr, &val, src, -1);

> +    RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);

>

>      if (ret != RISCV_EXCP_NONE) {

>          riscv_raise_exception(env, ret, GETPC());

> @@ -49,23 +48,20 @@ target_ulong helper_csrrw(CPURISCVState *env, target_ulong src,

>      return val;

>  }

>

> -target_ulong helper_csrrs(CPURISCVState *env, target_ulong src,

> -        target_ulong csr, target_ulong rs1_pass)

> +void helper_csrw(CPURISCVState *env, int csr, target_ulong src)

>  {

> -    target_ulong val = 0;

> -    RISCVException ret = riscv_csrrw(env, csr, &val, -1, rs1_pass ? src : 0);

> +    RISCVException ret = riscv_csrrw(env, csr, NULL, src, -1);

>

>      if (ret != RISCV_EXCP_NONE) {

>          riscv_raise_exception(env, ret, GETPC());

>      }

> -    return val;

>  }

>

> -target_ulong helper_csrrc(CPURISCVState *env, target_ulong src,

> -        target_ulong csr, target_ulong rs1_pass)

> +target_ulong helper_csrrw(CPURISCVState *env, int csr,

> +                          target_ulong src, target_ulong write_mask)

>  {

>      target_ulong val = 0;

> -    RISCVException ret = riscv_csrrw(env, csr, &val, 0, rs1_pass ? src : 0);

> +    RISCVException ret = riscv_csrrw(env, csr, &val, src, write_mask);

>

>      if (ret != RISCV_EXCP_NONE) {

>          riscv_raise_exception(env, ret, GETPC());

> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc

> index 840187a4d6..3705aad380 100644

> --- a/target/riscv/insn_trans/trans_rvi.c.inc

> +++ b/target/riscv/insn_trans/trans_rvi.c.inc

> @@ -452,80 +452,148 @@ static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a)

>      return true;

>  }

>

> -#define RISCV_OP_CSR_PRE do {\

> -    source1 = tcg_temp_new(); \

> -    csr_store = tcg_temp_new(); \

> -    dest = tcg_temp_new(); \

> -    rs1_pass = tcg_temp_new(); \

> -    gen_get_gpr(source1, a->rs1); \

> -    tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next); \

> -    tcg_gen_movi_tl(rs1_pass, a->rs1); \

> -    tcg_gen_movi_tl(csr_store, a->csr); \

> -    gen_io_start();\

> -} while (0)

> +static bool do_csr_post(DisasContext *ctx)

> +{

> +    /* We may have changed important cpu state -- exit to main loop. */

> +    tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn);

> +    exit_tb(ctx);

> +    ctx->base.is_jmp = DISAS_NORETURN;

> +    return true;

> +}

>

> -#define RISCV_OP_CSR_POST do {\

> -    gen_set_gpr(a->rd, dest); \

> -    tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn); \

> -    exit_tb(ctx); \

> -    ctx->base.is_jmp = DISAS_NORETURN; \

> -    tcg_temp_free(source1); \

> -    tcg_temp_free(csr_store); \

> -    tcg_temp_free(dest); \

> -    tcg_temp_free(rs1_pass); \

> -} while (0)

> +static bool do_csrr(DisasContext *ctx, int rd, int rc)

> +{

> +    TCGv dest = gpr_dst(ctx, rd);

> +    TCGv_i32 csr = tcg_constant_i32(rc);

>

> +    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {

> +        gen_io_start();

> +    }

> +    gen_helper_csrr(dest, cpu_env, csr);

> +    return do_csr_post(ctx);

> +}

> +

> +static bool do_csrw(DisasContext *ctx, int rc, TCGv src)

> +{

> +    TCGv_i32 csr = tcg_constant_i32(rc);

> +

> +    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {

> +        gen_io_start();

> +    }

> +    gen_helper_csrw(cpu_env, csr, src);

> +    return do_csr_post(ctx);

> +}

> +

> +static bool do_csrrw(DisasContext *ctx, int rd, int rc, TCGv src, TCGv mask)

> +{

> +    TCGv dest = gpr_dst(ctx, rd);

> +    TCGv_i32 csr = tcg_constant_i32(rc);

> +

> +    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {

> +        gen_io_start();

> +    }

> +    gen_helper_csrrw(dest, cpu_env, csr, src, mask);

> +    return do_csr_post(ctx);

> +}

>

>  static bool trans_csrrw(DisasContext *ctx, arg_csrrw *a)

>  {

> -    TCGv source1, csr_store, dest, rs1_pass;

> -    RISCV_OP_CSR_PRE;

> -    gen_helper_csrrw(dest, cpu_env, source1, csr_store);

> -    RISCV_OP_CSR_POST;

> -    return true;

> +    TCGv src = gpr_src(ctx, a->rs1);

> +

> +    /*

> +     * If rd == 0, the insn shall not read the csr, nor cause any of the

> +     * side effects that might occur on a csr read.

> +     */

> +    if (a->rd == 0) {

> +        return do_csrw(ctx, a->csr, src);

> +    }

> +

> +    TCGv mask = tcg_constant_tl(-1);

> +    return do_csrrw(ctx, a->rd, a->csr, src, mask);

>  }

>

>  static bool trans_csrrs(DisasContext *ctx, arg_csrrs *a)

>  {

> -    TCGv source1, csr_store, dest, rs1_pass;

> -    RISCV_OP_CSR_PRE;

> -    gen_helper_csrrs(dest, cpu_env, source1, csr_store, rs1_pass);

> -    RISCV_OP_CSR_POST;

> -    return true;

> +    /*

> +     * If rs1 == 0, the insn shall not write to the csr at all, nor

> +     * cause any of the side effects that might occur on a csr write.

> +     * Note that 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.

> +     */

> +    if (a->rs1 == 0) {

> +        return do_csrr(ctx, a->rd, a->csr);

> +    }

> +

> +    TCGv ones = tcg_constant_tl(-1);

> +    TCGv mask = gpr_src(ctx, a->rs1);

> +    return do_csrrw(ctx, a->rd, a->csr, ones, mask);

>  }

>

>  static bool trans_csrrc(DisasContext *ctx, arg_csrrc *a)

>  {

> -    TCGv source1, csr_store, dest, rs1_pass;

> -    RISCV_OP_CSR_PRE;

> -    gen_helper_csrrc(dest, cpu_env, source1, csr_store, rs1_pass);

> -    RISCV_OP_CSR_POST;

> -    return true;

> +    /*

> +     * If rs1 == 0, the insn shall not write to the csr at all, nor

> +     * cause any of the side effects that might occur on a csr write.

> +     * Note that 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.

> +     */

> +    if (a->rs1 == 0) {

> +        return do_csrr(ctx, a->rd, a->csr);

> +    }

> +

> +    TCGv mask = gpr_src(ctx, a->rs1);

> +    return do_csrrw(ctx, a->rd, a->csr, ctx->zero, mask);

>  }

>

>  static bool trans_csrrwi(DisasContext *ctx, arg_csrrwi *a)

>  {

> -    TCGv source1, csr_store, dest, rs1_pass;

> -    RISCV_OP_CSR_PRE;

> -    gen_helper_csrrw(dest, cpu_env, rs1_pass, csr_store);

> -    RISCV_OP_CSR_POST;

> -    return true;

> +    TCGv src = tcg_constant_tl(a->rs1);

> +

> +    /*

> +     * If rd == 0, the insn shall not read the csr, nor cause any of the

> +     * side effects that might occur on a csr read.

> +     */

> +    if (a->rd == 0) {

> +        return do_csrw(ctx, a->csr, src);

> +    }

> +

> +    TCGv mask = tcg_constant_tl(-1);

> +    return do_csrrw(ctx, a->rd, a->csr, src, mask);

>  }

>

>  static bool trans_csrrsi(DisasContext *ctx, arg_csrrsi *a)

>  {

> -    TCGv source1, csr_store, dest, rs1_pass;

> -    RISCV_OP_CSR_PRE;

> -    gen_helper_csrrs(dest, cpu_env, rs1_pass, csr_store, rs1_pass);

> -    RISCV_OP_CSR_POST;

> -    return true;

> +    /*

> +     * If rs1 == 0, the insn shall not write to the csr at all, nor

> +     * cause any of the side effects that might occur on a csr write.

> +     * Note that 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.

> +     */

> +    if (a->rs1 == 0) {

> +        return do_csrr(ctx, a->rd, a->csr);

> +    }

> +

> +    TCGv ones = tcg_constant_tl(-1);

> +    TCGv mask = tcg_constant_tl(a->rs1);

> +    return do_csrrw(ctx, a->rd, a->csr, ones, mask);

>  }

>

>  static bool trans_csrrci(DisasContext *ctx, arg_csrrci *a)

>  {

> -    TCGv source1, csr_store, dest, rs1_pass;

> -    RISCV_OP_CSR_PRE;

> -    gen_helper_csrrc(dest, cpu_env, rs1_pass, csr_store, rs1_pass);

> -    RISCV_OP_CSR_POST;

> -    return true;

> +    /*

> +     * If rs1 == 0, the insn shall not write to the csr at all, nor

> +     * cause any of the side effects that might occur on a csr write.

> +     * Note that 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.

> +     */

> +    if (a->rs1 == 0) {

> +        return do_csrr(ctx, a->rd, a->csr);

> +    }

> +

> +    TCGv mask = tcg_constant_tl(a->rs1);

> +    return do_csrrw(ctx, a->rd, a->csr, ctx->zero, mask);

>  }

> --

> 2.25.1

>

>
diff mbox series

Patch

diff --git a/target/riscv/helper.h b/target/riscv/helper.h
index 415e37bc37..460eee9988 100644
--- a/target/riscv/helper.h
+++ b/target/riscv/helper.h
@@ -65,9 +65,9 @@  DEF_HELPER_FLAGS_2(gorc, TCG_CALL_NO_RWG_SE, tl, tl, tl)
 DEF_HELPER_FLAGS_2(gorcw, TCG_CALL_NO_RWG_SE, tl, tl, tl)
 
 /* Special functions */
-DEF_HELPER_3(csrrw, tl, env, tl, tl)
-DEF_HELPER_4(csrrs, tl, env, tl, tl, tl)
-DEF_HELPER_4(csrrc, tl, env, tl, tl, tl)
+DEF_HELPER_2(csrr, tl, env, int)
+DEF_HELPER_3(csrw, void, env, int, tl)
+DEF_HELPER_4(csrrw, tl, env, int, tl, tl)
 #ifndef CONFIG_USER_ONLY
 DEF_HELPER_2(sret, tl, env, tl)
 DEF_HELPER_2(mret, tl, env, tl)
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 3c48e739ac..ee7c24efe7 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -37,11 +37,10 @@  void helper_raise_exception(CPURISCVState *env, uint32_t exception)
     riscv_raise_exception(env, exception, 0);
 }
 
-target_ulong helper_csrrw(CPURISCVState *env, target_ulong src,
-        target_ulong csr)
+target_ulong helper_csrr(CPURISCVState *env, int csr)
 {
     target_ulong val = 0;
-    RISCVException ret = riscv_csrrw(env, csr, &val, src, -1);
+    RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
 
     if (ret != RISCV_EXCP_NONE) {
         riscv_raise_exception(env, ret, GETPC());
@@ -49,23 +48,20 @@  target_ulong helper_csrrw(CPURISCVState *env, target_ulong src,
     return val;
 }
 
-target_ulong helper_csrrs(CPURISCVState *env, target_ulong src,
-        target_ulong csr, target_ulong rs1_pass)
+void helper_csrw(CPURISCVState *env, int csr, target_ulong src)
 {
-    target_ulong val = 0;
-    RISCVException ret = riscv_csrrw(env, csr, &val, -1, rs1_pass ? src : 0);
+    RISCVException ret = riscv_csrrw(env, csr, NULL, src, -1);
 
     if (ret != RISCV_EXCP_NONE) {
         riscv_raise_exception(env, ret, GETPC());
     }
-    return val;
 }
 
-target_ulong helper_csrrc(CPURISCVState *env, target_ulong src,
-        target_ulong csr, target_ulong rs1_pass)
+target_ulong helper_csrrw(CPURISCVState *env, int csr,
+                          target_ulong src, target_ulong write_mask)
 {
     target_ulong val = 0;
-    RISCVException ret = riscv_csrrw(env, csr, &val, 0, rs1_pass ? src : 0);
+    RISCVException ret = riscv_csrrw(env, csr, &val, src, write_mask);
 
     if (ret != RISCV_EXCP_NONE) {
         riscv_raise_exception(env, ret, GETPC());
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index 840187a4d6..3705aad380 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -452,80 +452,148 @@  static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a)
     return true;
 }
 
-#define RISCV_OP_CSR_PRE do {\
-    source1 = tcg_temp_new(); \
-    csr_store = tcg_temp_new(); \
-    dest = tcg_temp_new(); \
-    rs1_pass = tcg_temp_new(); \
-    gen_get_gpr(source1, a->rs1); \
-    tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next); \
-    tcg_gen_movi_tl(rs1_pass, a->rs1); \
-    tcg_gen_movi_tl(csr_store, a->csr); \
-    gen_io_start();\
-} while (0)
+static bool do_csr_post(DisasContext *ctx)
+{
+    /* We may have changed important cpu state -- exit to main loop. */
+    tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn);
+    exit_tb(ctx);
+    ctx->base.is_jmp = DISAS_NORETURN;
+    return true;
+}
 
-#define RISCV_OP_CSR_POST do {\
-    gen_set_gpr(a->rd, dest); \
-    tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn); \
-    exit_tb(ctx); \
-    ctx->base.is_jmp = DISAS_NORETURN; \
-    tcg_temp_free(source1); \
-    tcg_temp_free(csr_store); \
-    tcg_temp_free(dest); \
-    tcg_temp_free(rs1_pass); \
-} while (0)
+static bool do_csrr(DisasContext *ctx, int rd, int rc)
+{
+    TCGv dest = gpr_dst(ctx, rd);
+    TCGv_i32 csr = tcg_constant_i32(rc);
 
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_start();
+    }
+    gen_helper_csrr(dest, cpu_env, csr);
+    return do_csr_post(ctx);
+}
+
+static bool do_csrw(DisasContext *ctx, int rc, TCGv src)
+{
+    TCGv_i32 csr = tcg_constant_i32(rc);
+
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_start();
+    }
+    gen_helper_csrw(cpu_env, csr, src);
+    return do_csr_post(ctx);
+}
+
+static bool do_csrrw(DisasContext *ctx, int rd, int rc, TCGv src, TCGv mask)
+{
+    TCGv dest = gpr_dst(ctx, rd);
+    TCGv_i32 csr = tcg_constant_i32(rc);
+
+    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
+        gen_io_start();
+    }
+    gen_helper_csrrw(dest, cpu_env, csr, src, mask);
+    return do_csr_post(ctx);
+}
 
 static bool trans_csrrw(DisasContext *ctx, arg_csrrw *a)
 {
-    TCGv source1, csr_store, dest, rs1_pass;
-    RISCV_OP_CSR_PRE;
-    gen_helper_csrrw(dest, cpu_env, source1, csr_store);
-    RISCV_OP_CSR_POST;
-    return true;
+    TCGv src = gpr_src(ctx, a->rs1);
+
+    /*
+     * If rd == 0, the insn shall not read the csr, nor cause any of the
+     * side effects that might occur on a csr read.
+     */
+    if (a->rd == 0) {
+        return do_csrw(ctx, a->csr, src);
+    }
+
+    TCGv mask = tcg_constant_tl(-1);
+    return do_csrrw(ctx, a->rd, a->csr, src, mask);
 }
 
 static bool trans_csrrs(DisasContext *ctx, arg_csrrs *a)
 {
-    TCGv source1, csr_store, dest, rs1_pass;
-    RISCV_OP_CSR_PRE;
-    gen_helper_csrrs(dest, cpu_env, source1, csr_store, rs1_pass);
-    RISCV_OP_CSR_POST;
-    return true;
+    /*
+     * If rs1 == 0, the insn shall not write to the csr at all, nor
+     * cause any of the side effects that might occur on a csr write.
+     * Note that 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.
+     */
+    if (a->rs1 == 0) {
+        return do_csrr(ctx, a->rd, a->csr);
+    }
+
+    TCGv ones = tcg_constant_tl(-1);
+    TCGv mask = gpr_src(ctx, a->rs1);
+    return do_csrrw(ctx, a->rd, a->csr, ones, mask);
 }
 
 static bool trans_csrrc(DisasContext *ctx, arg_csrrc *a)
 {
-    TCGv source1, csr_store, dest, rs1_pass;
-    RISCV_OP_CSR_PRE;
-    gen_helper_csrrc(dest, cpu_env, source1, csr_store, rs1_pass);
-    RISCV_OP_CSR_POST;
-    return true;
+    /*
+     * If rs1 == 0, the insn shall not write to the csr at all, nor
+     * cause any of the side effects that might occur on a csr write.
+     * Note that 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.
+     */
+    if (a->rs1 == 0) {
+        return do_csrr(ctx, a->rd, a->csr);
+    }
+
+    TCGv mask = gpr_src(ctx, a->rs1);
+    return do_csrrw(ctx, a->rd, a->csr, ctx->zero, mask);
 }
 
 static bool trans_csrrwi(DisasContext *ctx, arg_csrrwi *a)
 {
-    TCGv source1, csr_store, dest, rs1_pass;
-    RISCV_OP_CSR_PRE;
-    gen_helper_csrrw(dest, cpu_env, rs1_pass, csr_store);
-    RISCV_OP_CSR_POST;
-    return true;
+    TCGv src = tcg_constant_tl(a->rs1);
+
+    /*
+     * If rd == 0, the insn shall not read the csr, nor cause any of the
+     * side effects that might occur on a csr read.
+     */
+    if (a->rd == 0) {
+        return do_csrw(ctx, a->csr, src);
+    }
+
+    TCGv mask = tcg_constant_tl(-1);
+    return do_csrrw(ctx, a->rd, a->csr, src, mask);
 }
 
 static bool trans_csrrsi(DisasContext *ctx, arg_csrrsi *a)
 {
-    TCGv source1, csr_store, dest, rs1_pass;
-    RISCV_OP_CSR_PRE;
-    gen_helper_csrrs(dest, cpu_env, rs1_pass, csr_store, rs1_pass);
-    RISCV_OP_CSR_POST;
-    return true;
+    /*
+     * If rs1 == 0, the insn shall not write to the csr at all, nor
+     * cause any of the side effects that might occur on a csr write.
+     * Note that 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.
+     */
+    if (a->rs1 == 0) {
+        return do_csrr(ctx, a->rd, a->csr);
+    }
+
+    TCGv ones = tcg_constant_tl(-1);
+    TCGv mask = tcg_constant_tl(a->rs1);
+    return do_csrrw(ctx, a->rd, a->csr, ones, mask);
 }
 
 static bool trans_csrrci(DisasContext *ctx, arg_csrrci *a)
 {
-    TCGv source1, csr_store, dest, rs1_pass;
-    RISCV_OP_CSR_PRE;
-    gen_helper_csrrc(dest, cpu_env, rs1_pass, csr_store, rs1_pass);
-    RISCV_OP_CSR_POST;
-    return true;
+    /*
+     * If rs1 == 0, the insn shall not write to the csr at all, nor
+     * cause any of the side effects that might occur on a csr write.
+     * Note that 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.
+     */
+    if (a->rs1 == 0) {
+        return do_csrr(ctx, a->rd, a->csr);
+    }
+
+    TCGv mask = tcg_constant_tl(a->rs1);
+    return do_csrrw(ctx, a->rd, a->csr, ctx->zero, mask);
 }