This proposal moves all the related IRQ routines of the sPAPR machine
behind a sPAPR IRQ backend interface 'spapr_irq' to prepare for future
changes. First of which will be to increase the size of the IRQ number
space, then, will follow a new backend for the POWER9 XIVE IRQ controller.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr.h     |  11 +-
 include/hw/ppc/spapr_irq.h |  22 ++++
 hw/ppc/spapr.c             | 180 +----------------------------
 hw/ppc/spapr_cpu_core.c    |   1 +
 hw/ppc/spapr_irq.c         | 230 +++++++++++++++++++++++++++++++++++++
 5 files changed, 259 insertions(+), 185 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 73067f5ee8aa..ad4d7cfd97b0 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -4,7 +4,6 @@
 #include "qemu/units.h"
 #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"
@@ -16,6 +15,7 @@ struct sPAPRNVRAM;
 typedef struct sPAPREventLogEntry sPAPREventLogEntry;
 typedef struct sPAPREventSource sPAPREventSource;
 typedef struct sPAPRPendingHPT sPAPRPendingHPT;
+typedef struct ICSState ICSState;
 
 #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
 #define SPAPR_ENTRY_POINT       0x100
@@ -110,6 +110,7 @@ struct sPAPRMachineClass {
                           unsigned n_dma, uint32_t *liobns, Error **errp);
     sPAPRResizeHPT resize_hpt_default;
     sPAPRCapabilities default_caps;
+    sPAPRIrq *irq;
 };
 
 /**
@@ -780,14 +781,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_find(sPAPRMachineState *spapr, int num, bool align,
-                   Error **errp);
-#define spapr_irq_findone(spapr, errp) spapr_irq_find(spapr, 1, false, errp)
-int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, 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
index 6f7f50548809..0e98c4474bb2 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -29,4 +29,26 @@ int spapr_irq_msi_alloc(sPAPRMachineState *spapr, uint32_t num, bool align,
 void spapr_irq_msi_free(sPAPRMachineState *spapr, int irq, uint32_t num);
 void spapr_irq_msi_reset(sPAPRMachineState *spapr);
 
+typedef struct sPAPRIrq {
+    uint32_t    nr_irqs;
+
+    void (*init)(sPAPRMachineState *spapr, Error **errp);
+    int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
+    void (*free)(sPAPRMachineState *spapr, int irq, int num);
+    qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
+    void (*print_info)(sPAPRMachineState *spapr, Monitor *mon);
+} sPAPRIrq;
+
+extern sPAPRIrq spapr_irq_xics;
+
+int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
+void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
+qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
+
+/*
+ * XICS legacy routines
+ */
+int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp);
+#define spapr_irq_findone(spapr, errp) spapr_irq_find(spapr, 1, false, errp)
+
 #endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 1794952d4e2a..693144e40d49 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -54,7 +54,6 @@
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
 #include "hw/pci-host/spapr.h"
-#include "hw/ppc/xics.h"
 #include "hw/pci/msi.h"
 
 #include "hw/pci/pci.h"
@@ -117,33 +116,6 @@ static bool spapr_is_thread0_in_vcore(sPAPRMachineState *spapr,
     return spapr_get_vcpu_id(cpu) % spapr->vsmt == 0;
 }
 
-static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
-                                  const char *type_ics,
-                                  int nr_irqs, Error **errp)
-{
-    Error *local_err = NULL;
-    Object *obj;
-
-    obj = object_new(type_ics);
-    object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
-    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
-                                   &error_abort);
-    object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
-    if (local_err) {
-        goto error;
-    }
-    object_property_set_bool(obj, true, "realized", &local_err);
-    if (local_err) {
-        goto error;
-    }
-
-    return ICS_BASE(obj);
-
-error:
-    error_propagate(errp, local_err);
-    return NULL;
-}
-
 static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
 {
     /* Dummy entries correspond to unused ICPState objects in older QEMUs,
@@ -184,43 +156,6 @@ static int xics_max_server_number(sPAPRMachineState *spapr)
     return DIV_ROUND_UP(max_cpus * spapr->vsmt, smp_threads);
 }
 
-static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
-{
-    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
-    Error *local_err = NULL;
-
-    /* Initialize the MSI IRQ allocator. */
-    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
-        spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
-    }
-
-    if (kvm_enabled()) {
-        if (machine_kernel_irqchip_allowed(machine) &&
-            !xics_kvm_init(spapr, &local_err)) {
-            spapr->icp_type = TYPE_KVM_ICP;
-            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
-                                          &local_err);
-        }
-        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
-            error_prepend(&local_err,
-                          "kernel_irqchip requested but unavailable: ");
-            goto error;
-        }
-        error_free(local_err);
-        local_err = NULL;
-    }
-
-    if (!spapr->ics) {
-        xics_spapr_init(spapr);
-        spapr->icp_type = TYPE_ICP;
-        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
-                                      &local_err);
-    }
-
-error:
-    error_propagate(errp, local_err);
-}
-
 static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
                                   int smt_threads)
 {
@@ -2618,7 +2553,7 @@ static void spapr_machine_init(MachineState *machine)
     load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
 
     /* Set up Interrupt Controller before we create the VCPUs */
-    xics_system_init(machine, XICS_IRQS_SPAPR, &error_fatal);
+    smc->irq->init(spapr, &error_fatal);
 
     /* Set up containers for ibm,client-architecture-support negotiated options
      */
@@ -3810,121 +3745,13 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
     return cpu ? ICP(cpu->intc) : NULL;
 }
 
-#define ICS_IRQ_FREE(ics, srcno)   \
-    (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK)))
-
-static int ics_find_free_block(ICSState *ics, int num, int alignnum)
-{
-    int first, i;
-
-    for (first = 0; first < ics->nr_irqs; first += alignnum) {
-        if (num > (ics->nr_irqs - first)) {
-            return -1;
-        }
-        for (i = first; i < first + num; ++i) {
-            if (!ICS_IRQ_FREE(ics, i)) {
-                break;
-            }
-        }
-        if (i == (first + num)) {
-            return first;
-        }
-    }
-
-    return -1;
-}
-
-int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
-{
-    ICSState *ics = spapr->ics;
-    int first = -1;
-
-    assert(ics);
-
-    /*
-     * MSIMesage::data is used for storing VIRQ so
-     * it has to be aligned to num to support multiple
-     * MSI vectors. MSI-X is not affected by this.
-     * The hint is used for the first IRQ, the rest should
-     * be allocated continuously.
-     */
-    if (align) {
-        assert((num == 1) || (num == 2) || (num == 4) ||
-               (num == 8) || (num == 16) || (num == 32));
-        first = ics_find_free_block(ics, num, num);
-    } else {
-        first = ics_find_free_block(ics, num, 1);
-    }
-
-    if (first < 0) {
-        error_setg(errp, "can't find a free %d-IRQ block", num);
-        return -1;
-    }
-
-    return first + ics->offset;
-}
-
-int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp)
-{
-    ICSState *ics = spapr->ics;
-
-    assert(ics);
-
-    if (!ics_valid_irq(ics, irq)) {
-        error_setg(errp, "IRQ %d is invalid", irq);
-        return -1;
-    }
-
-    if (!ICS_IRQ_FREE(ics, irq - ics->offset)) {
-        error_setg(errp, "IRQ %d is not free", irq);
-        return -1;
-    }
-
-    ics_set_irq_type(ics, irq - ics->offset, lsi);
-    return 0;
-}
-
-void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num)
-{
-    ICSState *ics = spapr->ics;
-    int srcno = irq - ics->offset;
-    int i;
-
-    if (ics_valid_irq(ics, irq)) {
-        trace_spapr_irq_free(0, irq, num);
-        for (i = srcno; i < srcno + num; ++i) {
-            if (ICS_IRQ_FREE(ics, i)) {
-                trace_spapr_irq_free_warn(0, i + ics->offset);
-            }
-            memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
-        }
-    }
-}
-
-qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
-{
-    ICSState *ics = spapr->ics;
-
-    if (ics_valid_irq(ics, irq)) {
-        return ics->qirqs[irq - ics->offset];
-    }
-
-    return NULL;
-}
-
 static void spapr_pic_print_info(InterruptStatsProvider *obj,
                                  Monitor *mon)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
-    CPUState *cs;
-
-    CPU_FOREACH(cs) {
-        PowerPCCPU *cpu = POWERPC_CPU(cs);
-
-        icp_pic_print_info(ICP(cpu->intc), mon);
-    }
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
 
-    ics_pic_print_info(spapr->ics, mon);
+    smc->irq->print_info(spapr, mon);
 }
 
 int spapr_get_vcpu_id(PowerPCCPU *cpu)
@@ -4036,6 +3863,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
     smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 16; /* 64kiB */
     spapr_caps_add_properties(smc, &error_abort);
