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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 5/22/23 10:11, Tommy Wu wrote:
> 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                      | 51 +++++++++++++++++++
>   4 files changed, 67 insertions(+)
> 
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index 98e97810fd..00f1032086 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -112,6 +112,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(tlb_flush, void, env)
>   DEF_HELPER_1(tlb_flush_all, void, env)
> diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> index 73d5d1b045..e0698f9dfb 100644
> --- a/target/riscv/insn32.decode
> +++ b/target/riscv/insn32.decode
> @@ -111,6 +111,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 7c2837194c..0c1c2db5c6 100644
> --- a/target/riscv/insn_trans/trans_privileged.c.inc
> +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> @@ -108,6 +108,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, cpu_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 f563dc3981..2de3f102b5 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -359,6 +359,57 @@ target_ulong helper_mret(CPURISCVState *env)
>       return retpc;
>   }
>   
> +target_ulong helper_mnret(CPURISCVState *env)
> +{
> +    RISCVCPU *cpu = env_archcpu(env);

You're not using 'cpu' for anything other than reading cfg flags, so you can use the
riscv_cpu_cfg(env) inline nd avoid an env_archcpu() to get the 'cpu' pointer:

> +
> +    if (!cpu->cfg.ext_smrnmi) {

Here:

       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 (cpu->cfg.pmp &&

And here:

     if (riscv_cpu_cfg(env)->pmp &&



Everything else LGTM.



Daniel

> +        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
> +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> +    }
> +
> +    riscv_cpu_set_mode(env, prev_priv);
> +    env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, true);
> +
> +    target_ulong prev_virt = get_field(env->mnstatus, MNSTATUS_MNPV);
> +
> +    /*
> +     * 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)) {
> +        if (prev_virt) {
> +            riscv_cpu_swap_hypervisor_regs(env);
> +        }
> +
> +        riscv_cpu_set_virt_enabled(env, prev_virt);
> +    }
> +
> +    return retpc;
> +}
> +
>   void helper_wfi(CPURISCVState *env)
>   {
>       CPUState *cs = env_cpu(env);