Re: [PATCH v3 12/20] target/riscv: implement zicfiss instructions

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta 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                    |  17 +-
>   target/riscv/insn_trans/trans_rva.c.inc       |  47 ++++++
>   target/riscv/insn_trans/trans_rvzicfiss.c.inc | 149 ++++++++++++++++++
>   target/riscv/translate.c                      |   1 +
>   5 files changed, 214 insertions(+), 2 deletions(-)
>   create mode 100644 target/riscv/insn_trans/trans_rvzicfiss.c.inc
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 226157896d..5ebc4dd5b3 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -702,6 +702,8 @@ typedef enum RISCVException {
>   
>   /* zicfilp defines lp violation results in sw check with tval = 2*/
>   #define RISCV_EXCP_SW_CHECK_FCFI_TVAL      2
> +/* zicfiss defines ss violation results in sw check with tval = 3*/
> +#define RISCV_EXCP_SW_CHECK_BCFI_TVAL      3
>   
>   #define RISCV_EXCP_INT_FLAG                0x80000000
>   #define RISCV_EXCP_INT_MASK                0x7fffffff
> diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> index c963c59c8e..c59c992ce2 100644
> --- a/target/riscv/insn32.decode
> +++ b/target/riscv/insn32.decode
> @@ -65,8 +65,10 @@
>   # Formats 32:
>   @r       .......   ..... ..... ... ..... ....... &r                %rs2 %rs1 
> %rd
>   @i       ............    ..... ... ..... ....... &i      imm=%imm_i     %rs1 
> %rd
> +@ss_pop  ............    ..... ... ..... ....... &i      imm=0 %rs1 rd=0
>   @b       .......   ..... ..... ... ..... ....... &b      imm=%imm_b %rs2 %rs1
>   @s       .......   ..... ..... ... ..... ....... &s      imm=%imm_s %rs2 %rs1
> +@ss_push .......   ..... ..... ... ..... ....... &s      imm=0 %rs2 rs1=0

No need for single-use formats, or even forcing them into specific argument 
sets.

> +{
> +  # zicfiss instructions carved out of mop.r
> +  ssrdp      1100110 11100     00000 100 ..... 1110011 %rd
> +  sspopchk   1100110 11100     ..... 100 00000 1110011 @ss_pop

You can check x1/x5 here:

{
  [
    ssrdp     1100110 111000 00000 100 rd:5  1110011
    sspopchk  1100110 111000 00001 100 00000 1110011  rs1=1
    sspopchk  1100110 111000 00101 100 00000 1110011  rs1=5
  ]
  mop_r_n ...
}

which will make things easier for the next insn carved out of mop_r_n.


> diff --git a/target/riscv/insn_trans/trans_rva.c.inc 
> b/target/riscv/insn_trans/trans_rva.c.inc
> index 39bbf60f3c..db6c03f6a8 100644
> --- a/target/riscv/insn_trans/trans_rva.c.inc
> +++ b/target/riscv/insn_trans/trans_rva.c.inc
> @@ -18,6 +18,8 @@
>    * this program.  If not, see <http://www.gnu.org/licenses/>.
>    */
>   
> +#include "exec/memop.h"
> +
>   #define REQUIRE_A_OR_ZAAMO(ctx) do {                      \
>       if (!ctx->cfg_ptr->ext_zaamo && !has_ext(ctx, RVA)) { \
>           return false;                                     \
> @@ -114,6 +116,28 @@ 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);
> +    /* default for qemu-user, use regular RW memory and thus mmu_idx=0 */
> +    int ss_mmu_idx = 0;
> +
> +    /* back cfi was not enabled, return false */
> +    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);
> +    src1 = get_address(ctx, a->rs1, 0);
> +
> +    tcg_gen_atomic_xchg_tl(dest, src1, src2, ss_mmu_idx, (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 +207,29 @@ 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);
> +    /* default for qemu-user, use regular RW memory and thus mmu_idx=0 */
> +    int ss_mmu_idx = 0;
> +
> +    /* back cfi was not enabled, return false */
> +    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);
> +    src1 = get_address(ctx, a->rs1, 0);
> +
> +    tcg_gen_atomic_xchg_tl(dest, src1, src2, ss_mmu_idx, (MO_ALIGN | MO_TESQ));
> +    gen_set_gpr(ctx, a->rd, dest);
> +    return true;
> +}

Why are these in trans_rva.c.inc instead of in trans_rvzicfiss.c.inc?

> +static MemOp mxl_memop(DisasContext *ctx)
> +{
> +    switch (get_xl(ctx)) {
> +    case MXL_RV32:
> +        return MO_TEUL;
> +
> +    case MXL_RV64:
> +        return MO_TEUQ;
> +
> +    case MXL_RV128:
> +        return MO_TEUO;
> +
> +    default:
> +        g_assert_not_reached();
> +    }
> +}

This should be

  return get_xl(ctx) + 1) | MO_TE;

and probably placed next to get_xlen() et al.

> +
> +static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a)
> +{
> +    /* default for qemu-user, use regular RW memory and thus mmu_idx=0 */
> +    int ss_mmu_idx = 0;

This can't be right, since 0 is M_MODE.

> +
> +    /* sspopchk only supported on 32bit and 64bit */
> +    if (get_xl(ctx) != MXL_RV32 && get_xl(ctx) != MXL_RV64) {
> +        return false;
> +    }

Again, where is this prohibited?  Even if your implementation doesn't allow RV128 (as 
certainly it would be a separate code path here) this should be checked at startup, not 
all over the implementation.

> +    /*
> +     * get data in TCGv using get_gpr
> +     * get addr in TCGv using gen_helper_csrr on CSR_SSP
> +     * use some tcg subtract arithmetic (subtract by XLEN) on addr
> +     * perform ss store on computed address
> +     */
> +
> +    TCGv addr = tcg_temp_new();
> +    TCGLabel *skip = gen_new_label();
> +    uint32_t tmp = (get_xl(ctx) == MXL_RV64) ? 8 : 4;
> +    TCGv_i32 ssp_csr = tcg_constant_i32(CSR_SSP);
> +    TCGv data = tcg_temp_new();
> +    gen_helper_csrr(addr, tcg_env, ssp_csr);

I think you can skip the helper.  You've just validated the extension is 
enabled:

  tcg_gen_ld_tl(addr, tcg_env, offsetof(CPURISCVState, ssp));

> +    TCGv rs1 = get_gpr(ctx, a->rs1, EXT_NONE);
> +    tcg_gen_brcond_tl(TCG_COND_EQ, data, rs1, skip);
> +    gen_helper_raise_sw_check_excep(tcg_env,
> +        tcg_constant_tl(RISCV_EXCP_SW_CHECK_BCFI_TVAL), data, rs1);
> +    gen_set_label(skip);
> +    tcg_gen_addi_tl(addr, addr, tmp);
> +    gen_helper_csrw(tcg_env, ssp_csr, addr);

  tcg_gen_st_tl(addr, tcg_env, ...);

> +static bool trans_sspush(DisasContext *ctx, arg_sspush *a)

Same comments apply.


r~