[PATCH v4 0/6] spapr/xics: fix migration of older machine types

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
34 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v4 0/6] spapr/xics: fix migration of older machine types

Greg Kurz-2
I've provided answers for all comments from the v3 review that I deliberately
don't address in v4.

v4: - some patches from v3 got merged
    - added some more preparatory cleanup in xics (patches 1,2)
    - merge cpu_setup() handler into realize() (patch 4)
    - see individual changelog for patches 3 and 6

v3: - preparatory cleanup in pnv (patch 1)
    - rework ICPState realization and vmstate registration (patches 2,3,4)
    - fix migration using dummy icp/server entries (patch 5)

v2: - some patches from v1 are already merged in ppc-for-2.10
    - added a new fix to a potential memory leak (patch 1)
    - consolidate dt_id computation (patch 3)
    - see individual changelogs for patch 2 and 4

I could successfully do the following on POWER8 host with full cores (SMT8):

1) start a pseries-2.9 machine with QEMU 2.9:
        -smp cores=1,threads=2,maxcpus=8
2) hotplug a core:
        device_add host-spapr-cpu-core,core-id=4
3) migrate to QEMU 2.10 configured with core-id 0,4
4) hotplug another core:
        device_add host-spapr-cpu-core,core-id=2
5) migrate back to QEMU 2.9 configured with core-id 0,4,2
6) hotplug the core in the last available slot:
        device_add host-spapr-cpu-core,core-id=6
7) migrate to QEMU 2.10 configured with core-id 0,4,2,6

I could check that the guest is functional after each migration.

--
Greg

---

Greg Kurz (6):
      xics: introduce macros for ICP/ICS link properties
      xics: pass appropriate types to realize() handlers.
      xics: setup cpu at realize time
      xics: drop ICPStateClass::cpu_setup() handler
      xics: directly register ICPState objects to vmstate
      spapr: fix migration of ICPState objects from/to older QEMU


 hw/intc/xics.c          |   95 ++++++++++++++++++++---------------------------
 hw/intc/xics_kvm.c      |   18 ++++-----
 hw/intc/xics_pnv.c      |    6 +--
 hw/ppc/pnv_core.c       |   17 ++++----
 hw/ppc/pnv_psi.c        |    3 +
 hw/ppc/spapr.c          |   89 +++++++++++++++++++++++++++++++++++++++++++-
 hw/ppc/spapr_cpu_core.c |   22 +++++------
 include/hw/ppc/spapr.h  |    2 +
 include/hw/ppc/xics.h   |   16 ++++----
 9 files changed, 168 insertions(+), 100 deletions(-)


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

Greg Kurz-2
These properties are part of the XICS API. They deserve to appear
explicitely in the XICS header file.

Signed-off-by: Greg Kurz <[hidden email]>
---
 hw/intc/xics.c          |    8 ++++----
 hw/ppc/pnv_core.c       |    3 ++-
 hw/ppc/pnv_psi.c        |    3 ++-
 hw/ppc/spapr.c          |    3 ++-
 hw/ppc/spapr_cpu_core.c |    3 ++-
 include/hw/ppc/xics.h   |    4 ++++
 6 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index ec73f02144c9..aa2c4e744f65 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -346,9 +346,9 @@ static void icp_realize(DeviceState *dev, Error **errp)
     Object *obj;
     Error *err = NULL;
 
-    obj = object_property_get_link(OBJECT(dev), "xics", &err);
+    obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, &err);
     if (!obj) {
-        error_setg(errp, "%s: required link 'xics' not found: %s",
+        error_setg(errp, "%s: required link '" ICP_PROP_XICS "' not found: %s",
                    __func__, error_get_pretty(err));
         return;
     }
@@ -654,9 +654,9 @@ static void ics_base_realize(DeviceState *dev, Error **errp)
     Object *obj;
     Error *err = NULL;
 
-    obj = object_property_get_link(OBJECT(dev), "xics", &err);
+    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
     if (!obj) {
-        error_setg(errp, "%s: required link 'xics' not found: %s",
+        error_setg(errp, "%s: required link '" ICS_PROP_XICS "' not found: %s",
                    __func__, error_get_pretty(err));
         return;
     }
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index e8a9a94d5a24..0b6e72950ca3 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -121,7 +121,8 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
     obj = object_new(TYPE_PNV_ICP);
     object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
     object_unref(obj);
-    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
+    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
+                                   &error_abort);
     object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 2bf5bfe3fdd6..9876c266223d 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -474,7 +474,8 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
     }
 
     /* Create PSI interrupt control source */
-    object_property_add_const_link(OBJECT(ics), "xics", obj,  &error_abort);
+    object_property_add_const_link(OBJECT(ics), ICS_PROP_XICS, obj,
+                                   &error_abort);
     object_property_set_int(OBJECT(ics), PSI_NUM_INTERRUPTS, "nr-irqs", &err);
     if (err) {
         error_propagate(errp, err);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 01dda9ea9fb7..b2951d7618d6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -107,7 +107,8 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
 
     obj = object_new(type_ics);
     object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
-    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
+    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
+                                   &error_abort);
     object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
     if (local_err) {
         goto error;
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 029a14120edd..e81879c7cad7 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -145,7 +145,8 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
     obj = object_new(spapr->icp_type);
     object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
     object_unref(obj);
-    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
+    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
+                                   &error_abort);
     object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
         goto error;
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 40a506eacfb4..31145326ebf9 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -86,6 +86,8 @@ struct ICPState {
     XICSFabric *xics;
 };
 
+#define ICP_PROP_XICS "xics"
+
 struct PnvICPState {
     ICPState parent_obj;
 
@@ -130,6 +132,8 @@ struct ICSState {
     XICSFabric *xics;
 };
 
+#define ICS_PROP_XICS "xics"
+
 static inline bool ics_valid_irq(ICSState *ics, uint32_t nr)
 {
     return (ics->offset != 0) && (nr >= ics->offset)


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v4 2/6] xics: pass appropriate types to realize() handlers.

Greg Kurz-2
In reply to this post by Greg Kurz-2
It makes more sense to pass an IPCState * to handlers of ICPStateClass
instead of a DeviceState *, if only to benefit from compile time type
checking. The same goes with ICSStateClass.

While here, we also change the declaration of ICPStateClass in xics.h
for consistency.

Signed-off-by: Greg Kurz <[hidden email]>
---
 hw/intc/xics.c        |   10 ++++------
 hw/intc/xics_kvm.c    |    6 ++----
 hw/intc/xics_pnv.c    |    6 +++---
 include/hw/ppc/xics.h |    8 ++++----
 4 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index aa2c4e744f65..f74a96e932d7 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -356,7 +356,7 @@ static void icp_realize(DeviceState *dev, Error **errp)
     icp->xics = XICS_FABRIC(obj);
 
     if (icpc->realize) {
-        icpc->realize(dev, errp);
+        icpc->realize(icp, errp);
     }
 
     qemu_register_reset(icp_reset, dev);
@@ -606,10 +606,8 @@ static void ics_simple_initfn(Object *obj)
     ics->offset = XICS_IRQ_BASE;
 }
 
-static void ics_simple_realize(DeviceState *dev, Error **errp)
+static void ics_simple_realize(ICSState *ics, Error **errp)
 {
-    ICSState *ics = ICS_SIMPLE(dev);
-
     if (!ics->nr_irqs) {
         error_setg(errp, "Number of interrupts needs to be greater 0");
         return;
@@ -617,7 +615,7 @@ static void ics_simple_realize(DeviceState *dev, Error **errp)
     ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
     ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
 
-    qemu_register_reset(ics_simple_reset, dev);
+    qemu_register_reset(ics_simple_reset, ics);
 }
 
 static Property ics_simple_properties[] = {
@@ -664,7 +662,7 @@ static void ics_base_realize(DeviceState *dev, Error **errp)
 
 
     if (icsc->realize) {
-        icsc->realize(dev, errp);
+        icsc->realize(ics, errp);
     }
 }
 
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 45bf110b51e6..41c5b9439562 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -328,10 +328,8 @@ static void ics_kvm_reset(void *dev)
     ics_set_kvm_state(ics, 1);
 }
 
