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

["Cédric Le Goater" <clg@kaod.org>]

On 2/13/19 6:16 AM, David Gibson wrote:
> On Mon, Jan 28, 2019 at 10:46:23AM +0100, Cédric Le Goater wrote:
>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>
>> Adds support for the Hypervisor directed interrupts in addition to the
>> OS ones.
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  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.


Yes clearly. This is much better.

Thanks,

C. 

>>          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 |
>