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.0On 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~
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~
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~
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~
© 2016 - 2024 Red Hat, Inc.