-static void ics_kvm_realize(DeviceState *dev, Error **errp)
+static void ics_kvm_realize(ICSState *ics, Error **errp)
 {
-    ICSState *ics = ICS_SIMPLE(dev);
-
     if (!ics->nr_irqs) {
         error_setg(errp, "Number of interrupts needs to be greater 0");
         return;
@@ -339,7 +337,7 @@ static void ics_kvm_realize(DeviceState *dev, Error **errp)
     ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
     ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
 
-    qemu_register_reset(ics_kvm_reset, dev);
+    qemu_register_reset(ics_kvm_reset, ics);
 }
 
 static void ics_kvm_class_init(ObjectClass *klass, void *data)
diff --git a/hw/intc/xics_pnv.c b/hw/intc/xics_pnv.c
index 12ae605f10e8..2a955a894678 100644
--- a/hw/intc/xics_pnv.c
+++ b/hw/intc/xics_pnv.c
@@ -159,11 +159,11 @@ static const MemoryRegionOps pnv_icp_ops = {
     },
 };
 
-static void pnv_icp_realize(DeviceState *dev, Error **errp)
+static void pnv_icp_realize(ICPState *icp, Error **errp)
 {
-    PnvICPState *icp = PNV_ICP(dev);
+    PnvICPState *pnv_icp = PNV_ICP(icp);
 
-    memory_region_init_io(&icp->mmio, OBJECT(dev), &pnv_icp_ops,
+    memory_region_init_io(&pnv_icp->mmio, OBJECT(icp), &pnv_icp_ops,
                           icp, "icp-thread", 0x1000);
 }
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 31145326ebf9..797df82fefc0 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -65,9 +65,9 @@ typedef struct XICSFabric XICSFabric;
 struct ICPStateClass {
     DeviceClass parent_class;
 
-    void (*realize)(DeviceState *dev, Error **errp);
-    void (*pre_save)(ICPState *s);
-    int (*post_load)(ICPState *s, int version_id);
+    void (*realize)(ICPState *icp, Error **errp);
+    void (*pre_save)(ICPState *icp);
+    int (*post_load)(ICPState *icp, int version_id);
     void (*cpu_setup)(ICPState *icp, PowerPCCPU *cpu);
     void (*reset)(ICPState *icp);
 };
@@ -113,7 +113,7 @@ struct PnvICPState {
 struct ICSStateClass {
     DeviceClass parent_class;
 
-    void (*realize)(DeviceState *dev, Error **errp);
+    void (*realize)(ICSState *s, Error **errp);
     void (*pre_save)(ICSState *s);
     int (*post_load)(ICSState *s, int version_id);
     void (*reject)(ICSState *s, uint32_t irq);


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v4 3/6] xics: setup cpu at realize time

Greg Kurz-2
In reply to this post by Greg Kurz-2
Until recently, spapr used to allocate ICPState objects for the lifetime
of the machine. They would only be associated to vCPUs in xics_cpu_setup()
when plugging a CPU core.

Now that ICPState objects have the same lifecycle as vCPUs, it is
possible to associate them during realization.

This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
is passed as a property. Note that vCPU now needs to be realized first
for the IRQs to be allocated. It also needs to resetted before ICPState
realization in order to synchronize with KVM.

Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
needed anymore and can be safely dropped.

Signed-off-by: Greg Kurz <[hidden email]>
---
v4: - renamed property from "cs" to "cpu" and make it a macro (ICP_PROP_CPU)
    - fixed copy/paste mistake in error message in icp_realize()
---
 hw/intc/xics.c          |   76 ++++++++++++++++++++---------------------------
 hw/ppc/pnv_core.c       |   18 +++++------
 hw/ppc/spapr_cpu_core.c |   23 ++++++--------
 include/hw/ppc/xics.h   |    3 +-
 4 files changed, 51 insertions(+), 69 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index f74a96e932d7..fdbfddffeea5 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -38,50 +38,6 @@
 #include "monitor/monitor.h"
 #include "hw/intc/intc.h"
 
-void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
-{
-    CPUState *cs = CPU(cpu);
-    ICPState *icp = ICP(cpu->intc);
-
-    assert(icp);
-    assert(cs == icp->cs);
-
-    icp->output = NULL;
-    icp->cs = NULL;
-}
-
-void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
-{
-    CPUState *cs = CPU(cpu);
-    CPUPPCState *env = &cpu->env;
-    ICPStateClass *icpc;
-
-    assert(icp);
-
-    cpu->intc = OBJECT(icp);
-    icp->cs = cs;
-
-    icpc = ICP_GET_CLASS(icp);
-    if (icpc->cpu_setup) {
-        icpc->cpu_setup(icp, cpu);
-    }
-
-    switch (PPC_INPUT(env)) {
-    case PPC_FLAGS_INPUT_POWER7:
-        icp->output = env->irq_inputs[POWER7_INPUT_INT];
-        break;
-
-    case PPC_FLAGS_INPUT_970:
-        icp->output = env->irq_inputs[PPC970_INPUT_INT];
-        break;
-
-    default:
-        error_report("XICS interrupt controller does not support this CPU "
-                     "bus model");
-        abort();
-    }
-}
-
 void icp_pic_print_info(ICPState *icp, Monitor *mon)
 {
     int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
@@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
 {
     ICPState *icp = ICP(dev);
     ICPStateClass *icpc = ICP_GET_CLASS(dev);
+    PowerPCCPU *cpu;
+    CPUPPCState *env;
     Object *obj;
     Error *err = NULL;
 
@@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
 
     icp->xics = XICS_FABRIC(obj);
 
+    obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, &err);
+    if (!obj) {
+        error_setg(errp, "%s: required link '" ICP_PROP_CPU "' not found: %s",
+                   __func__, error_get_pretty(err));
+        return;
+    }
+
+    cpu = POWERPC_CPU(obj);
+    cpu->intc = OBJECT(icp);
+    icp->cs = CPU(obj);
+
+    if (icpc->cpu_setup) {
+        icpc->cpu_setup(icp, cpu);
+    }
+
+    env = &cpu->env;
+    switch (PPC_INPUT(env)) {
+    case PPC_FLAGS_INPUT_POWER7:
+        icp->output = env->irq_inputs[POWER7_INPUT_INT];
+        break;
+
+    case PPC_FLAGS_INPUT_970:
+        icp->output = env->irq_inputs[PPC970_INPUT_INT];
+        break;
+
+    default:
+        error_setg(errp, "XICS interrupt controller does not support this CPU bus model");
+        return;
+    }
+
     if (icpc->realize) {
         icpc->realize(icp, errp);
     }
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 0b6e72950ca3..c7b00b610c1e 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -118,20 +118,20 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     Object *obj;
 
-    obj = object_new(TYPE_PNV_ICP);
-    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
-    object_unref(obj);
-    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
-                                   &error_abort);
-    object_property_set_bool(obj, true, "realized", &local_err);
+    object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
-    object_property_set_bool(child, true, "realized", &local_err);
+    obj = object_new(TYPE_PNV_ICP);
+    object_property_add_child(child, "icp", obj, NULL);
+    object_unref(obj);
+    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
+                                   &error_abort);
+    object_property_add_const_link(obj, ICP_PROP_CPU, child, &error_abort);
+    object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
-        object_unparent(obj);
         error_propagate(errp, local_err);
         return;
     }
@@ -142,8 +142,6 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
         error_propagate(errp, local_err);
         return;
     }
-
-    xics_cpu_setup(xi, cpu, ICP(obj));
 }
 
 static void pnv_core_realize(DeviceState *dev, Error **errp)
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index e81879c7cad7..9fb896b407db 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
 
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
-
-    xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
     qemu_unregister_reset(spapr_cpu_reset, cpu);
 }
 
@@ -140,29 +137,29 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     CPUState *cs = CPU(child);
     PowerPCCPU *cpu = POWERPC_CPU(cs);
-    Object *obj;
+    Object *obj = NULL;
 
-    obj = object_new(spapr->icp_type);
-    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
-    object_unref(obj);
-    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
-                                   &error_abort);
-    object_property_set_bool(obj, true, "realized", &local_err);
+    object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
         goto error;
     }
 
-    object_property_set_bool(child, true, "realized", &local_err);
+    spapr_cpu_init(spapr, cpu, &local_err);
     if (local_err) {
         goto error;
     }
 
-    spapr_cpu_init(spapr, cpu, &local_err);
+    obj = object_new(spapr->icp_type);
+    object_property_add_child(child, "icp", obj, &error_abort);
+    object_unref(obj);
+    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
+                                   &error_abort);
+    object_property_add_const_link(obj, ICP_PROP_CPU, child, &error_abort);
+    object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
         goto error;
     }
 
-    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
     return;
 
 error:
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 797df82fefc0..37b8fb1e8817 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -87,6 +87,7 @@ struct ICPState {
 };
 
 #define ICP_PROP_XICS "xics"
+#define ICP_PROP_CPU "cpu"
 
 struct PnvICPState {
     ICPState parent_obj;
@@ -187,8 +188,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
 
 qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
 ICPState *xics_icp_get(XICSFabric *xi, int server);
-void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
-void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
 
 /* Internal XICS interfaces */
 void icp_set_cppr(ICPState *icp, uint8_t cppr);


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v4 4/6] xics: drop ICPStateClass::cpu_setup() handler

Greg Kurz-2
In reply to this post by Greg Kurz-2
The cpu_setup() handler is only implemented by xics_kvm, where it really
does a typical "realize" job. Moreover, the realize() handler is called
shortly after cpu_setup(), on the same path.

This patch converts xics_kvm to implement realize() instead of cpu_setup().

Signed-off-by: Greg Kurz <[hidden email]>
---
 hw/intc/xics.c        |    4 ----
 hw/intc/xics_kvm.c    |   12 ++++++------
 include/hw/ppc/xics.h |    1 -
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index fdbfddffeea5..7ccfb53c55a0 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -324,10 +324,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
     cpu->intc = OBJECT(icp);
     icp->cs = CPU(obj);
 
-    if (icpc->cpu_setup) {
-        icpc->cpu_setup(icp, cpu);
-    }
-
     env = &cpu->env;
     switch (PPC_INPUT(env)) {
     case PPC_FLAGS_INPUT_POWER7:
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 41c5b9439562..3091ad3ac2c8 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -115,9 +115,9 @@ static void icp_kvm_reset(ICPState *icp)
     icp_set_kvm_state(icp, 1);
 }
 
-static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu)
+static void icp_kvm_realize(ICPState *icp, Error **errp)
 {
-    CPUState *cs = CPU(cpu);
+    CPUState *cs = icp->cs;
     KVMEnabledICP *enabled_icp;
     unsigned long vcpu_id = kvm_arch_vcpu_id(cs);
     int ret;
@@ -139,9 +139,9 @@ static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu)
 
     ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id);
     if (ret < 0) {
-        error_report("Unable to connect CPU%ld to kernel XICS: %s", vcpu_id,
-                     strerror(errno));
-        exit(1);
+        error_setg(errp, "Unable to connect CPU%ld to kernel XICS: %s", vcpu_id,
+                   strerror(errno));
+        return;
     }
     enabled_icp = g_malloc(sizeof(*enabled_icp));
     enabled_icp->vcpu_id = vcpu_id;
@@ -154,7 +154,7 @@ static void icp_kvm_class_init(ObjectClass *klass, void *data)
 
     icpc->pre_save = icp_get_kvm_state;
     icpc->post_load = icp_set_kvm_state;
-    icpc->cpu_setup = icp_kvm_cpu_setup;
+    icpc->realize = icp_kvm_realize;
     icpc->reset = icp_kvm_reset;
 }
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 37b8fb1e8817..28d248abad61 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -68,7 +68,6 @@ struct ICPStateClass {
     void (*realize)(ICPState *icp, Error **errp);
     void (*pre_save)(ICPState *icp);
     int (*post_load)(ICPState *icp, int version_id);
-    void (*cpu_setup)(ICPState *icp, PowerPCCPU *cpu);
     void (*reset)(ICPState *icp);
 };
 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v4 5/6] xics: directly register ICPState objects to vmstate

Greg Kurz-2
In reply to this post by Greg Kurz-2
The ICPState objects are currently registered to vmstate as qdev objects.
Their instance ids are hence computed automatically in the migration code,
and thus depends on the order the CPU cores were plugged.

If the destination had its CPU cores plugged in a different order than the
source, then ICPState objects will have different instance_ids and load
the wrong state.

Since CPU objects have a reliable cpu_index which is already used as
instance_id in vmstate, let's use it for ICPState as well.

Signed-off-by: Greg Kurz <[hidden email]>
---
 hw/intc/xics.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 7ccfb53c55a0..faa5c631f655 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -344,10 +344,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
     }
 
     qemu_register_reset(icp_reset, dev);
+    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
 }
 
 static void icp_unrealize(DeviceState *dev, Error **errp)
 {
+    ICPState *icp = ICP(dev);
+
+    vmstate_unregister(NULL, &vmstate_icp_server, icp);
     qemu_unregister_reset(icp_reset, dev);
 }
 
@@ -355,7 +359,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->vmsd = &vmstate_icp_server;
     dc->realize = icp_realize;
     dc->unrealize = icp_unrealize;
 }


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v4 6/6] spapr: fix migration of ICPState objects from/to older QEMU

Greg Kurz-2
In reply to this post by Greg Kurz-2
Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
sPAPRCPUCore") moved ICPState objects from the machine to CPU cores.
This is an improvement since we no longer allocate ICPState objects
that will never be used. But it has the side-effect of breaking
migration of older machine types from older QEMU versions.

This patch allows spapr to register dummy "icp/server" entries to vmstate.
These entries use a dedicated VMStateDescription that can swallow and
discard state of an incoming migration stream, and that don't send anything
on outgoing migration.

As for real ICPState objects, the instance_id is the cpu_index of the
corresponding vCPU, which happens to be equal to the generated instance_id
of older machine types.

The machine can unregister/register these entries when CPUs are dynamically
plugged/unplugged.

This is only available for pseries-2.9 and older machines, thanks to a
compat property.

Signed-off-by: Greg Kurz <[hidden email]>
---
v4: - dropped paranoid g_assert()s
---
 hw/ppc/spapr.c         |   86 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/ppc/spapr.h |    2 +
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b2951d7618d6..1379986c0c7b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -125,9 +125,50 @@ error:
     return NULL;
 }
 
+static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
+{
+    return false;
+}
+
+static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
+    .name = "icp/server",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = pre_2_10_vmstate_dummy_icp_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UNUSED(4), /* uint32_t xirr */
+        VMSTATE_UNUSED(1), /* uint8_t pending_priority */
+        VMSTATE_UNUSED(1), /* uint8_t mfrr */
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
+{
+    bool *flag = &spapr->pre_2_10_ignore_icp[i];
+
+    vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, flag);
+    *flag = true;
+}
+
+static void pre_2_10_vmstate_unregister_dummy_icp(sPAPRMachineState *spapr,
+                                                  int i)
+{
+    bool *flag = &spapr->pre_2_10_ignore_icp[i];
+
+    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, flag);
+    *flag = false;
+}
+
+static inline int xics_nr_servers(void)
+{
+    return DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(), smp_threads);
+}
+
 static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
 
     if (kvm_enabled()) {
         if (machine_kernel_irqchip_allowed(machine) &&
@@ -149,6 +190,15 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
             return;
         }
     }
+
+    if (smc->pre_2_10_has_unused_icps) {
+        int i;
+
+        spapr->pre_2_10_ignore_icp = g_malloc(xics_nr_servers());
+        for (i = 0; i < xics_nr_servers(); i++) {
+            pre_2_10_vmstate_register_dummy_icp(spapr, i);
+        }
+    }
 }
 
 static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
@@ -977,7 +1027,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
     void *fdt;
     sPAPRPHBState *phb;
     char *buf;
-    int smt = kvmppc_smt_threads();
 
     fdt = g_malloc0(FDT_MAX_SIZE);
     _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
@@ -1017,7 +1066,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
     _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
 
     /* /interrupt controller */
