Re: [RFC PATCH 5/5] target/ppc: powerpc_excp: Move interrupt raising code to QOM

[Bruno Piazera Larsen]

On 01/06/2021 18:46, Fabiano Rosas wrote:

> This patch introduces a new way to dispatch the emulated interrupts in
> powerpc_excp. It leverages the QEMU object model to store the
> implementations for each interrupt and link them to their identifier
> from POWERPC_EXCP enum. The processor-specific code then uses this
> identifier to register which interrupts it supports.
>
> Interrupts now come out of the big switch in powerpc_excp into their
> own functions:
>
>   static void ppc_intr_system_reset(<args>)
>   {
>       /*
>        * Interrupt code. Sets any specific registers and MSR bits.
>        */
>   }
>   PPC_DEFINE_INTR(POWERPC_EXCP_RESET, system_reset, "System reset");
>
>   ^This line registers the interrupt with QOM.
>
> When we initialize the emulated processor, the correct set of
> interrupts is instantiated (pretty much like we already do):
>
>   static void init_excp_POWER9(CPUPPCState *env)
>   {
>       ppc_intr_add(env, 0x00000100, POWERPC_EXCP_RESET);
>       (...)
>   }
>
> When it comes the time to inject the interrupt:
>
>   static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>   {
>       (...)
>
>       intr = &env->entry_points[excp];
>       intr->setup_regs(<args>);    <-- ppc_intr_system_reset function
>
>       (...)
>       env->spr[srr0] = nip;
>       env->spr[srr1] = msr;
>
>       env->nip = intr->addr;
>       env->msr = new_msr;
>   }
>
> Some points to notice:
>
> - The structure for the new PPCInterrupt class object is stored
>   directly inside of CPUPPCState (env) so the translation code can
>   still access it linearly at an offset.
>
> - Some interrupts were being registered for P7/8/9/10 but were never
>   implemented (i.e. not in the powerpc_excp switch statement). They
>   are likely never triggered. We now get the benefit of QOM warning in
>   such cases:
>
>   qemu-system-ppc64: missing object type 'POWERPC_EXCP_SDOOR'
>   qemu-system-ppc64: missing object type 'POWERPC_EXCP_HV_MAINT'
>
> - The code currently allows for Program interrupts to be ignored and
>   System call interrupts to be directed to the vhyp hypercall code. I
>   have added an 'ignore' flag to deal with these two cases and return
>   early from powerpc_excp.
>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>

I don't see anything really wrong with the code itself, but this patch should probably be broken up into at least 2, one for the code motion and another for the ppc_intr'ification of the exception model.

>  target/ppc/cpu.h         |  29 +-
>  target/ppc/cpu_init.c    | 640 +++++++++++++++++++--------------------
>  target/ppc/excp_helper.c | 545 ++-------------------------------
>  target/ppc/interrupts.c  | 638 ++++++++++++++++++++++++++++++++++++++
>  target/ppc/machine.c     |   2 +-
>  target/ppc/meson.build   |   1 +
>  target/ppc/ppc_intr.h    |  55 ++++
>  target/ppc/translate.c   |   3 +-
>  8 files changed, 1066 insertions(+), 847 deletions(-)
>  create mode 100644 target/ppc/interrupts.c
>  create mode 100644 target/ppc/ppc_intr.h
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index b0934d9be4..012677965f 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -174,6 +174,33 @@ enum {
>      POWERPC_EXCP_TRAP          = 0x40,
>  };
>  
> +typedef struct PPCInterrupt PPCInterrupt;
> +typedef struct ppc_intr_args ppc_intr_args;
> +typedef void (*ppc_intr_fn_t)(PowerPCCPU *cpu, PPCInterrupt *intr,
> +                              int excp_model, ppc_intr_args *regs,
> +                              bool *ignore);
> +
> +struct ppc_intr_args {
> +    target_ulong nip;
> +    target_ulong msr;
> +    target_ulong new_nip;
> +    target_ulong new_msr;
> +    int sprn_srr0;
> +    int sprn_srr1;
> +    int sprn_asrr0;
> +    int sprn_asrr1;
> +    int lev;
> +};
> +
>     
    This part also has me a bit confused. Why define it first in
    excp_helper.c in the last patch just to move it to here now?
