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

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

[Greg Kurz]

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

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

[Greg Kurz]

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

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 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)

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

[Greg Kurz]

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 <groug@kaod.org>
---
 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);

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

[Greg Kurz]

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 <groug@kaod.org>
---
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);

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

[Greg Kurz]

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 <groug@kaod.org>
---
 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);
 };
 

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

[Greg Kurz]

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 <groug@kaod.org>
---
 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;
 }

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

[Greg Kurz]

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 <groug@kaod.org>
---
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;

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

[Cédric Le Goater]

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 <groug@kaod.org>
> ---
>  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)
> 

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

[Cédric Le Goater]

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 <groug@kaod.org>

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

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

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

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

[Cédric Le Goater]

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 <groug@kaod.org>


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

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);
>  };
>  
> 

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

[Greg Kurz]

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

On Thu, 8 Jun 2017 16:04:21 +0200
Cédric Le Goater <clg@kaod.org> 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 <groug@kaod.org>
> > ---
> >  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)
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Cédric Le Goater]

On 06/08/2017 04:32 PM, Greg Kurz wrote:
> On Thu, 8 Jun 2017 16:04:21 +0200
> Cédric Le Goater <clg@kaod.org> 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.

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

[Greg Kurz]

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

On Thu, 8 Jun 2017 16:51:35 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 06/08/2017 04:32 PM, Greg Kurz wrote:
> > On Thu, 8 Jun 2017 16:04:21 +0200
> > Cédric Le Goater <clg@kaod.org> 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.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Cédric Le Goater]

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

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

[Greg Kurz]

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

On Thu, 8 Jun 2017 18:08:44 +0200
Cédric Le Goater <clg@kaod.org> 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.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Cédric Le Goater]

On 06/08/2017 07:00 PM, Greg Kurz wrote:
> On Thu, 8 Jun 2017 18:08:44 +0200
> Cédric Le Goater <clg@kaod.org> 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.
>>
> 

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

[David Gibson]

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

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 <clg@kaod.org> 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

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

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

[David Gibson]

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

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

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

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

[Cédric Le Goater]

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 <clg@kaod.org> 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.

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

[Greg Kurz]

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

On Fri, 9 Jun 2017 12:28:13 +1000
David Gibson <david@gibson.dropbear.id.au> 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.
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[David Gibson]

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

On Fri, Jun 09, 2017 at 11:36:31AM +0200, Greg Kurz wrote:
> On Fri, 9 Jun 2017 12:28:13 +1000
> David Gibson <david@gibson.dropbear.id.au> 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):

Drat.  What's your reproducer for this crash?

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

Thanks.  I'm going to be in China for the next couple of weeks.  I'll
still be working, but my time will be divided.

-- 
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 v4 0/6] spapr/xics: fix migration of older machine types

[Greg Kurz]

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

On Fri, 9 Jun 2017 20:28:32 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 09, 2017 at 11:36:31AM +0200, Greg Kurz wrote:
> > On Fri, 9 Jun 2017 12:28:13 +1000
> > David Gibson <david@gibson.dropbear.id.au> 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):  
> 
> Drat.  What's your reproducer for this crash?
> 

1) start guest

qemu-system-ppc64 \
 -nodefaults -nographic -snapshot -no-shutdown -serial mon:stdio \
 -device virtio-net,netdev=netdev0,id=net0 \
 -netdev bridge,id=netdev0,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
 -device virtio-blk,drive=drive0,id=blk0 \
 -drive file=/home/greg/images/sle12-sp1-ppc64le.qcow2,id=drive0,if=none \
 -machine type=pseries,accel=tcg -cpu POWER8

2) migrate

3) destination crashes (immediately or after very short delay) or hangs

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

Bisect leads to:

f0b0685d6694a28c66018f438e822596243b1250 is the first bad commit
commit f0b0685d6694a28c66018f438e822596243b1250
Author: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Date:   Thu Apr 27 10:48:23 2017 +0530

    tcg: enable MTTCG by default for PPC64 on x86

I guess we're still not completely ready to support MTTCG...

