diff mbox series

[09/20] spapr: Clarify and fix handling of nr_irqs

Message ID 20190925064534.19155-10-david@gibson.dropbear.id.au (mailing list archive)
State New, archived
Headers show
Series spapr: IRQ subsystem cleanups | expand

Commit Message

David Gibson Sept. 25, 2019, 6:45 a.m. UTC
Both the XICS and XIVE interrupt backends have a "nr-irqs" property, but
it means slightly different things.  For XICS (or, strictly, the ICS) it
indicates the number of "real" external IRQs.  Those start at XICS_IRQ_BASE
(0x1000) and don't include the special IPI vector.  For XIVE, however, it
includes the whole IRQ space, including XIVE's many IPI vectors.

The spapr code currently doesn't handle this sensibly, with the nr_irqs
value in SpaprIrq having different meanings depending on the backend.
We fix this by renaming nr_irqs to nr_xirqs and making it always indicate
just the number of external irqs, adjusting the value we pass to XIVE
accordingly.  We also use move to using common constants in most of the
irq configurations, to make it clearer that the IRQ space looks the same
to the guest (and emulated devices), even if the backend is different.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_irq.c         | 48 +++++++++++++++-----------------------
 include/hw/ppc/spapr_irq.h | 19 +++++++++------
 2 files changed, 31 insertions(+), 36 deletions(-)

Comments

Cédric Le Goater Sept. 25, 2019, 7:05 a.m. UTC | #1
On 25/09/2019 08:45, David Gibson wrote:
> Both the XICS and XIVE interrupt backends have a "nr-irqs" property, but
> it means slightly different things.  For XICS (or, strictly, the ICS) it
> indicates the number of "real" external IRQs.  Those start at XICS_IRQ_BASE
> (0x1000) and don't include the special IPI vector.  For XIVE, however, it
> includes the whole IRQ space, including XIVE's many IPI vectors.
> 
> The spapr code currently doesn't handle this sensibly, with the nr_irqs
> value in SpaprIrq having different meanings depending on the backend.
> We fix this by renaming nr_irqs to nr_xirqs and making it always indicate
> just the number of external irqs, adjusting the value we pass to XIVE
> accordingly.  We also use move to using common constants in most of the
> irq configurations, to make it clearer that the IRQ space looks the same
> to the guest (and emulated devices), even if the backend is different.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

one comment below,

