[Qemu-devel] [PATCH v3 0/4] sPAPR xics rework/cleanup (pending)

[Qemu-devel] [PATCH v3 0/4] sPAPR xics rework/cleanup (pending)

[Nikunj A Dadhania]

sPAPR xics related changes required for powernv platform. This brings
infrastructure to get the xics native mode for powernv. Tested pseries guests
in KVM and TCG mode. These are the pending patches of the original set.

Changelog v2:
 * Restore xirr_owner after migration
 * Call icp_resend after restoring all the ICP

Changelog v1:
 * Change XICS to XICS_SPAPR and KVM_XICS to XICS_KVM_SPAPR
 * Added xics_ to function get_cpu_index_by_dt_id as this is a global symbol
 * Dropped server parameter from  icp_check_ipi
 * Send HW_ERROR when ics is NULL
 * Remove redundant parameters in trace routines
 * Use type ICS_SIMPLE, ICS_BASE and ICS_KVM
 * Dropped xics-native and info pic patches for this version

ToDo:
 + Use ICPNative and XICSNative in "native" implementation
 + xics_spapr_alloc - getting rid of that

Benjamin Herrenschmidt (4):
  ppc/xics: Make the ICSState a list
  ppc/xics: An ICS with offset 0 is assumed to be uninitialized
  ppc/xics: Use a helper to add a new ICS
  ppc/xics: Split ICS into ics-base and ics class

 hw/intc/trace-events  |  15 +--
 hw/intc/xics.c        | 281 ++++++++++++++++++++++++++++++++------------------
 hw/intc/xics_kvm.c    |  35 ++++---
 hw/intc/xics_spapr.c  | 112 ++++++++++++--------
 hw/ppc/spapr_events.c |   2 +-
 hw/ppc/spapr_pci.c    |   5 +-
 hw/ppc/spapr_vio.c    |   2 +-
 include/hw/ppc/xics.h |  39 ++++---
 8 files changed, 310 insertions(+), 181 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/4] ppc/xics: Make the ICSState a list

[Nikunj A Dadhania]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Instead of an array of fixed sized blocks, use a list, as we will need
to have sources with variable number of interrupts. SPAPR only uses
a single entry. Native will create more. If performance becomes an
issue we can add some hashed lookup but for now this will do fine.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
[ move the initialization of list to xics_common_initfn,
  restore xirr_owner after migration and move restoring to
  icp_post_load]
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/intc/trace-events  |   5 +-
 hw/intc/xics.c        | 134 ++++++++++++++++++++++++++++++++------------------
 hw/intc/xics_kvm.c    |  27 +++++++---
 hw/intc/xics_spapr.c  |  88 +++++++++++++++++++++------------
 hw/ppc/spapr_events.c |   2 +-
 hw/ppc/spapr_pci.c    |   5 +-
 hw/ppc/spapr_vio.c    |   2 +-
 include/hw/ppc/xics.h |  13 ++---
 8 files changed, 175 insertions(+), 101 deletions(-)

diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 376dd18..41ced33 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -56,10 +56,11 @@ xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
 xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
 xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
 xics_ics_eoi(int nr) "ics_eoi: irq %#x"
-xics_alloc(int src, int irq) "source#%d, irq %d"
-xics_alloc_block(int src, int first, int num, bool lsi, int align) "source#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
+xics_alloc(int irq) "irq %d"
+xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d"
 xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
 xics_ics_free_warn(int src, int irq) "Source#%d, irq %d is already free"
+xics_icp_post_load(uint32_t server_no, uint32_t xirr, uint64_t addr, uint8_t pend) "server_no %d, xirr %#x, xirr_owner 0x%" PRIx64 ", pending %d"
 
 # hw/intc/s390_flic_kvm.c
 flic_create_device(int err) "flic: create device failed %d"
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index cd48f42..0af05c9 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -32,6 +32,7 @@
 #include "hw/hw.h"
 #include "trace.h"
 #include "qemu/timer.h"
+#include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
 #include "qemu/error-report.h"
 #include "qapi/visitor.h"
@@ -96,13 +97,16 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
 static void xics_common_reset(DeviceState *d)
 {
     XICSState *xics = XICS_COMMON(d);
+    ICSState *ics;
     int i;
 
     for (i = 0; i < xics->nr_servers; i++) {
         device_reset(DEVICE(&xics->ss[i]));
     }
 
-    device_reset(DEVICE(xics->ics));
+    QLIST_FOREACH(ics, &xics->ics, list) {
+        device_reset(DEVICE(ics));
+    }
 }
 
 static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char *name,
@@ -134,7 +138,6 @@ static void xics_prop_set_nr_irqs(Object *obj, Visitor *v, const char *name,
     }
 
     assert(info->set_nr_irqs);
-    assert(xics->ics);
     info->set_nr_irqs(xics, value, errp);
 }
 
@@ -174,6 +177,9 @@ static void xics_prop_set_nr_servers(Object *obj, Visitor *v,
 
 static void xics_common_initfn(Object *obj)
 {
+    XICSState *xics = XICS_COMMON(obj);
+
+    QLIST_INIT(&xics->ics);
     object_property_add(obj, "nr_irqs", "int",
                         xics_prop_get_nr_irqs, xics_prop_set_nr_irqs,
                         NULL, NULL, NULL);
@@ -212,33 +218,35 @@ static void ics_reject(ICSState *ics, int nr);
 static void ics_resend(ICSState *ics);
 static void ics_eoi(ICSState *ics, int nr);
 
-static void icp_check_ipi(XICSState *xics, int server)
+static void icp_check_ipi(ICPState *ss)
 {
-    ICPState *ss = xics->ss + server;
-
     if (XISR(ss) && (ss->pending_priority <= ss->mfrr)) {
         return;
     }
 
-    trace_xics_icp_check_ipi(server, ss->mfrr);
+    trace_xics_icp_check_ipi(ss->cs->cpu_index, ss->mfrr);
 
-    if (XISR(ss)) {
-        ics_reject(xics->ics, XISR(ss));
+    if (XISR(ss) && ss->xirr_owner) {
+        ics_reject(ss->xirr_owner, XISR(ss));
     }
 
     ss->xirr = (ss->xirr & ~XISR_MASK) | XICS_IPI;
     ss->pending_priority = ss->mfrr;
+    ss->xirr_owner = NULL;
     qemu_irq_raise(ss->output);
 }
 
 static void icp_resend(XICSState *xics, int server)
 {
     ICPState *ss = xics->ss + server;
+    ICSState *ics;
 
     if (ss->mfrr < CPPR(ss)) {
-        icp_check_ipi(xics, server);
+        icp_check_ipi(ss);
+    }
+    QLIST_FOREACH(ics, &xics->ics, list) {
+        ics_resend(ics);
     }
-    ics_resend(xics->ics);
 }
 
 void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
@@ -256,7 +264,10 @@ void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
             ss->xirr &= ~XISR_MASK; /* Clear XISR */
             ss->pending_priority = 0xff;
             qemu_irq_lower(ss->output);
-            ics_reject(xics->ics, old_xisr);
+            if (ss->xirr_owner) {
+                ics_reject(ss->xirr_owner, old_xisr);
+                ss->xirr_owner = NULL;
+            }
         }
     } else {
         if (!XISR(ss)) {
@@ -271,7 +282,7 @@ void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr)
 
     ss->mfrr = mfrr;
     if (mfrr < CPPR(ss)) {
-        icp_check_ipi(xics, server);
+        icp_check_ipi(ss);
     }
 }
 
@@ -282,6 +293,7 @@ uint32_t icp_accept(ICPState *ss)
     qemu_irq_lower(ss->output);
     ss->xirr = ss->pending_priority << 24;
     ss->pending_priority = 0xff;
+    ss->xirr_owner = NULL;
 
     trace_xics_icp_accept(xirr, ss->xirr);
 
@@ -299,30 +311,40 @@ uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr)
 void icp_eoi(XICSState *xics, int server, uint32_t xirr)
 {
     ICPState *ss = xics->ss + server;
+    ICSState *ics;
+    uint32_t irq;
 
     /* Send EOI -> ICS */
     ss->xirr = (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK);
     trace_xics_icp_eoi(server, xirr, ss->xirr);
-    ics_eoi(xics->ics, xirr & XISR_MASK);
+    irq = xirr & XISR_MASK;
+    QLIST_FOREACH(ics, &xics->ics, list) {
+        if (ics_valid_irq(ics, irq)) {
+            ics_eoi(ics, irq);
+        }
+    }
     if (!XISR(ss)) {
         icp_resend(xics, server);
     }
 }
 
-static void icp_irq(XICSState *xics, int server, int nr, uint8_t priority)
+static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
 {
+    XICSState *xics = ics->xics;
     ICPState *ss = xics->ss + server;
 
     trace_xics_icp_irq(server, nr, priority);
 
     if ((priority >= CPPR(ss))
         || (XISR(ss) && (ss->pending_priority <= priority))) {
-        ics_reject(xics->ics, nr);
+        ics_reject(ics, nr);
     } else {
-        if (XISR(ss)) {
-            ics_reject(xics->ics, XISR(ss));
+        if (XISR(ss) && ss->xirr_owner) {
+            ics_reject(ss->xirr_owner, XISR(ss));
+            ss->xirr_owner = NULL;
         }
         ss->xirr = (ss->xirr & ~XISR_MASK) | (nr & XISR_MASK);
+        ss->xirr_owner = ics;
         ss->pending_priority = priority;
         trace_xics_icp_raise(ss->xirr, ss->pending_priority);
         qemu_irq_raise(ss->output);
@@ -378,12 +400,42 @@ static void icp_reset(DeviceState *dev)
     qemu_set_irq(icp->output, 0);
 }
 
+static int icp_post_load(ICPState *ss, int version_id)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    XICSState *xics = spapr->xics;
+    uint32_t irq, i;
+    static uint32_t server_no = 0;
+
+    server_no++;
+    irq = XISR(ss);
+    if (!ss->cs || !irq)
+        goto icpend;
+
+    /* Restore the xirr_owner */
+    ss->xirr_owner = xics_find_source(xics, irq);
+
+ icpend:
+    trace_xics_icp_post_load(server_no, ss->xirr, (uint64_t)ss->xirr_owner, ss->pending_priority);
+    if (xics->nr_servers != server_no)
+        return 0;
+
+    /* All the ICPs are processed, not restore all the state */
+    for (i = 0; i < xics->nr_servers; i++) {
+        icp_resend(xics, i);
+    }
+    server_no = 0;
+    return 0;
+}
+
 static void icp_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ICPStateClass *icpc = ICP_CLASS(klass);
 
     dc->reset = icp_reset;
     dc->vmsd = &vmstate_icp_server;
+    icpc->post_load = icp_post_load;
 }
 
 static const TypeInfo icp_info = {
@@ -405,8 +457,7 @@ static void resend_msi(ICSState *ics, int srcno)
     if (irq->status & XICS_STATUS_REJECTED) {
         irq->status &= ~XICS_STATUS_REJECTED;
         if (irq->priority != 0xff) {
-            icp_irq(ics->xics, irq->server, srcno + ics->offset,
-                    irq->priority);
+            icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
         }
     }
 }
@@ -419,7 +470,7 @@ static void resend_lsi(ICSState *ics, int srcno)
         && (irq->status & XICS_STATUS_ASSERTED)
         && !(irq->status & XICS_STATUS_SENT)) {
         irq->status |= XICS_STATUS_SENT;
-        icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
+        icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
     }
 }
 
@@ -434,7 +485,7 @@ static void set_irq_msi(ICSState *ics, int srcno, int val)
             irq->status |= XICS_STATUS_MASKED_PENDING;
             trace_xics_masked_pending();
         } else  {
-            icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
+            icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
         }
     }
 }
@@ -473,7 +524,7 @@ static void write_xive_msi(ICSState *ics, int srcno)
     }
 
     irq->status &= ~XICS_STATUS_MASKED_PENDING;
-    icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
+    icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
 }
 
 static void write_xive_lsi(ICSState *ics, int srcno)
@@ -505,8 +556,11 @@ static void ics_reject(ICSState *ics, int nr)
     ICSIRQState *irq = ics->irqs + nr - ics->offset;
 
     trace_xics_ics_reject(nr, nr - ics->offset);
-    irq->status |= XICS_STATUS_REJECTED; /* Irrelevant but harmless for LSI */
-    irq->status &= ~XICS_STATUS_SENT; /* Irrelevant but harmless for MSI */
+    if (irq->flags & XICS_FLAGS_IRQ_MSI) {
+      irq->status |= XICS_STATUS_REJECTED;
+    } else {
+      irq->status &= ~XICS_STATUS_SENT;
+    }
 }
 
 static void ics_resend(ICSState *ics)
@@ -554,17 +608,6 @@ static void ics_reset(DeviceState *dev)
     }
 }
 
