On Thu, Oct 17, 2024 at 5:45 PM Clément Léger <cle...@rivosinc.com> wrote:
>
>
>
> On 17/10/2024 06:29, Alistair Francis wrote:
> > 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
>
> s/DTE/SDT
>
> >
> > Why not just clear it at sret/mret? Instead of having this assumption
>
> It has been cleared but since registers were swapped to enter virt mode,
> hypervisor SDT value is stored in mstatus_hs rather than mstatus. So I
> could have wrote it this way:
>
> + } else if (env->virt_enabled) {
> + /* VS -> HS */
> + sdt = (env->mstatus_hs & MSTATUS_SDT);
>
> Since this is always 0 better assume it is 0 (but should be sdt = 0
> instead of dte = 0). But if you prefer using mstatus_hs for clarity, I
> can use that of course.
We should use the register directly. That way if it is accidently not
cleared it's easier to catch and it makes the code easier to read
Alistair
>
> Clément
>
> >
> > Alistair
> >
> >> value of mstatus_hs, just assume it is false.
> >>
> >> Thanks,
> >>
> >> Clément
> >>
> >>>
> >>> Alistair
> >>
>
