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

Cédric Le Goater posted 4 patches 7 years, 5 months ago
[Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine
Posted by Cédric Le Goater 7 years, 5 months ago
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_IPI        0x0        /*  1 IRQ per CPU      */
    SPAPR_IRQ_EPOW       0x1000     /*  1 IRQ per device   */
    SPAPR_IRQ_HOTPLUG    0x1001     /*  1 IRQ per device   */
    SPAPR_IRQ_VIO        0x1100     /*  1 IRQ per device   */
    SPAPR_IRQ_PCI_LSI    0x1200     /*  4 IRQs per device  */
    SPAPR_IRQ_PCI_MSI    0x1400     /* 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 <clg@kaod.org>
---
 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 000000000000..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_IPI        0x0     /* 1 IRQ per CPU */
+#define SPAPR_IRQ_EPOW       0x1000  /* 1 IRQ per device */
+#define SPAPR_IRQ_HOTPLUG    0x1001  /* 1 IRQ per device */
+#define SPAPR_IRQ_VIO        0x1100  /* 1 IRQ per device */
+#define SPAPR_IRQ_PCI_LSI    0x1200  /* 4 IRQs per device */
+#define SPAPR_IRQ_PCI_MSI    0x1400  /* 1K IRQs per device covered by
+                                      * a bitmap allocator */
+
+typedef struct sPAPRIrq {
+    uint32_t    nr_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;
+
+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);
+
+#endif /* HW_SPAPR_IRQ_H */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 05a924a5f2da..09f095d73eae 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -116,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_SIMPLE(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,
@@ -183,32 +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);
-
-    if (kvm_enabled()) {
-        if (machine_kernel_irqchip_allowed(machine) &&
-            !xics_kvm_init(spapr, errp)) {
-            spapr->icp_type = TYPE_KVM_ICP;
-            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
-        }
-        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
-            error_prepend(errp, "kernel_irqchip requested but unavailable: ");
-            return;
-        }
-    }
-
-    if (!spapr->ics) {
-        xics_spapr_init(spapr);
-        spapr->icp_type = TYPE_ICP;
-        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
-        if (!spapr->ics) {
-            return;
-        }
-    }
-}
-
 static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
                                   int smt_threads)
 {
@@ -2580,7 +2527,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
      */
@@ -3766,127 +3713,6 @@ 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;
-}
-
-/*
- * Allocate the IRQ number and set the IRQ type, LSI or MSI
- */
-static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi)
-{
-    ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi);
-}
-
-int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp)
-{
-    ICSState *ics = spapr->ics;
-    int irq;
-
-    assert(ics);
-
-    irq = ics_find_free_block(ics, 1, 1);
-    if (irq < 0) {
-        error_setg(errp, "can't allocate IRQ: no IRQ left");
-        return -1;
-    }
-    irq += ics->offset;
-
-    spapr_irq_set_lsi(spapr, irq, lsi);
-    trace_spapr_irq_alloc(irq);
-
-    return irq;
-}
-
-/*
- * Allocate block of consecutive IRQs, and return the number of the first IRQ in
- * the block. If align==true, aligns the first IRQ number to num.
- */
-int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
-                          bool align, Error **errp)
-{
-    ICSState *ics = spapr->ics;
-    int i, 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;
-    }
-
-    first += ics->offset;
-    for (i = first; i < first + num; ++i) {
-        spapr_irq_set_lsi(spapr, i, lsi);
-    }
-
-    trace_spapr_irq_alloc_block(first, num, lsi, align);
-
-    return first;
-}
-
-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)
 {
@@ -4007,6 +3833,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
     smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
     spapr_caps_add_properties(smc, &error_abort);
+    smc->irq = &spapr_irq_default;
 }
 
 static const TypeInfo spapr_machine_info = {
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 64a67439beac..e457c5f18189 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -712,7 +712,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
     spapr->event_sources = spapr_event_sources_new();
 
     spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
-                                 spapr_irq_alloc(spapr, false, &error_fatal));
+                                 spapr_irq_alloc(spapr, SPAPR_IRQ_EPOW, 0,
+                                                 &error_fatal));
 
     /* NOTE: if machine supports modern/dedicated hotplug event source,
      * we add it to the device-tree unconditionally. This means we may
@@ -724,8 +725,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
      */
     if (spapr->use_hotplug_event_source) {
         spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG,
-                                     spapr_irq_alloc(spapr, false,
-                                                     &error_fatal));
+                                     spapr_irq_alloc(spapr, SPAPR_IRQ_HOTPLUG,
+                                                     0, &error_fatal));
     }
 
     spapr->epow_notifier.notify = spapr_powerdown_req;
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
new file mode 100644
index 000000000000..ff6cb1aafd25
--- /dev/null
+++ b/hw/ppc/spapr_irq.c
@@ -0,0 +1,233 @@
+/*
+ * QEMU PowerPC sPAPR IRQ backend
+ *
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "hw/pci/pci.h"
+#include "hw/ppc/spapr.h"
+#include "sysemu/kvm.h"
+#include "trace.h"
+
+/*
+ * Legacy XICS IRQ backend.
+ *
+ * The device IRQ 'range' is used to identify LSIs, and the device
+ * 'index' is unused
+ */
+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_SIMPLE(obj);
+
+error:
+    error_propagate(errp, local_err);
+    return NULL;
+}
+
+static void spapr_irq_init_2_12(sPAPRMachineState *spapr, Error **errp)
+{
+    MachineState *machine = MACHINE(spapr);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
+    uint32_t nr_irqs = smc->irq->nr_irqs;
+
+    if (kvm_enabled()) {
+        if (machine_kernel_irqchip_allowed(machine) &&
+            !xics_kvm_init(spapr, errp)) {
+            spapr->icp_type = TYPE_KVM_ICP;
+            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
+        }
+        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
+            error_prepend(errp, "kernel_irqchip requested but unavailable: ");
+            return;
+        }
+    }
+
+    if (!spapr->ics) {
+        xics_spapr_init(spapr);
+        spapr->icp_type = TYPE_ICP;
+        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
+        if (!spapr->ics) {
+            return;
+        }
+    }
+}
+
+#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;
+}
+
+static int spapr_irq_alloc_2_12(sPAPRMachineState *spapr,
+                                uint32_t range, uint32_t index, Error **errp)
+{
+    ICSState *ics = spapr->ics;
+    bool lsi = (range == SPAPR_IRQ_PCI_LSI);
+    int srcno;
+
+    assert(ics);
+
+    srcno = ics_find_free_block(ics, 1, 1);
+    if (srcno < 0) {
+        error_setg(errp, "can't allocate IRQ: no IRQ left");
+        return -1;
+    }
+
+    ics_set_irq_type(ics, srcno, lsi);
+    trace_spapr_irq_alloc(srcno);
+
+    return ics->offset + srcno;
+}
+
+/*
+ * Allocate block of consecutive IRQs, and return the number of the first IRQ in
+ * the block. If align==true, aligns the first IRQ number to num.
+ */
+static int spapr_irq_alloc_block_2_12(sPAPRMachineState *spapr, uint32_t range,
+                                      uint32_t index, int num, bool align,
+                                      Error **errp)
+{
+    ICSState *ics = spapr->ics;
+    bool lsi = (range == SPAPR_IRQ_PCI_LSI);
+    int i, srcno;
+
+    assert(ics);
+
+    /*
+     * MSIMessage::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.
+     */
+    if (align) {
+        assert((num == 1) || (num == 2) || (num == 4) ||
+               (num == 8) || (num == 16) || (num == 32));
+        srcno = ics_find_free_block(ics, num, num);
+    } else {
+        srcno = ics_find_free_block(ics, num, 1);
+    }
+
+    if (srcno < 0) {
+        error_setg(errp, "can't find a free %d-IRQ block", num);
+        return -1;
+    }
+
+    for (i = srcno; i < srcno + num; ++i) {
+        ics_set_irq_type(ics, i, lsi);
+    }
+
+    trace_spapr_irq_alloc_block(srcno, num, lsi, align);
+
+    return ics->offset + srcno;
+}
+
+static void spapr_irq_free_2_12(sPAPRMachineState *spapr, int irq, int num,
+                                Error **errp)
+{
+    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_2_12(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;
+}
+
+sPAPRIrq spapr_irq_default = {
+    .nr_irqs     = XICS_IRQS_SPAPR,
+    .init        = spapr_irq_init_2_12,
+    .alloc       = spapr_irq_alloc_2_12,
+    .alloc_block = spapr_irq_alloc_block_2_12,
+    .free        = spapr_irq_free_2_12,
+    .qirq        = spapr_qirq_2_12,
+};
+
+int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
+                    Error **errp)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    return smc->irq->alloc(spapr, range, index, errp);
+}
+
+int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
+                          uint32_t index, int num, bool align, Error **errp)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    return smc->irq->alloc_block(spapr, range, index, num, align, errp);
+}
+
+void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num,
+                    Error **errp)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    smc->irq->free(spapr, irq, num, errp);
+}
+
+qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    return smc->irq->qirq(spapr, irq);
+}
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 4fd97ffe4c6e..cca4169fa10b 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -333,7 +333,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
             return;
         }
 
