[PATCH v3 00/20] riscv support for control flow integrity extensions

[PATCH v3 00/20] riscv support for control flow integrity extensions

[Deepak Gupta]

Sending out v3 for riscv zicfilp and zicfiss extensions support in qemu.
I sent out v1 [1] and v2 [2] a while ago.

[1] - https://lists.nongnu.org/archive/html/qemu-devel/2024-07/msg06017.html
[2] - https://lore.kernel.org/all/ed23bcbc-fdc4-4492-803c-daa95880375a@linaro.org/T/

---
v3:
   - Removed prctl specific patches because they need to be upstream
     in kernel first.
   - As suggested by Richard, added TB flag if fcfi enabled
   - Re-worked translation for landing pad and shadow stack instructions
     to not require helper.
   - tcg helpers only for cfi violation cases so that trace hooks can be
     placed.
   - Style changes.
   - fixes assert condition in accel/tcg

v2:
   - added missed file (in v1) for shadow stack instructions implementation.

Deepak Gupta (20):
  accel/tcg: restrict assert on icount_enabled to qemu-system
  target/riscv: Add zicfilp extension
  target/riscv: Introduce elp state and enabling controls for zicfilp
  target/riscv: save and restore elp state on priv transitions
  target/riscv: additional code information for sw check
  target/riscv: tracking indirect branches (fcfi) for zicfilp
  target/riscv: zicfilp `lpad` impl and branch tracking
  disas/riscv: enabled `lpad` disassembly
  target/riscv: Add zicfiss extension
  target/riscv: introduce ssp and enabling controls for zicfiss
  target/riscv: tb flag for shadow stack  instructions
  target/riscv: implement zicfiss instructions
  target/riscv: compressed encodings for sspush and sspopchk
  target/riscv: mmu changes for zicfiss shadow stack protection
  target/riscv: shadow stack mmu index for shadow stack instructions
  disas/riscv: enable disassembly for zicfiss instructions
  disas/riscv: enable disassembly for compressed sspush/sspopchk
  target/riscv: add trace-hooks for each case of sw-check exception
  linux-user: permit RISC-V CFI dynamic entry in VDSO
  linux-user: Add RISC-V zicfilp support in VDSO

 accel/tcg/cpu-exec.c                          |   2 +-
 disas/riscv.c                                 |  71 +++++++-
 disas/riscv.h                                 |   4 +
 linux-user/gen-vdso-elfn.c.inc                |   7 +
 linux-user/riscv/vdso-64.so                   | Bin 3944 -> 4128 bytes
 linux-user/riscv/vdso.S                       |  50 ++++++
 target/riscv/cpu.c                            |  17 ++
 target/riscv/cpu.h                            |  28 +++
 target/riscv/cpu_bits.h                       |  29 +++
 target/riscv/cpu_cfg.h                        |   2 +
 target/riscv/cpu_helper.c                     | 167 +++++++++++++++++-
 target/riscv/cpu_user.h                       |   1 +
 target/riscv/csr.c                            | 106 +++++++++++
 target/riscv/helper.h                         |   3 +
 target/riscv/insn16.decode                    |   4 +
 target/riscv/insn32.decode                    |  23 ++-
 target/riscv/insn_trans/trans_rva.c.inc       |  55 ++++++
 target/riscv/insn_trans/trans_rvi.c.inc       |  68 +++++++
 target/riscv/insn_trans/trans_rvzicfiss.c.inc | 155 ++++++++++++++++
 target/riscv/internals.h                      |   4 +
 target/riscv/op_helper.c                      |  49 +++++
 target/riscv/pmp.c                            |   5 +
 target/riscv/pmp.h                            |   3 +-
 target/riscv/tcg/tcg-cpu.c                    |  20 +++
 target/riscv/trace-events                     |   6 +
 target/riscv/translate.c                      |  75 ++++++++
 26 files changed, 945 insertions(+), 9 deletions(-)
 create mode 100644 target/riscv/insn_trans/trans_rvzicfiss.c.inc

-- 
2.44.0

[PATCH v3 01/20] accel/tcg: restrict assert on icount_enabled to qemu-system

[Deepak Gupta]

commit 16ad9788 [1] restricted icount to qemu-system only. Although
assert in `cpu_loop_exec_tb` is on `icount_enabled()` which is 0 when
its qemu-user and debug build starts asserting.
Move assert for qemu-system.

[1] - https://lists.gnu.org/archive/html/qemu-riscv/2024-01/msg00608.html

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 accel/tcg/cpu-exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 245fd6327d..8cc2a6104f 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -927,9 +927,9 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
         return;
     }
 
+#ifndef CONFIG_USER_ONLY
     /* Instruction counter expired.  */
     assert(icount_enabled());
-#ifndef CONFIG_USER_ONLY
     /* Ensure global icount has gone forward */
     icount_update(cpu);
     /* Refill decrementer and continue execution.  */
-- 
2.44.0

Re: [PATCH v3 01/20] accel/tcg: restrict assert on icount_enabled to qemu-system

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> commit 16ad9788 [1] restricted icount to qemu-system only. Although
> assert in `cpu_loop_exec_tb` is on `icount_enabled()` which is 0 when
> its qemu-user and debug build starts asserting.
> Move assert for qemu-system.
> 
> [1] - https://lists.gnu.org/archive/html/qemu-riscv/2024-01/msg00608.html
> 
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>   accel/tcg/cpu-exec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 245fd6327d..8cc2a6104f 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -927,9 +927,9 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
>           return;
>       }
>   
> +#ifndef CONFIG_USER_ONLY
>       /* Instruction counter expired.  */
>       assert(icount_enabled());
> -#ifndef CONFIG_USER_ONLY
>       /* Ensure global icount has gone forward */
>       icount_update(cpu);
>       /* Refill decrementer and continue execution.  */

No, this is a real bug.

Just above we handled

   (1) exit for tcg (non-)chaining (!= TB_EXIT_REQUESTED),
   (2) exit for exception/interrupt (cpu_loop_exit_requested).

The only thing that is left is exit for icount expired.
And for that we *must* have icount enabled.

How did you encounter this?


r~

Re: [PATCH v3 01/20] accel/tcg: restrict assert on icount_enabled to qemu-system

[Deepak Gupta]

On Wed, Aug 07, 2024 at 10:48:56AM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>commit 16ad9788 [1] restricted icount to qemu-system only. Although
>>assert in `cpu_loop_exec_tb` is on `icount_enabled()` which is 0 when
>>its qemu-user and debug build starts asserting.
>>Move assert for qemu-system.
>>
>>[1] - https://lists.gnu.org/archive/html/qemu-riscv/2024-01/msg00608.html
>>
>>Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>>---
>>  accel/tcg/cpu-exec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>>index 245fd6327d..8cc2a6104f 100644
>>--- a/accel/tcg/cpu-exec.c
>>+++ b/accel/tcg/cpu-exec.c
>>@@ -927,9 +927,9 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
>>          return;
>>      }
>>+#ifndef CONFIG_USER_ONLY
>>      /* Instruction counter expired.  */
>>      assert(icount_enabled());
>>-#ifndef CONFIG_USER_ONLY
>>      /* Ensure global icount has gone forward */
>>      icount_update(cpu);
>>      /* Refill decrementer and continue execution.  */
>
>No, this is a real bug.
>
>Just above we handled
>
>  (1) exit for tcg (non-)chaining (!= TB_EXIT_REQUESTED),
>  (2) exit for exception/interrupt (cpu_loop_exit_requested).
>
>The only thing that is left is exit for icount expired.
>And for that we *must* have icount enabled.
>
>How did you encounter this?

hmm I was experimenting with reducing TB flags (i.e. not using two different TB flags
for zicfilp). But I swear, I didn't see it go away (for qemu-user) when I had switched to
two TB flags.

Now that you've pointed it out specifically, I tried again.
Its not asserting at this place anymore for qemu-ser

I'll remove this patch in next version. And if I encounter this again, will dig a little bit
more deep into it and try to get repro steps.

>
>
>r~

Re: [PATCH v3 01/20] accel/tcg: restrict assert on icount_enabled to qemu-system

[Deepak Gupta]

On Wed, Aug 07, 2024 at 10:48:56AM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>commit 16ad9788 [1] restricted icount to qemu-system only. Although
>>assert in `cpu_loop_exec_tb` is on `icount_enabled()` which is 0 when
>>its qemu-user and debug build starts asserting.
>>Move assert for qemu-system.
>>
>>[1] - https://lists.gnu.org/archive/html/qemu-riscv/2024-01/msg00608.html
>>
>>Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>>---
>>  accel/tcg/cpu-exec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>>index 245fd6327d..8cc2a6104f 100644
>>--- a/accel/tcg/cpu-exec.c
>>+++ b/accel/tcg/cpu-exec.c
>>@@ -927,9 +927,9 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
>>          return;
>>      }
>>+#ifndef CONFIG_USER_ONLY
>>      /* Instruction counter expired.  */
>>      assert(icount_enabled());
>>-#ifndef CONFIG_USER_ONLY
>>      /* Ensure global icount has gone forward */
>>      icount_update(cpu);
>>      /* Refill decrementer and continue execution.  */
>
>No, this is a real bug.
>
>Just above we handled
>
>  (1) exit for tcg (non-)chaining (!= TB_EXIT_REQUESTED),
>  (2) exit for exception/interrupt (cpu_loop_exit_requested).
>
>The only thing that is left is exit for icount expired.
>And for that we *must* have icount enabled.
>
>How did you encounter this?

I spent last week incorporating your suggestions. And during the flux of it, I started
seeing this issue again. As soon as I switch to branch from where I sent the patches out,
this icount assert issue disappears. So something definitley is triggering the issue.
It happens only when zicfilp and zicfiss are enabled.

I am still trying to root cause and in a fog right now. Although I am not very well versed
with tcg internals. So any pointer here which could help me debug it faster would be well
appreciated. Thanks.


>
>
>r~

[PATCH v3 02/20] target/riscv: Add zicfilp extension

[Deepak Gupta]

zicfilp [1] riscv cpu extension enables forward control flow integrity.
If enabled, all indirect calls must land on a landing pad instruction.

This patch sets up space for zicfilp extension in cpuconfig. zicfilp
is dependend on zicsr.

[1] - https://github.com/riscv/riscv-cfi

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.c         | 2 ++
 target/riscv/cpu_cfg.h     | 1 +
 target/riscv/tcg/tcg-cpu.c | 5 +++++
 3 files changed, 8 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 33ef4eb795..5dfb3f39ab 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -106,6 +106,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, has_priv_1_11),
     ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, has_priv_1_11),
     ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, has_priv_1_11),
+    ISA_EXT_DATA_ENTRY(zicfilp, PRIV_VERSION_1_12_0, ext_zicfilp),
     ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
     ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
     ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
@@ -1472,6 +1473,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     /* Defaults for standard extensions */
     MULTI_EXT_CFG_BOOL("sscofpmf", ext_sscofpmf, false),
     MULTI_EXT_CFG_BOOL("zifencei", ext_zifencei, true),
+    MULTI_EXT_CFG_BOOL("zicfilp", ext_zicfilp, false),
     MULTI_EXT_CFG_BOOL("zicsr", ext_zicsr, true),
     MULTI_EXT_CFG_BOOL("zihintntl", ext_zihintntl, true),
     MULTI_EXT_CFG_BOOL("zihintpause", ext_zihintpause, true),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 120905a254..88d5defbb5 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -67,6 +67,7 @@ struct RISCVCPUConfig {
     bool ext_zicbom;
     bool ext_zicbop;
     bool ext_zicboz;
+    bool ext_zicfilp;
     bool ext_zicond;
     bool ext_zihintntl;
     bool ext_zihintpause;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index b8814ab753..ed19586c9d 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -623,6 +623,11 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->pmu_avail_ctrs = 0;
     }
 
+    if (cpu->cfg.ext_zicfilp && !cpu->cfg.ext_zicsr) {
+        error_setg(errp, "zicfilp extension requires zicsr extension");
+        return;
+    }
+
     /*
      * Disable isa extensions based on priv spec after we
      * validated and set everything we need.
-- 
2.44.0

[PATCH v3 03/20] target/riscv: Introduce elp state and enabling controls for zicfilp

[Deepak Gupta]

zicfilp introduces a new state elp ("expected landing pad") in cpu.
During normal execution, elp is idle (NO_LP_EXPECTED) i.e not expecting
landing pad. On an indirect call, elp moves LP_EXPECTED. When elp is
LP_EXPECTED, only a subsquent landing pad instruction can set state back
to NO_LP_EXPECTED. On reset, elp is set to NO_LP_EXPECTED.

zicfilp is enabled via bit2 in *envcfg CSRs. Enabling control for M-mode
is in mseccfg CSR at bit position 10.

On trap, elp state is saved away in *status.

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.c      |  3 +++
 target/riscv/cpu.h      |  2 ++
 target/riscv/cpu_bits.h | 12 ++++++++++++
 target/riscv/csr.c      | 31 +++++++++++++++++++++++++++++++
 target/riscv/pmp.c      |  5 +++++
 target/riscv/pmp.h      |  3 ++-
 6 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5dfb3f39ab..82fa85a8d6 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -994,6 +994,9 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
     /* mmte is supposed to have pm.current hardwired to 1 */
     env->mmte |= (EXT_STATUS_INITIAL | MMTE_M_PM_CURRENT);
 
+    /* on reset elp is set to NO_LP_EXPECTED */
+    env->elp = NO_LP_EXPECTED;
+
     /*
      * Bits 10, 6, 2 and 12 of mideleg are read only 1 when the Hypervisor
      * extension is enabled.
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 87742047ce..ae436a3179 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -222,6 +222,8 @@ struct CPUArchState {
 
     target_ulong jvt;
 
+    /* elp state for zicfilp extension */
+    cfi_elp      elp;
 #ifdef CONFIG_USER_ONLY
     uint32_t elf_flags;
 #endif
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index c257c5ed7d..127f2179dc 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -545,6 +545,8 @@
 #define MSTATUS_TVM         0x00100000 /* since: priv-1.10 */
 #define MSTATUS_TW          0x00200000 /* since: priv-1.10 */
 #define MSTATUS_TSR         0x00400000 /* since: priv-1.10 */
+#define MSTATUS_SPELP       0x00800000 /* zicfilp */
+#define MSTATUS_MPELP       0x020000000000 /* zicfilp */
 #define MSTATUS_GVA         0x4000000000ULL
 #define MSTATUS_MPV         0x8000000000ULL
 
@@ -575,12 +577,19 @@ typedef enum {
 #define SSTATUS_XS          0x00018000
 #define SSTATUS_SUM         0x00040000 /* since: priv-1.10 */
 #define SSTATUS_MXR         0x00080000
+#define SSTATUS_SPELP       MSTATUS_SPELP   /* zicfilp */
 
 #define SSTATUS64_UXL       0x0000000300000000ULL
 
 #define SSTATUS32_SD        0x80000000
 #define SSTATUS64_SD        0x8000000000000000ULL
 
+/* enum for branch tracking state in cpu/hart */
+typedef enum {
+    NO_LP_EXPECTED = 0,
+    LP_EXPECTED = 1,
+} cfi_elp;
+
 /* hstatus CSR bits */
 #define HSTATUS_VSBE         0x00000020
 #define HSTATUS_GVA          0x00000040
@@ -747,6 +756,7 @@ typedef enum RISCVException {
 
 /* Execution environment configuration bits */
 #define MENVCFG_FIOM                       BIT(0)
+#define MENVCFG_LPE                        BIT(2) /* zicfilp */
 #define MENVCFG_CBIE                       (3UL << 4)
 #define MENVCFG_CBCFE                      BIT(6)
 #define MENVCFG_CBZE                       BIT(7)
@@ -760,11 +770,13 @@ typedef enum RISCVException {
 #define MENVCFGH_STCE                      BIT(31)
 
 #define SENVCFG_FIOM                       MENVCFG_FIOM
+#define SENVCFG_LPE                        MENVCFG_LPE
 #define SENVCFG_CBIE                       MENVCFG_CBIE
 #define SENVCFG_CBCFE                      MENVCFG_CBCFE
 #define SENVCFG_CBZE                       MENVCFG_CBZE
 
 #define HENVCFG_FIOM                       MENVCFG_FIOM
+#define HENVCFG_LPE                        MENVCFG_LPE
 #define HENVCFG_CBIE                       MENVCFG_CBIE
 #define HENVCFG_CBCFE                      MENVCFG_CBCFE
 #define HENVCFG_CBZE                       MENVCFG_CBZE
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 432c59dc66..5771a14848 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1400,6 +1400,11 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
         }
     }
 
+    /* If cfi lp extension is available, then apply cfi lp mask */
+    if (env_archcpu(env)->cfg.ext_zicfilp) {
+        mask |= (MSTATUS_MPELP | MSTATUS_SPELP);
+    }
+
     mstatus = (mstatus & ~mask) | (val & mask);
 
     env->mstatus = mstatus;
@@ -2101,6 +2106,10 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
         mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
                 (cfg->ext_sstc ? MENVCFG_STCE : 0) |
                 (cfg->ext_svadu ? MENVCFG_ADUE : 0);
+
+        if (env_archcpu(env)->cfg.ext_zicfilp) {
+            mask |= MENVCFG_LPE;
+        }
     }
     env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
 
@@ -2153,6 +2162,10 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
         return ret;
     }
 
+    if (env_archcpu(env)->cfg.ext_zicfilp) {
+        mask |= SENVCFG_LPE;
+    }
+
     env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
     return RISCV_EXCP_NONE;
 }
@@ -2190,6 +2203,10 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
 
     if (riscv_cpu_mxl(env) == MXL_RV64) {
         mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
+
+        if (env_archcpu(env)->cfg.ext_zicfilp) {
+            mask |= HENVCFG_LPE;
+        }
     }
 
     env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
@@ -2654,6 +2671,10 @@ static RISCVException read_sstatus_i128(CPURISCVState *env, int csrno,
         mask |= SSTATUS64_UXL;
     }
 
+    if (env_archcpu(env)->cfg.ext_zicfilp) {
+        mask |= SSTATUS_SPELP;
+    }
+
     *val = int128_make128(sstatus, add_status_sd(MXL_RV128, sstatus));
     return RISCV_EXCP_NONE;
 }
@@ -2665,6 +2686,11 @@ static RISCVException read_sstatus(CPURISCVState *env, int csrno,
     if (env->xl != MXL_RV32 || env->debugger) {
         mask |= SSTATUS64_UXL;
     }
+
+    if (env_archcpu(env)->cfg.ext_zicfilp) {
+        mask |= SSTATUS_SPELP;
+    }
+
     /* TODO: Use SXL not MXL. */
     *val = add_status_sd(riscv_cpu_mxl(env), env->mstatus & mask);
     return RISCV_EXCP_NONE;
@@ -2680,6 +2706,11 @@ static RISCVException write_sstatus(CPURISCVState *env, int csrno,
             mask |= SSTATUS64_UXL;
         }
     }
+
+    if (env_archcpu(env)->cfg.ext_zicfilp) {
+        mask |= SSTATUS_SPELP;
+    }
+
     target_ulong newval = (env->mstatus & ~mask) | (val & mask);
     return write_mstatus(env, CSR_MSTATUS, newval);
 }
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 9eea397e72..1111d08d08 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -598,6 +598,11 @@ void mseccfg_csr_write(CPURISCVState *env, target_ulong val)
         val &= ~(MSECCFG_MMWP | MSECCFG_MML | MSECCFG_RLB);
     }
 
+    /* M-mode forward cfi to be enabled if cfi extension is implemented */
+    if (env_archcpu(env)->cfg.ext_zicfilp) {
+        val |= (val & MSECCFG_MLPE);
+    }
+
     env->mseccfg = val;
 }
 
diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
index f5c10ce85c..e0530a17a3 100644
--- a/target/riscv/pmp.h
+++ b/target/riscv/pmp.h
@@ -44,7 +44,8 @@ typedef enum {
     MSECCFG_MMWP  = 1 << 1,
     MSECCFG_RLB   = 1 << 2,
     MSECCFG_USEED = 1 << 8,
-    MSECCFG_SSEED = 1 << 9
+    MSECCFG_SSEED = 1 << 9,
+    MSECCFG_MLPE =  1 << 10,
 } mseccfg_field_t;
 
 typedef struct {
-- 
2.44.0

Re: [PATCH v3 03/20] target/riscv: Introduce elp state and enabling controls for zicfilp

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> +/* enum for branch tracking state in cpu/hart */
> +typedef enum {
> +    NO_LP_EXPECTED = 0,
> +    LP_EXPECTED = 1,
> +} cfi_elp;

I know this is language is in the spec, but would it make more sense to use

   bool elp_expected;

?

If not, Coding Style requires CamelCase for typedefs.


r~

Re: [PATCH v3 03/20] target/riscv: Introduce elp state and enabling controls for zicfilp

[Deepak Gupta]

On Wed, Aug 07, 2024 at 10:56:12AM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>+/* enum for branch tracking state in cpu/hart */
>>+typedef enum {
>>+    NO_LP_EXPECTED = 0,
>>+    LP_EXPECTED = 1,
>>+} cfi_elp;
>
>I know this is language is in the spec, but would it make more sense to use
>
>  bool elp_expected;
>
>?

Sounds reasonable to me. Will take the suggestion.

>
>If not, Coding Style requires CamelCase for typedefs.
>
>
>r~

[PATCH v3 04/20] target/riscv: save and restore elp state on priv transitions

[Deepak Gupta]

elp state is recorded in *status on trap entry (less privilege to higher
privilege) and restored in elp from *status on trap exit (higher to less
privilege).

Additionally this patch introduces a forward cfi helper function to
determine if current privilege has forward cfi is enabled or not based on
*envcfg (for U, VU, S, VU, HS) or mseccfg csr (for M). For qemu-user, a
new field `ufcfien` is introduced which is by default set to false and
helper function returns value deposited in `ufcfien` for qemu-user.

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.c        |  5 ++++
 target/riscv/cpu.h        |  2 ++
 target/riscv/cpu_helper.c | 58 +++++++++++++++++++++++++++++++++++++++
 target/riscv/op_helper.c  | 18 ++++++++++++
 4 files changed, 83 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 82fa85a8d6..e1526c7ab5 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1022,6 +1022,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
     env->load_res = -1;
     set_default_nan_mode(1, &env->fp_status);
 
+#ifdef CONFIG_USER_ONLY
+    /* qemu-user for riscv, fcfi is off by default */
+    env->ufcfien = false;
+#endif
+
 #ifndef CONFIG_USER_ONLY
     if (cpu->cfg.debug) {
         riscv_trigger_reset_hold(env);
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index ae436a3179..8c7841fc08 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -226,6 +226,7 @@ struct CPUArchState {
     cfi_elp      elp;
 #ifdef CONFIG_USER_ONLY
     uint32_t elf_flags;
+    bool ufcfien;
 #endif
 
 #ifndef CONFIG_USER_ONLY
@@ -530,6 +531,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env, target_ulong geilen);
 bool riscv_cpu_vector_enabled(CPURISCVState *env);
 void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
 int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
+bool cpu_get_fcfien(CPURISCVState *env);
 G_NORETURN void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
                                                MMUAccessType access_type,
                                                int mmu_idx, uintptr_t retaddr);
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 6709622dd3..8c69c55576 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -33,6 +33,7 @@
 #include "cpu_bits.h"
 #include "debug.h"
 #include "tcg/oversized-guest.h"
+#include "pmp.h"
 
 int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
 {
@@ -63,6 +64,35 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
 #endif
 }
 
+bool cpu_get_fcfien(CPURISCVState *env)
+{
+#ifdef CONFIG_USER_ONLY
+    return env->ufcfien;
+#else
+    /* no cfi extension, return false */
+    if (!env_archcpu(env)->cfg.ext_zicfilp) {
+        return false;
+    }
+
+    switch (env->priv) {
+    case PRV_U:
+        if (riscv_has_ext(env, RVS)) {
+            return env->senvcfg & MENVCFG_LPE;
+        }
+        return env->menvcfg & MENVCFG_LPE;
+    case PRV_S:
+        if (env->virt_enabled) {
+            return env->henvcfg & HENVCFG_LPE;
+        }
+        return env->menvcfg & MENVCFG_LPE;
+    case PRV_M:
+        return env->mseccfg & MSECCFG_MLPE;
+    default:
+        g_assert_not_reached();
+    }
+#endif
+}
+
 void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
                           uint64_t *cs_base, uint32_t *pflags)
 {
@@ -546,6 +576,15 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env)
     }
     bool current_virt = env->virt_enabled;
 
+    /*
+     * If zicfilp extension available and henvcfg.LPE = 1,
+     * then apply SPELP mask on mstatus
+     */
+    if (env_archcpu(env)->cfg.ext_zicfilp &&
+        get_field(env->henvcfg, HENVCFG_LPE)) {
+        mstatus_mask |= SSTATUS_SPELP;
+    }
+
     g_assert(riscv_has_ext(env, RVH));
 
     if (current_virt) {
@@ -1754,6 +1793,11 @@ void riscv_cpu_do_interrupt(CPUState *cs)
     if (env->priv <= PRV_S && cause < 64 &&
         (((deleg >> cause) & 1) || s_injected || vs_injected)) {
         /* handle the trap in S-mode */
+        /* save elp status */
+        if (cpu_get_fcfien(env)) {
+            env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, env->elp);
+        }
+
         if (riscv_has_ext(env, RVH)) {
             uint64_t hdeleg = async ? env->hideleg : env->hedeleg;
 
@@ -1802,6 +1846,11 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         riscv_cpu_set_mode(env, PRV_S);
     } else {
         /* handle the trap in M-mode */
+        /* save elp status */
+        if (cpu_get_fcfien(env)) {
+            env->mstatus = set_field(env->mstatus, MSTATUS_MPELP, env->elp);
+        }
+
         if (riscv_has_ext(env, RVH)) {
             if (env->virt_enabled) {
                 riscv_cpu_swap_hypervisor_regs(env);
@@ -1833,6 +1882,15 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         riscv_cpu_set_mode(env, PRV_M);
     }
 
+    /*
+     * Interrupt/exception/trap delivery is asynchronous event and as per
+     * Zisslpcfi spec CPU should clear up the ELP state. If cfi extension is
+     * available, clear ELP state.
+     */
+
+    if (cpu->cfg.ext_zicfilp) {
+        env->elp = NO_LP_EXPECTED;
+    }
     /*
      * NOTE: it is not necessary to yield load reservations here. It is only
      * necessary for an SC from "another hart" to cause a load reservation
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 2baf5bc3ca..488116cc2e 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -313,6 +313,15 @@ target_ulong helper_sret(CPURISCVState *env)
 
     riscv_cpu_set_mode(env, prev_priv);
 
+    /*
+     * If forward cfi enabled for new priv, restore elp status
+     * and clear spelp in mstatus
+     */
+    if (cpu_get_fcfien(env)) {
+        env->elp = get_field(env->mstatus, MSTATUS_SPELP);
+        env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, 0);
+    }
+
     return retpc;
 }
 
@@ -357,6 +366,15 @@ target_ulong helper_mret(CPURISCVState *env)
         riscv_cpu_set_virt_enabled(env, prev_virt);
     }
 
+    /*
+     * If forward cfi enabled for new priv, restore elp status
+     * and clear mpelp in mstatus
+     */
+    if (cpu_get_fcfien(env)) {
+        env->elp = get_field(env->mstatus, MSTATUS_MPELP);
+        env->mstatus = set_field(env->mstatus, MSTATUS_MPELP, 0);
+    }
+
     return retpc;
 }
 
-- 
2.44.0

Re: [PATCH v3 04/20] target/riscv: save and restore elp state on priv transitions

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> elp state is recorded in *status on trap entry (less privilege to higher
> privilege) and restored in elp from *status on trap exit (higher to less
> privilege).
> 
> Additionally this patch introduces a forward cfi helper function to
> determine if current privilege has forward cfi is enabled or not based on
> *envcfg (for U, VU, S, VU, HS) or mseccfg csr (for M). For qemu-user, a
> new field `ufcfien` is introduced which is by default set to false and
> helper function returns value deposited in `ufcfien` for qemu-user.
> 
> 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.c        |  5 ++++
>   target/riscv/cpu.h        |  2 ++
>   target/riscv/cpu_helper.c | 58 +++++++++++++++++++++++++++++++++++++++
>   target/riscv/op_helper.c  | 18 ++++++++++++
>   4 files changed, 83 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 82fa85a8d6..e1526c7ab5 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1022,6 +1022,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>       env->load_res = -1;
>       set_default_nan_mode(1, &env->fp_status);
>   
> +#ifdef CONFIG_USER_ONLY
> +    /* qemu-user for riscv, fcfi is off by default */
> +    env->ufcfien = false;
> +#endif
> +
>   #ifndef CONFIG_USER_ONLY
>       if (cpu->cfg.debug) {
>           riscv_trigger_reset_hold(env);
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index ae436a3179..8c7841fc08 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -226,6 +226,7 @@ struct CPUArchState {
>       cfi_elp      elp;
>   #ifdef CONFIG_USER_ONLY
>       uint32_t elf_flags;
> +    bool ufcfien;
>   #endif
>   
>   #ifndef CONFIG_USER_ONLY
> @@ -530,6 +531,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env, target_ulong geilen);
>   bool riscv_cpu_vector_enabled(CPURISCVState *env);
>   void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
>   int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
> +bool cpu_get_fcfien(CPURISCVState *env);
>   G_NORETURN void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>                                                  MMUAccessType access_type,
>                                                  int mmu_idx, uintptr_t retaddr);
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 6709622dd3..8c69c55576 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -33,6 +33,7 @@
>   #include "cpu_bits.h"
>   #include "debug.h"
>   #include "tcg/oversized-guest.h"
> +#include "pmp.h"
>   
>   int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
>   {
> @@ -63,6 +64,35 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
>   #endif
>   }
>   
> +bool cpu_get_fcfien(CPURISCVState *env)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    return env->ufcfien;
> +#else
> +    /* no cfi extension, return false */
> +    if (!env_archcpu(env)->cfg.ext_zicfilp) {
> +        return false;
> +    }

Keep extension check common between user/system.
Recall that you can choose -cpu from user as well.

> +    /*
> +     * Interrupt/exception/trap delivery is asynchronous event and as per
> +     * Zisslpcfi spec CPU should clear up the ELP state. If cfi extension is
> +     * available, clear ELP state.
> +     */
> +
> +    if (cpu->cfg.ext_zicfilp) {
> +        env->elp = NO_LP_EXPECTED;
> +    }

If extension is not available, elp isn't a thing.
You can just as easily make the store unconditional and save the test.

> 
> +    /*
> +     * If forward cfi enabled for new priv, restore elp status
> +     * and clear spelp in mstatus
> +     */
> +    if (cpu_get_fcfien(env)) {
> +        env->elp = get_field(env->mstatus, MSTATUS_SPELP);
> +        env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, 0);
> +    }

The spec is perhaps poorly written here.  I read

   ... if xPP holds the value y, then ELP is set to the value of xPELP if yLPE is 1;
   otherwise, it is set to NO_LP_EXPECTED; xPELP is set to NO_LP_EXPECTED.

as xPELP always being cleared, regardless of yLPE.


r~

Re: [PATCH v3 04/20] target/riscv: save and restore elp state on priv transitions

[Deepak Gupta]

On Wed, Aug 07, 2024 at 11:06:49AM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>elp state is recorded in *status on trap entry (less privilege to higher
>>privilege) and restored in elp from *status on trap exit (higher to less
>>privilege).
>>
>>Additionally this patch introduces a forward cfi helper function to
>>determine if current privilege has forward cfi is enabled or not based on
>>*envcfg (for U, VU, S, VU, HS) or mseccfg csr (for M). For qemu-user, a
>>new field `ufcfien` is introduced which is by default set to false and
>>helper function returns value deposited in `ufcfien` for qemu-user.
>>
>>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.c        |  5 ++++
>>  target/riscv/cpu.h        |  2 ++
>>  target/riscv/cpu_helper.c | 58 +++++++++++++++++++++++++++++++++++++++
>>  target/riscv/op_helper.c  | 18 ++++++++++++
>>  4 files changed, 83 insertions(+)
>>
>>diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>index 82fa85a8d6..e1526c7ab5 100644
>>--- a/target/riscv/cpu.c
>>+++ b/target/riscv/cpu.c
>>@@ -1022,6 +1022,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>>      env->load_res = -1;
>>      set_default_nan_mode(1, &env->fp_status);
>>+#ifdef CONFIG_USER_ONLY
>>+    /* qemu-user for riscv, fcfi is off by default */
>>+    env->ufcfien = false;
>>+#endif
>>+
>>  #ifndef CONFIG_USER_ONLY
>>      if (cpu->cfg.debug) {
>>          riscv_trigger_reset_hold(env);
>>diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>index ae436a3179..8c7841fc08 100644
>>--- a/target/riscv/cpu.h
>>+++ b/target/riscv/cpu.h
>>@@ -226,6 +226,7 @@ struct CPUArchState {
>>      cfi_elp      elp;
>>  #ifdef CONFIG_USER_ONLY
>>      uint32_t elf_flags;
>>+    bool ufcfien;
>>  #endif
>>  #ifndef CONFIG_USER_ONLY
>>@@ -530,6 +531,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env, target_ulong geilen);
>>  bool riscv_cpu_vector_enabled(CPURISCVState *env);
>>  void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
>>  int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
>>+bool cpu_get_fcfien(CPURISCVState *env);
>>  G_NORETURN void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>>                                                 MMUAccessType access_type,
>>                                                 int mmu_idx, uintptr_t retaddr);
>>diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>index 6709622dd3..8c69c55576 100644
>>--- a/target/riscv/cpu_helper.c
>>+++ b/target/riscv/cpu_helper.c
>>@@ -33,6 +33,7 @@
>>  #include "cpu_bits.h"
>>  #include "debug.h"
>>  #include "tcg/oversized-guest.h"
>>+#include "pmp.h"
>>  int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
>>  {
>>@@ -63,6 +64,35 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
>>  #endif
>>  }
>>+bool cpu_get_fcfien(CPURISCVState *env)
>>+{
>>+#ifdef CONFIG_USER_ONLY
>>+    return env->ufcfien;
>>+#else
>>+    /* no cfi extension, return false */
>>+    if (!env_archcpu(env)->cfg.ext_zicfilp) {
>>+        return false;
>>+    }
>
>Keep extension check common between user/system.
>Recall that you can choose -cpu from user as well.

Ack. will put a check (for both extensions)

Side note: ufcfien (or ubcfien) will get set only via syscall prctls which does
check for extension.

>
>>+    /*
>>+     * Interrupt/exception/trap delivery is asynchronous event and as per
>>+     * Zisslpcfi spec CPU should clear up the ELP state. If cfi extension is
>>+     * available, clear ELP state.
>>+     */
>>+
>>+    if (cpu->cfg.ext_zicfilp) {
>>+        env->elp = NO_LP_EXPECTED;
>>+    }
>
>If extension is not available, elp isn't a thing.
>You can just as easily make the store unconditional and save the test.

Yes noted. make sense.

>
>>
>>+    /*
>>+     * If forward cfi enabled for new priv, restore elp status
>>+     * and clear spelp in mstatus
>>+     */
>>+    if (cpu_get_fcfien(env)) {
>>+        env->elp = get_field(env->mstatus, MSTATUS_SPELP);
>>+        env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, 0);
>>+    }
>
>The spec is perhaps poorly written here.  I read
>
>  ... if xPP holds the value y, then ELP is set to the value of xPELP if yLPE is 1;
>  otherwise, it is set to NO_LP_EXPECTED; xPELP is set to NO_LP_EXPECTED.
>
>as xPELP always being cleared, regardless of yLPE.

Yes that's what code above is also doing. restore elp status from SPELP field and clear
it at SPELP.

On `mret` same logic but for MPELP bitposition.

>
>
>r~

Re: [PATCH v3 04/20] target/riscv: save and restore elp state on priv transitions

[Richard Henderson]

On 8/8/24 06:11, Deepak Gupta wrote:
>>> +    /*
>>> +     * If forward cfi enabled for new priv, restore elp status
>>> +     * and clear spelp in mstatus
>>> +     */
>>> +    if (cpu_get_fcfien(env)) {
>>> +        env->elp = get_field(env->mstatus, MSTATUS_SPELP);
>>> +        env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, 0);
>>> +    }
>>
>> The spec is perhaps poorly written here.  I read
>>
>>  ... if xPP holds the value y, then ELP is set to the value of xPELP if yLPE is 1;
>>  otherwise, it is set to NO_LP_EXPECTED; xPELP is set to NO_LP_EXPECTED.
>>
>> as xPELP always being cleared, regardless of yLPE.
> 
> Yes that's what code above is also doing. restore elp status from SPELP field and clear
> it at SPELP.

No, my point is that the text doesn't seem to be

   if (enabled) {
     restore elp
     clear pelp
   }

but

   if (enabled) {
     restore elp
   }
   clear pelp

I.e. the clear is unconditional.


r~

Re: [PATCH v3 04/20] target/riscv: save and restore elp state on priv transitions

[Deepak Gupta]

On Thu, Aug 08, 2024 at 08:40:08AM +1000, Richard Henderson wrote:
>On 8/8/24 06:11, Deepak Gupta wrote:
>>>>+    /*
>>>>+     * If forward cfi enabled for new priv, restore elp status
>>>>+     * and clear spelp in mstatus
>>>>+     */
>>>>+    if (cpu_get_fcfien(env)) {
>>>>+        env->elp = get_field(env->mstatus, MSTATUS_SPELP);
>>>>+        env->mstatus = set_field(env->mstatus, MSTATUS_SPELP, 0);
>>>>+    }
>>>
>>>The spec is perhaps poorly written here.  I read
>>>
>>> ... if xPP holds the value y, then ELP is set to the value of xPELP if yLPE is 1;
>>> otherwise, it is set to NO_LP_EXPECTED; xPELP is set to NO_LP_EXPECTED.
>>>
>>>as xPELP always being cleared, regardless of yLPE.
>>
>>Yes that's what code above is also doing. restore elp status from SPELP field and clear
>>it at SPELP.
>
>No, my point is that the text doesn't seem to be
>
>  if (enabled) {
>    restore elp
>    clear pelp
>  }
>
>but
>
>  if (enabled) {
>    restore elp
>  }
>  clear pelp
>
>I.e. the clear is unconditional.

hmm. that's right. 
good catch here.

>
>
>r~

[PATCH v3 05/20] target/riscv: additional code information for sw check

[Deepak Gupta]

sw check exception support was recently added. This patch further augments
sw check exception by providing support for additional code which is
provided in *tval. Adds `sw_check_code` field in cpuarchstate. Whenever
sw check exception is raised *tval gets the value deposited in
`sw_check_code`.

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 target/riscv/cpu.h        | 2 ++
 target/riscv/cpu_helper.c | 2 ++
 target/riscv/csr.c        | 1 +
 3 files changed, 5 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 8c7841fc08..12334f9540 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -224,6 +224,8 @@ struct CPUArchState {
 
     /* elp state for zicfilp extension */
     cfi_elp      elp;
+    /* sw check code for sw check exception */
+    target_ulong sw_check_code;
 #ifdef CONFIG_USER_ONLY
     uint32_t elf_flags;
     bool ufcfien;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 8c69c55576..364f3ee212 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1762,6 +1762,8 @@ void riscv_cpu_do_interrupt(CPUState *cs)
                 cs->watchpoint_hit = NULL;
             }
             break;
+        case RISCV_EXCP_SW_CHECK:
+            tval = env->sw_check_code;
         default:
             break;
         }
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 5771a14848..a5a969a377 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1179,6 +1179,7 @@ static const uint64_t all_ints = M_MODE_INTERRUPTS | S_MODE_INTERRUPTS |
                          (1ULL << (RISCV_EXCP_INST_PAGE_FAULT)) | \
                          (1ULL << (RISCV_EXCP_LOAD_PAGE_FAULT)) | \
                          (1ULL << (RISCV_EXCP_STORE_PAGE_FAULT)) | \
+                         (1ULL << (RISCV_EXCP_SW_CHECK)) | \
                          (1ULL << (RISCV_EXCP_INST_GUEST_PAGE_FAULT)) | \
                          (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) | \
                          (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) | \
-- 
2.44.0

Re: [PATCH v3 05/20] target/riscv: additional code information for sw check

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> sw check exception support was recently added. This patch further augments
> sw check exception by providing support for additional code which is
> provided in *tval. Adds `sw_check_code` field in cpuarchstate. Whenever
> sw check exception is raised *tval gets the value deposited in
> `sw_check_code`.
> 
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>   target/riscv/cpu.h        | 2 ++
>   target/riscv/cpu_helper.c | 2 ++
>   target/riscv/csr.c        | 1 +
>   3 files changed, 5 insertions(+)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 8c7841fc08..12334f9540 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -224,6 +224,8 @@ struct CPUArchState {
>   
>       /* elp state for zicfilp extension */
>       cfi_elp      elp;
> +    /* sw check code for sw check exception */
> +    target_ulong sw_check_code;

There's probably room for consolidating the different fields
that feed into tval, to be set when raising the exception.

But anyway,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH v3 06/20] target/riscv: tracking indirect branches (fcfi) for zicfilp

[Deepak Gupta]

zicfilp protects forward control flow (if enabled) by enforcing all
indirect call and jmp must land on a landing pad instruction `lpad`. If
target of an indirect call or jmp is not `lpad` then cpu/hart must raise
a sw check exception with tval = 2.

This patch implements the mechanism using TCG. Target architecture branch
instruction must define the end of a TB. Using this property, during
translation of branch instruction, TB flag = FCFI_LP_EXPECTED can be set.
Translation of target TB can check if FCFI_LP_EXPECTED flag is set and a
flag (fcfi_lp_expected) can be set in DisasContext. If `lpad` gets
translated, fcfi_lp_expected flag in DisasContext can be cleared. Else
it'll fault.

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.h        |  3 +++
 target/riscv/cpu_bits.h   |  7 ++++++
 target/riscv/cpu_helper.c | 13 +++++++++++
 target/riscv/helper.h     |  3 +++
 target/riscv/op_helper.c  |  7 ++++++
 target/riscv/translate.c  | 45 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 78 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 12334f9540..b77481428f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -606,6 +606,9 @@ FIELD(TB_FLAGS, ITRIGGER, 22, 1)
 FIELD(TB_FLAGS, VIRT_ENABLED, 23, 1)
 FIELD(TB_FLAGS, PRIV, 24, 2)
 FIELD(TB_FLAGS, AXL, 26, 2)
