[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 18/24] target/riscv: Reorg csr instructions
From: |
Alistair Francis |
Subject: |
Re: [PATCH v5 18/24] target/riscv: Reorg csr instructions |
Date: |
Wed, 25 Aug 2021 16:03:19 +1000 |
On Tue, Aug 24, 2021 at 6:10 AM 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.
> Use get_gpr and dest_gpr.
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> 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 | 172 +++++++++++++++++-------
> 3 files changed, 131 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 76454fb7e2..920ae0edb3 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -426,80 +426,150 @@ 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(ctx, 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(ctx, 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 = dest_gpr(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);
> + gen_set_gpr(ctx, rd, dest);
> + 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 = dest_gpr(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);
> + gen_set_gpr(ctx, rd, dest);
> + 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 = get_gpr(ctx, a->rs1, EXT_NONE);
> +
> + /*
> + * 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 = get_gpr(ctx, a->rs1, EXT_ZERO);
> + 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 = get_gpr(ctx, a->rs1, EXT_ZERO);
> + 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
>
>
- Re: [PATCH v5 16/24] target/riscv: Fix rmw_sip, rmw_vsip, rmw_hsip vs write-only operation, (continued)
- [PATCH v5 13/24] target/riscv: Use extracts for sraiw and srliw, Richard Henderson, 2021/08/23
- [PATCH v5 15/24] target/riscv: Use {get, dest}_gpr for integer load/store, Richard Henderson, 2021/08/23
- [PATCH v5 17/24] target/riscv: Fix hgeie, hgeip, Richard Henderson, 2021/08/23
- [PATCH v5 18/24] target/riscv: Reorg csr instructions, Richard Henderson, 2021/08/23
- Re: [PATCH v5 18/24] target/riscv: Reorg csr instructions,
Alistair Francis <=
- [PATCH v5 20/24] target/riscv: Use gen_shift_imm_fn for slli_uw, Richard Henderson, 2021/08/23
- [PATCH v5 19/24] target/riscv: Use {get,dest}_gpr for RVA, Richard Henderson, 2021/08/23
- [PATCH v5 23/24] target/riscv: Tidy trans_rvh.c.inc, Richard Henderson, 2021/08/23
- [PATCH v5 22/24] target/riscv: Use {get,dest}_gpr for RVD, Richard Henderson, 2021/08/23
- [PATCH v5 21/24] target/riscv: Use {get,dest}_gpr for RVF, Richard Henderson, 2021/08/23