-        spapr_irq_free(spapr, msi->first_irq, msi->num);
+        spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
+        if (err) {
+            error_reportf_err(err, "Can't remove MSIs for device %x: ",
+                              config_addr);
+            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        }
         if (msi_present(pdev)) {
             spapr_msi_setmsg(pdev, 0, false, 0, 0);
         }
@@ -371,8 +376,8 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     }
 
     /* Allocate MSIs */
-    irq = spapr_irq_alloc_block(spapr, req_num, false,
-                           ret_intr_type == RTAS_TYPE_MSI, &err);
+    irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_MSI, phb->index, req_num,
+                                ret_intr_type == RTAS_TYPE_MSI, &err);
     if (err) {
         error_reportf_err(err, "Can't allocate MSIs for device %x: ",
                           config_addr);
@@ -382,7 +387,11 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 
     /* Release previous MSIs */
     if (msi) {
-        spapr_irq_free(spapr, msi->first_irq, msi->num);
+        spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
+        if (err) {
+            error_reportf_err(err, "Can't remove MSIs for device %x: ",
+                              config_addr);
+        }
         g_hash_table_remove(phb->msi, &config_addr);
     }
 
@@ -1696,7 +1705,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
 
     /* Initialize the LSI table */
-    irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
+    irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_LSI, sphb->index,
+                                PCI_NUM_PINS, false, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         error_prepend(errp, "can't allocate LSIs: ");
@@ -2112,6 +2122,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
     _FDT(fdt_setprop(fdt, bus_off, "ranges", &ranges, sizeof_ranges));
     _FDT(fdt_setprop(fdt, bus_off, "reg", &bus_reg, sizeof(bus_reg)));
     _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pci-config-space-type", 0x1));
+    /* TODO: fix the total count of allocatable MSIs per PHB */
     _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pe-total-#msi", XICS_IRQS_SPAPR));
 
     /* Dynamic DMA window */
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index cc064f64fccf..7ec69a29d806 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -416,6 +416,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
     }
 }
 
