Re: [PATCH v5 18/24] target/riscv: Reorg csr instructions

[Alistair Francis <alistair23@gmail.com>]

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