-    spapr_dt_xics(DIV_ROUND_UP(max_cpus * smt, smp_threads), fdt, PHANDLE_XICP);
+    spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);
 
     ret = spapr_populate_memory(spapr, fdt);
     if (ret < 0) {
@@ -2803,9 +2852,24 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
     MachineState *ms = MACHINE(qdev_get_machine());
+    sPAPRMachineState *spapr = SPAPR_MACHINE(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
 
+    if (spapr->pre_2_10_ignore_icp) {
+        sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
+        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
+        const char *typename = object_class_get_name(scc->cpu_class);
+        size_t size = object_type_get_instance_size(typename);
+        int i;
+
+        for (i = 0; i < cc->nr_threads; i++) {
+            CPUState *cs = CPU(sc->threads + i * size);
+
+            pre_2_10_vmstate_register_dummy_icp(spapr, cs->cpu_index);
+        }
+    }
+
     assert(core_slot);
     core_slot->cpu = NULL;
     object_unparent(OBJECT(dev));
@@ -2913,6 +2977,21 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         }
     }
     core_slot->cpu = OBJECT(dev);
+
+    if (spapr->pre_2_10_ignore_icp) {
+        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
+        const char *typename = object_class_get_name(scc->cpu_class);
+        size_t size = object_type_get_instance_size(typename);
+        int i;
+
+        for (i = 0; i < cc->nr_threads; i++) {
+            sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
+            void *obj = sc->threads + i * size;
+
+            cs = CPU(obj);
+            pre_2_10_vmstate_unregister_dummy_icp(spapr, cs->cpu_index);
+        }
+    }
 }
 
 static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
@@ -3362,9 +3441,12 @@ static void spapr_machine_2_9_instance_options(MachineState *machine)
 
 static void spapr_machine_2_9_class_options(MachineClass *mc)
 {
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
+
     spapr_machine_2_10_class_options(mc);
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
     mc->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
+    smc->pre_2_10_has_unused_icps = true;
 }
 
 DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index f973b0284596..64382623199d 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -53,6 +53,7 @@ struct sPAPRMachineClass {
     bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
     bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
     const char *tcg_default_cpu; /* which (TCG) CPU to simulate by default */
+    bool pre_2_10_has_unused_icps;
     void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
                           uint64_t *buid, hwaddr *pio,
                           hwaddr *mmio32, hwaddr *mmio64,
@@ -90,6 +91,7 @@ struct sPAPRMachineState {
     sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
     bool cas_reboot;
     bool cas_legacy_guest_workaround;
+    bool *pre_2_10_ignore_icp;
 
     Notifier epow_notifier;
     QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

Cédric Le Goater-2
In reply to this post by Greg Kurz-2
On 06/08/2017 03:42 PM, Greg Kurz wrote:
> These properties are part of the XICS API. They deserve to appear
> explicitely in the XICS header file.

I don't see the benefits.

C.


>
> Signed-off-by: Greg Kurz <[hidden email]>
> ---
>  hw/intc/xics.c          |    8 ++++----
>  hw/ppc/pnv_core.c       |    3 ++-
>  hw/ppc/pnv_psi.c        |    3 ++-
>  hw/ppc/spapr.c          |    3 ++-
>  hw/ppc/spapr_cpu_core.c |    3 ++-
>  include/hw/ppc/xics.h   |    4 ++++
>  6 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index ec73f02144c9..aa2c4e744f65 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -346,9 +346,9 @@ static void icp_realize(DeviceState *dev, Error **errp)
>      Object *obj;
>      Error *err = NULL;
>  
> -    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> +    obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, &err);
>      if (!obj) {
> -        error_setg(errp, "%s: required link 'xics' not found: %s",
> +        error_setg(errp, "%s: required link '" ICP_PROP_XICS "' not found: %s",
>                     __func__, error_get_pretty(err));
>          return;
>      }
> @@ -654,9 +654,9 @@ static void ics_base_realize(DeviceState *dev, Error **errp)
>      Object *obj;
>      Error *err = NULL;
>  
> -    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> +    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
>      if (!obj) {
> -        error_setg(errp, "%s: required link 'xics' not found: %s",
> +        error_setg(errp, "%s: required link '" ICS_PROP_XICS "' not found: %s",
>                     __func__, error_get_pretty(err));
>          return;
>      }
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index e8a9a94d5a24..0b6e72950ca3 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -121,7 +121,8 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>      obj = object_new(TYPE_PNV_ICP);
>      object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
>      object_unref(obj);
> -    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> +    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
> +                                   &error_abort);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 2bf5bfe3fdd6..9876c266223d 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -474,7 +474,8 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>      }
>  
>      /* Create PSI interrupt control source */
> -    object_property_add_const_link(OBJECT(ics), "xics", obj,  &error_abort);
> +    object_property_add_const_link(OBJECT(ics), ICS_PROP_XICS, obj,
> +                                   &error_abort);
>      object_property_set_int(OBJECT(ics), PSI_NUM_INTERRUPTS, "nr-irqs", &err);
>      if (err) {
>          error_propagate(errp, err);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 01dda9ea9fb7..b2951d7618d6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -107,7 +107,8 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>  
>      obj = object_new(type_ics);
>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
> -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> +    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
> +                                   &error_abort);
>      object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
>      if (local_err) {
>          goto error;
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 029a14120edd..e81879c7cad7 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -145,7 +145,8 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      obj = object_new(spapr->icp_type);
>      object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
>      object_unref(obj);
> -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> +    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
> +                                   &error_abort);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 40a506eacfb4..31145326ebf9 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -86,6 +86,8 @@ struct ICPState {
>      XICSFabric *xics;
>  };
>  
> +#define ICP_PROP_XICS "xics"
> +
>  struct PnvICPState {
>      ICPState parent_obj;
>  
> @@ -130,6 +132,8 @@ struct ICSState {
>      XICSFabric *xics;
>  };
>  
> +#define ICS_PROP_XICS "xics"
> +
>  static inline bool ics_valid_irq(ICSState *ics, uint32_t nr)
>  {
>      return (ics->offset != 0) && (nr >= ics->offset)
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 3/6] xics: setup cpu at realize time

Cédric Le Goater-2
In reply to this post by Greg Kurz-2
On 06/08/2017 03:42 PM, Greg Kurz wrote:

> Until recently, spapr used to allocate ICPState objects for the lifetime
> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> when plugging a CPU core.
>
> Now that ICPState objects have the same lifecycle as vCPUs, it is
> possible to associate them during realization.
>
> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> is passed as a property. Note that vCPU now needs to be realized first
> for the IRQs to be allocated. It also needs to resetted before ICPState
> realization in order to synchronize with KVM.
>
> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> needed anymore and can be safely dropped.
>
> Signed-off-by: Greg Kurz <[hidden email]>

I don't like the "ICP_PROP_CPU" define. A part from that :

Reviewed-by: Cédric Le Goater <[hidden email]>

C.


> ---
> v4: - renamed property from "cs" to "cpu" and make it a macro (ICP_PROP_CPU)
>     - fixed copy/paste mistake in error message in icp_realize()
> ---
>  hw/intc/xics.c          |   76 ++++++++++++++++++++---------------------------
>  hw/ppc/pnv_core.c       |   18 +++++------
>  hw/ppc/spapr_cpu_core.c |   23 ++++++--------
>  include/hw/ppc/xics.h   |    3 +-
>  4 files changed, 51 insertions(+), 69 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index f74a96e932d7..fdbfddffeea5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -38,50 +38,6 @@
>  #include "monitor/monitor.h"
>  #include "hw/intc/intc.h"
>  
> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
> -{
> -    CPUState *cs = CPU(cpu);
> -    ICPState *icp = ICP(cpu->intc);
> -
> -    assert(icp);
> -    assert(cs == icp->cs);
> -
> -    icp->output = NULL;
> -    icp->cs = NULL;
> -}
> -
> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
> -{
> -    CPUState *cs = CPU(cpu);
> -    CPUPPCState *env = &cpu->env;
> -    ICPStateClass *icpc;
> -
> -    assert(icp);
> -
> -    cpu->intc = OBJECT(icp);
> -    icp->cs = cs;
> -
> -    icpc = ICP_GET_CLASS(icp);
> -    if (icpc->cpu_setup) {
> -        icpc->cpu_setup(icp, cpu);
> -    }
> -
> -    switch (PPC_INPUT(env)) {
> -    case PPC_FLAGS_INPUT_POWER7:
> -        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> -        break;
> -
> -    case PPC_FLAGS_INPUT_970:
> -        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> -        break;
> -
> -    default:
> -        error_report("XICS interrupt controller does not support this CPU "
> -                     "bus model");
> -        abort();
> -    }
> -}
> -
>  void icp_pic_print_info(ICPState *icp, Monitor *mon)
>  {
>      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
>      ICPStateClass *icpc = ICP_GET_CLASS(dev);
> +    PowerPCCPU *cpu;
> +    CPUPPCState *env;
>      Object *obj;
>      Error *err = NULL;
>  
> @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
>  
>      icp->xics = XICS_FABRIC(obj);
>  
> +    obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, &err);
> +    if (!obj) {
> +        error_setg(errp, "%s: required link '" ICP_PROP_CPU "' not found: %s",
> +                   __func__, error_get_pretty(err));
> +        return;
> +    }
> +
> +    cpu = POWERPC_CPU(obj);
> +    cpu->intc = OBJECT(icp);
> +    icp->cs = CPU(obj);
> +
> +    if (icpc->cpu_setup) {
> +        icpc->cpu_setup(icp, cpu);
> +    }
> +
> +    env = &cpu->env;
> +    switch (PPC_INPUT(env)) {
> +    case PPC_FLAGS_INPUT_POWER7:
> +        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> +        break;
> +
> +    case PPC_FLAGS_INPUT_970:
> +        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> +        break;
> +
> +    default:
> +        error_setg(errp, "XICS interrupt controller does not support this CPU bus model");
> +        return;
> +    }
> +
>      if (icpc->realize) {
>          icpc->realize(icp, errp);
>      }
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 0b6e72950ca3..c7b00b610c1e 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -118,20 +118,20 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      Object *obj;
>  
> -    obj = object_new(TYPE_PNV_ICP);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> -    object_unref(obj);
> -    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
> -                                   &error_abort);
> -    object_property_set_bool(obj, true, "realized", &local_err);
> +    object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    obj = object_new(TYPE_PNV_ICP);
> +    object_property_add_child(child, "icp", obj, NULL);
> +    object_unref(obj);
> +    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
> +                                   &error_abort);
> +    object_property_add_const_link(obj, ICP_PROP_CPU, child, &error_abort);
> +    object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
> -        object_unparent(obj);
>          error_propagate(errp, local_err);
>          return;
>      }
> @@ -142,8 +142,6 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    xics_cpu_setup(xi, cpu, ICP(obj));
>  }
>  
>  static void pnv_core_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index e81879c7cad7..9fb896b407db 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
>  {
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> -
> -    xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
>  }
>  
> @@ -140,29 +137,29 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(child);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    Object *obj;
> +    Object *obj = NULL;
>  
> -    obj = object_new(spapr->icp_type);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> -    object_unref(obj);
> -    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
> -                                   &error_abort);
> -    object_property_set_bool(obj, true, "realized", &local_err);
> +    object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    spapr_cpu_init(spapr, cpu, &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    spapr_cpu_init(spapr, cpu, &local_err);
> +    obj = object_new(spapr->icp_type);
> +    object_property_add_child(child, "icp", obj, &error_abort);
> +    object_unref(obj);
> +    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
> +                                   &error_abort);
> +    object_property_add_const_link(obj, ICP_PROP_CPU, child, &error_abort);
> +    object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
>      return;
>  
>  error:
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 797df82fefc0..37b8fb1e8817 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -87,6 +87,7 @@ struct ICPState {
>  };
>  
>  #define ICP_PROP_XICS "xics"
> +#define ICP_PROP_CPU "cpu"
>  
>  struct PnvICPState {
>      ICPState parent_obj;
> @@ -187,8 +188,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
>  
>  qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
>  ICPState *xics_icp_get(XICSFabric *xi, int server);
> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
>  
>  /* Internal XICS interfaces */
>  void icp_set_cppr(ICPState *icp, uint8_t cppr);
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 4/6] xics: drop ICPStateClass::cpu_setup() handler