+/* TODO : poor VIO device indexing ... */
+static uint32_t vio_index;
+
 static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
@@ -455,7 +458,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
         dev->qdev.id = id;
     }
 
-    dev->irq = spapr_irq_alloc(spapr, false, &local_err);
+    dev->irq = spapr_irq_alloc(spapr, SPAPR_IRQ_VIO, vio_index++, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 86d82a6ec3ac..4fe3b7804d43 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -4,7 +4,7 @@ obj-y += ppc.o ppc_booke.o fdt.o
 obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
 obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
 obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
-obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
+obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
 # IBM PowerNV
 obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
 ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
-- 
2.13.6


Re: [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine
Posted by Greg Kurz 7 years, 5 months ago
On Fri, 18 May 2018 18:44:04 +0200
Cédric Le Goater <clg@kaod.org> 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_IPI        0x0        /*  1 IRQ per CPU      */
>     SPAPR_IRQ_EPOW       0x1000     /*  1 IRQ per device   */
>     SPAPR_IRQ_HOTPLUG    0x1001     /*  1 IRQ per device   */
>     SPAPR_IRQ_VIO        0x1100     /*  1 IRQ per device   */
>     SPAPR_IRQ_PCI_LSI    0x1200     /*  4 IRQs per device  */
>     SPAPR_IRQ_PCI_MSI    0x1400     /* 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 <clg@kaod.org>
> ---
>  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 000000000000..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_IPI        0x0     /* 1 IRQ per CPU */
> +#define SPAPR_IRQ_EPOW       0x1000  /* 1 IRQ per device */
> +#define SPAPR_IRQ_HOTPLUG    0x1001  /* 1 IRQ per device */
> +#define SPAPR_IRQ_VIO        0x1100  /* 1 IRQ per device */
> +#define SPAPR_IRQ_PCI_LSI    0x1200  /* 4 IRQs per device */
> +#define SPAPR_IRQ_PCI_MSI    0x1400  /* 1K IRQs per device covered by
> +                                      * a bitmap allocator */
> +
> +typedef struct sPAPRIrq {
> +    uint32_t    nr_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;
> +
> +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);
> +
> +#endif /* HW_SPAPR_IRQ_H */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 05a924a5f2da..09f095d73eae 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -116,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_SIMPLE(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,
> @@ -183,32 +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);
> -
> -    if (kvm_enabled()) {
> -        if (machine_kernel_irqchip_allowed(machine) &&
> -            !xics_kvm_init(spapr, errp)) {
> -            spapr->icp_type = TYPE_KVM_ICP;
> -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
> -        }
> -        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> -            error_prepend(errp, "kernel_irqchip requested but unavailable: ");
> -            return;
> -        }
> -    }
> -
> -    if (!spapr->ics) {
> -        xics_spapr_init(spapr);
> -        spapr->icp_type = TYPE_ICP;
> -        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> -        if (!spapr->ics) {
> -            return;
> -        }
> -    }
> -}
> -
>  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
>                                    int smt_threads)
>  {
> @@ -2580,7 +2527,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
>       */
> @@ -3766,127 +3713,6 @@ 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;
> -}
> -
> -/*
> - * Allocate the IRQ number and set the IRQ type, LSI or MSI
> - */
> -static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi)
> -{
> -    ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi);
> -}
> -
> -int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp)
> -{
> -    ICSState *ics = spapr->ics;
> -    int irq;
> -
> -    assert(ics);
> -
> -    irq = ics_find_free_block(ics, 1, 1);
> -    if (irq < 0) {
> -        error_setg(errp, "can't allocate IRQ: no IRQ left");
> -        return -1;
> -    }
> -    irq += ics->offset;
> -
> -    spapr_irq_set_lsi(spapr, irq, lsi);
> -    trace_spapr_irq_alloc(irq);
> -
> -    return irq;
> -}
> -
> -/*
> - * Allocate block of consecutive IRQs, and return the number of the first IRQ in
> - * the block. If align==true, aligns the first IRQ number to num.
> - */
> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> -                          bool align, Error **errp)
> -{
> -    ICSState *ics = spapr->ics;
> -    int i, 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;
> -    }
> -
> -    first += ics->offset;
> -    for (i = first; i < first + num; ++i) {
> -        spapr_irq_set_lsi(spapr, i, lsi);
> -    }
> -
> -    trace_spapr_irq_alloc_block(first, num, lsi, align);
> -
> -    return first;
> -}
> -
> -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)
>  {
> @@ -4007,6 +3833,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
>      smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
>      spapr_caps_add_properties(smc, &error_abort);
> +    smc->irq = &spapr_irq_default;
>  }
>  
>  static const TypeInfo spapr_machine_info = {
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 64a67439beac..e457c5f18189 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -712,7 +712,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
>      spapr->event_sources = spapr_event_sources_new();
>  
>      spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
> -                                 spapr_irq_alloc(spapr, false, &error_fatal));
> +                                 spapr_irq_alloc(spapr, SPAPR_IRQ_EPOW, 0,
> +                                                 &error_fatal));
>  
>      /* NOTE: if machine supports modern/dedicated hotplug event source,
>       * we add it to the device-tree unconditionally. This means we may
> @@ -724,8 +725,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
>       */
>      if (spapr->use_hotplug_event_source) {
>          spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG,
> -                                     spapr_irq_alloc(spapr, false,
> -                                                     &error_fatal));
> +                                     spapr_irq_alloc(spapr, SPAPR_IRQ_HOTPLUG,
> +                                                     0, &error_fatal));
>      }
>  
>      spapr->epow_notifier.notify = spapr_powerdown_req;
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> new file mode 100644
> index 000000000000..ff6cb1aafd25
> --- /dev/null
> +++ b/hw/ppc/spapr_irq.c
> @@ -0,0 +1,233 @@
> +/*
> + * QEMU PowerPC sPAPR IRQ backend
> + *
> + * 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.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "hw/pci/pci.h"
> +#include "hw/ppc/spapr.h"
> +#include "sysemu/kvm.h"
> +#include "trace.h"
> +
> +/*
> + * Legacy XICS IRQ backend.
> + *
> + * The device IRQ 'range' is used to identify LSIs, and the device
> + * 'index' is unused
> + */
> +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_SIMPLE(obj);
> +
> +error:
> +    error_propagate(errp, local_err);
> +    return NULL;
> +}
> +
> +static void spapr_irq_init_2_12(sPAPRMachineState *spapr, Error **errp)
> +{
> +    MachineState *machine = MACHINE(spapr);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> +    uint32_t nr_irqs = smc->irq->nr_irqs;
> +
> +    if (kvm_enabled()) {
> +        if (machine_kernel_irqchip_allowed(machine) &&
> +            !xics_kvm_init(spapr, errp)) {
> +            spapr->icp_type = TYPE_KVM_ICP;
> +            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
> +        }
> +        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> +            error_prepend(errp, "kernel_irqchip requested but unavailable: ");
> +            return;
> +        }
> +    }
> +
> +    if (!spapr->ics) {
> +        xics_spapr_init(spapr);
> +        spapr->icp_type = TYPE_ICP;
> +        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> +        if (!spapr->ics) {
> +            return;
> +        }
> +    }
> +}
> +
> +#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;
> +}
> +
> +static int spapr_irq_alloc_2_12(sPAPRMachineState *spapr,
> +                                uint32_t range, uint32_t index, Error **errp)
> +{
> +    ICSState *ics = spapr->ics;
> +    bool lsi = (range == SPAPR_IRQ_PCI_LSI);
> +    int srcno;
> +
> +    assert(ics);
> +
> +    srcno = ics_find_free_block(ics, 1, 1);
> +    if (srcno < 0) {
> +        error_setg(errp, "can't allocate IRQ: no IRQ left");
> +        return -1;
> +    }
> +
> +    ics_set_irq_type(ics, srcno, lsi);
> +    trace_spapr_irq_alloc(srcno);
> +
> +    return ics->offset + srcno;
> +}
> +
> +/*
> + * Allocate block of consecutive IRQs, and return the number of the first IRQ in
> + * the block. If align==true, aligns the first IRQ number to num.
> + */
> +static int spapr_irq_alloc_block_2_12(sPAPRMachineState *spapr, uint32_t range,
> +                                      uint32_t index, int num, bool align,
> +                                      Error **errp)
> +{
> +    ICSState *ics = spapr->ics;
> +    bool lsi = (range == SPAPR_IRQ_PCI_LSI);
> +    int i, srcno;
> +
> +    assert(ics);
> +
> +    /*
> +     * MSIMessage::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.
> +     */
> +    if (align) {
> +        assert((num == 1) || (num == 2) || (num == 4) ||
> +               (num == 8) || (num == 16) || (num == 32));
> +        srcno = ics_find_free_block(ics, num, num);
> +    } else {
> +        srcno = ics_find_free_block(ics, num, 1);
> +    }
> +
> +    if (srcno < 0) {
> +        error_setg(errp, "can't find a free %d-IRQ block", num);
> +        return -1;
> +    }
> +
> +    for (i = srcno; i < srcno + num; ++i) {
> +        ics_set_irq_type(ics, i, lsi);
> +    }
> +
> +    trace_spapr_irq_alloc_block(srcno, num, lsi, align);
> +
> +    return ics->offset + srcno;
> +}
> +
> +static void spapr_irq_free_2_12(sPAPRMachineState *spapr, int irq, int num,
> +                                Error **errp)
> +{
> +    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_2_12(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;
> +}
> +
> +sPAPRIrq spapr_irq_default = {
> +    .nr_irqs     = XICS_IRQS_SPAPR,
> +    .init        = spapr_irq_init_2_12,
> +    .alloc       = spapr_irq_alloc_2_12,
> +    .alloc_block = spapr_irq_alloc_block_2_12,
> +    .free        = spapr_irq_free_2_12,
> +    .qirq        = spapr_qirq_2_12,
> +};
> +
> +int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> +                    Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    return smc->irq->alloc(spapr, range, index, errp);
> +}
> +
> +int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
> +                          uint32_t index, int num, bool align, Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    return smc->irq->alloc_block(spapr, range, index, num, align, errp);
> +}
> +
> +void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num,
> +                    Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    smc->irq->free(spapr, irq, num, errp);
> +}
> +
> +qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    return smc->irq->qirq(spapr, irq);
> +}
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 4fd97ffe4c6e..cca4169fa10b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -333,7 +333,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>              return;
>          }
>  
> -        spapr_irq_free(spapr, msi->first_irq, msi->num);
> +        spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
> +        if (err) {
> +            error_reportf_err(err, "Can't remove MSIs for device %x: ",
> +                              config_addr);
> +            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> +        }
>          if (msi_present(pdev)) {
>              spapr_msi_setmsg(pdev, 0, false, 0, 0);
>          }
> @@ -371,8 +376,8 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      }
>  
>      /* Allocate MSIs */
> -    irq = spapr_irq_alloc_block(spapr, req_num, false,
> -                           ret_intr_type == RTAS_TYPE_MSI, &err);
> +    irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_MSI, phb->index, req_num,
> +                                ret_intr_type == RTAS_TYPE_MSI, &err);
>      if (err) {
>          error_reportf_err(err, "Can't allocate MSIs for device %x: ",
>                            config_addr);
> @@ -382,7 +387,11 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  
>      /* Release previous MSIs */
>      if (msi) {
> -        spapr_irq_free(spapr, msi->first_irq, msi->num);
> +        spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
> +        if (err) {
> +            error_reportf_err(err, "Can't remove MSIs for device %x: ",
> +                              config_addr);
> +        }
>          g_hash_table_remove(phb->msi, &config_addr);
>      }
>  
> @@ -1696,7 +1705,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>  
>      /* Initialize the LSI table */
> -    irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
> +    irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_LSI, sphb->index,
> +                                PCI_NUM_PINS, false, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          error_prepend(errp, "can't allocate LSIs: ");
> @@ -2112,6 +2122,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>      _FDT(fdt_setprop(fdt, bus_off, "ranges", &ranges, sizeof_ranges));
>      _FDT(fdt_setprop(fdt, bus_off, "reg", &bus_reg, sizeof(bus_reg)));
>      _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pci-config-space-type", 0x1));
> +    /* TODO: fix the total count of allocatable MSIs per PHB */
>      _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pe-total-#msi", XICS_IRQS_SPAPR));

I agree it is quite confusing that every PHB advertises the machine's total :-\

>  
>      /* Dynamic DMA window */
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index cc064f64fccf..7ec69a29d806 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -416,6 +416,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>      }
>  }
>  
> +/* TODO : poor VIO device indexing ... */
> +static uint32_t vio_index;