+/* zicfilp needs a TB flag to track indirect branches */
+FIELD(TB_FLAGS, FCFI_ENABLED, 28, 1)
+FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 29, 1)
 
 #ifdef TARGET_RISCV32
 #define riscv_cpu_mxl(env)  ((void)(env), MXL_RV32)
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 127f2179dc..1709564b32 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -590,6 +590,10 @@ typedef enum {
     LP_EXPECTED = 1,
 } cfi_elp;
 
+typedef enum {
+    MISSING_LPAD = 0,
+} cfi_violation_cause;
+
 /* hstatus CSR bits */
 #define HSTATUS_VSBE         0x00000020
 #define HSTATUS_GVA          0x00000040
@@ -691,6 +695,9 @@ typedef enum RISCVException {
     RISCV_EXCP_SEMIHOST = 0x3f,
 } RISCVException;
 
+/* zicfilp defines lp violation results in sw check with tval = 2*/
+#define RISCV_EXCP_SW_CHECK_FCFI_TVAL      2
+
 #define RISCV_EXCP_INT_FLAG                0x80000000
 #define RISCV_EXCP_INT_MASK                0x7fffffff
 
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 364f3ee212..c7af430f38 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -134,6 +134,19 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
         flags = FIELD_DP32(flags, TB_FLAGS, VILL, 1);
     }
 
+    if (cpu_get_fcfien(env)) {
+        /*
+         * For Forward CFI, only the expectation of a lpcll at
+         * the start of the block is tracked (which can only happen
+         * when FCFI is enabled for the current processor mode). A jump
+         * or call at the end of the previous TB will have updated
+         * env->elp to indicate the expectation.
+         */
+        flags = FIELD_DP32(flags, TB_FLAGS, FCFI_LP_EXPECTED,
+                           env->elp != NO_LP_EXPECTED);
+        flags = FIELD_DP32(flags, TB_FLAGS, FCFI_ENABLED, 1);
+    }
+
 #ifdef CONFIG_USER_ONLY
     fs = EXT_STATUS_DIRTY;
     vs = EXT_STATUS_DIRTY;
diff --git a/target/riscv/helper.h b/target/riscv/helper.h
index 451261ce5a..fc4c41db5e 100644
--- a/target/riscv/helper.h
+++ b/target/riscv/helper.h
@@ -121,6 +121,9 @@ DEF_HELPER_2(cbo_clean_flush, void, env, tl)
 DEF_HELPER_2(cbo_inval, void, env, tl)
 DEF_HELPER_2(cbo_zero, void, env, tl)
 
+/* helper for raising sw check exception */
+DEF_HELPER_4(raise_sw_check_excep, void, env, tl, tl, tl)
+
 /* Special functions */
 DEF_HELPER_2(csrr, tl, env, int)
 DEF_HELPER_3(csrw, void, env, int, tl)
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 488116cc2e..3b47fb34ea 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -259,6 +259,13 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address)
     /* We don't emulate the cache-hierarchy, so we're done. */
 }
 
+void helper_raise_sw_check_excep(CPURISCVState *env, target_ulong swcheck_code,
+                                 target_ulong arg1, target_ulong arg2)
+{
+    env->sw_check_code = swcheck_code;
+    riscv_raise_exception(env, RISCV_EXCP_SW_CHECK, GETPC());
+}
+
 #ifndef CONFIG_USER_ONLY
 
 target_ulong helper_sret(CPURISCVState *env)
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index acba90f170..fbca3b8a06 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -44,6 +44,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 +117,9 @@ typedef struct DisasContext {
     bool frm_valid;
     bool insn_start_updated;
     const GPtrArray *decoders;
+    /* zicfilp extension. fcfi_enabled, lp expected or not */
+    bool fcfi_enabled;
+    bool fcfi_lp_expected;
 } DisasContext;
 
 static inline bool has_ext(DisasContext *ctx, uint32_t ext)
@@ -1238,6 +1242,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 = FIELD_EX32(tb_flags, TB_FLAGS, FCFI_ENABLED);
     ctx->zero = tcg_constant_tl(0);
     ctx->virt_inst_excp = false;
     ctx->decoders = cpu->decoders;
@@ -1245,6 +1251,37 @@ 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);
+        gen_helper_raise_sw_check_excep(tcg_env,
+            tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
+            tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(0));
+        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;
+    }
 }
 
 static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
@@ -1303,6 +1340,14 @@ static void riscv_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
     default:
         g_assert_not_reached();
     }
+
+    if (ctx->fcfi_lp_expected) {
+        /*
+         * If the "lp expected" flag is still up, the block needs to take an
+         * illegal instruction exception.
+         */
+        tcg_set_insn_param(cfi_lp_check, 1, tcgv_i32_arg(tcg_constant_i32(1)));
+    }
 }
 
 static const TranslatorOps riscv_tr_ops = {
-- 
2.44.0

Re: [PATCH v3 06/20] target/riscv: tracking indirect branches (fcfi) for zicfilp

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 364f3ee212..c7af430f38 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -134,6 +134,19 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
>           flags = FIELD_DP32(flags, TB_FLAGS, VILL, 1);
>       }
>   
> +    if (cpu_get_fcfien(env)) {
> +        /*
> +         * For Forward CFI, only the expectation of a lpcll at
> +         * the start of the block is tracked (which can only happen
> +         * when FCFI is enabled for the current processor mode). A jump
> +         * or call at the end of the previous TB will have updated
> +         * env->elp to indicate the expectation.
> +         */
> +        flags = FIELD_DP32(flags, TB_FLAGS, FCFI_LP_EXPECTED,
> +                           env->elp != NO_LP_EXPECTED);

A good example why it's better to store this as bool in the first place.

>   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);
> +        gen_helper_raise_sw_check_excep(tcg_env,
> +            tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
> +            tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(0));
> +        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;
> +    }
>   }

Again, don't do this here.
There is a reason why only DISAS_NEXT is legal: plugins.
You *must* do this in riscv_tr_translate_insn, like ARM.


r~

Re: [PATCH v3 06/20] target/riscv: tracking indirect branches (fcfi) for zicfilp

[Deepak Gupta]

On Wed, Aug 07, 2024 at 11:23:00AM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>index 364f3ee212..c7af430f38 100644
>>--- a/target/riscv/cpu_helper.c
>>+++ b/target/riscv/cpu_helper.c
>>@@ -134,6 +134,19 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
>>          flags = FIELD_DP32(flags, TB_FLAGS, VILL, 1);
>>      }
>>+    if (cpu_get_fcfien(env)) {
>>+        /*
>>+         * For Forward CFI, only the expectation of a lpcll at
>>+         * the start of the block is tracked (which can only happen
>>+         * when FCFI is enabled for the current processor mode). A jump
>>+         * or call at the end of the previous TB will have updated
>>+         * env->elp to indicate the expectation.
>>+         */
>>+        flags = FIELD_DP32(flags, TB_FLAGS, FCFI_LP_EXPECTED,
>>+                           env->elp != NO_LP_EXPECTED);
>
>A good example why it's better to store this as bool in the first place.
>
>>  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);
>>+        gen_helper_raise_sw_check_excep(tcg_env,
>>+            tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
>>+            tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(0));
>>+        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;
>>+    }
>>  }
>
>Again, don't do this here.
>There is a reason why only DISAS_NEXT is legal: plugins.
>You *must* do this in riscv_tr_translate_insn, like ARM.

Sorry missed this. I remember you gave same feedack in last version.

>
>
>r~

[PATCH v3 07/20] target/riscv: zicfilp `lpad` impl and branch tracking

[Deepak Gupta]

Implements setting lp expected when `jalr` is encountered and implements
`lpad` instruction of zicfilp. `lpad` instruction is taken out of
auipc x0, <imm_20>. This is an existing HINTNOP space. If `lpad` is
target of an indirect branch, cpu checks for 20 bit value in x7 upper
with 20 bit value embedded in `lpad`. If they don't match, cpu raises a
sw check exception with tval = 2.

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/cpu_user.h                 |  1 +
 target/riscv/insn32.decode              |  6 ++-
 target/riscv/insn_trans/trans_rvi.c.inc | 66 +++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 1709564b32..2c585a63c2 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -592,6 +592,8 @@ typedef enum {
 
 typedef enum {
     MISSING_LPAD = 0,
+    MISALIGNED_LPAD = 1,
+    LABEL_MISMATCH_LPAD = 2,
 } cfi_violation_cause;
 
 /* hstatus CSR bits */
diff --git a/target/riscv/cpu_user.h b/target/riscv/cpu_user.h
index 02afad608b..e6927ff847 100644
--- a/target/riscv/cpu_user.h
+++ b/target/riscv/cpu_user.h
@@ -15,5 +15,6 @@
 #define xA6 16
 #define xA7 17  /* syscall number for RVI ABI */
 #define xT0 5   /* syscall number for RVE ABI */
+#define xT2 7
 
 #endif
diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
index c45b8fa1d8..c963c59c8e 100644
--- a/target/riscv/insn32.decode
+++ b/target/riscv/insn32.decode
@@ -40,6 +40,7 @@
 %imm_z6   26:1 15:5
 %imm_mop5 30:1 26:2 20:2
 %imm_mop3 30:1 26:2
+%imm_cfi20 12:20
 
 # Argument sets:
 &empty
@@ -123,7 +124,10 @@ sfence_vm   0001000    00100 ..... 000 00000 1110011 @sfence_vm
 
 # *** RV32I Base Instruction Set ***
 lui      ....................       ..... 0110111 @u
-auipc    ....................       ..... 0010111 @u
+{
+  lpad     ....................       00000 0010111 %imm_cfi20
+  auipc    ....................       ..... 0010111 @u
+}
 jal      ....................       ..... 1101111 @j
 jalr     ............     ..... 000 ..... 1100111 @i
 beq      ....... .....    ..... 000 ..... 1100011 @b
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index 98e3806d5e..cbd7d5c395 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -36,6 +36,58 @@ static bool trans_lui(DisasContext *ctx, arg_lui *a)
     return true;
 }
 
+static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
+{
+    bool lp_expected;
+    /* zicfilp only supported on 32bit and 64bit */
+    if (get_xl(ctx) != MXL_RV32 && get_xl(ctx) != MXL_RV64) {
+        return false;
+    }
+
+    lp_expected = ctx->fcfi_lp_expected;
+    /* forward cfi not enabled or lp not expected, return false */
+    if (!ctx->fcfi_enabled) {
+        return false;
+    }
+
+    /*
+     * If this is the first instruction of the TB, let the translator
+     * know the landing pad requirement was satisfied. No need to bother
+     * checking for CFI feature or enablement.
+     */
+
+    if (ctx->base.pc_next == ctx->base.pc_first) {
+        ctx->fcfi_lp_expected = false;
+        /* If landing pad was expected, PC must be 4 byte aligned */
+        if (lp_expected && ((ctx->base.pc_next) & 0x3)) {
+            /*
+             * misaligned, according to spec we should raise sw check exception
+             */
+            gen_helper_raise_sw_check_excep(tcg_env,
+                tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
+                tcg_constant_tl(MISALIGNED_LPAD), tcg_constant_tl(0));
+            return true;
+        }
+    }
+
+    /* if lp was expected, do label check */
+    if (lp_expected) {
+        TCGLabel *skip = gen_new_label();
+        TCGv tmp = tcg_temp_new();
+        tcg_gen_st_tl(tcg_constant_tl(NO_LP_EXPECTED),
+                      tcg_env, offsetof(CPURISCVState, elp));
+        tcg_gen_extract_tl(tmp, get_gpr(ctx, xT2, EXT_NONE), 12, 20);
+        tcg_gen_brcondi_tl(TCG_COND_EQ, tcg_constant_tl(a->imm_cfi20), 0, skip);
+        tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, a->imm_cfi20, skip);
+        gen_helper_raise_sw_check_excep(tcg_env,
+            tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
+            tcg_constant_tl(LABEL_MISMATCH_LPAD), tcg_constant_tl(0));
+        gen_set_label(skip);
+    }
+
+    return true;
+}
+
 static bool trans_auipc(DisasContext *ctx, arg_auipc *a)
 {
     TCGv target_pc = dest_gpr(ctx, a->rd);
@@ -75,6 +127,20 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
     gen_set_gpr(ctx, a->rd, succ_pc);
 
     tcg_gen_mov_tl(cpu_pc, target_pc);
+    if (ctx->cfg_ptr->ext_zicfilp && ctx->fcfi_enabled) {
+        /*
+         * Rely on a helper to check the forward CFI enable for the
+         * current process mode. The alternatives would be (1) include
+         * "fcfi enabled" in the cflags or (2) maintain a "fcfi
+         * currently enabled" in tcg_env and emit TCG code to access
+         * and test it.
+         */
+        if (a->rs1 != xRA && a->rs1 != xT0 && a->rs1 != xT2) {
+            tcg_gen_st_tl(tcg_constant_tl(LP_EXPECTED),
+                          tcg_env, offsetof(CPURISCVState, elp));
+        }
+    }
+
     lookup_and_goto_ptr(ctx);
 
     if (misaligned) {
-- 
2.44.0

Re: [PATCH v3 07/20] target/riscv: zicfilp `lpad` impl and branch tracking

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> diff --git a/target/riscv/cpu_user.h b/target/riscv/cpu_user.h
> index 02afad608b..e6927ff847 100644
> --- a/target/riscv/cpu_user.h
> +++ b/target/riscv/cpu_user.h
> @@ -15,5 +15,6 @@
>   #define xA6 16
>   #define xA7 17  /* syscall number for RVI ABI */
>   #define xT0 5   /* syscall number for RVE ABI */
> +#define xT2 7

Maybe just add them all?

> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
> index 98e3806d5e..cbd7d5c395 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -36,6 +36,58 @@ static bool trans_lui(DisasContext *ctx, arg_lui *a)
>       return true;
>   }
>   
> +static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
> +{
> +    bool lp_expected;
> +    /* zicfilp only supported on 32bit and 64bit */
> +    if (get_xl(ctx) != MXL_RV32 && get_xl(ctx) != MXL_RV64) {
> +        return false;
> +    }

Where does it say that?
There doesn't seem to be anything in the spec that excludes rv128.

> +    lp_expected = ctx->fcfi_lp_expected;
> +    /* forward cfi not enabled or lp not expected, return false */
> +    if (!ctx->fcfi_enabled) {
> +        return false;
> +    }

Comments should explain why, not exactly mirror the code.
In any case, better as:

   /*
    * If zicfilp not present, the encoding is a nop.
    * If forward cfi not enabled, the implementation is a nop.
    */
   if (!ctx->fcfi_enabled) {
       return true;
   }

No need to fall through into AUIPC to re-discover nop-ness.

> +
> +    /*
> +     * If this is the first instruction of the TB, let the translator
> +     * know the landing pad requirement was satisfied. No need to bother
> +     * checking for CFI feature or enablement.
> +     */

Comment is strange, because you have just checked enablement.

> +    /* if lp was expected, do label check */
> +    if (lp_expected) {
> +        TCGLabel *skip = gen_new_label();
> +        TCGv tmp = tcg_temp_new();
> +        tcg_gen_st_tl(tcg_constant_tl(NO_LP_EXPECTED),
> +                      tcg_env, offsetof(CPURISCVState, elp));

This placement is wrong, according to the LPAD pseudocode.
It must go last.

> +        tcg_gen_extract_tl(tmp, get_gpr(ctx, xT2, EXT_NONE), 12, 20);
> +        tcg_gen_brcondi_tl(TCG_COND_EQ, tcg_constant_tl(a->imm_cfi20), 0, skip);

This one you check at translation time:

   if (a->imm_cfi20 != 0)


> +        tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, a->imm_cfi20, skip);
> +        gen_helper_raise_sw_check_excep(tcg_env,
> +            tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
> +            tcg_constant_tl(LABEL_MISMATCH_LPAD), tcg_constant_tl(0));
> +        gen_set_label(skip);
> +    }
> +
> +    return true;
> +}
> +
>   static bool trans_auipc(DisasContext *ctx, arg_auipc *a)
>   {
>       TCGv target_pc = dest_gpr(ctx, a->rd);
> @@ -75,6 +127,20 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
>       gen_set_gpr(ctx, a->rd, succ_pc);
>   
>       tcg_gen_mov_tl(cpu_pc, target_pc);
> +    if (ctx->cfg_ptr->ext_zicfilp && ctx->fcfi_enabled) {

No need to check ext_zicfilp, as fcfi_enabled includes that already.

> +        /*
> +         * Rely on a helper to check the forward CFI enable for the
> +         * current process mode. The alternatives would be (1) include
> +         * "fcfi enabled" in the cflags or (2) maintain a "fcfi
> +         * currently enabled" in tcg_env and emit TCG code to access
> +         * and test it.
> +         */

Comment is out of date.

> +        if (a->rs1 != xRA && a->rs1 != xT0 && a->rs1 != xT2) {
> +            tcg_gen_st_tl(tcg_constant_tl(LP_EXPECTED),
> +                          tcg_env, offsetof(CPURISCVState, elp));
> +        }
> +    }
> +
>       lookup_and_goto_ptr(ctx);
>   
>       if (misaligned) {


r~

Re: [PATCH v3 07/20] target/riscv: zicfilp `lpad` impl and branch tracking

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> +++ b/target/riscv/insn32.decode
> @@ -40,6 +40,7 @@
>   %imm_z6   26:1 15:5
>   %imm_mop5 30:1 26:2 20:2
>   %imm_mop3 30:1 26:2
> +%imm_cfi20 12:20
>   
>   # Argument sets:
>   &empty
> @@ -123,7 +124,10 @@ sfence_vm   0001000    00100 ..... 000 00000 1110011 @sfence_vm
>   
>   # *** RV32I Base Instruction Set ***
>   lui      ....................       ..... 0110111 @u
> -auipc    ....................       ..... 0010111 @u
> +{
> +  lpad     ....................       00000 0010111 %imm_cfi20

IMO, better as

   lpad  imm:20 00000 0010111

without the %imm_cfi20 indirection.


r~

[PATCH v3 08/20] disas/riscv: enabled `lpad` disassembly

[Deepak Gupta]

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>
---
 disas/riscv.c | 18 +++++++++++++++++-
 disas/riscv.h |  2 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/disas/riscv.c b/disas/riscv.c
index c8364c2b07..c7c92acef7 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -974,6 +974,7 @@ typedef enum {
     rv_op_amomaxu_h = 943,
     rv_op_amocas_b  = 944,
     rv_op_amocas_h  = 945,
+    rv_op_lpad = 946,
 } rv_op;
 
 /* register names */
@@ -2232,6 +2233,7 @@ const rv_opcode_data rvi_opcode_data[] = {
     { "amomaxu.h", rv_codec_r_a, rv_fmt_aqrl_rd_rs2_rs1, NULL, 0, 0, 0 },
     { "amocas.b", rv_codec_r_a, rv_fmt_aqrl_rd_rs2_rs1, NULL, 0, 0, 0 },
     { "amocas.h", rv_codec_r_a, rv_fmt_aqrl_rd_rs2_rs1, NULL, 0, 0, 0 },
+    { "lpad", rv_codec_lp, rv_fmt_imm, NULL, 0, 0, 0 },
 };
 
 /* CSR names */
@@ -2925,7 +2927,13 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa)
             case 7: op = rv_op_andi; break;
             }
             break;
-        case 5: op = rv_op_auipc; break;
+        case 5:
+            op = rv_op_auipc;
+            if (dec->cfg->ext_zicfilp &&
+                (((inst >> 7) & 0b11111) == 0b00000)) {
+                op = rv_op_lpad;
+            }
+            break;
         case 6:
             switch ((inst >> 12) & 0b111) {
             case 0: op = rv_op_addiw; break;
@@ -4482,6 +4490,11 @@ static uint32_t operand_tbl_index(rv_inst inst)
     return ((inst << 54) >> 56);
 }
 
+static uint32_t operand_lpl(rv_inst inst)
+{
+    return inst >> 12;
+}
+
 /* decode operands */
 
 static void decode_inst_operands(rv_decode *dec, rv_isa isa)
@@ -4869,6 +4882,9 @@ static void decode_inst_operands(rv_decode *dec, rv_isa isa)
         dec->imm = sextract32(operand_rs2(inst), 0, 5);
         dec->imm1 = operand_imm2(inst);
         break;
+    case rv_codec_lp:
+        dec->imm = operand_lpl(inst);
+        break;
     };
 }
 
diff --git a/disas/riscv.h b/disas/riscv.h
index 16a08e4895..1182457aff 100644
--- a/disas/riscv.h
+++ b/disas/riscv.h
@@ -166,6 +166,7 @@ typedef enum {
     rv_codec_r2_immhl,
     rv_codec_r2_imm2_imm5,
     rv_codec_fli,
+    rv_codec_lp,
 } rv_codec;
 
 /* structures */
