Re: [PATCH v5 11/15] target/riscv: mmu changes for zicfiss shadow stack protection

[Richard Henderson <richard.henderson@linaro.org>]

On 8/20/24 17:35, Deepak Gupta wrote:
>> +    /* If shadow stack instruction initiated this access, treat it as store */
>> +    if (mmu_idx & MMU_IDX_SS_WRITE) {
>> +        access_type = MMU_DATA_STORE;
>> +    }
>> +
> 
> I think I forgot to address this. Do you still want me to fix this up like you
> had suggested?

Yes, this still needs fixing.


> IIRC, you mentioned to use TARGET_INSN_START_EXTRA_WORDS=2. Honestly I don't know
> what it means and how its used. Based on git grep and some readup, are you expecting 
> something
> along the below lines?
> 
> 
> """
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index fee31b8037..dfd2efa941 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -47,7 +47,7 @@ typedef struct CPUArchState CPURISCVState;
>    * RISC-V-specific extra insn start words:
>    * 1: Original instruction opcode
>    */
> -#define TARGET_INSN_START_EXTRA_WORDS 1
> +#define TARGET_INSN_START_EXTRA_WORDS 2
> 
>   #define RV(x) ((target_ulong)1 << (x - 'A'))
> 
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index f74a1216b1..b266177e46 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1271,6 +1271,11 @@ static void raise_mmu_exception(CPURISCVState *env, target_ulong 
> address,
>   {
>       CPUState *cs = env_cpu(env);
> 
> +     if (!pmp_violation &&
> +         tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] & 1) {
> +         tcg_ctx->gen_insn_data[TARGET_INSN_START_EXTRA_WORDS-1] &= ~1;
> +         access_type = MMU_DATA_STORE;
> +     }

The first thing to understand is that the unwind data is stored by the compiler and 
recovered by the unwinder.

The unwind data is exposed to the target via one of two methods:

(1) TCGCPUOps.restore_state_to_opc, i.e. riscv_restore_state_to_opc.
     The data[] argument contains the extra words.

     With this method, the extra words are restored to env and are
     available in a later call to riscv_cpu_do_interrupt.
     Compare env->bins from the first extra word, which is used exactly so.

     This is probably the easiest and best option.
     You'd promote LOAD* to STORE_AMO* while dispatching the interrupt.

(2) cpu_unwind_state_data()

     With this method, you have immediate access to the extra words,
     and don't need to store them anywhere else.

     This is supposed to be used when we are *not* going to raise
     an exception, merely look something up and continue execution.
     Otherwise, we'd be performing the unwind operation twice,
     and it's not cheap.

So, tcg_ctx->gen_insn_data[] is not something you'd ever touch,
and this is the wrong spot to do anything.


r~