Cédric Le Goater-2
In reply to this post by Greg Kurz-2
On 06/08/2017 03:43 PM, Greg Kurz wrote:
> The cpu_setup() handler is only implemented by xics_kvm, where it really
> does a typical "realize" job. Moreover, the realize() handler is called
> shortly after cpu_setup(), on the same path.
>
> This patch converts xics_kvm to implement realize() instead of cpu_setup().
>
> Signed-off-by: Greg Kurz <[hidden email]>


Reviewed-by: Cédric Le Goater <[hidden email]>

Thanks,

C.

> ---
>  hw/intc/xics.c        |    4 ----
>  hw/intc/xics_kvm.c    |   12 ++++++------
>  include/hw/ppc/xics.h |    1 -
>  3 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index fdbfddffeea5..7ccfb53c55a0 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -324,10 +324,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
>      cpu->intc = OBJECT(icp);
>      icp->cs = CPU(obj);
>  
> -    if (icpc->cpu_setup) {
> -        icpc->cpu_setup(icp, cpu);
> -    }
> -
>      env = &cpu->env;
>      switch (PPC_INPUT(env)) {
>      case PPC_FLAGS_INPUT_POWER7:
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 41c5b9439562..3091ad3ac2c8 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -115,9 +115,9 @@ static void icp_kvm_reset(ICPState *icp)
>      icp_set_kvm_state(icp, 1);
>  }
>  
> -static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu)
> +static void icp_kvm_realize(ICPState *icp, Error **errp)
>  {
> -    CPUState *cs = CPU(cpu);
> +    CPUState *cs = icp->cs;
>      KVMEnabledICP *enabled_icp;
>      unsigned long vcpu_id = kvm_arch_vcpu_id(cs);
>      int ret;
> @@ -139,9 +139,9 @@ static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu)
>  
>      ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id);
>      if (ret < 0) {
> -        error_report("Unable to connect CPU%ld to kernel XICS: %s", vcpu_id,
> -                     strerror(errno));
> -        exit(1);
> +        error_setg(errp, "Unable to connect CPU%ld to kernel XICS: %s", vcpu_id,
> +                   strerror(errno));
> +        return;
>      }
>      enabled_icp = g_malloc(sizeof(*enabled_icp));
>      enabled_icp->vcpu_id = vcpu_id;
> @@ -154,7 +154,7 @@ static void icp_kvm_class_init(ObjectClass *klass, void *data)
>  
>      icpc->pre_save = icp_get_kvm_state;
>      icpc->post_load = icp_set_kvm_state;
> -    icpc->cpu_setup = icp_kvm_cpu_setup;
> +    icpc->realize = icp_kvm_realize;
>      icpc->reset = icp_kvm_reset;
>  }
>  
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 37b8fb1e8817..28d248abad61 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -68,7 +68,6 @@ struct ICPStateClass {
>      void (*realize)(ICPState *icp, Error **errp);
>      void (*pre_save)(ICPState *icp);
>      int (*post_load)(ICPState *icp, int version_id);
> -    void (*cpu_setup)(ICPState *icp, PowerPCCPU *cpu);
>      void (*reset)(ICPState *icp);
>  };
>  
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

Greg Kurz-2
In reply to this post by Cédric Le Goater-2
On Thu, 8 Jun 2017 16:04:21 +0200
Cédric Le Goater <[hidden email]> wrote:

> On 06/08/2017 03:42 PM, Greg Kurz wrote:
> > These properties are part of the XICS API. They deserve to appear
> > explicitely in the XICS header file.  
>
> I don't see the benefits.
>

The links need to be set with the appropriate name otherwise XICS bails out.
When David asked to rename the "cs" prop to "cpu", I forgot to patch pnv
accordingly in the first place. :)

FWIW, other people do that as well (see hw/i386/pc_q35.c for example).