@@ -228,6 +229,7 @@ enum {
 #define rv_fmt_rs1_rs2                "O\t1,2"
 #define rv_fmt_rd_imm                 "O\t0,i"
 #define rv_fmt_rd_uimm                "O\t0,Ui"
+#define rv_fmt_imm                    "O\ti"
 #define rv_fmt_rd_offset              "O\t0,o"
 #define rv_fmt_rd_uoffset             "O\t0,Uo"
 #define rv_fmt_rd_rs1_rs2             "O\t0,1,2"
-- 
2.44.0

Re: [PATCH v3 08/20] disas/riscv: enabled `lpad` disassembly

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> 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>
> ---
>   disas/riscv.c | 18 +++++++++++++++++-
>   disas/riscv.h |  2 ++
>   2 files changed, 19 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v3 09/20] target/riscv: Add zicfiss extension

[Deepak Gupta]

zicfiss [1] riscv cpu extension enables backward control flow integrity.

This patch sets up space for zicfiss extension in cpuconfig. And imple-
ments dependency on zicsr, zimop and zcmop extensions.

[1] - https://github.com/riscv/riscv-cfi

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.c         |  2 ++
 target/riscv/cpu_cfg.h     |  1 +
 target/riscv/tcg/tcg-cpu.c | 15 +++++++++++++++
 3 files changed, 18 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index e1526c7ab5..54fcf380ff 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -107,6 +107,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, has_priv_1_11),
     ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, has_priv_1_11),
     ISA_EXT_DATA_ENTRY(zicfilp, PRIV_VERSION_1_12_0, ext_zicfilp),
+    ISA_EXT_DATA_ENTRY(zicfiss, PRIV_VERSION_1_13_0, ext_zicfiss),
     ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
     ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
     ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
@@ -1482,6 +1483,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     MULTI_EXT_CFG_BOOL("sscofpmf", ext_sscofpmf, false),
     MULTI_EXT_CFG_BOOL("zifencei", ext_zifencei, true),
     MULTI_EXT_CFG_BOOL("zicfilp", ext_zicfilp, false),
+    MULTI_EXT_CFG_BOOL("zicfiss", ext_zicfiss, false),
     MULTI_EXT_CFG_BOOL("zicsr", ext_zicsr, true),
     MULTI_EXT_CFG_BOOL("zihintntl", ext_zihintntl, true),
     MULTI_EXT_CFG_BOOL("zihintpause", ext_zihintpause, true),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 88d5defbb5..2499f38407 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -68,6 +68,7 @@ struct RISCVCPUConfig {
     bool ext_zicbop;
     bool ext_zicboz;
     bool ext_zicfilp;
+    bool ext_zicfiss;
     bool ext_zicond;
     bool ext_zihintntl;
     bool ext_zihintpause;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index ed19586c9d..4fd2fd7a28 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -618,6 +618,21 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.ext_zihpm = false;
     }
 
+    if (cpu->cfg.ext_zicfiss) {
+        if (!cpu->cfg.ext_zicsr) {
+            error_setg(errp, "zicfiss extension requires zicsr extension");
+            return;
+        }
+        if (!cpu->cfg.ext_zimop) {
+            error_setg(errp, "zicfiss extension requires zimop extension");
+            return;
+        }
+        if (cpu->cfg.ext_zca && !cpu->cfg.ext_zcmop) {
+            error_setg(errp, "zicfiss with zca requires zcmop extension");
+            return;
+        }
+    }
+
     if (!cpu->cfg.ext_zihpm) {
         cpu->cfg.pmu_mask = 0;
         cpu->pmu_avail_ctrs = 0;
-- 
2.44.0

[PATCH v3 10/20] target/riscv: introduce ssp and enabling controls for zicfiss

[Deepak Gupta]

zicfiss introduces a new state ssp ("shadow stack register") in cpu.
ssp is expressed as a new unprivileged csr (CSR_SSP=0x11) and holds
virtual address for shadow stack as programmed by software.

Shadow stack (for each mode) is enabled via bit3 in *envcfg CSRs.
Shadow stack can be enabled for a mode only if it's higher privileged
mode had it enabled for itself. M mode doesn't need enabling control,
it's always available if extension is available on cpu.

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.c      |  3 ++
 target/riscv/cpu.h      |  2 ++
 target/riscv/cpu_bits.h |  6 ++++
 target/riscv/csr.c      | 74 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 85 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 54fcf380ff..6b50ae0e45 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -998,6 +998,9 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
     /* on reset elp is set to NO_LP_EXPECTED */
     env->elp = NO_LP_EXPECTED;
 
+    /* on reset ssp is set to 0 */
+    env->ssp = 0;
+
     /*
      * Bits 10, 6, 2 and 12 of mideleg are read only 1 when the Hypervisor
      * extension is enabled.
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index b77481428f..53b005b34c 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -224,6 +224,8 @@ struct CPUArchState {
 
     /* elp state for zicfilp extension */
     cfi_elp      elp;
+    /* shadow stack register for zicfiss extension */
+    target_ulong ssp;
     /* sw check code for sw check exception */
     target_ulong sw_check_code;
 #ifdef CONFIG_USER_ONLY
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 2c585a63c2..226157896d 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -34,6 +34,9 @@
 
 /* Control and Status Registers */
 
+/* zicfiss user ssp csr */
+#define CSR_SSP             0x011
+
 /* User Trap Setup */
 #define CSR_USTATUS         0x000
 #define CSR_UIE             0x004
@@ -766,6 +769,7 @@ typedef enum RISCVException {
 /* Execution environment configuration bits */
 #define MENVCFG_FIOM                       BIT(0)
 #define MENVCFG_LPE                        BIT(2) /* zicfilp */
+#define MENVCFG_SSE                        BIT(3) /* zicfiss */
 #define MENVCFG_CBIE                       (3UL << 4)
 #define MENVCFG_CBCFE                      BIT(6)
 #define MENVCFG_CBZE                       BIT(7)
@@ -780,12 +784,14 @@ typedef enum RISCVException {
 
 #define SENVCFG_FIOM                       MENVCFG_FIOM
 #define SENVCFG_LPE                        MENVCFG_LPE
+#define SENVCFG_SSE                        MENVCFG_SSE
 #define SENVCFG_CBIE                       MENVCFG_CBIE
 #define SENVCFG_CBCFE                      MENVCFG_CBCFE
 #define SENVCFG_CBZE                       MENVCFG_CBZE
 
 #define HENVCFG_FIOM                       MENVCFG_FIOM
 #define HENVCFG_LPE                        MENVCFG_LPE
+#define HENVCFG_SSE                        MENVCFG_SSE
 #define HENVCFG_CBIE                       MENVCFG_CBIE
 #define HENVCFG_CBCFE                      MENVCFG_CBCFE
 #define HENVCFG_CBZE                       MENVCFG_CBZE
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index a5a969a377..d72d6289fb 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -185,6 +185,47 @@ static RISCVException zcmt(CPURISCVState *env, int csrno)
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException cfi_ss(CPURISCVState *env, int csrno)
+{
+    /* no cfi extension, access to csr is illegal */
+    if (!env_archcpu(env)->cfg.ext_zicfiss) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+    /*
+     * CONFIG_USER_MODE always allow access for now. Better for user mode only
+     * functionality
+     */
+#if !defined(CONFIG_USER_ONLY)
+    if (env->debugger) {
+        return RISCV_EXCP_NONE;
+    }
+    /* current priv not M */
+    if (env->priv != PRV_M) {
+        /* menvcfg says no shadow stack enable */
+        if (!get_field(env->menvcfg, MENVCFG_SSE)) {
+            return RISCV_EXCP_ILLEGAL_INST;
+        }
+
+        /* V = 1 and henvcfg says no shadow stack enable */
+        if (env->virt_enabled &&
+            !get_field(env->henvcfg, HENVCFG_SSE)) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
+
+        /*
+         * SSP are not accessible to U mode if disabled via senvcfg
+         * CSR
+         */
+        if ((env->priv == PRV_U) &&
+            (!get_field(env->senvcfg, SENVCFG_SSE))) {
+            return RISCV_EXCP_ILLEGAL_INST;
+        }
+    }
+#endif
+
+    return RISCV_EXCP_NONE;
+}
+
 #if !defined(CONFIG_USER_ONLY)
 static RISCVException mctr(CPURISCVState *env, int csrno)
 {
@@ -596,6 +637,19 @@ static RISCVException seed(CPURISCVState *env, int csrno)
 #endif
 }
 
+/* zicfiss CSR_SSP read and write */
+static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val)
+{
+    *val = env->ssp;
+    return RISCV_EXCP_NONE;
+}
+
+static int write_ssp(CPURISCVState *env, int csrno, target_ulong val)
+{
+    env->ssp = val;
+    return RISCV_EXCP_NONE;
+}
+
 /* User Floating-Point CSRs */
 static RISCVException read_fflags(CPURISCVState *env, int csrno,
                                   target_ulong *val)
@@ -2111,6 +2165,10 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
         if (env_archcpu(env)->cfg.ext_zicfilp) {
             mask |= MENVCFG_LPE;
         }
+
+        if (env_archcpu(env)->cfg.ext_zicfiss) {
+            mask |= MENVCFG_SSE;
+        }
     }
     env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
 
@@ -2167,6 +2225,13 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
         mask |= SENVCFG_LPE;
     }
 
+    /* Higher mode SSE must be ON for next-less mode SSE to be ON */
+    if (env_archcpu(env)->cfg.ext_zicfiss &&
+        get_field(env->menvcfg, MENVCFG_SSE) &&
+        (env->virt_enabled ? get_field(env->henvcfg, HENVCFG_SSE) : true)) {
+        mask |= SENVCFG_SSE;
+    }
+
     env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
     return RISCV_EXCP_NONE;
 }
@@ -2208,6 +2273,12 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
         if (env_archcpu(env)->cfg.ext_zicfilp) {
             mask |= HENVCFG_LPE;
         }
+
+        /* H can light up SSE for VS only if HS had it from menvcfg */
+        if (env_archcpu(env)->cfg.ext_zicfiss &&
+            get_field(env->menvcfg, MENVCFG_SSE)) {
+            mask |= HENVCFG_SSE;
+        }
     }
 
     env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
@@ -4663,6 +4734,9 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     /* Zcmt Extension */
     [CSR_JVT] = {"jvt", zcmt, read_jvt, write_jvt},
 
+    /* zicfiss Extension, shadow stack register */
+    [CSR_SSP]  = { "ssp", cfi_ss, read_ssp, write_ssp },
+
 #if !defined(CONFIG_USER_ONLY)
     /* Machine Timers and Counters */
     [CSR_MCYCLE]    = { "mcycle",    any,   read_hpmcounter,
-- 
2.44.0

Re: [PATCH v3 10/20] target/riscv: introduce ssp and enabling controls for zicfiss

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index a5a969a377..d72d6289fb 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -185,6 +185,47 @@ static RISCVException zcmt(CPURISCVState *env, int csrno)
>       return RISCV_EXCP_NONE;
>   }
>   
> +static RISCVException cfi_ss(CPURISCVState *env, int csrno)
> +{
> +    /* no cfi extension, access to csr is illegal */
> +    if (!env_archcpu(env)->cfg.ext_zicfiss) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +    /*
> +     * CONFIG_USER_MODE always allow access for now. Better for user mode only
> +     * functionality
> +     */

In the next patch you add ubcfien, which would apply here.


r~

Re: [PATCH v3 10/20] target/riscv: introduce ssp and enabling controls for zicfiss

[Richard Henderson]

On 8/7/24 12:11, Richard Henderson wrote:
> On 8/7/24 10:06, Deepak Gupta wrote:
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index a5a969a377..d72d6289fb 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -185,6 +185,47 @@ static RISCVException zcmt(CPURISCVState *env, int csrno)
>>       return RISCV_EXCP_NONE;
>>   }
>> +static RISCVException cfi_ss(CPURISCVState *env, int csrno)
>> +{
>> +    /* no cfi extension, access to csr is illegal */
>> +    if (!env_archcpu(env)->cfg.ext_zicfiss) {
>> +        return RISCV_EXCP_ILLEGAL_INST;
>> +    }
>> +    /*
>> +     * CONFIG_USER_MODE always allow access for now. Better for user mode only
>> +     * functionality
>> +     */
> 
> In the next patch you add ubcfien, which would apply here.

... anyway, surely cpu_get_bcfien() is the right check anyway?


r~

Re: [PATCH v3 10/20] target/riscv: introduce ssp and enabling controls for zicfiss

[Deepak Gupta]

On Wed, Aug 07, 2024 at 12:12:52PM +1000, Richard Henderson wrote:
>On 8/7/24 12:11, Richard Henderson wrote:
>>On 8/7/24 10:06, Deepak Gupta wrote:
>>>diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>index a5a969a377..d72d6289fb 100644
>>>--- a/target/riscv/csr.c
>>>+++ b/target/riscv/csr.c
>>>@@ -185,6 +185,47 @@ static RISCVException zcmt(CPURISCVState *env, int csrno)
>>>      return RISCV_EXCP_NONE;
>>>  }
>>>+static RISCVException cfi_ss(CPURISCVState *env, int csrno)
>>>+{
>>>+    /* no cfi extension, access to csr is illegal */
>>>+    if (!env_archcpu(env)->cfg.ext_zicfiss) {
>>>+        return RISCV_EXCP_ILLEGAL_INST;
>>>+    }
>>>+    /*
>>>+     * CONFIG_USER_MODE always allow access for now. Better for user mode only
>>>+     * functionality
>>>+     */
>>
>>In the next patch you add ubcfien, which would apply here.
>
>... anyway, surely cpu_get_bcfien() is the right check anyway?

Yeah you're right, `cpu_get_bcfien()` works and simplify it. will fix it.
>
>
>r~

[PATCH v3 11/20] target/riscv: tb flag for shadow stack instructions

[Deepak Gupta]

Shadow stack instructions can be decoded as zimop / zcmop or shadow stack
instructions depending on whether shadow stack are enabled at current
privilege. This requires a TB flag so that correct TB generation and correct
TB lookup happens. `DisasContext` gets a field indicating whether bcfi is
enabled or not.

This patch also implements helper bcfi function which determines if bcfi
is enabled at current privilege or not. qemu-user also gets field
`ubcfien` indicating whether qemu user has shadow stack enabled or not.

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.c        |  2 ++
 target/riscv/cpu.h        |  4 ++++
 target/riscv/cpu_helper.c | 30 ++++++++++++++++++++++++++++++
 target/riscv/translate.c  |  4 ++++
 4 files changed, 40 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 6b50ae0e45..e1ff246c24 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1029,6 +1029,8 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
 #ifdef CONFIG_USER_ONLY
     /* qemu-user for riscv, fcfi is off by default */
     env->ufcfien = false;
+    /* qemu-user for riscv, bcfi is off by default */
+    env->ubcfien = false;
 #endif
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 53b005b34c..6da94c417c 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -231,6 +231,7 @@ struct CPUArchState {
 #ifdef CONFIG_USER_ONLY
     uint32_t elf_flags;
     bool ufcfien;
+    bool ubcfien;
 #endif
 
 #ifndef CONFIG_USER_ONLY
@@ -536,6 +537,7 @@ bool riscv_cpu_vector_enabled(CPURISCVState *env);
 void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
 int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
 bool cpu_get_fcfien(CPURISCVState *env);
+bool cpu_get_bcfien(CPURISCVState *env);
 G_NORETURN void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
                                                MMUAccessType access_type,
                                                int mmu_idx, uintptr_t retaddr);
@@ -611,6 +613,8 @@ FIELD(TB_FLAGS, AXL, 26, 2)
 /* zicfilp needs a TB flag to track indirect branches */
 FIELD(TB_FLAGS, FCFI_ENABLED, 28, 1)
 FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 29, 1)
+/* zicfiss needs a TB flag so that correct TB is located based on tb flags */
+FIELD(TB_FLAGS, BCFI_ENABLED, 30, 1)
 
 #ifdef TARGET_RISCV32
 #define riscv_cpu_mxl(env)  ((void)(env), MXL_RV32)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index c7af430f38..fb6c0d4e1f 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -93,6 +93,32 @@ bool cpu_get_fcfien(CPURISCVState *env)
 #endif
 }
 
+bool cpu_get_bcfien(CPURISCVState *env)
+{
+#ifdef CONFIG_USER_ONLY
+    return env->ubcfien;
+#else
+    /* no cfi extension, return false */
+    if (!env_archcpu(env)->cfg.ext_zicfiss) {
+        return false;
+    }
+
+    switch (env->priv) {
+    case PRV_U:
+        return env->senvcfg & SENVCFG_SSE;
+    case PRV_S:
+        if (env->virt_enabled) {
+            return env->henvcfg & HENVCFG_SSE;
+        }
+        return env->menvcfg & MENVCFG_SSE;
+    case PRV_M: /* M-mode shadow stack is always on if hart implements */
+        return true;
+    default:
+        g_assert_not_reached();
+    }
+#endif
+}
+
 void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
                           uint64_t *cs_base, uint32_t *pflags)
 {
@@ -147,6 +173,10 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
         flags = FIELD_DP32(flags, TB_FLAGS, FCFI_ENABLED, 1);
     }
 
+    if (cpu_get_bcfien(env)) {
+        flags = FIELD_DP32(flags, TB_FLAGS, BCFI_ENABLED, 1);
+    }
+
 #ifdef CONFIG_USER_ONLY
     fs = EXT_STATUS_DIRTY;
     vs = EXT_STATUS_DIRTY;
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index fbca3b8a06..b0526f5d79 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -120,6 +120,8 @@ typedef struct DisasContext {
     /* zicfilp extension. fcfi_enabled, lp expected or not */
     bool fcfi_enabled;
     bool fcfi_lp_expected;
+    /* zicfiss extension, if shadow stack was enabled during TB gen */
+    bool bcfi_enabled;
 } DisasContext;
 
 static inline bool has_ext(DisasContext *ctx, uint32_t ext)
@@ -1242,6 +1244,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->bcfi_enabled = cpu_get_bcfien(env) &&
+                        FIELD_EX32(tb_flags, TB_FLAGS, BCFI_ENABLED);
     ctx->fcfi_lp_expected = FIELD_EX32(tb_flags, TB_FLAGS, FCFI_LP_EXPECTED);
     ctx->fcfi_enabled = FIELD_EX32(tb_flags, TB_FLAGS, FCFI_ENABLED);
     ctx->zero = tcg_constant_tl(0);
-- 
2.44.0

Re: [PATCH v3 11/20] target/riscv: tb flag for shadow stack instructions

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> Shadow stack instructions can be decoded as zimop / zcmop or shadow stack
> instructions depending on whether shadow stack are enabled at current
> privilege. This requires a TB flag so that correct TB generation and correct
> TB lookup happens. `DisasContext` gets a field indicating whether bcfi is
> enabled or not.
> 
> This patch also implements helper bcfi function which determines if bcfi
> is enabled at current privilege or not. qemu-user also gets field
> `ubcfien` indicating whether qemu user has shadow stack enabled or not.
> 
> 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.c        |  2 ++
>   target/riscv/cpu.h        |  4 ++++
>   target/riscv/cpu_helper.c | 30 ++++++++++++++++++++++++++++++
>   target/riscv/translate.c  |  4 ++++
>   4 files changed, 40 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Deepak Gupta]

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
 @u       ....................      ..... ....... &u      imm=%imm_u          %rd
 @j       ....................      ..... ....... &j      imm=%imm_j          %rd
 
@@ -247,6 +249,7 @@ remud    0000001 .....  ..... 111 ..... 1111011 @r
 lr_w       00010 . . 00000 ..... 010 ..... 0101111 @atom_ld
 sc_w       00011 . . ..... ..... 010 ..... 0101111 @atom_st
 amoswap_w  00001 . . ..... ..... 010 ..... 0101111 @atom_st
+ssamoswap_w 01001 . . ..... ..... 010 ..... 0101111 @atom_st
 amoadd_w   00000 . . ..... ..... 010 ..... 0101111 @atom_st
 amoxor_w   00100 . . ..... ..... 010 ..... 0101111 @atom_st
 amoand_w   01100 . . ..... ..... 010 ..... 0101111 @atom_st
@@ -260,6 +263,7 @@ amomaxu_w  11100 . . ..... ..... 010 ..... 0101111 @atom_st
 lr_d       00010 . . 00000 ..... 011 ..... 0101111 @atom_ld
 sc_d       00011 . . ..... ..... 011 ..... 0101111 @atom_st
 amoswap_d  00001 . . ..... ..... 011 ..... 0101111 @atom_st
+ssamoswap_d 01001 . . ..... ..... 011 ..... 0101111 @atom_st
 amoadd_d   00000 . . ..... ..... 011 ..... 0101111 @atom_st
 amoxor_d   00100 . . ..... ..... 011 ..... 0101111 @atom_st
 amoand_d   01100 . . ..... ..... 011 ..... 0101111 @atom_st
@@ -1023,8 +1027,17 @@ amocas_d    00101 . . ..... ..... 011 ..... 0101111 @atom_st
 amocas_q    00101 . . ..... ..... 100 ..... 0101111 @atom_st
 
 # *** Zimop may-be-operation extension ***