> ---
>  hw/ppc/spapr_irq.c         | 48 +++++++++++++++-----------------------
>  include/hw/ppc/spapr_irq.h | 19 +++++++++------
>  2 files changed, 31 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 8c26fa2d1e..5190a33e08 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -92,7 +92,7 @@ static void spapr_irq_init_kvm(SpaprMachineState *spapr,
>   * XICS IRQ backend.
>   */
>  
> -static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
> +static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_xirqs,
>                                  Error **errp)
>  {
>      Object *obj;
> @@ -102,7 +102,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
>                                     &error_fatal);
> -    object_property_set_int(obj, nr_irqs, "nr-irqs",  &error_fatal);
> +    object_property_set_int(obj, nr_xirqs, "nr-irqs",  &error_fatal);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -234,13 +234,9 @@ static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
>      }
>  }
>  
> -#define SPAPR_IRQ_XICS_NR_IRQS     0x1000
> -#define SPAPR_IRQ_XICS_NR_MSIS     \
> -    (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
> -
>  SpaprIrq spapr_irq_xics = {
> -    .nr_irqs     = SPAPR_IRQ_XICS_NR_IRQS,
> -    .nr_msis     = SPAPR_IRQ_XICS_NR_MSIS,
> +    .nr_xirqs    = SPAPR_NR_XIRQS,
> +    .nr_msis     = SPAPR_NR_MSIS,
>      .ov5         = SPAPR_OV5_XIVE_LEGACY,
>  
>      .init        = spapr_irq_init_xics,
> @@ -260,7 +256,7 @@ SpaprIrq spapr_irq_xics = {
>  /*
>   * XIVE IRQ backend.
>   */
> -static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
> +static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_xirqs,
>                                  Error **errp)
>  {
>      uint32_t nr_servers = spapr_max_server_number(spapr);
> @@ -268,7 +264,7 @@ static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
>      int i;
>  
>      dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
> -    qdev_prop_set_uint32(dev, "nr-irqs", nr_irqs);
> +    qdev_prop_set_uint32(dev, "nr-irqs", nr_xirqs + SPAPR_XIRQ_BASE);
>      /*
>       * 8 XIVE END structures per CPU. One for each available priority
>       */
> @@ -308,7 +304,7 @@ static qemu_irq spapr_qirq_xive(SpaprMachineState *spapr, int irq)
>  {
>      SpaprXive *xive = spapr->xive;
>  
> -    if (irq >= xive->nr_irqs) {
> +    if ((irq < SPAPR_XIRQ_BASE) || (irq >= xive->nr_irqs)) {

So IPIs cannot be pulsed ? I think that is OK in QEMU.

XIVE unifies all the interrupts at the controller level. Any one can trigger 
an interrupt with a store on the associate ESB page.

>          return NULL;
>      }
>  
> @@ -409,12 +405,9 @@ static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
>   * with XICS.
>   */
>  
> -#define SPAPR_IRQ_XIVE_NR_IRQS     0x2000
> -#define SPAPR_IRQ_XIVE_NR_MSIS     (SPAPR_IRQ_XIVE_NR_IRQS - SPAPR_IRQ_MSI)
> -
>  SpaprIrq spapr_irq_xive = {
> -    .nr_irqs     = SPAPR_IRQ_XIVE_NR_IRQS,
> -    .nr_msis     = SPAPR_IRQ_XIVE_NR_MSIS,
> +    .nr_xirqs    = SPAPR_NR_XIRQS,
> +    .nr_msis     = SPAPR_NR_MSIS,
>      .ov5         = SPAPR_OV5_XIVE_EXPLOIT,
>  
>      .init        = spapr_irq_init_xive,
> @@ -450,18 +443,18 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState *spapr)
>          &spapr_irq_xive : &spapr_irq_xics;
>  }
>  
> -static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_irqs,
> +static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_xirqs,
>                                  Error **errp)
>  {
>      Error *local_err = NULL;
>  
> -    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_irqs, &local_err);
> +    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_xirqs, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_irqs, &local_err);
> +    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_xirqs, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -586,12 +579,9 @@ static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
>  /*
>   * Define values in sync with the XIVE and XICS backend
>   */
> -#define SPAPR_IRQ_DUAL_NR_IRQS     0x2000
> -#define SPAPR_IRQ_DUAL_NR_MSIS     (SPAPR_IRQ_DUAL_NR_IRQS - SPAPR_IRQ_MSI)
> -
>  SpaprIrq spapr_irq_dual = {
> -    .nr_irqs     = SPAPR_IRQ_DUAL_NR_IRQS,
> -    .nr_msis     = SPAPR_IRQ_DUAL_NR_MSIS,
> +    .nr_xirqs    = SPAPR_NR_XIRQS,
> +    .nr_msis     = SPAPR_NR_MSIS,
>      .ov5         = SPAPR_OV5_XIVE_BOTH,
>  
>      .init        = spapr_irq_init_dual,
> @@ -693,10 +683,10 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
>          spapr_irq_msi_init(spapr, spapr->irq->nr_msis);
>      }
>  
> -    spapr->irq->init(spapr, spapr->irq->nr_irqs, errp);
> +    spapr->irq->init(spapr, spapr->irq->nr_xirqs, errp);
>  
>      spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr,
> -                                      spapr->irq->nr_irqs);
> +                                      spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE);
>  }
>  
>  int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp)
> @@ -804,11 +794,11 @@ int spapr_irq_find(SpaprMachineState *spapr, int num, bool align, Error **errp)
>      return first + ics->offset;
>  }
>  
> -#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     0x400
> +#define SPAPR_IRQ_XICS_LEGACY_NR_XIRQS     0x400
>  
>  SpaprIrq spapr_irq_xics_legacy = {
> -    .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> -    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> +    .nr_xirqs    = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
> +    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
>      .ov5         = SPAPR_OV5_XIVE_LEGACY,
>  
>      .init        = spapr_irq_init_xics,
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5db305165c..a8f9a2ab11 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -16,13 +16,18 @@
>   * IRQ range offsets per device type
>   */
>  #define SPAPR_IRQ_IPI        0x0
> -#define SPAPR_IRQ_EPOW       0x1000  /* XICS_IRQ_BASE offset */
> -#define SPAPR_IRQ_HOTPLUG    0x1001
> -#define SPAPR_IRQ_VIO        0x1100  /* 256 VIO devices */
> -#define SPAPR_IRQ_PCI_LSI    0x1200  /* 32+ PHBs devices */
>  
> -#define SPAPR_IRQ_MSI        0x1300  /* Offset of the dynamic range covered
> -                                      * by the bitmap allocator */
> +#define SPAPR_XIRQ_BASE      XICS_IRQ_BASE /* 0x1000 */
> +#define SPAPR_IRQ_EPOW       (SPAPR_XIRQ_BASE + 0x0000)
> +#define SPAPR_IRQ_HOTPLUG    (SPAPR_XIRQ_BASE + 0x0001)
> +#define SPAPR_IRQ_VIO        (SPAPR_XIRQ_BASE + 0x0100)  /* 256 VIO devices */
> +#define SPAPR_IRQ_PCI_LSI    (SPAPR_XIRQ_BASE + 0x0200)  /* 32+ PHBs devices */
> +
> +/* Offset of the dynamic range covered by the bitmap allocator */
> +#define SPAPR_IRQ_MSI        (SPAPR_XIRQ_BASE + 0x0300)
> +
> +#define SPAPR_NR_XIRQS       0x1000
> +#define SPAPR_NR_MSIS        (SPAPR_XIRQ_BASE + SPAPR_NR_XIRQS - SPAPR_IRQ_MSI)
>  
>  typedef struct SpaprMachineState SpaprMachineState;
>  
> @@ -32,7 +37,7 @@ int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
>  void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
>  
>  typedef struct SpaprIrq {
> -    uint32_t    nr_irqs;
> +    uint32_t    nr_xirqs;
>      uint32_t    nr_msis;
>      uint8_t     ov5;
>  
>
Greg Kurz Sept. 25, 2019, 5:13 p.m. UTC | #2
On Wed, 25 Sep 2019 16:45:23 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Both the XICS and XIVE interrupt backends have a "nr-irqs" property, but
> it means slightly different things.  For XICS (or, strictly, the ICS) it
> indicates the number of "real" external IRQs.  Those start at XICS_IRQ_BASE
> (0x1000) and don't include the special IPI vector.  For XIVE, however, it
> includes the whole IRQ space, including XIVE's many IPI vectors.
> 
> The spapr code currently doesn't handle this sensibly, with the nr_irqs
> value in SpaprIrq having different meanings depending on the backend.
> We fix this by renaming nr_irqs to nr_xirqs and making it always indicate
> just the number of external irqs, adjusting the value we pass to XIVE
> accordingly.  We also use move to using common constants in most of the
                        ^^^
s/use//

> irq configurations, to make it clearer that the IRQ space looks the same
> to the guest (and emulated devices), even if the backend is different.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_irq.c         | 48 +++++++++++++++-----------------------
>  include/hw/ppc/spapr_irq.h | 19 +++++++++------
>  2 files changed, 31 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 8c26fa2d1e..5190a33e08 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -92,7 +92,7 @@ static void spapr_irq_init_kvm(SpaprMachineState *spapr,
>   * XICS IRQ backend.
>   */
>  
> -static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
> +static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_xirqs,
>                                  Error **errp)
>  {
>      Object *obj;
> @@ -102,7 +102,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
>                                     &error_fatal);
> -    object_property_set_int(obj, nr_irqs, "nr-irqs",  &error_fatal);
> +    object_property_set_int(obj, nr_xirqs, "nr-irqs",  &error_fatal);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -234,13 +234,9 @@ static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
>      }
>  }
>  
> -#define SPAPR_IRQ_XICS_NR_IRQS     0x1000
> -#define SPAPR_IRQ_XICS_NR_MSIS     \
> -    (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
> -
>  SpaprIrq spapr_irq_xics = {
> -    .nr_irqs     = SPAPR_IRQ_XICS_NR_IRQS,
> -    .nr_msis     = SPAPR_IRQ_XICS_NR_MSIS,
> +    .nr_xirqs    = SPAPR_NR_XIRQS,
> +    .nr_msis     = SPAPR_NR_MSIS,
>      .ov5         = SPAPR_OV5_XIVE_LEGACY,
>  
>      .init        = spapr_irq_init_xics,
> @@ -260,7 +256,7 @@ SpaprIrq spapr_irq_xics = {
>  /*
>   * XIVE IRQ backend.
>   */
> -static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
> +static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_xirqs,
>                                  Error **errp)
>  {
>      uint32_t nr_servers = spapr_max_server_number(spapr);
> @@ -268,7 +264,7 @@ static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
>      int i;
>  
>      dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
> -    qdev_prop_set_uint32(dev, "nr-irqs", nr_irqs);
> +    qdev_prop_set_uint32(dev, "nr-irqs", nr_xirqs + SPAPR_XIRQ_BASE);
>      /*
>       * 8 XIVE END structures per CPU. One for each available priority
>       */
> @@ -308,7 +304,7 @@ static qemu_irq spapr_qirq_xive(SpaprMachineState *spapr, int irq)
>  {
>      SpaprXive *xive = spapr->xive;
>  
> -    if (irq >= xive->nr_irqs) {
> +    if ((irq < SPAPR_XIRQ_BASE) || (irq >= xive->nr_irqs)) {
>          return NULL;
>      }
>  
> @@ -409,12 +405,9 @@ static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
>   * with XICS.
>   */
>  
> -#define SPAPR_IRQ_XIVE_NR_IRQS     0x2000
> -#define SPAPR_IRQ_XIVE_NR_MSIS     (SPAPR_IRQ_XIVE_NR_IRQS - SPAPR_IRQ_MSI)
> -
>  SpaprIrq spapr_irq_xive = {
> -    .nr_irqs     = SPAPR_IRQ_XIVE_NR_IRQS,
> -    .nr_msis     = SPAPR_IRQ_XIVE_NR_MSIS,
> +    .nr_xirqs    = SPAPR_NR_XIRQS,
> +    .nr_msis     = SPAPR_NR_MSIS,
>      .ov5         = SPAPR_OV5_XIVE_EXPLOIT,
>  
>      .init        = spapr_irq_init_xive,
> @@ -450,18 +443,18 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState *spapr)
>          &spapr_irq_xive : &spapr_irq_xics;
>  }
>  
> -static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_irqs,
> +static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_xirqs,
>                                  Error **errp)
>  {
>      Error *local_err = NULL;
>  
> -    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_irqs, &local_err);
> +    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_xirqs, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_irqs, &local_err);
> +    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_xirqs, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -586,12 +579,9 @@ static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
>  /*
>   * Define values in sync with the XIVE and XICS backend
>   */
> -#define SPAPR_IRQ_DUAL_NR_IRQS     0x2000
> -#define SPAPR_IRQ_DUAL_NR_MSIS     (SPAPR_IRQ_DUAL_NR_IRQS - SPAPR_IRQ_MSI)
> -
>  SpaprIrq spapr_irq_dual = {
> -    .nr_irqs     = SPAPR_IRQ_DUAL_NR_IRQS,
> -    .nr_msis     = SPAPR_IRQ_DUAL_NR_MSIS,
> +    .nr_xirqs    = SPAPR_NR_XIRQS,
> +    .nr_msis     = SPAPR_NR_MSIS,
>      .ov5         = SPAPR_OV5_XIVE_BOTH,
>  
>      .init        = spapr_irq_init_dual,
> @@ -693,10 +683,10 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
>          spapr_irq_msi_init(spapr, spapr->irq->nr_msis);
>      }
>  
> -    spapr->irq->init(spapr, spapr->irq->nr_irqs, errp);
> +    spapr->irq->init(spapr, spapr->irq->nr_xirqs, errp);
>  
>      spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr,
> -                                      spapr->irq->nr_irqs);
> +                                      spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE);
>  }
>  
>  int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp)
> @@ -804,11 +794,11 @@ int spapr_irq_find(SpaprMachineState *spapr, int num, bool align, Error **errp)
>      return first + ics->offset;
>  }
>  
> -#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     0x400
> +#define SPAPR_IRQ_XICS_LEGACY_NR_XIRQS     0x400
>  
>  SpaprIrq spapr_irq_xics_legacy = {
> -    .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> -    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> +    .nr_xirqs    = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
> +    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
>      .ov5         = SPAPR_OV5_XIVE_LEGACY,
>  
>      .init        = spapr_irq_init_xics,
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5db305165c..a8f9a2ab11 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -16,13 +16,18 @@
>   * IRQ range offsets per device type
>   */
>  #define SPAPR_IRQ_IPI        0x0
> -#define SPAPR_IRQ_EPOW       0x1000  /* XICS_IRQ_BASE offset */
> -#define SPAPR_IRQ_HOTPLUG    0x1001
> -#define SPAPR_IRQ_VIO        0x1100  /* 256 VIO devices */
> -#define SPAPR_IRQ_PCI_LSI    0x1200  /* 32+ PHBs devices */
>  
> -#define SPAPR_IRQ_MSI        0x1300  /* Offset of the dynamic range covered
> -                                      * by the bitmap allocator */
> +#define SPAPR_XIRQ_BASE      XICS_IRQ_BASE /* 0x1000 */
> +#define SPAPR_IRQ_EPOW       (SPAPR_XIRQ_BASE + 0x0000)
> +#define SPAPR_IRQ_HOTPLUG    (SPAPR_XIRQ_BASE + 0x0001)
> +#define SPAPR_IRQ_VIO        (SPAPR_XIRQ_BASE + 0x0100)  /* 256 VIO devices */
> +#define SPAPR_IRQ_PCI_LSI    (SPAPR_XIRQ_BASE + 0x0200)  /* 32+ PHBs devices */
> +
> +/* Offset of the dynamic range covered by the bitmap allocator */
> +#define SPAPR_IRQ_MSI        (SPAPR_XIRQ_BASE + 0x0300)
> +
> +#define SPAPR_NR_XIRQS       0x1000
> +#define SPAPR_NR_MSIS        (SPAPR_XIRQ_BASE + SPAPR_NR_XIRQS - SPAPR_IRQ_MSI)
>  
>  typedef struct SpaprMachineState SpaprMachineState;
>  
> @@ -32,7 +37,7 @@ int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
>  void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
>  
>  typedef struct SpaprIrq {
> -    uint32_t    nr_irqs;
> +    uint32_t    nr_xirqs;
>      uint32_t    nr_msis;
>      uint8_t     ov5;
>
David Gibson Sept. 26, 2019, 1:03 a.m. UTC | #3
On Wed, Sep 25, 2019 at 09:05:34AM +0200, Cédric Le Goater wrote:
> On 25/09/2019 08:45, David Gibson wrote:
> > Both the XICS and XIVE interrupt backends have a "nr-irqs" property, but
> > it means slightly different things.  For XICS (or, strictly, the ICS) it
> > indicates the number of "real" external IRQs.  Those start at XICS_IRQ_BASE
> > (0x1000) and don't include the special IPI vector.  For XIVE, however, it
> > includes the whole IRQ space, including XIVE's many IPI vectors.
> > 
> > The spapr code currently doesn't handle this sensibly, with the nr_irqs
> > value in SpaprIrq having different meanings depending on the backend.
> > We fix this by renaming nr_irqs to nr_xirqs and making it always indicate
> > just the number of external irqs, adjusting the value we pass to XIVE
> > accordingly.  We also use move to using common constants in most of the
> > irq configurations, to make it clearer that the IRQ space looks the same
> > to the guest (and emulated devices), even if the backend is different.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> one comment below,
> 
> > ---
> >  hw/ppc/spapr_irq.c         | 48 +++++++++++++++-----------------------
> >  include/hw/ppc/spapr_irq.h | 19 +++++++++------
> >  2 files changed, 31 insertions(+), 36 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index 8c26fa2d1e..5190a33e08 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -92,7 +92,7 @@ static void spapr_irq_init_kvm(SpaprMachineState *spapr,
> >   * XICS IRQ backend.
> >   */
> >  
> > -static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
> > +static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_xirqs,
> >                                  Error **errp)
> >  {
> >      Object *obj;
> > @@ -102,7 +102,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
> >      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
> >      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
> >                                     &error_fatal);
> > -    object_property_set_int(obj, nr_irqs, "nr-irqs",  &error_fatal);
> > +    object_property_set_int(obj, nr_xirqs, "nr-irqs",  &error_fatal);
> >      object_property_set_bool(obj, true, "realized", &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> > @@ -234,13 +234,9 @@ static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
> >      }
> >  }
> >  
> > -#define SPAPR_IRQ_XICS_NR_IRQS     0x1000
> > -#define SPAPR_IRQ_XICS_NR_MSIS     \
> > -    (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
> > -
> >  SpaprIrq spapr_irq_xics = {
> > -    .nr_irqs     = SPAPR_IRQ_XICS_NR_IRQS,
> > -    .nr_msis     = SPAPR_IRQ_XICS_NR_MSIS,
> > +    .nr_xirqs    = SPAPR_NR_XIRQS,
> > +    .nr_msis     = SPAPR_NR_MSIS,
> >      .ov5         = SPAPR_OV5_XIVE_LEGACY,
> >  
> >      .init        = spapr_irq_init_xics,
> > @@ -260,7 +256,7 @@ SpaprIrq spapr_irq_xics = {
> >  /*
> >   * XIVE IRQ backend.
> >   */
> > -static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
> > +static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_xirqs,
> >                                  Error **errp)
> >  {
> >      uint32_t nr_servers = spapr_max_server_number(spapr);
> > @@ -268,7 +264,7 @@ static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
> >      int i;
> >  
> >      dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
> > -    qdev_prop_set_uint32(dev, "nr-irqs", nr_irqs);
> > +    qdev_prop_set_uint32(dev, "nr-irqs", nr_xirqs + SPAPR_XIRQ_BASE);
> >      /*
> >       * 8 XIVE END structures per CPU. One for each available priority
> >       */
> > @@ -308,7 +304,7 @@ static qemu_irq spapr_qirq_xive(SpaprMachineState *spapr, int irq)
> >  {
> >      SpaprXive *xive = spapr->xive;
> >  
> > -    if (irq >= xive->nr_irqs) {
> > +    if ((irq < SPAPR_XIRQ_BASE) || (irq >= xive->nr_irqs)) {
> 
> So IPIs cannot be pulsed ? I think that is OK in QEMU.

They can be pulsed, they just can't be retrieved via the spapr_qirq()
interface.  Since that interface basically exists for the spapr root
devices (VIO and PHBs) to find the qemu_irqs to wire themselves up to,
I think that's fine.

If we discover some reason we need to grab IPI qirqs by global number
then we can revisit this.

I'll add a comment to clarify this in the later patch where I unify
the qirq implementations.

> XIVE unifies all the interrupts at the controller level. Any one can trigger 
> an interrupt with a store on the associate ESB page.

Absolutely, and nothing's stopping them.
> 
> >          return NULL;
> >      }
> >  
> > @@ -409,12 +405,9 @@ static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
> >   * with XICS.
> >   */
> >  
> > -#define SPAPR_IRQ_XIVE_NR_IRQS     0x2000
> > -#define SPAPR_IRQ_XIVE_NR_MSIS     (SPAPR_IRQ_XIVE_NR_IRQS - SPAPR_IRQ_MSI)
> > -
> >  SpaprIrq spapr_irq_xive = {
> > -    .nr_irqs     = SPAPR_IRQ_XIVE_NR_IRQS,
> > -    .nr_msis     = SPAPR_IRQ_XIVE_NR_MSIS,
> > +    .nr_xirqs    = SPAPR_NR_XIRQS,
> > +    .nr_msis     = SPAPR_NR_MSIS,
> >      .ov5         = SPAPR_OV5_XIVE_EXPLOIT,
> >  
> >      .init        = spapr_irq_init_xive,
> > @@ -450,18 +443,18 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState *spapr)
> >          &spapr_irq_xive : &spapr_irq_xics;
> >  }
> >  
> > -static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_irqs,
> > +static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_xirqs,
> >                                  Error **errp)
> >  {
> >      Error *local_err = NULL;
> >  
> > -    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_irqs, &local_err);
> > +    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_xirqs, &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> >  
> > -    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_irqs, &local_err);
> > +    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_xirqs, &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> > @@ -586,12 +579,9 @@ static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
> >  /*
> >   * Define values in sync with the XIVE and XICS backend
> >   */
> > -#define SPAPR_IRQ_DUAL_NR_IRQS     0x2000
> > -#define SPAPR_IRQ_DUAL_NR_MSIS     (SPAPR_IRQ_DUAL_NR_IRQS - SPAPR_IRQ_MSI)
> > -
> >  SpaprIrq spapr_irq_dual = {
> > -    .nr_irqs     = SPAPR_IRQ_DUAL_NR_IRQS,
> > -    .nr_msis     = SPAPR_IRQ_DUAL_NR_MSIS,
> > +    .nr_xirqs    = SPAPR_NR_XIRQS,
> > +    .nr_msis     = SPAPR_NR_MSIS,
> >      .ov5         = SPAPR_OV5_XIVE_BOTH,
> >  
> >      .init        = spapr_irq_init_dual,
> > @@ -693,10 +683,10 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> >          spapr_irq_msi_init(spapr, spapr->irq->nr_msis);
> >      }
> >  
> > -    spapr->irq->init(spapr, spapr->irq->nr_irqs, errp);
> > +    spapr->irq->init(spapr, spapr->irq->nr_xirqs, errp);
> >  
> >      spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr,
> > -                                      spapr->irq->nr_irqs);
> > +                                      spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE);
> >  }
> >  
> >  int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp)
> > @@ -804,11 +794,11 @@ int spapr_irq_find(SpaprMachineState *spapr, int num, bool align, Error **errp)
> >      return first + ics->offset;
> >  }
> >  
> > -#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     0x400
> > +#define SPAPR_IRQ_XICS_LEGACY_NR_XIRQS     0x400
> >  
> >  SpaprIrq spapr_irq_xics_legacy = {
> > -    .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> > -    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> > +    .nr_xirqs    = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
> > +    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
> >      .ov5         = SPAPR_OV5_XIVE_LEGACY,
> >  
> >      .init        = spapr_irq_init_xics,
> > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> > index 5db305165c..a8f9a2ab11 100644
> > --- a/include/hw/ppc/spapr_irq.h
> > +++ b/include/hw/ppc/spapr_irq.h
> > @@ -16,13 +16,18 @@
> >   * IRQ range offsets per device type
> >   */
> >  #define SPAPR_IRQ_IPI        0x0
> > -#define SPAPR_IRQ_EPOW       0x1000  /* XICS_IRQ_BASE offset */
> > -#define SPAPR_IRQ_HOTPLUG    0x1001
> > -#define SPAPR_IRQ_VIO        0x1100  /* 256 VIO devices */
> > -#define SPAPR_IRQ_PCI_LSI    0x1200  /* 32+ PHBs devices */
> >  
> > -#define SPAPR_IRQ_MSI        0x1300  /* Offset of the dynamic range covered
> > -                                      * by the bitmap allocator */
> > +#define SPAPR_XIRQ_BASE      XICS_IRQ_BASE /* 0x1000 */
> > +#define SPAPR_IRQ_EPOW       (SPAPR_XIRQ_BASE + 0x0000)
> > +#define SPAPR_IRQ_HOTPLUG    (SPAPR_XIRQ_BASE + 0x0001)
> > +#define SPAPR_IRQ_VIO        (SPAPR_XIRQ_BASE + 0x0100)  /* 256 VIO devices */
> > +#define SPAPR_IRQ_PCI_LSI    (SPAPR_XIRQ_BASE + 0x0200)  /* 32+ PHBs devices */
> > +
> > +/* Offset of the dynamic range covered by the bitmap allocator */
> > +#define SPAPR_IRQ_MSI        (SPAPR_XIRQ_BASE + 0x0300)
> > +
> > +#define SPAPR_NR_XIRQS       0x1000
> > +#define SPAPR_NR_MSIS        (SPAPR_XIRQ_BASE + SPAPR_NR_XIRQS - SPAPR_IRQ_MSI)
> >  
> >  typedef struct SpaprMachineState SpaprMachineState;
> >  
> > @@ -32,7 +37,7 @@ int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
> >  void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
> >  
> >  typedef struct SpaprIrq {
> > -    uint32_t    nr_irqs;
> > +    uint32_t    nr_xirqs;
> >      uint32_t    nr_msis;
> >      uint8_t     ov5;
> >  
> > 
>
Cédric Le Goater Sept. 26, 2019, 7:02 a.m. UTC | #4
On 26/09/2019 03:03, David Gibson wrote:
> On Wed, Sep 25, 2019 at 09:05:34AM +0200, Cédric Le Goater wrote:
>> On 25/09/2019 08:45, David Gibson wrote:
>>> Both the XICS and XIVE interrupt backends have a "nr-irqs" property, but
>>> it means slightly different things.  For XICS (or, strictly, the ICS) it
>>> indicates the number of "real" external IRQs.  Those start at XICS_IRQ_BASE
>>> (0x1000) and don't include the special IPI vector.  For XIVE, however, it
>>> includes the whole IRQ space, including XIVE's many IPI vectors.
>>>
>>> The spapr code currently doesn't handle this sensibly, with the nr_irqs
>>> value in SpaprIrq having different meanings depending on the backend.
>>> We fix this by renaming nr_irqs to nr_xirqs and making it always indicate
>>> just the number of external irqs, adjusting the value we pass to XIVE
>>> accordingly.  We also use move to using common constants in most of the
>>> irq configurations, to make it clearer that the IRQ space looks the same
>>> to the guest (and emulated devices), even if the backend is different.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> one comment below,
>>
>>> ---
>>>  hw/ppc/spapr_irq.c         | 48 +++++++++++++++-----------------------
>>>  include/hw/ppc/spapr_irq.h | 19 +++++++++------
>>>  2 files changed, 31 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>>> index 8c26fa2d1e..5190a33e08 100644
>>> --- a/hw/ppc/spapr_irq.c
>>> +++ b/hw/ppc/spapr_irq.c
>>> @@ -92,7 +92,7 @@ static void spapr_irq_init_kvm(SpaprMachineState *spapr,
>>>   * XICS IRQ backend.
>>>   */
>>>  
>>> -static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
>>> +static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_xirqs,
>>>                                  Error **errp)
>>>  {
>>>      Object *obj;
>>> @@ -102,7 +102,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
>>>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>>>      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
>>>                                     &error_fatal);
>>> -    object_property_set_int(obj, nr_irqs, "nr-irqs",  &error_fatal);
>>> +    object_property_set_int(obj, nr_xirqs, "nr-irqs",  &error_fatal);
>>>      object_property_set_bool(obj, true, "realized", &local_err);
>>>      if (local_err) {
>>>          error_propagate(errp, local_err);
>>> @@ -234,13 +234,9 @@ static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
>>>      }
>>>  }
>>>  
>>> -#define SPAPR_IRQ_XICS_NR_IRQS     0x1000
>>> -#define SPAPR_IRQ_XICS_NR_MSIS     \
>>> -    (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
>>> -
>>>  SpaprIrq spapr_irq_xics = {
>>> -    .nr_irqs     = SPAPR_IRQ_XICS_NR_IRQS,
>>> -    .nr_msis     = SPAPR_IRQ_XICS_NR_MSIS,
>>> +    .nr_xirqs    = SPAPR_NR_XIRQS,
>>> +    .nr_msis     = SPAPR_NR_MSIS,
>>>      .ov5         = SPAPR_OV5_XIVE_LEGACY,
>>>  
>>>      .init        = spapr_irq_init_xics,
>>> @@ -260,7 +256,7 @@ SpaprIrq spapr_irq_xics = {
>>>  /*
>>>   * XIVE IRQ backend.
>>>   */
>>> -static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
>>> +static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_xirqs,
>>>                                  Error **errp)
>>>  {
>>>      uint32_t nr_servers = spapr_max_server_number(spapr);
>>> @@ -268,7 +264,7 @@ static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
>>>      int i;
>>>  
>>>      dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
>>> -    qdev_prop_set_uint32(dev, "nr-irqs", nr_irqs);
>>> +    qdev_prop_set_uint32(dev, "nr-irqs", nr_xirqs + SPAPR_XIRQ_BASE);
>>>      /*
>>>       * 8 XIVE END structures per CPU. One for each available priority
>>>       */
>>> @@ -308,7 +304,7 @@ static qemu_irq spapr_qirq_xive(SpaprMachineState *spapr, int irq)
>>>  {
>>>      SpaprXive *xive = spapr->xive;
>>>  
>>> -    if (irq >= xive->nr_irqs) {
>>> +    if ((irq < SPAPR_XIRQ_BASE) || (irq >= xive->nr_irqs)) {
>>
>> So IPIs cannot be pulsed ? I think that is OK in QEMU.
> 
> They can be pulsed, they just can't be retrieved via the spapr_qirq()
> interface.  Since that interface basically exists for the spapr root
> devices (VIO and PHBs) to find the qemu_irqs to wire themselves up to,
> I think that's fine.
> 
> If we discover some reason we need to grab IPI qirqs by global number
> then we can revisit this.

Not from QEMU I think. This is fine.

C. 

> 
> I'll add a comment to clarify this in the later patch where I unify
> the qirq implementations.
> 
>> XIVE unifies all the interrupts at the controller level. Any one can trigger 
>> an interrupt with a store on the associate ESB page.
> 
> Absolutely, and nothing's stopping them.
>>
>>>          return NULL;
>>>      }
>>>  
>>> @@ -409,12 +405,9 @@ static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
>>>   * with XICS.
>>>   */
>>>  
>>> -#define SPAPR_IRQ_XIVE_NR_IRQS     0x2000
>>> -#define SPAPR_IRQ_XIVE_NR_MSIS     (SPAPR_IRQ_XIVE_NR_IRQS - SPAPR_IRQ_MSI)
>>> -
>>>  SpaprIrq spapr_irq_xive = {
>>> -    .nr_irqs     = SPAPR_IRQ_XIVE_NR_IRQS,
>>> -    .nr_msis     = SPAPR_IRQ_XIVE_NR_MSIS,
>>> +    .nr_xirqs    = SPAPR_NR_XIRQS,
>>> +    .nr_msis     = SPAPR_NR_MSIS,
>>>      .ov5         = SPAPR_OV5_XIVE_EXPLOIT,
>>>  
>>>      .init        = spapr_irq_init_xive,
>>> @@ -450,18 +443,18 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState *spapr)
>>>          &spapr_irq_xive : &spapr_irq_xics;
>>>  }
>>>  
>>> -static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_irqs,
>>> +static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_xirqs,
>>>                                  Error **errp)
>>>  {
>>>      Error *local_err = NULL;
>>>  
>>> -    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_irqs, &local_err);
>>> +    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_xirqs, &local_err);
>>>      if (local_err) {
>>>          error_propagate(errp, local_err);
>>>          return;
>>>      }
>>>  
>>> -    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_irqs, &local_err);
>>> +    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_xirqs, &local_err);
>>>      if (local_err) {
>>>          error_propagate(errp, local_err);
>>>          return;
>>> @@ -586,12 +579,9 @@ static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
>>>  /*
>>>   * Define values in sync with the XIVE and XICS backend
>>>   */
>>> -#define SPAPR_IRQ_DUAL_NR_IRQS     0x2000
>>> -#define SPAPR_IRQ_DUAL_NR_MSIS     (SPAPR_IRQ_DUAL_NR_IRQS - SPAPR_IRQ_MSI)
>>> -
>>>  SpaprIrq spapr_irq_dual = {
>>> -    .nr_irqs     = SPAPR_IRQ_DUAL_NR_IRQS,
>>> -    .nr_msis     = SPAPR_IRQ_DUAL_NR_MSIS,
>>> +    .nr_xirqs    = SPAPR_NR_XIRQS,
>>> +    .nr_msis     = SPAPR_NR_MSIS,
>>>      .ov5         = SPAPR_OV5_XIVE_BOTH,
>>>  
>>>      .init        = spapr_irq_init_dual,
>>> @@ -693,10 +683,10 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
>>>          spapr_irq_msi_init(spapr, spapr->irq->nr_msis);
>>>      }
>>>  
>>> -    spapr->irq->init(spapr, spapr->irq->nr_irqs, errp);
>>> +    spapr->irq->init(spapr, spapr->irq->nr_xirqs, errp);
>>>  
>>>      spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr,
>>> -                                      spapr->irq->nr_irqs);
>>> +                                      spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE);
>>>  }
>>>  
>>>  int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp)
>>> @@ -804,11 +794,11 @@ int spapr_irq_find(SpaprMachineState *spapr, int num, bool align, Error **errp)
>>>      return first + ics->offset;
>>>  }
>>>  
>>> -#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     0x400
>>> +#define SPAPR_IRQ_XICS_LEGACY_NR_XIRQS     0x400
>>>  
>>>  SpaprIrq spapr_irq_xics_legacy = {
>>> -    .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>>> -    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>>> +    .nr_xirqs    = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
>>> +    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
>>>      .ov5         = SPAPR_OV5_XIVE_LEGACY,
>>>  
>>>      .init        = spapr_irq_init_xics,
>>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>>> index 5db305165c..a8f9a2ab11 100644
>>> --- a/include/hw/ppc/spapr_irq.h
>>> +++ b/include/hw/ppc/spapr_irq.h
>>> @@ -16,13 +16,18 @@
>>>   * IRQ range offsets per device type
>>>   */
>>>  #define SPAPR_IRQ_IPI        0x0
>>> -#define SPAPR_IRQ_EPOW       0x1000  /* XICS_IRQ_BASE offset */
>>> -#define SPAPR_IRQ_HOTPLUG    0x1001
>>> -#define SPAPR_IRQ_VIO        0x1100  /* 256 VIO devices */
>>> -#define SPAPR_IRQ_PCI_LSI    0x1200  /* 32+ PHBs devices */
>>>  
>>> -#define SPAPR_IRQ_MSI        0x1300  /* Offset of the dynamic range covered
>>> -                                      * by the bitmap allocator */
>>> +#define SPAPR_XIRQ_BASE      XICS_IRQ_BASE /* 0x1000 */
>>> +#define SPAPR_IRQ_EPOW       (SPAPR_XIRQ_BASE + 0x0000)
>>> +#define SPAPR_IRQ_HOTPLUG    (SPAPR_XIRQ_BASE + 0x0001)
>>> +#define SPAPR_IRQ_VIO        (SPAPR_XIRQ_BASE + 0x0100)  /* 256 VIO devices */
>>> +#define SPAPR_IRQ_PCI_LSI    (SPAPR_XIRQ_BASE + 0x0200)  /* 32+ PHBs devices */
>>> +
>>> +/* Offset of the dynamic range covered by the bitmap allocator */
>>> +#define SPAPR_IRQ_MSI        (SPAPR_XIRQ_BASE + 0x0300)
>>> +
>>> +#define SPAPR_NR_XIRQS       0x1000
>>> +#define SPAPR_NR_MSIS        (SPAPR_XIRQ_BASE + SPAPR_NR_XIRQS - SPAPR_IRQ_MSI)
>>>  
>>>  typedef struct SpaprMachineState SpaprMachineState;
>>>  
>>> @@ -32,7 +37,7 @@ int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
>>>  void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
>>>  
>>>  typedef struct SpaprIrq {
>>> -    uint32_t    nr_irqs;
>>> +    uint32_t    nr_xirqs;
>>>      uint32_t    nr_msis;
>>>      uint8_t     ov5;
>>>  
>>>
>>
>
diff mbox series

Patch

diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 8c26fa2d1e..5190a33e08 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -92,7 +92,7 @@  static void spapr_irq_init_kvm(SpaprMachineState *spapr,
  * XICS IRQ backend.
  */
 
-static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
+static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_xirqs,
                                 Error **errp)
 {
     Object *obj;
@@ -102,7 +102,7 @@  static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
     object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
     object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
                                    &error_fatal);
-    object_property_set_int(obj, nr_irqs, "nr-irqs",  &error_fatal);
+    object_property_set_int(obj, nr_xirqs, "nr-irqs",  &error_fatal);
     object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -234,13 +234,9 @@  static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
     }
 }
 
