On Fri, Jul 9, 2021 at 2:46 PM Richard Henderson <richard.hender...@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.hender...@linaro.org>
Reviewed-by: Alistair Francis <alistair.fran...@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 > >