-mop_r_n     1 . 00 .. 0111 .. ..... 100 ..... 1110011 @mop5
-mop_rr_n    1 . 00 .. 1 ..... ..... 100 ..... 1110011 @mop3
+{
+  # zicfiss instructions carved out of mop.r
+  ssrdp      1100110 11100     00000 100 ..... 1110011 %rd
+  sspopchk   1100110 11100     ..... 100 00000 1110011 @ss_pop
+  mop_r_n    1 . 00 .. 0111 .. ..... 100 ..... 1110011 @mop5
+}
+{
+  # zicfiss instruction carved out of mop.rr
+  sspush     1100111 .....     00000 100 00000 1110011 @ss_push
+  mop_rr_n   1 . 00 .. 1 ..... ..... 100 ..... 1110011 @mop3
+}
 
 # *** Zabhb Standard Extension ***
 amoswap_b  00001 . . ..... ..... 000 ..... 0101111 @atom_st
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;
+}
+
 static bool trans_amoadd_d(DisasContext *ctx, arg_amoadd_d *a)
 {
     REQUIRE_64BIT(ctx);
diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
new file mode 100644
index 0000000000..c538b7ad99
--- /dev/null
+++ b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
@@ -0,0 +1,149 @@
+/*
+ * RISC-V translation routines for the Control-Flow Integrity Extension
+ *
+ * Copyright (c) 2024 Rivos Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+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();
+    }
+}
+
+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;
+
+    /* sspopchk only supported on 32bit and 64bit */
+    if (get_xl(ctx) != MXL_RV32 && get_xl(ctx) != MXL_RV64) {
+        return false;
+    }
+
+    /* back cfi was not enabled, return false */
+    if (!ctx->bcfi_enabled) {
+        return false;
+    }
+
+    /*
+     * sspopchk can only compare with x1 or x5. Everything else defaults to
+     * zimops
+     */
+
+    if (a->rs1 != 1 && a->rs1 != 5) {
+        return false;
+    }
+
+    /*
+     * 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);
+
+    tcg_gen_qemu_ld_tl(data, addr, ss_mmu_idx,
+                       mxl_memop(ctx) | MO_ALIGN);
+    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);
+
+    return true;
+}
+
+static bool trans_sspush(DisasContext *ctx, arg_sspush *a)
+{
+    /* default for qemu-user, use regular RW memory and thus mmu_idx=0 */
+    int ss_mmu_idx = 0;
+
+    /* sspush only supported on 32bit and 64bit */
+    if (get_xl(ctx) != MXL_RV32 && get_xl(ctx) != MXL_RV64) {
+        return false;
+    }
+
+    /* back cfi was not enabled, return false */
+    if (!ctx->bcfi_enabled) {
+        return false;
+    }
+
+    /*
+     * sspush can only push from x1 or x5. Everything else defaults to zimop
+     */
+    if (a->rs2 != 1 && a->rs2 != 5) {
+        return false;
+    }
+
+    /*
+     * 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();
+    int tmp = (get_xl(ctx) == MXL_RV64) ? -8 : -4;
+    TCGv_i32 ssp_csr = tcg_constant_i32(CSR_SSP);
+    TCGv data = get_gpr(ctx, a->rs2, EXT_NONE);
+    gen_helper_csrr(addr, tcg_env, ssp_csr);
+
+    tcg_gen_addi_tl(addr, addr, tmp);
+
+    tcg_gen_qemu_st_tl(data, addr, ss_mmu_idx,
+                       mxl_memop(ctx) | MO_ALIGN);
+    gen_helper_csrw(tcg_env, ssp_csr, addr);
+
+    return true;
+}
+
+static bool trans_ssrdp(DisasContext *ctx, arg_ssrdp *a)
+{
+    /* ssrdp only supported on 32bit and 64bit */
+    if (get_xl(ctx) != MXL_RV32 && get_xl(ctx) != MXL_RV64) {
+        return false;
+    }
+
+    /* back cfi was not enabled, return false */
+    if (!ctx->bcfi_enabled) {
+        return false;
+    }
+
+    TCGv dest = get_gpr(ctx, a->rd, EXT_NONE);
+    TCGv_i32 ssp_csr = tcg_constant_i32(CSR_SSP);
+    gen_helper_csrr(dest, tcg_env, ssp_csr);
+    gen_set_gpr(ctx, a->rd, dest);
+
+    return true;
+}
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index b0526f5d79..de375c32a1 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1142,6 +1142,7 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
 #include "insn_trans/trans_rvzawrs.c.inc"
 #include "insn_trans/trans_rvzicbo.c.inc"
 #include "insn_trans/trans_rvzimop.c.inc"
+#include "insn_trans/trans_rvzicfiss.c.inc"
 #include "insn_trans/trans_rvzfa.c.inc"
 #include "insn_trans/trans_rvzfh.c.inc"
 #include "insn_trans/trans_rvk.c.inc"
-- 
2.44.0

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~

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

[Richard Henderson]

On 8/7/24 12:39, Richard Henderson wrote:
>> +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.

I'm wrong about m-mode here, but "0" is certainly not right.

I strongly suspect you want "ctx->mem_idx | MMU_IDX_SS_ACCESS",
once you add that bit in a few patches.


r~

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

[Deepak Gupta]

On Wed, Aug 07, 2024 at 12:56:46PM +1000, Richard Henderson wrote:
>On 8/7/24 12:39, Richard Henderson wrote:
>>>+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.
>
>I'm wrong about m-mode here, but "0" is certainly not right.

I followed `riscv_env_mmu_index` here. If CONFIG_USER_ONLY, it returns 0.
For qemu-user, I didn't bother to protect shadow stack from normal stores.
And simply used index 0.

>
>I strongly suspect you want "ctx->mem_idx | MMU_IDX_SS_ACCESS",
>once you add that bit in a few patches.
>
>
>r~

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

[Deepak Gupta]

On Wed, Aug 07, 2024 at 12:39:15PM +1000, Richard Henderson wrote:
>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.
>

Noted.

>>+{
>>+  # 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.
>

Will fix as you suggesting.

>
>>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?

encodings are coming out of existing unused atomics and this is why zicfiss is dependent on 
'A' extension.

Although if shadow stack are not enabled for the execution environment then it should be
illegal instruction.

I am fine placing it in trans_rvzicfiss.c.inc as well. Let me know.
>
>>+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.

Noted.

>
>>+
>>+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.

This is 0 only when qemu-user.
If not qemu-user, its obtained differently.

>
>>+
>>+    /* 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.
>

It's a left over from when I was starting out and didn't know a lot on qemu (still don't :-))
and RISC-V.

Will remove this and at other places as well.

>>+    /*
>>+     * 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));

Yeah that's right, will do that.

>
>>+    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~

[PATCH v3 13/20] target/riscv: compressed encodings for sspush and sspopchk

[Deepak Gupta]

sspush/sspopchk have compressed encodings carved out of zcmops.
compressed sspush is designated as c.mop.1 while compressed sspopchk
is designated as c.mop.5.

Note that c.sspush x1 exists while c.sspush x5 doesn't. Similarly
c.sspopchk x5 exists while c.sspopchk x1 doesn't.

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/insn16.decode | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/riscv/insn16.decode b/target/riscv/insn16.decode
index 3953bcf82d..d9fb74fef6 100644
--- a/target/riscv/insn16.decode
+++ b/target/riscv/insn16.decode
@@ -69,10 +69,12 @@
 # Formats 16:
 @cr        ....  ..... .....  .. &r      rs2=%rs2_5       rs1=%rd     %rd
 @ci        ... . ..... .....  .. &i      imm=%imm_ci      rs1=%rd     %rd
+@c_sspop   ... . ..... .....  .. &i      imm=0      rs1=5     rd=0
 @cl_q      ... . .....  ..... .. &i      imm=%uimm_cl_q   rs1=%rs1_3  rd=%rs2_3
 @cl_d      ... ... ... .. ... .. &i      imm=%uimm_cl_d   rs1=%rs1_3  rd=%rs2_3
 @cl_w      ... ... ... .. ... .. &i      imm=%uimm_cl_w   rs1=%rs1_3  rd=%rs2_3
 @cs_2      ... ... ... .. ... .. &r      rs2=%rs2_3       rs1=%rs1_3  rd=%rs1_3
+@c_sspush  ... ... ... .. ... .. &s      imm=0 rs1=0 rs2=1
 @cs_q      ... ... ... .. ... .. &s      imm=%uimm_cl_q   rs1=%rs1_3  rs2=%rs2_3
 @cs_d      ... ... ... .. ... .. &s      imm=%uimm_cl_d   rs1=%rs1_3  rs2=%rs2_3
 @cs_w      ... ... ... .. ... .. &s      imm=%uimm_cl_w   rs1=%rs1_3  rs2=%rs2_3
@@ -140,6 +142,8 @@ sw                110  ... ... .. ... 00 @cs_w
 addi              000 .  .....  ..... 01 @ci
 addi              010 .  .....  ..... 01 @c_li
 {
+  sspush          011 0  00001  00000 01 @c_sspush # c.sspush x1 carving out of zcmops
+  sspopchk        011 0  00101  00000 01 @c_sspop # c.sspopchk x5 carving out of zcmops
   c_mop_n         011 0 0 n:3 1 00000 01
   illegal         011 0  -----  00000 01 # c.addi16sp and c.lui, RES nzimm=0
   addi            011 .  00010  ..... 01 @c_addi16sp
-- 
2.44.0

Re: [PATCH v3 13/20] target/riscv: compressed encodings for sspush and sspopchk

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> sspush/sspopchk have compressed encodings carved out of zcmops.
> compressed sspush is designated as c.mop.1 while compressed sspopchk
> is designated as c.mop.5.
> 
> Note that c.sspush x1 exists while c.sspush x5 doesn't. Similarly
> c.sspopchk x5 exists while c.sspopchk x1 doesn't.
> 
> 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/insn16.decode | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/riscv/insn16.decode b/target/riscv/insn16.decode
> index 3953bcf82d..d9fb74fef6 100644
> --- a/target/riscv/insn16.decode
> +++ b/target/riscv/insn16.decode
> @@ -69,10 +69,12 @@
>   # Formats 16:
>   @cr        ....  ..... .....  .. &r      rs2=%rs2_5       rs1=%rd     %rd
>   @ci        ... . ..... .....  .. &i      imm=%imm_ci      rs1=%rd     %rd
> +@c_sspop   ... . ..... .....  .. &i      imm=0      rs1=5     rd=0
>   @cl_q      ... . .....  ..... .. &i      imm=%uimm_cl_q   rs1=%rs1_3  rd=%rs2_3
>   @cl_d      ... ... ... .. ... .. &i      imm=%uimm_cl_d   rs1=%rs1_3  rd=%rs2_3
>   @cl_w      ... ... ... .. ... .. &i      imm=%uimm_cl_w   rs1=%rs1_3  rd=%rs2_3
>   @cs_2      ... ... ... .. ... .. &r      rs2=%rs2_3       rs1=%rs1_3  rd=%rs1_3
> +@c_sspush  ... ... ... .. ... .. &s      imm=0 rs1=0 rs2=1
>   @cs_q      ... ... ... .. ... .. &s      imm=%uimm_cl_q   rs1=%rs1_3  rs2=%rs2_3
>   @cs_d      ... ... ... .. ... .. &s      imm=%uimm_cl_d   rs1=%rs1_3  rs2=%rs2_3
>   @cs_w      ... ... ... .. ... .. &s      imm=%uimm_cl_w   rs1=%rs1_3  rs2=%rs2_3
> @@ -140,6 +142,8 @@ sw                110  ... ... .. ... 00 @cs_w
>   addi              000 .  .....  ..... 01 @ci
>   addi              010 .  .....  ..... 01 @c_li
>   {
> +  sspush          011 0  00001  00000 01 @c_sspush # c.sspush x1 carving out of zcmops
> +  sspopchk        011 0  00101  00000 01 @c_sspop # c.sspopchk x5 carving out of zcmops

Again, drop the single-use formats.


r~

[PATCH v3 14/20] target/riscv: mmu changes for zicfiss shadow stack protection

[Deepak Gupta]

zicfiss protects shadow stack using new page table encodings PTE.W=0,
PTE.R=0 and PTE.X=0. This encoding is reserved if zicfiss is not
implemented or if shadow stack are not enabled.
Loads on shadow stack memory are allowed while stores to shadow stack
memory leads to access faults. Shadow stack accesses to RO memory
leads to store page fault.

To implement special nature of shadow stack memory where only selected
stores (shadow stack stores from sspush) have to be allowed while rest
of regular stores disallowed, new MMU TLB index is created for shadow
stack.

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 target/riscv/cpu_helper.c | 61 +++++++++++++++++++++++++++++++++++++--
 target/riscv/internals.h  |  3 ++
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index fb6c0d4e1f..5d5da8dce1 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -820,6 +820,18 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
     env->load_res = -1;
 }
 
+static bool legal_sstack_access(int access_type, bool sstack_inst,
+                                bool sstack_attribute)
+{
+    /*
+     * Read/write/execution permissions are checked as usual. Shadow
+     * stack enforcement is just that (1) instruction type must match
+     * the attribute unless (2) a non-SS load to an SS region.
+     */
+    return (sstack_inst == sstack_attribute) ||
+        ((access_type == MMU_DATA_LOAD) && sstack_attribute);
+}
+
 /*
  * get_physical_address_pmp - check PMP permission for this physical address
  *
@@ -897,6 +909,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
     hwaddr ppn;
     int napot_bits = 0;
     target_ulong napot_mask;
+    bool is_sstack_insn = ((mmu_idx & MMU_IDX_SS_ACCESS) == MMU_IDX_SS_ACCESS);
+    bool sstack_page = false;
 
     /*
      * Check if we should use the background registers for the two
@@ -1105,15 +1119,45 @@ restart:
         return TRANSLATE_FAIL;
     }
 
+    /*
+     * When backward CFI is enabled, the R=0, W=1, X=0 reserved encoding
+     * is used to mark Shadow Stack (SS) pages. If back CFI enabled, allow
+     * normal loads on SS pages, regular stores raise store access fault
+     * and avoid hitting the reserved-encoding case. Only shadow stack
+     * stores are allowed on SS pages. Shadow stack loads and stores on
+     * regular memory (non-SS) raise load and store/AMO access fault.
+     * Second stage translations don't participate in Shadow Stack.
+     */
+    sstack_page = (cpu_get_bcfien(env) && first_stage &&
+                  ((pte & (PTE_R | PTE_W | PTE_X)) == PTE_W));
+
     /* Check for reserved combinations of RWX flags. */
     switch (pte & (PTE_R | PTE_W | PTE_X)) {
-    case PTE_W:
     case PTE_W | PTE_X:
+    case PTE_W:
+        if (sstack_page) { /* if shadow stack page, PTE_W is not reserved */
+            break;
+        }
         return TRANSLATE_FAIL;
     }
 
+    /* Illegal combo of instruction type and page attribute */
+    if (!legal_sstack_access(access_type, is_sstack_insn,
+                            sstack_page)) {
+        /* shadow stack instruction and RO page then it's a page fault */
+        if (is_sstack_insn && ((pte & (PTE_R | PTE_W | PTE_X)) == PTE_R)) {
+                return TRANSLATE_FAIL;
+        }
+        /* In all other cases it's an access fault, so raise PMP_FAIL */
+        return TRANSLATE_PMP_FAIL;
+    }
+
     int prot = 0;
