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~