Re: [PATCH v3 3/4] target/riscv: Handle Smrnmi interrupt and exception.

[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/cpu_helper.c | 60 +++++++++++++++++++++++++++++++++++----
>   1 file changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index cc7898f103..7bdea0d2ca 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -405,6 +405,19 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *env)
>   {
>       int virq;
>       uint64_t irqs, pending, mie, hsie, vsie;
> +    RISCVCPU *cpu = env_archcpu(env);
> +
> +    /* Priority: RNMI > Other interrupt. */
> +    if (cpu->cfg.ext_smrnmi) {

Since you're using "cpu" just to read a cfg flag you can use the 'riscv_cpu_cfg(env)'
inline to avoid a call to env_archcpu():

>       int virq;
>       uint64_t irqs, pending, mie, hsie, vsie;
> +
> +    /* Priority: RNMI > Other interrupt. */
> +    if (riscv_cpu_cfg(env)->ext_smrnmi) {


> +        /* If mnstatus.NMIE == 0, all interrupts are disabled. */
> +        if (!get_field(env->mnstatus, MNSTATUS_NMIE)) {
> +            return RISCV_EXCP_NONE;
> +        }
> +
> +        if (env->rnmip) {
> +            return ctz64(env->rnmip); /* since non-zero */
> +        }
> +    }
>   
>       /* Determine interrupt enable state of all privilege modes */
>       if (env->virt_enabled) {
> @@ -451,7 +464,9 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *env)
>   
>   bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>   {
> -    if (interrupt_request & CPU_INTERRUPT_HARD) {
> +    uint32_t mask = CPU_INTERRUPT_HARD | CPU_INTERRUPT_RNMI;
> +
> +    if (interrupt_request & mask) {
>           RISCVCPU *cpu = RISCV_CPU(cs);
>           CPURISCVState *env = &cpu->env;
>           int interruptno = riscv_cpu_local_irq_pending(env);
> @@ -1613,6 +1628,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>       CPURISCVState *env = &cpu->env;
>       bool write_gva = false;
>       uint64_t s;
> +    int mode;
>   
>       /*
>        * cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide
> @@ -1625,6 +1641,22 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>       target_ulong tinst = 0;
>       target_ulong htval = 0;
>       target_ulong mtval2 = 0;
> +    bool nmi_execp = false;
> +
> +    if (cpu->cfg.ext_smrnmi) {
> +        if (env->rnmip && async) {

You're not using an 'else' clause for this if, so I suggest avoiding this nested
'if' by doing everything in the same conditional:

> +    if (cpu->cfg.ext_smrnmi && env->rnmip && async) {

As a bonus you'll also avoid an extra indent level.


Thanks,


Daniel



> +            env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, false);
> +            env->mnstatus = set_field(env->mnstatus, MNSTATUS_MNPV,
> +                                      env->virt_enabled);
> +            env->mnstatus = set_field(env->mnstatus, MNSTATUS_MNPP,
> +                                      env->priv);
> +            env->mncause = cause | ((target_ulong)1U << (TARGET_LONG_BITS - 1));
> +            env->mnepc = env->pc;
> +            env->pc = env->rnmi_irqvec;
> +            riscv_cpu_set_mode(env, PRV_M);
> +            goto handled;
> +        }
> +    }
>   
>       if  (cause == RISCV_EXCP_SEMIHOST) {
>           do_common_semihosting(cs);
> @@ -1711,8 +1743,20 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>                     __func__, env->mhartid, async, cause, env->pc, tval,
>                     riscv_cpu_get_trap_name(cause, async));
>   
> -    if (env->priv <= PRV_S &&
> -            cause < TARGET_LONG_BITS && ((deleg >> cause) & 1)) {
> +    mode = env->priv <= PRV_S &&
> +        cause < TARGET_LONG_BITS && ((deleg >> cause) & 1) ? PRV_S : PRV_M;
> +
> +    /*
> +     * If the hart encounters an exception while executing in M-mode,
> +     * with the mnstatus.NMIE bit clear, the program counter is set to
> +     * the RNMI exception trap handler address.
> +     */
> +    nmi_execp = cpu->cfg.ext_smrnmi &&
> +                !get_field(env->mnstatus, MNSTATUS_NMIE) &&
> +                !async &&
> +                mode == PRV_M;
> +
> +    if (mode == PRV_S) {
>           /* handle the trap in S-mode */
>           if (riscv_has_ext(env, RVH)) {
>               uint64_t hdeleg = async ? env->hideleg : env->hedeleg;
> @@ -1787,8 +1831,12 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>           env->mtval = tval;
>           env->mtval2 = mtval2;
>           env->mtinst = tinst;
> -        env->pc = (env->mtvec >> 2 << 2) +
> -                  ((async && (env->mtvec & 3) == 1) ? cause * 4 : 0);
> +        if (cpu->cfg.ext_smrnmi && nmi_execp) {
> +            env->pc = env->rnmi_excpvec;
> +        } else {
> +            env->pc = (env->mtvec >> 2 << 2) +
> +                      ((async && (env->mtvec & 3) == 1) ? cause * 4 : 0);
> +        }
>           riscv_cpu_set_mode(env, PRV_M);
>       }
>   
> @@ -1801,6 +1849,8 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>   
>       env->two_stage_lookup = false;
>       env->two_stage_indirect_lookup = false;
> +
> +handled:
>   #endif
>       cs->exception_index = RISCV_EXCP_NONE; /* mark handled to qemu */
>   }