-    if (pte & PTE_R) {
+    /*
+     * If PTE has read bit in it or it's shadow stack page,
+     * then reads allowed
+     */
+    if ((pte & PTE_R) || sstack_page) {
         prot |= PAGE_READ;
     }
     if (pte & PTE_W) {
@@ -1351,9 +1395,17 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
         break;
     case MMU_DATA_LOAD:
         cs->exception_index = RISCV_EXCP_LOAD_ADDR_MIS;
+        /* shadow stack mis aligned accesses are access faults */
+        if (mmu_idx & MMU_IDX_SS_ACCESS) {
+            cs->exception_index = RISCV_EXCP_LOAD_ACCESS_FAULT;
+        }
         break;
     case MMU_DATA_STORE:
         cs->exception_index = RISCV_EXCP_STORE_AMO_ADDR_MIS;
+        /* shadow stack mis aligned accesses are access faults */
+        if (mmu_idx & MMU_IDX_SS_ACCESS) {
+            cs->exception_index = RISCV_EXCP_STORE_AMO_ACCESS_FAULT;
+        }
         break;
     default:
         g_assert_not_reached();
@@ -1409,6 +1461,11 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
                   __func__, address, access_type, mmu_idx);
 
+    /* If shadow stack instruction initiated this access, treat it as store */
+    if (mmu_idx & MMU_IDX_SS_ACCESS) {
+        access_type = MMU_DATA_STORE;
+    }
+
     pmu_tlb_fill_incr_ctr(cpu, access_type);
     if (two_stage_lookup) {
         /* Two stage lookup */
diff --git a/target/riscv/internals.h b/target/riscv/internals.h
index 0ac17bc5ad..dad0657c80 100644
--- a/target/riscv/internals.h
+++ b/target/riscv/internals.h
@@ -30,12 +30,15 @@
  *  - U+2STAGE          0b100
  *  - S+2STAGE          0b101
  *  - S+SUM+2STAGE      0b110
+ *  - Shadow stack+U   0b1000
+ *  - Shadow stack+S   0b1001
  */
 #define MMUIdx_U            0
 #define MMUIdx_S            1
 #define MMUIdx_S_SUM        2
 #define MMUIdx_M            3
 #define MMU_2STAGE_BIT      (1 << 2)
+#define MMU_IDX_SS_ACCESS   (1 << 3)
 
 static inline int mmuidx_priv(int mmu_idx)
 {
-- 
2.44.0

Re: [PATCH v3 14/20] target/riscv: mmu changes for zicfiss shadow stack protection

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> @@ -1105,15 +1119,45 @@ restart:
>           return TRANSLATE_FAIL;
>       }
>   
> +    /*
> +     * When backward CFI is enabled, the R=0, W=1, X=0 reserved encoding
> +     * is used to mark Shadow Stack (SS) pages. If back CFI enabled, allow
> +     * normal loads on SS pages, regular stores raise store access fault
> +     * and avoid hitting the reserved-encoding case. Only shadow stack
> +     * stores are allowed on SS pages. Shadow stack loads and stores on
> +     * regular memory (non-SS) raise load and store/AMO access fault.
> +     * Second stage translations don't participate in Shadow Stack.
> +     */
> +    sstack_page = (cpu_get_bcfien(env) && first_stage &&
> +                  ((pte & (PTE_R | PTE_W | PTE_X)) == PTE_W));
> +
>       /* Check for reserved combinations of RWX flags. */
>       switch (pte & (PTE_R | PTE_W | PTE_X)) {
> -    case PTE_W:
>       case PTE_W | PTE_X:
> +    case PTE_W:
> +        if (sstack_page) { /* if shadow stack page, PTE_W is not reserved */
> +            break;

This is oddly written, and duplicates checks.  Better as

     switch (pte & RWX) {
     case W | X:
         return TRANSLATE_FAIL;
     case W:
         if (bcfi && first_stage) {
             sstack_page = true;
             break;
         }
         return TRANSLATE_FAIL;
     }


> +    /* Illegal combo of instruction type and page attribute */
> +    if (!legal_sstack_access(access_type, is_sstack_insn,
> +                            sstack_page)) {
> +        /* shadow stack instruction and RO page then it's a page fault */
> +        if (is_sstack_insn && ((pte & (PTE_R | PTE_W | PTE_X)) == PTE_R)) {
> +                return TRANSLATE_FAIL;
> +        }
> +        /* In all other cases it's an access fault, so raise PMP_FAIL */
> +        return TRANSLATE_PMP_FAIL;
> +    }
> +
>       int prot = 0;
> -    if (pte & PTE_R) {
> +    /*
> +     * If PTE has read bit in it or it's shadow stack page,
> +     * then reads allowed
> +     */
> +    if ((pte & PTE_R) || sstack_page) {
>           prot |= PAGE_READ;
>       }

I feel like this logic could be simplified somehow.
I'll think about it.

> @@ -1409,6 +1461,11 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>                     __func__, address, access_type, mmu_idx);
>   
> +    /* If shadow stack instruction initiated this access, treat it as store */
> +    if (mmu_idx & MMU_IDX_SS_ACCESS) {
> +        access_type = MMU_DATA_STORE;
> +    }

I know you're trying to massage the fault type, but I think this is the wrong place.


r~

Re: [PATCH v3 14/20] target/riscv: mmu changes for zicfiss shadow stack protection

[Deepak Gupta]

On Wed, Aug 07, 2024 at 01:19:55PM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>      int prot = 0;
>>-    if (pte & PTE_R) {
>>+    /*
>>+     * If PTE has read bit in it or it's shadow stack page,
>>+     * then reads allowed
>>+     */
>>+    if ((pte & PTE_R) || sstack_page) {
>>          prot |= PAGE_READ;
>>      }
>
>I feel like this logic could be simplified somehow.
>I'll think about it.

Ok let me know.

>
>>@@ -1409,6 +1461,11 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>      qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>>                    __func__, address, access_type, mmu_idx);
>>+    /* If shadow stack instruction initiated this access, treat it as store */
>>+    if (mmu_idx & MMU_IDX_SS_ACCESS) {
>>+        access_type = MMU_DATA_STORE;
>>+    }
>
>I know you're trying to massage the fault type, but I think this is the wrong place.

Is it okay if I add `mmu_idx` argument to `raise_mmu_exception` ?
Inside `raise_mmu_exception`, then based on `mmu_idx == shadow stack index`, I can convert
a fault due to access_type=MMU_DATA_LOAD into store page fault.

>
>
>r~

Re: [PATCH v3 14/20] target/riscv: mmu changes for zicfiss shadow stack protection

[Richard Henderson]

On 8/10/24 04:55, Deepak Gupta wrote:
> On Wed, Aug 07, 2024 at 01:19:55PM +1000, Richard Henderson wrote:
>> On 8/7/24 10:06, Deepak Gupta wrote:
>>>      int prot = 0;
>>> -    if (pte & PTE_R) {
>>> +    /*
>>> +     * If PTE has read bit in it or it's shadow stack page,
>>> +     * then reads allowed
>>> +     */
>>> +    if ((pte & PTE_R) || sstack_page) {
>>>          prot |= PAGE_READ;
>>>      }
>>
>> I feel like this logic could be simplified somehow.
>> I'll think about it.
> 
> Ok let me know.
> 
>>
>>> @@ -1409,6 +1461,11 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>>      qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>>>                    __func__, address, access_type, mmu_idx);
>>> +    /* If shadow stack instruction initiated this access, treat it as store */
>>> +    if (mmu_idx & MMU_IDX_SS_ACCESS) {
>>> +        access_type = MMU_DATA_STORE;
>>> +    }
>>
>> I know you're trying to massage the fault type, but I think this is the wrong place.
> 
> Is it okay if I add `mmu_idx` argument to `raise_mmu_exception` ?
> Inside `raise_mmu_exception`, then based on `mmu_idx == shadow stack index`, I can convert
> a fault due to access_type=MMU_DATA_LOAD into store page fault.

We have other places where we miss-categorize amo instructions and raise the wrong fault, 
I think particularly without smp, when we implement amo without host atomic operations. 
We should perhaps come up with a general purpose solution.

For instance, set TARGET_INSN_START_EXTRA_WORDS to 2.  In the second extra unwind word, 
set bit 0 to 1 if the instruction should raise STORE_AMO on a read fault.  Handle this in 
raise_mmu_exception, which would then need to perform the restore and exit itself.


r~

[PATCH v3 15/20] target/riscv: shadow stack mmu index for shadow stack instructions

[Deepak Gupta]

Shadow stack instructions shadow stack mmu index for load/stores.
`MMU_IDX_SS_ACCESS` at bit positon 3 is used as shadow stack index.
Shadow stack mmu index depend on privilege and SUM bit. If shadow stack
accesses happening in user mode, shadow stack mmu index = 0b1000. If
shaodw stack access happening in supervisor mode mmu index = 0b1001. If
shadow stack access happening in supervisor mode with SUM=1 then mmu
index = 0b1010

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 target/riscv/cpu.h                            | 13 ++++++++++
 target/riscv/cpu_helper.c                     |  3 +++
 target/riscv/insn_trans/trans_rva.c.inc       |  8 ++++++
 target/riscv/insn_trans/trans_rvzicfiss.c.inc |  6 +++++
 target/riscv/internals.h                      |  1 +
 target/riscv/translate.c                      | 25 +++++++++++++++++++
 6 files changed, 56 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 6da94c417c..3ad220a9fe 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -615,6 +615,19 @@ FIELD(TB_FLAGS, FCFI_ENABLED, 28, 1)
 FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 29, 1)
 /* zicfiss needs a TB flag so that correct TB is located based on tb flags */
 FIELD(TB_FLAGS, BCFI_ENABLED, 30, 1)
+/*
+ * zicfiss shadow stack is special memory on which regular stores aren't
+ * allowed but shadow stack stores are allowed. Shadow stack stores can
+ * happen as `sspush` or `ssamoswap` instructions. `sspush` implicitly
+ * takes shadow stack address from CSR_SSP. But `ssamoswap` takes address
+ * from encoded input register and it will be used by supervisor software
+ * to access (read/write) user shadow stack for setting up rt_frame during
+ * signal delivery. Supervisor software will do so by setting SUM=1. Thus
+ * a TB flag is needed if SUM was 1 during TB generation to correctly
+ * reflect memory permissions to access shadow stack user memory from
+ * supervisor mode.
+ */
+FIELD(TB_FLAGS, SUM, 31, 1)
 
 #ifdef TARGET_RISCV32
 #define riscv_cpu_mxl(env)  ((void)(env), MXL_RV32)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 5d5da8dce1..ad40b10e74 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -181,6 +181,9 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
     fs = EXT_STATUS_DIRTY;
     vs = EXT_STATUS_DIRTY;
 #else
+   flags = FIELD_DP32(flags, TB_FLAGS, SUM,
+                    ((env->mstatus & MSTATUS_SUM) == MSTATUS_SUM));
+
     flags = FIELD_DP32(flags, TB_FLAGS, PRIV, env->priv);
 
     flags |= riscv_env_mmu_index(env, 0);
diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
index db6c03f6a8..68b71339a3 100644
--- a/target/riscv/insn_trans/trans_rva.c.inc
+++ b/target/riscv/insn_trans/trans_rva.c.inc
@@ -132,6 +132,10 @@ static bool trans_ssamoswap_w(DisasContext *ctx, arg_amoswap_w *a)
 
     decode_save_opc(ctx);
     src1 = get_address(ctx, a->rs1, 0);
+#ifndef CONFIG_USER_ONLY
+    /* Shadow stack access and thus index is SS TLB index */
+    ss_mmu_idx = get_ss_index(ctx);
+#endif
 
     tcg_gen_atomic_xchg_tl(dest, src1, src2, ss_mmu_idx, (MO_ALIGN | MO_TESL));
     gen_set_gpr(ctx, a->rd, dest);
@@ -224,6 +228,10 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a)
 
     decode_save_opc(ctx);
     src1 = get_address(ctx, a->rs1, 0);
+#ifndef CONFIG_USER_ONLY
+    /* Shadow stack access and thus index is SS TLB index */
+    ss_mmu_idx = get_ss_index(ctx);
+#endif
 
     tcg_gen_atomic_xchg_tl(dest, src1, src2, ss_mmu_idx, (MO_ALIGN | MO_TESQ));
     gen_set_gpr(ctx, a->rd, dest);
diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
index c538b7ad99..4e741c061d 100644
--- a/target/riscv/insn_trans/trans_rvzicfiss.c.inc
+++ b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
@@ -70,6 +70,9 @@ static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a)
     TCGv_i32 ssp_csr = tcg_constant_i32(CSR_SSP);
     TCGv data = tcg_temp_new();
     gen_helper_csrr(addr, tcg_env, ssp_csr);
+#ifndef CONFIG_USER_ONLY
+    ss_mmu_idx = get_ss_index(ctx);
+#endif
 
     tcg_gen_qemu_ld_tl(data, addr, ss_mmu_idx,
                        mxl_memop(ctx) | MO_ALIGN);
@@ -118,6 +121,9 @@ static bool trans_sspush(DisasContext *ctx, arg_sspush *a)
     TCGv_i32 ssp_csr = tcg_constant_i32(CSR_SSP);
     TCGv data = get_gpr(ctx, a->rs2, EXT_NONE);
     gen_helper_csrr(addr, tcg_env, ssp_csr);
+#ifndef CONFIG_USER_ONLY
+    ss_mmu_idx = get_ss_index(ctx);
+#endif
 
     tcg_gen_addi_tl(addr, addr, tmp);
 
diff --git a/target/riscv/internals.h b/target/riscv/internals.h
index dad0657c80..5147d6bf90 100644
--- a/target/riscv/internals.h
+++ b/target/riscv/internals.h
@@ -32,6 +32,7 @@
  *  - S+SUM+2STAGE      0b110
  *  - Shadow stack+U   0b1000
  *  - Shadow stack+S   0b1001
+ *  - Shadow stack+SUM 0b1010
  */
 #define MMUIdx_U            0
 #define MMUIdx_S            1
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index de375c32a1..4772191bd8 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -122,6 +122,8 @@ typedef struct DisasContext {
     bool fcfi_lp_expected;
     /* zicfiss extension, if shadow stack was enabled during TB gen */
     bool bcfi_enabled;
+    /* SUM was on during tb translation? */
+    bool sum;
 } DisasContext;
 
 static inline bool has_ext(DisasContext *ctx, uint32_t ext)
@@ -1127,6 +1129,29 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
     return translator_ldl(env, &ctx->base, pc);
 }
 
+#ifndef CONFIG_USER_ONLY
+static unsigned int get_ss_index(DisasContext *ctx)
+{
+    int ss_mmu_idx = MMU_IDX_SS_ACCESS;
+
+    /*
+     * If priv mode is S then a separate index for supervisor
+     * shadow stack accesses
+     */
+    if (ctx->priv == PRV_S) {
+        ss_mmu_idx |= MMUIdx_S;
+    }
+
+    /* If SUM was set, SS index should have S cleared */
+    if (ctx->sum) {
+        ss_mmu_idx &= ~(MMUIdx_S);
+        ss_mmu_idx |= MMUIdx_S_SUM;
+    }
+
+    return ss_mmu_idx;
+}
+#endif
+
 /* Include insn module translation function */
 #include "insn_trans/trans_rvi.c.inc"
 #include "insn_trans/trans_rvm.c.inc"
-- 
2.44.0

Re: [PATCH v3 15/20] target/riscv: shadow stack mmu index for shadow stack instructions

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> Shadow stack instructions shadow stack mmu index for load/stores.
> `MMU_IDX_SS_ACCESS` at bit positon 3 is used as shadow stack index.
> Shadow stack mmu index depend on privilege and SUM bit. If shadow stack
> accesses happening in user mode, shadow stack mmu index = 0b1000. If
> shaodw stack access happening in supervisor mode mmu index = 0b1001. If
> shadow stack access happening in supervisor mode with SUM=1 then mmu
> index = 0b1010
> 
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>   target/riscv/cpu.h                            | 13 ++++++++++
>   target/riscv/cpu_helper.c                     |  3 +++
>   target/riscv/insn_trans/trans_rva.c.inc       |  8 ++++++
>   target/riscv/insn_trans/trans_rvzicfiss.c.inc |  6 +++++
>   target/riscv/internals.h                      |  1 +
>   target/riscv/translate.c                      | 25 +++++++++++++++++++
>   6 files changed, 56 insertions(+)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 6da94c417c..3ad220a9fe 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -615,6 +615,19 @@ FIELD(TB_FLAGS, FCFI_ENABLED, 28, 1)
>   FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 29, 1)
>   /* zicfiss needs a TB flag so that correct TB is located based on tb flags */
>   FIELD(TB_FLAGS, BCFI_ENABLED, 30, 1)
> +/*
> + * zicfiss shadow stack is special memory on which regular stores aren't
> + * allowed but shadow stack stores are allowed. Shadow stack stores can
> + * happen as `sspush` or `ssamoswap` instructions. `sspush` implicitly
> + * takes shadow stack address from CSR_SSP. But `ssamoswap` takes address
> + * from encoded input register and it will be used by supervisor software
> + * to access (read/write) user shadow stack for setting up rt_frame during
> + * signal delivery. Supervisor software will do so by setting SUM=1. Thus
> + * a TB flag is needed if SUM was 1 during TB generation to correctly
> + * reflect memory permissions to access shadow stack user memory from
> + * supervisor mode.
> + */
> +FIELD(TB_FLAGS, SUM, 31, 1)

This is already encoded into the mmu_idx as MMUIdx_S_SUM.


r~

Re: [PATCH v3 15/20] target/riscv: shadow stack mmu index for shadow stack instructions

[Deepak Gupta]

On Wed, Aug 07, 2024 at 12:43:31PM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>Shadow stack instructions shadow stack mmu index for load/stores.
>>`MMU_IDX_SS_ACCESS` at bit positon 3 is used as shadow stack index.
>>Shadow stack mmu index depend on privilege and SUM bit. If shadow stack
>>accesses happening in user mode, shadow stack mmu index = 0b1000. If
>>shaodw stack access happening in supervisor mode mmu index = 0b1001. If
>>shadow stack access happening in supervisor mode with SUM=1 then mmu
>>index = 0b1010
>>
>>Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>>---
>>  target/riscv/cpu.h                            | 13 ++++++++++
>>  target/riscv/cpu_helper.c                     |  3 +++
>>  target/riscv/insn_trans/trans_rva.c.inc       |  8 ++++++
>>  target/riscv/insn_trans/trans_rvzicfiss.c.inc |  6 +++++
>>  target/riscv/internals.h                      |  1 +
>>  target/riscv/translate.c                      | 25 +++++++++++++++++++
>>  6 files changed, 56 insertions(+)
>>
>>diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>index 6da94c417c..3ad220a9fe 100644
>>--- a/target/riscv/cpu.h
>>+++ b/target/riscv/cpu.h
>>@@ -615,6 +615,19 @@ FIELD(TB_FLAGS, FCFI_ENABLED, 28, 1)
>>  FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 29, 1)
>>  /* zicfiss needs a TB flag so that correct TB is located based on tb flags */
>>  FIELD(TB_FLAGS, BCFI_ENABLED, 30, 1)
>>+/*
>>+ * zicfiss shadow stack is special memory on which regular stores aren't
>>+ * allowed but shadow stack stores are allowed. Shadow stack stores can
>>+ * happen as `sspush` or `ssamoswap` instructions. `sspush` implicitly
>>+ * takes shadow stack address from CSR_SSP. But `ssamoswap` takes address
>>+ * from encoded input register and it will be used by supervisor software
>>+ * to access (read/write) user shadow stack for setting up rt_frame during
>>+ * signal delivery. Supervisor software will do so by setting SUM=1. Thus
>>+ * a TB flag is needed if SUM was 1 during TB generation to correctly
>>+ * reflect memory permissions to access shadow stack user memory from
>>+ * supervisor mode.
>>+ */
>>+FIELD(TB_FLAGS, SUM, 31, 1)
>
>This is already encoded into the mmu_idx as MMUIdx_S_SUM.

This is where I need some help / suggestion and clarifications.

`riscv_env_mmu_index` is the which does mode --> mmu index translation and that's
where `MMUIdx_S_SUM` is set.

Although above function assumes following things
    -- Only loads ands stores are supposed to do read and write.
    -- Translates env/priv --> mmu index

In case of shadow stack, we need to hold following true:
Shadow stack are not writeable via regular stores but are allowed to be readable.
Shadow stack are writeable only via shadow stack instruction.
Shadow stack instructions can't operate on non-shadow stack memory.