Cc'ing Nikunj for insights.

> Thanks.  I'm going to be in China for the next couple of weeks.  I'll
> still be working, but my time will be divided.
> 

Hey, have a good trip! :)

Cheers,

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[David Gibson]

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

On Fri, Jun 09, 2017 at 05:09:13PM +0200, Greg Kurz wrote:
> On Fri, 9 Jun 2017 20:28:32 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jun 09, 2017 at 11:36:31AM +0200, Greg Kurz wrote:
> > > On Fri, 9 Jun 2017 12:28:13 +1000
> > > David Gibson <david@gibson.dropbear.id.au> 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):  
> > 
> > Drat.  What's your reproducer for this crash?
> > 
> 
> 1) start guest
> 
> qemu-system-ppc64 \
>  -nodefaults -nographic -snapshot -no-shutdown -serial mon:stdio \
>  -device virtio-net,netdev=netdev0,id=net0 \
>  -netdev bridge,id=netdev0,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
>  -device virtio-blk,drive=drive0,id=blk0 \
>  -drive file=/home/greg/images/sle12-sp1-ppc64le.qcow2,id=drive0,if=none \
>  -machine type=pseries,accel=tcg -cpu POWER8
> 
> 2) migrate
> 
> 3) destination crashes (immediately or after very short delay) or
> hangs

Ok.  I'll bisect it when I can, but you might well get to it first.


-- 
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 v4 5/6] xics: directly register ICPState objects to vmstate

[David Gibson]

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

On Thu, Jun 08, 2017 at 03:43:18PM +0200, Greg Kurz wrote:
> 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 <groug@kaod.org>

This is certainly an improvement.  You answered my query on the
previous version as to why this doesn't break migration, but that
information should go into the commit message.

So, ideally, we would use the XICS "server number" as the migration
key.  That's an architected part of the XICs state, since those values
are entered explicitly into the ICS.  We have a way to go from server
number to ICP at the moment, but not the reverse, but we can fix that.

Unfortunately I think those won't always match existing automatically
generated IDs, which makes things harder.

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

-- 
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 v4 6/6] spapr: fix migration of ICPState objects from/to older QEMU

[David Gibson]

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

On Thu, Jun 08, 2017 at 03:43:27PM +0200, Greg Kurz wrote:
> 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 <groug@kaod.org>
> ---
> 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;

Comment here with why always false makes sense here would be useful
for the future.

> +}
> +
> +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];

AFAICT you now set, but never check the flags in the ignore_icp array,
so you should be able to get rid of it.

> +    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);

A comment around here explaining that the dummy entries get
deregistered when real ICPs are registered would also be helpful.

> +        }
> +    }
>  }
>  
>  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;
> 

-- 
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 v4 5/6] xics: directly register ICPState objects to vmstate

[Greg Kurz]

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

On Mon, 12 Jun 2017 22:15:46 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 08, 2017 at 03:43:18PM +0200, Greg Kurz wrote:
> > 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 <groug@kaod.org>  
> 
> This is certainly an improvement.  You answered my query on the
> previous version as to why this doesn't break migration, but that
> information should go into the commit message.
> 

I'll add this explanation to the changelog.

> So, ideally, we would use the XICS "server number" as the migration
> key.  That's an architected part of the XICs state, since those values
> are entered explicitly into the ICS.  We have a way to go from server
> number to ICP at the moment, but not the reverse, but we can fix that.
> 
> Unfortunately I think those won't always match existing automatically
> generated IDs, which makes things harder.
> 

Maybe things will get better when cpu_dt_id can be contiguous:

http://www.spinics.net/lists/kvm/msg150390.html

But I guess it won't help with existing machine types.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Greg Kurz]

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

On Mon, 12 Jun 2017 22:21:48 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 08, 2017 at 03:43:27PM +0200, Greg Kurz wrote:
> > 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 <groug@kaod.org>
> > ---
> > 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;  
> 
> Comment here with why always false makes sense here would be useful
> for the future.
> 

Ok.