> +struct PPCInterrupt {
> +    Object parent;
> +
> +    int id;
> +    const char *name;
> +    target_ulong addr;
> +    ppc_intr_fn_t setup_regs;
> +};
> +
>  #define PPC_INPUT(env) ((env)->bus_model)
>  
>  /*****************************************************************************/
> @@ -1115,7 +1142,7 @@ struct CPUPPCState {
>      uint32_t irq_input_state;
>      void **irq_inputs;
>  
> -    target_ulong excp_vectors[POWERPC_EXCP_NB]; /* Exception vectors */
> +    PPCInterrupt entry_points[POWERPC_EXCP_NB];
>      target_ulong excp_prefix;
>      target_ulong ivor_mask;
>      target_ulong ivpr_mask;
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index d0411e7302..d91183357d 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -46,6 +46,7 @@
>  #include "helper_regs.h"
>  #include "internal.h"
>  #include "spr_tcg.h"
> +#include "ppc_intr.h"
>  
>  /* #define PPC_DEBUG_SPR */
>  /* #define USE_APPLE_GDB */
> @@ -2132,16 +2133,16 @@ static void register_8xx_sprs(CPUPPCState *env)
>  static void init_excp_4xx_real(CPUPPCState *env)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> -    env->excp_vectors[POWERPC_EXCP_CRITICAL] = 0x00000100;
> -    env->excp_vectors[POWERPC_EXCP_MCHECK]   = 0x00000200;
> -    env->excp_vectors[POWERPC_EXCP_EXTERNAL] = 0x00000500;
> -    env->excp_vectors[POWERPC_EXCP_ALIGN]    = 0x00000600;
> -    env->excp_vectors[POWERPC_EXCP_PROGRAM]  = 0x00000700;
> -    env->excp_vectors[POWERPC_EXCP_SYSCALL]  = 0x00000C00;
> -    env->excp_vectors[POWERPC_EXCP_PIT]      = 0x00001000;
> -    env->excp_vectors[POWERPC_EXCP_FIT]      = 0x00001010;
> -    env->excp_vectors[POWERPC_EXCP_WDT]      = 0x00001020;
> -    env->excp_vectors[POWERPC_EXCP_DEBUG]    = 0x00002000;
> +    ppc_intr_add(env, 0x00000100, POWERPC_EXCP_CRITICAL);
> +    ppc_intr_add(env, 0x00000200, POWERPC_EXCP_MCHECK);
> +    ppc_intr_add(env, 0x00000500, POWERPC_EXCP_EXTERNAL);
> +    ppc_intr_add(env, 0x00000600, POWERPC_EXCP_ALIGN);
> +    ppc_intr_add(env, 0x00000700, POWERPC_EXCP_PROGRAM);
> +    ppc_intr_add(env, 0x00000C00, POWERPC_EXCP_SYSCALL);
> +    ppc_intr_add(env, 0x00001000, POWERPC_EXCP_PIT);
> +    ppc_intr_add(env, 0x00001010, POWERPC_EXCP_FIT);
> +    ppc_intr_add(env, 0x00001020, POWERPC_EXCP_WDT);
> +    ppc_intr_add(env, 0x00002000, POWERPC_EXCP_DEBUG);
>      env->ivor_mask = 0x0000FFF0UL;
>      env->ivpr_mask = 0xFFFF0000UL;
>      /* Hardware reset vector */
>     
    <snip>
> @@ -2624,8 +2625,8 @@ static void init_excp_POWER9(CPUPPCState *env)
>      init_excp_POWER8(env);
>  
>  #if !defined(CONFIG_USER_ONLY)
> -    env->excp_vectors[POWERPC_EXCP_HVIRT]    = 0x00000EA0;
> -    env->excp_vectors[POWERPC_EXCP_SYSCALL_VECTORED] = 0x00017000;
> +    ppc_intr_add(env, 0x00000EA0, POWERPC_EXCP_HVIRT);
> +    ppc_intr_add(env, 0x00017000, POWERPC_EXCP_SYSCALL_VECTORED);
>  #endif
>  }
>     
    Not sure if this is possible, but if this bit can be done separately
    as an earlier patch, it would make reviewing a lot easier.
>  
> @@ -8375,13 +8376,8 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
>  #if !defined(CONFIG_USER_ONLY)
> -    int i;
>  
>      env->irq_inputs = NULL;
> -    /* Set all exception vectors to an invalid address */
> -    for (i = 0; i < POWERPC_EXCP_NB; i++) {
> -        env->excp_vectors[i] = (target_ulong)(-1ULL);
> -    }
>     
    We don't need to use this to set invalid values?
>      env->ivor_mask = 0x00000000;
>      env->ivpr_mask = 0x00000000;
>      /* Default MMU definitions */
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 12bf829c8f..26cbfab923 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -29,14 +29,6 @@
>  #endif
>  
>  /* #define DEBUG_OP */
> -/* #define DEBUG_SOFTWARE_TLB */
> -/* #define DEBUG_EXCEPTIONS */
> -
> -#ifdef DEBUG_EXCEPTIONS
> -#  define LOG_EXCP(...) qemu_log(__VA_ARGS__)
> -#else
> -#  define LOG_EXCP(...) do { } while (0)
> -#endif
>  
>  /*****************************************************************************/
>  /* Exception processing */
>     
    <snip>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index e16a2721e2..2c82bda8cc 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -951,7 +951,8 @@ void spr_write_excp_vector(DisasContext *ctx, int sprn, int gprn)
>      TCGv t0 = tcg_temp_new();
>      tcg_gen_ld_tl(t0, cpu_env, offsetof(CPUPPCState, ivor_mask));
>      tcg_gen_and_tl(t0, t0, cpu_gpr[gprn]);
> -    tcg_gen_st_tl(t0, cpu_env, offsetof(CPUPPCState, excp_vectors[sprn_offs]));
> +    tcg_gen_st_tl(t0, cpu_env, offsetof(CPUPPCState, entry_points[sprn_offs]) +
> +                  offsetof(PPCInterrupt, addr));
>      gen_store_spr(sprn, t0);
>      tcg_temp_free(t0);
>  }
>     
    Other than that, from what I can see, looks ok

--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer