Re: [PATCH v4 05/16] target/riscv: tracking indirect branches (fcfi) for zicfilp

[Deepak Gupta <debug@rivosinc.com>]

On Fri, Aug 16, 2024 at 01:41:51PM +1000, Richard Henderson wrote:
>On 8/16/24 11:06, Deepak Gupta wrote:
>>@@ -1245,6 +1250,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>>  static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
>>  {
>>+
>>  }
>
>Watch the unrelated changes.
>
>>@@ -1266,6 +1272,28 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>>      CPURISCVState *env = cpu_env(cpu);
>>      uint16_t opcode16 = translator_lduw(env, &ctx->base, ctx->base.pc_next);
>>+    if (ctx->fcfi_lp_expected) {
>>+        /*
>>+         * Since we can't look ahead to confirm that the first
>>+         * instruction is a legal landing pad instruction, emit
>>+         * compare-and-branch sequence that will be fixed-up in
>>+         * riscv_tr_tb_stop() to either statically hit or skip an
>>+         * illegal instruction exception depending on whether the
>>+         * flag was lowered by translation of a CJLP or JLP as
>>+         * the first instruction in the block.
>>+         */
>>+        TCGv_i32 immediate;
>>+        TCGLabel *l;
>>+        l = gen_new_label();
>>+        immediate = tcg_temp_new_i32();
>>+        tcg_gen_movi_i32(immediate, 0);
>>+        tcg_ctx->cfi_lp_check = tcg_last_op();
>>+        tcg_gen_brcondi_i32(TCG_COND_EQ, immediate, 0, l);
>>+        gen_helper_raise_sw_check_excep(tcg_env,
>>+                tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL));
>>+        gen_set_label(l);
>>+    }
>>+
>
>I think this is over-complicated.
>
>>      ctx->ol = ctx->xl;
>>      decode_opc(env, ctx, opcode16);
>>      ctx->base.pc_next += ctx->cur_insn_len;
>
>If we delay the check until here, then
>
>(1) we've decoded the opcode, and processed lpad or not.
>(2) we can know that lpad will have cleared ctx->fcfi_lp_expected,
>    so that if it is still set here, then we didn't see an lpad.
>
>We can go back an insert the exception like so:
>
>    if (ctx->fcfi_lp_expected) {
>        /* Emit after insn_start, i.e. before the op following insn_start. */
>        tcg_ctx->emit_before_op = QTAILQ_NEXT(ctx->base.insn_start, link);
>
>        tcg_gen_st_tl(tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
>                      tcg_env, offsetof(CPURISCVState, sw_check_code));
>        gen_helper_raise_exception(tcg_env, tcg_constant_i32(RISCV_EXCP_SW_CHECK));
>
>        tcg_ctx->emit_before_op = NULL;
>        ctx->base.is_jmp = DISAS_NORETURN;
>    }
>

Hmm. Yes this reduces complication of check. Let me do that.

>Emit the store to sw_check_code directly; no need for an extra helper. 
>Using gen_helper_raise_exception instead of generate_exception means 
>we don't get a spurious pc update.

>
>r~