> > +}
> > +
> > +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];  
> 
> AFAICT you now set, but never check the flags in the ignore_icp array,
> so you should be able to get rid of it.
> 

As explained in another mail, I need to pass a unique (void *) argument
to vmstate_register() and vmstate_unregister(). But I could also pass
(void*) i, in which case I could get rid of the array indeed.

> > +    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);  
> 
> A comment around here explaining that the dummy entries get
> deregistered when real ICPs are registered would also be helpful.
> 

Ok.

> > +        }
> > +    }
> >  }
> >  
> >  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;
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Greg Kurz]

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

On Sun, 11 Jun 2017 17:38:42 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 09, 2017 at 05:09:13PM +0200, Greg Kurz wrote:
> > On Fri, 9 Jun 2017 20:28:32 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Jun 09, 2017 at 11:36:31AM +0200, Greg Kurz wrote:  
> > > > On Fri, 9 Jun 2017 12:28:13 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> 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):    
> > > 
> > > Drat.  What's your reproducer for this crash?
> > >   
> > 
> > 1) start guest
> > 
> > qemu-system-ppc64 \
> >  -nodefaults -nographic -snapshot -no-shutdown -serial mon:stdio \
> >  -device virtio-net,netdev=netdev0,id=net0 \
> >  -netdev bridge,id=netdev0,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
> >  -device virtio-blk,drive=drive0,id=blk0 \
> >  -drive file=/home/greg/images/sle12-sp1-ppc64le.qcow2,id=drive0,if=none \
> >  -machine type=pseries,accel=tcg -cpu POWER8
> > 
> > 2) migrate
> > 
> > 3) destination crashes (immediately or after very short delay) or
> > hangs  
> 
> Ok.  I'll bisect it when I can, but you might well get to it first.
> 
> 

Heh, maybe you didn't see in my mail but I did bisect:

f0b0685d6694a28c66018f438e822596243b1250 is the first bad commit
commit f0b0685d6694a28c66018f438e822596243b1250
Author: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Date:   Thu Apr 27 10:48:23 2017 +0530

    tcg: enable MTTCG by default for PPC64 on x86


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v4 5/6] xics: directly register ICPState objects to vmstate

[David Gibson]

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

On Tue, Jun 13, 2017 at 09:14:55AM +0200, Greg Kurz wrote:
> On Mon, 12 Jun 2017 22:15:46 +0800
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 08, 2017 at 03:43:18PM +0200, Greg Kurz wrote:
> > > 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 <groug@kaod.org>  
> > 
> > This is certainly an improvement.  You answered my query on the
> > previous version as to why this doesn't break migration, but that
> > information should go into the commit message.
> > 
> 
> I'll add this explanation to the changelog.
> 
> > So, ideally, we would use the XICS "server number" as the migration
> > key.  That's an architected part of the XICs state, since those values
> > are entered explicitly into the ICS.  We have a way to go from server
> > number to ICP at the moment, but not the reverse, but we can fix that.
> > 
> > Unfortunately I think those won't always match existing automatically
> > generated IDs, which makes things harder.
> > 
> 
> Maybe things will get better when cpu_dt_id can be contiguous:
> 
> http://www.spinics.net/lists/kvm/msg150390.html
> 
> But I guess it won't help with existing machine types.

Right.

-- 
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 v4 0/6] spapr/xics: fix migration of older machine types

[Nikunj A Dadhania]

Greg Kurz <groug@kaod.org> writes:

> On Sun, 11 Jun 2017 17:38:42 +0800
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
>> On Fri, Jun 09, 2017 at 05:09:13PM +0200, Greg Kurz wrote:
>> > On Fri, 9 Jun 2017 20:28:32 +1000
>> > David Gibson <david@gibson.dropbear.id.au> wrote:
>> >   
>> > > On Fri, Jun 09, 2017 at 11:36:31AM +0200, Greg Kurz wrote:  
>> > > > On Fri, 9 Jun 2017 12:28:13 +1000
>> > > > David Gibson <david@gibson.dropbear.id.au> 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):    
>> > > 
>> > > Drat.  What's your reproducer for this crash?
>> > >   
>> > 
>> > 1) start guest
>> > 
>> > qemu-system-ppc64 \
>> >  -nodefaults -nographic -snapshot -no-shutdown -serial mon:stdio \
>> >  -device virtio-net,netdev=netdev0,id=net0 \
>> >  -netdev bridge,id=netdev0,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
>> >  -device virtio-blk,drive=drive0,id=blk0 \
>> >  -drive file=/home/greg/images/sle12-sp1-ppc64le.qcow2,id=drive0,if=none \
>> >  -machine type=pseries,accel=tcg -cpu POWER8

Strangely, your command line does not have multiple threads. Need to see
what is the side effect of enabling MTTCG by default here.

>> > 
>> > 2) migrate
>> > 
>> > 3) destination crashes (immediately or after very short delay) or
>> > hangs  
>> 
>> Ok.  I'll bisect it when I can, but you might well get to it first.
>> 
>> 
>
> Heh, maybe you didn't see in my mail but I did bisect:
>
> f0b0685d6694a28c66018f438e822596243b1250 is the first bad commit
> commit f0b0685d6694a28c66018f438e822596243b1250
> Author: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> Date:   Thu Apr 27 10:48:23 2017 +0530
>
>     tcg: enable MTTCG by default for PPC64 on x86

Let me have a look at it.

Regards,
Nikunj

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

[David Gibson]

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

On Tue, Jun 13, 2017 at 01:59:29PM +0530, Nikunj A Dadhania wrote:
> Greg Kurz <groug@kaod.org> writes:
> 
> > On Sun, 11 Jun 2017 17:38:42 +0800
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> >> On Fri, Jun 09, 2017 at 05:09:13PM +0200, Greg Kurz wrote:
> >> > On Fri, 9 Jun 2017 20:28:32 +1000
> >> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >> >   
> >> > > On Fri, Jun 09, 2017 at 11:36:31AM +0200, Greg Kurz wrote:  
> >> > > > On Fri, 9 Jun 2017 12:28:13 +1000
> >> > > > David Gibson <david@gibson.dropbear.id.au> 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):    
> >> > > 
> >> > > Drat.  What's your reproducer for this crash?
> >> > >   
> >> > 
> >> > 1) start guest
> >> > 
> >> > qemu-system-ppc64 \
> >> >  -nodefaults -nographic -snapshot -no-shutdown -serial mon:stdio \
> >> >  -device virtio-net,netdev=netdev0,id=net0 \
> >> >  -netdev bridge,id=netdev0,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
> >> >  -device virtio-blk,drive=drive0,id=blk0 \
> >> >  -drive file=/home/greg/images/sle12-sp1-ppc64le.qcow2,id=drive0,if=none \
> >> >  -machine type=pseries,accel=tcg -cpu POWER8
> 
> Strangely, your command line does not have multiple threads. Need to see
> what is the side effect of enabling MTTCG by default here.
> 
> >> > 
> >> > 2) migrate
> >> > 
> >> > 3) destination crashes (immediately or after very short delay) or
> >> > hangs  
> >> 
> >> Ok.  I'll bisect it when I can, but you might well get to it first.
> >> 
> >> 
> >
> > Heh, maybe you didn't see in my mail but I did bisect:
> >
> > f0b0685d6694a28c66018f438e822596243b1250 is the first bad commit
> > commit f0b0685d6694a28c66018f438e822596243b1250
> > Author: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> > Date:   Thu Apr 27 10:48:23 2017 +0530
> >
> >     tcg: enable MTTCG by default for PPC64 on x86
> 
> Let me have a look at it.

Is this crash fixed by the patch from Thomas I merged yesterday?

-- 
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 v4 0/6] spapr/xics: fix migration of older machine types

[Nikunj A Dadhania]

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

