Re: [Qemu-devel] [PATCH 17/19] target/ppc: Add POWER9 external interrupt model

[David Gibson]

On Mon, Jan 28, 2019 at 10:46:23AM +0100, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt <address@hidden>
> 
> Adds support for the Hypervisor directed interrupts in addition to the
> OS ones.
> 
> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>  include/hw/ppc/ppc.h            |  2 ++
>  target/ppc/cpu-qom.h            |  2 ++
>  target/ppc/cpu.h                |  7 +++++++
>  hw/intc/xics.c                  |  1 +
>  hw/intc/xive.c                  |  2 +-
>  hw/ppc/ppc.c                    | 14 ++++++++++++++
>  target/ppc/translate_init.inc.c |  4 ++--
>  7 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> index daaa04a22dbf..4bdcb8bacd4e 100644
> --- a/include/hw/ppc/ppc.h
> +++ b/include/hw/ppc/ppc.h
> @@ -74,6 +74,7 @@ static inline void ppc40x_irq_init(PowerPCCPU *cpu) {}
>  static inline void ppc6xx_irq_init(PowerPCCPU *cpu) {}
>  static inline void ppc970_irq_init(PowerPCCPU *cpu) {}
>  static inline void ppcPOWER7_irq_init(PowerPCCPU *cpu) {}
> +static inline void ppcPOWER9_irq_init(PowerPCCPU *cpu) {}
>  static inline void ppce500_irq_init(PowerPCCPU *cpu) {}
>  #else
>  void ppc40x_irq_init(PowerPCCPU *cpu);
> @@ -81,6 +82,7 @@ void ppce500_irq_init(PowerPCCPU *cpu);
>  void ppc6xx_irq_init(PowerPCCPU *cpu);
>  void ppc970_irq_init(PowerPCCPU *cpu);
>  void ppcPOWER7_irq_init(PowerPCCPU *cpu);
> +void ppcPOWER9_irq_init(PowerPCCPU *cpu);
>  #endif
>  
>  /* PPC machines for OpenBIOS */
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index 7ff8b2d68632..079fbb9d8718 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -142,6 +142,8 @@ enum powerpc_input_t {
>      PPC_FLAGS_INPUT_970,
>      /* PowerPC POWER7 bus               */
>      PPC_FLAGS_INPUT_POWER7,
> +    /* PowerPC POWER9 bus               */
> +    PPC_FLAGS_INPUT_POWER9,
>      /* PowerPC 401 bus                  */
>      PPC_FLAGS_INPUT_401,
>      /* Freescale RCPU bus               */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 385d33bd37ff..cc41ae6f3017 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2322,6 +2322,13 @@ enum {
>       * them */
>      POWER7_INPUT_NB,
>  };
> +
> +enum {
> +    /* POWER9 input pins */
> +    POWER9_INPUT_INT        = 0, /* Must match POWER7_INPUT_INT */

Rather than having this vital comment here...

> +    POWER9_INPUT_HINT       = 1,
> +    POWER9_INPUT_NB,
> +};
>  #endif
>  
>  /* Hardware exceptions definitions */
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 16e8ffa2aaf7..1f786175168f 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -342,6 +342,7 @@ static void icp_realize(DeviceState *dev, Error **errp)
>      env = &cpu->env;
>      switch (PPC_INPUT(env)) {
>      case PPC_FLAGS_INPUT_POWER7:
> +    case PPC_FLAGS_INPUT_POWER9: /* For SPAPR xics emulation */

.. you could just split the cases here.  It's a little more verbose,
but it's more robust.

>          icp->output = env->irq_inputs[POWER7_INPUT_INT];
>          break;
>  
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 0e0e1dc9c1b7..119bb02d345d 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -553,7 +553,7 @@ static void xive_tctx_realize(DeviceState *dev, Error 
> **errp)
>  
>      env = &cpu->env;
>      switch (PPC_INPUT(env)) {
> -    case PPC_FLAGS_INPUT_POWER7:
> +    case PPC_FLAGS_INPUT_POWER9:

And here, of course.

>          tctx->output = env->irq_inputs[POWER7_INPUT_INT];
>          break;
>  
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 608405f6f2ca..1a5e087c9b1e 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -289,6 +289,12 @@ static void power7_set_irq(void *opaque, int pin, int 
> level)
>                  __func__, level);
>          ppc_set_irq(cpu, PPC_INTERRUPT_EXT, level);
>          break;
> +    case POWER9_INPUT_HINT:

Having a POWER9 specific case in a function named for power7 is pretty
odd.  Best to split these as well, I think.

> +        /* Level sensitive - active high */
> +        LOG_IRQ("%s: set the external IRQ state to %d\n",
> +                __func__, level);
> +        ppc_set_irq(cpu, PPC_INTERRUPT_HVIRT, level);
> +        break;
>      default:
>          /* Unknown pin - do nothing */
>          LOG_IRQ("%s: unknown IRQ pin %d\n", __func__, pin);
> @@ -308,6 +314,14 @@ void ppcPOWER7_irq_init(PowerPCCPU *cpu)
>      env->irq_inputs = (void **)qemu_allocate_irqs(&power7_set_irq, cpu,
>                                                    POWER7_INPUT_NB);
>  }
> +
> +void ppcPOWER9_irq_init(PowerPCCPU *cpu)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    env->irq_inputs = (void **)qemu_allocate_irqs(&power7_set_irq, cpu,
> +                                                  POWER9_INPUT_NB);
> +}
>  #endif /* defined(TARGET_PPC64) */
>  
>  /* PowerPC 40x internal IRQ controller */
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 6ffa4a8fe0fa..7f25215f0192 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -8793,7 +8793,7 @@ static void init_proc_POWER9(CPUPPCState *env)
>  
>      /* Allocate hardware IRQ controller */
>      init_excp_POWER9(env);
> -    ppcPOWER7_irq_init(ppc_env_get_cpu(env));
> +    ppcPOWER9_irq_init(ppc_env_get_cpu(env));
>  }
>  
>  static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, uint32_t pvr)
> @@ -8920,7 +8920,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>      pcc->radix_page_info = &POWER9_radix_page_info;
>  #endif
>      pcc->excp_model = POWERPC_EXCP_POWER9;
> -    pcc->bus_model = PPC_FLAGS_INPUT_POWER7;
> +    pcc->bus_model = PPC_FLAGS_INPUT_POWER9;
>      pcc->bfd_mach = bfd_mach_ppc64;
>      pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
>                   POWERPC_FLAG_BE | POWERPC_FLAG_PMM |

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature