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 <faro...@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>

Reply via email to