> On Tue, Jun 13, 2017 at 01:59:29PM +0530, Nikunj A Dadhania wrote:
>> Greg Kurz <groug@kaod.org> writes:
>> 
>> > On Sun, 11 Jun 2017 17:38:42 +0800
>> > David Gibson <david@gibson.dropbear.id.au> wrote:
>> >
>> >> On Fri, Jun 09, 2017 at 05:09:13PM +0200, Greg Kurz wrote:
>> >> > On Fri, 9 Jun 2017 20:28:32 +1000
>> >> > David Gibson <david@gibson.dropbear.id.au> wrote:
>> >> >   
>> >> > > On Fri, Jun 09, 2017 at 11:36:31AM +0200, Greg Kurz wrote:  
>> >> > > > On Fri, 9 Jun 2017 12:28:13 +1000
>> >> > > > David Gibson <david@gibson.dropbear.id.au> 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):    
>> >> > > 
>> >> > > Drat.  What's your reproducer for this crash?
>> >> > >   
>> >> > 
>> >> > 1) start guest
>> >> > 
>> >> > qemu-system-ppc64 \
>> >> >  -nodefaults -nographic -snapshot -no-shutdown -serial mon:stdio \
>> >> >  -device virtio-net,netdev=netdev0,id=net0 \
>> >> >  -netdev bridge,id=netdev0,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
>> >> >  -device virtio-blk,drive=drive0,id=blk0 \
>> >> >  -drive file=/home/greg/images/sle12-sp1-ppc64le.qcow2,id=drive0,if=none \
>> >> >  -machine type=pseries,accel=tcg -cpu POWER8
>> 
>> Strangely, your command line does not have multiple threads. Need to see
>> what is the side effect of enabling MTTCG by default here.
>> 
>> >> > 
>> >> > 2) migrate
>> >> > 
>> >> > 3) destination crashes (immediately or after very short delay) or
>> >> > hangs  
>> >> 
>> >> Ok.  I'll bisect it when I can, but you might well get to it first.
>> >> 
>> >> 
>> >
>> > Heh, maybe you didn't see in my mail but I did bisect:
>> >
>> > f0b0685d6694a28c66018f438e822596243b1250 is the first bad commit
>> > commit f0b0685d6694a28c66018f438e822596243b1250
>> > Author: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> > Date:   Thu Apr 27 10:48:23 2017 +0530
>> >
>> >     tcg: enable MTTCG by default for PPC64 on x86
>> 
>> Let me have a look at it.
>
> Is this crash fixed by the patch from Thomas I merged yesterday?

No, it doesn't. Moreover, in Greg's scenario there is only one cpu.

Regards,
Nikunj

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

[Nikunj A Dadhania]

Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:

> Greg Kurz <groug@kaod.org> writes:
>
>> On Sun, 11 Jun 2017 17:38:42 +0800
>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>
>>> On Fri, Jun 09, 2017 at 05:09:13PM +0200, Greg Kurz wrote:
>>> > On Fri, 9 Jun 2017 20:28:32 +1000
>>> > David Gibson <david@gibson.dropbear.id.au> wrote:
>>> >   
>>> > > On Fri, Jun 09, 2017 at 11:36:31AM +0200, Greg Kurz wrote:  
>>> > > > On Fri, 9 Jun 2017 12:28:13 +1000
>>> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
>>> > > >     
>>> > 1) start guest
>>> > 
>>> > qemu-system-ppc64 \
>>> >  -nodefaults -nographic -snapshot -no-shutdown -serial mon:stdio \
>>> >  -device virtio-net,netdev=netdev0,id=net0 \
>>> >  -netdev bridge,id=netdev0,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
>>> >  -device virtio-blk,drive=drive0,id=blk0 \
>>> >  -drive file=/home/greg/images/sle12-sp1-ppc64le.qcow2,id=drive0,if=none \
>>> >  -machine type=pseries,accel=tcg -cpu POWER8
>
> Strangely, your command line does not have multiple threads. Need to see
> what is the side effect of enabling MTTCG by default here.
>
>>> > 
>>> > 2) migrate
>>> > 
>>> > 3) destination crashes (immediately or after very short delay) or
>>> > hangs  
>>> 
>>> Ok.  I'll bisect it when I can, but you might well get to it first.
>>> 
>>> 
>>
>> Heh, maybe you didn't see in my mail but I did bisect:
>>
>> f0b0685d6694a28c66018f438e822596243b1250 is the first bad commit
>> commit f0b0685d6694a28c66018f438e822596243b1250
>> Author: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> Date:   Thu Apr 27 10:48:23 2017 +0530
>>
>>     tcg: enable MTTCG by default for PPC64 on x86
>
> Let me have a look at it.

