Re: [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine

[Cédric Le Goater]

On 06/13/2018 07:00 AM, David Gibson wrote:
> On Fri, May 18, 2018 at 06:44:04PM +0200, Cédric Le Goater wrote:
>> This proposal moves all the related IRQ routines of the sPAPR machine
>> behind a class interface to prepare for future changes in the IRQ
>> controller model. First of which is a reorganization of the IRQ number
>> space layout and a second, coming later, will be to integrate the
>> support for the new POWER9 XIVE IRQ controller.
>>
>> The new interface defines a set of fixed IRQ number ranges, for each
>> IRQ type, in which devices allocate the IRQ numbers they need
>> depending on a unique device index. Here is the layout :
>>
>> SPAPR_IRQ_IPI0x0/*  1 IRQ per CPU  */
>> SPAPR_IRQ_EPOW   0x1000 /*  1 IRQ per device   */
>> SPAPR_IRQ_HOTPLUG0x1001 /*  1 IRQ per device   */
>> SPAPR_IRQ_VIO0x1100 /*  1 IRQ per device   */
>> SPAPR_IRQ_PCI_LSI0x1200 /*  4 IRQs per device  */
>> SPAPR_IRQ_PCI_MSI0x1400 /* 1K IRQs per device  */
>>
>> The IPI range is reserved for future use when XIVE support
>> comes in.
>>
>> The routines of this interface encompass the previous needs and the
>> new ones and seem complex but the provided IRQ backend should
>> implement what we have today without any functional changes.
>>
>> Each device model is modified to take the new interface into account
>> using the IRQ range/type definitions and a device index. A part from
>> the VIO devices, lacking an id, the changes are relatively simple.
> 
> I find your use of "back end" vs. "front end" in this patch and the
> next kind of confusing.

This is the the front end, interface used by the machine and devices :

  int spapr_irq_assign(sPAPRMachineState *spapr, uint32_t range, uint32_t irq,
   Error **errp);
  int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
Error **errp);
  int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
  uint32_t index, int num, bool align, Error **errp);
  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num, Error **errp);
  qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);

and the backend, which can be different depending on the machine level, 
old vs. new layout, or on depending on the interrupt controller.

  typedef struct sPAPRIrq {
uint32_tnr_irqs;
const sPAPRPIrqRange *ranges;

void (*init)(sPAPRMachineState *spapr, Error **errp);
int (*assign)(sPAPRMachineState *spapr, uint32_t range, uint32_t irq,
  Error **errp);
int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
 Error **errp);
int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range,
   uint32_t index, int num, bool align, Error **errp);
void (*free)(sPAPRMachineState *spapr, int irq, int num, Error **errp);
qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
void (*print_info)(sPAPRMachineState *spapr, Monitor *mon);
} sPAPRIrq;


>> Signed-off-by: Cédric Le Goater 
>> ---
>>  include/hw/ppc/spapr.h |  10 +-
>>  include/hw/ppc/spapr_irq.h |  46 +
>>  hw/ppc/spapr.c | 177 +-
>>  hw/ppc/spapr_events.c  |   7 +-
>>  hw/ppc/spapr_irq.c | 233 
>> +
>>  hw/ppc/spapr_pci.c |  21 +++-
>>  hw/ppc/spapr_vio.c |   5 +-
>>  hw/ppc/Makefile.objs   |   2 +-
>>  8 files changed, 308 insertions(+), 193 deletions(-)
>>  create mode 100644 include/hw/ppc/spapr_irq.h
>>  create mode 100644 hw/ppc/spapr_irq.c
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 2cfdfdd67eaf..4eb212b16a51 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -3,10 +3,10 @@
>>  
>>  #include "sysemu/dma.h"
>>  #include "hw/boards.h"
>> -#include "hw/ppc/xics.h"
>>  #include "hw/ppc/spapr_drc.h"
>>  #include "hw/mem/pc-dimm.h"
>>  #include "hw/ppc/spapr_ovec.h"
>> +#include "hw/ppc/spapr_irq.h"
>>  
>>  struct VIOsPAPRBus;
>>  struct sPAPRPHBState;
>> @@ -104,6 +104,7 @@ struct sPAPRMachineClass {
>>unsigned n_dma, uint32_t *liobns, Error **errp);
>>  sPAPRResizeHPT resize_hpt_default;
>>  sPAPRCapabilities default_caps;
>> +sPAPRIrq *irq;
>>  };
>>  
>>  /**
>> @@ -773,13 +774,6 @@ int spapr_get_vcpu_id(PowerPCCPU *cpu);
>>  void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
>>  PowerPCCPU *spapr_find_cpu(int vcpu_id);
>>  
>> -int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp);
>> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
>> -  bool align, Error **errp);
>> -void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
>> -qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
>> -
>> -
>>  int spapr_caps_pre_load(void *opaque);
>>  

Re: [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine

[David Gibson]

On Fri, May 18, 2018 at 06:44:04PM +0200, Cédric Le Goater wrote:
> This proposal moves all the related IRQ routines of the sPAPR machine
> behind a class interface to prepare for future changes in the IRQ
> controller model. First of which is a reorganization of the IRQ number
> space layout and a second, coming later, will be to integrate the
> support for the new POWER9 XIVE IRQ controller.
> 
> The new interface defines a set of fixed IRQ number ranges, for each
> IRQ type, in which devices allocate the IRQ numbers they need
> depending on a unique device index. Here is the layout :
> 
> SPAPR_IRQ_IPI0x0/*  1 IRQ per CPU  */
> SPAPR_IRQ_EPOW   0x1000 /*  1 IRQ per device   */
> SPAPR_IRQ_HOTPLUG0x1001 /*  1 IRQ per device   */
> SPAPR_IRQ_VIO0x1100 /*  1 IRQ per device   */
> SPAPR_IRQ_PCI_LSI0x1200 /*  4 IRQs per device  */
> SPAPR_IRQ_PCI_MSI0x1400 /* 1K IRQs per device  */
> 
> The IPI range is reserved for future use when XIVE support
> comes in.
> 
> The routines of this interface encompass the previous needs and the
> new ones and seem complex but the provided IRQ backend should
> implement what we have today without any functional changes.
> 
> Each device model is modified to take the new interface into account
> using the IRQ range/type definitions and a device index. A part from
> the VIO devices, lacking an id, the changes are relatively simple.