-static int ics_post_load(ICSState *ics, int version_id)
-{
-    int i;
-
-    for (i = 0; i < ics->xics->nr_servers; i++) {
-        icp_resend(ics->xics, i);
-    }
-
-    return 0;
-}
-
 static void ics_dispatch_pre_save(void *opaque)
 {
     ICSState *ics = opaque;
@@ -639,12 +682,10 @@ static void ics_realize(DeviceState *dev, Error **errp)
 static void ics_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    ICSStateClass *isc = ICS_CLASS(klass);
 
     dc->realize = ics_realize;
     dc->vmsd = &vmstate_ics;
     dc->reset = ics_reset;
-    isc->post_load = ics_post_load;
 }
 
 static const TypeInfo ics_info = {
@@ -659,28 +700,23 @@ static const TypeInfo ics_info = {
 /*
  * Exported functions
  */
-int xics_find_source(XICSState *xics, int irq)
+ICSState *xics_find_source(XICSState *xics, int irq)
 {
-    int sources = 1;
-    int src;
+    ICSState *ics;
 
-    /* FIXME: implement multiple sources */
-    for (src = 0; src < sources; ++src) {
-        ICSState *ics = &xics->ics[src];
+    QLIST_FOREACH(ics, &xics->ics, list) {
         if (ics_valid_irq(ics, irq)) {
-            return src;
+            return ics;
         }
     }
-
-    return -1;
+    return NULL;
 }
 
 qemu_irq xics_get_qirq(XICSState *xics, int irq)
 {
-    int src = xics_find_source(xics, irq);
+    ICSState *ics = xics_find_source(xics, irq);
 
-    if (src >= 0) {
-        ICSState *ics = &xics->ics[src];
+    if (ics) {
         return ics->qirqs[irq - ics->offset];
     }
 
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index edbd62f..04fa7cb 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -365,7 +365,13 @@ static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
 static void xics_kvm_set_nr_irqs(XICSState *xics, uint32_t nr_irqs,
                                  Error **errp)
 {
-    xics->nr_irqs = xics->ics->nr_irqs = nr_irqs;
+    ICSState *ics = QLIST_FIRST(&xics->ics);
+
+    /* This needs to be deprecated ... */
+    xics->nr_irqs = nr_irqs;
+    if (ics) {
+        ics->nr_irqs = nr_irqs;
+    }
 }
 
 static void xics_kvm_set_nr_servers(XICSState *xics, uint32_t nr_servers,
@@ -398,6 +404,7 @@ static void xics_kvm_realize(DeviceState *dev, Error **errp)
 {
     KVMXICSState *xicskvm = XICS_SPAPR_KVM(dev);
     XICSState *xics = XICS_COMMON(dev);
+    ICSState *ics;
     int i, rc;
     Error *error = NULL;
     struct kvm_create_device xics_create_device = {
@@ -449,10 +456,12 @@ static void xics_kvm_realize(DeviceState *dev, Error **errp)
 
     xicskvm->kernel_xics_fd = xics_create_device.fd;
 
-    object_property_set_bool(OBJECT(xics->ics), true, "realized", &error);
-    if (error) {
-        error_propagate(errp, error);
-        goto fail;
+    QLIST_FOREACH(ics, &xics->ics, list) {
+        object_property_set_bool(OBJECT(ics), true, "realized", &error);
+        if (error) {
+            error_propagate(errp, error);
+            goto fail;
+        }
     }
 
     assert(xics->nr_servers);
@@ -481,10 +490,12 @@ fail:
 static void xics_kvm_initfn(Object *obj)
 {
     XICSState *xics = XICS_COMMON(obj);
+    ICSState *ics;
 
-    xics->ics = ICS(object_new(TYPE_KVM_ICS));
-    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
-    xics->ics->xics = xics;
+    ics = ICS(object_new(TYPE_KVM_ICS));
+    object_property_add_child(obj, "ics", OBJECT(ics), NULL);
+    ics->xics = xics;
+    QLIST_INSERT_HEAD(&xics->ics, ics, list);
 }
 
 static void xics_kvm_class_init(ObjectClass *oc, void *data)
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 618826d..0b0845d 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -113,13 +113,17 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           uint32_t nargs, target_ulong args,
                           uint32_t nret, target_ulong rets)
 {
-    ICSState *ics = spapr->xics->ics;
+    ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
     uint32_t nr, server, priority;
 
     if ((nargs != 3) || (nret != 1)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
         return;
     }
+    if (!ics) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
 
     nr = rtas_ld(args, 0);
     server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
@@ -141,13 +145,17 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           uint32_t nargs, target_ulong args,
                           uint32_t nret, target_ulong rets)
 {
-    ICSState *ics = spapr->xics->ics;
+    ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
     uint32_t nr;
 
     if ((nargs != 1) || (nret != 3)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
         return;
     }
+    if (!ics) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
 
     nr = rtas_ld(args, 0);
 
@@ -166,13 +174,17 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                          uint32_t nargs, target_ulong args,
                          uint32_t nret, target_ulong rets)
 {
-    ICSState *ics = spapr->xics->ics;
+    ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
     uint32_t nr;
 
     if ((nargs != 1) || (nret != 1)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
         return;
     }
+    if (!ics) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
 
     nr = rtas_ld(args, 0);
 
@@ -192,13 +204,17 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                         uint32_t nargs, target_ulong args,
                         uint32_t nret, target_ulong rets)
 {
-    ICSState *ics = spapr->xics->ics;
+    ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
     uint32_t nr;
 
     if ((nargs != 1) || (nret != 1)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
         return;
     }
+    if (!ics) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
 
     nr = rtas_ld(args, 0);
 
@@ -217,7 +233,13 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static void xics_spapr_set_nr_irqs(XICSState *xics, uint32_t nr_irqs,
                                    Error **errp)
 {
-    xics->nr_irqs = xics->ics->nr_irqs = nr_irqs;
+    ICSState *ics = QLIST_FIRST(&xics->ics);
+
+    /* This needs to be deprecated ... */
+    xics->nr_irqs = nr_irqs;
+    if (ics) {
+        ics->nr_irqs = nr_irqs;
+    }
 }
 
 static void xics_spapr_set_nr_servers(XICSState *xics, uint32_t nr_servers,
@@ -240,6 +262,7 @@ static void xics_spapr_set_nr_servers(XICSState *xics, uint32_t nr_servers,
 static void xics_spapr_realize(DeviceState *dev, Error **errp)
 {
     XICSState *xics = XICS_SPAPR(dev);
+    ICSState *ics;
     Error *error = NULL;
     int i;
 
@@ -261,10 +284,12 @@ static void xics_spapr_realize(DeviceState *dev, Error **errp)
     spapr_register_hypercall(H_EOI, h_eoi);
     spapr_register_hypercall(H_IPOLL, h_ipoll);
 
-    object_property_set_bool(OBJECT(xics->ics), true, "realized", &error);
-    if (error) {
-        error_propagate(errp, error);
-        return;
+    QLIST_FOREACH(ics, &xics->ics, list) {
+        object_property_set_bool(OBJECT(ics), true, "realized", &error);
+        if (error) {
+            error_propagate(errp, error);
+            return;
+        }
     }
 
     for (i = 0; i < xics->nr_servers; i++) {
@@ -280,10 +305,12 @@ static void xics_spapr_realize(DeviceState *dev, Error **errp)
 static void xics_spapr_initfn(Object *obj)
 {
     XICSState *xics = XICS_SPAPR(obj);
+    ICSState *ics;
 
-    xics->ics = ICS(object_new(TYPE_ICS));
-    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
-    xics->ics->xics = xics;
+    ics = ICS(object_new(TYPE_ICS));
+    object_property_add_child(obj, "ics", OBJECT(ics), NULL);
+    ics->xics = xics;
+    QLIST_INSERT_HEAD(&xics->ics, ics, list);
 }
 
 static void xics_spapr_class_init(ObjectClass *oc, void *data)
@@ -329,14 +356,15 @@ static int ics_find_free_block(ICSState *ics, int num, int alignnum)
     return -1;
 }
 
-int xics_spapr_alloc(XICSState *xics, int src, int irq_hint, bool lsi,
-                     Error **errp)
+int xics_spapr_alloc(XICSState *xics, int irq_hint, bool lsi, Error **errp)
 {
-    ICSState *ics = &xics->ics[src];
+    ICSState *ics = QLIST_FIRST(&xics->ics);
     int irq;
 
+    if (!ics) {
+        return -1;
+    }
     if (irq_hint) {
-        assert(src == xics_find_source(xics, irq_hint));
         if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) {
             error_setg(errp, "can't allocate IRQ %d: already in use", irq_hint);
             return -1;
@@ -352,7 +380,7 @@ int xics_spapr_alloc(XICSState *xics, int src, int irq_hint, bool lsi,
     }
 
     ics_set_irq_type(ics, irq - ics->offset, lsi);
-    trace_xics_alloc(src, irq);
+    trace_xics_alloc(irq);
 
     return irq;
 }
@@ -361,13 +389,16 @@ int xics_spapr_alloc(XICSState *xics, int src, int irq_hint, bool lsi,
  * 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 xics_spapr_alloc_block(XICSState *xics, int src, int num, bool lsi,
-                           bool align, Error **errp)
+int xics_spapr_alloc_block(XICSState *xics, int num, bool lsi, bool align,
+                           Error **errp)
 {
+    ICSState *ics = QLIST_FIRST(&xics->ics);
     int i, first = -1;
-    ICSState *ics = &xics->ics[src];
 
-    assert(src == 0);
+    if (!ics) {
+        return -1;
+    }
+
     /*
      * MSIMesage::data is used for storing VIRQ so
      * it has to be aligned to num to support multiple
@@ -394,7 +425,7 @@ int xics_spapr_alloc_block(XICSState *xics, int src, int num, bool lsi,
     }
     first += ics->offset;
 
-    trace_xics_alloc_block(src, first, num, lsi, align);
+    trace_xics_alloc_block(first, num, lsi, align);
 
     return first;
 }
@@ -405,7 +436,7 @@ static void ics_free(ICSState *ics, int srcno, int num)
 
     for (i = srcno; i < srcno + num; ++i) {
         if (ICS_IRQ_FREE(ics, i)) {
-            trace_xics_ics_free_warn(ics - ics->xics->ics, i + ics->offset);
+            trace_xics_ics_free_warn(0, i + ics->offset);
         }
         memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
     }
@@ -413,15 +444,10 @@ static void ics_free(ICSState *ics, int srcno, int num)
 
 void xics_spapr_free(XICSState *xics, int irq, int num)
 {
-    int src = xics_find_source(xics, irq);
-
-    if (src >= 0) {
-        ICSState *ics = &xics->ics[src];
-
-        /* FIXME: implement multiple sources */
-        assert(src == 0);
+    ICSState *ics = xics_find_source(xics, irq);
 
-        trace_xics_ics_free(ics - xics->ics, irq, num);
+    if (ics) {
+        trace_xics_ics_free(0, irq, num);
         ics_free(ics, irq - ics->offset, num);
     }
 }
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index b0668b3..adf8da4 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -603,7 +603,7 @@ out_no_events:
 void spapr_events_init(sPAPRMachineState *spapr)
 {
     QTAILQ_INIT(&spapr->pending_events);
-    spapr->check_exception_irq = xics_spapr_alloc(spapr->xics, 0, 0, false,
+    spapr->check_exception_irq = xics_spapr_alloc(spapr->xics, 0, false,
                                             &error_fatal);
     spapr->epow_notifier.notify = spapr_powerdown_req;
     qemu_register_powerdown_notifier(&spapr->epow_notifier);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 949c44f..4d06794 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -362,7 +362,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     }
 
     /* Allocate MSIs */
-    irq = xics_spapr_alloc_block(spapr->xics, 0, req_num, false,
+    irq = xics_spapr_alloc_block(spapr->xics, req_num, false,
                            ret_intr_type == RTAS_TYPE_MSI, &err);
     if (err) {
         error_reportf_err(err, "Can't allocate MSIs for device %x: ",
@@ -1444,8 +1444,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
         uint32_t irq;
         Error *local_err = NULL;
 
-        irq = xics_spapr_alloc_block(spapr->xics, 0, 1, true, false,
-                                     &local_err);
+        irq = xics_spapr_alloc_block(spapr->xics, 1, true, false, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             error_prepend(errp, "can't allocate LSIs: ");
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index f93244d..22360af 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -463,7 +463,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
         dev->qdev.id = id;
     }
 
-    dev->irq = xics_spapr_alloc(spapr->xics, 0, dev->irq, false, &local_err);
+    dev->irq = xics_spapr_alloc(spapr->xics, dev->irq, false, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 6189a3b..6ad3057 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -84,7 +84,7 @@ struct XICSState {
     uint32_t nr_servers;
     uint32_t nr_irqs;
     ICPState *ss;
-    ICSState *ics;
+    QLIST_HEAD(, ICSState) ics;
 };
 
 #define TYPE_ICP "icp"
@@ -110,6 +110,7 @@ struct ICPState {
     DeviceState parent_obj;
     /*< public >*/
     CPUState *cs;
+    ICSState *xirr_owner;
     uint32_t xirr;
     uint8_t pending_priority;
     uint8_t mfrr;
@@ -144,6 +145,7 @@ struct ICSState {
     qemu_irq *qirqs;
     ICSIRQState *irqs;
     XICSState *xics;
+    QLIST_ENTRY(ICSState) list;
 };
 
 static inline bool ics_valid_irq(ICSState *ics, uint32_t nr)
@@ -171,10 +173,9 @@ struct ICSIRQState {
 #define XICS_IRQS_SPAPR               1024
 
 qemu_irq xics_get_qirq(XICSState *icp, int irq);
-int xics_spapr_alloc(XICSState *icp, int src, int irq_hint, bool lsi,
-                     Error **errp);
-int xics_spapr_alloc_block(XICSState *icp, int src, int num, bool lsi,
-                           bool align, Error **errp);
+int xics_spapr_alloc(XICSState *icp, int irq_hint, bool lsi, Error **errp);
+int xics_spapr_alloc_block(XICSState *icp, int num, bool lsi, bool align,
+                           Error **errp);
 void xics_spapr_free(XICSState *icp, int irq, int num);
 
 void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
@@ -194,6 +195,6 @@ void ics_write_xive(ICSState *ics, int nr, int server,
 
 void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
 
-int xics_find_source(XICSState *icp, int irq);
+ICSState *xics_find_source(XICSState *icp, int irq);
 
 #endif /* __XICS_H__ */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 1/4] ppc/xics: Make the ICSState a list

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 28815 bytes --]

On Thu, Jul 07, 2016 at 11:24:15PM +0530, Nikunj A Dadhania wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Instead of an array of fixed sized blocks, use a list, as we will need
> to have sources with variable number of interrupts. SPAPR only uses
> a single entry. Native will create more. If performance becomes an
> issue we can add some hashed lookup but for now this will do fine.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> [ move the initialization of list to xics_common_initfn,
>   restore xirr_owner after migration and move restoring to
>   icp_post_load]
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/intc/trace-events  |   5 +-
>  hw/intc/xics.c        | 134 ++++++++++++++++++++++++++++++++------------------
>  hw/intc/xics_kvm.c    |  27 +++++++---
>  hw/intc/xics_spapr.c  |  88 +++++++++++++++++++++------------
>  hw/ppc/spapr_events.c |   2 +-
>  hw/ppc/spapr_pci.c    |   5 +-
>  hw/ppc/spapr_vio.c    |   2 +-
>  include/hw/ppc/xics.h |  13 ++---
>  8 files changed, 175 insertions(+), 101 deletions(-)
> 
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 376dd18..41ced33 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -56,10 +56,11 @@ xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
>  xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
>  xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
>  xics_ics_eoi(int nr) "ics_eoi: irq %#x"
> -xics_alloc(int src, int irq) "source#%d, irq %d"
> -xics_alloc_block(int src, int first, int num, bool lsi, int align) "source#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
> +xics_alloc(int irq) "irq %d"
> +xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d"
>  xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
>  xics_ics_free_warn(int src, int irq) "Source#%d, irq %d is already free"
> +xics_icp_post_load(uint32_t server_no, uint32_t xirr, uint64_t addr, uint8_t pend) "server_no %d, xirr %#x, xirr_owner 0x%" PRIx64 ", pending %d"
>  
>  # hw/intc/s390_flic_kvm.c
>  flic_create_device(int err) "flic: create device failed %d"
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index cd48f42..0af05c9 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -32,6 +32,7 @@
>  #include "hw/hw.h"
>  #include "trace.h"
>  #include "qemu/timer.h"
> +#include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
>  #include "qemu/error-report.h"
>  #include "qapi/visitor.h"
> @@ -96,13 +97,16 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>  static void xics_common_reset(DeviceState *d)
>  {
>      XICSState *xics = XICS_COMMON(d);
> +    ICSState *ics;
>      int i;
>  
>      for (i = 0; i < xics->nr_servers; i++) {
>          device_reset(DEVICE(&xics->ss[i]));
>      }
>  
> -    device_reset(DEVICE(xics->ics));
> +    QLIST_FOREACH(ics, &xics->ics, list) {
> +        device_reset(DEVICE(ics));
> +    }
>  }
>  
>  static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char *name,
> @@ -134,7 +138,6 @@ static void xics_prop_set_nr_irqs(Object *obj, Visitor *v, const char *name,
>      }
>  
>      assert(info->set_nr_irqs);
> -    assert(xics->ics);
>      info->set_nr_irqs(xics, value, errp);
>  }
>  
> @@ -174,6 +177,9 @@ static void xics_prop_set_nr_servers(Object *obj, Visitor *v,
>  
>  static void xics_common_initfn(Object *obj)
>  {
> +    XICSState *xics = XICS_COMMON(obj);
> +
> +    QLIST_INIT(&xics->ics);
>      object_property_add(obj, "nr_irqs", "int",
>                          xics_prop_get_nr_irqs, xics_prop_set_nr_irqs,
>                          NULL, NULL, NULL);
> @@ -212,33 +218,35 @@ static void ics_reject(ICSState *ics, int nr);
>  static void ics_resend(ICSState *ics);
>  static void ics_eoi(ICSState *ics, int nr);
>  
> -static void icp_check_ipi(XICSState *xics, int server)
> +static void icp_check_ipi(ICPState *ss)
>  {
> -    ICPState *ss = xics->ss + server;
> -
>      if (XISR(ss) && (ss->pending_priority <= ss->mfrr)) {
>          return;
>      }
>  
> -    trace_xics_icp_check_ipi(server, ss->mfrr);
> +    trace_xics_icp_check_ipi(ss->cs->cpu_index, ss->mfrr);

The dt_id might be more useful here, given the changes that are going
on in that area.  But it's not worth holding up this series just for
that.

>  
> -    if (XISR(ss)) {
> -        ics_reject(xics->ics, XISR(ss));
> +    if (XISR(ss) && ss->xirr_owner) {
> +        ics_reject(ss->xirr_owner, XISR(ss));
>      }
>  
>      ss->xirr = (ss->xirr & ~XISR_MASK) | XICS_IPI;
>      ss->pending_priority = ss->mfrr;
> +    ss->xirr_owner = NULL;
>      qemu_irq_raise(ss->output);
>  }
>  
>  static void icp_resend(XICSState *xics, int server)
>  {
>      ICPState *ss = xics->ss + server;
> +    ICSState *ics;

Possibly this should take icp instead of xics for consistency.  Or
else be renamed xics_resend().  But that can be a cleanup for another
time.

>      if (ss->mfrr < CPPR(ss)) {
> -        icp_check_ipi(xics, server);
> +        icp_check_ipi(ss);
> +    }
> +    QLIST_FOREACH(ics, &xics->ics, list) {
> +        ics_resend(ics);
>      }
> -    ics_resend(xics->ics);
>  }
>  
>  void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
> @@ -256,7 +264,10 @@ void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
>              ss->xirr &= ~XISR_MASK; /* Clear XISR */
>              ss->pending_priority = 0xff;
>              qemu_irq_lower(ss->output);
> -            ics_reject(xics->ics, old_xisr);
> +            if (ss->xirr_owner) {
> +                ics_reject(ss->xirr_owner, old_xisr);
> +                ss->xirr_owner = NULL;
> +            }
>          }
>      } else {
>          if (!XISR(ss)) {
> @@ -271,7 +282,7 @@ void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr)
>  
>      ss->mfrr = mfrr;
>      if (mfrr < CPPR(ss)) {
> -        icp_check_ipi(xics, server);
> +        icp_check_ipi(ss);
>      }
>  }
>  
> @@ -282,6 +293,7 @@ uint32_t icp_accept(ICPState *ss)
>      qemu_irq_lower(ss->output);
>      ss->xirr = ss->pending_priority << 24;
>      ss->pending_priority = 0xff;
> +    ss->xirr_owner = NULL;
>  
>      trace_xics_icp_accept(xirr, ss->xirr);
>  
> @@ -299,30 +311,40 @@ uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr)
>  void icp_eoi(XICSState *xics, int server, uint32_t xirr)
>  {
>      ICPState *ss = xics->ss + server;
> +    ICSState *ics;
> +    uint32_t irq;
>  
>      /* Send EOI -> ICS */
>      ss->xirr = (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK);
>      trace_xics_icp_eoi(server, xirr, ss->xirr);
> -    ics_eoi(xics->ics, xirr & XISR_MASK);
> +    irq = xirr & XISR_MASK;
> +    QLIST_FOREACH(ics, &xics->ics, list) {
> +        if (ics_valid_irq(ics, irq)) {
> +            ics_eoi(ics, irq);

You could slightly optimize this by adding a break; here, but again
that can be a tweak for another time.

> +        }
> +    }
>      if (!XISR(ss)) {
>          icp_resend(xics, server);
>      }
>  }
>  
> -static void icp_irq(XICSState *xics, int server, int nr, uint8_t priority)
> +static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
>  {
> +    XICSState *xics = ics->xics;
>      ICPState *ss = xics->ss + server;

It's kind of weird for an icp_() function to take an ics but not an
icp.  It might make more sense for it to take both, even though
obviously it can be derived from just the ics.  Again, won't hold up
the series for this.

>      trace_xics_icp_irq(server, nr, priority);
>  
>      if ((priority >= CPPR(ss))
>          || (XISR(ss) && (ss->pending_priority <= priority))) {
> -        ics_reject(xics->ics, nr);
> +        ics_reject(ics, nr);
>      } else {
> -        if (XISR(ss)) {
> -            ics_reject(xics->ics, XISR(ss));
> +        if (XISR(ss) && ss->xirr_owner) {
> +            ics_reject(ss->xirr_owner, XISR(ss));
> +            ss->xirr_owner = NULL;
>          }
>          ss->xirr = (ss->xirr & ~XISR_MASK) | (nr & XISR_MASK);
> +        ss->xirr_owner = ics;
>          ss->pending_priority = priority;
>          trace_xics_icp_raise(ss->xirr, ss->pending_priority);
>          qemu_irq_raise(ss->output);
> @@ -378,12 +400,42 @@ static void icp_reset(DeviceState *dev)
>      qemu_set_irq(icp->output, 0);
>  }
>  
> +static int icp_post_load(ICPState *ss, int version_id)
> +{
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    XICSState *xics = spapr->xics;
> +    uint32_t irq, i;
> +    static uint32_t server_no = 0;

Eugh.. static locals are usually horrid.

> +    server_no++;
> +    irq = XISR(ss);
> +    if (!ss->cs || !irq)
> +        goto icpend;

That's an awful use of a goto - inverting the if sense is clearer and
no longer.

> +
> +    /* Restore the xirr_owner */
> +    ss->xirr_owner = xics_find_source(xics, irq);

Do we actually need this?  Have you confirmed that replaying the
icp_resend()s won't already accomplish this?

> + icpend:
> +    trace_xics_icp_post_load(server_no, ss->xirr, (uint64_t)ss->xirr_owner, ss->pending_priority);
> +    if (xics->nr_servers != server_no)
> +        return 0;
> +
> +    /* All the ICPs are processed, not restore all the state */
> +    for (i = 0; i < xics->nr_servers; i++) {
> +        icp_resend(xics, i);
> +    }
> +    server_no = 0;

Eugh.  This is really ugly.  It really looks like we need some
migration state on the overall xics, not just the component icps and
icss.  Dealing with backwards compatibility will be hairy, but I
really hope we can do better than this hack.

> +    return 0;
> +}
> +
>  static void icp_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICPStateClass *icpc = ICP_CLASS(klass);
>  
>      dc->reset = icp_reset;
>      dc->vmsd = &vmstate_icp_server;
> +    icpc->post_load = icp_post_load;
>  }
>  
>  static const TypeInfo icp_info = {
> @@ -405,8 +457,7 @@ static void resend_msi(ICSState *ics, int srcno)
>      if (irq->status & XICS_STATUS_REJECTED) {
>          irq->status &= ~XICS_STATUS_REJECTED;
>          if (irq->priority != 0xff) {
> -            icp_irq(ics->xics, irq->server, srcno + ics->offset,
> -                    irq->priority);
> +            icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
>          }
>      }
>  }
> @@ -419,7 +470,7 @@ static void resend_lsi(ICSState *ics, int srcno)
>          && (irq->status & XICS_STATUS_ASSERTED)
>          && !(irq->status & XICS_STATUS_SENT)) {
>          irq->status |= XICS_STATUS_SENT;
> -        icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
> +        icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
>      }
>  }
>  
> @@ -434,7 +485,7 @@ static void set_irq_msi(ICSState *ics, int srcno, int val)
>              irq->status |= XICS_STATUS_MASKED_PENDING;
>              trace_xics_masked_pending();
>          } else  {
> -            icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
> +            icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
>          }
>      }
>  }
> @@ -473,7 +524,7 @@ static void write_xive_msi(ICSState *ics, int srcno)
>      }
>  
>      irq->status &= ~XICS_STATUS_MASKED_PENDING;
> -    icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
> +    icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
>  }
>  
>  static void write_xive_lsi(ICSState *ics, int srcno)
> @@ -505,8 +556,11 @@ static void ics_reject(ICSState *ics, int nr)
>      ICSIRQState *irq = ics->irqs + nr - ics->offset;
>  
>      trace_xics_ics_reject(nr, nr - ics->offset);
> -    irq->status |= XICS_STATUS_REJECTED; /* Irrelevant but harmless for LSI */
> -    irq->status &= ~XICS_STATUS_SENT; /* Irrelevant but harmless for MSI */
> +    if (irq->flags & XICS_FLAGS_IRQ_MSI) {
> +      irq->status |= XICS_STATUS_REJECTED;
> +    } else {
> +      irq->status &= ~XICS_STATUS_SENT;
> +    }

This change appears unrelated to the rest.  If not that needs an explanation.

>  }
>  
>  static void ics_resend(ICSState *ics)
> @@ -554,17 +608,6 @@ static void ics_reset(DeviceState *dev)
>      }
>  }
>  
> -static int ics_post_load(ICSState *ics, int version_id)
> -{
> -    int i;
> -
> -    for (i = 0; i < ics->xics->nr_servers; i++) {
> -        icp_resend(ics->xics, i);
> -    }
> -
> -    return 0;
> -}
> -
>  static void ics_dispatch_pre_save(void *opaque)
>  {
>      ICSState *ics = opaque;
> @@ -639,12 +682,10 @@ static void ics_realize(DeviceState *dev, Error **errp)
>  static void ics_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    ICSStateClass *isc = ICS_CLASS(klass);
>  
>      dc->realize = ics_realize;
>      dc->vmsd = &vmstate_ics;
>      dc->reset = ics_reset;
> -    isc->post_load = ics_post_load;
>  }
>  
>  static const TypeInfo ics_info = {
> @@ -659,28 +700,23 @@ static const TypeInfo ics_info = {
>  /*
>   * Exported functions
>   */
> -int xics_find_source(XICSState *xics, int irq)
> +ICSState *xics_find_source(XICSState *xics, int irq)
>  {
> -    int sources = 1;
> -    int src;
> +    ICSState *ics;
>  
> -    /* FIXME: implement multiple sources */
> -    for (src = 0; src < sources; ++src) {
> -        ICSState *ics = &xics->ics[src];
> +    QLIST_FOREACH(ics, &xics->ics, list) {
>          if (ics_valid_irq(ics, irq)) {
> -            return src;
> +            return ics;
>          }
>      }
> -
> -    return -1;
> +    return NULL;
>  }
>  
>  qemu_irq xics_get_qirq(XICSState *xics, int irq)
>  {
> -    int src = xics_find_source(xics, irq);
> +    ICSState *ics = xics_find_source(xics, irq);
>  
> -    if (src >= 0) {
> -        ICSState *ics = &xics->ics[src];
> +    if (ics) {
>          return ics->qirqs[irq - ics->offset];
>      }
>  
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index edbd62f..04fa7cb 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -365,7 +365,13 @@ static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>  static void xics_kvm_set_nr_irqs(XICSState *xics, uint32_t nr_irqs,
>                                   Error **errp)
>  {
> -    xics->nr_irqs = xics->ics->nr_irqs = nr_irqs;
> +    ICSState *ics = QLIST_FIRST(&xics->ics);
> +
> +    /* This needs to be deprecated ... */
> +    xics->nr_irqs = nr_irqs;
> +    if (ics) {
> +        ics->nr_irqs = nr_irqs;
> +    }
>  }
>  
>  static void xics_kvm_set_nr_servers(XICSState *xics, uint32_t nr_servers,
> @@ -398,6 +404,7 @@ static void xics_kvm_realize(DeviceState *dev, Error **errp)
>  {
>      KVMXICSState *xicskvm = XICS_SPAPR_KVM(dev);
>      XICSState *xics = XICS_COMMON(dev);
> +    ICSState *ics;
>      int i, rc;
>      Error *error = NULL;
>      struct kvm_create_device xics_create_device = {
> @@ -449,10 +456,12 @@ static void xics_kvm_realize(DeviceState *dev, Error **errp)
>  
>      xicskvm->kernel_xics_fd = xics_create_device.fd;
>  
> -    object_property_set_bool(OBJECT(xics->ics), true, "realized", &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        goto fail;
> +    QLIST_FOREACH(ics, &xics->ics, list) {
> +        object_property_set_bool(OBJECT(ics), true, "realized", &error);
> +        if (error) {
> +            error_propagate(errp, error);
> +            goto fail;
> +        }
>      }
>  
>      assert(xics->nr_servers);
> @@ -481,10 +490,12 @@ fail:
>  static void xics_kvm_initfn(Object *obj)
>  {
>      XICSState *xics = XICS_COMMON(obj);
> +    ICSState *ics;
>  
> -    xics->ics = ICS(object_new(TYPE_KVM_ICS));
> -    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
> -    xics->ics->xics = xics;
> +    ics = ICS(object_new(TYPE_KVM_ICS));
> +    object_property_add_child(obj, "ics", OBJECT(ics), NULL);
> +    ics->xics = xics;
> +    QLIST_INSERT_HEAD(&xics->ics, ics, list);
>  }
>  
>  static void xics_kvm_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 618826d..0b0845d 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -113,13 +113,17 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            uint32_t nargs, target_ulong args,
>                            uint32_t nret, target_ulong rets)
>  {
> -    ICSState *ics = spapr->xics->ics;
> +    ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
>      uint32_t nr, server, priority;
>  
>      if ((nargs != 3) || (nret != 1)) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>          return;
>      }
> +    if (!ics) {
> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> +        return;
> +    }
>  
>      nr = rtas_ld(args, 0);
>      server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
> @@ -141,13 +145,17 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            uint32_t nargs, target_ulong args,
>                            uint32_t nret, target_ulong rets)
>  {
> -    ICSState *ics = spapr->xics->ics;
> +    ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
>      uint32_t nr;
>  
>      if ((nargs != 1) || (nret != 3)) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>          return;
>      }
> +    if (!ics) {
> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> +        return;
> +    }
>  
>      nr = rtas_ld(args, 0);
>  
> @@ -166,13 +174,17 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                           uint32_t nargs, target_ulong args,
>                           uint32_t nret, target_ulong rets)
>  {
> -    ICSState *ics = spapr->xics->ics;
> +    ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
>      uint32_t nr;
>  
>      if ((nargs != 1) || (nret != 1)) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>          return;
>      }
> +    if (!ics) {
> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> +        return;
> +    }
>  
>      nr = rtas_ld(args, 0);
>  
> @@ -192,13 +204,17 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                          uint32_t nargs, target_ulong args,
>                          uint32_t nret, target_ulong rets)
>  {
> -    ICSState *ics = spapr->xics->ics;
> +    ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
>      uint32_t nr;
>  
>      if ((nargs != 1) || (nret != 1)) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>          return;
>      }
> +    if (!ics) {
> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> +        return;
> +    }
>  
>      nr = rtas_ld(args, 0);
>  
> @@ -217,7 +233,13 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static void xics_spapr_set_nr_irqs(XICSState *xics, uint32_t nr_irqs,
>                                     Error **errp)
>  {
> -    xics->nr_irqs = xics->ics->nr_irqs = nr_irqs;
> +    ICSState *ics = QLIST_FIRST(&xics->ics);
> +
> +    /* This needs to be deprecated ... */
> +    xics->nr_irqs = nr_irqs;
> +    if (ics) {
> +        ics->nr_irqs = nr_irqs;
> +    }
>  }
>  
>  static void xics_spapr_set_nr_servers(XICSState *xics, uint32_t nr_servers,
> @@ -240,6 +262,7 @@ static void xics_spapr_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>  static void xics_spapr_realize(DeviceState *dev, Error **errp)
>  {
>      XICSState *xics = XICS_SPAPR(dev);
> +    ICSState *ics;
>      Error *error = NULL;
>      int i;
>  
> @@ -261,10 +284,12 @@ static void xics_spapr_realize(DeviceState *dev, Error **errp)
>      spapr_register_hypercall(H_EOI, h_eoi);
>      spapr_register_hypercall(H_IPOLL, h_ipoll);
>  
> -    object_property_set_bool(OBJECT(xics->ics), true, "realized", &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        return;
> +    QLIST_FOREACH(ics, &xics->ics, list) {
> +        object_property_set_bool(OBJECT(ics), true, "realized", &error);
> +        if (error) {
> +            error_propagate(errp, error);
> +            return;
> +        }
>      }
>  
>      for (i = 0; i < xics->nr_servers; i++) {
> @@ -280,10 +305,12 @@ static void xics_spapr_realize(DeviceState *dev, Error **errp)
>  static void xics_spapr_initfn(Object *obj)
>  {
>      XICSState *xics = XICS_SPAPR(obj);
> +    ICSState *ics;
>  
> -    xics->ics = ICS(object_new(TYPE_ICS));
> -    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
> -    xics->ics->xics = xics;
> +    ics = ICS(object_new(TYPE_ICS));
> +    object_property_add_child(obj, "ics", OBJECT(ics), NULL);
> +    ics->xics = xics;
> +    QLIST_INSERT_HEAD(&xics->ics, ics, list);
>  }
>  
>  static void xics_spapr_class_init(ObjectClass *oc, void *data)
> @@ -329,14 +356,15 @@ static int ics_find_free_block(ICSState *ics, int num, int alignnum)
>      return -1;
>  }
>  
> -int xics_spapr_alloc(XICSState *xics, int src, int irq_hint, bool lsi,
> -                     Error **errp)
> +int xics_spapr_alloc(XICSState *xics, int irq_hint, bool lsi, Error **errp)
>  {
> -    ICSState *ics = &xics->ics[src];
> +    ICSState *ics = QLIST_FIRST(&xics->ics);
>      int irq;
>  
> +    if (!ics) {
> +        return -1;
> +    }
>      if (irq_hint) {
> -        assert(src == xics_find_source(xics, irq_hint));
>          if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) {
>              error_setg(errp, "can't allocate IRQ %d: already in use", irq_hint);
>              return -1;
> @@ -352,7 +380,7 @@ int xics_spapr_alloc(XICSState *xics, int src, int irq_hint, bool lsi,
>      }
>  
>      ics_set_irq_type(ics, irq - ics->offset, lsi);
> -    trace_xics_alloc(src, irq);
> +    trace_xics_alloc(irq);
>  
>      return irq;
>  }
> @@ -361,13 +389,16 @@ int xics_spapr_alloc(XICSState *xics, int src, int irq_hint, bool lsi,
>   * 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 xics_spapr_alloc_block(XICSState *xics, int src, int num, bool lsi,
> -                           bool align, Error **errp)
> +int xics_spapr_alloc_block(XICSState *xics, int num, bool lsi, bool align,
> +                           Error **errp)
>  {
> +    ICSState *ics = QLIST_FIRST(&xics->ics);
>      int i, first = -1;
> -    ICSState *ics = &xics->ics[src];
>  
> -    assert(src == 0);
> +    if (!ics) {
> +        return -1;
> +    }
> +
>      /*
>       * MSIMesage::data is used for storing VIRQ so
>       * it has to be aligned to num to support multiple
> @@ -394,7 +425,7 @@ int xics_spapr_alloc_block(XICSState *xics, int src, int num, bool lsi,
>      }
>      first += ics->offset;
>  
> -    trace_xics_alloc_block(src, first, num, lsi, align);
> +    trace_xics_alloc_block(first, num, lsi, align);
>  
>      return first;
>  }
> @@ -405,7 +436,7 @@ static void ics_free(ICSState *ics, int srcno, int num)
>  
>      for (i = srcno; i < srcno + num; ++i) {
>          if (ICS_IRQ_FREE(ics, i)) {
> -            trace_xics_ics_free_warn(ics - ics->xics->ics, i + ics->offset);
> +            trace_xics_ics_free_warn(0, i + ics->offset);
>          }
>          memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
>      }
> @@ -413,15 +444,10 @@ static void ics_free(ICSState *ics, int srcno, int num)
>  
>  void xics_spapr_free(XICSState *xics, int irq, int num)
>  {
> -    int src = xics_find_source(xics, irq);
> -
> -    if (src >= 0) {
> -        ICSState *ics = &xics->ics[src];
> -
> -        /* FIXME: implement multiple sources */
> -        assert(src == 0);
> +    ICSState *ics = xics_find_source(xics, irq);
>  
> -        trace_xics_ics_free(ics - xics->ics, irq, num);
> +    if (ics) {
> +        trace_xics_ics_free(0, irq, num);
>          ics_free(ics, irq - ics->offset, num);
>      }
>  }
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index b0668b3..adf8da4 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -603,7 +603,7 @@ out_no_events:
>  void spapr_events_init(sPAPRMachineState *spapr)
>  {
>      QTAILQ_INIT(&spapr->pending_events);
> -    spapr->check_exception_irq = xics_spapr_alloc(spapr->xics, 0, 0, false,
> +    spapr->check_exception_irq = xics_spapr_alloc(spapr->xics, 0, false,
>                                              &error_fatal);
>      spapr->epow_notifier.notify = spapr_powerdown_req;
>      qemu_register_powerdown_notifier(&spapr->epow_notifier);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 949c44f..4d06794 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -362,7 +362,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      }
>  
>      /* Allocate MSIs */
> -    irq = xics_spapr_alloc_block(spapr->xics, 0, req_num, false,
> +    irq = xics_spapr_alloc_block(spapr->xics, req_num, false,
>                             ret_intr_type == RTAS_TYPE_MSI, &err);
>      if (err) {
>          error_reportf_err(err, "Can't allocate MSIs for device %x: ",
> @@ -1444,8 +1444,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          uint32_t irq;
>          Error *local_err = NULL;
>  
> -        irq = xics_spapr_alloc_block(spapr->xics, 0, 1, true, false,
> -                                     &local_err);
> +        irq = xics_spapr_alloc_block(spapr->xics, 1, true, false, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              error_prepend(errp, "can't allocate LSIs: ");
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index f93244d..22360af 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -463,7 +463,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>          dev->qdev.id = id;
>      }
>  
> -    dev->irq = xics_spapr_alloc(spapr->xics, 0, dev->irq, false, &local_err);
> +    dev->irq = xics_spapr_alloc(spapr->xics, dev->irq, false, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6189a3b..6ad3057 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -84,7 +84,7 @@ struct XICSState {
>      uint32_t nr_servers;
>      uint32_t nr_irqs;
>      ICPState *ss;
> -    ICSState *ics;
> +    QLIST_HEAD(, ICSState) ics;
>  };
>  
>  #define TYPE_ICP "icp"
> @@ -110,6 +110,7 @@ struct ICPState {
>      DeviceState parent_obj;
>      /*< public >*/
>      CPUState *cs;
> +    ICSState *xirr_owner;
>      uint32_t xirr;
>      uint8_t pending_priority;
>      uint8_t mfrr;
> @@ -144,6 +145,7 @@ struct ICSState {
>      qemu_irq *qirqs;
>      ICSIRQState *irqs;
>      XICSState *xics;
> +    QLIST_ENTRY(ICSState) list;
>  };
>  
>  static inline bool ics_valid_irq(ICSState *ics, uint32_t nr)
> @@ -171,10 +173,9 @@ struct ICSIRQState {
>  #define XICS_IRQS_SPAPR               1024
>  
>  qemu_irq xics_get_qirq(XICSState *icp, int irq);
> -int xics_spapr_alloc(XICSState *icp, int src, int irq_hint, bool lsi,
> -                     Error **errp);
> -int xics_spapr_alloc_block(XICSState *icp, int src, int num, bool lsi,
> -                           bool align, Error **errp);
> +int xics_spapr_alloc(XICSState *icp, int irq_hint, bool lsi, Error **errp);
> +int xics_spapr_alloc_block(XICSState *icp, int num, bool lsi, bool align,
> +                           Error **errp);
>  void xics_spapr_free(XICSState *icp, int irq, int num);
>  
>  void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
> @@ -194,6 +195,6 @@ void ics_write_xive(ICSState *ics, int nr, int server,
>  
>  void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
>  
> -int xics_find_source(XICSState *icp, int irq);
> +ICSState *xics_find_source(XICSState *icp, int irq);
>  
>  #endif /* __XICS_H__ */

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 1/4] ppc/xics: Make the ICSState a list

[Nikunj A Dadhania]

David Gibson <david@gibson.dropbear.id.au> writes:

> [ Unknown signature status ]
> On Thu, Jul 07, 2016 at 11:24:15PM +0530, Nikunj A Dadhania wrote:
>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> 
>> Instead of an array of fixed sized blocks, use a list, as we will need
>> to have sources with variable number of interrupts. SPAPR only uses
>> a single entry. Native will create more. If performance becomes an
>> issue we can add some hashed lookup but for now this will do fine.
>> 
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> [ move the initialization of list to xics_common_initfn,
>>   restore xirr_owner after migration and move restoring to
>>   icp_post_load]
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  hw/intc/trace-events  |   5 +-
>>  hw/intc/xics.c        | 134 ++++++++++++++++++++++++++++++++------------------
>>  hw/intc/xics_kvm.c    |  27 +++++++---
>>  hw/intc/xics_spapr.c  |  88 +++++++++++++++++++++------------
>>  hw/ppc/spapr_events.c |   2 +-
>>  hw/ppc/spapr_pci.c    |   5 +-
>>  hw/ppc/spapr_vio.c    |   2 +-
>>  include/hw/ppc/xics.h |  13 ++---
>>  8 files changed, 175 insertions(+), 101 deletions(-)
>> 
>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
>> index 376dd18..41ced33 100644
>> --- a/hw/intc/trace-events
>> +++ b/hw/intc/trace-events
>> @@ -56,10 +56,11 @@ xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
>>  xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
>>  xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
>>  xics_ics_eoi(int nr) "ics_eoi: irq %#x"
>> -xics_alloc(int src, int irq) "source#%d, irq %d"
>> -xics_alloc_block(int src, int first, int num, bool lsi, int align) "source#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
>> +xics_alloc(int irq) "irq %d"
>> +xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d"
>>  xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
>>  xics_ics_free_warn(int src, int irq) "Source#%d, irq %d is already free"
>> +xics_icp_post_load(uint32_t server_no, uint32_t xirr, uint64_t addr, uint8_t pend) "server_no %d, xirr %#x, xirr_owner 0x%" PRIx64 ", pending %d"
>>  
>>  # hw/intc/s390_flic_kvm.c
>>  flic_create_device(int err) "flic: create device failed %d"
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index cd48f42..0af05c9 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -32,6 +32,7 @@
>>  #include "hw/hw.h"
>>  #include "trace.h"
>>  #include "qemu/timer.h"
>> +#include "hw/ppc/spapr.h"
>>  #include "hw/ppc/xics.h"
>>  #include "qemu/error-report.h"
>>  #include "qapi/visitor.h"
>> @@ -96,13 +97,16 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>>  static void xics_common_reset(DeviceState *d)
>>  {
>>      XICSState *xics = XICS_COMMON(d);
>> +    ICSState *ics;
>>      int i;
>>  
>>      for (i = 0; i < xics->nr_servers; i++) {
>>          device_reset(DEVICE(&xics->ss[i]));
>>      }
>>  
>> -    device_reset(DEVICE(xics->ics));
>> +    QLIST_FOREACH(ics, &xics->ics, list) {
>> +        device_reset(DEVICE(ics));
>> +    }
>>  }
>>  
>>  static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char *name,
>> @@ -134,7 +138,6 @@ static void xics_prop_set_nr_irqs(Object *obj, Visitor *v, const char *name,
>>      }
>>  
>>      assert(info->set_nr_irqs);
>> -    assert(xics->ics);
>>      info->set_nr_irqs(xics, value, errp);
>>  }
>>  
>> @@ -174,6 +177,9 @@ static void xics_prop_set_nr_servers(Object *obj, Visitor *v,
>>  
>>  static void xics_common_initfn(Object *obj)
>>  {
>> +    XICSState *xics = XICS_COMMON(obj);
>> +
>> +    QLIST_INIT(&xics->ics);
>>      object_property_add(obj, "nr_irqs", "int",
>>                          xics_prop_get_nr_irqs, xics_prop_set_nr_irqs,
>>                          NULL, NULL, NULL);
>> @@ -212,33 +218,35 @@ static void ics_reject(ICSState *ics, int nr);
>>  static void ics_resend(ICSState *ics);
>>  static void ics_eoi(ICSState *ics, int nr);
>>  
>> -static void icp_check_ipi(XICSState *xics, int server)
>> +static void icp_check_ipi(ICPState *ss)
>>  {
>> -    ICPState *ss = xics->ss + server;
>> -
>>      if (XISR(ss) && (ss->pending_priority <= ss->mfrr)) {
>>          return;
>>      }
>>  
>> -    trace_xics_icp_check_ipi(server, ss->mfrr);
>> +    trace_xics_icp_check_ipi(ss->cs->cpu_index, ss->mfrr);
>
> The dt_id might be more useful here, given the changes that are going
> on in that area.  But it's not worth holding up this series just for
> that.

Yes, I am seeing those changes, will adjust accordingly if that goes
first.

>
>>  
>> -    if (XISR(ss)) {
>> -        ics_reject(xics->ics, XISR(ss));
>> +    if (XISR(ss) && ss->xirr_owner) {
>> +        ics_reject(ss->xirr_owner, XISR(ss));
>>      }
>>  
>>      ss->xirr = (ss->xirr & ~XISR_MASK) | XICS_IPI;
>>      ss->pending_priority = ss->mfrr;
>> +    ss->xirr_owner = NULL;
>>      qemu_irq_raise(ss->output);
>>  }
>>  
>>  static void icp_resend(XICSState *xics, int server)
>>  {
>>      ICPState *ss = xics->ss + server;
>> +    ICSState *ics;
>
> Possibly this should take icp instead of xics for consistency.  Or
> else be renamed xics_resend().  But that can be a cleanup for another
> time.
>
>>      if (ss->mfrr < CPPR(ss)) {
>> -        icp_check_ipi(xics, server);
>> +        icp_check_ipi(ss);
>> +    }
>> +    QLIST_FOREACH(ics, &xics->ics, list) {
>> +        ics_resend(ics);
>>      }
>> -    ics_resend(xics->ics);
>>  }
>>  
>>  void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
>> @@ -256,7 +264,10 @@ void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
>>              ss->xirr &= ~XISR_MASK; /* Clear XISR */
>>              ss->pending_priority = 0xff;
>>              qemu_irq_lower(ss->output);
>> -            ics_reject(xics->ics, old_xisr);
>> +            if (ss->xirr_owner) {
>> +                ics_reject(ss->xirr_owner, old_xisr);
>> +                ss->xirr_owner = NULL;
>> +            }
>>          }
>>      } else {
>>          if (!XISR(ss)) {
>> @@ -271,7 +282,7 @@ void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr)
>>  
>>      ss->mfrr = mfrr;
>>      if (mfrr < CPPR(ss)) {
>> -        icp_check_ipi(xics, server);
>> +        icp_check_ipi(ss);
>>      }
>>  }
>>  
>> @@ -282,6 +293,7 @@ uint32_t icp_accept(ICPState *ss)
>>      qemu_irq_lower(ss->output);
>>      ss->xirr = ss->pending_priority << 24;
>>      ss->pending_priority = 0xff;
>> +    ss->xirr_owner = NULL;
>>  
>>      trace_xics_icp_accept(xirr, ss->xirr);
>>  
>> @@ -299,30 +311,40 @@ uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr)
>>  void icp_eoi(XICSState *xics, int server, uint32_t xirr)
>>  {
>>      ICPState *ss = xics->ss + server;
>> +    ICSState *ics;
>> +    uint32_t irq;
>>  
>>      /* Send EOI -> ICS */
>>      ss->xirr = (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK);
>>      trace_xics_icp_eoi(server, xirr, ss->xirr);
>> -    ics_eoi(xics->ics, xirr & XISR_MASK);
>> +    irq = xirr & XISR_MASK;
>> +    QLIST_FOREACH(ics, &xics->ics, list) {
>> +        if (ics_valid_irq(ics, irq)) {
>> +            ics_eoi(ics, irq);
>
> You could slightly optimize this by adding a break; here, but again
> that can be a tweak for another time.

Sure.

>
>> +        }
>> +    }
>>      if (!XISR(ss)) {
>>          icp_resend(xics, server);
>>      }
>>  }
>>  
>> -static void icp_irq(XICSState *xics, int server, int nr, uint8_t priority)
>> +static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
>>  {
>> +    XICSState *xics = ics->xics;
>>      ICPState *ss = xics->ss + server;
>
> It's kind of weird for an icp_() function to take an ics but not an
> icp.  It might make more sense for it to take both, even though
> obviously it can be derived from just the ics.  Again, won't hold up
> the series for this.
>
>>      trace_xics_icp_irq(server, nr, priority);
>>  
>>      if ((priority >= CPPR(ss))
>>          || (XISR(ss) && (ss->pending_priority <= priority))) {
>> -        ics_reject(xics->ics, nr);
>> +        ics_reject(ics, nr);
>>      } else {
>> -        if (XISR(ss)) {
>> -            ics_reject(xics->ics, XISR(ss));
>> +        if (XISR(ss) && ss->xirr_owner) {
>> +            ics_reject(ss->xirr_owner, XISR(ss));
>> +            ss->xirr_owner = NULL;
>>          }
>>          ss->xirr = (ss->xirr & ~XISR_MASK) | (nr & XISR_MASK);
>> +        ss->xirr_owner = ics;
>>          ss->pending_priority = priority;
>>          trace_xics_icp_raise(ss->xirr, ss->pending_priority);
>>          qemu_irq_raise(ss->output);
>> @@ -378,12 +400,42 @@ static void icp_reset(DeviceState *dev)
>>      qemu_set_irq(icp->output, 0);
>>  }
>>  
>> +static int icp_post_load(ICPState *ss, int version_id)
>> +{
>> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>> +    XICSState *xics = spapr->xics;
>> +    uint32_t irq, i;
>> +    static uint32_t server_no = 0;
>
> Eugh.. static locals are usually horrid.
>
>> +    server_no++;
>> +    irq = XISR(ss);
>> +    if (!ss->cs || !irq)
>> +        goto icpend;
>
> That's an awful use of a goto - inverting the if sense is clearer and
> no longer.

Sure.

>
>> +
>> +    /* Restore the xirr_owner */
>> +    ss->xirr_owner = xics_find_source(xics, irq);
>
> Do we actually need this?  Have you confirmed that replaying the
> icp_resend()s won't already accomplish this?

Yes, i have had migration hang on the target. Other thing its only
affecting "kernel-irqchip=off"

>> + icpend:
>> +    trace_xics_icp_post_load(server_no, ss->xirr, (uint64_t)ss->xirr_owner, ss->pending_priority);
>> +    if (xics->nr_servers != server_no)
>> +        return 0;
>> +
>> +    /* All the ICPs are processed, not restore all the state */
>> +    for (i = 0; i < xics->nr_servers; i++) {
>> +        icp_resend(xics, i);
>> +    }
>> +    server_no = 0;
>
> Eugh.  This is really ugly.  It really looks like we need some
> migration state on the overall xics, not just the component icps and
> icss.  Dealing with backwards compatibility will be hairy, but I
> really hope we can do better than this hack.

Earlier icp_resend() was called from ics_post_load() path, and
icp_post_load gets called after ics_post_load, the xirr_owner is not
restored during ics_post_load.

If we can get icp_post_load called before ics_post_load, we would not
need the above code. Or as you suggested somehere above this.

>
>> +    return 0;
>> +}
>> +
>>  static void icp_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> +    ICPStateClass *icpc = ICP_CLASS(klass);
>>  
>>      dc->reset = icp_reset;
>>      dc->vmsd = &vmstate_icp_server;
>> +    icpc->post_load = icp_post_load;
>>  }
>>  
>>  static const TypeInfo icp_info = {
>> @@ -405,8 +457,7 @@ static void resend_msi(ICSState *ics, int srcno)
>>      if (irq->status & XICS_STATUS_REJECTED) {
>>          irq->status &= ~XICS_STATUS_REJECTED;
>>          if (irq->priority != 0xff) {
>> -            icp_irq(ics->xics, irq->server, srcno + ics->offset,
>> -                    irq->priority);
>> +            icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
>>          }
>>      }
>>  }
>> @@ -419,7 +470,7 @@ static void resend_lsi(ICSState *ics, int srcno)
>>          && (irq->status & XICS_STATUS_ASSERTED)
>>          && !(irq->status & XICS_STATUS_SENT)) {
>>          irq->status |= XICS_STATUS_SENT;
>> -        icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
>> +        icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
>>      }
>>  }
>>  
>> @@ -434,7 +485,7 @@ static void set_irq_msi(ICSState *ics, int srcno, int val)
>>              irq->status |= XICS_STATUS_MASKED_PENDING;
>>              trace_xics_masked_pending();
>>          } else  {
>> -            icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
>> +            icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
>>          }
>>      }
>>  }
>> @@ -473,7 +524,7 @@ static void write_xive_msi(ICSState *ics, int srcno)
>>      }
>>  
>>      irq->status &= ~XICS_STATUS_MASKED_PENDING;
>> -    icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
>> +    icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
>>  }
>>  
>>  static void write_xive_lsi(ICSState *ics, int srcno)
>> @@ -505,8 +556,11 @@ static void ics_reject(ICSState *ics, int nr)
>>      ICSIRQState *irq = ics->irqs + nr - ics->offset;
>>  
>>      trace_xics_ics_reject(nr, nr - ics->offset);
>> -    irq->status |= XICS_STATUS_REJECTED; /* Irrelevant but harmless for LSI */
>> -    irq->status &= ~XICS_STATUS_SENT; /* Irrelevant but harmless for MSI */
>> +    if (irq->flags & XICS_FLAGS_IRQ_MSI) {
>> +      irq->status |= XICS_STATUS_REJECTED;
>> +    } else {
>> +      irq->status &= ~XICS_STATUS_SENT;
>> +    }
>
> This change appears unrelated to the rest.  If not that needs an explanation.

I was seeing inconsistent status because of this in the trace logs, like
for LSI status as 0x7, i.e. XICS_STATUS_ASSERTED, XICS_STATUS_SENT and
XICS_STATUS_REJECTED all set, which did not make sense. So the REJECTED
would have been set in earlier interrupt cycle, and then asserted and
sent in this current one.

Regards
Nikunj

Re: [Qemu-devel] [PATCH v3 1/4] ppc/xics: Make the ICSState a list

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 15509 bytes --]

On Fri, Jul 08, 2016 at 10:20:32AM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > [ Unknown signature status ]
> > On Thu, Jul 07, 2016 at 11:24:15PM +0530, Nikunj A Dadhania wrote:
> >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> 
> >> Instead of an array of fixed sized blocks, use a list, as we will need
> >> to have sources with variable number of interrupts. SPAPR only uses
> >> a single entry. Native will create more. If performance becomes an
> >> issue we can add some hashed lookup but for now this will do fine.
> >> 
> >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> [ move the initialization of list to xics_common_initfn,
> >>   restore xirr_owner after migration and move restoring to
> >>   icp_post_load]
> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> >> ---
> >>  hw/intc/trace-events  |   5 +-
> >>  hw/intc/xics.c        | 134 ++++++++++++++++++++++++++++++++------------------
> >>  hw/intc/xics_kvm.c    |  27 +++++++---
> >>  hw/intc/xics_spapr.c  |  88 +++++++++++++++++++++------------
> >>  hw/ppc/spapr_events.c |   2 +-
> >>  hw/ppc/spapr_pci.c    |   5 +-
> >>  hw/ppc/spapr_vio.c    |   2 +-
> >>  include/hw/ppc/xics.h |  13 ++---
> >>  8 files changed, 175 insertions(+), 101 deletions(-)
> >> 
> >> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> >> index 376dd18..41ced33 100644
> >> --- a/hw/intc/trace-events
> >> +++ b/hw/intc/trace-events
> >> @@ -56,10 +56,11 @@ xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
> >>  xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
> >>  xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
> >>  xics_ics_eoi(int nr) "ics_eoi: irq %#x"
> >> -xics_alloc(int src, int irq) "source#%d, irq %d"
> >> -xics_alloc_block(int src, int first, int num, bool lsi, int align) "source#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
> >> +xics_alloc(int irq) "irq %d"
> >> +xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d"
> >>  xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
> >>  xics_ics_free_warn(int src, int irq) "Source#%d, irq %d is already free"
> >> +xics_icp_post_load(uint32_t server_no, uint32_t xirr, uint64_t addr, uint8_t pend) "server_no %d, xirr %#x, xirr_owner 0x%" PRIx64 ", pending %d"
> >>  
> >>  # hw/intc/s390_flic_kvm.c
> >>  flic_create_device(int err) "flic: create device failed %d"
> >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> >> index cd48f42..0af05c9 100644
> >> --- a/hw/intc/xics.c
> >> +++ b/hw/intc/xics.c
> >> @@ -32,6 +32,7 @@
> >>  #include "hw/hw.h"
> >>  #include "trace.h"
> >>  #include "qemu/timer.h"
> >> +#include "hw/ppc/spapr.h"
> >>  #include "hw/ppc/xics.h"
> >>  #include "qemu/error-report.h"
> >>  #include "qapi/visitor.h"
> >> @@ -96,13 +97,16 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
> >>  static void xics_common_reset(DeviceState *d)
> >>  {
> >>      XICSState *xics = XICS_COMMON(d);
> >> +    ICSState *ics;
> >>      int i;
> >>  
> >>      for (i = 0; i < xics->nr_servers; i++) {
> >>          device_reset(DEVICE(&xics->ss[i]));
> >>      }
> >>  
> >> -    device_reset(DEVICE(xics->ics));
> >> +    QLIST_FOREACH(ics, &xics->ics, list) {
> >> +        device_reset(DEVICE(ics));
> >> +    }
> >>  }
> >>  
> >>  static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char *name,
> >> @@ -134,7 +138,6 @@ static void xics_prop_set_nr_irqs(Object *obj, Visitor *v, const char *name,
> >>      }
> >>  
> >>      assert(info->set_nr_irqs);
> >> -    assert(xics->ics);
> >>      info->set_nr_irqs(xics, value, errp);
> >>  }
> >>  
> >> @@ -174,6 +177,9 @@ static void xics_prop_set_nr_servers(Object *obj, Visitor *v,
> >>  
> >>  static void xics_common_initfn(Object *obj)
> >>  {
> >> +    XICSState *xics = XICS_COMMON(obj);
> >> +
> >> +    QLIST_INIT(&xics->ics);
> >>      object_property_add(obj, "nr_irqs", "int",
> >>                          xics_prop_get_nr_irqs, xics_prop_set_nr_irqs,
> >>                          NULL, NULL, NULL);
> >> @@ -212,33 +218,35 @@ static void ics_reject(ICSState *ics, int nr);
> >>  static void ics_resend(ICSState *ics);
> >>  static void ics_eoi(ICSState *ics, int nr);
> >>  
> >> -static void icp_check_ipi(XICSState *xics, int server)
> >> +static void icp_check_ipi(ICPState *ss)
> >>  {
> >> -    ICPState *ss = xics->ss + server;
> >> -
> >>      if (XISR(ss) && (ss->pending_priority <= ss->mfrr)) {
> >>          return;
> >>      }
> >>  
> >> -    trace_xics_icp_check_ipi(server, ss->mfrr);
> >> +    trace_xics_icp_check_ipi(ss->cs->cpu_index, ss->mfrr);
> >
> > The dt_id might be more useful here, given the changes that are going
> > on in that area.  But it's not worth holding up this series just for
> > that.
> 
> Yes, I am seeing those changes, will adjust accordingly if that goes
> first.
> 
> >
> >>  
> >> -    if (XISR(ss)) {
> >> -        ics_reject(xics->ics, XISR(ss));
> >> +    if (XISR(ss) && ss->xirr_owner) {
> >> +        ics_reject(ss->xirr_owner, XISR(ss));
> >>      }
> >>  
> >>      ss->xirr = (ss->xirr & ~XISR_MASK) | XICS_IPI;
> >>      ss->pending_priority = ss->mfrr;
> >> +    ss->xirr_owner = NULL;
> >>      qemu_irq_raise(ss->output);
> >>  }
> >>  
> >>  static void icp_resend(XICSState *xics, int server)
> >>  {
> >>      ICPState *ss = xics->ss + server;
> >> +    ICSState *ics;
> >
> > Possibly this should take icp instead of xics for consistency.  Or
> > else be renamed xics_resend().  But that can be a cleanup for another
> > time.
> >
> >>      if (ss->mfrr < CPPR(ss)) {
> >> -        icp_check_ipi(xics, server);
> >> +        icp_check_ipi(ss);
> >> +    }
> >> +    QLIST_FOREACH(ics, &xics->ics, list) {
> >> +        ics_resend(ics);
> >>      }
> >> -    ics_resend(xics->ics);
> >>  }
> >>  
> >>  void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
> >> @@ -256,7 +264,10 @@ void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
> >>              ss->xirr &= ~XISR_MASK; /* Clear XISR */
> >>              ss->pending_priority = 0xff;
> >>              qemu_irq_lower(ss->output);
> >> -            ics_reject(xics->ics, old_xisr);
> >> +            if (ss->xirr_owner) {
> >> +                ics_reject(ss->xirr_owner, old_xisr);
> >> +                ss->xirr_owner = NULL;
> >> +            }
> >>          }
> >>      } else {
> >>          if (!XISR(ss)) {
> >> @@ -271,7 +282,7 @@ void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr)
> >>  
> >>      ss->mfrr = mfrr;
> >>      if (mfrr < CPPR(ss)) {
> >> -        icp_check_ipi(xics, server);
> >> +        icp_check_ipi(ss);
> >>      }
> >>  }
> >>  
> >> @@ -282,6 +293,7 @@ uint32_t icp_accept(ICPState *ss)
> >>      qemu_irq_lower(ss->output);
> >>      ss->xirr = ss->pending_priority << 24;
> >>      ss->pending_priority = 0xff;
> >> +    ss->xirr_owner = NULL;
> >>  
> >>      trace_xics_icp_accept(xirr, ss->xirr);
> >>  
> >> @@ -299,30 +311,40 @@ uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr)
> >>  void icp_eoi(XICSState *xics, int server, uint32_t xirr)
> >>  {
> >>      ICPState *ss = xics->ss + server;
> >> +    ICSState *ics;
> >> +    uint32_t irq;
> >>  
> >>      /* Send EOI -> ICS */
> >>      ss->xirr = (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK);
> >>      trace_xics_icp_eoi(server, xirr, ss->xirr);
> >> -    ics_eoi(xics->ics, xirr & XISR_MASK);
> >> +    irq = xirr & XISR_MASK;
> >> +    QLIST_FOREACH(ics, &xics->ics, list) {
> >> +        if (ics_valid_irq(ics, irq)) {
> >> +            ics_eoi(ics, irq);
> >
> > You could slightly optimize this by adding a break; here, but again
> > that can be a tweak for another time.
> 
> Sure.
> 
> >
> >> +        }
> >> +    }
> >>      if (!XISR(ss)) {
> >>          icp_resend(xics, server);
> >>      }
> >>  }
> >>  
> >> -static void icp_irq(XICSState *xics, int server, int nr, uint8_t priority)
> >> +static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
> >>  {
> >> +    XICSState *xics = ics->xics;
> >>      ICPState *ss = xics->ss + server;
> >
> > It's kind of weird for an icp_() function to take an ics but not an
> > icp.  It might make more sense for it to take both, even though
> > obviously it can be derived from just the ics.  Again, won't hold up
> > the series for this.
> >
> >>      trace_xics_icp_irq(server, nr, priority);
> >>  
> >>      if ((priority >= CPPR(ss))
> >>          || (XISR(ss) && (ss->pending_priority <= priority))) {
> >> -        ics_reject(xics->ics, nr);
> >> +        ics_reject(ics, nr);
> >>      } else {
> >> -        if (XISR(ss)) {
> >> -            ics_reject(xics->ics, XISR(ss));
> >> +        if (XISR(ss) && ss->xirr_owner) {
> >> +            ics_reject(ss->xirr_owner, XISR(ss));
> >> +            ss->xirr_owner = NULL;
> >>          }
> >>          ss->xirr = (ss->xirr & ~XISR_MASK) | (nr & XISR_MASK);
> >> +        ss->xirr_owner = ics;
> >>          ss->pending_priority = priority;
> >>          trace_xics_icp_raise(ss->xirr, ss->pending_priority);
> >>          qemu_irq_raise(ss->output);
> >> @@ -378,12 +400,42 @@ static void icp_reset(DeviceState *dev)
> >>      qemu_set_irq(icp->output, 0);
> >>  }
> >>  
> >> +static int icp_post_load(ICPState *ss, int version_id)
> >> +{
> >> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >> +    XICSState *xics = spapr->xics;
> >> +    uint32_t irq, i;
> >> +    static uint32_t server_no = 0;
> >
> > Eugh.. static locals are usually horrid.
> >
> >> +    server_no++;
> >> +    irq = XISR(ss);
> >> +    if (!ss->cs || !irq)
> >> +        goto icpend;
> >
> > That's an awful use of a goto - inverting the if sense is clearer and
> > no longer.
> 
> Sure.
> 
> >
> >> +
> >> +    /* Restore the xirr_owner */
> >> +    ss->xirr_owner = xics_find_source(xics, irq);
> >
> > Do we actually need this?  Have you confirmed that replaying the
> > icp_resend()s won't already accomplish this?
> 
> Yes, i have had migration hang on the target. Other thing its only
> affecting "kernel-irqchip=off"

Hrm.. It would be good to understand exactly what is going on here so
we can be confident we've solved it correctly.


> >> + icpend:
> >> +    trace_xics_icp_post_load(server_no, ss->xirr, (uint64_t)ss->xirr_owner, ss->pending_priority);
> >> +    if (xics->nr_servers != server_no)
> >> +        return 0;
> >> +
> >> +    /* All the ICPs are processed, not restore all the state */
> >> +    for (i = 0; i < xics->nr_servers; i++) {
> >> +        icp_resend(xics, i);
> >> +    }
> >> +    server_no = 0;
> >
> > Eugh.  This is really ugly.  It really looks like we need some
> > migration state on the overall xics, not just the component icps and
> > icss.  Dealing with backwards compatibility will be hairy, but I
> > really hope we can do better than this hack.
> 
> Earlier icp_resend() was called from ics_post_load() path, and
> icp_post_load gets called after ics_post_load, the xirr_owner is not
> restored during ics_post_load.
> 
> If we can get icp_post_load called before ics_post_load, we would not
> need the above code. Or as you suggested somehere above this.

Hm, ok.  I'm wondering if we can move most of the post_load logic to a
top-level xicsstate structure.  I think the migration layer guarantees
that parent objects have their post_load called after all the
subordinate objects have their state restore.

Migration compat could be tricky.  Since I don't think we need any
actual associated with the top-level object it should be possible in
theory, but I'm not sure if there's inbuilt support for an object with
no on-wire presence, but just a post_load handler.

> 
> >
> >> +    return 0;
> >> +}
> >> +
> >>  static void icp_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      DeviceClass *dc = DEVICE_CLASS(klass);
> >> +    ICPStateClass *icpc = ICP_CLASS(klass);
> >>  
> >>      dc->reset = icp_reset;
> >>      dc->vmsd = &vmstate_icp_server;
> >> +    icpc->post_load = icp_post_load;
> >>  }
> >>  
> >>  static const TypeInfo icp_info = {
> >> @@ -405,8 +457,7 @@ static void resend_msi(ICSState *ics, int srcno)
> >>      if (irq->status & XICS_STATUS_REJECTED) {
> >>          irq->status &= ~XICS_STATUS_REJECTED;
> >>          if (irq->priority != 0xff) {
> >> -            icp_irq(ics->xics, irq->server, srcno + ics->offset,
> >> -                    irq->priority);
> >> +            icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
> >>          }
> >>      }
> >>  }
> >> @@ -419,7 +470,7 @@ static void resend_lsi(ICSState *ics, int srcno)
> >>          && (irq->status & XICS_STATUS_ASSERTED)
> >>          && !(irq->status & XICS_STATUS_SENT)) {
> >>          irq->status |= XICS_STATUS_SENT;
> >> -        icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
> >> +        icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
> >>      }
> >>  }
> >>  
> >> @@ -434,7 +485,7 @@ static void set_irq_msi(ICSState *ics, int srcno, int val)
> >>              irq->status |= XICS_STATUS_MASKED_PENDING;
> >>              trace_xics_masked_pending();
> >>          } else  {
> >> -            icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
> >> +            icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
> >>          }
> >>      }
> >>  }
> >> @@ -473,7 +524,7 @@ static void write_xive_msi(ICSState *ics, int srcno)
> >>      }
> >>  
> >>      irq->status &= ~XICS_STATUS_MASKED_PENDING;
> >> -    icp_irq(ics->xics, irq->server, srcno + ics->offset, irq->priority);
> >> +    icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
> >>  }
> >>  
> >>  static void write_xive_lsi(ICSState *ics, int srcno)
> >> @@ -505,8 +556,11 @@ static void ics_reject(ICSState *ics, int nr)
> >>      ICSIRQState *irq = ics->irqs + nr - ics->offset;
> >>  
> >>      trace_xics_ics_reject(nr, nr - ics->offset);
> >> -    irq->status |= XICS_STATUS_REJECTED; /* Irrelevant but harmless for LSI */
> >> -    irq->status &= ~XICS_STATUS_SENT; /* Irrelevant but harmless for MSI */
> >> +    if (irq->flags & XICS_FLAGS_IRQ_MSI) {
> >> +      irq->status |= XICS_STATUS_REJECTED;
> >> +    } else {
> >> +      irq->status &= ~XICS_STATUS_SENT;
> >> +    }
> >
> > This change appears unrelated to the rest.  If not that needs an explanation.
> 
> I was seeing inconsistent status because of this in the trace logs, like
> for LSI status as 0x7, i.e. XICS_STATUS_ASSERTED, XICS_STATUS_SENT and
> XICS_STATUS_REJECTED all set, which did not make sense. So the REJECTED
> would have been set in earlier interrupt cycle, and then asserted and
> sent in this current one.

Ah, ok.  Can you split this out into a separate patch then, with an
explanation that it's to remove confusing debug output?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH v3 2/4] ppc/xics: An ICS with offset 0 is assumed to be uninitialized

[Nikunj A Dadhania]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

This will make life easier for dealing with dynamically configured
ICSes such as PHB3

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 include/hw/ppc/xics.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 6ad3057..8c22daf 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -150,7 +150,7 @@ struct ICSState {
 
 static inline bool ics_valid_irq(ICSState *ics, uint32_t nr)
 {
-    return (nr >= ics->offset)
+    return (ics->offset != 0) && (nr >= ics->offset)
         && (nr < (ics->offset + ics->nr_irqs));
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 2/4] ppc/xics: An ICS with offset 0 is assumed to be uninitialized

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]

On Thu, Jul 07, 2016 at 11:24:16PM +0530, Nikunj A Dadhania wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> This will make life easier for dealing with dynamically configured
> ICSes such as PHB3
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

This doesn't do anything without the ics list change, but it's also
harmless, so I've merged it to ppc-for-2.7.

> ---
>  include/hw/ppc/xics.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6ad3057..8c22daf 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -150,7 +150,7 @@ struct ICSState {
>  
>  static inline bool ics_valid_irq(ICSState *ics, uint32_t nr)
>  {
> -    return (nr >= ics->offset)
> +    return (ics->offset != 0) && (nr >= ics->offset)
>          && (nr < (ics->offset + ics->nr_irqs));
>  }
>  

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH v3 3/4] ppc/xics: Use a helper to add a new ICS

[Nikunj A Dadhania]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
[Move object allocation and adding child to the helper]
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/intc/xics.c        | 10 ++++++++++
 hw/intc/xics_spapr.c  |  6 +-----
 include/hw/ppc/xics.h |  1 +
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 0af05c9..7a658b2 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -109,6 +109,16 @@ static void xics_common_reset(DeviceState *d)
     }
 }
 
+void xics_add_ics(XICSState *xics)
+{
+    ICSState *ics;
+
+    ics = ICS(object_new(TYPE_ICS));
+    object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
+    ics->xics = xics;
+    QLIST_INSERT_HEAD(&xics->ics, ics, list);
+}
+
 static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char *name,
                                   void *opaque, Error **errp)
 {
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 0b0845d..270f20e 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -305,12 +305,8 @@ static void xics_spapr_realize(DeviceState *dev, Error **errp)
 static void xics_spapr_initfn(Object *obj)
 {
     XICSState *xics = XICS_SPAPR(obj);
-    ICSState *ics;
 
-    ics = ICS(object_new(TYPE_ICS));
-    object_property_add_child(obj, "ics", OBJECT(ics), NULL);
-    ics->xics = xics;
-    QLIST_INSERT_HEAD(&xics->ics, ics, list);
+    xics_add_ics(xics);
 }
 
 static void xics_spapr_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 8c22daf..8433bf9 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -196,5 +196,6 @@ void ics_write_xive(ICSState *ics, int nr, int server,
 void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
 
 ICSState *xics_find_source(XICSState *icp, int irq);
+void xics_add_ics(XICSState *xics);
 
 #endif /* __XICS_H__ */
-- 
2.7.4

[Qemu-devel] [PATCH v3 4/4] ppc/xics: Split ICS into ics-base and ics class

[Nikunj A Dadhania]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

The existing implementation remains same and ics-base is introduced. The
type name "ics" is retained, and all the related functions renamed as
ics_simple_*

This will allow different implementations for the source controllers
such as the MSI support of PHB3 on Power8 which uses in-memory state
tables for example.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/intc/trace-events  |  10 ++--
 hw/intc/xics.c        | 143 +++++++++++++++++++++++++++++++-------------------
 hw/intc/xics_kvm.c    |  10 ++--
 hw/intc/xics_spapr.c  |  28 +++++-----
 include/hw/ppc/xics.h |  23 +++++---
 5 files changed, 131 insertions(+), 83 deletions(-)

diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 41ced33..914f25b 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr) "icp_accept: XIRR %#"PRIx3
 xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32
 xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to deliver irq %#"PRIx32" priority %#x"
 xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new XIRR=%#x new pending priority=%#x"
-xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]"
+xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]"
 xics_masked_pending(void) "set_irq_msi: masked pending"
-xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
-xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
-xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
-xics_ics_eoi(int nr) "ics_eoi: irq %#x"
+xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
+xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
+xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]"
+xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x"
 xics_alloc(int irq) "irq %d"
 xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d"
 xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 7a658b2..bee73e2 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics)
 {
     ICSState *ics;
 
-    ics = ICS(object_new(TYPE_ICS));
+    ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
     object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
     ics->xics = xics;
     QLIST_INSERT_HEAD(&xics->ics, ics, list);
@@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
 #define XISR(ss)   (((ss)->xirr) & XISR_MASK)
 #define CPPR(ss)   (((ss)->xirr) >> 24)
 
-static void ics_reject(ICSState *ics, int nr);
-static void ics_resend(ICSState *ics);
-static void ics_eoi(ICSState *ics, int nr);
+static void ics_reject(ICSState *ics, uint32_t nr)
+{
+    ICSStateClass *k = ICS_GET_CLASS(ics);
+
+    if (k->reject) {
+        k->reject(ics, nr);
+    }
+}
+
+static void ics_resend(ICSState *ics)
+{
+    ICSStateClass *k = ICS_GET_CLASS(ics);
+
+    if (k->resend) {
+        k->resend(ics);
+    }
+}
+
+static void ics_eoi(ICSState *ics, int nr)
+{
+    ICSStateClass *k = ICS_GET_CLASS(ics);
+
+    if (k->eoi) {
+        k->eoi(ics, nr);
+    }
+}
 
 static void icp_check_ipi(ICPState *ss)
 {
@@ -459,7 +482,7 @@ static const TypeInfo icp_info = {
 /*
  * ICS: Source layer
  */
-static void resend_msi(ICSState *ics, int srcno)
+static void ics_simple_resend_msi(ICSState *ics, int srcno)
 {
     ICSIRQState *irq = ics->irqs + srcno;
 
@@ -472,7 +495,7 @@ static void resend_msi(ICSState *ics, int srcno)
     }
 }
 
-static void resend_lsi(ICSState *ics, int srcno)
+static void ics_simple_resend_lsi(ICSState *ics, int srcno)
 {
     ICSIRQState *irq = ics->irqs + srcno;
 
@@ -484,11 +507,11 @@ static void resend_lsi(ICSState *ics, int srcno)
     }
 }
 
-static void set_irq_msi(ICSState *ics, int srcno, int val)
+static void ics_simple_set_irq_msi(ICSState *ics, int srcno, int val)
 {
     ICSIRQState *irq = ics->irqs + srcno;
 
-    trace_xics_set_irq_msi(srcno, srcno + ics->offset);
+    trace_xics_ics_simple_set_irq_msi(srcno, srcno + ics->offset);
 
     if (val) {
         if (irq->priority == 0xff) {
@@ -500,31 +523,31 @@ static void set_irq_msi(ICSState *ics, int srcno, int val)
     }
 }
 
-static void set_irq_lsi(ICSState *ics, int srcno, int val)
+static void ics_simple_set_irq_lsi(ICSState *ics, int srcno, int val)
 {
     ICSIRQState *irq = ics->irqs + srcno;
 
-    trace_xics_set_irq_lsi(srcno, srcno + ics->offset);
+    trace_xics_ics_simple_set_irq_lsi(srcno, srcno + ics->offset);
     if (val) {
         irq->status |= XICS_STATUS_ASSERTED;
     } else {
         irq->status &= ~XICS_STATUS_ASSERTED;
     }
-    resend_lsi(ics, srcno);
+    ics_simple_resend_lsi(ics, srcno);
 }
 
-static void ics_set_irq(void *opaque, int srcno, int val)
+static void ics_simple_set_irq(void *opaque, int srcno, int val)
 {
     ICSState *ics = (ICSState *)opaque;
 
     if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
-        set_irq_lsi(ics, srcno, val);
+        ics_simple_set_irq_lsi(ics, srcno, val);
     } else {
-        set_irq_msi(ics, srcno, val);
+        ics_simple_set_irq_msi(ics, srcno, val);
     }
 }
 
-static void write_xive_msi(ICSState *ics, int srcno)
+static void ics_simple_write_xive_msi(ICSState *ics, int srcno)
 {
     ICSIRQState *irq = ics->irqs + srcno;
 
@@ -537,35 +560,35 @@ static void write_xive_msi(ICSState *ics, int srcno)
     icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
 }
 
-static void write_xive_lsi(ICSState *ics, int srcno)
+static void ics_simple_write_xive_lsi(ICSState *ics, int srcno)
 {
-    resend_lsi(ics, srcno);
+    ics_simple_resend_lsi(ics, srcno);
 }
 
-void ics_write_xive(ICSState *ics, int nr, int server,
-                    uint8_t priority, uint8_t saved_priority)
+void ics_simple_write_xive(ICSState *ics, int srcno, int server,
+                           uint8_t priority, uint8_t saved_priority)
 {
-    int srcno = nr - ics->offset;
     ICSIRQState *irq = ics->irqs + srcno;
 
     irq->server = server;
     irq->priority = priority;
     irq->saved_priority = saved_priority;
 
-    trace_xics_ics_write_xive(nr, srcno, server, priority);
+    trace_xics_ics_simple_write_xive(ics->offset + srcno, srcno, server,
+                                     priority);
 
     if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
-        write_xive_lsi(ics, srcno);
+        ics_simple_write_xive_lsi(ics, srcno);
     } else {
-        write_xive_msi(ics, srcno);
+        ics_simple_write_xive_msi(ics, srcno);
     }
 }
 
-static void ics_reject(ICSState *ics, int nr)
+static void ics_simple_reject(ICSState *ics, uint32_t nr)
 {
     ICSIRQState *irq = ics->irqs + nr - ics->offset;
 
-    trace_xics_ics_reject(nr, nr - ics->offset);
+    trace_xics_ics_simple_reject(nr, nr - ics->offset);
     if (irq->flags & XICS_FLAGS_IRQ_MSI) {
       irq->status |= XICS_STATUS_REJECTED;
     } else {
@@ -573,35 +596,35 @@ static void ics_reject(ICSState *ics, int nr)
     }
 }
 
-static void ics_resend(ICSState *ics)
+static void ics_simple_resend(ICSState *ics)
 {
     int i;
 
     for (i = 0; i < ics->nr_irqs; i++) {
         /* FIXME: filter by server#? */
         if (ics->irqs[i].flags & XICS_FLAGS_IRQ_LSI) {
-            resend_lsi(ics, i);
+            ics_simple_resend_lsi(ics, i);
         } else {
-            resend_msi(ics, i);
+            ics_simple_resend_msi(ics, i);
         }
     }
 }
 
-static void ics_eoi(ICSState *ics, int nr)
+static void ics_simple_eoi(ICSState *ics, uint32_t nr)
 {
     int srcno = nr - ics->offset;
     ICSIRQState *irq = ics->irqs + srcno;
 
-    trace_xics_ics_eoi(nr);
+    trace_xics_ics_simple_eoi(nr);
 
     if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
         irq->status &= ~XICS_STATUS_SENT;
     }
 }
 
-static void ics_reset(DeviceState *dev)
+static void ics_simple_reset(DeviceState *dev)
 {
-    ICSState *ics = ICS(dev);
+    ICSState *ics = ICS_SIMPLE(dev);
     int i;
     uint8_t flags[ics->nr_irqs];
 
@@ -618,7 +641,7 @@ static void ics_reset(DeviceState *dev)
     }
 }
 
-static void ics_dispatch_pre_save(void *opaque)
+static void ics_simple_dispatch_pre_save(void *opaque)
 {
     ICSState *ics = opaque;
     ICSStateClass *info = ICS_GET_CLASS(ics);
@@ -628,7 +651,7 @@ static void ics_dispatch_pre_save(void *opaque)
     }
 }
 
-static int ics_dispatch_post_load(void *opaque, int version_id)
+static int ics_simple_dispatch_post_load(void *opaque, int version_id)
 {
     ICSState *ics = opaque;
     ICSStateClass *info = ICS_GET_CLASS(ics);
@@ -640,7 +663,7 @@ static int ics_dispatch_post_load(void *opaque, int version_id)
     return 0;
 }
 
-static const VMStateDescription vmstate_ics_irq = {
+static const VMStateDescription vmstate_ics_simple_irq = {
     .name = "ics/irq",
     .version_id = 2,
     .minimum_version_id = 1,
@@ -654,57 +677,70 @@ static const VMStateDescription vmstate_ics_irq = {
     },
 };
 
-static const VMStateDescription vmstate_ics = {
+static const VMStateDescription vmstate_ics_simple = {
     .name = "ics",
     .version_id = 1,
     .minimum_version_id = 1,
-    .pre_save = ics_dispatch_pre_save,
-    .post_load = ics_dispatch_post_load,
+    .pre_save = ics_simple_dispatch_pre_save,
+    .post_load = ics_simple_dispatch_post_load,
     .fields = (VMStateField[]) {
         /* Sanity check */
         VMSTATE_UINT32_EQUAL(nr_irqs, ICSState),
 
         VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs,
-                                             vmstate_ics_irq, ICSIRQState),
+                                             vmstate_ics_simple_irq,
+                                             ICSIRQState),
         VMSTATE_END_OF_LIST()
     },
 };
 
-static void ics_initfn(Object *obj)
+static void ics_simple_initfn(Object *obj)
 {
-    ICSState *ics = ICS(obj);
+    ICSState *ics = ICS_SIMPLE(obj);
 
     ics->offset = XICS_IRQ_BASE;
 }
 
-static void ics_realize(DeviceState *dev, Error **errp)
+static void ics_simple_realize(DeviceState *dev, Error **errp)
 {
-    ICSState *ics = ICS(dev);
+    ICSState *ics = ICS_SIMPLE(dev);
 
     if (!ics->nr_irqs) {
         error_setg(errp, "Number of interrupts needs to be greater 0");
         return;
     }
     ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
-    ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
+    ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
 }
 
-static void ics_class_init(ObjectClass *klass, void *data)
+static void ics_simple_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ICSStateClass *isc = ICS_CLASS(klass);
 
-    dc->realize = ics_realize;
-    dc->vmsd = &vmstate_ics;
-    dc->reset = ics_reset;
+    dc->realize = ics_simple_realize;
+    dc->vmsd = &vmstate_ics_simple;
+    dc->reset = ics_simple_reset;
+    isc->reject = ics_simple_reject;
+    isc->resend = ics_simple_resend;
+    isc->eoi = ics_simple_eoi;
 }
 
-static const TypeInfo ics_info = {
-    .name = TYPE_ICS,
+static const TypeInfo ics_simple_info = {
+    .name = TYPE_ICS_SIMPLE,
+    .parent = TYPE_ICS_BASE,
+    .instance_size = sizeof(ICSState),
+    .class_init = ics_simple_class_init,
+    .class_size = sizeof(ICSStateClass),
+    .instance_init = ics_simple_initfn,
+};
+
+static const TypeInfo ics_base_info = {
+    .name = TYPE_ICS_BASE,
     .parent = TYPE_DEVICE,
+    .abstract = true,
     .instance_size = sizeof(ICSState),
-    .class_init = ics_class_init,
     .class_size = sizeof(ICSStateClass),
-    .instance_init = ics_initfn,
 };
 
 /*
@@ -744,7 +780,8 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
 static void xics_register_types(void)
 {
     type_register_static(&xics_common_info);
-    type_register_static(&ics_info);
+    type_register_static(&ics_simple_info);
+    type_register_static(&ics_base_info);
     type_register_static(&icp_info);
 }
 
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 04fa7cb..89862df 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -272,7 +272,7 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val)
 
 static void ics_kvm_reset(DeviceState *dev)
 {
-    ICSState *ics = ICS(dev);
+    ICSState *ics = ICS_SIMPLE(dev);
     int i;
     uint8_t flags[ics->nr_irqs];
 
@@ -293,7 +293,7 @@ static void ics_kvm_reset(DeviceState *dev)
 
 static void ics_kvm_realize(DeviceState *dev, Error **errp)
 {
-    ICSState *ics = ICS(dev);
+    ICSState *ics = ICS_SIMPLE(dev);
 
     if (!ics->nr_irqs) {
         error_setg(errp, "Number of interrupts needs to be greater 0");
@@ -315,8 +315,8 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo ics_kvm_info = {
-    .name = TYPE_KVM_ICS,
-    .parent = TYPE_ICS,
+    .name = TYPE_ICS_KVM,
+    .parent = TYPE_ICS_SIMPLE,
     .instance_size = sizeof(ICSState),
     .class_init = ics_kvm_class_init,
 };
@@ -492,7 +492,7 @@ static void xics_kvm_initfn(Object *obj)
     XICSState *xics = XICS_COMMON(obj);
     ICSState *ics;
 
-    ics = ICS(object_new(TYPE_KVM_ICS));
+    ics = ICS_SIMPLE(object_new(TYPE_ICS_KVM));
     object_property_add_child(obj, "ics", OBJECT(ics), NULL);
     ics->xics = xics;
     QLIST_INSERT_HEAD(&xics->ics, ics, list);
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 270f20e..4dd1399 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -114,7 +114,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           uint32_t nret, target_ulong rets)
 {
     ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
-    uint32_t nr, server, priority;
+    uint32_t nr, srcno, server, priority;
 
     if ((nargs != 3) || (nret != 1)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
@@ -135,7 +135,8 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         return;
     }
 
-    ics_write_xive(ics, nr, server, priority, priority);
+    srcno = nr - ics->offset;
+    ics_simple_write_xive(ics, srcno, server, priority, priority);
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
@@ -146,7 +147,7 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           uint32_t nret, target_ulong rets)
 {
     ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
-    uint32_t nr;
+    uint32_t nr, srcno;
 
     if ((nargs != 1) || (nret != 3)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
@@ -165,8 +166,9 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     }
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
-    rtas_st(rets, 1, ics->irqs[nr - ics->offset].server);
-    rtas_st(rets, 2, ics->irqs[nr - ics->offset].priority);
+    srcno = nr - ics->offset;
+    rtas_st(rets, 1, ics->irqs[srcno].server);
+    rtas_st(rets, 2, ics->irqs[srcno].priority);
 }
 
 static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr,
@@ -175,7 +177,7 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                          uint32_t nret, target_ulong rets)
 {
     ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
-    uint32_t nr;
+    uint32_t nr, srcno;
 
     if ((nargs != 1) || (nret != 1)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
@@ -193,8 +195,9 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         return;
     }
 
-    ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server, 0xff,
-                   ics->irqs[nr - ics->offset].priority);
+    srcno = nr - ics->offset;
+    ics_simple_write_xive(ics, srcno, ics->irqs[srcno].server, 0xff,
+                          ics->irqs[srcno].priority);
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
@@ -205,7 +208,7 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                         uint32_t nret, target_ulong rets)
 {
     ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
-    uint32_t nr;
+    uint32_t nr, srcno;
 
     if ((nargs != 1) || (nret != 1)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
@@ -223,9 +226,10 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         return;
     }
 
-    ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server,
-                   ics->irqs[nr - ics->offset].saved_priority,
-                   ics->irqs[nr - ics->offset].saved_priority);
+    srcno = nr - ics->offset;
+    ics_simple_write_xive(ics, srcno, ics->irqs[srcno].server,
+                          ics->irqs[srcno].saved_priority,
+                          ics->irqs[srcno].saved_priority);
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 8433bf9..87d5bd4 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -118,22 +118,29 @@ struct ICPState {
     bool cap_irq_xics_enabled;
 };
 
-#define TYPE_ICS "ics"
-#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
+#define TYPE_ICS_BASE "ics-base"
+#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE)
 
-#define TYPE_KVM_ICS "icskvm"
-#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS)
+/* Retain ics for sPAPR for migration from existing sPAPR guests */
+#define TYPE_ICS_SIMPLE "ics"
+#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
+
+#define TYPE_ICS_KVM "icskvm"
+#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM)
 
 #define ICS_CLASS(klass) \
-     OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
+     OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_SIMPLE)
 #define ICS_GET_CLASS(obj) \
-     OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS)
+     OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_SIMPLE)
 
 struct ICSStateClass {
     DeviceClass parent_class;
 
     void (*pre_save)(ICSState *s);
     int (*post_load)(ICSState *s, int version_id);
+    void (*reject)(ICSState *s, uint32_t irq);
+    void (*resend)(ICSState *s);
+    void (*eoi)(ICSState *s, uint32_t irq);
 };
 
 struct ICSState {
@@ -190,8 +197,8 @@ uint32_t icp_accept(ICPState *ss);
 uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
 void icp_eoi(XICSState *icp, int server, uint32_t xirr);
 
-void ics_write_xive(ICSState *ics, int nr, int server,
-                    uint8_t priority, uint8_t saved_priority);
+void ics_simple_write_xive(ICSState *ics, int nr, int server,
+                           uint8_t priority, uint8_t saved_priority);
 
 void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 4/4] ppc/xics: Split ICS into ics-base and ics class

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 21044 bytes --]

On Thu, Jul 07, 2016 at 11:24:18PM +0530, Nikunj A Dadhania wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> The existing implementation remains same and ics-base is introduced. The
> type name "ics" is retained, and all the related functions renamed as
> ics_simple_*
> 
> This will allow different implementations for the source controllers
> such as the MSI support of PHB3 on Power8 which uses in-memory state
> tables for example.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/intc/trace-events  |  10 ++--
>  hw/intc/xics.c        | 143 +++++++++++++++++++++++++++++++-------------------
>  hw/intc/xics_kvm.c    |  10 ++--
>  hw/intc/xics_spapr.c  |  28 +++++-----
>  include/hw/ppc/xics.h |  23 +++++---
>  5 files changed, 131 insertions(+), 83 deletions(-)
> 
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 41ced33..914f25b 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr) "icp_accept: XIRR %#"PRIx3
>  xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32
>  xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to deliver irq %#"PRIx32" priority %#x"
>  xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new XIRR=%#x new pending priority=%#x"
> -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]"
> +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]"
>  xics_masked_pending(void) "set_irq_msi: masked pending"
> -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
> -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
> -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
> -xics_ics_eoi(int nr) "ics_eoi: irq %#x"
> +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
> +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
> +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]"
> +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x"
>  xics_alloc(int irq) "irq %d"
>  xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d irqs, lsi=%d, alignnum %d"
>  xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 7a658b2..bee73e2 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics)
>  {
>      ICSState *ics;
>  
> -    ics = ICS(object_new(TYPE_ICS));
> +    ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
>      object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
>      ics->xics = xics;
>      QLIST_INSERT_HEAD(&xics->ics, ics, list);
> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
>  #define XISR(ss)   (((ss)->xirr) & XISR_MASK)
>  #define CPPR(ss)   (((ss)->xirr) >> 24)
>  
> -static void ics_reject(ICSState *ics, int nr);
> -static void ics_resend(ICSState *ics);
> -static void ics_eoi(ICSState *ics, int nr);
> +static void ics_reject(ICSState *ics, uint32_t nr)
> +{
> +    ICSStateClass *k = ICS_GET_CLASS(ics);
> +
> +    if (k->reject) {
> +        k->reject(ics, nr);
> +    }
> +}
> +
> +static void ics_resend(ICSState *ics)
> +{
> +    ICSStateClass *k = ICS_GET_CLASS(ics);
> +
> +    if (k->resend) {
> +        k->resend(ics);
> +    }
> +}
> +
> +static void ics_eoi(ICSState *ics, int nr)
> +{
> +    ICSStateClass *k = ICS_GET_CLASS(ics);
> +
> +    if (k->eoi) {
> +        k->eoi(ics, nr);
> +    }
> +}

These dispatchers are now simple enough they could go into the header
as static inlines, but I don't know if it's worth it.


>  static void icp_check_ipi(ICPState *ss)
>  {
> @@ -459,7 +482,7 @@ static const TypeInfo icp_info = {
>  /*
>   * ICS: Source layer
>   */
> -static void resend_msi(ICSState *ics, int srcno)
> +static void ics_simple_resend_msi(ICSState *ics, int srcno)
>  {
>      ICSIRQState *irq = ics->irqs + srcno;
>  
> @@ -472,7 +495,7 @@ static void resend_msi(ICSState *ics, int srcno)
>      }
>  }
>  
> -static void resend_lsi(ICSState *ics, int srcno)
> +static void ics_simple_resend_lsi(ICSState *ics, int srcno)
>  {
>      ICSIRQState *irq = ics->irqs + srcno;
>  
> @@ -484,11 +507,11 @@ static void resend_lsi(ICSState *ics, int srcno)
>      }
>  }
>  
> -static void set_irq_msi(ICSState *ics, int srcno, int val)
> +static void ics_simple_set_irq_msi(ICSState *ics, int srcno, int val)
>  {
>      ICSIRQState *irq = ics->irqs + srcno;
>  
> -    trace_xics_set_irq_msi(srcno, srcno + ics->offset);
> +    trace_xics_ics_simple_set_irq_msi(srcno, srcno + ics->offset);
>  
>      if (val) {
>          if (irq->priority == 0xff) {
> @@ -500,31 +523,31 @@ static void set_irq_msi(ICSState *ics, int srcno, int val)
>      }
>  }
>  
> -static void set_irq_lsi(ICSState *ics, int srcno, int val)
> +static void ics_simple_set_irq_lsi(ICSState *ics, int srcno, int val)
>  {
>      ICSIRQState *irq = ics->irqs + srcno;
>  
> -    trace_xics_set_irq_lsi(srcno, srcno + ics->offset);
> +    trace_xics_ics_simple_set_irq_lsi(srcno, srcno + ics->offset);
>      if (val) {
>          irq->status |= XICS_STATUS_ASSERTED;
>      } else {
>          irq->status &= ~XICS_STATUS_ASSERTED;
>      }
> -    resend_lsi(ics, srcno);
> +    ics_simple_resend_lsi(ics, srcno);
>  }
>  
> -static void ics_set_irq(void *opaque, int srcno, int val)
> +static void ics_simple_set_irq(void *opaque, int srcno, int val)
>  {
>      ICSState *ics = (ICSState *)opaque;
>  
>      if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
> -        set_irq_lsi(ics, srcno, val);
> +        ics_simple_set_irq_lsi(ics, srcno, val);
>      } else {
> -        set_irq_msi(ics, srcno, val);
> +        ics_simple_set_irq_msi(ics, srcno, val);
>      }
>  }
>  
> -static void write_xive_msi(ICSState *ics, int srcno)
> +static void ics_simple_write_xive_msi(ICSState *ics, int srcno)
>  {
>      ICSIRQState *irq = ics->irqs + srcno;
>  
> @@ -537,35 +560,35 @@ static void write_xive_msi(ICSState *ics, int srcno)
>      icp_irq(ics, irq->server, srcno + ics->offset, irq->priority);
>  }
>  
> -static void write_xive_lsi(ICSState *ics, int srcno)
> +static void ics_simple_write_xive_lsi(ICSState *ics, int srcno)
>  {
> -    resend_lsi(ics, srcno);
> +    ics_simple_resend_lsi(ics, srcno);
>  }
>  
> -void ics_write_xive(ICSState *ics, int nr, int server,
> -                    uint8_t priority, uint8_t saved_priority)
> +void ics_simple_write_xive(ICSState *ics, int srcno, int server,
> +                           uint8_t priority, uint8_t saved_priority)
>  {
> -    int srcno = nr - ics->offset;
>      ICSIRQState *irq = ics->irqs + srcno;
>  
>      irq->server = server;
>      irq->priority = priority;
>      irq->saved_priority = saved_priority;
>  
> -    trace_xics_ics_write_xive(nr, srcno, server, priority);
> +    trace_xics_ics_simple_write_xive(ics->offset + srcno, srcno, server,
> +                                     priority);
>  
>      if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
> -        write_xive_lsi(ics, srcno);
> +        ics_simple_write_xive_lsi(ics, srcno);
>      } else {
> -        write_xive_msi(ics, srcno);
> +        ics_simple_write_xive_msi(ics, srcno);
>      }
>  }
>  
> -static void ics_reject(ICSState *ics, int nr)
> +static void ics_simple_reject(ICSState *ics, uint32_t nr)
>  {
>      ICSIRQState *irq = ics->irqs + nr - ics->offset;
>  
> -    trace_xics_ics_reject(nr, nr - ics->offset);
> +    trace_xics_ics_simple_reject(nr, nr - ics->offset);
>      if (irq->flags & XICS_FLAGS_IRQ_MSI) {
>        irq->status |= XICS_STATUS_REJECTED;
>      } else {
> @@ -573,35 +596,35 @@ static void ics_reject(ICSState *ics, int nr)
>      }
>  }
>  
> -static void ics_resend(ICSState *ics)
> +static void ics_simple_resend(ICSState *ics)
>  {
>      int i;
>  
>      for (i = 0; i < ics->nr_irqs; i++) {
>          /* FIXME: filter by server#? */
>          if (ics->irqs[i].flags & XICS_FLAGS_IRQ_LSI) {
> -            resend_lsi(ics, i);
> +            ics_simple_resend_lsi(ics, i);
>          } else {
> -            resend_msi(ics, i);
> +            ics_simple_resend_msi(ics, i);
>          }
>      }
>  }
>  
> -static void ics_eoi(ICSState *ics, int nr)
> +static void ics_simple_eoi(ICSState *ics, uint32_t nr)
>  {
>      int srcno = nr - ics->offset;
>      ICSIRQState *irq = ics->irqs + srcno;
>  
> -    trace_xics_ics_eoi(nr);
> +    trace_xics_ics_simple_eoi(nr);
>  
>      if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
>          irq->status &= ~XICS_STATUS_SENT;
>      }
>  }
>  
> -static void ics_reset(DeviceState *dev)
> +static void ics_simple_reset(DeviceState *dev)
>  {
> -    ICSState *ics = ICS(dev);
> +    ICSState *ics = ICS_SIMPLE(dev);
>      int i;
>      uint8_t flags[ics->nr_irqs];
>  
> @@ -618,7 +641,7 @@ static void ics_reset(DeviceState *dev)
>      }
>  }
>  
> -static void ics_dispatch_pre_save(void *opaque)
> +static void ics_simple_dispatch_pre_save(void *opaque)
>  {
>      ICSState *ics = opaque;
>      ICSStateClass *info = ICS_GET_CLASS(ics);
> @@ -628,7 +651,7 @@ static void ics_dispatch_pre_save(void *opaque)
>      }
>  }
>  
> -static int ics_dispatch_post_load(void *opaque, int version_id)
> +static int ics_simple_dispatch_post_load(void *opaque, int version_id)
>  {
>      ICSState *ics = opaque;
>      ICSStateClass *info = ICS_GET_CLASS(ics);
> @@ -640,7 +663,7 @@ static int ics_dispatch_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> -static const VMStateDescription vmstate_ics_irq = {
> +static const VMStateDescription vmstate_ics_simple_irq = {
>      .name = "ics/irq",
>      .version_id = 2,
>      .minimum_version_id = 1,
> @@ -654,57 +677,70 @@ static const VMStateDescription vmstate_ics_irq = {
>      },
>  };
>  
> -static const VMStateDescription vmstate_ics = {
> +static const VMStateDescription vmstate_ics_simple = {
>      .name = "ics",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .pre_save = ics_dispatch_pre_save,
> -    .post_load = ics_dispatch_post_load,
> +    .pre_save = ics_simple_dispatch_pre_save,
> +    .post_load = ics_simple_dispatch_post_load,
>      .fields = (VMStateField[]) {
>          /* Sanity check */
>          VMSTATE_UINT32_EQUAL(nr_irqs, ICSState),
>  
>          VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs,
> -                                             vmstate_ics_irq, ICSIRQState),
> +                                             vmstate_ics_simple_irq,
> +                                             ICSIRQState),
>          VMSTATE_END_OF_LIST()
>      },
>  };
>  
> -static void ics_initfn(Object *obj)
> +static void ics_simple_initfn(Object *obj)
>  {
> -    ICSState *ics = ICS(obj);
> +    ICSState *ics = ICS_SIMPLE(obj);
>  
>      ics->offset = XICS_IRQ_BASE;
>  }
>  
> -static void ics_realize(DeviceState *dev, Error **errp)
> +static void ics_simple_realize(DeviceState *dev, Error **errp)
>  {
> -    ICSState *ics = ICS(dev);
> +    ICSState *ics = ICS_SIMPLE(dev);
>  
>      if (!ics->nr_irqs) {
>          error_setg(errp, "Number of interrupts needs to be greater 0");
>          return;
>      }
>      ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> -    ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
> +    ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
>  }
>  
> -static void ics_class_init(ObjectClass *klass, void *data)
> +static void ics_simple_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICSStateClass *isc = ICS_CLASS(klass);
>  
> -    dc->realize = ics_realize;
> -    dc->vmsd = &vmstate_ics;
> -    dc->reset = ics_reset;
> +    dc->realize = ics_simple_realize;
> +    dc->vmsd = &vmstate_ics_simple;
> +    dc->reset = ics_simple_reset;
> +    isc->reject = ics_simple_reject;
> +    isc->resend = ics_simple_resend;
> +    isc->eoi = ics_simple_eoi;
>  }
>  
> -static const TypeInfo ics_info = {
> -    .name = TYPE_ICS,
> +static const TypeInfo ics_simple_info = {
> +    .name = TYPE_ICS_SIMPLE,
> +    .parent = TYPE_ICS_BASE,
> +    .instance_size = sizeof(ICSState),
> +    .class_init = ics_simple_class_init,
> +    .class_size = sizeof(ICSStateClass),
> +    .instance_init = ics_simple_initfn,
> +};
> +
> +static const TypeInfo ics_base_info = {
> +    .name = TYPE_ICS_BASE,
>      .parent = TYPE_DEVICE,
> +    .abstract = true,
>      .instance_size = sizeof(ICSState),
> -    .class_init = ics_class_init,
>      .class_size = sizeof(ICSStateClass),
> -    .instance_init = ics_initfn,
>  };
>  
>  /*
> @@ -744,7 +780,8 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
>  static void xics_register_types(void)
>  {
>      type_register_static(&xics_common_info);
> -    type_register_static(&ics_info);
> +    type_register_static(&ics_simple_info);
> +    type_register_static(&ics_base_info);
>      type_register_static(&icp_info);
>  }
>  
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 04fa7cb..89862df 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -272,7 +272,7 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val)
>  
>  static void ics_kvm_reset(DeviceState *dev)
>  {
> -    ICSState *ics = ICS(dev);
> +    ICSState *ics = ICS_SIMPLE(dev);
>      int i;
>      uint8_t flags[ics->nr_irqs];
>  
> @@ -293,7 +293,7 @@ static void ics_kvm_reset(DeviceState *dev)
>  
>  static void ics_kvm_realize(DeviceState *dev, Error **errp)
>  {
> -    ICSState *ics = ICS(dev);
> +    ICSState *ics = ICS_SIMPLE(dev);
>  
>      if (!ics->nr_irqs) {
>          error_setg(errp, "Number of interrupts needs to be greater 0");
> @@ -315,8 +315,8 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
>  }
>  
>  static const TypeInfo ics_kvm_info = {
> -    .name = TYPE_KVM_ICS,
> -    .parent = TYPE_ICS,
> +    .name = TYPE_ICS_KVM,
> +    .parent = TYPE_ICS_SIMPLE,
>      .instance_size = sizeof(ICSState),
>      .class_init = ics_kvm_class_init,
>  };
> @@ -492,7 +492,7 @@ static void xics_kvm_initfn(Object *obj)
>      XICSState *xics = XICS_COMMON(obj);
>      ICSState *ics;
>  
> -    ics = ICS(object_new(TYPE_KVM_ICS));
> +    ics = ICS_SIMPLE(object_new(TYPE_ICS_KVM));
>      object_property_add_child(obj, "ics", OBJECT(ics), NULL);
>      ics->xics = xics;
>      QLIST_INSERT_HEAD(&xics->ics, ics, list);
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 270f20e..4dd1399 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -114,7 +114,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            uint32_t nret, target_ulong rets)
>  {
>      ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
> -    uint32_t nr, server, priority;
> +    uint32_t nr, srcno, server, priority;
>  
>      if ((nargs != 3) || (nret != 1)) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -135,7 +135,8 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          return;
>      }
>  
> -    ics_write_xive(ics, nr, server, priority, priority);
> +    srcno = nr - ics->offset;
> +    ics_simple_write_xive(ics, srcno, server, priority, priority);
>  
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
> @@ -146,7 +147,7 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            uint32_t nret, target_ulong rets)
>  {
>      ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
> -    uint32_t nr;
> +    uint32_t nr, srcno;
>  
>      if ((nargs != 1) || (nret != 3)) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -165,8 +166,9 @@ static void rtas_get_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      }
>  
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> -    rtas_st(rets, 1, ics->irqs[nr - ics->offset].server);
> -    rtas_st(rets, 2, ics->irqs[nr - ics->offset].priority);
> +    srcno = nr - ics->offset;
> +    rtas_st(rets, 1, ics->irqs[srcno].server);
> +    rtas_st(rets, 2, ics->irqs[srcno].priority);
>  }
>  
>  static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> @@ -175,7 +177,7 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                           uint32_t nret, target_ulong rets)
>  {
>      ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
> -    uint32_t nr;
> +    uint32_t nr, srcno;
>  
>      if ((nargs != 1) || (nret != 1)) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -193,8 +195,9 @@ static void rtas_int_off(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          return;
>      }
>  
> -    ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server, 0xff,
> -                   ics->irqs[nr - ics->offset].priority);
> +    srcno = nr - ics->offset;
> +    ics_simple_write_xive(ics, srcno, ics->irqs[srcno].server, 0xff,
> +                          ics->irqs[srcno].priority);
>  
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
> @@ -205,7 +208,7 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                          uint32_t nret, target_ulong rets)
>  {
>      ICSState *ics = QLIST_FIRST(&spapr->xics->ics);
> -    uint32_t nr;
> +    uint32_t nr, srcno;
>  
>      if ((nargs != 1) || (nret != 1)) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -223,9 +226,10 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          return;
>      }
>  
> -    ics_write_xive(ics, nr, ics->irqs[nr - ics->offset].server,
> -                   ics->irqs[nr - ics->offset].saved_priority,
> -                   ics->irqs[nr - ics->offset].saved_priority);
> +    srcno = nr - ics->offset;
> +    ics_simple_write_xive(ics, srcno, ics->irqs[srcno].server,
> +                          ics->irqs[srcno].saved_priority,
> +                          ics->irqs[srcno].saved_priority);
>  
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 8433bf9..87d5bd4 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -118,22 +118,29 @@ struct ICPState {
>      bool cap_irq_xics_enabled;
>  };
>  
> -#define TYPE_ICS "ics"
> -#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
> +#define TYPE_ICS_BASE "ics-base"
> +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE)
>  
> -#define TYPE_KVM_ICS "icskvm"
> -#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS)
> +/* Retain ics for sPAPR for migration from existing sPAPR guests */
> +#define TYPE_ICS_SIMPLE "ics"
> +#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
> +
> +#define TYPE_ICS_KVM "icskvm"
> +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM)
>  
>  #define ICS_CLASS(klass) \
> -     OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
> +     OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_SIMPLE)
>  #define ICS_GET_CLASS(obj) \
> -     OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS)
> +     OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_SIMPLE)
>  
>  struct ICSStateClass {
>      DeviceClass parent_class;
>  
>      void (*pre_save)(ICSState *s);
>      int (*post_load)(ICSState *s, int version_id);
> +    void (*reject)(ICSState *s, uint32_t irq);
> +    void (*resend)(ICSState *s);
> +    void (*eoi)(ICSState *s, uint32_t irq);
>  };
>  
>  struct ICSState {
> @@ -190,8 +197,8 @@ uint32_t icp_accept(ICPState *ss);
>  uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
>  void icp_eoi(XICSState *icp, int server, uint32_t xirr);
>  
> -void ics_write_xive(ICSState *ics, int nr, int server,
> -                    uint8_t priority, uint8_t saved_priority);
> +void ics_simple_write_xive(ICSState *ics, int nr, int server,
> +                           uint8_t priority, uint8_t saved_priority);
>  
>  void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
>  

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]