diff mbox series

[v2,2/8] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior

Message ID 20240925115808.77874-3-cleger@rivosinc.com (mailing list archive)
State New, archived
Headers show
Series target/riscv: Add support for Smdbltrp and Ssdbltrp extensions | expand

Commit Message

Clément Léger Sept. 25, 2024, 11:58 a.m. UTC 
When the Ssdbltrp extension is enabled, SSTATUS.SDT field is cleared
when executing sret. When executing mret/mnret, SSTATUS.SDT is cleared
when returning to U, VS or VU and VSSTATUS.SDT is cleared when returning
to VU from HS.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/op_helper.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

Comments

Alistair Francis Oct. 11, 2024, 3:13 a.m. UTC | #1 
On Wed, Sep 25, 2024 at 9:58 PM Clément Léger <cleger@rivosinc.com> wrote:
>
> When the Ssdbltrp extension is enabled, SSTATUS.SDT field is cleared
> when executing sret. When executing mret/mnret, SSTATUS.SDT is cleared
> when returning to U, VS or VU and VSSTATUS.SDT is cleared when returning
> to VU from HS.

I don't see mret being mentioned in the spec. Where do you see that
V/SSTATUS.SDT should be cleared?

Alistair

>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  target/riscv/op_helper.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 6895c7596b..00b6f75102 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -287,6 +287,18 @@ target_ulong helper_sret(CPURISCVState *env)
>                          get_field(mstatus, MSTATUS_SPIE));
>      mstatus = set_field(mstatus, MSTATUS_SPIE, 1);
>      mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
> +
> +    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
> +        if (riscv_has_ext(env, RVH)) {
> +            target_ulong prev_vu = get_field(env->hstatus, HSTATUS_SPV) &&
> +                                   prev_priv == PRV_U;
> +            /* Returning to VU from HS, vsstatus.sdt = 0 */
> +            if (!env->virt_enabled && prev_vu) {
> +                env->vsstatus = set_field(env->vsstatus, MSTATUS_SDT, 0);
> +            }
> +        }
> +        mstatus = set_field(mstatus, MSTATUS_SDT, 0);
> +    }
>      if (env->priv_ver >= PRIV_VERSION_1_12_0) {
>          mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
>      }
> @@ -297,7 +309,6 @@ target_ulong helper_sret(CPURISCVState *env)
>          target_ulong hstatus = env->hstatus;
>
>          prev_virt = get_field(hstatus, HSTATUS_SPV);
> -
>          hstatus = set_field(hstatus, HSTATUS_SPV, 0);
>
>          env->hstatus = hstatus;
> @@ -328,6 +339,22 @@ static void check_ret_from_m_mode(CPURISCVState *env, target_ulong retpc,
>          riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
>      }
>  }
> +static target_ulong ssdbltrp_mxret(CPURISCVState *env, target_ulong mstatus,
> +                                   target_ulong prev_priv,
> +                                   target_ulong prev_virt)
> +{
> +    /* If returning to U, VS or VU, sstatus.sdt = 0 */
> +    if (prev_priv == PRV_U || (prev_virt &&
> +        (prev_priv == PRV_S || prev_priv == PRV_U))) {
> +        mstatus = set_field(mstatus, MSTATUS_SDT, 0);
> +        /* If returning to VU, vsstatus.sdt = 0 */
> +        if (prev_virt && prev_priv == PRV_U) {
> +            env->vsstatus = set_field(env->vsstatus, MSTATUS_SDT, 0);
> +        }
> +    }
> +
> +    return mstatus;
> +}
>
>  target_ulong helper_mret(CPURISCVState *env)
>  {
> @@ -345,6 +372,9 @@ target_ulong helper_mret(CPURISCVState *env)
>      mstatus = set_field(mstatus, MSTATUS_MPP,
>                          riscv_has_ext(env, RVU) ? PRV_U : PRV_M);
>      mstatus = set_field(mstatus, MSTATUS_MPV, 0);
> +    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
> +        mstatus = ssdbltrp_mxret(env, mstatus, prev_priv, prev_virt);
> +    }
>      if ((env->priv_ver >= PRIV_VERSION_1_12_0) && (prev_priv != PRV_M)) {
>          mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
>      }
> @@ -382,6 +412,9 @@ target_ulong helper_mnret(CPURISCVState *env)
>      if (prev_priv < PRV_M) {
>          env->mstatus = set_field(env->mstatus, MSTATUS_MPRV, false);
>      }
> +    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
> +        env->mstatus = ssdbltrp_mxret(env, env->mstatus, prev_priv, prev_virt);
> +    }
>
>      if (riscv_has_ext(env, RVH) && prev_virt) {
>          riscv_cpu_swap_hypervisor_regs(env);
> --
> 2.45.2
>
>
Ved Shanbhogue Oct. 11, 2024, 6:52 p.m. UTC | #2 
Alistair Francis wrote:
>> When the Ssdbltrp extension is enabled, SSTATUS.SDT field is cleared
>> when executing sret. When executing mret/mnret, SSTATUS.SDT is cleared
>> when returning to U, VS or VU and VSSTATUS.SDT is cleared when returning
>> to VU from HS.
>
>I don't see mret being mentioned in the spec. Where do you see that
>V/SSTATUS.SDT should be cleared?
>

Ssdbltrp specifies:
    In S-mode, the SRET instruction sets sstatus.SDT to 0,
    and if the new privilege mode is VU, it also sets
    vsstatus.SDT to 0. However, in VS-mode, only vsstatus.SDT
    is set to to 0.

    The MRET instructions sets sstatus.SDT to 0, if the new
    privilege mode is U, VS, or VU. Additionally, if it is
    VU, then vsstatus.SDT is also set to 0.

Smdbltrp specifies:
    The MRET and SRET instructions, when executed in M-mode,
    set the MDT bit to 0. If the new privilege mode is U, VS,
    or VU, then sstatus.SDT is also set to 0. Additionally,
    if it is VU, then vsstatus.SDT is also set to 0.

    The MNRET instruction sets the MDT bit to 0 if the new
    privilege mode is not M. If it is U, VS, or VU, then
    sstatus.SDT is also set to 0. Additionally, if it is VU,
    then vsstatus.SDT is also set to 0.

regards
ved
Alistair Francis Oct. 17, 2024, 4:36 a.m. UTC | #3 
On Sat, Oct 12, 2024 at 4:52 AM Ved Shanbhogue <ved@rivosinc.com> wrote:
>
> Alistair Francis wrote:
> >> When the Ssdbltrp extension is enabled, SSTATUS.SDT field is cleared
> >> when executing sret. When executing mret/mnret, SSTATUS.SDT is cleared
> >> when returning to U, VS or VU and VSSTATUS.SDT is cleared when returning
> >> to VU from HS.
> >
> >I don't see mret being mentioned in the spec. Where do you see that
> >V/SSTATUS.SDT should be cleared?
> >
>
> Ssdbltrp specifies:
>     In S-mode, the SRET instruction sets sstatus.SDT to 0,
>     and if the new privilege mode is VU, it also sets
>     vsstatus.SDT to 0. However, in VS-mode, only vsstatus.SDT
>     is set to to 0.

I cannot find this

$ git show --shortstat
commit ef2ec9dc9afd003d0dab6d5ca36db59864c8483c (HEAD -> main, tag:
riscv-isa-release-ef2ec9d-2024-10-16, origin/main)
Author: Andrew Waterman <andrew@sifive.com>
Date:   Wed Oct 16 12:09:41 2024 -0700

   Remove future tense from description of now-ratified text (#1685)

2 files changed, 8 insertions(+), 15 deletions(-)

$ grep -r sstatus.SDT | grep SRET
src/hypervisor.adoc:if the new privilege mode is VU, the `SRET`
instruction sets `vsstatus.SDT`

What am I missing here?

Alistair

>
>     The MRET instructions sets sstatus.SDT to 0, if the new
>     privilege mode is U, VS, or VU. Additionally, if it is
>     VU, then vsstatus.SDT is also set to 0.
>
> Smdbltrp specifies:
>     The MRET and SRET instructions, when executed in M-mode,
>     set the MDT bit to 0. If the new privilege mode is U, VS,
>     or VU, then sstatus.SDT is also set to 0. Additionally,
>     if it is VU, then vsstatus.SDT is also set to 0.
>
>     The MNRET instruction sets the MDT bit to 0 if the new
>     privilege mode is not M. If it is U, VS, or VU, then
>     sstatus.SDT is also set to 0. Additionally, if it is VU,
>     then vsstatus.SDT is also set to 0.
>
> regards
> ved
Ved Shanbhogue Oct. 17, 2024, 6:27 p.m. UTC | #4 
Alistair Francis wrote:
>$ grep -r sstatus.SDT | grep SRET
>src/hypervisor.adoc:if the new privilege mode is VU, the `SRET`
>instruction sets `vsstatus.SDT`
>
>What am I missing here?

https://github.com/riscv/riscv-isa-manual/blob/ef2ec9dc9afd003d0dab6d5ca36db59864c8483c/src/machine.adoc?plain=1#L538


regards
ved
Alistair Francis Oct. 18, 2024, 2:21 a.m. UTC | #5 
On Fri, Oct 18, 2024 at 4:27 AM Ved Shanbhogue <ved@rivosinc.com> wrote:
>
> Alistair Francis wrote:
> >$ grep -r sstatus.SDT | grep SRET
> >src/hypervisor.adoc:if the new privilege mode is VU, the `SRET`
> >instruction sets `vsstatus.SDT`
> >
> >What am I missing here?
>
> https://github.com/riscv/riscv-isa-manual/blob/ef2ec9dc9afd003d0dab6d5ca36db59864c8483c/src/machine.adoc?plain=1#L538

Ah, I thought you were quoting the spec directly.

Makes sense. This patch misses the MDT bit clearing though. I'm
guessing that's implemented in a different patch, but it should be
pulled in here instead

Alistair

>
>
> regards
> ved
Clément Léger Oct. 18, 2024, 7:02 a.m. UTC | #6 
On 18/10/2024 04:21, Alistair Francis wrote:
> On Fri, Oct 18, 2024 at 4:27 AM Ved Shanbhogue <ved@rivosinc.com> wrote:
>>
>> Alistair Francis wrote:
>>> $ grep -r sstatus.SDT | grep SRET
>>> src/hypervisor.adoc:if the new privilege mode is VU, the `SRET`
>>> instruction sets `vsstatus.SDT`
>>>
>>> What am I missing here?
>>
>> https://github.com/riscv/riscv-isa-manual/blob/ef2ec9dc9afd003d0dab6d5ca36db59864c8483c/src/machine.adoc?plain=1#L538
> 
> Ah, I thought you were quoting the spec directly.
> 
> Makes sense. This patch misses the MDT bit clearing though. I'm
> guessing that's implemented in a different patch, but it should be
> pulled in here instead

This is actually done in the patch that adds Smdbltrp (this on is for
Ssdbltrp).

Thanks,

Clément

> 
> Alistair
> 
>>
>>
>> regards
>> ved
diff mbox series

Patch

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 6895c7596b..00b6f75102 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -287,6 +287,18 @@  target_ulong helper_sret(CPURISCVState *env)
                         get_field(mstatus, MSTATUS_SPIE));
     mstatus = set_field(mstatus, MSTATUS_SPIE, 1);
     mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
+
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        if (riscv_has_ext(env, RVH)) {
+            target_ulong prev_vu = get_field(env->hstatus, HSTATUS_SPV) &&
+                                   prev_priv == PRV_U;
+            /* Returning to VU from HS, vsstatus.sdt = 0 */
+            if (!env->virt_enabled && prev_vu) {
+                env->vsstatus = set_field(env->vsstatus, MSTATUS_SDT, 0);
+            }
+        }
+        mstatus = set_field(mstatus, MSTATUS_SDT, 0);
+    }
     if (env->priv_ver >= PRIV_VERSION_1_12_0) {
         mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
     }