I find your use of "back end" vs. "front end" in this patch and the
next kind of confusing.

> Signed-off-by: Cédric Le Goater 
> ---
>  include/hw/ppc/spapr.h |  10 +-
>  include/hw/ppc/spapr_irq.h |  46 +
>  hw/ppc/spapr.c | 177 +-
>  hw/ppc/spapr_events.c  |   7 +-
>  hw/ppc/spapr_irq.c | 233 
> +
>  hw/ppc/spapr_pci.c |  21 +++-
>  hw/ppc/spapr_vio.c |   5 +-
>  hw/ppc/Makefile.objs   |   2 +-
>  8 files changed, 308 insertions(+), 193 deletions(-)
>  create mode 100644 include/hw/ppc/spapr_irq.h
>  create mode 100644 hw/ppc/spapr_irq.c
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2cfdfdd67eaf..4eb212b16a51 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -3,10 +3,10 @@
>  
>  #include "sysemu/dma.h"
>  #include "hw/boards.h"
> -#include "hw/ppc/xics.h"
>  #include "hw/ppc/spapr_drc.h"
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/ppc/spapr_ovec.h"
> +#include "hw/ppc/spapr_irq.h"
>  
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
> @@ -104,6 +104,7 @@ struct sPAPRMachineClass {
>unsigned n_dma, uint32_t *liobns, Error **errp);
>  sPAPRResizeHPT resize_hpt_default;
>  sPAPRCapabilities default_caps;
> +sPAPRIrq *irq;
>  };
>  
>  /**
> @@ -773,13 +774,6 @@ int spapr_get_vcpu_id(PowerPCCPU *cpu);
>  void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
>  PowerPCCPU *spapr_find_cpu(int vcpu_id);
>  
> -int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp);
> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> -  bool align, Error **errp);
> -void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
> -qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> -
> -
>  int spapr_caps_pre_load(void *opaque);
>  int spapr_caps_pre_save(void *opaque);
>  
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> new file mode 100644
> index ..caf4c33d4cec
> --- /dev/null
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -0,0 +1,46 @@
> +/*
> + * QEMU PowerPC sPAPR IRQ backend definitions
> + *
> + * Copyright (c) 2018, IBM Corporation.
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +#ifndef HW_SPAPR_IRQ_H
> +#define HW_SPAPR_IRQ_H
> +
> +#include "hw/ppc/xics.h"
> +
> +/*
> + * IRQ ranges
> + */
> +#define SPAPR_IRQ_IPI0x0 /* 1 IRQ per CPU */
> +#define SPAPR_IRQ_EPOW   0x1000  /* 1 IRQ per device */
> +#define SPAPR_IRQ_HOTPLUG0x1001  /* 1 IRQ per device */
> +#define SPAPR_IRQ_VIO0x1100  /* 1 IRQ per device */
> +#define SPAPR_IRQ_PCI_LSI0x1200  /* 4 IRQs per device */
> +#define SPAPR_IRQ_PCI_MSI0x1400  /* 1K IRQs per device covered by
> +  * a bitmap allocator */

These look like they belong in the next patch with the fixed allocations.

> +typedef struct sPAPRIrq {

Much of this structure doesn't make sense to me.  AIUI, the idea is
that this method structure will vary with the xics vs. xive backend,
yes?  Comments below are based on that assumption.

> +uint32_tnr_irqs;
> +
> +void (*init)(sPAPRMachineState *spapr, Error **errp);
> +int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> + Error **errp);

'range' has no 

