| Message ID | 20240826152949.294506-4-debug@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv support for control flow integrity extensions | expand |
On 8/27/24 01:29, Deepak Gupta wrote: > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 8e1f05e5b1..083d405516 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 ... > @@ -226,6 +226,7 @@ struct CPUArchState { > bool elp; > #ifdef CONFIG_USER_ONLY > uint32_t elf_flags; > + bool ufcfien; > #endif Thinking about this more, I think adding separate controls for user-only is a bad precedent to set. You said you are adding these because senvcfg/menvcfg are ifdefed: well, that should be the thing that we fix. The only real user of *envcfg that I see so far is check_zicbo_envcfg, which does not use the same switch statement as this: > + 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(); > + } I think your function should look more like check_zicbo_envcfg: (1) PRV_U may be either U or VU, and different tests apply; (2) M-mode disable means that no lower level may be enabled. It would be nice if you could generalize check_zicbo_envcfg to apply to your new use as well. Perhaps some tri-state function to say "enabled", "disabled", "virtual disabled". r~
On Tue, Aug 27, 2024 at 10:33:04AM +1000, Richard Henderson wrote: >On 8/27/24 01:29, Deepak Gupta wrote: >>diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>index 8e1f05e5b1..083d405516 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 >... >>@@ -226,6 +226,7 @@ struct CPUArchState { >> bool elp; >> #ifdef CONFIG_USER_ONLY >> uint32_t elf_flags; >>+ bool ufcfien; >> #endif > >Thinking about this more, I think adding separate controls for >user-only is a bad precedent to set. You said you are adding these >because senvcfg/menvcfg are ifdefed: well, that should be the thing >that we fix. If a binary is compiled with zicbo, it'll have those instructions in user binary. I am not sure how those binaries will run on qemu-user (or if it was tested with qemu-user) In case of zicfilp + zicfiss we are runnning binaries compiled with zicfilp and zicfiss with qemu-user and qemu-system both. Use of qemu-user to generate traces for feeding into CPU modeling is quite useful. Thus mechanism to track if landing pad and shadow stack is enabled for current user task (in case of qemu-user) is very useful. senvcfg and menvcfg belong to S and M state and don't actually mean anything for qemu-user. However if that's how it is for arm as well (i.e. exposing system state for qemu-user), then probably there is precedent. But it looks like a much larger exercise to me. > >The only real user of *envcfg that I see so far is check_zicbo_envcfg, >which does not use the same switch statement as this: > >>+ 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(); >>+ } > >I think your function should look more like check_zicbo_envcfg: (1) >PRV_U may be either U or VU, and different tests apply; (2) M-mode >disable means that no lower level may be enabled. In case of landing pad, (2) doesn't hold true. Each mode can independently enable landing pad for next lower mode even if it wasn't enabled for current mode. Reason being that you can have a firmware which is still not landing pad enabled and you would want to avoid landing pad related faults in M mode but still want to make sure S mode and U mode can enable landing pad for themselves. Same goes with respect to S mode, S mode can have its landing pad disabled while it can enable landing pad for U mode. Although logic for shadow stack enable is same as zicbo. Shadow stack enable requires target software to opt-in by having instructions compiled in and doesn't impact behavior of existing instructions. > >It would be nice if you could generalize check_zicbo_envcfg to apply >to your new use as well. Perhaps some tri-state function to say >"enabled", "disabled", "virtual disabled". So while shadow stack and zicbo are similar in terms of enabling. Landing pad enable differs. You still want me to generalize *envcfg flow? > > >r~
On 8/27/24 10:52, Deepak Gupta wrote: > senvcfg and menvcfg belong to S and M state and don't actually mean anything > for qemu-user. Certainly they do, in that you obviously have arch_prctl calls that adjust them. The fact that you've modeled those so far as separate variables is part of what is confusing. > However if that's how it is for arm as well (i.e. exposing system > state for qemu-user), then probably there is precedent. I think having special paths for qemu-user in target/arch/ is maintenance burden, and should be avoided if possible. The best way to reason with user-only is to treat it just like the full machine, and set the fields just like the real bios and kernel would do. > So while shadow stack and zicbo are similar in terms of enabling. Landing pad > enable differs. > You still want me to generalize *envcfg flow? Hmm. I'll leave that to riscv maintainers; perhaps a third user is required to show generality... r~
On Tue, Aug 27, 2024 at 11:36 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 8/27/24 10:52, Deepak Gupta wrote: > > senvcfg and menvcfg belong to S and M state and don't actually mean anything > > for qemu-user. > > Certainly they do, in that you obviously have arch_prctl calls that adjust them. The fact > that you've modeled those so far as separate variables is part of what is confusing. > > > However if that's how it is for arm as well (i.e. exposing system > > state for qemu-user), then probably there is precedent. > > I think having special paths for qemu-user in target/arch/ is maintenance burden, and > should be avoided if possible. The best way to reason with user-only is to treat it just > like the full machine, and set the fields just like the real bios and kernel would do. I agree about avoiding adding these special variables. Can't we just use the extension check to enable it for userspace? Exposing the *envcfg CSRs to userspace seems tricky as everything is currently built with the S/M CSRs removed from user builds. Alistair > > > So while shadow stack and zicbo are similar in terms of enabling. Landing pad > > enable differs. > > You still want me to generalize *envcfg flow? > > Hmm. I'll leave that to riscv maintainers; perhaps a third user is required to show > generality... > > > r~ >
On 8/27/24 13:53, Alistair Francis wrote: > Exposing the *envcfg CSRs to userspace seems tricky as everything is > currently built with the S/M CSRs removed from user builds. It is as simple as moving them out of ifdefs, then initializing them as needed in reset for CONFIG_USER_ONLY. That's what we do for Arm. r~
On Tue, Aug 27, 2024 at 1:58 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 8/27/24 13:53, Alistair Francis wrote: > > Exposing the *envcfg CSRs to userspace seems tricky as everything is > > currently built with the S/M CSRs removed from user builds. > > It is as simple as moving them out of ifdefs, then initializing them as needed in reset > for CONFIG_USER_ONLY. That's what we do for Arm. Is that really better though? Then we have these CSRs that are included in the build, so people can write code that checks the CSRs, but they are never actually changed. I guess it simplified the CONFIG_USER_ONLY checks, which is handy and your original point. But it seems like it is clunky that we have these CSRs that are kind of fake Alistair > > > r~
On 8/27/24 14:03, Alistair Francis wrote: > On Tue, Aug 27, 2024 at 1:58 PM Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 8/27/24 13:53, Alistair Francis wrote: >>> Exposing the *envcfg CSRs to userspace seems tricky as everything is >>> currently built with the S/M CSRs removed from user builds. >> >> It is as simple as moving them out of ifdefs, then initializing them as needed in reset >> for CONFIG_USER_ONLY. That's what we do for Arm. > > Is that really better though? > > Then we have these CSRs that are included in the build, so people can > write code that checks the CSRs, but they are never actually changed. > > I guess it simplified the CONFIG_USER_ONLY checks, which is handy and > your original point. But it seems like it is clunky that we have these > CSRs that are kind of fake They're not fake. They're a reflection of how the system-mode kernel configures the system-mode user environment. The u[bf]cfien variables introduced in this patch set are an indication of this. Within this patch set they're always false. But the intent is to implement the (proposed) prctl syscalls that will set them to true (on hold waiting for kernel abi to land upstream, but were present in an earlier patch set revision.) The correct implementation of those syscalls, in my opinion, is to set the corresponding [ms]envcfg bits. Just as linux-user/aarch64/target_prctl.h does for SVE et al. r~
On Tue, Aug 27, 2024 at 2:29 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 8/27/24 14:03, Alistair Francis wrote: > > On Tue, Aug 27, 2024 at 1:58 PM Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> > >> On 8/27/24 13:53, Alistair Francis wrote: > >>> Exposing the *envcfg CSRs to userspace seems tricky as everything is > >>> currently built with the S/M CSRs removed from user builds. > >> > >> It is as simple as moving them out of ifdefs, then initializing them as needed in reset > >> for CONFIG_USER_ONLY. That's what we do for Arm. > > > > Is that really better though? > > > > Then we have these CSRs that are included in the build, so people can > > write code that checks the CSRs, but they are never actually changed. > > > > I guess it simplified the CONFIG_USER_ONLY checks, which is handy and > > your original point. But it seems like it is clunky that we have these > > CSRs that are kind of fake > > They're not fake. They're a reflection of how the system-mode kernel configures the > system-mode user environment. > > The u[bf]cfien variables introduced in this patch set are an indication of this. Within > this patch set they're always false. But the intent is to implement the (proposed) prctl > syscalls that will set them to true (on hold waiting for kernel abi to land upstream, but > were present in an earlier patch set revision.) > > The correct implementation of those syscalls, in my opinion, is to set the corresponding > [ms]envcfg bits. Just as linux-user/aarch64/target_prctl.h does for SVE et al. Yeah, that actually does sound better. That sounds like the way to go then Alistair
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 8e1f05e5b1..083d405516 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 f966c36a31..7be0fa30f7 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -226,6 +226,7 @@ struct CPUArchState { bool 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..12484ca7d2 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,34 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch) #endif } +bool cpu_get_fcfien(CPURISCVState *env) +{ + /* no cfi extension, return false */ + if (!env_archcpu(env)->cfg.ext_zicfilp) { + return false; + } +#ifdef CONFIG_USER_ONLY + return env->ufcfien; +#else + 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 +575,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 +1792,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 +1845,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 +1881,13 @@ void riscv_cpu_do_interrupt(CPUState *cs) riscv_cpu_set_mode(env, PRV_M); } + /* + * Interrupt/exception/trap delivery is asynchronous event and as per + * zicfilp spec CPU should clear up the ELP state. No harm in clearing + * unconditionally. + */ + env->elp = false; + /* * 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..5848aaf437 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; }