[v5] target/riscv: Add Smrnmi support.

[PATCH v5 4/5] target/riscv: Add Smrnmi mnret instruction.

Posted by Tommy Wu 1 year, 6 months ago

This patch adds a new instruction `mnret`. `mnret` is an M-mode-only
instruction that uses the values in `mnepc` and `mnstatus` to return to the
program counter, privilege mode, and virtualization mode of the
interrupted context.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
---
 target/riscv/helper.h                         |  1 +
 target/riscv/insn32.decode                    |  3 ++
 .../riscv/insn_trans/trans_privileged.c.inc   | 12 +++++
 target/riscv/op_helper.c                      | 46 +++++++++++++++++++
 4 files changed, 62 insertions(+)

diff --git a/target/riscv/helper.h b/target/riscv/helper.h
index 451261ce5a..16ea240d26 100644
--- a/target/riscv/helper.h
+++ b/target/riscv/helper.h
@@ -131,6 +131,7 @@ DEF_HELPER_6(csrrw_i128, tl, env, int, tl, tl, tl, tl)
 #ifndef CONFIG_USER_ONLY
 DEF_HELPER_1(sret, tl, env)
 DEF_HELPER_1(mret, tl, env)
+DEF_HELPER_1(mnret, tl, env)
 DEF_HELPER_1(wfi, void, env)
 DEF_HELPER_1(wrs_nto, void, env)
 DEF_HELPER_1(tlb_flush, void, env)
diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
index c45b8fa1d8..d320631e8c 100644
--- a/target/riscv/insn32.decode
+++ b/target/riscv/insn32.decode
@@ -121,6 +121,9 @@ wfi         0001000    00101 00000 000 00000 1110011
 sfence_vma  0001001    ..... ..... 000 00000 1110011 @sfence_vma
 sfence_vm   0001000    00100 ..... 000 00000 1110011 @sfence_vm
 
+# *** NMI ***
+mnret       0111000    00010 00000 000 00000 1110011
+
 # *** RV32I Base Instruction Set ***
 lui      ....................       ..... 0110111 @u
 auipc    ....................       ..... 0010111 @u
diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
index bc5263a4e0..06bc20dda4 100644
--- a/target/riscv/insn_trans/trans_privileged.c.inc
+++ b/target/riscv/insn_trans/trans_privileged.c.inc
@@ -106,6 +106,18 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
 #endif
 }
 
+static bool trans_mnret(DisasContext *ctx, arg_mnret *a)
+{
+#ifndef CONFIG_USER_ONLY
+    gen_helper_mnret(cpu_pc, tcg_env);
+    tcg_gen_exit_tb(NULL, 0); /* no chaining */
+    ctx->base.is_jmp = DISAS_NORETURN;
+    return true;
+#else
+    return false;
+#endif
+}
+
 static bool trans_wfi(DisasContext *ctx, arg_wfi *a)
 {
 #ifndef CONFIG_USER_ONLY
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 25a5263573..3e26392e65 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -353,6 +353,52 @@ target_ulong helper_mret(CPURISCVState *env)
     return retpc;
 }
 
+target_ulong helper_mnret(CPURISCVState *env)
+{
+    if (!riscv_cpu_cfg(env)->ext_smrnmi) {
+        /* RNMI feature is not presented. */
+        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
+    }
+
+    if (!(env->priv >= PRV_M)) {
+        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
+    }
+
+    /* Get return PC from mnepc CSR. */
+    target_ulong retpc = env->mnepc;
+    if (!riscv_has_ext(env, RVC) && (retpc & 0x3)) {
+        riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
+    }
+
+    /* Get previous privilege level from mnstatus CSR. */
+    target_ulong prev_priv = get_field(env->mnstatus, MNSTATUS_MNPP);
+
+    if (riscv_cpu_cfg(env)->pmp &&
+        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
+        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
+    }
+
+    target_ulong prev_virt = get_field(env->mnstatus, MNSTATUS_MNPV) &&
+                             (prev_priv != PRV_M);
+    env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, true);
+
+    /*
+     * If MNRET changes the privilege mode to a mode
+     * less privileged than M, it also sets mstatus.MPRV to 0.
+     */
+    if (prev_priv < PRV_M) {
+        env->mstatus = set_field(env->mstatus, MSTATUS_MPRV, false);
+    }
+
+    if (riscv_has_ext(env, RVH) && prev_virt) {
+        riscv_cpu_swap_hypervisor_regs(env);
+    }
+
+    riscv_cpu_set_mode(env, prev_priv, prev_virt);
+
+    return retpc;
+}
+
 void helper_wfi(CPURISCVState *env)
 {
     CPUState *cs = env_cpu(env);
-- 
2.39.3

Re: [PATCH v5 4/5] target/riscv: Add Smrnmi mnret instruction.

Posted by Alistair Francis 1 year, 5 months ago

On Fri, Aug 9, 2024 at 6:12 PM Tommy Wu <tommy.wu@sifive.com> wrote:
>
> This patch adds a new instruction `mnret`. `mnret` is an M-mode-only
> instruction that uses the values in `mnepc` and `mnstatus` to return to the
> program counter, privilege mode, and virtualization mode of the
> interrupted context.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> ---
>  target/riscv/helper.h                         |  1 +
>  target/riscv/insn32.decode                    |  3 ++
>  .../riscv/insn_trans/trans_privileged.c.inc   | 12 +++++
>  target/riscv/op_helper.c                      | 46 +++++++++++++++++++
>  4 files changed, 62 insertions(+)
>
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index 451261ce5a..16ea240d26 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -131,6 +131,7 @@ DEF_HELPER_6(csrrw_i128, tl, env, int, tl, tl, tl, tl)
>  #ifndef CONFIG_USER_ONLY
>  DEF_HELPER_1(sret, tl, env)
>  DEF_HELPER_1(mret, tl, env)
> +DEF_HELPER_1(mnret, tl, env)
>  DEF_HELPER_1(wfi, void, env)
>  DEF_HELPER_1(wrs_nto, void, env)
>  DEF_HELPER_1(tlb_flush, void, env)
> diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> index c45b8fa1d8..d320631e8c 100644
> --- a/target/riscv/insn32.decode
> +++ b/target/riscv/insn32.decode
> @@ -121,6 +121,9 @@ wfi         0001000    00101 00000 000 00000 1110011
>  sfence_vma  0001001    ..... ..... 000 00000 1110011 @sfence_vma
>  sfence_vm   0001000    00100 ..... 000 00000 1110011 @sfence_vm
>
> +# *** NMI ***
> +mnret       0111000    00010 00000 000 00000 1110011
> +
>  # *** RV32I Base Instruction Set ***
>  lui      ....................       ..... 0110111 @u
>  auipc    ....................       ..... 0010111 @u
> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
> index bc5263a4e0..06bc20dda4 100644
> --- a/target/riscv/insn_trans/trans_privileged.c.inc
> +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> @@ -106,6 +106,18 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
>  #endif
>  }
>
> +static bool trans_mnret(DisasContext *ctx, arg_mnret *a)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    gen_helper_mnret(cpu_pc, tcg_env);
> +    tcg_gen_exit_tb(NULL, 0); /* no chaining */
> +    ctx->base.is_jmp = DISAS_NORETURN;
> +    return true;
> +#else
> +    return false;
> +#endif
> +}
> +
>  static bool trans_wfi(DisasContext *ctx, arg_wfi *a)
>  {
>  #ifndef CONFIG_USER_ONLY
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 25a5263573..3e26392e65 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -353,6 +353,52 @@ target_ulong helper_mret(CPURISCVState *env)
>      return retpc;
>  }
>
> +target_ulong helper_mnret(CPURISCVState *env)
> +{
> +    if (!riscv_cpu_cfg(env)->ext_smrnmi) {
> +        /* RNMI feature is not presented. */
> +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> +    }
> +
> +    if (!(env->priv >= PRV_M)) {

This should just be (env->priv != PRV_M)

> +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> +    }
> +
> +    /* Get return PC from mnepc CSR. */
> +    target_ulong retpc = env->mnepc;
> +    if (!riscv_has_ext(env, RVC) && (retpc & 0x3)) {

What is the purpose of this check? The low bits should be zero,
unrelated to the compressed instructions

> +        riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
> +    }
> +
> +    /* Get previous privilege level from mnstatus CSR. */
> +    target_ulong prev_priv = get_field(env->mnstatus, MNSTATUS_MNPP);
> +
> +    if (riscv_cpu_cfg(env)->pmp &&
> +        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {

I don't see this mentioned in:

https://github.com/riscv/riscv-isa-manual/blob/a4382e9c8e285360a88d8056c1253e1525552393/src/rnmi.adoc

Alistair

> +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> +    }
> +
> +    target_ulong prev_virt = get_field(env->mnstatus, MNSTATUS_MNPV) &&
> +                             (prev_priv != PRV_M);
> +    env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, true);
> +
> +    /*
> +     * If MNRET changes the privilege mode to a mode
> +     * less privileged than M, it also sets mstatus.MPRV to 0.
> +     */
> +    if (prev_priv < PRV_M) {
> +        env->mstatus = set_field(env->mstatus, MSTATUS_MPRV, false);
> +    }
> +
> +    if (riscv_has_ext(env, RVH) && prev_virt) {
> +        riscv_cpu_swap_hypervisor_regs(env);
> +    }
> +
> +    riscv_cpu_set_mode(env, prev_priv, prev_virt);
> +
> +    return retpc;
> +}
> +
>  void helper_wfi(CPURISCVState *env)
>  {
>      CPUState *cs = env_cpu(env);
> --
> 2.39.3
>

Re: [PATCH v5 4/5] target/riscv: Add Smrnmi mnret instruction.

Posted by Tommy Wu 1 year, 5 months ago

On Mon, Aug 19, 2024 at 11:49 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, Aug 9, 2024 at 6:12 PM Tommy Wu <tommy.wu@sifive.com> wrote:
> >
> > This patch adds a new instruction `mnret`. `mnret` is an M-mode-only
> > instruction that uses the values in `mnepc` and `mnstatus` to return to the
> > program counter, privilege mode, and virtualization mode of the
> > interrupted context.
> >
> > Signed-off-by: Frank Chang <frank.chang@sifive.com>
> > Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> > ---
> >  target/riscv/helper.h                         |  1 +
> >  target/riscv/insn32.decode                    |  3 ++
> >  .../riscv/insn_trans/trans_privileged.c.inc   | 12 +++++
> >  target/riscv/op_helper.c                      | 46 +++++++++++++++++++
> >  4 files changed, 62 insertions(+)
> >
> > diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> > index 451261ce5a..16ea240d26 100644
> > --- a/target/riscv/helper.h
> > +++ b/target/riscv/helper.h
> > @@ -131,6 +131,7 @@ DEF_HELPER_6(csrrw_i128, tl, env, int, tl, tl, tl, tl)
> >  #ifndef CONFIG_USER_ONLY
> >  DEF_HELPER_1(sret, tl, env)
> >  DEF_HELPER_1(mret, tl, env)
> > +DEF_HELPER_1(mnret, tl, env)
> >  DEF_HELPER_1(wfi, void, env)
> >  DEF_HELPER_1(wrs_nto, void, env)
> >  DEF_HELPER_1(tlb_flush, void, env)
> > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> > index c45b8fa1d8..d320631e8c 100644
> > --- a/target/riscv/insn32.decode
> > +++ b/target/riscv/insn32.decode
> > @@ -121,6 +121,9 @@ wfi         0001000    00101 00000 000 00000 1110011
> >  sfence_vma  0001001    ..... ..... 000 00000 1110011 @sfence_vma
> >  sfence_vm   0001000    00100 ..... 000 00000 1110011 @sfence_vm
> >
> > +# *** NMI ***
> > +mnret       0111000    00010 00000 000 00000 1110011
> > +
> >  # *** RV32I Base Instruction Set ***
> >  lui      ....................       ..... 0110111 @u
> >  auipc    ....................       ..... 0010111 @u
> > diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
> > index bc5263a4e0..06bc20dda4 100644
> > --- a/target/riscv/insn_trans/trans_privileged.c.inc
> > +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> > @@ -106,6 +106,18 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
> >  #endif
> >  }
> >
> > +static bool trans_mnret(DisasContext *ctx, arg_mnret *a)
> > +{
> > +#ifndef CONFIG_USER_ONLY
> > +    gen_helper_mnret(cpu_pc, tcg_env);
> > +    tcg_gen_exit_tb(NULL, 0); /* no chaining */
> > +    ctx->base.is_jmp = DISAS_NORETURN;
> > +    return true;
> > +#else
> > +    return false;
> > +#endif
> > +}
> > +
> >  static bool trans_wfi(DisasContext *ctx, arg_wfi *a)
> >  {
> >  #ifndef CONFIG_USER_ONLY
> > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > index 25a5263573..3e26392e65 100644
> > --- a/target/riscv/op_helper.c
> > +++ b/target/riscv/op_helper.c
> > @@ -353,6 +353,52 @@ target_ulong helper_mret(CPURISCVState *env)
> >      return retpc;
> >  }
> >
> > +target_ulong helper_mnret(CPURISCVState *env)
> > +{
> > +    if (!riscv_cpu_cfg(env)->ext_smrnmi) {
> > +        /* RNMI feature is not presented. */
> > +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > +    }
> > +
> > +    if (!(env->priv >= PRV_M)) {
>
> This should just be (env->priv != PRV_M)
>

Thanks for the suggestion. You’re right.
The reason that we write the code is that we want to do the same
checks that `helper_mret` does.[1]

Maybe we can send another patchset to fix both `helper_mret` and `helper_mnret`.

[1]  https://github.com/qemu/qemu/commit/0c3e702aca76ca6ebf2aac4451870efc9d52a7a3

> > +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > +    }
> > +
> > +    /* Get return PC from mnepc CSR. */
> > +    target_ulong retpc = env->mnepc;
> > +    if (!riscv_has_ext(env, RVC) && (retpc & 0x3)) {
>
> What is the purpose of this check? The low bits should be zero,
> unrelated to the compressed instructions
>

This is also the same check that `helper_mret` does.[1]
Maybe we can send another patchset to fix both `helper_mret` and `helper_mnret`.

[1]  https://github.com/qemu/qemu/commit/0c3e702aca76ca6ebf2aac4451870efc9d52a7a3

> > +        riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
> > +    }
> > +
> > +    /* Get previous privilege level from mnstatus CSR. */
> > +    target_ulong prev_priv = get_field(env->mnstatus, MNSTATUS_MNPP);
> > +
> > +    if (riscv_cpu_cfg(env)->pmp &&
> > +        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
>
> I don't see this mentioned in:
>
> https://github.com/riscv/riscv-isa-manual/blob/a4382e9c8e285360a88d8056c1253e1525552393/src/rnmi.adoc
>
> Alistair
>

This is also the same check that `helper_mret` does.[2]
It seems that this part is mentioned in the `riscv-privileged`
specification, `Priority and Matching Logic` section.

[2] https://github.com/qemu/qemu/commit/0fbb5d2d3c9ded9fbd3f6f993974cc5e88e28912

Thanks for all the suggestions and code review.

Best Regards,
Tommy Wu.

> > +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > +    }
> > +
> > +    target_ulong prev_virt = get_field(env->mnstatus, MNSTATUS_MNPV) &&
> > +                             (prev_priv != PRV_M);
> > +    env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, true);
> > +
> > +    /*
> > +     * If MNRET changes the privilege mode to a mode
> > +     * less privileged than M, it also sets mstatus.MPRV to 0.
> > +     */
> > +    if (prev_priv < PRV_M) {
> > +        env->mstatus = set_field(env->mstatus, MSTATUS_MPRV, false);
> > +    }
> > +
> > +    if (riscv_has_ext(env, RVH) && prev_virt) {
> > +        riscv_cpu_swap_hypervisor_regs(env);
> > +    }
> > +
> > +    riscv_cpu_set_mode(env, prev_priv, prev_virt);
> > +
> > +    return retpc;
> > +}
> > +
> >  void helper_wfi(CPURISCVState *env)
> >  {
> >      CPUState *cs = env_cpu(env);
> > --
> > 2.39.3
> >

Re: [PATCH v5 4/5] target/riscv: Add Smrnmi mnret instruction.

Posted by Alistair Francis 1 year, 5 months ago

On Fri, Aug 30, 2024 at 2:12 AM Tommy Wu <tommy.wu@sifive.com> wrote:
>
> On Mon, Aug 19, 2024 at 11:49 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Fri, Aug 9, 2024 at 6:12 PM Tommy Wu <tommy.wu@sifive.com> wrote:
> > >
> > > This patch adds a new instruction `mnret`. `mnret` is an M-mode-only
> > > instruction that uses the values in `mnepc` and `mnstatus` to return to the
> > > program counter, privilege mode, and virtualization mode of the
> > > interrupted context.
> > >
> > > Signed-off-by: Frank Chang <frank.chang@sifive.com>
> > > Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> > > ---
> > >  target/riscv/helper.h                         |  1 +
> > >  target/riscv/insn32.decode                    |  3 ++
> > >  .../riscv/insn_trans/trans_privileged.c.inc   | 12 +++++
> > >  target/riscv/op_helper.c                      | 46 +++++++++++++++++++
> > >  4 files changed, 62 insertions(+)
> > >
> > > diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> > > index 451261ce5a..16ea240d26 100644
> > > --- a/target/riscv/helper.h
> > > +++ b/target/riscv/helper.h
> > > @@ -131,6 +131,7 @@ DEF_HELPER_6(csrrw_i128, tl, env, int, tl, tl, tl, tl)
> > >  #ifndef CONFIG_USER_ONLY
> > >  DEF_HELPER_1(sret, tl, env)
> > >  DEF_HELPER_1(mret, tl, env)
> > > +DEF_HELPER_1(mnret, tl, env)
> > >  DEF_HELPER_1(wfi, void, env)
> > >  DEF_HELPER_1(wrs_nto, void, env)
> > >  DEF_HELPER_1(tlb_flush, void, env)
> > > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> > > index c45b8fa1d8..d320631e8c 100644
> > > --- a/target/riscv/insn32.decode
> > > +++ b/target/riscv/insn32.decode
> > > @@ -121,6 +121,9 @@ wfi         0001000    00101 00000 000 00000 1110011
> > >  sfence_vma  0001001    ..... ..... 000 00000 1110011 @sfence_vma
> > >  sfence_vm   0001000    00100 ..... 000 00000 1110011 @sfence_vm
> > >
> > > +# *** NMI ***
> > > +mnret       0111000    00010 00000 000 00000 1110011
> > > +
> > >  # *** RV32I Base Instruction Set ***
> > >  lui      ....................       ..... 0110111 @u
> > >  auipc    ....................       ..... 0010111 @u
> > > diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
> > > index bc5263a4e0..06bc20dda4 100644
> > > --- a/target/riscv/insn_trans/trans_privileged.c.inc
> > > +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> > > @@ -106,6 +106,18 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
> > >  #endif
> > >  }
> > >
> > > +static bool trans_mnret(DisasContext *ctx, arg_mnret *a)
> > > +{
> > > +#ifndef CONFIG_USER_ONLY
> > > +    gen_helper_mnret(cpu_pc, tcg_env);
> > > +    tcg_gen_exit_tb(NULL, 0); /* no chaining */
> > > +    ctx->base.is_jmp = DISAS_NORETURN;
> > > +    return true;
> > > +#else
> > > +    return false;
> > > +#endif
> > > +}
> > > +
> > >  static bool trans_wfi(DisasContext *ctx, arg_wfi *a)
> > >  {
> > >  #ifndef CONFIG_USER_ONLY
> > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > > index 25a5263573..3e26392e65 100644
> > > --- a/target/riscv/op_helper.c
> > > +++ b/target/riscv/op_helper.c
> > > @@ -353,6 +353,52 @@ target_ulong helper_mret(CPURISCVState *env)
> > >      return retpc;
> > >  }
> > >
> > > +target_ulong helper_mnret(CPURISCVState *env)
> > > +{
> > > +    if (!riscv_cpu_cfg(env)->ext_smrnmi) {
> > > +        /* RNMI feature is not presented. */
> > > +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > > +    }
> > > +
> > > +    if (!(env->priv >= PRV_M)) {
> >
> > This should just be (env->priv != PRV_M)
> >
>
> Thanks for the suggestion. You’re right.
> The reason that we write the code is that we want to do the same
> checks that `helper_mret` does.[1]
>
> Maybe we can send another patchset to fix both `helper_mret` and `helper_mnret`.
>
> [1]  https://github.com/qemu/qemu/commit/0c3e702aca76ca6ebf2aac4451870efc9d52a7a3
>
> > > +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > > +    }
> > > +
> > > +    /* Get return PC from mnepc CSR. */
> > > +    target_ulong retpc = env->mnepc;
> > > +    if (!riscv_has_ext(env, RVC) && (retpc & 0x3)) {
> >
> > What is the purpose of this check? The low bits should be zero,
> > unrelated to the compressed instructions
> >
>
> This is also the same check that `helper_mret` does.[1]
> Maybe we can send another patchset to fix both `helper_mret` and `helper_mnret`.

Ah, ok. It's probably worth splitting the shared helper_mret() code
into a helper function then

Alistair

>
> [1]  https://github.com/qemu/qemu/commit/0c3e702aca76ca6ebf2aac4451870efc9d52a7a3
>
> > > +        riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
> > > +    }
> > > +
> > > +    /* Get previous privilege level from mnstatus CSR. */
> > > +    target_ulong prev_priv = get_field(env->mnstatus, MNSTATUS_MNPP);
> > > +
> > > +    if (riscv_cpu_cfg(env)->pmp &&
> > > +        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
> >
> > I don't see this mentioned in:
> >
> > https://github.com/riscv/riscv-isa-manual/blob/a4382e9c8e285360a88d8056c1253e1525552393/src/rnmi.adoc
> >
> > Alistair
> >
>
> This is also the same check that `helper_mret` does.[2]
> It seems that this part is mentioned in the `riscv-privileged`
> specification, `Priority and Matching Logic` section.
>
> [2] https://github.com/qemu/qemu/commit/0fbb5d2d3c9ded9fbd3f6f993974cc5e88e28912
>
> Thanks for all the suggestions and code review.
>
> Best Regards,
> Tommy Wu.
>
> > > +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > > +    }
> > > +
> > > +    target_ulong prev_virt = get_field(env->mnstatus, MNSTATUS_MNPV) &&
> > > +                             (prev_priv != PRV_M);
> > > +    env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, true);
> > > +
> > > +    /*
> > > +     * If MNRET changes the privilege mode to a mode
> > > +     * less privileged than M, it also sets mstatus.MPRV to 0.
> > > +     */
> > > +    if (prev_priv < PRV_M) {
> > > +        env->mstatus = set_field(env->mstatus, MSTATUS_MPRV, false);
> > > +    }
> > > +
> > > +    if (riscv_has_ext(env, RVH) && prev_virt) {
> > > +        riscv_cpu_swap_hypervisor_regs(env);
> > > +    }
> > > +
> > > +    riscv_cpu_set_mode(env, prev_priv, prev_virt);
> > > +
> > > +    return retpc;
> > > +}
> > > +
> > >  void helper_wfi(CPURISCVState *env)
> > >  {
> > >      CPUState *cs = env_cpu(env);
> > > --
> > > 2.39.3
> > >

Re: [PATCH v5 4/5] target/riscv: Add Smrnmi mnret instruction.

Posted by Tommy Wu 1 year, 5 months ago

On Fri, Aug 30, 2024 at 7:42 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, Aug 30, 2024 at 2:12 AM Tommy Wu <tommy.wu@sifive.com> wrote:
> >
> > On Mon, Aug 19, 2024 at 11:49 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Fri, Aug 9, 2024 at 6:12 PM Tommy Wu <tommy.wu@sifive.com> wrote:
> > > >
> > > > This patch adds a new instruction `mnret`. `mnret` is an M-mode-only
> > > > instruction that uses the values in `mnepc` and `mnstatus` to return to the
> > > > program counter, privilege mode, and virtualization mode of the
> > > > interrupted context.
> > > >
> > > > Signed-off-by: Frank Chang <frank.chang@sifive.com>
> > > > Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> > > > ---
> > > >  target/riscv/helper.h                         |  1 +
> > > >  target/riscv/insn32.decode                    |  3 ++
> > > >  .../riscv/insn_trans/trans_privileged.c.inc   | 12 +++++
> > > >  target/riscv/op_helper.c                      | 46 +++++++++++++++++++
> > > >  4 files changed, 62 insertions(+)
> > > >
> > > > diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> > > > index 451261ce5a..16ea240d26 100644
> > > > --- a/target/riscv/helper.h
> > > > +++ b/target/riscv/helper.h
> > > > @@ -131,6 +131,7 @@ DEF_HELPER_6(csrrw_i128, tl, env, int, tl, tl, tl, tl)
> > > >  #ifndef CONFIG_USER_ONLY
> > > >  DEF_HELPER_1(sret, tl, env)
> > > >  DEF_HELPER_1(mret, tl, env)
> > > > +DEF_HELPER_1(mnret, tl, env)
> > > >  DEF_HELPER_1(wfi, void, env)
> > > >  DEF_HELPER_1(wrs_nto, void, env)
> > > >  DEF_HELPER_1(tlb_flush, void, env)
> > > > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> > > > index c45b8fa1d8..d320631e8c 100644
> > > > --- a/target/riscv/insn32.decode
> > > > +++ b/target/riscv/insn32.decode
> > > > @@ -121,6 +121,9 @@ wfi         0001000    00101 00000 000 00000 1110011
> > > >  sfence_vma  0001001    ..... ..... 000 00000 1110011 @sfence_vma
> > > >  sfence_vm   0001000    00100 ..... 000 00000 1110011 @sfence_vm
> > > >
> > > > +# *** NMI ***
> > > > +mnret       0111000    00010 00000 000 00000 1110011
> > > > +
> > > >  # *** RV32I Base Instruction Set ***
> > > >  lui      ....................       ..... 0110111 @u
> > > >  auipc    ....................       ..... 0010111 @u
> > > > diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
> > > > index bc5263a4e0..06bc20dda4 100644
> > > > --- a/target/riscv/insn_trans/trans_privileged.c.inc
> > > > +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> > > > @@ -106,6 +106,18 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
> > > >  #endif
> > > >  }
> > > >
> > > > +static bool trans_mnret(DisasContext *ctx, arg_mnret *a)
> > > > +{
> > > > +#ifndef CONFIG_USER_ONLY
> > > > +    gen_helper_mnret(cpu_pc, tcg_env);
> > > > +    tcg_gen_exit_tb(NULL, 0); /* no chaining */
> > > > +    ctx->base.is_jmp = DISAS_NORETURN;
> > > > +    return true;
> > > > +#else
> > > > +    return false;
> > > > +#endif
> > > > +}
> > > > +
> > > >  static bool trans_wfi(DisasContext *ctx, arg_wfi *a)
> > > >  {
> > > >  #ifndef CONFIG_USER_ONLY
> > > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > > > index 25a5263573..3e26392e65 100644
> > > > --- a/target/riscv/op_helper.c
> > > > +++ b/target/riscv/op_helper.c
> > > > @@ -353,6 +353,52 @@ target_ulong helper_mret(CPURISCVState *env)
> > > >      return retpc;
> > > >  }
> > > >
> > > > +target_ulong helper_mnret(CPURISCVState *env)
> > > > +{
> > > > +    if (!riscv_cpu_cfg(env)->ext_smrnmi) {
> > > > +        /* RNMI feature is not presented. */
> > > > +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > > > +    }
> > > > +
> > > > +    if (!(env->priv >= PRV_M)) {
> > >
> > > This should just be (env->priv != PRV_M)
> > >
> >
> > Thanks for the suggestion. You’re right.
> > The reason that we write the code is that we want to do the same
> > checks that `helper_mret` does.[1]
> >
> > Maybe we can send another patchset to fix both `helper_mret` and `helper_mnret`.
> >
> > [1]  https://github.com/qemu/qemu/commit/0c3e702aca76ca6ebf2aac4451870efc9d52a7a3
> >
> > > > +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > > > +    }
> > > > +
> > > > +    /* Get return PC from mnepc CSR. */
> > > > +    target_ulong retpc = env->mnepc;
> > > > +    if (!riscv_has_ext(env, RVC) && (retpc & 0x3)) {
> > >
> > > What is the purpose of this check? The low bits should be zero,
> > > unrelated to the compressed instructions
> > >
> >
> > This is also the same check that `helper_mret` does.[1]
> > Maybe we can send another patchset to fix both `helper_mret` and `helper_mnret`.
>
> Ah, ok. It's probably worth splitting the shared helper_mret() code
> into a helper function then
>
> Alistair
>

Thanks for the suggestion, I'll split the shared code into
a helper function in the v6 patchset.

Best Regards,
Tommy Wu

> >
> > [1]  https://github.com/qemu/qemu/commit/0c3e702aca76ca6ebf2aac4451870efc9d52a7a3
> >
> > > > +        riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
> > > > +    }
> > > > +
> > > > +    /* Get previous privilege level from mnstatus CSR. */
> > > > +    target_ulong prev_priv = get_field(env->mnstatus, MNSTATUS_MNPP);
> > > > +
> > > > +    if (riscv_cpu_cfg(env)->pmp &&
> > > > +        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
> > >
> > > I don't see this mentioned in:
> > >
> > > https://github.com/riscv/riscv-isa-manual/blob/a4382e9c8e285360a88d8056c1253e1525552393/src/rnmi.adoc
> > >
> > > Alistair
> > >
> >
> > This is also the same check that `helper_mret` does.[2]
> > It seems that this part is mentioned in the `riscv-privileged`
> > specification, `Priority and Matching Logic` section.
> >
> > [2] https://github.com/qemu/qemu/commit/0fbb5d2d3c9ded9fbd3f6f993974cc5e88e28912
> >
> > Thanks for all the suggestions and code review.
> >
> > Best Regards,
> > Tommy Wu.
> >
> > > > +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > > > +    }
> > > > +
> > > > +    target_ulong prev_virt = get_field(env->mnstatus, MNSTATUS_MNPV) &&
> > > > +                             (prev_priv != PRV_M);
> > > > +    env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, true);
> > > > +
> > > > +    /*
> > > > +     * If MNRET changes the privilege mode to a mode
> > > > +     * less privileged than M, it also sets mstatus.MPRV to 0.
> > > > +     */
> > > > +    if (prev_priv < PRV_M) {
> > > > +        env->mstatus = set_field(env->mstatus, MSTATUS_MPRV, false);
> > > > +    }
> > > > +
> > > > +    if (riscv_has_ext(env, RVH) && prev_virt) {
> > > > +        riscv_cpu_swap_hypervisor_regs(env);
> > > > +    }
> > > > +
> > > > +    riscv_cpu_set_mode(env, prev_priv, prev_virt);
> > > > +
> > > > +    return retpc;
> > > > +}
> > > > +
> > > >  void helper_wfi(CPURISCVState *env)
> > > >  {
> > > >      CPUState *cs = env_cpu(env);
> > > > --
> > > > 2.39.3
> > > >

[PATCH v5 1/5] target/riscv: Add `ext_smrnmi` in the RISCVCPUConfig.
[PATCH v5 2/5] target/riscv: Handle Smrnmi interrupt and exception.
[PATCH v5 3/5] target/riscv: Add Smrnmi CSRs.
[PATCH v5 4/5] target/riscv: Add Smrnmi mnret instruction.
[PATCH v5 5/5] target/riscv: Add Smrnmi cpu extension.