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

[Deepak Gupta <debug@rivosinc.com>]

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~