| Message ID | 20240925115808.77874-3-cleger@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv: Add support for Smdbltrp and Ssdbltrp extensions | expand |
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 > >
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
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
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
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
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 --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);
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(-)