1. Patchew
  2. QEMU
  3. [PATCH v2 0/8] target/riscv: Add support for Smdbltrp and Ssdbltrp extensions
  4. View patch

[PATCH v2 3/8] target/riscv: Implement Ssdbltrp exception handling

Clément Léger posted 8 patches 3 weeks, 6 days ago
There is a newer version of this series
[PATCH v2 3/8] target/riscv: Implement Ssdbltrp exception handling
Posted by Clément Léger 3 weeks, 6 days ago 
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 <cleger@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;
+            }
+        }
+        smode_double_trap = dte && sdt;
+        if (smode_double_trap) {
+            mode = PRV_M;
+        }
+    }
+
     if (mode == PRV_S) {
         /* handle the trap in S-mode */
         if (riscv_has_ext(env, RVH)) {
-            uint64_t hdeleg = async ? env->hideleg : env->hedeleg;
-
-            if (env->virt_enabled &&
-                (((hdeleg >> cause) & 1) || vs_injected)) {
+            if (vsmode_exc) {
                 /* Trap to VS mode */
                 /*
                  * See if we need to adjust cause. Yes if its VS mode interrupt
@@ -1876,6 +1903,9 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         s = set_field(s, MSTATUS_SPIE, get_field(s, MSTATUS_SIE));
         s = set_field(s, MSTATUS_SPP, env->priv);
         s = set_field(s, MSTATUS_SIE, 0);
+        if (riscv_env_smode_dbltrp_enabled(env, virt)) {
+            s = set_field(s, MSTATUS_SDT, 1);
+        }
         env->mstatus = s;
         env->scause = cause | ((target_ulong)async << (TARGET_LONG_BITS - 1));
         env->sepc = env->pc;
@@ -1909,9 +1939,14 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         s = set_field(s, MSTATUS_MIE, 0);
         env->mstatus = s;
         env->mcause = cause | ~(((target_ulong)-1) >> async);
+        if (smode_double_trap) {
+            env->mtval2 = env->mcause;
+            env->mcause = RISCV_EXCP_DOUBLE_TRAP;
+        } else {
+            env->mtval2 = mtval2;
+        }
         env->mepc = env->pc;
         env->mtval = tval;
-        env->mtval2 = mtval2;
         env->mtinst = tinst;
 
         if (cpu->cfg.ext_smrnmi && nmi_execp) {
-- 
2.45.2
Re: [PATCH v2 3/8] target/riscv: Implement Ssdbltrp exception handling
Posted by Alistair Francis 1 week, 4 days ago 
On Wed, Sep 25, 2024 at 9:59 PM Clément Léger <cleger@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 <cleger@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

Alistair
Re: [PATCH v2 3/8] target/riscv: Implement Ssdbltrp exception handling
Posted by Clément Léger 1 week, 1 day ago 

On 11/10/2024 05:22, Alistair Francis wrote:
> On Wed, Sep 25, 2024 at 9:59 PM Clément Léger <cleger@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 <cleger@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
value of mstatus_hs, just assume it is false.

Thanks,

Clément

> 
> Alistair
Re: [PATCH v2 3/8] target/riscv: Implement Ssdbltrp exception handling
Posted by Alistair Francis 5 days, 17 hours ago 
On Mon, Oct 14, 2024 at 5:43 PM Clément Léger <cleger@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 <cleger@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 <cleger@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
>
Re: [PATCH v2 3/8] target/riscv: Implement Ssdbltrp exception handling
Posted by Clément Léger 5 days, 14 hours ago 

On 17/10/2024 06:29, Alistair Francis wrote:
> On Mon, Oct 14, 2024 at 5:43 PM Clément Léger <cleger@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 <cleger@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 <cleger@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.

Clément

> 
> Alistair
> 
>> value of mstatus_hs, just assume it is false.
>>
>> Thanks,
>>
>> Clément
>>
>>>
>>> Alistair
>>
Re: [PATCH v2 3/8] target/riscv: Implement Ssdbltrp exception handling
Posted by Alistair Francis 4 days, 19 hours ago 
On Thu, Oct 17, 2024 at 5:45 PM Clément Léger <cleger@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 <cleger@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 <cleger@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 <cleger@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
> >>
>
Re: [PATCH v2 3/8] target/riscv: Implement Ssdbltrp exception handling
Posted by Clément Léger 4 days, 15 hours ago 

On 18/10/2024 04:25, Alistair Francis wrote:
> On Thu, Oct 17, 2024 at 5:45 PM Clément Léger <cleger@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 <cleger@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 <cleger@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 <cleger@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

Yes indeed. I did that in the next version.

Thanks,

Clément

> 
> Alistair
> 
>>
>> Clément
>>
>>>
>>> Alistair
>>>
>>>> value of mstatus_hs, just assume it is false.
>>>>
>>>> Thanks,
>>>>
>>>> Clément
>>>>
>>>>>
>>>>> Alistair
>>>>
>>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.