+    smc->irq = &spapr_irq_xics;
 }
 
 static const TypeInfo spapr_machine_info = {
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 993759db47fa..fb091995a11f 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -11,6 +11,7 @@
 #include "hw/ppc/spapr_cpu_core.h"
 #include "target/ppc/cpu.h"
 #include "hw/ppc/spapr.h"
+#include "hw/ppc/xics.h" /* for icp_create() - to be removed */
 #include "hw/boards.h"
 #include "qapi/error.h"
 #include "sysemu/cpus.h"
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 24e9c1d4433c..0cbb5dd39368 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -13,6 +13,9 @@
 #include "qapi/error.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
+#include "sysemu/kvm.h"
+
+#include "trace.h"
 
 void spapr_irq_msi_init(sPAPRMachineState *spapr, uint32_t nr_msis)
 {
@@ -54,3 +57,230 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr)
 {
     bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
 }
+
+
+/*
+ * XICS IRQ backend.
+ */
+
+static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
+                                  const char *type_ics,
+                                  int nr_irqs, Error **errp)
+{
+    Error *local_err = NULL;
+    Object *obj;
+
+    obj = object_new(type_ics);
+    object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
+    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
+                                   &error_abort);
+    object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
+    if (local_err) {
+        goto error;
+    }
+    object_property_set_bool(obj, true, "realized", &local_err);
+    if (local_err) {
+        goto error;
+    }
+
+    return ICS_BASE(obj);
+
+error:
+    error_propagate(errp, local_err);
+    return NULL;
+}
+
+static void spapr_irq_init_xics(sPAPRMachineState *spapr, Error **errp)
+{
+    MachineState *machine = MACHINE(spapr);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+    int nr_irqs = smc->irq->nr_irqs;
+    Error *local_err = NULL;
+
+    /* Initialize the MSI IRQ allocator. */
+    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+        spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
+    }
+
+    if (kvm_enabled()) {
+        if (machine_kernel_irqchip_allowed(machine) &&
+            !xics_kvm_init(spapr, &local_err)) {
+            spapr->icp_type = TYPE_KVM_ICP;
+            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
+                                          &local_err);
+        }
+        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
+            error_prepend(&local_err,
+                          "kernel_irqchip requested but unavailable: ");
+            goto error;
+        }
+        error_free(local_err);
+        local_err = NULL;
+    }
+
+    if (!spapr->ics) {
+        xics_spapr_init(spapr);
+        spapr->icp_type = TYPE_ICP;
+        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
+                                      &local_err);
+    }
+
+error:
+    error_propagate(errp, local_err);
+}
+
+#define ICS_IRQ_FREE(ics, srcno)   \
+    (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK)))
+
+static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi,
+                                Error **errp)
+{
+    ICSState *ics = spapr->ics;
+
+    assert(ics);
+
+    if (!ics_valid_irq(ics, irq)) {
+        error_setg(errp, "IRQ %d is invalid", irq);
+        return -1;
+    }
+
+    if (!ICS_IRQ_FREE(ics, irq - ics->offset)) {
+        error_setg(errp, "IRQ %d is not free", irq);
+        return -1;
+    }
+
+    ics_set_irq_type(ics, irq - ics->offset, lsi);
+    return 0;
+}
+
+static void spapr_irq_free_xics(sPAPRMachineState *spapr, int irq, int num)
+{
+    ICSState *ics = spapr->ics;
+    uint32_t srcno = irq - ics->offset;
+    int i;
+
+    if (ics_valid_irq(ics, irq)) {
+        trace_spapr_irq_free(0, irq, num);
+        for (i = srcno; i < srcno + num; ++i) {
+            if (ICS_IRQ_FREE(ics, i)) {
+                trace_spapr_irq_free_warn(0, i);
+            }
+            memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
+        }
+    }
+}
+
+static qemu_irq spapr_qirq_xics(sPAPRMachineState *spapr, int irq)
+{
+    ICSState *ics = spapr->ics;
+    uint32_t srcno = irq - ics->offset;
+
+    if (ics_valid_irq(ics, irq)) {
+        return ics->qirqs[srcno];
+    }
+
+    return NULL;
+}
+
+static void spapr_irq_print_info_xics(sPAPRMachineState *spapr, Monitor *mon)
+{
+    CPUState *cs;
+
+    CPU_FOREACH(cs) {
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+        icp_pic_print_info(ICP(cpu->intc), mon);
+    }
+
+    ics_pic_print_info(spapr->ics, mon);
+}
+
+sPAPRIrq spapr_irq_xics = {
+    .nr_irqs     = XICS_IRQS_SPAPR,
+
+    .init        = spapr_irq_init_xics,
+    .claim       = spapr_irq_claim_xics,
+    .free        = spapr_irq_free_xics,
+    .qirq        = spapr_qirq_xics,
+    .print_info  = spapr_irq_print_info_xics,
+};
+
+/*
+ * sPAPR IRQ frontend routines for devices
+ */
+
+int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    return smc->irq->claim(spapr, irq, lsi, errp);
+}
+
+void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    smc->irq->free(spapr, irq, num);
+}
+
+qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    return smc->irq->qirq(spapr, irq);
+}
+
+/*
+ * XICS legacy routines - to deprecate one day
+ */
+
+static int ics_find_free_block(ICSState *ics, int num, int alignnum)
+{
+    int first, i;
+
+    for (first = 0; first < ics->nr_irqs; first += alignnum) {
+        if (num > (ics->nr_irqs - first)) {
+            return -1;
+        }
+        for (i = first; i < first + num; ++i) {
+            if (!ICS_IRQ_FREE(ics, i)) {
+                break;
+            }
+        }
+        if (i == (first + num)) {
+            return first;
+        }
+    }
+
+    return -1;
+}
+
+int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
+{
+    ICSState *ics = spapr->ics;
+    int first = -1;
+
+    assert(ics);
+
+    /*
+     * MSIMesage::data is used for storing VIRQ so
+     * it has to be aligned to num to support multiple
+     * MSI vectors. MSI-X is not affected by this.
+     * The hint is used for the first IRQ, the rest should
+     * be allocated continuously.
+     */
+    if (align) {
+        assert((num == 1) || (num == 2) || (num == 4) ||
+               (num == 8) || (num == 16) || (num == 32));
+        first = ics_find_free_block(ics, num, num);
+    } else {
+        first = ics_find_free_block(ics, num, 1);
+    }
+
+    if (first < 0) {
+        error_setg(errp, "can't find a free %d-IRQ block", num);
+        return -1;
+    }
+
+    return first + ics->offset;
+}
-- 
2.17.1

