| Message ID | 20240809081227.1588508-5-tommy.wu@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Add Smrnmi support. | expand |
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 >
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 > >
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 > > >
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 > > > >
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);