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

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

On 8/21/24 04:55, Deepak Gupta wrote:
> Something on the below lines? I've one question as well for you in comment.
> 
> """"
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index fee31b8037..b4e04fe849 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -46,8 +46,14 @@ typedef struct CPUArchState CPURISCVState;
>   /*
>    * RISC-V-specific extra insn start words:
>    * 1: Original instruction opcode
> + * 2: more information about instruction
>    */
> -#define TARGET_INSN_START_EXTRA_WORDS 1
> +#define TARGET_INSN_START_EXTRA_WORDS 2
> +
> +/*
> + * b0: Whether a shadow stack operation/instruction or not.
> + */
> +#define RISCV_INSN_START_WORD2_SS_OP 1

Ah, here: not shadow-stack specific.  Set for any insn which should always generate 
STORE_AMO, including the actual AMO instructions.  It's a current emulation error, IIRC.

> @@ -226,6 +232,7 @@ struct CPUArchState {
>       bool      elp;
>       /* shadow stack register for zicfiss extension */
>       target_ulong ssp;
> +    bool      ss_op;

For generality, maybe just store the whole word as excp_uw2?

>       if (!async) {
> +        /* shadow stack op, promote load page fault to store page fault */
> +        if (env->ss_op && cause == RISCV_EXCP_LOAD_PAGE_FAULT) {
> +            cause = RISCV_EXCP_STORE_PAGE_FAULT;
> +        }
>           /* set tval to badaddr for traps with address information */
>           switch (cause) {
>           case RISCV_EXCP_SEMIHOST:

     case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
         if (env->excp_uw2 & RISCV_UW2_ALWAYS_STORE_AMO) {
             cause = RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT;
         }
         goto load_store_fault;
     case RISCV_EXCP_LOAD_ACCESS_FAULT:
         ...
     case RISCV_EXCP_LOAD_PAGE_FAULT:
         ...
     case RISCV_EXCP_STORE_PAGE_FAULT:
     load_store_fault:

> @@ -1301,6 +1301,14 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, 
> CPUState *cpu)
>           ctx->base.is_jmp = DISAS_NORETURN;
>       }
> 
> +    /* shadow stack index means shadow stack instruction is translated */
> +    if (ctx->mem_idx & MMU_IDX_SS_WRITE) {
> +        /* Is this needed to set true? */
> +        ctx->insn_start_updated = true;
> +        tcg_set_insn_start_param(ctx->base.insn_start, 2,
> +                                 RISCV_INSN_START_WORD2_SS_OP);
> +    }

No, SS_WRITE is never part of mem_idx, and setting insn_start_updated here would break things.

You'll want to change decode_save_opcode() to take the second parameter (or introduce a 
new helper for the second parameter, leaving decode_save_opcode alone).  But you do have 
to handle the update on a per-insn basis.


r~