I guess we don't really care as we don't (and likely never will) support hotplug
of VIO devices.

This patch looks good for me.

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

> +
>  static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> @@ -455,7 +458,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>          dev->qdev.id = id;
>      }
>  
> -    dev->irq = spapr_irq_alloc(spapr, false, &local_err);
> +    dev->irq = spapr_irq_alloc(spapr, SPAPR_IRQ_VIO, vio_index++, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 86d82a6ec3ac..4fe3b7804d43 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -4,7 +4,7 @@ obj-y += ppc.o ppc_booke.o fdt.o
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> -obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
> +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
>  # IBM PowerNV
>  obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)


Re: [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine
Posted by David Gibson 7 years, 4 months ago
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_IPI        0x0        /*  1 IRQ per CPU      */
>     SPAPR_IRQ_EPOW       0x1000     /*  1 IRQ per device   */
>     SPAPR_IRQ_HOTPLUG    0x1001     /*  1 IRQ per device   */
>     SPAPR_IRQ_VIO        0x1100     /*  1 IRQ per device   */
>     SPAPR_IRQ_PCI_LSI    0x1200     /*  4 IRQs per device  */
>     SPAPR_IRQ_PCI_MSI    0x1400     /* 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 <clg@kaod.org>
> ---
>  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 000000000000..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_IPI        0x0     /* 1 IRQ per CPU */
> +#define SPAPR_IRQ_EPOW       0x1000  /* 1 IRQ per device */
> +#define SPAPR_IRQ_HOTPLUG    0x1001  /* 1 IRQ per device */
> +#define SPAPR_IRQ_VIO        0x1100  /* 1 IRQ per device */
> +#define SPAPR_IRQ_PCI_LSI    0x1200  /* 4 IRQs per device */
> +#define SPAPR_IRQ_PCI_MSI    0x1400  /* 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_t    nr_irqs;
> +
> +    void (*init)(sPAPRMachineState *spapr, Error **errp);
> +    int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> +                 Error **errp);

