On Fri, Aug 30, 2024 at 2:12 AM Tommy Wu <tommy...@sifive.com> wrote:
>
> On Mon, Aug 19, 2024 at 11:49 AM Alistair Francis <alistai...@gmail.com>
> wrote:
> >
> > On Fri, Aug 9, 2024 at 6:12 PM Tommy Wu <tommy...@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.ch...@sifive.com>
> > > Signed-off-by: Tommy Wu <tommy...@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
> > >
