Re: [PATCH 08/11 v2] target/riscv: Update interrupt handling in CLIC mode

[Alistair Francis]

On Tue, Aug 20, 2024 at 2:14 AM Ian Brockbank <Ian.Brockbank@cirrus.com> wrote:
>
> From: Ian Brockbank <ian.brockbank@cirrus.com>
>
> Decode CLIC interrupt information from exccode, includes interrupt
> privilege mode, interrupt level, and irq number.
>
> Then update CSRs xcause, xstatus, xepc, xintstatus and jump to
> correct PC according to the CLIC specification.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> Signed-off-by: Ian Brockbank <ian.brockbank@cirrus.com>
> ---
>  target/riscv/cpu_helper.c | 129 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 119 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 395a1d9140..944afb68d2 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -24,6 +24,7 @@
>  #include "internals.h"
>  #include "pmu.h"
>  #include "exec/exec-all.h"
> +#include "exec/cpu_ldst.h"
>  #include "exec/page-protection.h"
>  #include "instmap.h"
>  #include "tcg/tcg-op.h"
> @@ -33,6 +34,7 @@
>  #include "cpu_bits.h"
>  #include "debug.h"
>  #include "tcg/oversized-guest.h"
> +#include "hw/intc/riscv_clic.h"
>
>  int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
>  {
> @@ -428,6 +430,20 @@ int riscv_cpu_vsirq_pending(CPURISCVState *env)
>                                      (irqs | irqs_f_vs), env->hviprio);
>  }
>
> +static int riscv_cpu_local_irq_mode_enabled(CPURISCVState *env, int mode)
> +{
> +    switch (mode) {
> +    case PRV_M:
> +        return env->priv < PRV_M ||
> +            (env->priv == PRV_M && get_field(env->mstatus, MSTATUS_MIE));
> +    case PRV_S:
> +        return env->priv < PRV_S ||
> +            (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_SIE));
> +    default:
> +        return false;
> +    }
> +}
> +
>  static int riscv_cpu_local_irq_pending(CPURISCVState *env)
>  {
>      uint64_t irqs, pending, mie, hsie, vsie, irqs_f, irqs_f_vs;
> @@ -506,6 +522,18 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int 
> interrupt_request)
>              return true;
>          }
>      }
> +    if (interrupt_request & CPU_INTERRUPT_CLIC) {
> +        RISCVCPU *cpu = RISCV_CPU(cs);
> +        CPURISCVState *env = &cpu->env;
> +        int mode = get_field(env->exccode, RISCV_EXCP_CLIC_MODE);
> +        int enabled = riscv_cpu_local_irq_mode_enabled(env, mode);
> +        if (enabled) {
> +            cs->exception_index = RISCV_EXCP_CLIC | env->exccode;
> +            cs->interrupt_request = cs->interrupt_request & 
> ~CPU_INTERRUPT_CLIC;
> +            riscv_cpu_do_interrupt(cs);
> +            return true;
> +        }
> +    }
>      return false;
>  }
>
> @@ -1641,6 +1669,60 @@ static target_ulong 
> riscv_transformed_insn(CPURISCVState *env,
>      return xinsn;
>  }
>
> +static target_ulong riscv_intr_pc(CPURISCVState *env, target_ulong tvec,
> +                                  target_ulong tvt, bool async,
> +                                  int cause, int mode)
> +{
> +    int mode1 = tvec & XTVEC_MODE;
> +    int mode2 = tvec & XTVEC_FULL_MODE;
> +

This is going to need extension checks

> +    if (!async) {
> +        return tvec & XTVEC_OBASE;
> +    }
> +    /* bits [1:0] encode mode; 0 = direct, 1 = vectored, 2 >= reserved */
> +    switch (mode1) {
> +    case XTVEC_CLINT_DIRECT:
> +        return tvec & XTVEC_OBASE;
> +    case XTVEC_CLINT_VECTORED:
> +        return (tvec & XTVEC_OBASE) + cause * 4;
> +    default:
> +        if (env->clic && (mode2 == XTVEC_CLIC)) {
> +            /* Non-vectored, clicintattr[i].shv = 0 || cliccfg.nvbits = 0 */
> +            if (!riscv_clic_shv_interrupt(env->clic, cause)) {
> +                /* NBASE = mtvec[XLEN-1:6]<<6 */
> +                return tvec & XTVEC_NBASE;
> +            } else {
> +                /*
> +                 * pc := M[TBASE + XLEN/8 * exccode)] & ~1,
> +                 * TBASE = mtvt[XLEN-1:6]<<6
> +                 */
> +                int size = TARGET_LONG_BITS / 8;
> +                target_ulong tbase = (tvt & XTVEC_NBASE) + size * cause;
> +                void *host = tlb_vaddr_to_host(env, tbase, MMU_DATA_LOAD, 
> mode);

This doesn't look right.

I think you want cpu_ l*_mmuidx_ra(). That will raise an exception on
an access failure

> +                if (host != NULL) {
> +                    target_ulong new_pc = tbase;
> +                    if (!riscv_clic_use_jump_table(env->clic)) {
> +                        /*
> +                         * Standard CLIC: the vector entry is a function 
> pointer
> +                         * so look up the destination.
> +                         */
> +                        new_pc = ldn_p(host, size);
> +                        host = tlb_vaddr_to_host(env, new_pc,
> +                                                 MMU_INST_FETCH, mode);

At xtvt is the base address of a table of pointers, you also then want
to call cpu_ l*_mmuidx_ra() a second time and that value should be
returned as the next PC

> +                    }
> +                    if (host) {
> +                        return new_pc;
> +                    }
> +                }
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "CLIC: load trap handler error!\n");
> +                exit(1);

Don't allow the guest to exit. Print a guest error and keep going

Alistair