On Thu, Jul 26, 2018 at 03:37:22PM +0200, Cédric Le Goater wrote:
> This proposal moves all the related IRQ routines of the sPAPR machine
> behind a sPAPR IRQ backend interface 'spapr_irq' to prepare for future
> changes. First of which will be to increase the size of the IRQ number
> space, then, will follow a new backend for the POWER9 XIVE IRQ controller.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

This is looking pretty reasonable, some comments below, though.

> ---
>  include/hw/ppc/spapr.h     |  11 +-
>  include/hw/ppc/spapr_irq.h |  22 ++++
>  hw/ppc/spapr.c             | 180 +----------------------------
>  hw/ppc/spapr_cpu_core.c    |   1 +
>  hw/ppc/spapr_irq.c         | 230 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 259 insertions(+), 185 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 73067f5ee8aa..ad4d7cfd97b0 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -4,7 +4,6 @@
>  #include "qemu/units.h"
>  #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"
> @@ -16,6 +15,7 @@ struct sPAPRNVRAM;
>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>  typedef struct sPAPREventSource sPAPREventSource;
>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> +typedef struct ICSState ICSState;
>  
>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>  #define SPAPR_ENTRY_POINT       0x100
> @@ -110,6 +110,7 @@ struct sPAPRMachineClass {
>                            unsigned n_dma, uint32_t *liobns, Error **errp);
>      sPAPRResizeHPT resize_hpt_default;
>      sPAPRCapabilities default_caps;
> +    sPAPRIrq *irq;
>  };
>  
>  /**
> @@ -780,14 +781,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_find(sPAPRMachineState *spapr, int num, bool align,
> -                   Error **errp);
> -#define spapr_irq_findone(spapr, errp) spapr_irq_find(spapr, 1, false, errp)
> -int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, 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
> index 6f7f50548809..0e98c4474bb2 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -29,4 +29,26 @@ int spapr_irq_msi_alloc(sPAPRMachineState *spapr, uint32_t num, bool align,
>  void spapr_irq_msi_free(sPAPRMachineState *spapr, int irq, uint32_t num);
>  void spapr_irq_msi_reset(sPAPRMachineState *spapr);
>  
> +typedef struct sPAPRIrq {
> +    uint32_t    nr_irqs;
> +
> +    void (*init)(sPAPRMachineState *spapr, Error **errp);
> +    int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
> +    void (*free)(sPAPRMachineState *spapr, int irq, int num);
> +    qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
> +    void (*print_info)(sPAPRMachineState *spapr, Monitor *mon);
> +} sPAPRIrq;
> +
> +extern sPAPRIrq spapr_irq_xics;
> +
> +int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
> +void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
> +qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> +
> +/*
> + * XICS legacy routines
> + */
> +int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp);
> +#define spapr_irq_findone(spapr, errp) spapr_irq_find(spapr, 1, false, errp)
> +
>  #endif
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1794952d4e2a..693144e40d49 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -54,7 +54,6 @@
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_vio.h"
>  #include "hw/pci-host/spapr.h"
> -#include "hw/ppc/xics.h"
>  #include "hw/pci/msi.h"
>  
>  #include "hw/pci/pci.h"
> @@ -117,33 +116,6 @@ static bool spapr_is_thread0_in_vcore(sPAPRMachineState *spapr,
>      return spapr_get_vcpu_id(cpu) % spapr->vsmt == 0;
>  }
>  
> -static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> -                                  const char *type_ics,
> -                                  int nr_irqs, Error **errp)
> -{
> -    Error *local_err = NULL;
> -    Object *obj;
> -
> -    obj = object_new(type_ics);
> -    object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
> -    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
> -                                   &error_abort);
> -    object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
> -    if (local_err) {
> -        goto error;
> -    }
> -    object_property_set_bool(obj, true, "realized", &local_err);
> -    if (local_err) {
> -        goto error;
> -    }
> -
> -    return ICS_BASE(obj);
> -
> -error:
> -    error_propagate(errp, local_err);
> -    return NULL;
> -}
> -
>  static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
>  {
>      /* Dummy entries correspond to unused ICPState objects in older QEMUs,
> @@ -184,43 +156,6 @@ static int xics_max_server_number(sPAPRMachineState *spapr)
>      return DIV_ROUND_UP(max_cpus * spapr->vsmt, smp_threads);
>  }
>  
> -static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> -{
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> -    Error *local_err = NULL;
> -
> -    /* Initialize the MSI IRQ allocator. */
> -    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> -        spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
> -    }
> -
> -    if (kvm_enabled()) {
> -        if (machine_kernel_irqchip_allowed(machine) &&
> -            !xics_kvm_init(spapr, &local_err)) {
> -            spapr->icp_type = TYPE_KVM_ICP;
> -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> -                                          &local_err);
> -        }
> -        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> -            error_prepend(&local_err,
> -                          "kernel_irqchip requested but unavailable: ");
> -            goto error;
> -        }
> -        error_free(local_err);
> -        local_err = NULL;
> -    }
> -
> -    if (!spapr->ics) {
> -        xics_spapr_init(spapr);
> -        spapr->icp_type = TYPE_ICP;
> -        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> -                                      &local_err);
> -    }
> -
> -error:
> -    error_propagate(errp, local_err);
> -}
> -
>  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
>                                    int smt_threads)
>  {
> @@ -2618,7 +2553,7 @@ static void spapr_machine_init(MachineState *machine)
>      load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
>  
>      /* Set up Interrupt Controller before we create the VCPUs */
> -    xics_system_init(machine, XICS_IRQS_SPAPR, &error_fatal);
> +    smc->irq->init(spapr, &error_fatal);
>  
>      /* Set up containers for ibm,client-architecture-support negotiated options
>       */
> @@ -3810,121 +3745,13 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
>      return cpu ? ICP(cpu->intc) : NULL;
>  }
>  
> -#define ICS_IRQ_FREE(ics, srcno)   \
> -    (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK)))
> -
> -static int ics_find_free_block(ICSState *ics, int num, int alignnum)
> -{
> -    int first, i;
> -
> -    for (first = 0; first < ics->nr_irqs; first += alignnum) {
> -        if (num > (ics->nr_irqs - first)) {
> -            return -1;
> -        }
> -        for (i = first; i < first + num; ++i) {
> -            if (!ICS_IRQ_FREE(ics, i)) {
> -                break;
> -            }
> -        }
> -        if (i == (first + num)) {
> -            return first;
> -        }
> -    }
> -
> -    return -1;
> -}
> -
> -int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
> -{
> -    ICSState *ics = spapr->ics;
> -    int first = -1;
> -
> -    assert(ics);
> -
> -    /*
> -     * MSIMesage::data is used for storing VIRQ so
> -     * it has to be aligned to num to support multiple
> -     * MSI vectors. MSI-X is not affected by this.
> -     * The hint is used for the first IRQ, the rest should
> -     * be allocated continuously.
> -     */
> -    if (align) {
> -        assert((num == 1) || (num == 2) || (num == 4) ||
> -               (num == 8) || (num == 16) || (num == 32));
> -        first = ics_find_free_block(ics, num, num);
> -    } else {
> -        first = ics_find_free_block(ics, num, 1);
> -    }
> -
> -    if (first < 0) {
> -        error_setg(errp, "can't find a free %d-IRQ block", num);
> -        return -1;
> -    }
> -
> -    return first + ics->offset;
> -}
> -
> -int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp)
> -{
> -    ICSState *ics = spapr->ics;
> -
> -    assert(ics);
> -
> -    if (!ics_valid_irq(ics, irq)) {
> -        error_setg(errp, "IRQ %d is invalid", irq);
> -        return -1;
> -    }
> -
> -    if (!ICS_IRQ_FREE(ics, irq - ics->offset)) {
> -        error_setg(errp, "IRQ %d is not free", irq);
> -        return -1;
> -    }
> -
> -    ics_set_irq_type(ics, irq - ics->offset, lsi);
> -    return 0;
> -}
> -
> -void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num)
> -{
> -    ICSState *ics = spapr->ics;
> -    int srcno = irq - ics->offset;
> -    int i;
> -
> -    if (ics_valid_irq(ics, irq)) {
> -        trace_spapr_irq_free(0, irq, num);
> -        for (i = srcno; i < srcno + num; ++i) {
> -            if (ICS_IRQ_FREE(ics, i)) {
> -                trace_spapr_irq_free_warn(0, i + ics->offset);
> -            }
> -            memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
> -        }
> -    }
> -}
> -
> -qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
> -{
> -    ICSState *ics = spapr->ics;
> -
> -    if (ics_valid_irq(ics, irq)) {
> -        return ics->qirqs[irq - ics->offset];
> -    }
> -
> -    return NULL;
> -}
> -
>  static void spapr_pic_print_info(InterruptStatsProvider *obj,
>                                   Monitor *mon)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> -    CPUState *cs;
> -
> -    CPU_FOREACH(cs) {
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -
> -        icp_pic_print_info(ICP(cpu->intc), mon);
> -    }
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>  
> -    ics_pic_print_info(spapr->ics, mon);
> +    smc->irq->print_info(spapr, mon);
>  }
>  
>  int spapr_get_vcpu_id(PowerPCCPU *cpu)
> @@ -4036,6 +3863,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
>      smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 16; /* 64kiB */
>      spapr_caps_add_properties(smc, &error_abort);
> +    smc->irq = &spapr_irq_xics;
>  }
>  
>  static const TypeInfo spapr_machine_info = {
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 993759db47fa..fb091995a11f 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -11,6 +11,7 @@
>  #include "hw/ppc/spapr_cpu_core.h"
>  #include "target/ppc/cpu.h"
>  #include "hw/ppc/spapr.h"
> +#include "hw/ppc/xics.h" /* for icp_create() - to be removed */
>  #include "hw/boards.h"
>  #include "qapi/error.h"
>  #include "sysemu/cpus.h"
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 24e9c1d4433c..0cbb5dd39368 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -13,6 +13,9 @@
>  #include "qapi/error.h"
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
> +#include "sysemu/kvm.h"
> +
> +#include "trace.h"
>  
>  void spapr_irq_msi_init(sPAPRMachineState *spapr, uint32_t nr_msis)
>  {
> @@ -54,3 +57,230 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr)
>  {
>      bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
>  }
> +
> +
> +/*
> + * XICS IRQ backend.
> + */
> +
> +static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> +                                  const char *type_ics,
> +                                  int nr_irqs, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    Object *obj;
> +
> +    obj = object_new(type_ics);
> +    object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
> +    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
> +                                   &error_abort);
> +    object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
> +    if (local_err) {
> +        goto error;
> +    }
> +    object_property_set_bool(obj, true, "realized", &local_err);
> +    if (local_err) {
> +        goto error;
> +    }
> +
> +    return ICS_BASE(obj);
> +
> +error:
> +    error_propagate(errp, local_err);
> +    return NULL;
> +}
> +
> +static void spapr_irq_init_xics(sPAPRMachineState *spapr, Error **errp)
> +{
> +    MachineState *machine = MACHINE(spapr);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +    int nr_irqs = smc->irq->nr_irqs;
> +    Error *local_err = NULL;
> +
> +    /* Initialize the MSI IRQ allocator. */
> +    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> +        spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
> +    }

