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

Clément Léger posted 8 patches 3 weeks, 6 days ago
There is a newer version of this series
[PATCH v2 2/8] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior
Posted by Clément Léger 3 weeks, 6 days ago
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(-)

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


Re: [PATCH v2 2/8] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior
Posted by Alistair Francis 1 week, 4 days ago
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
>
>
Re: [PATCH v2 2/8] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior
Posted by Ved Shanbhogue 1 week, 4 days ago
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
Re: [PATCH v2 2/8] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior
Posted by Alistair Francis 5 days, 17 hours ago
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
Re: [PATCH v2 2/8] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior
Posted by Ved Shanbhogue 5 days, 3 hours ago
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
Re: [PATCH v2 2/8] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior
Posted by Alistair Francis 4 days, 19 hours ago
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
Re: [PATCH v2 2/8] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior
Posted by Clément Léger 4 days, 15 hours ago

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