On Mon, Oct 14, 2024 at 5:43 PM Clément Léger <cle...@rivosinc.com> wrote:
>
>
>
> On 11/10/2024 05:22, Alistair Francis wrote:
> > On Wed, Sep 25, 2024 at 9:59 PM Clément Léger <cle...@rivosinc.com> wrote:
> >>
> >> When the Ssdbltrp ISA extension is enabled, if a trap happens in S-mode
> >> while SSTATUS.SDT isn't cleared, generate a double trap exception to
> >> M-mode.
> >>
> >> Signed-off-by: Clément Léger <cle...@rivosinc.com>
> >> ---
> >> target/riscv/cpu.c | 2 +-
> >> target/riscv/cpu_bits.h | 1 +
> >> target/riscv/cpu_helper.c | 47 ++++++++++++++++++++++++++++++++++-----
> >> 3 files changed, 43 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index cf06cd741a..65347ccd5a 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -284,7 +284,7 @@ static const char * const riscv_excp_names[] = {
> >> "load_page_fault",
> >> "reserved",
> >> "store_page_fault",
> >> - "reserved",
> >> + "double_trap",
> >> "reserved",
> >> "reserved",
> >> "reserved",
> >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> >> index 3a5588d4df..5557a86348 100644
> >> --- a/target/riscv/cpu_bits.h
> >> +++ b/target/riscv/cpu_bits.h
> >> @@ -699,6 +699,7 @@ typedef enum RISCVException {
> >> RISCV_EXCP_INST_PAGE_FAULT = 0xc, /* since: priv-1.10.0 */
> >> RISCV_EXCP_LOAD_PAGE_FAULT = 0xd, /* since: priv-1.10.0 */
> >> RISCV_EXCP_STORE_PAGE_FAULT = 0xf, /* since: priv-1.10.0 */
> >> + RISCV_EXCP_DOUBLE_TRAP = 0x10,
> >> RISCV_EXCP_SW_CHECK = 0x12, /* since: priv-1.13.0 */
> >> RISCV_EXCP_HW_ERR = 0x13, /* since: priv-1.13.0 */
> >> RISCV_EXCP_INST_GUEST_PAGE_FAULT = 0x14,
> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> index 395d8235ce..69da3c3384 100644
> >> --- a/target/riscv/cpu_helper.c
> >> +++ b/target/riscv/cpu_helper.c
> >> @@ -575,7 +575,9 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env)
> >> mstatus_mask |= MSTATUS_FS;
> >> }
> >> bool current_virt = env->virt_enabled;
> >> -
> >> + if (riscv_env_smode_dbltrp_enabled(env, current_virt)) {
> >> + mstatus_mask |= MSTATUS_SDT;
> >> + }
> >> g_assert(riscv_has_ext(env, RVH));
> >>
> >> if (current_virt) {
> >> @@ -1707,6 +1709,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >> CPURISCVState *env = &cpu->env;
> >> bool virt = env->virt_enabled;
> >> bool write_gva = false;
> >> + bool vsmode_exc;
> >> uint64_t s;
> >> int mode;
> >>
> >> @@ -1721,6 +1724,8 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >> !(env->mip & (1 << cause));
> >> bool vs_injected = env->hvip & (1 << cause) & env->hvien &&
> >> !(env->mip & (1 << cause));
> >> + bool smode_double_trap = false;
> >> + uint64_t hdeleg = async ? env->hideleg : env->hedeleg;
> >> target_ulong tval = 0;
> >> target_ulong tinst = 0;
> >> target_ulong htval = 0;
> >> @@ -1837,13 +1842,35 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >> !async &&
> >> mode == PRV_M;
> >>
> >> + vsmode_exc = env->virt_enabled && (((hdeleg >> cause) & 1) ||
> >> vs_injected);
> >> + /*
> >> + * Check double trap condition only if already in S-mode and targeting
> >> + * S-mode
> >> + */
> >> + if (cpu->cfg.ext_ssdbltrp && env->priv == PRV_S && mode == PRV_S) {
> >> + bool dte = (env->menvcfg & MENVCFG_DTE) != 0;
> >> + bool sdt = (env->mstatus & MSTATUS_SDT) != 0;
> >> + /* In VS or HS */
> >> + if (riscv_has_ext(env, RVH)) {
> >> + if (vsmode_exc) {
> >> + /* VS -> VS */
> >> + /* Stay in VS mode, use henvcfg instead of menvcfg*/
> >> + dte = (env->henvcfg & HENVCFG_DTE) != 0;
> >> + } else if (env->virt_enabled) {
> >> + /* VS -> HS */
> >> + dte = false;
> >
> > I don't follow why this is false
>
> Hi Alistair,
>
> It's indeed probably lacking some comments here. The rationale is that
> if you are trapping from VS to HS, then at some point, you returned to
> VS using a sret/mret and thus cleared DTE, so rather than checking the
Why not just clear it at sret/mret? Instead of having this assumption
Alistair
> value of mstatus_hs, just assume it is false.
>
> Thanks,
>
> Clément
>
> >
> > Alistair
>