I think this belongs in common code, rather than xics specific.

> +
> +    if (kvm_enabled()) {
> +        if (machine_kernel_irqchip_allowed(machine) &&
> +            !xics_kvm_init(spapr, &local_err)) {
> +            spapr->icp_type = TYPE_KVM_ICP;
> +            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> +                                          &local_err);
> +        }
> +        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> +            error_prepend(&local_err,
> +                          "kernel_irqchip requested but unavailable: ");
> +            goto error;
> +        }
> +        error_free(local_err);
> +        local_err = NULL;
> +    }
> +
> +    if (!spapr->ics) {

When could this get called with spapr->ics already initialized?

> +        xics_spapr_init(spapr);
> +        spapr->icp_type = TYPE_ICP;
> +        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> +                                      &local_err);
> +    }
> +
> +error:
> +    error_propagate(errp, local_err);
> +}
> +
> +#define ICS_IRQ_FREE(ics, srcno)   \
> +    (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK)))
> +
> +static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi,
> +                                Error **errp)
> +{
> +    ICSState *ics = spapr->ics;
> +
> +    assert(ics);
> +
> +    if (!ics_valid_irq(ics, irq)) {
> +        error_setg(errp, "IRQ %d is invalid", irq);
> +        return -1;
> +    }
> +
> +    if (!ICS_IRQ_FREE(ics, irq - ics->offset)) {
> +        error_setg(errp, "IRQ %d is not free", irq);
> +        return -1;
> +    }
> +
> +    ics_set_irq_type(ics, irq - ics->offset, lsi);
> +    return 0;
> +}
> +
> +static void spapr_irq_free_xics(sPAPRMachineState *spapr, int irq, int num)
> +{
> +    ICSState *ics = spapr->ics;
> +    uint32_t srcno = irq - ics->offset;
> +    int i;
> +
> +    if (ics_valid_irq(ics, irq)) {
> +        trace_spapr_irq_free(0, irq, num);
> +        for (i = srcno; i < srcno + num; ++i) {
> +            if (ICS_IRQ_FREE(ics, i)) {
> +                trace_spapr_irq_free_warn(0, i);
> +            }
> +            memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
> +        }
> +    }
> +}
> +
> +static qemu_irq spapr_qirq_xics(sPAPRMachineState *spapr, int irq)
> +{
> +    ICSState *ics = spapr->ics;
> +    uint32_t srcno = irq - ics->offset;
> +
> +    if (ics_valid_irq(ics, irq)) {
> +        return ics->qirqs[srcno];
> +    }
> +
> +    return NULL;
> +}
> +
> +static void spapr_irq_print_info_xics(sPAPRMachineState *spapr, Monitor *mon)
> +{
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +        icp_pic_print_info(ICP(cpu->intc), mon);
> +    }
> +
> +    ics_pic_print_info(spapr->ics, mon);
> +}
> +
> +sPAPRIrq spapr_irq_xics = {
> +    .nr_irqs     = XICS_IRQS_SPAPR,
> +
> +    .init        = spapr_irq_init_xics,
> +    .claim       = spapr_irq_claim_xics,
> +    .free        = spapr_irq_free_xics,
> +    .qirq        = spapr_qirq_xics,
> +    .print_info  = spapr_irq_print_info_xics,
> +};
> +
> +/*
> + * sPAPR IRQ frontend routines for devices
> + */
> +
> +int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    return smc->irq->claim(spapr, irq, lsi, errp);
> +}
> +
> +void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    smc->irq->free(spapr, irq, num);
> +}
> +
> +qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    return smc->irq->qirq(spapr, irq);
> +}
> +
> +/*
> + * XICS legacy routines - to deprecate one day
> + */
> +
> +static int ics_find_free_block(ICSState *ics, int num, int alignnum)
> +{
> +    int first, i;
> +
> +    for (first = 0; first < ics->nr_irqs; first += alignnum) {
> +        if (num > (ics->nr_irqs - first)) {
> +            return -1;
> +        }
> +        for (i = first; i < first + num; ++i) {
> +            if (!ICS_IRQ_FREE(ics, i)) {
> +                break;
> +            }
> +        }
> +        if (i == (first + num)) {
> +            return first;
> +        }
> +    }
> +
> +    return -1;
> +}
> +
> +int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
> +{
> +    ICSState *ics = spapr->ics;
> +    int first = -1;
> +
> +    assert(ics);
> +
> +    /*
> +     * MSIMesage::data is used for storing VIRQ so
> +     * it has to be aligned to num to support multiple
> +     * MSI vectors. MSI-X is not affected by this.
> +     * The hint is used for the first IRQ, the rest should
> +     * be allocated continuously.
> +     */
> +    if (align) {
> +        assert((num == 1) || (num == 2) || (num == 4) ||
> +               (num == 8) || (num == 16) || (num == 32));
> +        first = ics_find_free_block(ics, num, num);
> +    } else {
> +        first = ics_find_free_block(ics, num, 1);
> +    }
> +
> +    if (first < 0) {
> +        error_setg(errp, "can't find a free %d-IRQ block", num);
> +        return -1;
> +    }
> +
> +    return first + ics->offset;
> +}

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