This let me to create a new mmu index (as you saw in patches). This mmu index is only
setup by shadow stack instruction and thus has to be known at translation time
(and that's why SUM TB flag)

There is no way of telling in `riscv_env_mmu_index` about whether mmu index is requested
for regular load/store or some other instruction (in this case shadow stack instruction).
If that is available then I think I can use `riscv_env_mmu_index`.

Question:
I see that `riscv_env_mmu_index` could be called from a bunch of places in (like
`accel/tcg/ldst_common.c.inc` as well. Does it exclude loads, stores which calculate mmu
indexes during translation (like shadow stack load, stores) ?

>
>
>r~

Re: [PATCH v3 15/20] target/riscv: shadow stack mmu index for shadow stack instructions

[Richard Henderson]

On 8/8/24 07:23, Deepak Gupta wrote:
> On Wed, Aug 07, 2024 at 12:43:31PM +1000, Richard Henderson wrote:
>> On 8/7/24 10:06, Deepak Gupta wrote:
>>> Shadow stack instructions shadow stack mmu index for load/stores.
>>> `MMU_IDX_SS_ACCESS` at bit positon 3 is used as shadow stack index.
>>> Shadow stack mmu index depend on privilege and SUM bit. If shadow stack
>>> accesses happening in user mode, shadow stack mmu index = 0b1000. If
>>> shaodw stack access happening in supervisor mode mmu index = 0b1001. If
>>> shadow stack access happening in supervisor mode with SUM=1 then mmu
>>> index = 0b1010
>>>
>>> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>>> ---
>>>  target/riscv/cpu.h                            | 13 ++++++++++
>>>  target/riscv/cpu_helper.c                     |  3 +++
>>>  target/riscv/insn_trans/trans_rva.c.inc       |  8 ++++++
>>>  target/riscv/insn_trans/trans_rvzicfiss.c.inc |  6 +++++
>>>  target/riscv/internals.h                      |  1 +
>>>  target/riscv/translate.c                      | 25 +++++++++++++++++++
>>>  6 files changed, 56 insertions(+)
>>>
>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>> index 6da94c417c..3ad220a9fe 100644
>>> --- a/target/riscv/cpu.h
>>> +++ b/target/riscv/cpu.h
>>> @@ -615,6 +615,19 @@ FIELD(TB_FLAGS, FCFI_ENABLED, 28, 1)
>>>  FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 29, 1)
>>>  /* zicfiss needs a TB flag so that correct TB is located based on tb flags */
>>>  FIELD(TB_FLAGS, BCFI_ENABLED, 30, 1)
>>> +/*
>>> + * zicfiss shadow stack is special memory on which regular stores aren't
>>> + * allowed but shadow stack stores are allowed. Shadow stack stores can
>>> + * happen as `sspush` or `ssamoswap` instructions. `sspush` implicitly
>>> + * takes shadow stack address from CSR_SSP. But `ssamoswap` takes address
>>> + * from encoded input register and it will be used by supervisor software
>>> + * to access (read/write) user shadow stack for setting up rt_frame during
>>> + * signal delivery. Supervisor software will do so by setting SUM=1. Thus
>>> + * a TB flag is needed if SUM was 1 during TB generation to correctly
>>> + * reflect memory permissions to access shadow stack user memory from
>>> + * supervisor mode.
>>> + */
>>> +FIELD(TB_FLAGS, SUM, 31, 1)
>>
>> This is already encoded into the mmu_idx as MMUIdx_S_SUM.
> 
> This is where I need some help / suggestion and clarifications.
> 
> `riscv_env_mmu_index` is the which does mode --> mmu index translation and that's
> where `MMUIdx_S_SUM` is set.
> 
> Although above function assumes following things
>     -- Only loads ands stores are supposed to do read and write.
>     -- Translates env/priv --> mmu index
> 
> In case of shadow stack, we need to hold following true:
> Shadow stack are not writeable via regular stores but are allowed to be readable.
> Shadow stack are writeable only via shadow stack instruction.
> Shadow stack instructions can't operate on non-shadow stack memory.
> 
> This let me to create a new mmu index (as you saw in patches). This mmu index is only
> setup by shadow stack instruction and thus has to be known at translation time

All good so far.

> There is no way of telling in `riscv_env_mmu_index` about whether mmu index is requested
> for regular load/store or some other instruction (in this case shadow stack instruction).
> If that is available then I think I can use `riscv_env_mmu_index`.

What you miss is that the result of riscv_env_mmu_index is stored

   ctx->mem_idx

So that takes care of U, S, SUM, M, VS, VU, etc.  All you need at
this point is to or in your shadow stack bit:

   ctx->mem_idx | MMU_IDX_SS_ACCESS

(Perhaps SS_WRITE is a better name, since read access is never prohibited?)

Note that you can do this without ifdefs -- user-only will happily accept and ignore the 
mmu index.  Also note that user-only will *not* be able to restrict access to the shadow 
stack pages in the way the spec describes.  We rely on the host mmu for read/write 
permission for user-only.  For now -- changing that is a long term goal.


> Question:
> I see that `riscv_env_mmu_index` could be called from a bunch of places in (like
> `accel/tcg/ldst_common.c.inc` as well. Does it exclude loads, stores which calculate mmu
> indexes during translation (like shadow stack load, stores) ?

It means you cannot use the legacy interfaces for the shadow stack.
The current interfaces:

  *        cpu_ld{sign}{size}{end}_mmuidx_ra(env, ptr, mmu_idx, retaddr)
  *        cpu_ld{sign}{size}{end}_mmu(env, ptr, oi, retaddr)
...
  *        cpu_st{size}{end}_mmuidx_ra(env, ptr, val, mmu_idx, retaddr)
  *        cpu_st{size}{end}_mmu(env, ptr, val, oi, retaddr)

take the mmu_idx as a parameter.

But as it happens, the shadow stack instructions are simple enough to implement all 
inline, so you won't need to call the out-of-line load/store functions from cpu helpers.


r~

Re: [PATCH v3 15/20] target/riscv: shadow stack mmu index for shadow stack instructions

[Deepak Gupta]

On Thu, Aug 08, 2024 at 08:57:47AM +1000, Richard Henderson wrote:
>On 8/8/24 07:23, Deepak Gupta wrote:
>>On Wed, Aug 07, 2024 at 12:43:31PM +1000, Richard Henderson wrote:
>>>On 8/7/24 10:06, Deepak Gupta wrote:
>>>>Shadow stack instructions shadow stack mmu index for load/stores.
>>>>`MMU_IDX_SS_ACCESS` at bit positon 3 is used as shadow stack index.
>>>>Shadow stack mmu index depend on privilege and SUM bit. If shadow stack
>>>>accesses happening in user mode, shadow stack mmu index = 0b1000. If
>>>>shaodw stack access happening in supervisor mode mmu index = 0b1001. If
>>>>shadow stack access happening in supervisor mode with SUM=1 then mmu
>>>>index = 0b1010
>>>>
>>>>Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>>>>---
>>>> target/riscv/cpu.h                            | 13 ++++++++++
>>>> target/riscv/cpu_helper.c                     |  3 +++
>>>> target/riscv/insn_trans/trans_rva.c.inc       |  8 ++++++
>>>> target/riscv/insn_trans/trans_rvzicfiss.c.inc |  6 +++++
>>>> target/riscv/internals.h                      |  1 +
>>>> target/riscv/translate.c                      | 25 +++++++++++++++++++
>>>> 6 files changed, 56 insertions(+)
>>>>
>>>>diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>>index 6da94c417c..3ad220a9fe 100644
>>>>--- a/target/riscv/cpu.h
>>>>+++ b/target/riscv/cpu.h
>>>>@@ -615,6 +615,19 @@ FIELD(TB_FLAGS, FCFI_ENABLED, 28, 1)
>>>> FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 29, 1)
>>>> /* zicfiss needs a TB flag so that correct TB is located based on tb flags */
>>>> FIELD(TB_FLAGS, BCFI_ENABLED, 30, 1)
>>>>+/*
>>>>+ * zicfiss shadow stack is special memory on which regular stores aren't
>>>>+ * allowed but shadow stack stores are allowed. Shadow stack stores can
>>>>+ * happen as `sspush` or `ssamoswap` instructions. `sspush` implicitly
>>>>+ * takes shadow stack address from CSR_SSP. But `ssamoswap` takes address
>>>>+ * from encoded input register and it will be used by supervisor software
>>>>+ * to access (read/write) user shadow stack for setting up rt_frame during
>>>>+ * signal delivery. Supervisor software will do so by setting SUM=1. Thus
>>>>+ * a TB flag is needed if SUM was 1 during TB generation to correctly
>>>>+ * reflect memory permissions to access shadow stack user memory from
>>>>+ * supervisor mode.
>>>>+ */
>>>>+FIELD(TB_FLAGS, SUM, 31, 1)
>>>
>>>This is already encoded into the mmu_idx as MMUIdx_S_SUM.
>>
>>This is where I need some help / suggestion and clarifications.
>>
>>`riscv_env_mmu_index` is the which does mode --> mmu index translation and that's
>>where `MMUIdx_S_SUM` is set.
>>
>>Although above function assumes following things
>>    -- Only loads ands stores are supposed to do read and write.
>>    -- Translates env/priv --> mmu index
>>
>>In case of shadow stack, we need to hold following true:
>>Shadow stack are not writeable via regular stores but are allowed to be readable.
>>Shadow stack are writeable only via shadow stack instruction.
>>Shadow stack instructions can't operate on non-shadow stack memory.
>>
>>This let me to create a new mmu index (as you saw in patches). This mmu index is only
>>setup by shadow stack instruction and thus has to be known at translation time
>
>All good so far.
>
>>There is no way of telling in `riscv_env_mmu_index` about whether mmu index is requested
>>for regular load/store or some other instruction (in this case shadow stack instruction).
>>If that is available then I think I can use `riscv_env_mmu_index`.
>
>What you miss is that the result of riscv_env_mmu_index is stored
>
>  ctx->mem_idx

Yeah, that's right. thanks.

>
>So that takes care of U, S, SUM, M, VS, VU, etc.  All you need at
>this point is to or in your shadow stack bit:
>
>  ctx->mem_idx | MMU_IDX_SS_ACCESS


>
>(Perhaps SS_WRITE is a better name, since read access is never prohibited?)

Yeah MMU_IDX_SS_WRITE is probably more self-explainatory.

>
>Note that you can do this without ifdefs -- user-only will happily 
>accept and ignore the mmu index.  Also note that user-only will *not* 
>be able to restrict access to the shadow stack pages in the way the 
>spec describes.  We rely on the host mmu for read/write permission for 
>user-only.  For now -- changing that is a long term goal.

Ok, didn't know user-only will ignore.
I'll remove ifdef and simply do

ctx->mem_idx =| MMU_IDX_SS_WRITE

>
>
>>Question:
>>I see that `riscv_env_mmu_index` could be called from a bunch of places in (like
>>`accel/tcg/ldst_common.c.inc` as well. Does it exclude loads, stores which calculate mmu
>>indexes during translation (like shadow stack load, stores) ?
>
>It means you cannot use the legacy interfaces for the shadow stack.
>The current interfaces:
>
> *        cpu_ld{sign}{size}{end}_mmuidx_ra(env, ptr, mmu_idx, retaddr)
> *        cpu_ld{sign}{size}{end}_mmu(env, ptr, oi, retaddr)
>...
> *        cpu_st{size}{end}_mmuidx_ra(env, ptr, val, mmu_idx, retaddr)
> *        cpu_st{size}{end}_mmu(env, ptr, val, oi, retaddr)
>
>take the mmu_idx as a parameter.
>
>But as it happens, the shadow stack instructions are simple enough to 
>implement all inline, so you won't need to call the out-of-line 
>load/store functions from cpu helpers.

Thanks for explaining this.

>
>
>r~

[PATCH v3 16/20] disas/riscv: enable disassembly for zicfiss instructions

[Deepak Gupta]

Enable disassembly for sspush, sspopchk, ssrdp & ssamoswap.
Disasembly is only enabled if zimop and zicfiss ext is set to true.

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 disas/riscv.c | 34 ++++++++++++++++++++++++++++++++++
 disas/riscv.h |  1 +
 2 files changed, 35 insertions(+)

diff --git a/disas/riscv.c b/disas/riscv.c
index c7c92acef7..c4e47fbc78 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -975,6 +975,11 @@ typedef enum {
     rv_op_amocas_b  = 944,
     rv_op_amocas_h  = 945,
     rv_op_lpad = 946,
+    rv_op_sspush = 947,
+    rv_op_sspopchk = 948,
+    rv_op_ssrdp = 949,
+    rv_op_ssamoswap_w = 950,
+    rv_op_ssamoswap_d = 951,
 } rv_op;
 
 /* register names */
@@ -2234,6 +2239,11 @@ const rv_opcode_data rvi_opcode_data[] = {
     { "amocas.b", rv_codec_r_a, rv_fmt_aqrl_rd_rs2_rs1, NULL, 0, 0, 0 },
     { "amocas.h", rv_codec_r_a, rv_fmt_aqrl_rd_rs2_rs1, NULL, 0, 0, 0 },
     { "lpad", rv_codec_lp, rv_fmt_imm, NULL, 0, 0, 0 },
+    { "sspush", rv_codec_r, rv_fmt_rs2, NULL, 0, 0, 0 },
+    { "sspopchk", rv_codec_r, rv_fmt_rs1, NULL, 0, 0, 0 },
+    { "ssrdp", rv_codec_r, rv_fmt_rd, NULL, 0, 0, 0 },
+    { "ssamoswap.w", rv_codec_r_a, rv_fmt_aqrl_rd_rs2_rs1, NULL, 0, 0, 0 },
+    { "ssamoswap.d", rv_codec_r_a, rv_fmt_aqrl_rd_rs2_rs1, NULL, 0, 0, 0 },
 };
 
 /* CSR names */
@@ -2251,6 +2261,7 @@ static const char *csr_name(int csrno)
     case 0x0009: return "vxsat";
     case 0x000a: return "vxrm";
     case 0x000f: return "vcsr";
+    case 0x0011: return "ssp";
     case 0x0015: return "seed";
     case 0x0017: return "jvt";
     case 0x0040: return "uscratch";
@@ -3077,6 +3088,8 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa)
             case 66: op = rv_op_amoor_w; break;
             case 67: op = rv_op_amoor_d; break;
             case 68: op = rv_op_amoor_q; break;
+            case 74: op = rv_op_ssamoswap_w; break;
+            case 75: op = rv_op_ssamoswap_d; break;
             case 96: op = rv_op_amoand_b; break;
             case 97: op = rv_op_amoand_h; break;
             case 98: op = rv_op_amoand_w; break;
@@ -4036,11 +4049,32 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa)
                                                        extract32(inst, 26, 2)),
                                              4, 1, extract32(inst, 30, 1));
                         op = rv_mop_r_0 + imm_mop5;
+                        /* if zicfiss enabled and mop5 is shadow stack */
+                        if (dec->cfg->ext_zicfiss &&
+                            ((imm_mop5 & 0b11100) == 0b11100)) {
+                                /* rs1=0 means ssrdp */
+                                if ((inst & (0b011111 << 15)) == 0) {
+                                    op = rv_op_ssrdp;
+                                    }
+                                /* rd=0 means sspopchk */
+                                if ((inst & (0b011111 << 7)) == 0) {
+                                    op = rv_op_sspopchk;
+                                }
+                        }
                     } else if ((extract32(inst, 25, 7) & 0b1011001)
                                == 0b1000001) {
                         imm_mop3 = deposit32(extract32(inst, 26, 2),
                                              2, 1, extract32(inst, 30, 1));
                         op = rv_mop_rr_0 + imm_mop3;
+                        /* if zicfiss enabled and mop3 is shadow stack */
+                        if (dec->cfg->ext_zicfiss &&
+                            ((imm_mop3 & 0b111) == 0b111)) {
+                                /* rs1=0 and rd=0 means sspush */
+                                if (((inst & (0b011111 << 15)) == 0) &&
+                                    ((inst & (0b011111 << 7)) == 0)) {
+                                    op = rv_op_sspush;
+                                }
+                        }
                     }
                 }
                 break;
diff --git a/disas/riscv.h b/disas/riscv.h
index 1182457aff..4895c5a301 100644
--- a/disas/riscv.h
+++ b/disas/riscv.h
@@ -224,6 +224,7 @@ enum {
 
 #define rv_fmt_none                   "O\t"
 #define rv_fmt_rs1                    "O\t1"
+#define rv_fmt_rs2                    "O\t2"
 #define rv_fmt_offset                 "O\to"
 #define rv_fmt_pred_succ              "O\tp,s"
 #define rv_fmt_rs1_rs2                "O\t1,2"
-- 
2.44.0

Re: [PATCH v3 16/20] disas/riscv: enable disassembly for zicfiss instructions

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> +                        /* if zicfiss enabled and mop5 is shadow stack */
> +                        if (dec->cfg->ext_zicfiss &&
> +                            ((imm_mop5 & 0b11100) == 0b11100)) {
> +                                /* rs1=0 means ssrdp */
> +                                if ((inst & (0b011111 << 15)) == 0) {
> +                                    op = rv_op_ssrdp;
> +                                    }
> +                                /* rd=0 means sspopchk */
> +                                if ((inst & (0b011111 << 7)) == 0) {
> +                                    op = rv_op_sspopchk;
> +                                }
> +                        }

sspopchk are only rs1=1 or rs1=5.
Similarly for sspush and rs2.


r~

[PATCH v3 17/20] disas/riscv: enable disassembly for compressed sspush/sspopchk

[Deepak Gupta]

sspush and sspopchk have equivalent compressed encoding taken from zcmop.
cmop.1 is sspush x1 while cmop.5 is sspopchk x5. Due to unusual encoding
for both rs1 and rs2 from space bitfield, this required a new codec.

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 disas/riscv.c | 19 ++++++++++++++++++-
 disas/riscv.h |  1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/disas/riscv.c b/disas/riscv.c
index c4e47fbc78..82175e75ee 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -980,6 +980,8 @@ typedef enum {
     rv_op_ssrdp = 949,
     rv_op_ssamoswap_w = 950,
     rv_op_ssamoswap_d = 951,
+    rv_op_c_sspush = 952,
+    rv_op_c_sspopchk = 953,
 } rv_op;
 
 /* register names */
@@ -2244,6 +2246,10 @@ const rv_opcode_data rvi_opcode_data[] = {
     { "ssrdp", rv_codec_r, rv_fmt_rd, NULL, 0, 0, 0 },
     { "ssamoswap.w", rv_codec_r_a, rv_fmt_aqrl_rd_rs2_rs1, NULL, 0, 0, 0 },
     { "ssamoswap.d", rv_codec_r_a, rv_fmt_aqrl_rd_rs2_rs1, NULL, 0, 0, 0 },
+    { "c.sspush", rv_codec_cmop_ss, rv_fmt_rs2, NULL, rv_op_sspush,
+      rv_op_sspush, 0 },
+    { "c.sspopchk", rv_codec_cmop_ss, rv_fmt_rs1, NULL, rv_op_sspopchk,
+      rv_op_sspopchk, 0 },
 };
 
 /* CSR names */
@@ -2604,7 +2610,13 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa)
             if (dec->cfg->ext_zcmop) {
                 if ((((inst >> 2) & 0b111111) == 0b100000) &&
                     (((inst >> 11) & 0b11) == 0b0)) {
-                    op = rv_c_mop_1 + ((inst >> 8) & 0b111);
+                    unsigned int cmop_code = 0;
+                    cmop_code = ((inst >> 8) & 0b111);
+                    op = rv_c_mop_1 + cmop_code;
+                    if (dec->cfg->ext_zicfiss) {
+                        op = (cmop_code == 0) ? rv_op_c_sspush : op;
+                        op = (cmop_code == 2) ? rv_op_c_sspopchk : op;
+                    }
                     break;
                 }
             }
@@ -4919,6 +4931,11 @@ static void decode_inst_operands(rv_decode *dec, rv_isa isa)
     case rv_codec_lp:
         dec->imm = operand_lpl(inst);
         break;
+    case rv_codec_cmop_ss:
+        dec->rd = rv_ireg_zero;
+        dec->rs1 = dec->rs2 = operand_crs1(inst);
+        dec->imm = 0;
+        break;
     };
 }
 
diff --git a/disas/riscv.h b/disas/riscv.h
index 4895c5a301..6a3b371cd3 100644
--- a/disas/riscv.h
+++ b/disas/riscv.h
@@ -167,6 +167,7 @@ typedef enum {
     rv_codec_r2_imm2_imm5,
     rv_codec_fli,
     rv_codec_lp,
+    rv_codec_cmop_ss,
 } rv_codec;
 
 /* structures */
-- 
2.44.0

[PATCH v3 18/20] target/riscv: add trace-hooks for each case of sw-check exception

[Deepak Gupta]

Violations to control flow rules setup by zicfilp and zicfiss lead to
software check exceptions. To debug and fix such sw check issues in guest
, add trace-hooks for each case.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 target/riscv/insn_trans/trans_rvi.c.inc |  6 ++++--
 target/riscv/op_helper.c                | 24 ++++++++++++++++++++++++
 target/riscv/trace-events               |  6 ++++++
 target/riscv/translate.c                |  2 +-
 4 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index cbd7d5c395..0f5d5def60 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -65,7 +65,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
              */
             gen_helper_raise_sw_check_excep(tcg_env,
                 tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
-                tcg_constant_tl(MISALIGNED_LPAD), tcg_constant_tl(0));
+                tcg_constant_tl(MISALIGNED_LPAD),
+                tcg_constant_tl(ctx->base.pc_next));
             return true;
         }
     }
@@ -81,7 +82,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
         tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, a->imm_cfi20, skip);
         gen_helper_raise_sw_check_excep(tcg_env,
             tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
-            tcg_constant_tl(LABEL_MISMATCH_LPAD), tcg_constant_tl(0));
+            tcg_constant_tl(LABEL_MISMATCH_LPAD),
+            tcg_constant_tl(a->imm_cfi20));
         gen_set_label(skip);
     }
 
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 3b47fb34ea..07990e6589 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -24,6 +24,7 @@
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "exec/helper-proto.h"
+#include "trace.h"
 
 /* Exceptions processing helpers */
 G_NORETURN void riscv_raise_exception(CPURISCVState *env,
@@ -262,6 +263,29 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address)
 void helper_raise_sw_check_excep(CPURISCVState *env, target_ulong swcheck_code,
                                  target_ulong arg1, target_ulong arg2)
 {
+    switch (swcheck_code) {
+    case RISCV_EXCP_SW_CHECK_FCFI_TVAL:
+        switch (arg1) {
+        case MISSING_LPAD:
+            trace_zicfilp_missing_lpad_instr(arg2);
+            break;
+        case MISALIGNED_LPAD:
+            trace_zicfilp_unaligned_lpad_instr(arg2);
+            break;
+        case LABEL_MISMATCH_LPAD:
+            trace_zicfilp_lpad_reg_mismatch(arg2);
+            break;
+        }
+        break;
+    case RISCV_EXCP_SW_CHECK_BCFI_TVAL:
+        trace_zicfiss_sspopchk_reg_mismatch(arg1, arg2);
+        break;
+    default:
+        /* any other value of swcheck_code is asserted */
+        assert(swcheck_code || (swcheck_code == 0));
+        break;
+    }
+
     env->sw_check_code = swcheck_code;
     riscv_raise_exception(env, RISCV_EXCP_SW_CHECK, GETPC());
 }
diff --git a/target/riscv/trace-events b/target/riscv/trace-events
index 49ec4d3b7d..0e8807f0d4 100644
--- a/target/riscv/trace-events
+++ b/target/riscv/trace-events
@@ -9,3 +9,9 @@ pmpaddr_csr_write(uint64_t mhartid, uint32_t addr_index, uint64_t val) "hart %"
 
 mseccfg_csr_read(uint64_t mhartid, uint64_t val) "hart %" PRIu64 ": read mseccfg, val: 0x%" PRIx64
 mseccfg_csr_write(uint64_t mhartid, uint64_t val) "hart %" PRIu64 ": write mseccfg, val: 0x%" PRIx64
+
+# zicfiss/lp
+zicfiss_sspopchk_reg_mismatch(uint64_t ssra, uint64_t rs1) "shadow_stack_ra: 0x%" PRIx64 ", rs1: 0x%" PRIx64
+zicfilp_missing_lpad_instr(uint64_t pc_first) "pc_first: 0x%" PRIx64
+zicfilp_unaligned_lpad_instr(uint64_t pc_next) "pc_next: 0x%" PRIx64
+zicfilp_lpad_reg_mismatch(int lpad_label) "lpad_label: 0x%x"
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 4772191bd8..9ef1f220e0 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1302,7 +1302,7 @@ static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
         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),
-            tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(0));
+            tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(ctx->base.pc_first));
         gen_set_label(l);
         /*
          * Despite the use of gen_exception_illegal(), the rest of
-- 
2.44.0

Re: [PATCH v3 18/20] target/riscv: add trace-hooks for each case of sw-check exception

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> Violations to control flow rules setup by zicfilp and zicfiss lead to
> software check exceptions. To debug and fix such sw check issues in guest
> , add trace-hooks for each case.
> 
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>   target/riscv/insn_trans/trans_rvi.c.inc |  6 ++++--
>   target/riscv/op_helper.c                | 24 ++++++++++++++++++++++++
>   target/riscv/trace-events               |  6 ++++++
>   target/riscv/translate.c                |  2 +-
>   4 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
> index cbd7d5c395..0f5d5def60 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -65,7 +65,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
>                */
>               gen_helper_raise_sw_check_excep(tcg_env,
>                   tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
> -                tcg_constant_tl(MISALIGNED_LPAD), tcg_constant_tl(0));
> +                tcg_constant_tl(MISALIGNED_LPAD),
> +                tcg_constant_tl(ctx->base.pc_next));
>               return true;
>           }
>       }
> @@ -81,7 +82,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
>           tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, a->imm_cfi20, skip);
>           gen_helper_raise_sw_check_excep(tcg_env,
>               tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
> -            tcg_constant_tl(LABEL_MISMATCH_LPAD), tcg_constant_tl(0));
> +            tcg_constant_tl(LABEL_MISMATCH_LPAD),
> +            tcg_constant_tl(a->imm_cfi20));
>           gen_set_label(skip);
>       }
>   
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 3b47fb34ea..07990e6589 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -24,6 +24,7 @@
>   #include "exec/exec-all.h"
>   #include "exec/cpu_ldst.h"
>   #include "exec/helper-proto.h"
> +#include "trace.h"
>   
>   /* Exceptions processing helpers */
>   G_NORETURN void riscv_raise_exception(CPURISCVState *env,
> @@ -262,6 +263,29 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address)
>   void helper_raise_sw_check_excep(CPURISCVState *env, target_ulong swcheck_code,
>                                    target_ulong arg1, target_ulong arg2)
>   {
> +    switch (swcheck_code) {
> +    case RISCV_EXCP_SW_CHECK_FCFI_TVAL:
> +        switch (arg1) {
> +        case MISSING_LPAD:
> +            trace_zicfilp_missing_lpad_instr(arg2);
> +            break;
> +        case MISALIGNED_LPAD:
> +            trace_zicfilp_unaligned_lpad_instr(arg2);
> +            break;
> +        case LABEL_MISMATCH_LPAD:
> +            trace_zicfilp_lpad_reg_mismatch(arg2);
> +            break;
> +        }
> +        break;
> +    case RISCV_EXCP_SW_CHECK_BCFI_TVAL:
> +        trace_zicfiss_sspopchk_reg_mismatch(arg1, arg2);
> +        break;
> +    default:
> +        /* any other value of swcheck_code is asserted */
> +        assert(swcheck_code || (swcheck_code == 0));
> +        break;
> +    }

I think this is a mistake.
Better to have 4 different helpers and eliminate MISSING_LPAD, etc entirely.

> @@ -1302,7 +1302,7 @@ static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
>           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),
> -            tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(0));
> +            tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(ctx->base.pc_first));