Interesting problem here, I see that when the migration is completed on
source and there is a crash on destination:

[   56.185314] Unable to handle kernel paging request for data at address 0x5deadbeef0000108
[   56.185401] Faulting instruction address: 0xc000000000277bc8

   0xc000000000277bb8 <+168>:	ld      r7,8(r4)
   0xc000000000277bbc <+172>:	ld      r6,0(r4)                  <========
   0xc000000000277bc0 <+176>:	ori     r8,r8,56302
   0xc000000000277bc4 <+180>:	rldicr  r8,r8,32,31
   0xc000000000277bc8 <+184>:	std     r7,8(r6)

r4 = 0xf0000000000107a0
r6 = 0x5deadbeef0000100

Code at 0xc000000000277bbc <+172>, gave junk value in r6, that leads to
the guest crash. When I inspect the memory on source and destination in
qemu monitor, I get the following differences:

diff -u s.txt d.txt 
--- s.txt	2017-06-16 10:34:39.657221125 +0530
+++ d.txt	2017-06-16 10:34:18.452238305 +0530
@@ -8,8 +8,8 @@
 f000000000010760: 0x20de0b00 0x000000f0 0x60040100 0x000000f0
 f000000000010770: 0x00000000 0x00000000 0x0004036d 0x000000c0
 f000000000010780: 0x6c000100 0xf8ff3f00 0x7817f977 0x000000c0
-f000000000010790: 0x15000000 0x00000000 0xffffffff 0x01000000
-f0000000000107a0: 0x3090a96d 0x000000c0 0x3090a96d 0x000000c0
+f000000000010790: 0x01000000 0x00000000 0xffffffff 0x01000000
+f0000000000107a0: 0x000100f0 0xeedbea5d 0x000200f0 0xeedbea5d
 f0000000000107b0: 0x00000000 0x00000000 0x00d0a96d 0x000000c0
 f0000000000107c0: 0x28000000 0xf8ff3f00 0x8852cc77 0x000000c0
 f0000000000107d0: 0x00000000 0x00000000 0xffffffff 0x01000000

Source had a valid address at 0xf0000000000107a0, while garbage on the
destination.

Some observations:

* Source updates the memory location (probably atomic_cmpxchg), but the
  updated page didnt get transferred to the destination
  
* Getting rid of atomic_cmpxchg tcg ops in ldarx/stdcx, makes migration
  work fine. MTTCG running with 1 cpu.

While I continue debugging, any hints would help.

Regards
Nikunj

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

[David Gibson]

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