-#define SPAPR_IRQ_XICS_NR_IRQS     0x1000
-#define SPAPR_IRQ_XICS_NR_MSIS     \
-    (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
-
 SpaprIrq spapr_irq_xics = {
-    .nr_irqs     = SPAPR_IRQ_XICS_NR_IRQS,
-    .nr_msis     = SPAPR_IRQ_XICS_NR_MSIS,
+    .nr_xirqs    = SPAPR_NR_XIRQS,
+    .nr_msis     = SPAPR_NR_MSIS,
     .ov5         = SPAPR_OV5_XIVE_LEGACY,
 
     .init        = spapr_irq_init_xics,
@@ -260,7 +256,7 @@  SpaprIrq spapr_irq_xics = {
 /*
  * XIVE IRQ backend.
  */
-static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
+static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_xirqs,
                                 Error **errp)
 {
     uint32_t nr_servers = spapr_max_server_number(spapr);
@@ -268,7 +264,7 @@  static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
     int i;
 
     dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
-    qdev_prop_set_uint32(dev, "nr-irqs", nr_irqs);
+    qdev_prop_set_uint32(dev, "nr-irqs", nr_xirqs + SPAPR_XIRQ_BASE);
     /*
      * 8 XIVE END structures per CPU. One for each available priority
      */
@@ -308,7 +304,7 @@  static qemu_irq spapr_qirq_xive(SpaprMachineState *spapr, int irq)
 {
     SpaprXive *xive = spapr->xive;
 
-    if (irq >= xive->nr_irqs) {
+    if ((irq < SPAPR_XIRQ_BASE) || (irq >= xive->nr_irqs)) {
         return NULL;
     }
 
@@ -409,12 +405,9 @@  static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
  * with XICS.
  */
 
-#define SPAPR_IRQ_XIVE_NR_IRQS     0x2000
-#define SPAPR_IRQ_XIVE_NR_MSIS     (SPAPR_IRQ_XIVE_NR_IRQS - SPAPR_IRQ_MSI)
-
 SpaprIrq spapr_irq_xive = {
-    .nr_irqs     = SPAPR_IRQ_XIVE_NR_IRQS,
-    .nr_msis     = SPAPR_IRQ_XIVE_NR_MSIS,
+    .nr_xirqs    = SPAPR_NR_XIRQS,
+    .nr_msis     = SPAPR_NR_MSIS,
     .ov5         = SPAPR_OV5_XIVE_EXPLOIT,
 
     .init        = spapr_irq_init_xive,
@@ -450,18 +443,18 @@  static SpaprIrq *spapr_irq_current(SpaprMachineState *spapr)
         &spapr_irq_xive : &spapr_irq_xics;
 }
 
-static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_irqs,
+static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_xirqs,
                                 Error **errp)
 {
     Error *local_err = NULL;
 
-    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_irqs, &local_err);
+    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_xirqs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
-    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_irqs, &local_err);
+    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_xirqs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -586,12 +579,9 @@  static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
 /*
  * Define values in sync with the XIVE and XICS backend
  */
-#define SPAPR_IRQ_DUAL_NR_IRQS     0x2000
-#define SPAPR_IRQ_DUAL_NR_MSIS     (SPAPR_IRQ_DUAL_NR_IRQS - SPAPR_IRQ_MSI)
-
 SpaprIrq spapr_irq_dual = {
-    .nr_irqs     = SPAPR_IRQ_DUAL_NR_IRQS,
-    .nr_msis     = SPAPR_IRQ_DUAL_NR_MSIS,
+    .nr_xirqs    = SPAPR_NR_XIRQS,
+    .nr_msis     = SPAPR_NR_MSIS,
     .ov5         = SPAPR_OV5_XIVE_BOTH,
 
     .init        = spapr_irq_init_dual,
@@ -693,10 +683,10 @@  void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
         spapr_irq_msi_init(spapr, spapr->irq->nr_msis);
     }
 
-    spapr->irq->init(spapr, spapr->irq->nr_irqs, errp);
+    spapr->irq->init(spapr, spapr->irq->nr_xirqs, errp);
 
     spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr,
-                                      spapr->irq->nr_irqs);
+                                      spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE);
 }
 
 int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp)
