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

[Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>]

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


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 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

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