'range' has no place here - working out the indices should be entirely
on the spapr side, only the final irq number should need to go to the
backend.

I'd also prefer to call it "claim" to distinguish it from the existing
"alloc" function which finds a free irq as well as setting it up for
our exclusive use.

> +    int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range,
> +                       uint32_t index, int num, bool align, Error **errp);

There should be no need for this.  We needed an alloc_block routine
before, because we wanted to ensure that we got a contiguous (and
maybe aligned) block of irqs.  By the time we go to the backend we
should already have absolute irq numbers, so we can just claim each of
them separately.

> +    void (*free)(sPAPRMachineState *spapr, int irq, int num, Error **errp);
> +    qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
> +} sPAPRIrq;
> +
> +extern sPAPRIrq spapr_irq_default;
> +
> +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);
> +
> +#endif /* HW_SPAPR_IRQ_H */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 05a924a5f2da..09f095d73eae 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -116,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_SIMPLE(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,
> @@ -183,32 +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);
> -
> -    if (kvm_enabled()) {
> -        if (machine_kernel_irqchip_allowed(machine) &&
> -            !xics_kvm_init(spapr, errp)) {
> -            spapr->icp_type = TYPE_KVM_ICP;
> -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
> -        }
> -        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> -            error_prepend(errp, "kernel_irqchip requested but unavailable: ");
> -            return;
> -        }
> -    }
> -
> -    if (!spapr->ics) {
> -        xics_spapr_init(spapr);
> -        spapr->icp_type = TYPE_ICP;
> -        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> -        if (!spapr->ics) {
> -            return;
> -        }
> -    }
> -}
> -
>  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
>                                    int smt_threads)
>  {
> @@ -2580,7 +2527,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
>       */
> @@ -3766,127 +3713,6 @@ 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;
> -}
> -
> -/*
> - * Allocate the IRQ number and set the IRQ type, LSI or MSI
> - */
> -static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi)
> -{
> -    ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi);
> -}
> -
> -int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp)
> -{
> -    ICSState *ics = spapr->ics;
> -    int irq;
> -
> -    assert(ics);
> -
> -    irq = ics_find_free_block(ics, 1, 1);
> -    if (irq < 0) {
> -        error_setg(errp, "can't allocate IRQ: no IRQ left");
> -        return -1;
> -    }
> -    irq += ics->offset;
> -
> -    spapr_irq_set_lsi(spapr, irq, lsi);
> -    trace_spapr_irq_alloc(irq);
> -
> -    return irq;
> -}
> -
> -/*
> - * Allocate block of consecutive IRQs, and return the number of the first IRQ in
> - * the block. If align==true, aligns the first IRQ number to num.
> - */
> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> -                          bool align, Error **errp)
> -{
> -    ICSState *ics = spapr->ics;
> -    int i, 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;
> -    }
> -
> -    first += ics->offset;
> -    for (i = first; i < first + num; ++i) {
> -        spapr_irq_set_lsi(spapr, i, lsi);
> -    }
> -
> -    trace_spapr_irq_alloc_block(first, num, lsi, align);
> -
> -    return first;
> -}
> -
> -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)
>  {
> @@ -4007,6 +3833,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
>      smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
>      spapr_caps_add_properties(smc, &error_abort);
> +    smc->irq = &spapr_irq_default;
>  }
>  
>  static const TypeInfo spapr_machine_info = {
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 64a67439beac..e457c5f18189 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -712,7 +712,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
>      spapr->event_sources = spapr_event_sources_new();
>  
>      spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
> -                                 spapr_irq_alloc(spapr, false, &error_fatal));
> +                                 spapr_irq_alloc(spapr, SPAPR_IRQ_EPOW, 0,
> +                                                 &error_fatal));
>  
>      /* NOTE: if machine supports modern/dedicated hotplug event source,
>       * we add it to the device-tree unconditionally. This means we may
> @@ -724,8 +725,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
>       */
>      if (spapr->use_hotplug_event_source) {
>          spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG,
> -                                     spapr_irq_alloc(spapr, false,
> -                                                     &error_fatal));
> +                                     spapr_irq_alloc(spapr, SPAPR_IRQ_HOTPLUG,
> +                                                     0, &error_fatal));
>      }
>  
>      spapr->epow_notifier.notify = spapr_powerdown_req;
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> new file mode 100644
> index 000000000000..ff6cb1aafd25
> --- /dev/null
> +++ b/hw/ppc/spapr_irq.c
> @@ -0,0 +1,233 @@
> +/*
> + * QEMU PowerPC sPAPR IRQ backend
> + *
> + * 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.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "hw/pci/pci.h"
> +#include "hw/ppc/spapr.h"
> +#include "sysemu/kvm.h"
> +#include "trace.h"
> +
> +/*
> + * Legacy XICS IRQ backend.
> + *
> + * The device IRQ 'range' is used to identify LSIs, and the device
> + * 'index' is unused
> + */

