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

[Deepak Gupta <debug@rivosinc.com>]

On Thu, Aug 29, 2024 at 10:07:15AM +1000, Alistair Francis wrote:
>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

Ok noted.
I'll wait for a day or two. If there are not objections from anyone else, I'll move
them to rvzicfiss.

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