You certainly don't need to pass pc_first, since if lp_expected is set that is always 
env->pc in the helper.


r~

Re: [PATCH v3 18/20] target/riscv: add trace-hooks for each case of sw-check exception

[Deepak Gupta]

On Wed, Aug 07, 2024 at 01:27:22PM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>Violations to control flow rules setup by zicfilp and zicfiss lead to
>>software check exceptions. To debug and fix such sw check issues in guest
>>, add trace-hooks for each case.
>>
>>Signed-off-by: Jim Shu <jim.shu@sifive.com>
>>Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>>---
>>  target/riscv/insn_trans/trans_rvi.c.inc |  6 ++++--
>>  target/riscv/op_helper.c                | 24 ++++++++++++++++++++++++
>>  target/riscv/trace-events               |  6 ++++++
>>  target/riscv/translate.c                |  2 +-
>>  4 files changed, 35 insertions(+), 3 deletions(-)
>>
>>diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
>>index cbd7d5c395..0f5d5def60 100644
>>--- a/target/riscv/insn_trans/trans_rvi.c.inc
>>+++ b/target/riscv/insn_trans/trans_rvi.c.inc
>>@@ -65,7 +65,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
>>               */
>>              gen_helper_raise_sw_check_excep(tcg_env,
>>                  tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
>>-                tcg_constant_tl(MISALIGNED_LPAD), tcg_constant_tl(0));
>>+                tcg_constant_tl(MISALIGNED_LPAD),
>>+                tcg_constant_tl(ctx->base.pc_next));
>>              return true;
>>          }
>>      }
>>@@ -81,7 +82,8 @@ static bool trans_lpad(DisasContext *ctx, arg_lpad *a)
>>          tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, a->imm_cfi20, skip);
>>          gen_helper_raise_sw_check_excep(tcg_env,
>>              tcg_constant_tl(RISCV_EXCP_SW_CHECK_FCFI_TVAL),
>>-            tcg_constant_tl(LABEL_MISMATCH_LPAD), tcg_constant_tl(0));
>>+            tcg_constant_tl(LABEL_MISMATCH_LPAD),
>>+            tcg_constant_tl(a->imm_cfi20));
>>          gen_set_label(skip);
>>      }
>>diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>>index 3b47fb34ea..07990e6589 100644
>>--- a/target/riscv/op_helper.c
>>+++ b/target/riscv/op_helper.c
>>@@ -24,6 +24,7 @@
>>  #include "exec/exec-all.h"
>>  #include "exec/cpu_ldst.h"
>>  #include "exec/helper-proto.h"
>>+#include "trace.h"
>>  /* Exceptions processing helpers */
>>  G_NORETURN void riscv_raise_exception(CPURISCVState *env,
>>@@ -262,6 +263,29 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address)
>>  void helper_raise_sw_check_excep(CPURISCVState *env, target_ulong swcheck_code,
>>                                   target_ulong arg1, target_ulong arg2)
>>  {
>>+    switch (swcheck_code) {
>>+    case RISCV_EXCP_SW_CHECK_FCFI_TVAL:
>>+        switch (arg1) {
>>+        case MISSING_LPAD:
>>+            trace_zicfilp_missing_lpad_instr(arg2);
>>+            break;
>>+        case MISALIGNED_LPAD:
>>+            trace_zicfilp_unaligned_lpad_instr(arg2);
>>+            break;
>>+        case LABEL_MISMATCH_LPAD:
>>+            trace_zicfilp_lpad_reg_mismatch(arg2);
>>+            break;
>>+        }
>>+        break;
>>+    case RISCV_EXCP_SW_CHECK_BCFI_TVAL:
>>+        trace_zicfiss_sspopchk_reg_mismatch(arg1, arg2);
>>+        break;
>>+    default:
>>+        /* any other value of swcheck_code is asserted */
>>+        assert(swcheck_code || (swcheck_code == 0));
>>+        break;
>>+    }
>
>I think this is a mistake.
>Better to have 4 different helpers and eliminate MISSING_LPAD, etc entirely.

Honestly for raising sw check exception, helper is not needed.

I added a helper for raising sw check exception, because needed to add trace
capability on violations. Helps locate issues in cfi compiled binaries faster.

If you prefer 4 different helpers for 4 different trace needs, I will do that.

>
>>@@ -1302,7 +1302,7 @@ static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
>>          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),
>>-            tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(0));
>>+            tcg_constant_tl(MISSING_LPAD), tcg_constant_tl(ctx->base.pc_first));
>
>You certainly don't need to pass pc_first, since if lp_expected is set 
>that is always env->pc in the helper.

noted.

>
>
>r~

[PATCH v3 19/20] linux-user: permit RISC-V CFI dynamic entry in VDSO

[Deepak Gupta]

RISC-V CFI use new processor-specific dynamic entry in ELF. Permit it in
VDSO post-processing script.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 linux-user/gen-vdso-elfn.c.inc | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/linux-user/gen-vdso-elfn.c.inc b/linux-user/gen-vdso-elfn.c.inc
index 95856eb839..59c818eb11 100644
--- a/linux-user/gen-vdso-elfn.c.inc
+++ b/linux-user/gen-vdso-elfn.c.inc
@@ -273,6 +273,13 @@ static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
                 errors++;
                 break;
 
+            case PT_LOPROC + 2:
+                /* RISCV_ZICFILP_PLT: for RISC-V zicfilp extension */
+                if (ehdr->e_machine == EM_RISCV) {
+                    break;
+                }
+                goto do_default;
+
             case PT_LOPROC + 3:
                 if (ehdr->e_machine == EM_PPC64) {
                     break;  /* DT_PPC64_OPT: integer bitmask */
-- 
2.44.0

Re: [PATCH v3 19/20] linux-user: permit RISC-V CFI dynamic entry in VDSO

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> RISC-V CFI use new processor-specific dynamic entry in ELF. Permit it in
> VDSO post-processing script.
> 
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>   linux-user/gen-vdso-elfn.c.inc | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/linux-user/gen-vdso-elfn.c.inc b/linux-user/gen-vdso-elfn.c.inc
> index 95856eb839..59c818eb11 100644
> --- a/linux-user/gen-vdso-elfn.c.inc
> +++ b/linux-user/gen-vdso-elfn.c.inc
> @@ -273,6 +273,13 @@ static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
>                   errors++;
>                   break;
>   
> +            case PT_LOPROC + 2:
> +                /* RISCV_ZICFILP_PLT: for RISC-V zicfilp extension */
> +                if (ehdr->e_machine == EM_RISCV) {
> +                    break;
> +                }
> +                goto do_default;

Documentation?  This symbol does not appear in either llvm or binutils sources.  I presume 
this is on a development branch somewhere.

The comment is poor.  Notice:

>               case PT_LOPROC + 3:
>                   if (ehdr->e_machine == EM_PPC64) {
>                       break;  /* DT_PPC64_OPT: integer bitmask */

the ppc64 comment describes the data payload.
As do the other comments beforehand.


r~

Re: [PATCH v3 19/20] linux-user: permit RISC-V CFI dynamic entry in VDSO

[Deepak Gupta]

On Wed, Aug 07, 2024 at 01:36:34PM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>RISC-V CFI use new processor-specific dynamic entry in ELF. Permit it in
>>VDSO post-processing script.
>>
>>Signed-off-by: Jim Shu <jim.shu@sifive.com>
>>Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>>---
>>  linux-user/gen-vdso-elfn.c.inc | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>>diff --git a/linux-user/gen-vdso-elfn.c.inc b/linux-user/gen-vdso-elfn.c.inc
>>index 95856eb839..59c818eb11 100644
>>--- a/linux-user/gen-vdso-elfn.c.inc
>>+++ b/linux-user/gen-vdso-elfn.c.inc
>>@@ -273,6 +273,13 @@ static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
>>                  errors++;
>>                  break;
>>+            case PT_LOPROC + 2:
>>+                /* RISCV_ZICFILP_PLT: for RISC-V zicfilp extension */
>>+                if (ehdr->e_machine == EM_RISCV) {
>>+                    break;
>>+                }
>>+                goto do_default;
>
>Documentation?  This symbol does not appear in either llvm or binutils 
>sources.  I presume this is on a development branch somewhere.
>
>The comment is poor.  Notice:

Noted. will do better.
>
>>              case PT_LOPROC + 3:
>>                  if (ehdr->e_machine == EM_PPC64) {
>>                      break;  /* DT_PPC64_OPT: integer bitmask */
>
>the ppc64 comment describes the data payload.
>As do the other comments beforehand.
>
>
>r~

[PATCH v3 20/20] linux-user: Add RISC-V zicfilp support in VDSO

[Deepak Gupta]

Add zicfilp support in VDSO. VDSO functions need lpad instruction
so that userspace could call this function when landing pad extension is
enabled. This solution only works when toolchain always use landing pad
label 1.

Otherwise, If extension is not enabled, lpad instructions will be lui
instructions with rd=x0 (which is nop). Prebuilt VDSO is still
compatible with RISC-V core w/o zicfilp extension.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 linux-user/riscv/vdso-64.so | Bin 3944 -> 4128 bytes
 linux-user/riscv/vdso.S     |  50 ++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/linux-user/riscv/vdso-64.so b/linux-user/riscv/vdso-64.so
index ae49f5b043b5941b9d304a056c2b50c185f413b0..cd7f2fa7bdb811af6be2dcc3fb9601b66e3d1c81 100755
GIT binary patch
delta 1345
zcmah}O=uHA6rRa8*`3W#v-vSWgUvy(QL#}%mV&j76iPuQBHAj2(ng|zmM)E!P>`g>
ziy+eQP!K%yD2kv2Jb7q5_;(V#<f8T>2zrPIp$E0T*_|nLFFu%g^L_K)%k2BfxBcts
zwSKzU%uIMvDl|QN=xp<WSxBkG7O6?t!5&mSxH{CqZapIS5in@ND0&^M9SwtYn2kCl
z8HHvbTIYee+1S|&oZsNl6@EhD=NK-I`TfVcovANRA6MVpdHd;BIWn<tz;C}Zg!f#o
zJIeOs$1M@)*Wc|0j<Y-<ig*^WdPuKL`0SmKqyiq##X<Z^OE9+jz3uoXh2tNAc{aFo
z1twr<QCRm}!4()%@Eu+GDUKoG>4}g4*$}@d(n<~qepB*LP!3)Siz)<!cU)L~kXC{}
zEqLOxKPmXG%f8cUD^<!G_?jY`yo4d|;kbMX*GF*m<L!yoO|MGplL+N?0uP|A=~d*e
zHVAQWSlx}&F5K8<AH_U!-zdg#{GVyuzc7Q_Vx?MIB6I?e-o>SSu5ui{`}Ri4l{qVG
z<))V_rE;ZO#UoHP&Zd{==Wom%v$EK`eUMXAv;*hV1Wm$<8dtCYs1tO{Mm~~-=ZGxa
z<BEpgVQ6uMk)*A4Qbe7gH5*}xprDP>jpj-e9%`|fX?zbQKeuJaBedlj?wn7H+zXnl
z+6N3On@wEY6ZY=Tcmf7X)L-B&?+<q+-wWQ|H=hOXuJ8}RyE}+CAdkP(XK2SI=J0*Q
z-CCqns+~DMS-yO9fgGs8S6}+Sm1w<YuP7ab3>M^(KWxa1Nj(DZ`~!MYOa>phK%U8T
zbfFM1nH*fK8zMQjS!g4|p|!;V8Z`BtS@y!IU|yFKn)JeIFwbQ2i_i|5tR_lP0~#_7
pnM$drU_4|p`G+?Pw?igvKsz+7r*-ESGZggRJRA2r@IJ6$-#>;G!0Z43

delta 1236
zcmZux&1(};5TBP#+z*pYV+<*_)IAj1YN_?B+bY<^rnX2aRgfZHEGbA2QZyn~#MXa6
z69zo>Q1McbLL?%ocu2)V4jv*F!CQ|xnnMLet@AeXqAU))otfYJy|4M$HK*Q{?-jj;
zzFD!24I_@VKv0a~RwP*{I_d3w;EB@E*7O6Uf;7sa>HGB{<AWFz$(R#rvRWEP#3-gj
z=kh_C&}d9dUxAJB?DWLN7i-UF5AA;Y{Ap&b@ba}>XUh-Cou=~6m1b2gB-#DFx9A!2
zLL__`q}b;tKwVy%#A+&d0)Qt2<9$E(n(OP#|HVGj;Vb(!Jnn^O9`m|=HV73ypSJ_~
z<O2|fJRcb5i6e}!D;#fWJy$=lXD}<ltX0Kge2VdkkAIHwS3Z8Z)X;Lmd_cFEA<8=5
z3>{6VtH|v2)99wz{?bNB7jqeM)ifG;D@Xo~6$*{frvJ5_f9#bOCr+W3+&Ha4qi9He
z`aFGZFXXa!K@5`_!U4;{c`Jrnfi7ItJ4G3v>4^@ll@B7dM5F9h@S~p4LQq9vBn42^
z6PgYw(n%q6kkCx1d)fjA=g8j=lUShHyjQ?)jZ>c0vk;|y1vK_lb*f|98Q<a9Dg1<I
z(5|Y4cw(IS{)}HEJVyHiUb5rMGWY&0-6zKXYT_~D{_L$X?yrs_<E<JZU&?zLa(}9t
z4i8YNglu-<^o7ju<$*=$zE^r^y%V@%J9s_Z7E|F+dJrFlk6Efc>H&Nc9x~NiEBHO^
znyS~TI1+KqRtw@1d8*G+xEXP+8h24Gh(97jmTIbc5YN~{ri!eCOSWrHa-1h|({^L3
qZ<NlUh`Ofw^Ne9S>WX$;ijJCP(|ap?q2JVD+=;fE1#ar668QsJ{<Wt7

diff --git a/linux-user/riscv/vdso.S b/linux-user/riscv/vdso.S
index c37275233a..d0817c58ce 100644
--- a/linux-user/riscv/vdso.S
+++ b/linux-user/riscv/vdso.S
@@ -14,6 +14,52 @@
 #endif
 #include "vdso-asmoffset.h"
 
+/* GNU_PROPERTY_RISCV64_* macros from elf.h for use in asm code.  */
+#define FEATURE_1_AND 0xc0000000
+
+#define GNU_PROPERTY(type, value)	\
+  .section .note.gnu.property, "a";	\
+  .p2align 3;                       \
+  .word 4;                          \
+  .word 16;                         \
+  .word 5;                          \
+  .asciz "GNU";                     \
+  .word type;                       \
+  .word 4;                          \
+  .word value;                      \
+  .word 0;                          \
+  .text
+
+/* Add GNU property note with the supported features to all asm code
+   where sysdep.h is included.  */
+#undef __VALUE_FOR_FEATURE_1_AND
+#if defined (__riscv_zicfilp) || defined (__riscv_zicfiss)
+#  if defined (__riscv_zicfilp)
+#    if defined (__riscv_zicfiss)
+#      define __VALUE_FOR_FEATURE_1_AND 0x3
+#    else
+#      define __VALUE_FOR_FEATURE_1_AND 0x1
+#    endif
+#  else
+#    if defined (__riscv_zicfiss)
+#      define __VALUE_FOR_FEATURE_1_AND 0x2
+#    else
+#      error "What?"
+#    endif
+#  endif
+#endif
+
+#if defined (__VALUE_FOR_FEATURE_1_AND)
+GNU_PROPERTY (FEATURE_1_AND, __VALUE_FOR_FEATURE_1_AND)
+#endif
+#undef __VALUE_FOR_FEATURE_1_AND
+
+#ifdef __riscv_zicfilp
+# define LPAD       lpad 1
+#else
+# define LPAD
+#endif
+
 	.text
 
 .macro endf name
@@ -29,6 +75,7 @@
 
 .macro vdso_syscall name, nr
 \name:
+	LPAD
 	raw_syscall \nr
 	ret
 endf	\name
@@ -36,6 +83,7 @@ endf	\name
 
 __vdso_gettimeofday:
 	.cfi_startproc
+	LPAD
 #ifdef __NR_gettimeofday
 	raw_syscall __NR_gettimeofday
 	ret
@@ -86,6 +134,7 @@ vdso_syscall __vdso_getcpu, __NR_getcpu
 
 __vdso_flush_icache:
 	/* qemu does not need to flush the icache */
+	LPAD
 	li	a0, 0
 	ret
 endf __vdso_flush_icache
@@ -181,6 +230,7 @@ endf __vdso_flush_icache
 	nop
 
 __vdso_rt_sigreturn:
+	LPAD
 	raw_syscall __NR_rt_sigreturn
 endf __vdso_rt_sigreturn
 
-- 
2.44.0

Re: [PATCH v3 20/20] linux-user: Add RISC-V zicfilp support in VDSO

[Richard Henderson]

On 8/7/24 10:06, Deepak Gupta wrote:
> Add zicfilp support in VDSO. VDSO functions need lpad instruction
> so that userspace could call this function when landing pad extension is
> enabled. This solution only works when toolchain always use landing pad
> label 1.

Well, no, the lpad insns *could* use imm=0.
Why would the toolchain always use label 1?

Much more explanation is required here.

> +/* GNU_PROPERTY_RISCV64_* macros from elf.h for use in asm code.  */
> +#define FEATURE_1_AND 0xc0000000
> +
> +#define GNU_PROPERTY(type, value)	\
> +  .section .note.gnu.property, "a";	\
> +  .p2align 3;                       \
> +  .word 4;                          \
> +  .word 16;                         \
> +  .word 5;                          \
> +  .asciz "GNU";                     \
> +  .word type;                       \
> +  .word 4;                          \
> +  .word value;                      \
> +  .word 0;                          \
> +  .text
> +
> +/* Add GNU property note with the supported features to all asm code
> +   where sysdep.h is included.  */
> +#undef __VALUE_FOR_FEATURE_1_AND
> +#if defined (__riscv_zicfilp) || defined (__riscv_zicfiss)
> +#  if defined (__riscv_zicfilp)
> +#    if defined (__riscv_zicfiss)

Why are you checking __riscv_* symbols, for when the toolchain
has these features enabled on the command-line?

This is the kind of feature you want enabled always.

> +#ifdef __riscv_zicfilp
> +# define LPAD       lpad 1
> +#else
> +# define LPAD
> +#endif

Here, especially, you should be using ".insn", not omitting the lpad insn.


r~

Re: [PATCH v3 20/20] linux-user: Add RISC-V zicfilp support in VDSO

[Deepak Gupta]

On Wed, Aug 07, 2024 at 01:41:37PM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>Add zicfilp support in VDSO. VDSO functions need lpad instruction
>>so that userspace could call this function when landing pad extension is
>>enabled. This solution only works when toolchain always use landing pad
>>label 1.
>
>Well, no, the lpad insns *could* use imm=0.
>Why would the toolchain always use label 1?
>
>Much more explanation is required here.

Sorry this is amiss on my end. label=1 is kind of experimental, dev enabling
to ensure that label checkings are working. Eventually label scheme would be
hash (20bit truncates) based label scheme. Hash calculated over function
prototype.

This is again chicken and an egg problem. Things have to make it to upstream
in toolchain and libc for this scheme to be usable.

For now, I am thinking of doing `lpad 0` (as you also hinted) for vDSO.
I hope that's fine.

>
>>+/* GNU_PROPERTY_RISCV64_* macros from elf.h for use in asm code.  */
>>+#define FEATURE_1_AND 0xc0000000
>>+
>>+#define GNU_PROPERTY(type, value)	\
>>+  .section .note.gnu.property, "a";	\
>>+  .p2align 3;                       \
>>+  .word 4;                          \
>>+  .word 16;                         \
>>+  .word 5;                          \
>>+  .asciz "GNU";                     \
>>+  .word type;                       \
>>+  .word 4;                          \
>>+  .word value;                      \
>>+  .word 0;                          \
>>+  .text
>>+
>>+/* Add GNU property note with the supported features to all asm code
>>+   where sysdep.h is included.  */
>>+#undef __VALUE_FOR_FEATURE_1_AND
>>+#if defined (__riscv_zicfilp) || defined (__riscv_zicfiss)
>>+#  if defined (__riscv_zicfilp)
>>+#    if defined (__riscv_zicfiss)
>
>Why are you checking __riscv_* symbols, for when the toolchain
>has these features enabled on the command-line?
>

Yes, you're right.

>This is the kind of feature you want enabled always.

Yes, you're right here as well.
It should be compiled into vDSO unconditionally.

Will do that.

>
>>+#ifdef __riscv_zicfilp
>>+# define LPAD       lpad 1
>>+#else
>>+# define LPAD
>>+#endif
>
>Here, especially, you should be using ".insn", not omitting the lpad insn.
>
>
>r~
>