Ugh.. ok.  This backend/frontend interface seems to be conflating
things that vary with xics vs. xive (what I'd think of as the "back
end") and things that vary with our irq allocation model.  Those
really need to be separate.

An ops structure might make sense for the xics vs. xive stuff.  I'm
pretty sure it will be overkill for the allocation model.

> +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_SIMPLE(obj);
> +
> +error:
> +    error_propagate(errp, local_err);
> +    return NULL;
> +}
> +
> +static void spapr_irq_init_2_12(sPAPRMachineState *spapr, Error **errp)
> +{
> +    MachineState *machine = MACHINE(spapr);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> +    uint32_t nr_irqs = smc->irq->nr_irqs;
> +
> +    if (kvm_enabled()) {
> +        if (machine_kernel_irqchip_allowed(machine) &&
> +            !xics_kvm_init(spapr, errp)) {
> +            spapr->icp_type = TYPE_KVM_ICP;
> +            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
> +        }
> +        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> +            error_prepend(errp, "kernel_irqchip requested but unavailable: ");
> +            return;
> +        }
> +    }
> +
> +    if (!spapr->ics) {
> +        xics_spapr_init(spapr);
> +        spapr->icp_type = TYPE_ICP;
> +        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> +        if (!spapr->ics) {
> +            return;
> +        }
> +    }
> +}
> +
> +#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;
> +}
> +
> +static int spapr_irq_alloc_2_12(sPAPRMachineState *spapr,
> +                                uint32_t range, uint32_t index, Error **errp)
> +{
> +    ICSState *ics = spapr->ics;
> +    bool lsi = (range == SPAPR_IRQ_PCI_LSI);
> +    int srcno;
> +
> +    assert(ics);
> +
> +    srcno = ics_find_free_block(ics, 1, 1);
> +    if (srcno < 0) {
> +        error_setg(errp, "can't allocate IRQ: no IRQ left");
> +        return -1;
> +    }
> +
> +    ics_set_irq_type(ics, srcno, lsi);
> +    trace_spapr_irq_alloc(srcno);
> +
> +    return ics->offset + srcno;
> +}
> +
> +/*
> + * Allocate block of consecutive IRQs, and return the number of the first IRQ in
> + * the block. If align==true, aligns the first IRQ number to num.
> + */
> +static int spapr_irq_alloc_block_2_12(sPAPRMachineState *spapr, uint32_t range,
> +                                      uint32_t index, int num, bool align,
> +                                      Error **errp)
> +{
> +    ICSState *ics = spapr->ics;
> +    bool lsi = (range == SPAPR_IRQ_PCI_LSI);
> +    int i, srcno;
> +
> +    assert(ics);
> +
> +    /*
> +     * MSIMessage::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.
> +     */
> +    if (align) {
> +        assert((num == 1) || (num == 2) || (num == 4) ||
> +               (num == 8) || (num == 16) || (num == 32));
> +        srcno = ics_find_free_block(ics, num, num);
> +    } else {
> +        srcno = ics_find_free_block(ics, num, 1);
> +    }
> +
> +    if (srcno < 0) {
> +        error_setg(errp, "can't find a free %d-IRQ block", num);
> +        return -1;
> +    }
> +
> +    for (i = srcno; i < srcno + num; ++i) {
> +        ics_set_irq_type(ics, i, lsi);
> +    }
> +
> +    trace_spapr_irq_alloc_block(srcno, num, lsi, align);
> +
> +    return ics->offset + srcno;
> +}
> +
> +static void spapr_irq_free_2_12(sPAPRMachineState *spapr, int irq, int num,
> +                                Error **errp)
> +{
> +    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_2_12(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;
> +}
> +
> +sPAPRIrq spapr_irq_default = {
> +    .nr_irqs     = XICS_IRQS_SPAPR,
> +    .init        = spapr_irq_init_2_12,
> +    .alloc       = spapr_irq_alloc_2_12,
> +    .alloc_block = spapr_irq_alloc_block_2_12,
> +    .free        = spapr_irq_free_2_12,
> +    .qirq        = spapr_qirq_2_12,
> +};
> +
> +int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> +                    Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    return smc->irq->alloc(spapr, range, index, errp);
> +}
> +
> +int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
> +                          uint32_t index, int num, bool align, Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    return smc->irq->alloc_block(spapr, range, index, num, align, errp);
> +}
> +
> +void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num,
> +                    Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    smc->irq->free(spapr, irq, num, errp);
> +}
> +
> +qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> +    return smc->irq->qirq(spapr, irq);
> +}
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 4fd97ffe4c6e..cca4169fa10b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -333,7 +333,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>              return;
>          }
>  
> -        spapr_irq_free(spapr, msi->first_irq, msi->num);
> +        spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
> +        if (err) {
> +            error_reportf_err(err, "Can't remove MSIs for device %x: ",
> +                              config_addr);
> +            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> +        }
>          if (msi_present(pdev)) {
>              spapr_msi_setmsg(pdev, 0, false, 0, 0);
>          }
> @@ -371,8 +376,8 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      }
>  
>      /* Allocate MSIs */
> -    irq = spapr_irq_alloc_block(spapr, req_num, false,
> -                           ret_intr_type == RTAS_TYPE_MSI, &err);
> +    irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_MSI, phb->index, req_num,
> +                                ret_intr_type == RTAS_TYPE_MSI, &err);
>      if (err) {
>          error_reportf_err(err, "Can't allocate MSIs for device %x: ",
>                            config_addr);
> @@ -382,7 +387,11 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  
>      /* Release previous MSIs */
>      if (msi) {
> -        spapr_irq_free(spapr, msi->first_irq, msi->num);
> +        spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
> +        if (err) {
> +            error_reportf_err(err, "Can't remove MSIs for device %x: ",
> +                              config_addr);
> +        }
>          g_hash_table_remove(phb->msi, &config_addr);
>      }
>  
> @@ -1696,7 +1705,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>  
>      /* Initialize the LSI table */
> -    irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
> +    irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_LSI, sphb->index,
> +                                PCI_NUM_PINS, false, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          error_prepend(errp, "can't allocate LSIs: ");
> @@ -2112,6 +2122,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>      _FDT(fdt_setprop(fdt, bus_off, "ranges", &ranges, sizeof_ranges));
>      _FDT(fdt_setprop(fdt, bus_off, "reg", &bus_reg, sizeof(bus_reg)));
>      _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pci-config-space-type", 0x1));
> +    /* TODO: fix the total count of allocatable MSIs per PHB */
>      _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pe-total-#msi", XICS_IRQS_SPAPR));
>  
>      /* Dynamic DMA window */
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index cc064f64fccf..7ec69a29d806 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -416,6 +416,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>      }
>  }
>  
> +/* TODO : poor VIO device indexing ... */
> +static uint32_t vio_index;
> +
>  static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> @@ -455,7 +458,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>          dev->qdev.id = id;
>      }
>  
> -    dev->irq = spapr_irq_alloc(spapr, false, &local_err);
> +    dev->irq = spapr_irq_alloc(spapr, SPAPR_IRQ_VIO, vio_index++, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 86d82a6ec3ac..4fe3b7804d43 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -4,7 +4,7 @@ obj-y += ppc.o ppc_booke.o fdt.o
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> -obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
> +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
>  # IBM PowerNV
>  obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)