> C.
>
>
> >
> > Signed-off-by: Greg Kurz <[hidden email]>
> > ---
> >  hw/intc/xics.c          |    8 ++++----
> >  hw/ppc/pnv_core.c       |    3 ++-
> >  hw/ppc/pnv_psi.c        |    3 ++-
> >  hw/ppc/spapr.c          |    3 ++-
> >  hw/ppc/spapr_cpu_core.c |    3 ++-
> >  include/hw/ppc/xics.h   |    4 ++++
> >  6 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index ec73f02144c9..aa2c4e744f65 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -346,9 +346,9 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >      Object *obj;
> >      Error *err = NULL;
> >  
> > -    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> > +    obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, &err);
> >      if (!obj) {
> > -        error_setg(errp, "%s: required link 'xics' not found: %s",
> > +        error_setg(errp, "%s: required link '" ICP_PROP_XICS "' not found: %s",
> >                     __func__, error_get_pretty(err));
> >          return;
> >      }
> > @@ -654,9 +654,9 @@ static void ics_base_realize(DeviceState *dev, Error **errp)
> >      Object *obj;
> >      Error *err = NULL;
> >  
> > -    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> > +    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
> >      if (!obj) {
> > -        error_setg(errp, "%s: required link 'xics' not found: %s",
> > +        error_setg(errp, "%s: required link '" ICS_PROP_XICS "' not found: %s",
> >                     __func__, error_get_pretty(err));
> >          return;
> >      }
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index e8a9a94d5a24..0b6e72950ca3 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -121,7 +121,8 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
> >      obj = object_new(TYPE_PNV_ICP);
> >      object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> >      object_unref(obj);
> > -    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> > +    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
> > +                                   &error_abort);
> >      object_property_set_bool(obj, true, "realized", &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> > index 2bf5bfe3fdd6..9876c266223d 100644
> > --- a/hw/ppc/pnv_psi.c
> > +++ b/hw/ppc/pnv_psi.c
> > @@ -474,7 +474,8 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      /* Create PSI interrupt control source */
> > -    object_property_add_const_link(OBJECT(ics), "xics", obj,  &error_abort);
> > +    object_property_add_const_link(OBJECT(ics), ICS_PROP_XICS, obj,
> > +                                   &error_abort);
> >      object_property_set_int(OBJECT(ics), PSI_NUM_INTERRUPTS, "nr-irqs", &err);
> >      if (err) {
> >          error_propagate(errp, err);
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 01dda9ea9fb7..b2951d7618d6 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -107,7 +107,8 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> >  
> >      obj = object_new(type_ics);
> >      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
> > -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> > +    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
> > +                                   &error_abort);
> >      object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
> >      if (local_err) {
> >          goto error;
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 029a14120edd..e81879c7cad7 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -145,7 +145,8 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> >      obj = object_new(spapr->icp_type);
> >      object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> >      object_unref(obj);
> > -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> > +    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
> > +                                   &error_abort);
> >      object_property_set_bool(obj, true, "realized", &local_err);
> >      if (local_err) {
> >          goto error;
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index 40a506eacfb4..31145326ebf9 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -86,6 +86,8 @@ struct ICPState {
> >      XICSFabric *xics;
> >  };
> >  
> > +#define ICP_PROP_XICS "xics"
> > +
> >  struct PnvICPState {
> >      ICPState parent_obj;
> >  
> > @@ -130,6 +132,8 @@ struct ICSState {
> >      XICSFabric *xics;
> >  };
> >  
> > +#define ICS_PROP_XICS "xics"
> > +
> >  static inline bool ics_valid_irq(ICSState *ics, uint32_t nr)
> >  {
> >      return (ics->offset != 0) && (nr >= ics->offset)
> >  
>


attachment0 (188 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

Cédric Le Goater-2
On 06/08/2017 04:32 PM, Greg Kurz wrote:

> On Thu, 8 Jun 2017 16:04:21 +0200
> Cédric Le Goater <[hidden email]> wrote:
>
>> On 06/08/2017 03:42 PM, Greg Kurz wrote:
>>> These properties are part of the XICS API. They deserve to appear
>>> explicitely in the XICS header file.  
>>
>> I don't see the benefits.
>>
>
> The links need to be set with the appropriate name otherwise XICS bails out.
> When David asked to rename the "cs" prop to "cpu",

yes. that is better.

> I forgot to patch pnv accordingly in the first place. :)
>
> FWIW, other people do that as well (see hw/i386/pc_q35.c for example).

well, I don't see the benefits of changing a string constant by a
define.

Cheers,

C.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

Greg Kurz-2
On Thu, 8 Jun 2017 16:51:35 +0200
Cédric Le Goater <[hidden email]> wrote:

> On 06/08/2017 04:32 PM, Greg Kurz wrote:
> > On Thu, 8 Jun 2017 16:04:21 +0200
> > Cédric Le Goater <[hidden email]> wrote:
> >  
> >> On 06/08/2017 03:42 PM, Greg Kurz wrote:  
> >>> These properties are part of the XICS API. They deserve to appear
> >>> explicitely in the XICS header file.    
> >>
> >> I don't see the benefits.
> >>  
> >
> > The links need to be set with the appropriate name otherwise XICS bails out.
> > When David asked to rename the "cs" prop to "cpu",  
>
> yes. that is better.
>
> > I forgot to patch pnv accordingly in the first place. :)
> >
> > FWIW, other people do that as well (see hw/i386/pc_q35.c for example).  
>
> well, I don't see the benefits of changing a string constant by a
> define.
>
Improved semantics, especially since the "xics" string appears in many
places with different meanings. But I don't want to bikeshed more, I'll
do as David says :)

> Cheers,
>
> C.
>


attachment0 (188 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

Cédric Le Goater-2

>>> FWIW, other people do that as well (see hw/i386/pc_q35.c for example).  
>>
>> well, I don't see the benefits of changing a string constant by a
>> define.
>>
>
> Improved semantics,  especially since the "xics" string appears in
> many places with different meanings.

ah ? If so, we should do a cleanup up. The code seems consistent from
what I can see. xics is a general name for :

        'PowerPC interrupt controller (type 2)'

and it is mostly used as a prefix. There are no "xics" object, only a
XICSFabric which is a QOM interface of the machine with a bunch of
handlers. It worked out pretty well for sPAPR and PowerNV.

It won't be the case for XIVE, the interrupt controller (type 3) for
the P9. It is more complex and there are quite a lot of internal
states which will need to be gathered under an object. We can keep
the ICPState fortunately but the sources are quite different.

C.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

Greg Kurz-2
On Thu, 8 Jun 2017 18:08:44 +0200
Cédric Le Goater <[hidden email]> wrote:

> >>> FWIW, other people do that as well (see hw/i386/pc_q35.c for example).    
> >>
> >> well, I don't see the benefits of changing a string constant by a
> >> define.
> >>  
> >
> > Improved semantics,  especially since the "xics" string appears in
> > many places with different meanings.  
>
> ah ? If so, we should do a cleanup up. The code seems consistent from
> what I can see. xics is a general name for :
>
> 'PowerPC interrupt controller (type 2)'
>
> and it is mostly used as a prefix. There are no "xics" object, only a
I'm only talking about "xics" as a property name actually:

$ git grep '"xics"'
hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
hw/ppc/pnv.c:    object_property_add_const_link(OBJECT(&chip->psi), "xics",
hw/ppc/pnv.c:        object_property_add_const_link(OBJECT(pnv_core), "xics",
hw/ppc/pnv_core.c:    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
hw/ppc/pnv_core.c:    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
hw/ppc/pnv_psi.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
hw/ppc/pnv_psi.c:    object_property_add_const_link(OBJECT(ics), "xics", obj,  &error_abort);
hw/ppc/spapr.c:    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
hw/ppc/spapr_cpu_core.c:    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);

You have to read the code to know which ones are related.

With this patch applied, it is mostly obvious, even for the newbie:

$ git grep -E 'IC._PROP_XICS|"xics"'
hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, &err);
hw/intc/xics.c:        error_setg(errp, "%s: required link '" ICP_PROP_XICS "' not found: %s",
hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
hw/intc/xics.c:        error_setg(errp, "%s: required link '" ICS_PROP_XICS "' not found: %s",

hw/ppc/pnv.c:    object_property_add_const_link(OBJECT(&chip->psi), "xics",
hw/ppc/pnv.c:        object_property_add_const_link(OBJECT(pnv_core), "xics",

these two ^^ are related to...

hw/ppc/pnv_core.c:    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),

hw/ppc/pnv_core.c:    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
hw/ppc/pnv_psi.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);

these two ^^

hw/ppc/pnv_psi.c:    object_property_add_const_link(OBJECT(ics), ICS_PROP_XICS, obj,
hw/ppc/spapr.c:    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
hw/ppc/spapr_cpu_core.c:    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
include/hw/ppc/xics.h:#define ICP_PROP_XICS "xics"
include/hw/ppc/xics.h:#define ICS_PROP_XICS "xics"

> XICSFabric which is a QOM interface of the machine with a bunch of
> handlers. It worked out pretty well for sPAPR and PowerNV.
>
> It won't be the case for XIVE, the interrupt controller (type 3) for
> the P9. It is more complex and there are quite a lot of internal
> states which will need to be gathered under an object. We can keep
> the ICPState fortunately but the sources are quite different.
>
> C.
>


attachment0 (188 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

Cédric Le Goater-2
On 06/08/2017 07:00 PM, Greg Kurz wrote:

> On Thu, 8 Jun 2017 18:08:44 +0200
> Cédric Le Goater <[hidden email]> wrote:
>
>>>>> FWIW, other people do that as well (see hw/i386/pc_q35.c for example).    
>>>>
>>>> well, I don't see the benefits of changing a string constant by a
>>>> define.
>>>>  
>>>
>>> Improved semantics,  especially since the "xics" string appears in
>>> many places with different meanings.  
>>
>> ah ? If so, we should do a cleanup up. The code seems consistent from
>> what I can see. xics is a general name for :
>>
>> 'PowerPC interrupt controller (type 2)'
>>
>> and it is mostly used as a prefix. There are no "xics" object, only a
>
> I'm only talking about "xics" as a property name actually:
>
> $ git grep '"xics"'
> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> hw/ppc/pnv.c:    object_property_add_const_link(OBJECT(&chip->psi), "xics",
> hw/ppc/pnv.c:        object_property_add_const_link(OBJECT(pnv_core), "xics",
> hw/ppc/pnv_core.c:    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> hw/ppc/pnv_core.c:    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
> hw/ppc/pnv_psi.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> hw/ppc/pnv_psi.c:    object_property_add_const_link(OBJECT(ics), "xics", obj,  &error_abort);
> hw/ppc/spapr.c:    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> hw/ppc/spapr_cpu_core.c:    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
>
> You have to read the code to know which ones are related.

The "xics" property link always point to the same object :
the XICSFabric object which is the machine, spapr or pnv.

> With this patch applied, it is mostly obvious, even for the newbie:

ah. the goal is to know where in the code the link was set.
It can be even more complex with aliases.

Cheers,

C.

>
> $ git grep -E 'IC._PROP_XICS|"xics"'
> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, &err);
> hw/intc/xics.c:        error_setg(errp, "%s: required link '" ICP_PROP_XICS "' not found: %s",
> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
> hw/intc/xics.c:        error_setg(errp, "%s: required link '" ICS_PROP_XICS "' not found: %s",
>
> hw/ppc/pnv.c:    object_property_add_const_link(OBJECT(&chip->psi), "xics",
> hw/ppc/pnv.c:        object_property_add_const_link(OBJECT(pnv_core), "xics",
>
> these two ^^ are related to...
>
> hw/ppc/pnv_core.c:    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
>
> hw/ppc/pnv_core.c:    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
> hw/ppc/pnv_psi.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
>
> these two ^^
>
> hw/ppc/pnv_psi.c:    object_property_add_const_link(OBJECT(ics), ICS_PROP_XICS, obj,
> hw/ppc/spapr.c:    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
> hw/ppc/spapr_cpu_core.c:    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(spapr),
> include/hw/ppc/xics.h:#define ICP_PROP_XICS "xics"
> include/hw/ppc/xics.h:#define ICS_PROP_XICS "xics"
>
>> XICSFabric which is a QOM interface of the machine with a bunch of
>> handlers. It worked out pretty well for sPAPR and PowerNV.
>>
>> It won't be the case for XIVE, the interrupt controller (type 3) for
>> the P9. It is more complex and there are quite a lot of internal
>> states which will need to be gathered under an object. We can keep
>> the ICPState fortunately but the sources are quite different.
>>
>> C.
>>
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

David Gibson
On Thu, Jun 08, 2017 at 07:26:35PM +0200, Cédric Le Goater wrote:

> On 06/08/2017 07:00 PM, Greg Kurz wrote:
> > On Thu, 8 Jun 2017 18:08:44 +0200
> > Cédric Le Goater <[hidden email]> wrote:
> >
> >>>>> FWIW, other people do that as well (see hw/i386/pc_q35.c for example).    
> >>>>
> >>>> well, I don't see the benefits of changing a string constant by a
> >>>> define.
> >>>>  
> >>>
> >>> Improved semantics,  especially since the "xics" string appears in
> >>> many places with different meanings.  
> >>
> >> ah ? If so, we should do a cleanup up. The code seems consistent from
> >> what I can see. xics is a general name for :
> >>
> >> 'PowerPC interrupt controller (type 2)'
> >>
> >> and it is mostly used as a prefix. There are no "xics" object, only a
> >
> > I'm only talking about "xics" as a property name actually:
> >
> > $ git grep '"xics"'
> > hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> > hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> > hw/ppc/pnv.c:    object_property_add_const_link(OBJECT(&chip->psi), "xics",
> > hw/ppc/pnv.c:        object_property_add_const_link(OBJECT(pnv_core), "xics",
> > hw/ppc/pnv_core.c:    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> > hw/ppc/pnv_core.c:    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
> > hw/ppc/pnv_psi.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> > hw/ppc/pnv_psi.c:    object_property_add_const_link(OBJECT(ics), "xics", obj,  &error_abort);
> > hw/ppc/spapr.c:    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> > hw/ppc/spapr_cpu_core.c:    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> >
> > You have to read the code to know which ones are related.
>
> The "xics" property link always point to the same object :
> the XICSFabric object which is the machine, spapr or pnv.
>
> > With this patch applied, it is mostly obvious, even for the newbie:
>
> ah. the goal is to know where in the code the link was set.
> It can be even more complex with aliases.
There doesn't seem to be a strong convention about whether to use raw
property names or defines across qemu.  I'm not all that fussed either
way.

I do see one small advantage to use defines: if you make a typo, it
will probably result in a compile time error, whereas with a bare
string it won't show up until a runtime error.

In this case, I intend to take the macro patch, mostly just on the
basis of avoiding further delays to rework the remaining patches.

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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 0/6] spapr/xics: fix migration of older machine types

David Gibson
In reply to this post by Greg Kurz-2
On Thu, Jun 08, 2017 at 03:42:32PM +0200, Greg Kurz wrote:
> I've provided answers for all comments from the v3 review that I deliberately
> don't address in v4.

I've merged patches 1-4.  5 & 6 I'm still reviewing.

>
> v4: - some patches from v3 got merged
>     - added some more preparatory cleanup in xics (patches 1,2)
>     - merge cpu_setup() handler into realize() (patch 4)
>     - see individual changelog for patches 3 and 6
>
> v3: - preparatory cleanup in pnv (patch 1)
>     - rework ICPState realization and vmstate registration (patches 2,3,4)
>     - fix migration using dummy icp/server entries (patch 5)
>
> v2: - some patches from v1 are already merged in ppc-for-2.10
>     - added a new fix to a potential memory leak (patch 1)
>     - consolidate dt_id computation (patch 3)
>     - see individual changelogs for patch 2 and 4
>
> I could successfully do the following on POWER8 host with full cores (SMT8):
>
> 1) start a pseries-2.9 machine with QEMU 2.9:
>         -smp cores=1,threads=2,maxcpus=8
> 2) hotplug a core:
>         device_add host-spapr-cpu-core,core-id=4
> 3) migrate to QEMU 2.10 configured with core-id 0,4
> 4) hotplug another core:
>         device_add host-spapr-cpu-core,core-id=2
> 5) migrate back to QEMU 2.9 configured with core-id 0,4,2
> 6) hotplug the core in the last available slot:
>         device_add host-spapr-cpu-core,core-id=6
> 7) migrate to QEMU 2.10 configured with core-id 0,4,2,6
>
> I could check that the guest is functional after each migration.
>
--
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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

Cédric Le Goater-2
In reply to this post by David Gibson
On 06/09/2017 04:10 AM, David Gibson wrote:

> On Thu, Jun 08, 2017 at 07:26:35PM +0200, Cédric Le Goater wrote:
>> On 06/08/2017 07:00 PM, Greg Kurz wrote:
>>> On Thu, 8 Jun 2017 18:08:44 +0200
>>> Cédric Le Goater <[hidden email]> wrote:
>>>
>>>>>>> FWIW, other people do that as well (see hw/i386/pc_q35.c for example).    
>>>>>>
>>>>>> well, I don't see the benefits of changing a string constant by a
>>>>>> define.
>>>>>>  
>>>>>
>>>>> Improved semantics,  especially since the "xics" string appears in
>>>>> many places with different meanings.  
>>>>
>>>> ah ? If so, we should do a cleanup up. The code seems consistent from
>>>> what I can see. xics is a general name for :
>>>>
>>>> 'PowerPC interrupt controller (type 2)'
>>>>
>>>> and it is mostly used as a prefix. There are no "xics" object, only a
>>>
>>> I'm only talking about "xics" as a property name actually:
>>>
>>> $ git grep '"xics"'
>>> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
>>> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
>>> hw/ppc/pnv.c:    object_property_add_const_link(OBJECT(&chip->psi), "xics",
>>> hw/ppc/pnv.c:        object_property_add_const_link(OBJECT(pnv_core), "xics",
>>> hw/ppc/pnv_core.c:    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
>>> hw/ppc/pnv_core.c:    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
>>> hw/ppc/pnv_psi.c:    obj = object_property_get_link(OBJECT(dev), "xics", &err);
>>> hw/ppc/pnv_psi.c:    object_property_add_const_link(OBJECT(ics), "xics", obj,  &error_abort);
>>> hw/ppc/spapr.c:    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
>>> hw/ppc/spapr_cpu_core.c:    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
>>>
>>> You have to read the code to know which ones are related.
>>
>> The "xics" property link always point to the same object :
>> the XICSFabric object which is the machine, spapr or pnv.
>>
>>> With this patch applied, it is mostly obvious, even for the newbie:
>>
>> ah. the goal is to know where in the code the link was set.
>> It can be even more complex with aliases.
>
> There doesn't seem to be a strong convention about whether to use raw
> property names or defines across qemu.  I'm not all that fussed either
> way.
>
> I do see one small advantage to use defines: if you make a typo, it
> will probably result in a compile time error, whereas with a bare
> string it won't show up until a runtime error.

ok. I can agree with that.

> In this case, I intend to take the macro patch, mostly just on the
> basis of avoiding further delays to rework the remaining patches.

But I don't think having two different defines is a good idea :

+#define ICP_PROP_XICS "xics"
+#define ICS_PROP_XICS "xics"

C.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v4 0/6] spapr/xics: fix migration of older machine types

Greg Kurz-2
In reply to this post by David Gibson
On Fri, 9 Jun 2017 12:28:13 +1000
David Gibson <[hidden email]> wrote:

> On Thu, Jun 08, 2017 at 03:42:32PM +0200, Greg Kurz wrote:
> > I've provided answers for all comments from the v3 review that I deliberately
> > don't address in v4.  
>
> I've merged patches 1-4.  5 & 6 I'm still reviewing.
>

Cool. FYI, I forgot to mention that I only tested with KVM.

I'm now trying with TCG and I hit various guest crash on
the destination (using your ppc-for-2.10 branch WITHOUT
my patches):

cpu 0x0: Vector: 700 (Program Check) at [c0000000787ebae0]
    pc: c0000000002803c4: __fput+0x284/0x310
    lr: c000000000280258: __fput+0x118/0x310
    sp: c0000000787ebd60
   msr: 8000000000029033
  current = 0xc00000007cbab640
  paca    = 0xc000000007b80000   softe: 0        irq_happened: 0x01
    pid   = 1812, comm = gawk
kernel BUG at ../include/linux/fs.h:2399!
enter ? for help
[c0000000787ebdb0] c0000000000d7d84 task_work_run+0xe4/0x160
[c0000000787ebe00] c000000000018054 do_notify_resume+0xb4/0xc0
[c0000000787ebe30] c00000000000a730 ret_from_except_lite+0x5c/0x60
--- Exception: c00 (System Call) at 00003fff9026dd90
SP (3fffcb37b790) is in userspace
0:mon>

or

cpu 0x0: Vector: 300 (Data Access) at [c00000007fff7490]
    pc: c0000000001ef768: free_pcppages_bulk+0x2b8/0x500
    lr: c0000000001ef524: free_pcppages_bulk+0x74/0x500
    sp: c00000007fff7710
   msr: 8000000000009033
   dar: c0000000807afc70
 dsisr: 40000000
  current = 0xc00000007c609190
  paca    = 0xc000000007b80000   softe: 0        irq_happened: 0x01
    pid   = 1631, comm = systemctl
enter ? for help
[c00000007fff77c0] c0000000001eff24 free_hot_cold_page+0x204/0x270
[c00000007fff7810] c0000000001f5848 __put_single_page+0x48/0x60
[c00000007fff7840] c00000000059ac50 skb_release_data+0xb0/0x180
[c00000007fff7880] c00000000059ae38 kfree_skb+0x58/0x130
[c00000007fff78c0] c00000000063f604 __udp4_lib_mcast_deliver+0x3d4/0x460
[c00000007fff7a50] c00000000063fb0c __udp4_lib_rcv+0x47c/0x770
[c00000007fff7b00] c0000000006023a8 ip_local_deliver_finish+0x148/0x310
[c00000007fff7b50] c0000000006026c4 ip_rcv_finish+0x154/0x420
[c00000007fff7bd0] c0000000005b1154 __netif_receive_skb_core+0x874/0xac0
[c00000007fff7cc0] c0000000005b30d4 netif_receive_skb+0x34/0xd0
[c00000007fff7d00] d000000000ef3c74 virtnet_poll+0x514/0x8a0 [virtio_net]
[c00000007fff7e10] c0000000005b3668 net_rx_action+0x1d8/0x310
[c00000007fff7ea0] c0000000000b0cc4 __do_softirq+0x154/0x330
[c00000007fff7f90] c0000000000251ac call_do_softirq+0x14/0x24
[c00000007fff3ef0] c000000000011be0 do_softirq+0xe0/0x110
[c00000007fff3f30] c0000000000b10e8 irq_exit+0xc8/0x110
[c00000007fff3f60] c0000000000117e8 __do_irq+0xb8/0x1c0
[c00000007fff3f90] c0000000000251d0 call_do_irq+0x14/0x24
[c00000007a94bac0] c000000000011990 do_IRQ+0xa0/0x120
[c00000007a94bb20] c00000000000a8b0 restore_check_irq_replay+0x2c/0x5c
--- Exception: 501 (Hardware Interrupt) at c000000000010f84 arch_local_irq_restore+0x74/0x90
[c00000007a94be10] 000000000000000c (unreliable)
[c00000007a94be30] c00000000000a704 ret_from_except_lite+0x30/0x60
--- Exception: 501 (Hardware Interrupt) at 00003fffa04a2c28
SP (3ffff7f1bf60) is in userspace
0:mon>

These doesn't seem to occur with QEMU master. I'll try to investigate.

> >
> > v4: - some patches from v3 got merged
> >     - added some more preparatory cleanup in xics (patches 1,2)
> >     - merge cpu_setup() handler into realize() (patch 4)
> >     - see individual changelog for patches 3 and 6
> >
> > v3: - preparatory cleanup in pnv (patch 1)
> >     - rework ICPState realization and vmstate registration (patches 2,3,4)
> >     - fix migration using dummy icp/server entries (patch 5)
> >
> > v2: - some patches from v1 are already merged in ppc-for-2.10
> >     - added a new fix to a potential memory leak (patch 1)
> >     - consolidate dt_id computation (patch 3)
> >     - see individual changelogs for patch 2 and 4
> >
> > I could successfully do the following on POWER8 host with full cores (SMT8):
> >
> > 1) start a pseries-2.9 machine with QEMU 2.9:
> >         -smp cores=1,threads=2,maxcpus=8
> > 2) hotplug a core:
> >         device_add host-spapr-cpu-core,core-id=4
> > 3) migrate to QEMU 2.10 configured with core-id 0,4
> > 4) hotplug another core:
> >         device_add host-spapr-cpu-core,core-id=2
> > 5) migrate back to QEMU 2.9 configured with core-id 0,4,2
> > 6) hotplug the core in the last available slot:
> >         device_add host-spapr-cpu-core,core-id=6
> > 7) migrate to QEMU 2.10 configured with core-id 0,4,2,6
> >
> > I could check that the guest is functional after each migration.
> >  
>


attachment0 (188 bytes) Download Attachment
12
Loading...