@@ -804,11 +794,11 @@  int spapr_irq_find(SpaprMachineState *spapr, int num, bool align, Error **errp)
     return first + ics->offset;
 }
 
-#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     0x400
+#define SPAPR_IRQ_XICS_LEGACY_NR_XIRQS     0x400
 
 SpaprIrq spapr_irq_xics_legacy = {
-    .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
-    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
+    .nr_xirqs    = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
+    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
     .ov5         = SPAPR_OV5_XIVE_LEGACY,
 
     .init        = spapr_irq_init_xics,
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 5db305165c..a8f9a2ab11 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -16,13 +16,18 @@ 
  * IRQ range offsets per device type
  */
 #define SPAPR_IRQ_IPI        0x0
-#define SPAPR_IRQ_EPOW       0x1000  /* XICS_IRQ_BASE offset */
-#define SPAPR_IRQ_HOTPLUG    0x1001
-#define SPAPR_IRQ_VIO        0x1100  /* 256 VIO devices */
-#define SPAPR_IRQ_PCI_LSI    0x1200  /* 32+ PHBs devices */
 
-#define SPAPR_IRQ_MSI        0x1300  /* Offset of the dynamic range covered
-                                      * by the bitmap allocator */
+#define SPAPR_XIRQ_BASE      XICS_IRQ_BASE /* 0x1000 */
+#define SPAPR_IRQ_EPOW       (SPAPR_XIRQ_BASE + 0x0000)
+#define SPAPR_IRQ_HOTPLUG    (SPAPR_XIRQ_BASE + 0x0001)
+#define SPAPR_IRQ_VIO        (SPAPR_XIRQ_BASE + 0x0100)  /* 256 VIO devices */
+#define SPAPR_IRQ_PCI_LSI    (SPAPR_XIRQ_BASE + 0x0200)  /* 32+ PHBs devices */
+
+/* Offset of the dynamic range covered by the bitmap allocator */
+#define SPAPR_IRQ_MSI        (SPAPR_XIRQ_BASE + 0x0300)
+
+#define SPAPR_NR_XIRQS       0x1000
+#define SPAPR_NR_MSIS        (SPAPR_XIRQ_BASE + SPAPR_NR_XIRQS - SPAPR_IRQ_MSI)
 
 typedef struct SpaprMachineState SpaprMachineState;
 
@@ -32,7 +37,7 @@  int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
 void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
 
 typedef struct SpaprIrq {
-    uint32_t    nr_irqs;
+    uint32_t    nr_xirqs;
     uint32_t    nr_msis;
     uint8_t     ov5;