@@ -297,7 +309,6 @@  target_ulong helper_sret(CPURISCVState *env)
         target_ulong hstatus = env->hstatus;
 
         prev_virt = get_field(hstatus, HSTATUS_SPV);
-
         hstatus = set_field(hstatus, HSTATUS_SPV, 0);
 
         env->hstatus = hstatus;
@@ -328,6 +339,22 @@  static void check_ret_from_m_mode(CPURISCVState *env, target_ulong retpc,
         riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
     }
 }
+static target_ulong ssdbltrp_mxret(CPURISCVState *env, target_ulong mstatus,
+                                   target_ulong prev_priv,
+                                   target_ulong prev_virt)
+{
+    /* If returning to U, VS or VU, sstatus.sdt = 0 */
+    if (prev_priv == PRV_U || (prev_virt &&
+        (prev_priv == PRV_S || prev_priv == PRV_U))) {
+        mstatus = set_field(mstatus, MSTATUS_SDT, 0);
+        /* If returning to VU, vsstatus.sdt = 0 */
+        if (prev_virt && prev_priv == PRV_U) {
+            env->vsstatus = set_field(env->vsstatus, MSTATUS_SDT, 0);
+        }
+    }
+
+    return mstatus;
+}
 
 target_ulong helper_mret(CPURISCVState *env)
 {
@@ -345,6 +372,9 @@  target_ulong helper_mret(CPURISCVState *env)
     mstatus = set_field(mstatus, MSTATUS_MPP,
                         riscv_has_ext(env, RVU) ? PRV_U : PRV_M);
     mstatus = set_field(mstatus, MSTATUS_MPV, 0);
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        mstatus = ssdbltrp_mxret(env, mstatus, prev_priv, prev_virt);
+    }
     if ((env->priv_ver >= PRIV_VERSION_1_12_0) && (prev_priv != PRV_M)) {
         mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
     }
@@ -382,6 +412,9 @@  target_ulong helper_mnret(CPURISCVState *env)
     if (prev_priv < PRV_M) {
         env->mstatus = set_field(env->mstatus, MSTATUS_MPRV, false);
     }
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        env->mstatus = ssdbltrp_mxret(env, env->mstatus, prev_priv, prev_virt);
+    }
 
     if (riscv_has_ext(env, RVH) && prev_virt) {
         riscv_cpu_swap_hypervisor_regs(env);