Re: [PATCH v11 16/20] target/riscv: implement zicfiss instructions

[Alistair Francis]

On Thu, Aug 29, 2024 at 10:06 AM Deepak Gupta <debug@rivosinc.com> wrote:
>
> On Thu, Aug 29, 2024 at 10:01:30AM +1000, Alistair Francis wrote:
> >On Thu, Aug 29, 2024 at 3:53 AM Deepak Gupta <debug@rivosinc.com> wrote:
> >>
> >> zicfiss has following instructions
> >>  - sspopchk: pops a value from shadow stack and compares with x1/x5.
> >>    If they dont match, reports a sw check exception with tval = 3.
> >>  - sspush: pushes value in x1/x5 on shadow stack
> >>  - ssrdp: reads current shadow stack
> >>  - ssamoswap: swaps contents of shadow stack atomically
> >>
> >> sspopchk/sspush/ssrdp default to zimop if zimop implemented and SSE=0
> >>
> >> If SSE=0, ssamoswap is illegal instruction exception.
> >>
> >> This patch implements shadow stack operations for qemu-user and shadow
> >> stack is not protected.
> >>
> >> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> >> Co-developed-by: Jim Shu <jim.shu@sifive.com>
> >> Co-developed-by: Andy Chiu <andy.chiu@sifive.com>
> >> ---
> >>  target/riscv/cpu_bits.h                       |  2 +
> >>  target/riscv/insn32.decode                    | 21 +++++-
> >>  target/riscv/insn_trans/trans_rva.c.inc       | 39 ++++++++++
> >>  target/riscv/insn_trans/trans_rvzicfiss.c.inc | 75 +++++++++++++++++++
> >>  target/riscv/translate.c                      |  5 ++
> >>  5 files changed, 140 insertions(+), 2 deletions(-)
> >>  create mode 100644 target/riscv/insn_trans/trans_rvzicfiss.c.inc
> >>
> >>  # *** Zabhb Standard Extension ***
> >>  amoswap_b  00001 . . ..... ..... 000 ..... 0101111 @atom_st
> >> diff --git a/target/riscv/insn_trans/trans_rva.c.inc 
> >> b/target/riscv/insn_trans/trans_rva.c.inc
> >> index 9cf3ae8019..a2119393a6 100644
> >> --- a/target/riscv/insn_trans/trans_rva.c.inc
> >> +++ b/target/riscv/insn_trans/trans_rva.c.inc
> >> @@ -114,6 +114,25 @@ static bool trans_amoswap_w(DisasContext *ctx, 
> >> arg_amoswap_w *a)
> >>      return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, MO_TESL);
> >>  }
> >>
> >> +static bool trans_ssamoswap_w(DisasContext *ctx, arg_amoswap_w *a)
> >> +{
> >> +    REQUIRE_A_OR_ZAAMO(ctx);
> >> +    if (!ctx->bcfi_enabled) {
> >> +        return false;
> >> +    }
> >> +
> >> +    TCGv dest = dest_gpr(ctx, a->rd);
> >> +    TCGv src1, src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> >> +
> >> +    decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
> >> +    src1 = get_address(ctx, a->rs1, 0);
> >> +
> >> +    tcg_gen_atomic_xchg_tl(dest, src1, src2, SS_MMU_INDEX(ctx),
> >> +                           (MO_ALIGN | MO_TESL));
> >> +    gen_set_gpr(ctx, a->rd, dest);
> >> +    return true;
> >> +}
> >> +
> >>  static bool trans_amoadd_w(DisasContext *ctx, arg_amoadd_w *a)
> >>  {
> >>      REQUIRE_A_OR_ZAAMO(ctx);
> >> @@ -183,6 +202,26 @@ static bool trans_amoswap_d(DisasContext *ctx, 
> >> arg_amoswap_d *a)
> >>      return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, MO_TEUQ);
> >>  }
> >>
> >> +static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a)
> >> +{
> >> +    REQUIRE_64BIT(ctx);
> >> +    REQUIRE_A_OR_ZAAMO(ctx);
> >> +    if (!ctx->bcfi_enabled) {
> >> +        return false;
> >> +    }
> >> +
> >> +    TCGv dest = dest_gpr(ctx, a->rd);
> >> +    TCGv src1, src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> >> +
> >> +    decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
> >> +    src1 = get_address(ctx, a->rs1, 0);
> >> +
> >> +    tcg_gen_atomic_xchg_tl(dest, src1, src2, SS_MMU_INDEX(ctx),
> >> +                           (MO_ALIGN | MO_TESQ));
> >> +    gen_set_gpr(ctx, a->rd, dest);
> >> +    return true;
> >> +}
> >
> >Why aren't these in the rvzicfiss file?
>
> `ssamoswap` encodings are coming from (reserved) AMO encodings (and not zimop)
> That's why kept it in trans_rva

But the instructions are defined in the rvzicfiss extension, so I feel
it makes sense to include them there

Alistair

>
> >
> >Otherwise:
> >
> >Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >
> >Alistair
> >
> >> +
> >>  static bool trans_amoadd_d(DisasContext *ctx, arg_amoadd_d *a)
> >>  {