-- 
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
Re: [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine
Posted by Cédric Le Goater 7 years, 4 months ago
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_IPI        0x0        /*  1 IRQ per CPU      */
>>     SPAPR_IRQ_EPOW       0x1000     /*  1 IRQ per device   */
>>     SPAPR_IRQ_HOTPLUG    0x1001     /*  1 IRQ per device   */
>>     SPAPR_IRQ_VIO        0x1100     /*  1 IRQ per device   */
>>     SPAPR_IRQ_PCI_LSI    0x1200     /*  4 IRQs per device  */
>>     SPAPR_IRQ_PCI_MSI    0x1400     /* 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_t    nr_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 <clg@kaod.org>
>> ---
>>  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 000000000000..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_IPI        0x0     /* 1 IRQ per CPU */
>> +#define SPAPR_IRQ_EPOW       0x1000  /* 1 IRQ per device */
>> +#define SPAPR_IRQ_HOTPLUG    0x1001  /* 1 IRQ per device */
>> +#define SPAPR_IRQ_VIO        0x1100  /* 1 IRQ per device */
>> +#define SPAPR_IRQ_PCI_LSI    0x1200  /* 4 IRQs per device */
>> +#define SPAPR_IRQ_PCI_MSI    0x1400  /* 1K IRQs per device covered by
>> +                                      * a bitmap allocator */
> 
> These look like they belong in the next patch with the fixed allocations.

They act as ids also, can  be discussed.

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

This is not only XIVE. the static allocation scheme changes all the 
backend.


> Comments below are based on that assumption.
>
>> +    uint32_t    nr_irqs;
>> +
>> +    void (*init)(sPAPRMachineState *spapr, Error **errp);
>> +    int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
>> +                 Error **errp);
> 
> 'range' has no place here - working out the indices should be entirely
> on the spapr side, only the final irq number should need to go to the
> backend.

RAnge identifies the device, see above. Maybe badly named.  

> I'd also prefer to call it "claim" to distinguish it from the existing
> "alloc" function which finds a free irq as well as setting it up for
> our exclusive use.
...

>> +    int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range,
>> +                       uint32_t index, int num, bool align, Error **errp);
> 
> There should be no need for this.  We needed an alloc_block routine
> before, because we wanted to ensure that we got a contiguous (and
> maybe aligned) block of irqs.  By the time we go to the backend we
> should already have absolute irq numbers, so we can just claim each of
> them separately.

...

So, David, let's stop wasting our time. please provide what you feel is 
right and I will build on top of it. 
 
For your information, please understand that I generally make sure that 
a patchset fits older and newer machines, that migration is tested and 
that the XIVE patchset is resynced. It takes a HUGE amount of time and 
it's a not a ramdom idea that just looks nice. 

Thanks,

C.