[ ... ] 

>> +static void spapr_irq_init_xics(sPAPRMachineState *spapr, Error **errp)
>> +{
>> +    MachineState *machine = MACHINE(spapr);
>> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>> +    int nr_irqs = smc->irq->nr_irqs;
>> +    Error *local_err = NULL;
>> +
>> +    /* Initialize the MSI IRQ allocator. */
>> +    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
>> +        spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
>> +    }
> 
> I think this belongs in common code, rather than xics specific.

yes.
 
>> +
>> +    if (kvm_enabled()) {
>> +        if (machine_kernel_irqchip_allowed(machine) &&
>> +            !xics_kvm_init(spapr, &local_err)) {
>> +            spapr->icp_type = TYPE_KVM_ICP;
>> +            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
>> +                                          &local_err);
>> +        }
>> +        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
>> +            error_prepend(&local_err,
>> +                          "kernel_irqchip requested but unavailable: ");
>> +            goto error;
>> +        }
>> +        error_free(local_err);
>> +        local_err = NULL;
>> +    }
>> +
>> +    if (!spapr->ics) {
> 
> When could this get called with spapr->ics already initialized?

if the code above succeeds when running with the KVM accel. 

What are thinking about ? introducing a xics-kvm backend ?   

C.

>> +        xics_spapr_init(spapr);
>> +        spapr->icp_type = TYPE_ICP;
>> +        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
>> +                                      &local_err);
>> +    }
>> +
>> +error:
>> +    error_propagate(errp, local_err);
>> +}
>> +