On Fri, Jun 16, 2017 at 04:23:38PM +0530, Nikunj A Dadhania wrote:
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
> 
> > Greg Kurz <groug@kaod.org> writes:
> >
> >> On Sun, 11 Jun 2017 17:38:42 +0800
> >> David Gibson <david@gibson.dropbear.id.au> wrote:
> >>
> >>> On Fri, Jun 09, 2017 at 05:09:13PM +0200, Greg Kurz wrote:
> >>> > On Fri, 9 Jun 2017 20:28:32 +1000
> >>> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >>> >   
> >>> > > On Fri, Jun 09, 2017 at 11:36:31AM +0200, Greg Kurz wrote:  
> >>> > > > On Fri, 9 Jun 2017 12:28:13 +1000
> >>> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> >>> > > >     
> >>> > 1) start guest
> >>> > 
> >>> > qemu-system-ppc64 \
> >>> >  -nodefaults -nographic -snapshot -no-shutdown -serial mon:stdio \
> >>> >  -device virtio-net,netdev=netdev0,id=net0 \
> >>> >  -netdev bridge,id=netdev0,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
> >>> >  -device virtio-blk,drive=drive0,id=blk0 \
> >>> >  -drive file=/home/greg/images/sle12-sp1-ppc64le.qcow2,id=drive0,if=none \
> >>> >  -machine type=pseries,accel=tcg -cpu POWER8
> >
> > Strangely, your command line does not have multiple threads. Need to see
> > what is the side effect of enabling MTTCG by default here.
> >
> >>> > 
> >>> > 2) migrate
> >>> > 
> >>> > 3) destination crashes (immediately or after very short delay) or
> >>> > hangs  
> >>> 
> >>> Ok.  I'll bisect it when I can, but you might well get to it first.
> >>> 
> >>> 
> >>
> >> Heh, maybe you didn't see in my mail but I did bisect:
> >>
> >> f0b0685d6694a28c66018f438e822596243b1250 is the first bad commit
> >> commit f0b0685d6694a28c66018f438e822596243b1250
> >> Author: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> >> Date:   Thu Apr 27 10:48:23 2017 +0530
> >>
> >>     tcg: enable MTTCG by default for PPC64 on x86
> >
> > Let me have a look at it.
> 
> Interesting problem here, I see that when the migration is completed on
> source and there is a crash on destination:
> 
> [   56.185314] Unable to handle kernel paging request for data at address 0x5deadbeef0000108
> [   56.185401] Faulting instruction address: 0xc000000000277bc8
> 
>    0xc000000000277bb8 <+168>:	ld      r7,8(r4)
>    0xc000000000277bbc <+172>:	ld      r6,0(r4)                  <========
>    0xc000000000277bc0 <+176>:	ori     r8,r8,56302
>    0xc000000000277bc4 <+180>:	rldicr  r8,r8,32,31
>    0xc000000000277bc8 <+184>:	std     r7,8(r6)
> 
> r4 = 0xf0000000000107a0
> r6 = 0x5deadbeef0000100
> 
> Code at 0xc000000000277bbc <+172>, gave junk value in r6, that leads to
> the guest crash. When I inspect the memory on source and destination in
> qemu monitor, I get the following differences:
> 
> diff -u s.txt d.txt 
> --- s.txt	2017-06-16 10:34:39.657221125 +0530
> +++ d.txt	2017-06-16 10:34:18.452238305 +0530
> @@ -8,8 +8,8 @@
>  f000000000010760: 0x20de0b00 0x000000f0 0x60040100 0x000000f0
>  f000000000010770: 0x00000000 0x00000000 0x0004036d 0x000000c0
>  f000000000010780: 0x6c000100 0xf8ff3f00 0x7817f977 0x000000c0
> -f000000000010790: 0x15000000 0x00000000 0xffffffff 0x01000000
> -f0000000000107a0: 0x3090a96d 0x000000c0 0x3090a96d 0x000000c0
> +f000000000010790: 0x01000000 0x00000000 0xffffffff 0x01000000
> +f0000000000107a0: 0x000100f0 0xeedbea5d 0x000200f0 0xeedbea5d
>  f0000000000107b0: 0x00000000 0x00000000 0x00d0a96d 0x000000c0
>  f0000000000107c0: 0x28000000 0xf8ff3f00 0x8852cc77 0x000000c0
>  f0000000000107d0: 0x00000000 0x00000000 0xffffffff 0x01000000
> 
> Source had a valid address at 0xf0000000000107a0, while garbage on the
> destination.
> 
> Some observations:
> 
> * Source updates the memory location (probably atomic_cmpxchg), but the
>   updated page didnt get transferred to the destination
>   
> * Getting rid of atomic_cmpxchg tcg ops in ldarx/stdcx, makes migration
>   work fine. MTTCG running with 1 cpu.
> 
> While I continue debugging, any hints would help.

My first guess would be that some or all of the new TCG atomic
primitives aren't updating the dirty page bitmap.

My second guess would be a race between the atomic TCG ops and the
migration / dirty map handling which means we can lost a memory update
and not transfer it to the destination.

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