Re: [RFC PATCH 11/11] target/riscv: Update interrupt return in CLIC mode

[Frank Chang <frank.chang@sifive.com>]

[-- Attachment #1: Type: text/plain, Size: 3613 bytes --]

LIU Zhiwei <zhiwei_liu@c-sky.com> 於 2021年4月9日 週五 下午3:55寫道：

> When a vectored interrupt is selected and serviced, the hardware will
> automatically clear the corresponding pending bit in edge-triggered mode.
> This may lead to a lower priviledge interrupt pending forever.
>
> Therefore when interrupts return, pull a pending interrupt to service.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
>  target/riscv/op_helper.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 1eddcb94de..42563b22ba 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -24,6 +24,10 @@
>  #include "exec/exec-all.h"
>  #include "exec/helper-proto.h"
>
> +#if !defined(CONFIG_USER_ONLY)
> +#include "hw/intc/riscv_clic.h"
> +#endif
> +
>  /* Exceptions processing helpers */
>  void QEMU_NORETURN riscv_raise_exception(CPURISCVState *env,
>                                            uint32_t exception, uintptr_t
> pc)
> @@ -130,6 +134,17 @@ target_ulong helper_sret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
>          mstatus = set_field(mstatus, MSTATUS_SPIE, 1);
>          mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
>          env->mstatus = mstatus;
> +
> +        if (riscv_clic_is_clic_mode(env)) {
> +            CPUState *cs = env_cpu(env);
> +            target_ulong spil = get_field(env->scause, SCAUSE_SPIL);
> +            env->mintstatus = set_field(env->mintstatus, MINTSTATUS_SIL,
> spil);
> +            env->scause = set_field(env->scause, SCAUSE_SPIE, 0);
> +            env->scause = set_field(env->scause, SCAUSE_SPP, PRV_U);
> +            qemu_mutex_lock_iothread();
> +            riscv_clic_get_next_interrupt(env->clic, cs->cpu_index);
> +            qemu_mutex_unlock_iothread();
> +        }
>      }
>
>      riscv_cpu_set_mode(env, prev_priv);
> @@ -172,6 +187,16 @@ target_ulong helper_mret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
>          riscv_cpu_set_virt_enabled(env, prev_virt);
>      }
>
> +    if (riscv_clic_is_clic_mode(env)) {
> +        CPUState *cs = env_cpu(env);
> +        target_ulong mpil = get_field(env->mcause, MCAUSE_MPIL);
> +        env->mintstatus = set_field(env->mintstatus, MINTSTATUS_MIL,
> mpil);
> +        env->mcause = set_field(env->mcause, MCAUSE_MPIE, 0);
> +        env->mcause = set_field(env->mcause, MCAUSE_MPP, PRV_U);
> +        qemu_mutex_lock_iothread();
> +        riscv_clic_get_next_interrupt(env->clic, cs->cpu_index);
> +        qemu_mutex_unlock_iothread();
> +    }
>      return retpc;
>  }
>
> --
> 2.25.1
>
>
>
A little note here.

According to spec, for nesting interrupts:
  To take advantage of hardware preemption in the new CLIC,
  inline handlers must save and restore xepc and xcause before enabling
interrupts.
  (Section 9.2)

However, xstatus.xpp will be set to U-mode when xret instruction is
executed.
Which will incorrectly switch to U-mode when executing xret instruction
second time in first ISR.
E.g.

ISR 1 --------------------------------------------------------- xret =>
Current privilege is incorrectly set to U-mode.
                ISR 2 --------------- xret => xstatus.xpp is set to U-mode.

Therefore, in our SiFive CLIC hardware implementation.
xstatus.xpp is set to the privilege mode when xret instruction is executed
(i.e. current privilege mode) under CLIC mode.
But this behavior is not documented in CLIC spec explicitly.

Regards,
Frank Chang

[-- Attachment #2: Type: text/html, Size: 4528 bytes --]