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

[Richard Henderson]

On 7/30/24 03:53, Deepak Gupta wrote:
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index acba90f170..c746d7df08 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -20,6 +20,7 @@
>   #include "qemu/log.h"
>   #include "cpu.h"
>   #include "tcg/tcg-op.h"
> +#include "tcg/tcg-temp-internal.h"

No, this is internal to tcg, as the filename says.


>   #include "exec/exec-all.h"
>   #include "exec/helper-proto.h"
>   #include "exec/helper-gen.h"
> @@ -44,6 +45,7 @@ static TCGv load_val;
>   /* globals for PM CSRs */
>   static TCGv pm_mask;
>   static TCGv pm_base;
> +static TCGOp *cfi_lp_check;
>   
>   /*
>    * If an operation is being performed on less than TARGET_LONG_BITS,
> @@ -116,6 +118,9 @@ typedef struct DisasContext {
>       bool frm_valid;
>       bool insn_start_updated;
>       const GPtrArray *decoders;
> +    /* zicfilp extension. cfi enabled or not. lp expected or not */
> +    bool fcfi_enabled;
> +    bool fcfi_lp_expected;
>   } DisasContext;
>   
>   static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> @@ -1238,6 +1243,8 @@ static void riscv_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>       ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
>       ctx->ztso = cpu->cfg.ext_ztso;
>       ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
> +    ctx->fcfi_lp_expected = FIELD_EX32(tb_flags, TB_FLAGS, FCFI_LP_EXPECTED);
> +    ctx->fcfi_enabled = cpu_get_fcfien(env) && ctx->fcfi_lp_expected;

This is incorrect.  You cannot check fcfien like this here; you must place it in a tb flag 
like "lp_expected".


> @@ -1245,6 +1252,39 @@ static void riscv_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>   
>   static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
>   {
> +    DisasContext *ctx = container_of(db, DisasContext, base);
> +
> +    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);
> +        cfi_lp_check = tcg_last_op();
> +        tcg_gen_brcondi_i32(TCG_COND_EQ, immediate, 0, l);
> +        tcg_temp_free_i32(immediate);
> +        tcg_gen_st_tl(
> +            tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
> +            tcg_env, offsetof(CPURISCVState, sw_check_code));
> +        generate_exception(ctx, RISCV_EXCP_SW_CHECK);
> +        gen_set_label(l);
> +        /*
> +         * Despite the use of gen_exception_illegal(), the rest of
> +         * the TB needs to be generated. The TCG optimizer will
> +         * clean things up depending on which path ends up being
> +         * active.
> +         */
> +        ctx->base.is_jmp = DISAS_NEXT;
> +    }
>   }

Better to simply delay the check to the first insn load.
See aarch64_tr_translate_insn, dc_isar_feature(aa64_bti, s).


r~