On Fri, Jul 27, 2018 at 09:04:17AM +0200, Cédric Le Goater wrote:
> [ ... ] 
> 
> >> +static void spapr_irq_init_xics(sPAPRMachineState *spapr, Error **errp)
> >> +{
> >> +    MachineState *machine = MACHINE(spapr);
> >> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> >> +    int nr_irqs = smc->irq->nr_irqs;
> >> +    Error *local_err = NULL;
> >> +
> >> +    /* Initialize the MSI IRQ allocator. */
> >> +    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> >> +        spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
> >> +    }
> > 
> > I think this belongs in common code, rather than xics specific.
> 
> yes.
>  
> >> +
> >> +    if (kvm_enabled()) {
> >> +        if (machine_kernel_irqchip_allowed(machine) &&
> >> +            !xics_kvm_init(spapr, &local_err)) {
> >> +            spapr->icp_type = TYPE_KVM_ICP;
> >> +            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> >> +                                          &local_err);
> >> +        }
> >> +        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> >> +            error_prepend(&local_err,
> >> +                          "kernel_irqchip requested but unavailable: ");
> >> +            goto error;
> >> +        }
> >> +        error_free(local_err);
> >> +        local_err = NULL;
> >> +    }
> >> +
> >> +    if (!spapr->ics) {
> > 
> > When could this get called with spapr->ics already initialized?
> 
> if the code above succeeds when running with the KVM accel. 
> 
> What are thinking about ? introducing a xics-kvm backend ?

Duh, sorry, no I just wasnt reading the earlier code clearly enough.

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

>> +static void spapr_irq_init_xics(sPAPRMachineState *spapr, Error **errp)
>> +{
>> +    MachineState *machine = MACHINE(spapr);
>> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>> +    int nr_irqs = smc->irq->nr_irqs;
>> +    Error *local_err = NULL;
>> +
>> +    /* Initialize the MSI IRQ allocator. */
>> +    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
>> +        spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
>> +    }
> 
> I think this belongs in common code, rather than xics specific.

The XIVE IRQ backend will also need to initialize the MSI allocator but 
the XIVE IRQ number space covers the full number space, it has no IRQ 
number offset. The initialization will differ slightly :

+    /* Initialize the MSI IRQ allocator */
+    spapr_irq_msi_init(spapr, nr_irqs - SPAPR_IRQ_MSI);
+

So I rather keep the call where it is today. This is really a small
duplication.

Thanks,

C.