Re: [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine

[Greg Kurz]

On Fri, 18 May 2018 18:44:04 +0200
Cédric Le Goater  wrote:

> This proposal moves all the related IRQ routines of the sPAPR machine
> behind a class interface to prepare for future changes in the IRQ
> controller model. First of which is a reorganization of the IRQ number
> space layout and a second, coming later, will be to integrate the
> support for the new POWER9 XIVE IRQ controller.
> 
> The new interface defines a set of fixed IRQ number ranges, for each
> IRQ type, in which devices allocate the IRQ numbers they need
> depending on a unique device index. Here is the layout :
> 
> SPAPR_IRQ_IPI0x0/*  1 IRQ per CPU  */
> SPAPR_IRQ_EPOW   0x1000 /*  1 IRQ per device   */
> SPAPR_IRQ_HOTPLUG0x1001 /*  1 IRQ per device   */
> SPAPR_IRQ_VIO0x1100 /*  1 IRQ per device   */
> SPAPR_IRQ_PCI_LSI0x1200 /*  4 IRQs per device  */
> SPAPR_IRQ_PCI_MSI0x1400 /* 1K IRQs per device  */
> 
> The IPI range is reserved for future use when XIVE support
> comes in.
> 
> The routines of this interface encompass the previous needs and the
> new ones and seem complex but the provided IRQ backend should
> implement what we have today without any functional changes.
> 
> Each device model is modified to take the new interface into account
> using the IRQ range/type definitions and a device index. A part from
> the VIO devices, lacking an id, the changes are relatively simple.
> 
> Signed-off-by: Cédric Le Goater 
> ---
>  include/hw/ppc/spapr.h |  10 +-
>  include/hw/ppc/spapr_irq.h |  46 +
>  hw/ppc/spapr.c | 177 +-
>  hw/ppc/spapr_events.c  |   7 +-
>  hw/ppc/spapr_irq.c | 233 
> +
>  hw/ppc/spapr_pci.c |  21 +++-
>  hw/ppc/spapr_vio.c |   5 +-
>  hw/ppc/Makefile.objs   |   2 +-
>  8 files changed, 308 insertions(+), 193 deletions(-)
>  create mode 100644 include/hw/ppc/spapr_irq.h
>  create mode 100644 hw/ppc/spapr_irq.c
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2cfdfdd67eaf..4eb212b16a51 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -3,10 +3,10 @@
>  
>  #include "sysemu/dma.h"
>  #include "hw/boards.h"
> -#include "hw/ppc/xics.h"
>  #include "hw/ppc/spapr_drc.h"
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/ppc/spapr_ovec.h"
> +#include "hw/ppc/spapr_irq.h"
>  
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
> @@ -104,6 +104,7 @@ struct sPAPRMachineClass {
>unsigned n_dma, uint32_t *liobns, Error **errp);
>  sPAPRResizeHPT resize_hpt_default;
>  sPAPRCapabilities default_caps;
> +sPAPRIrq *irq;
>  };
>  
>  /**
> @@ -773,13 +774,6 @@ int spapr_get_vcpu_id(PowerPCCPU *cpu);
>  void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
>  PowerPCCPU *spapr_find_cpu(int vcpu_id);
>  
> -int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp);
> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> -  bool align, Error **errp);
> -void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
> -qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> -
> -
>  int spapr_caps_pre_load(void *opaque);
>  int spapr_caps_pre_save(void *opaque);
>  
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> new file mode 100644
> index ..caf4c33d4cec
> --- /dev/null
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -0,0 +1,46 @@
> +/*
> + * QEMU PowerPC sPAPR IRQ backend definitions
> + *
> + * Copyright (c) 2018, IBM Corporation.
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +#ifndef HW_SPAPR_IRQ_H
> +#define HW_SPAPR_IRQ_H
> +
> +#include "hw/ppc/xics.h"
> +
> +/*
> + * IRQ ranges
> + */
> +#define SPAPR_IRQ_IPI0x0 /* 1 IRQ per CPU */
> +#define SPAPR_IRQ_EPOW   0x1000  /* 1 IRQ per device */
> +#define SPAPR_IRQ_HOTPLUG0x1001  /* 1 IRQ per device */
> +#define SPAPR_IRQ_VIO0x1100  /* 1 IRQ per device */
> +#define SPAPR_IRQ_PCI_LSI0x1200  /* 4 IRQs per device */
> +#define SPAPR_IRQ_PCI_MSI0x1400  /* 1K IRQs per device covered by
> +  * a bitmap allocator */
> +
> +typedef struct sPAPRIrq {
> +uint32_tnr_irqs;
> +
> +void (*init)(sPAPRMachineState *spapr, Error **errp);
> +int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> + Error **errp);
> +int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range,
> +   uint32_t index, int num, bool align, Error **errp);
> +void (*free)(sPAPRMachineState *spapr, int irq, int num, Error **errp);
> +qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
> +} sPAPRIrq;
> +
> +extern sPAPRIrq spapr_irq_default;
> +
>