[Qemu-devel] [RFC PATCH v1 0/5] sPAPR: Fix migration when CPUs are removed in random order

[Qemu-devel] [RFC PATCH v1 0/5] sPAPR: Fix migration when CPUs are removed in random order

[Bharata B Rao]

device_add/del based CPU hotplug and unplug support is upstream for
sPAPR PowerPC and is under development for x86. Both of these will
support CPU device removal in random order (and not necessarily in LIFO
order). Random order removal will result in holes in cpu_index range
which causes migration to fail. This needs fixes in both generic code
as well as arch specific code.

- migration_id is newly introduced and used as instance_id when registering
  CPU devices using vmstate_register. migration_id is set by the
  target machine code. To support forward migration, as per Igor's
  suggestion, this needs to be done conditionally based on machine type
  version.
- From pseries-2.7 onwards, we start using migration_id for migration as
  well as in XICS code.

vmstate registration calls are moved to cpu_common_realizefn and newly
introduced cpu_common_unrealizefn.

This patchset depends on Greg Kurz's patchset where among other things,
he is deriving cpu_dt_it (arch_id) based on core-id.
(https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00210.html)

Tested changes to XICS code with both kernel_irqchip=on/off.
This applies on ppc-for-2.7 branch of David's tree.

Changes in v1
-------------
- Introduced CPUState::migration_id to be used with vmstate_register()
- migration_id is now set by target machine code via
  CPUClass::get_migration_id()
- Use of migration is controlled by a property CPUState:use_migration_id.
- Use of SPAPR_COMPAT_2_6 to set use-migration-id property for all CPUs
  starting from pseries-2.7 onwards.
- Calling vmstate_[un]register() from cpu_common_[un]realizefn calls.

v0: http://lists.nongnu.org/archive/html/qemu-ppc/2016-07/msg00090.html

Bharata B Rao (5):
  cpu,target-ppc: Move cpu_vmstate_[un]register calls to
    cpu_common_[un]realize
  cpu: Introduce CPUState::migration_id
  spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs
  xics: Use migration_id instead of cpu_index in XICS code
  cpu,spapr: Use migration_id from pseries-2.7 onwards

 exec.c                      | 54 ++++++++++++++++++++++++++++-----------------
 hw/intc/xics.c              | 12 ++++++----
 hw/intc/xics_kvm.c          | 11 +++++----
 hw/intc/xics_spapr.c        | 28 +++++++++++++++++------
 hw/ppc/spapr.c              |  5 +++++
 include/qom/cpu.h           |  8 +++++++
 qom/cpu.c                   | 29 ++++++++++++++++++++++++
 target-ppc/cpu-qom.h        |  2 ++
 target-ppc/translate_init.c | 15 +++++++++++++
 9 files changed, 127 insertions(+), 37 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC PATCH v1 1/5] cpu, target-ppc: Move cpu_vmstate_[un]register calls to cpu_common_[un]realize

[Bharata B Rao]

Move vmstate_register() call to cpu_common_realize().
Introduce cpu_common_unrealize() and move vmstate_unregister() to it.

Change those archs that implement their own CPU unrealize routine to
mandatorily call CPUClass::unrealize().

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 exec.c                      | 53 ++++++++++++++++++++++++++++-----------------
 include/qom/cpu.h           |  2 ++
 qom/cpu.c                   |  7 ++++++
 target-ppc/cpu-qom.h        |  2 ++
 target-ppc/translate_init.c |  3 +++
 5 files changed, 47 insertions(+), 20 deletions(-)

diff --git a/exec.c b/exec.c
index 0122ef7..fb73910 100644
--- a/exec.c
+++ b/exec.c
@@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
     /* Return the AddressSpace corresponding to the specified index */
     return cpu->cpu_ases[asidx].as;
 }
-#endif
 
-#ifndef CONFIG_USER_ONLY
 static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
 
 static int cpu_get_free_index(Error **errp)
@@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
 {
     bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
 }
+
+void cpu_vmstate_register(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
+        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
+    }
+    if (cc->vmsd != NULL) {
+        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
+    }
+}
+
+void cpu_vmstate_unregister(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->vmsd != NULL) {
+        vmstate_unregister(NULL, cc->vmsd, cpu);
+    }
+    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
+        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
+    }
+}
+
 #else
 
 static int cpu_get_free_index(Error **errp)
@@ -634,12 +657,18 @@ static void cpu_release_index(CPUState *cpu)
 {
     return;
 }
+
+void cpu_vmstate_register(CPUState *cpu)
+{
+}
+
+void cpu_vmstate_unregister(CPUState *cpu)
+{
+}
 #endif
 
 void cpu_exec_exit(CPUState *cpu)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
-
 #if defined(CONFIG_USER_ONLY)
     cpu_list_lock();
 #endif
@@ -657,18 +686,10 @@ void cpu_exec_exit(CPUState *cpu)
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
 #endif
-
-    if (cc->vmsd != NULL) {
-        vmstate_unregister(NULL, cc->vmsd, cpu);
-    }
-    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
-    }
 }
 
 void cpu_exec_init(CPUState *cpu, Error **errp)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
 
     cpu->as = NULL;
@@ -705,15 +726,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
     }
     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
 #if defined(CONFIG_USER_ONLY)
-    (void) cc;
     cpu_list_unlock();
-#else
-    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
-    }
-    if (cc->vmsd != NULL) {
-        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
-    }
 #endif
 }
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 32f3af3..29ccf5c 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -870,4 +870,6 @@ extern const struct VMStateDescription vmstate_cpu_common;
     .offset = 0,                                                            \
 }
 
+void cpu_vmstate_register(CPUState *cpu);
+void cpu_vmstate_unregister(CPUState *cpu);
 #endif
diff --git a/qom/cpu.c b/qom/cpu.c
index 751e992..488ecc6 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -310,10 +310,16 @@ static void cpu_common_parse_features(CPUState *cpu, char *features,
     }
 }
 
+static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
+{
+    cpu_vmstate_unregister(CPU(dev));
+}
+
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
 
+    cpu_vmstate_register(cpu);
     if (dev->hotplugged) {
         cpu_synchronize_post_init(cpu);
         cpu_resume(cpu);
@@ -367,6 +373,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->cpu_exec_exit = cpu_common_noop;
     k->cpu_exec_interrupt = cpu_common_exec_interrupt;
     dc->realize = cpu_common_realizefn;
+    dc->unrealize = cpu_common_unrealizefn;
     /*
      * Reason: CPUs still need special care by board code: wiring up
      * IRQs, adding reset handlers, halting non-first CPUs, ...
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 2864105..6ec2fca 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -163,6 +163,7 @@ struct ppc_segment_page_sizes;
 /**
  * PowerPCCPUClass:
  * @parent_realize: The parent class' realize handler.
+ * @parent_unrealize: The parent class' unrealize handler.
  * @parent_reset: The parent class' reset handler.
  *
  * A PowerPC CPU model.
@@ -173,6 +174,7 @@ typedef struct PowerPCCPUClass {
     /*< public >*/
 
     DeviceRealize parent_realize;
+    DeviceUnrealize parent_unrealize;
     void (*parent_reset)(CPUState *cpu);
 
     uint32_t pvr;
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 8f257fb..efd6b88 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9773,10 +9773,12 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
 static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
 {
     PowerPCCPU *cpu = POWERPC_CPU(dev);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
     opc_handler_t **table;
     int i, j;
 
+    pcc->parent_unrealize(dev, errp);
     cpu_exec_exit(CPU(dev));
 
     for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
@@ -10364,6 +10366,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     pcc->parent_realize = dc->realize;
+    pcc->parent_unrealize = dc->unrealize;
     pcc->pvr_match = ppc_pvr_match_default;
     pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
     dc->realize = ppc_cpu_realizefn;
-- 
2.7.4

[Qemu-devel] [RFC PATCH v1 2/5] cpu: Introduce CPUState::migration_id

[Bharata B Rao]

Add CPUState::migration_id and use that as instance_id in
vmstate_register() call.

Introduce use-migration-id property that allows target machines to
optionally switch to using migration_id instead of cpu_index.
This will help allow successful migration in cases where holes are
introduced in cpu_index range after CPU hot removals.

The default implementation of CPUClass:get_migration_id() continues
to return cpu_index.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 exec.c            |  5 +++--
 include/qom/cpu.h |  6 ++++++
 qom/cpu.c         | 21 +++++++++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index fb73910..a8afeda 100644
--- a/exec.c
+++ b/exec.c
@@ -619,12 +619,13 @@ static void cpu_release_index(CPUState *cpu)
 void cpu_vmstate_register(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
+    int instance_id = cc->get_migration_id(cpu);
 
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
+        vmstate_register(NULL, instance_id, &vmstate_cpu_common, cpu);
     }
     if (cc->vmsd != NULL) {
-        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
+        vmstate_register(NULL, instance_id, cc->vmsd, cpu);
     }
 }
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 29ccf5c..6d00143 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -187,6 +187,7 @@ typedef struct CPUClass {
     bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
 
     void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
+    int (*get_migration_id)(CPUState *cpu);
 } CPUClass;
 
 #ifdef HOST_WORDS_BIGENDIAN
@@ -273,6 +274,9 @@ struct qemu_work_item {
  * @kvm_fd: vCPU file descriptor for KVM.
  * @work_mutex: Lock to prevent multiple access to queued_work_*.
  * @queued_work_first: First asynchronous work pending.
+ * @migration_id: Use as instance_id argument in cpu vmstate_register calls
+ * @use_migration_id: Set to enforce the use of CPUClass.get_migration_id()
+ *     over cpu_index during vmstate registration.
  *
  * State of one CPU core or thread.
  */
@@ -360,6 +364,8 @@ struct CPUState {
        (absolute value) offset as small as possible.  This reduces code
        size, especially for hosts without large memory offsets.  */
     uint32_t tcg_exit_req;
+    int migration_id;
+    bool use_migration_id;
 };
 
 QTAILQ_HEAD(CPUTailQ, CPUState);
diff --git a/qom/cpu.c b/qom/cpu.c
index 488ecc6..01cf136 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -326,6 +326,17 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
     }
 }
 
+static bool cpu_common_get_use_migration_id(Object *obj, Error **errp)
+{
+    return CPU(obj)->use_migration_id;
+}
+
+static void cpu_common_set_use_migration_id(Object *obj, bool value,
+                                            Error **err)
+{
+    CPU(obj)->use_migration_id = value;
+}
+
 static void cpu_common_initfn(Object *obj)
 {
     CPUState *cpu = CPU(obj);
@@ -336,6 +347,10 @@ static void cpu_common_initfn(Object *obj)
     qemu_mutex_init(&cpu->work_mutex);
     QTAILQ_INIT(&cpu->breakpoints);
     QTAILQ_INIT(&cpu->watchpoints);
+
+    object_property_add_bool(obj, "use-migration-id",
+                             cpu_common_get_use_migration_id,
+                             cpu_common_set_use_migration_id, NULL);
 }
 
 static void cpu_common_finalize(Object *obj)
@@ -348,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
     return cpu->cpu_index;
 }
 
+static int cpu_common_get_migration_id(CPUState *cpu)
+{
+    return cpu->cpu_index;
+}
+
 static void cpu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -372,6 +392,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->cpu_exec_enter = cpu_common_noop;
     k->cpu_exec_exit = cpu_common_noop;
     k->cpu_exec_interrupt = cpu_common_exec_interrupt;
+    k->get_migration_id = cpu_common_get_migration_id;
     dc->realize = cpu_common_realizefn;
     dc->unrealize = cpu_common_unrealizefn;
     /*
-- 
2.7.4

[Qemu-devel] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[Bharata B Rao]

cpu_index is used as migration_id by default. For machine type versions
that set use-migration-id property, cpu_dt_it is returned.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 target-ppc/translate_init.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index efd6b88..9ca2f5e 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
 #endif
 }
 
+static int ppc_cpu_get_migration_id(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+    if (cs->use_migration_id) {
+        return (int) cpu->cpu_dt_id;
+    } else {
+        return cs->cpu_index;
+    }
+}
+
 static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
@@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 #ifndef CONFIG_USER_ONLY
     cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
 #endif
+    cc->get_migration_id = ppc_cpu_get_migration_id;
 
     dc->fw_name = "PowerPC,UNKNOWN";
 }
-- 
2.7.4

[Qemu-devel] [RFC PATCH v1 4/5] xics: Use migration_id instead of cpu_index in XICS code

[Bharata B Rao]

xics maintains an array of ICPState structures which is indexed
by cpu_index. Change this to index the ICPState array by migration_id
for pseries-2.7 onwards. This allows migration of guest to suceed
when there are holes in cpu_index range due to CPU hot removal.

NOTE: In rtas_set_xive() and h_ipi(), cpu_dt_id is implicitly
assume to be equivalent to migration_id.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/intc/xics.c       | 12 ++++++++----
 hw/intc/xics_kvm.c   | 11 +++++------
 hw/intc/xics_spapr.c | 28 +++++++++++++++++++++-------
 3 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index cd48f42..ce7571e 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -50,9 +50,11 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
 void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
-    ICPState *ss = &xics->ss[cs->cpu_index];
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cc->get_migration_id(cs);
+    ICPState *ss = &xics->ss[server];
 
-    assert(cs->cpu_index < xics->nr_servers);
+    assert(server < xics->nr_servers);
     assert(cs == ss->cs);
 
     ss->output = NULL;
@@ -63,10 +65,12 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
-    ICPState *ss = &xics->ss[cs->cpu_index];
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cc->get_migration_id(cs);
+    ICPState *ss = &xics->ss[server];
     XICSStateClass *info = XICS_COMMON_GET_CLASS(xics);
 
-    assert(cs->cpu_index < xics->nr_servers);
+    assert(server < xics->nr_servers);
 
     ss->cs = cs;
 
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index edbd62f..2c087a4 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -326,14 +326,13 @@ static const TypeInfo ics_kvm_info = {
  */
 static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
 {
-    CPUState *cs;
-    ICPState *ss;
+    CPUState *cs = CPU(cpu);
     KVMXICSState *xicskvm = XICS_SPAPR_KVM(xics);
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cc->get_migration_id(cs);
+    ICPState *ss = ss = &xics->ss[server];
 
-    cs = CPU(cpu);
-    ss = &xics->ss[cs->cpu_index];
-
-    assert(cs->cpu_index < xics->nr_servers);
+    assert(server < xics->nr_servers);
     if (xicskvm->kernel_xics_fd == -1) {
         abort();
     }
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 618826d..281ba76 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -43,16 +43,20 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cc->get_migration_id(cs);
     target_ulong cppr = args[0];
 
-    icp_set_cppr(spapr->xics, cs->cpu_index, cppr);
+    icp_set_cppr(spapr->xics, server, cppr);
     return H_SUCCESS;
 }
 
 static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           target_ulong opcode, target_ulong *args)
 {
-    target_ulong server = xics_get_cpu_index_by_dt_id(args[0]);
+    CPUState *cs = CPU(cpu);
+    target_ulong server = cs->use_migration_id ? args[0] :
+                          xics_get_cpu_index_by_dt_id(args[0]);
     target_ulong mfrr = args[1];
 
     if (server >= spapr->xics->nr_servers) {
@@ -67,7 +71,9 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
-    uint32_t xirr = icp_accept(spapr->xics->ss + cs->cpu_index);
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cc->get_migration_id(cs);
+    uint32_t xirr = icp_accept(spapr->xics->ss + server);
 
     args[0] = xirr;
     return H_SUCCESS;
@@ -77,7 +83,9 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                              target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
-    ICPState *ss = &spapr->xics->ss[cs->cpu_index];
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cc->get_migration_id(cs);
+    ICPState *ss = &spapr->xics->ss[server];
     uint32_t xirr = icp_accept(ss);
 
     args[0] = xirr;
@@ -89,9 +97,11 @@ static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cc->get_migration_id(cs);
     target_ulong xirr = args[0];
 
-    icp_eoi(spapr->xics, cs->cpu_index, xirr);
+    icp_eoi(spapr->xics, server, xirr);
     return H_SUCCESS;
 }
 
@@ -99,8 +109,10 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                             target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cc->get_migration_id(cs);
     uint32_t mfrr;
-    uint32_t xirr = icp_ipoll(spapr->xics->ss + cs->cpu_index, &mfrr);
+    uint32_t xirr = icp_ipoll(spapr->xics->ss + server, &mfrr);
 
     args[0] = xirr;
     args[1] = mfrr;
@@ -113,6 +125,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           uint32_t nargs, target_ulong args,
                           uint32_t nret, target_ulong rets)
 {
+    CPUState *cs = CPU(cpu);
     ICSState *ics = spapr->xics->ics;
     uint32_t nr, server, priority;
 
@@ -122,7 +135,8 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     }
 
     nr = rtas_ld(args, 0);
-    server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
+    server = cs->use_migration_id ? rtas_ld(args, 1) :
+             xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
     priority = rtas_ld(args, 2);
 
     if (!ics_valid_irq(ics, nr) || (server >= ics->xics->nr_servers)
-- 
2.7.4

[Qemu-devel] [RFC PATCH v1 5/5] cpu, spapr: Use migration_id from pseries-2.7 onwards

[Bharata B Rao]

Turn on use-migration-id property. Starting from pseries-2.7, prefer the
use of migration_id (cpu_dt_id) over cpu_index for cpu vmstate registration
and in XICS code.

This allows migration to work when CPU cores are not necessarily
unplugged in LIFO order.

TODO: Property use-migration-id is by default turned on, check
if this needs to turned off for older machine type versions of
all archs.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 5 +++++
 qom/cpu.c      | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7f33a1b..be9af10 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2501,6 +2501,11 @@ DEFINE_SPAPR_MACHINE(2_7, "2.7", true);
         .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
         .property = "ddw",\
         .value    = stringify(off),\
+    }, \
+    { \
+        .driver   = TYPE_CPU,\
+        .property = "use-migration-id",\
+        .value    = stringify(off),\
     },
 
 static void spapr_machine_2_6_instance_options(MachineState *machine)
diff --git a/qom/cpu.c b/qom/cpu.c
index 01cf136..e505810 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -351,6 +351,7 @@ static void cpu_common_initfn(Object *obj)
     object_property_add_bool(obj, "use-migration-id",
                              cpu_common_get_use_migration_id,
                              cpu_common_set_use_migration_id, NULL);
+    object_property_set_bool(obj, true, "use-migration-id", NULL);
 }
 
 static void cpu_common_finalize(Object *obj)
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH v1 4/5] xics: Use migration_id instead of cpu_index in XICS code

[Nikunj A Dadhania]

Bharata B Rao <bharata@linux.vnet.ibm.com> writes:

> xics maintains an array of ICPState structures which is indexed
> by cpu_index. Change this to index the ICPState array by migration_id
> for pseries-2.7 onwards. This allows migration of guest to suceed
> when there are holes in cpu_index range due to CPU hot removal.
>
> NOTE: In rtas_set_xive() and h_ipi(), cpu_dt_id is implicitly
> assume to be equivalent to migration_id.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/intc/xics.c       | 12 ++++++++----
>  hw/intc/xics_kvm.c   | 11 +++++------
>  hw/intc/xics_spapr.c | 28 +++++++++++++++++++++-------
>  3 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index cd48f42..ce7571e 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -50,9 +50,11 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
>  void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
> -    ICPState *ss = &xics->ss[cs->cpu_index];
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    int server = cc->get_migration_id(cs);
> +    ICPState *ss = &xics->ss[server];

A helper like this will make code much more compact at other places:

static inline int xics_get_server(PowerPCCPU cpu)
{
    CPUState *cs = CPU(cpu);
    CPUClass *cc = CPU_GET_CLASS(cs);

    return cc->get_migration_id(cs);
}


void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu)
{
    int server = xics_get_server(cpu);
    ICPState *ss = &xics->ss[server];
....

}

Regards
Nikunj

Re: [Qemu-devel] [RFC PATCH v1 1/5] cpu, target-ppc: Move cpu_vmstate_[un]register calls to cpu_common_[un]realize

[Igor Mammedov]

On Wed,  6 Jul 2016 14:29:17 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Move vmstate_register() call to cpu_common_realize().
> Introduce cpu_common_unrealize() and move vmstate_unregister() to it.
> 
> Change those archs that implement their own CPU unrealize routine to
> mandatorily call CPUClass::unrealize().
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  exec.c                      | 53
> ++++++++++++++++++++++++++++-----------------
> include/qom/cpu.h           |  2 ++ qom/cpu.c                   |  7
> ++++++ target-ppc/cpu-qom.h        |  2 ++
>  target-ppc/translate_init.c |  3 +++
>  5 files changed, 47 insertions(+), 20 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 0122ef7..fb73910 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState
> *cpu, int asidx) /* Return the AddressSpace corresponding to the
> specified index */ return cpu->cpu_ases[asidx].as;
>  }
> -#endif
>  
> -#ifndef CONFIG_USER_ONLY
>  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
>  
>  static int cpu_get_free_index(Error **errp)
> @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
>  {
>      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
>  }
> +
> +void cpu_vmstate_register(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> +        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common,
> cpu);
> +    }
> +    if (cc->vmsd != NULL) {
> +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +    }
> +}
> +
> +void cpu_vmstate_unregister(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->vmsd != NULL) {
> +        vmstate_unregister(NULL, cc->vmsd, cpu);
> +    }
> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> +    }
> +}

Is there any reason to keep these in exec.c,
I'd put them in qom/cpu.c


>  #else
>  
>  static int cpu_get_free_index(Error **errp)
> @@ -634,12 +657,18 @@ static void cpu_release_index(CPUState *cpu)
>  {
>      return;
>  }
> +
> +void cpu_vmstate_register(CPUState *cpu)
> +{
> +}
> +
> +void cpu_vmstate_unregister(CPUState *cpu)
> +{
> +}
>  #endif
>  
>  void cpu_exec_exit(CPUState *cpu)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> -
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
>  #endif
> @@ -657,18 +686,10 @@ void cpu_exec_exit(CPUState *cpu)
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
>  #endif
> -
> -    if (cc->vmsd != NULL) {
> -        vmstate_unregister(NULL, cc->vmsd, cpu);
> -    }
> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> -    }
>  }
>  
>  void cpu_exec_init(CPUState *cpu, Error **errp)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
>      Error *local_err = NULL;
>  
>      cpu->as = NULL;
> @@ -705,15 +726,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      }
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>  #if defined(CONFIG_USER_ONLY)
> -    (void) cc;
>      cpu_list_unlock();
> -#else
> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common,
> cpu);
> -    }
> -    if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> -    }
>  #endif
>  }
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 32f3af3..29ccf5c 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -870,4 +870,6 @@ extern const struct VMStateDescription
> vmstate_cpu_common; .offset =
> 0,                                                            \ }
>  
> +void cpu_vmstate_register(CPUState *cpu);
> +void cpu_vmstate_unregister(CPUState *cpu);
>  #endif
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 751e992..488ecc6 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -310,10 +310,16 @@ static void cpu_common_parse_features(CPUState
> *cpu, char *features, }
>  }
>  
> +static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
> +{
> +    cpu_vmstate_unregister(CPU(dev));
> +}
> +
>  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cpu = CPU(dev);
>  
> +    cpu_vmstate_register(cpu);
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(cpu);
>          cpu_resume(cpu);
> @@ -367,6 +373,7 @@ static void cpu_class_init(ObjectClass *klass,
> void *data) k->cpu_exec_exit = cpu_common_noop;
>      k->cpu_exec_interrupt = cpu_common_exec_interrupt;
>      dc->realize = cpu_common_realizefn;
> +    dc->unrealize = cpu_common_unrealizefn;
>      /*
>       * Reason: CPUs still need special care by board code: wiring up
>       * IRQs, adding reset handlers, halting non-first CPUs, ...
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index 2864105..6ec2fca 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -163,6 +163,7 @@ struct ppc_segment_page_sizes;
>  /**
>   * PowerPCCPUClass:
>   * @parent_realize: The parent class' realize handler.
> + * @parent_unrealize: The parent class' unrealize handler.
>   * @parent_reset: The parent class' reset handler.
>   *
>   * A PowerPC CPU model.
> @@ -173,6 +174,7 @@ typedef struct PowerPCCPUClass {
>      /*< public >*/
>  
>      DeviceRealize parent_realize;
> +    DeviceUnrealize parent_unrealize;
>      void (*parent_reset)(CPUState *cpu);
>  
>      uint32_t pvr;
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 8f257fb..efd6b88 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9773,10 +9773,12 @@ static void ppc_cpu_realizefn(DeviceState
> *dev, Error **errp) static void ppc_cpu_unrealizefn(DeviceState *dev,
> Error **errp) {
>      PowerPCCPU *cpu = POWERPC_CPU(dev);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
>      opc_handler_t **table;
>      int i, j;
>  
> +    pcc->parent_unrealize(dev, errp);
>      cpu_exec_exit(CPU(dev));
>  
>      for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
> @@ -10364,6 +10366,7 @@ static void ppc_cpu_class_init(ObjectClass
> *oc, void *data) DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      pcc->parent_realize = dc->realize;
> +    pcc->parent_unrealize = dc->unrealize;
>      pcc->pvr_match = ppc_pvr_match_default;
>      pcc->interrupts_big_endian =
> ppc_cpu_interrupts_big_endian_always; dc->realize = ppc_cpu_realizefn;

Re: [Qemu-devel] [RFC PATCH v1 2/5] cpu: Introduce CPUState::migration_id

[Igor Mammedov]

On Wed,  6 Jul 2016 14:29:18 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Add CPUState::migration_id and use that as instance_id in
> vmstate_register() call.
> 
> Introduce use-migration-id property that allows target machines to
> optionally switch to using migration_id instead of cpu_index.
> This will help allow successful migration in cases where holes are
> introduced in cpu_index range after CPU hot removals.
> 
> The default implementation of CPUClass:get_migration_id() continues
> to return cpu_index.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  exec.c            |  5 +++--
>  include/qom/cpu.h |  6 ++++++
>  qom/cpu.c         | 21 +++++++++++++++++++++
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index fb73910..a8afeda 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -619,12 +619,13 @@ static void cpu_release_index(CPUState *cpu)
>  void cpu_vmstate_register(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int instance_id = cc->get_migration_id(cpu);
>  
>      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common,
> cpu);
> +        vmstate_register(NULL, instance_id, &vmstate_cpu_common,
> cpu); }
>      if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +        vmstate_register(NULL, instance_id, cc->vmsd, cpu);
>      }
>  }
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 29ccf5c..6d00143 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -187,6 +187,7 @@ typedef struct CPUClass {
>      bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
>  
>      void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
> +    int (*get_migration_id)(CPUState *cpu);
>  } CPUClass;
>  
>  #ifdef HOST_WORDS_BIGENDIAN
> @@ -273,6 +274,9 @@ struct qemu_work_item {
>   * @kvm_fd: vCPU file descriptor for KVM.
>   * @work_mutex: Lock to prevent multiple access to queued_work_*.
>   * @queued_work_first: First asynchronous work pending.
> + * @migration_id: Use as instance_id argument in cpu
> vmstate_register calls
with this field get_migration_id() callback is redundant.
You can drop get_migration_id() and assign value to migration_id in
machine specific code /spapr/ppc/x86/.../ before realize is called.
That should simplify this patch and its users as well.

> + * @use_migration_id: Set to enforce the use of
> CPUClass.get_migration_id()
> + *     over cpu_index during vmstate registration.
>   *
>   * State of one CPU core or thread.
>   */
> @@ -360,6 +364,8 @@ struct CPUState {
>         (absolute value) offset as small as possible.  This reduces
> code size, especially for hosts without large memory offsets.  */
>      uint32_t tcg_exit_req;
> +    int migration_id;
> +    bool use_migration_id;
>  };
>  
>  QTAILQ_HEAD(CPUTailQ, CPUState);
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 488ecc6..01cf136 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -326,6 +326,17 @@ static void cpu_common_realizefn(DeviceState
> *dev, Error **errp) }
>  }
>  
> +static bool cpu_common_get_use_migration_id(Object *obj, Error
> **errp) +{
> +    return CPU(obj)->use_migration_id;
> +}
> +
> +static void cpu_common_set_use_migration_id(Object *obj, bool value,
> +                                            Error **err)
> +{
> +    CPU(obj)->use_migration_id = value;
> +}
It's possible to create dumb property in a more compact way,
see DEFINE_PROP_BOOL()


>  static void cpu_common_initfn(Object *obj)
>  {
>      CPUState *cpu = CPU(obj);
> @@ -336,6 +347,10 @@ static void cpu_common_initfn(Object *obj)
>      qemu_mutex_init(&cpu->work_mutex);
>      QTAILQ_INIT(&cpu->breakpoints);
>      QTAILQ_INIT(&cpu->watchpoints);
> +
> +    object_property_add_bool(obj, "use-migration-id",
> +                             cpu_common_get_use_migration_id,
> +                             cpu_common_set_use_migration_id, NULL);
>  }
>  
>  static void cpu_common_finalize(Object *obj)
> @@ -348,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState
> *cpu) return cpu->cpu_index;
>  }
>  
> +static int cpu_common_get_migration_id(CPUState *cpu)
> +{
> +    return cpu->cpu_index;
> +}
> +
>  static void cpu_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -372,6 +392,7 @@ static void cpu_class_init(ObjectClass *klass,
> void *data) k->cpu_exec_enter = cpu_common_noop;
>      k->cpu_exec_exit = cpu_common_noop;
>      k->cpu_exec_interrupt = cpu_common_exec_interrupt;
> +    k->get_migration_id = cpu_common_get_migration_id;
>      dc->realize = cpu_common_realizefn;
>      dc->unrealize = cpu_common_unrealizefn;
>      /*

Re: [Qemu-devel] [RFC PATCH v1 5/5] cpu, spapr: Use migration_id from pseries-2.7 onwards

[Igor Mammedov]

On Wed,  6 Jul 2016 14:29:21 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Turn on use-migration-id property. Starting from pseries-2.7, prefer
> the use of migration_id (cpu_dt_id) over cpu_index for cpu vmstate
> registration and in XICS code.
> 
> This allows migration to work when CPU cores are not necessarily
> unplugged in LIFO order.
> 
> TODO: Property use-migration-id is by default turned on, check
> if this needs to turned off for older machine type versions of
> all archs.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 5 +++++
>  qom/cpu.c      | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7f33a1b..be9af10 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2501,6 +2501,11 @@ DEFINE_SPAPR_MACHINE(2_7, "2.7", true);
>          .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
>          .property = "ddw",\
>          .value    = stringify(off),\
> +    }, \
> +    { \
> +        .driver   = TYPE_CPU,\
> +        .property = "use-migration-id",\
> +        .value    = stringify(off),\
>      },
>  
>  static void spapr_machine_2_6_instance_options(MachineState *machine)
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 01cf136..e505810 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -351,6 +351,7 @@ static void cpu_common_initfn(Object *obj)
>      object_property_add_bool(obj, "use-migration-id",
>                               cpu_common_get_use_migration_id,
>                               cpu_common_set_use_migration_id, NULL);
> +    object_property_set_bool(obj, true, "use-migration-id", NULL);
this hunk doesn't belong here, it should be in patch that introduces,
default FALSE and targets that use it can flip it on and off depending
on machine version.


>  }
>  
>  static void cpu_common_finalize(Object *obj)

Re: [Qemu-devel] [RFC PATCH v1 5/5] cpu, spapr: Use migration_id from pseries-2.7 onwards

[Igor Mammedov]

On Wed,  6 Jul 2016 14:29:21 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Turn on use-migration-id property. Starting from pseries-2.7, prefer
> the use of migration_id (cpu_dt_id) over cpu_index for cpu vmstate
> registration and in XICS code.
> 
> This allows migration to work when CPU cores are not necessarily
> unplugged in LIFO order.
> 
> TODO: Property use-migration-id is by default turned on, check
> if this needs to turned off for older machine type versions of
> all archs.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 5 +++++
>  qom/cpu.c      | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7f33a1b..be9af10 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2501,6 +2501,11 @@ DEFINE_SPAPR_MACHINE(2_7, "2.7", true);
>          .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
>          .property = "ddw",\
>          .value    = stringify(off),\
> +    }, \
> +    { \
> +        .driver   = TYPE_CPU,\
> +        .property = "use-migration-id",\
> +        .value    = stringify(off),\
nit,
 why not to use "off" directly, stringify looks odd here.

>      },
>  
>  static void spapr_machine_2_6_instance_options(MachineState *machine)
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 01cf136..e505810 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -351,6 +351,7 @@ static void cpu_common_initfn(Object *obj)
>      object_property_add_bool(obj, "use-migration-id",
>                               cpu_common_get_use_migration_id,
>                               cpu_common_set_use_migration_id, NULL);
> +    object_property_set_bool(obj, true, "use-migration-id", NULL);
>  }
>  
>  static void cpu_common_finalize(Object *obj)

Re: [Qemu-devel] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[Igor Mammedov]

On Wed,  6 Jul 2016 14:29:19 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> cpu_index is used as migration_id by default. For machine type
> versions that set use-migration-id property, cpu_dt_it is returned.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  target-ppc/translate_init.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index efd6b88..9ca2f5e 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
>  #endif
>  }
>  
> +static int ppc_cpu_get_migration_id(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    if (cs->use_migration_id) {
> +        return (int) cpu->cpu_dt_id;
Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe
to do so, that's the reason why I'm going to use index in possible_cpus
on ARM and for the sake of uniformity do the same for x86 (even though
it's possible to use 32bit APIC ID).

> +    } else {
> +        return cs->cpu_index;
> +    }
> +}
> +
>  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass
> *oc, void *data) #ifndef CONFIG_USER_ONLY
>      cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
>  #endif
> +    cc->get_migration_id = ppc_cpu_get_migration_id;
>  
>      dc->fw_name = "PowerPC,UNKNOWN";
>  }

Re: [Qemu-devel] [RFC PATCH v1 1/5] cpu, target-ppc: Move cpu_vmstate_[un]register calls to cpu_common_[un]realize

[Bharata B Rao]

On Wed, Jul 06, 2016 at 12:57:49PM +0200, Igor Mammedov wrote:
> On Wed,  6 Jul 2016 14:29:17 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Move vmstate_register() call to cpu_common_realize().
> > Introduce cpu_common_unrealize() and move vmstate_unregister() to it.
> > 
> > Change those archs that implement their own CPU unrealize routine to
> > mandatorily call CPUClass::unrealize().
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  exec.c                      | 53
> > ++++++++++++++++++++++++++++-----------------
> > include/qom/cpu.h           |  2 ++ qom/cpu.c                   |  7
> > ++++++ target-ppc/cpu-qom.h        |  2 ++
> >  target-ppc/translate_init.c |  3 +++
> >  5 files changed, 47 insertions(+), 20 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index 0122ef7..fb73910 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState
> > *cpu, int asidx) /* Return the AddressSpace corresponding to the
> > specified index */ return cpu->cpu_ases[asidx].as;
> >  }
> > -#endif
> >  
> > -#ifndef CONFIG_USER_ONLY
> >  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> >  
> >  static int cpu_get_free_index(Error **errp)
> > @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
> >  {
> >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> >  }
> > +
> > +void cpu_vmstate_register(CPUState *cpu)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +
> > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > +        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common,
> > cpu);
> > +    }
> > +    if (cc->vmsd != NULL) {
> > +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > +    }
> > +}
> > +
> > +void cpu_vmstate_unregister(CPUState *cpu)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +
> > +    if (cc->vmsd != NULL) {
> > +        vmstate_unregister(NULL, cc->vmsd, cpu);
> > +    }
> > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> > +    }
> > +}
> 
> Is there any reason to keep these in exec.c,
> I'd put them in qom/cpu.c

I started with it, had to move vmstate_cpu_common and its pre/post_load
routines and one of them called tlb_flush() whose header qom/cpu.c
didn't like. So I saved the movement for later.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v1 2/5] cpu: Introduce CPUState::migration_id

[Bharata B Rao]

On Wed, Jul 06, 2016 at 01:34:59PM +0200, Igor Mammedov wrote:
> On Wed,  6 Jul 2016 14:29:18 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Add CPUState::migration_id and use that as instance_id in
> > vmstate_register() call.
> > 
> > Introduce use-migration-id property that allows target machines to
> > optionally switch to using migration_id instead of cpu_index.
> > This will help allow successful migration in cases where holes are
> > introduced in cpu_index range after CPU hot removals.
> > 
> > The default implementation of CPUClass:get_migration_id() continues
> > to return cpu_index.
> > 
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  exec.c            |  5 +++--
> >  include/qom/cpu.h |  6 ++++++
> >  qom/cpu.c         | 21 +++++++++++++++++++++
> >  3 files changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index fb73910..a8afeda 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -619,12 +619,13 @@ static void cpu_release_index(CPUState *cpu)
> >  void cpu_vmstate_register(CPUState *cpu)
> >  {
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > +    int instance_id = cc->get_migration_id(cpu);
> >  
> >      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common,
> > cpu);
> > +        vmstate_register(NULL, instance_id, &vmstate_cpu_common,
> > cpu); }
> >      if (cc->vmsd != NULL) {
> > -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > +        vmstate_register(NULL, instance_id, cc->vmsd, cpu);
> >      }
> >  }
> >  
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 29ccf5c..6d00143 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -187,6 +187,7 @@ typedef struct CPUClass {
> >      bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
> >  
> >      void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
> > +    int (*get_migration_id)(CPUState *cpu);
> >  } CPUClass;
> >  
> >  #ifdef HOST_WORDS_BIGENDIAN
> > @@ -273,6 +274,9 @@ struct qemu_work_item {
> >   * @kvm_fd: vCPU file descriptor for KVM.
> >   * @work_mutex: Lock to prevent multiple access to queued_work_*.
> >   * @queued_work_first: First asynchronous work pending.
> > + * @migration_id: Use as instance_id argument in cpu
> > vmstate_register calls
> with this field get_migration_id() callback is redundant.
> You can drop get_migration_id() and assign value to migration_id in
> machine specific code /spapr/ppc/x86/.../ before realize is called.
> That should simplify this patch and its users as well.

Let me give that a try and see how it looks.

> 
> > + * @use_migration_id: Set to enforce the use of
> > CPUClass.get_migration_id()
> > + *     over cpu_index during vmstate registration.
> >   *
> >   * State of one CPU core or thread.
> >   */
> > @@ -360,6 +364,8 @@ struct CPUState {
> >         (absolute value) offset as small as possible.  This reduces
> > code size, especially for hosts without large memory offsets.  */
> >      uint32_t tcg_exit_req;
> > +    int migration_id;
> > +    bool use_migration_id;
> >  };
> >  
> >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > diff --git a/qom/cpu.c b/qom/cpu.c
> > index 488ecc6..01cf136 100644
> > --- a/qom/cpu.c
> > +++ b/qom/cpu.c
> > @@ -326,6 +326,17 @@ static void cpu_common_realizefn(DeviceState
> > *dev, Error **errp) }
> >  }
> >  
> > +static bool cpu_common_get_use_migration_id(Object *obj, Error
> > **errp) +{
> > +    return CPU(obj)->use_migration_id;
> > +}
> > +
> > +static void cpu_common_set_use_migration_id(Object *obj, bool value,
> > +                                            Error **err)
> > +{
> > +    CPU(obj)->use_migration_id = value;
> > +}
> It's possible to create dumb property in a more compact way,
> see DEFINE_PROP_BOOL()

I stated with it and defined dc->props, but then felt it is a bit too
much to define a list just for one property. But if that preferable
let me switch back to it.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[Bharata B Rao]

On Wed, Jul 06, 2016 at 02:01:14PM +0200, Igor Mammedov wrote:
> On Wed,  6 Jul 2016 14:29:19 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > cpu_index is used as migration_id by default. For machine type
> > versions that set use-migration-id property, cpu_dt_it is returned.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  target-ppc/translate_init.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index efd6b88..9ca2f5e 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
> >  #endif
> >  }
> >  
> > +static int ppc_cpu_get_migration_id(CPUState *cs)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > +    if (cs->use_migration_id) {
> > +        return (int) cpu->cpu_dt_id;
> Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe
> to do so, that's the reason why I'm going to use index in possible_cpus
> on ARM and for the sake of uniformity do the same for x86 (even though
> it's possible to use 32bit APIC ID).

For the existing max_cpus limit, 32 bit should be fine, but obviously
that is not good and future safe.

I had a brief look at the implementation around possible_cpus in
x86, let me see if I can do something similar for generating stable
ids for PowerPC. I thought device tree ID would be our stable id, but
that being 64 bit and migration code requiring 32 bit value isn't
helping :(

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v1 5/5] cpu, spapr: Use migration_id from pseries-2.7 onwards

[Bharata B Rao]

On Wed, Jul 06, 2016 at 01:45:54PM +0200, Igor Mammedov wrote:
> On Wed,  6 Jul 2016 14:29:21 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Turn on use-migration-id property. Starting from pseries-2.7, prefer
> > the use of migration_id (cpu_dt_id) over cpu_index for cpu vmstate
> > registration and in XICS code.
> > 
> > This allows migration to work when CPU cores are not necessarily
> > unplugged in LIFO order.
> > 
> > TODO: Property use-migration-id is by default turned on, check
> > if this needs to turned off for older machine type versions of
> > all archs.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c | 5 +++++
> >  qom/cpu.c      | 1 +
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7f33a1b..be9af10 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2501,6 +2501,11 @@ DEFINE_SPAPR_MACHINE(2_7, "2.7", true);
> >          .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
> >          .property = "ddw",\
> >          .value    = stringify(off),\
> > +    }, \
> > +    { \
> > +        .driver   = TYPE_CPU,\
> > +        .property = "use-migration-id",\
> > +        .value    = stringify(off),\
> nit,
>  why not to use "off" directly, stringify looks odd here.

Was just following the previous property "ddw" did, will check if
stringify can be dropped.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[Greg Kurz]

On Wed,  6 Jul 2016 14:29:19 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> cpu_index is used as migration_id by default. For machine type versions
> that set use-migration-id property, cpu_dt_it is returned.
> 

It looks wrong to hijack cpu_dt_id to fix migration.

For pseries-2.7, you need to pass the sum of the index of the core in
spapr->cores and the index of the thread in sc->threads.

All other machines just need to pass the index of the cpu in their
respective arrays, which happens to be equal to cs->cpu_index.

> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  target-ppc/translate_init.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index efd6b88..9ca2f5e 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
>  #endif
>  }
>  
> +static int ppc_cpu_get_migration_id(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    if (cs->use_migration_id) {
> +        return (int) cpu->cpu_dt_id;
> +    } else {
> +        return cs->cpu_index;
> +    }
> +}
> +
>  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  #ifndef CONFIG_USER_ONLY
>      cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
>  #endif
> +    cc->get_migration_id = ppc_cpu_get_migration_id;
>  
>      dc->fw_name = "PowerPC,UNKNOWN";
>  }

Re: [Qemu-devel] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[Greg Kurz]

On Wed, 6 Jul 2016 19:51:01 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Wed, Jul 06, 2016 at 02:01:14PM +0200, Igor Mammedov wrote:
> > On Wed,  6 Jul 2016 14:29:19 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > cpu_index is used as migration_id by default. For machine type
> > > versions that set use-migration-id property, cpu_dt_it is returned.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  target-ppc/translate_init.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > > index efd6b88..9ca2f5e 100644
> > > --- a/target-ppc/translate_init.c
> > > +++ b/target-ppc/translate_init.c
> > > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
> > >  #endif
> > >  }
> > >  
> > > +static int ppc_cpu_get_migration_id(CPUState *cs)
> > > +{
> > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > +
> > > +    if (cs->use_migration_id) {
> > > +        return (int) cpu->cpu_dt_id;  
> > Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe
> > to do so, that's the reason why I'm going to use index in possible_cpus
> > on ARM and for the sake of uniformity do the same for x86 (even though
> > it's possible to use 32bit APIC ID).  
> 
> For the existing max_cpus limit, 32 bit should be fine, but obviously
> that is not good and future safe.
> 

And BTW, cpu_dt_id is actually an int

> I had a brief look at the implementation around possible_cpus in
> x86, let me see if I can do something similar for generating stable
> ids for PowerPC. I thought device tree ID would be our stable id, but
> that being 64 bit and migration code requiring 32 bit value isn't
> helping :(
> 

Yes you have stable ids, look at my other answer to this patch.

> Regards,
> Bharata.
> 

Re: [Qemu-devel] [RFC PATCH v1 1/5] cpu, target-ppc: Move cpu_vmstate_[un]register calls to cpu_common_[un]realize

[Igor Mammedov]

On Wed, 6 Jul 2016 19:46:13 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Wed, Jul 06, 2016 at 12:57:49PM +0200, Igor Mammedov wrote:
> > On Wed,  6 Jul 2016 14:29:17 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > Move vmstate_register() call to cpu_common_realize().
> > > Introduce cpu_common_unrealize() and move vmstate_unregister() to
> > > it.
> > > 
> > > Change those archs that implement their own CPU unrealize routine
> > > to mandatorily call CPUClass::unrealize().
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  exec.c                      | 53
> > > ++++++++++++++++++++++++++++-----------------
> > > include/qom/cpu.h           |  2 ++ qom/cpu.c
> > > |  7 ++++++ target-ppc/cpu-qom.h        |  2 ++
> > >  target-ppc/translate_init.c |  3 +++
> > >  5 files changed, 47 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/exec.c b/exec.c
> > > index 0122ef7..fb73910 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState
> > > *cpu, int asidx) /* Return the AddressSpace corresponding to the
> > > specified index */ return cpu->cpu_ases[asidx].as;
> > >  }
> > > -#endif
> > >  
> > > -#ifndef CONFIG_USER_ONLY
> > >  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> > >  
> > >  static int cpu_get_free_index(Error **errp)
> > > @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
> > >  {
> > >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > >  }
> > > +
> > > +void cpu_vmstate_register(CPUState *cpu)
> > > +{
> > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > +
> > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > +        vmstate_register(NULL, cpu->cpu_index,
> > > &vmstate_cpu_common, cpu);
> > > +    }
> > > +    if (cc->vmsd != NULL) {
> > > +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > > +    }
> > > +}
> > > +
> > > +void cpu_vmstate_unregister(CPUState *cpu)
> > > +{
> > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > +
> > > +    if (cc->vmsd != NULL) {
> > > +        vmstate_unregister(NULL, cc->vmsd, cpu);
> > > +    }
> > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> > > +    }
> > > +}
> > 
> > Is there any reason to keep these in exec.c,
> > I'd put them in qom/cpu.c
> 
> I started with it, had to move vmstate_cpu_common and its
> pre/post_load routines and one of them called tlb_flush() whose
> header qom/cpu.c didn't like. So I saved the movement for later.
ok,

alternatively you can check for use_migration_id in exec.c without
moving vmstate_* around.

> 
> Regards,
> Bharata.
> 

Re: [Qemu-devel] [RFC PATCH v1 2/5] cpu: Introduce CPUState::migration_id

[Igor Mammedov]

On Wed, 6 Jul 2016 19:48:03 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Wed, Jul 06, 2016 at 01:34:59PM +0200, Igor Mammedov wrote:
> > On Wed,  6 Jul 2016 14:29:18 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > Add CPUState::migration_id and use that as instance_id in
> > > vmstate_register() call.
> > > 
> > > Introduce use-migration-id property that allows target machines to
> > > optionally switch to using migration_id instead of cpu_index.
> > > This will help allow successful migration in cases where holes are
> > > introduced in cpu_index range after CPU hot removals.
> > > 
> > > The default implementation of CPUClass:get_migration_id()
> > > continues to return cpu_index.
> > > 
> > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  exec.c            |  5 +++--
> > >  include/qom/cpu.h |  6 ++++++
> > >  qom/cpu.c         | 21 +++++++++++++++++++++
> > >  3 files changed, 30 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/exec.c b/exec.c
> > > index fb73910..a8afeda 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -619,12 +619,13 @@ static void cpu_release_index(CPUState *cpu)
> > >  void cpu_vmstate_register(CPUState *cpu)
> > >  {
> > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > > +    int instance_id = cc->get_migration_id(cpu);
> > >  
> > >      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > -        vmstate_register(NULL, cpu->cpu_index,
> > > &vmstate_cpu_common, cpu);
> > > +        vmstate_register(NULL, instance_id, &vmstate_cpu_common,
> > > cpu); }
> > >      if (cc->vmsd != NULL) {
> > > -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > > +        vmstate_register(NULL, instance_id, cc->vmsd, cpu);
> > >      }
> > >  }
> > >  
> > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > > index 29ccf5c..6d00143 100644
> > > --- a/include/qom/cpu.h
> > > +++ b/include/qom/cpu.h
> > > @@ -187,6 +187,7 @@ typedef struct CPUClass {
> > >      bool (*cpu_exec_interrupt)(CPUState *cpu, int
> > > interrupt_request); 
> > >      void (*disas_set_info)(CPUState *cpu, disassemble_info
> > > *info);
> > > +    int (*get_migration_id)(CPUState *cpu);
> > >  } CPUClass;
> > >  
> > >  #ifdef HOST_WORDS_BIGENDIAN
> > > @@ -273,6 +274,9 @@ struct qemu_work_item {
> > >   * @kvm_fd: vCPU file descriptor for KVM.
> > >   * @work_mutex: Lock to prevent multiple access to queued_work_*.
> > >   * @queued_work_first: First asynchronous work pending.
> > > + * @migration_id: Use as instance_id argument in cpu
> > > vmstate_register calls
> > with this field get_migration_id() callback is redundant.
> > You can drop get_migration_id() and assign value to migration_id in
> > machine specific code /spapr/ppc/x86/.../ before realize is called.
> > That should simplify this patch and its users as well.
> 
> Let me give that a try and see how it looks.
> 
> > 
> > > + * @use_migration_id: Set to enforce the use of
> > > CPUClass.get_migration_id()
> > > + *     over cpu_index during vmstate registration.
> > >   *
> > >   * State of one CPU core or thread.
> > >   */
> > > @@ -360,6 +364,8 @@ struct CPUState {
> > >         (absolute value) offset as small as possible.  This
> > > reduces code size, especially for hosts without large memory
> > > offsets.  */ uint32_t tcg_exit_req;
> > > +    int migration_id;
> > > +    bool use_migration_id;
> > >  };
> > >  
> > >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > index 488ecc6..01cf136 100644
> > > --- a/qom/cpu.c
> > > +++ b/qom/cpu.c
> > > @@ -326,6 +326,17 @@ static void cpu_common_realizefn(DeviceState
> > > *dev, Error **errp) }
> > >  }
> > >  
> > > +static bool cpu_common_get_use_migration_id(Object *obj, Error
> > > **errp) +{
> > > +    return CPU(obj)->use_migration_id;
> > > +}
> > > +
> > > +static void cpu_common_set_use_migration_id(Object *obj, bool
> > > value,
> > > +                                            Error **err)
> > > +{
> > > +    CPU(obj)->use_migration_id = value;
> > > +}
> > It's possible to create dumb property in a more compact way,
> > see DEFINE_PROP_BOOL()
> 
> I stated with it and defined dc->props, but then felt it is a bit too
> much to define a list just for one property. But if that preferable
> let me switch back to it.
static prop is less code to maintain and not scattered all around file,
so it's more preferred if it can do the job.

> 
> Regards,
> Bharata.
> 
> 

Re: [Qemu-devel] [RFC PATCH v1 1/5] cpu, target-ppc: Move cpu_vmstate_[un]register calls to cpu_common_[un]realize

[Bharata B Rao]

On Wed, Jul 06, 2016 at 04:44:17PM +0200, Igor Mammedov wrote:
> On Wed, 6 Jul 2016 19:46:13 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Jul 06, 2016 at 12:57:49PM +0200, Igor Mammedov wrote:
> > > On Wed,  6 Jul 2016 14:29:17 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > 
> > > > Move vmstate_register() call to cpu_common_realize().
> > > > Introduce cpu_common_unrealize() and move vmstate_unregister() to
> > > > it.
> > > > 
> > > > Change those archs that implement their own CPU unrealize routine
> > > > to mandatorily call CPUClass::unrealize().
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  exec.c                      | 53
> > > > ++++++++++++++++++++++++++++-----------------
> > > > include/qom/cpu.h           |  2 ++ qom/cpu.c
> > > > |  7 ++++++ target-ppc/cpu-qom.h        |  2 ++
> > > >  target-ppc/translate_init.c |  3 +++
> > > >  5 files changed, 47 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/exec.c b/exec.c
> > > > index 0122ef7..fb73910 100644
> > > > --- a/exec.c
> > > > +++ b/exec.c
> > > > @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState
> > > > *cpu, int asidx) /* Return the AddressSpace corresponding to the
> > > > specified index */ return cpu->cpu_ases[asidx].as;
> > > >  }
> > > > -#endif
> > > >  
> > > > -#ifndef CONFIG_USER_ONLY
> > > >  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> > > >  
> > > >  static int cpu_get_free_index(Error **errp)
> > > > @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
> > > >  {
> > > >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > > >  }
> > > > +
> > > > +void cpu_vmstate_register(CPUState *cpu)
> > > > +{
> > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > +
> > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > +        vmstate_register(NULL, cpu->cpu_index,
> > > > &vmstate_cpu_common, cpu);
> > > > +    }
> > > > +    if (cc->vmsd != NULL) {
> > > > +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > > > +    }
> > > > +}
> > > > +
> > > > +void cpu_vmstate_unregister(CPUState *cpu)
> > > > +{
> > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > +
> > > > +    if (cc->vmsd != NULL) {
> > > > +        vmstate_unregister(NULL, cc->vmsd, cpu);
> > > > +    }
> > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> > > > +    }
> > > > +}
> > > 
> > > Is there any reason to keep these in exec.c,
> > > I'd put them in qom/cpu.c
> > 
> > I started with it, had to move vmstate_cpu_common and its
> > pre/post_load routines and one of them called tlb_flush() whose
> > header qom/cpu.c didn't like. So I saved the movement for later.
> ok,
> 
> alternatively you can check for use_migration_id in exec.c without
> moving vmstate_* around.

Ok, just to be clear, you are suggesting that I move back to my
first implementation which just consolidated vmstate_* calls
and called them from within cpu_exec_init/exit() ?

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[Bharata B Rao]

On Wed, Jul 06, 2016 at 04:35:28PM +0200, Greg Kurz wrote:
> On Wed,  6 Jul 2016 14:29:19 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > cpu_index is used as migration_id by default. For machine type versions
> > that set use-migration-id property, cpu_dt_it is returned.
> > 
> 
> It looks wrong to hijack cpu_dt_id to fix migration.
> 
> For pseries-2.7, you need to pass the sum of the index of the core in
> spapr->cores and the index of the thread in sc->threads.
> 
> All other machines just need to pass the index of the cpu in their
> respective arrays, which happens to be equal to cs->cpu_index.

Yes, that is indeed good and simplifies things. Thanks for noting this.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v1 1/5] cpu, target-ppc: Move cpu_vmstate_[un]register calls to cpu_common_[un]realize

[David Gibson]

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

On Wed, Jul 06, 2016 at 02:29:17PM +0530, Bharata B Rao wrote:
> Move vmstate_register() call to cpu_common_realize().
> Introduce cpu_common_unrealize() and move vmstate_unregister() to it.
> 
> Change those archs that implement their own CPU unrealize routine to
> mandatorily call CPUClass::unrealize().
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

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

> ---
>  exec.c                      | 53 ++++++++++++++++++++++++++++-----------------
>  include/qom/cpu.h           |  2 ++
>  qom/cpu.c                   |  7 ++++++
>  target-ppc/cpu-qom.h        |  2 ++
>  target-ppc/translate_init.c |  3 +++
>  5 files changed, 47 insertions(+), 20 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 0122ef7..fb73910 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
>      /* Return the AddressSpace corresponding to the specified index */
>      return cpu->cpu_ases[asidx].as;
>  }
> -#endif
>  
> -#ifndef CONFIG_USER_ONLY
>  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
>  
>  static int cpu_get_free_index(Error **errp)
> @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
>  {
>      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
>  }
> +
> +void cpu_vmstate_register(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> +        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> +    }
> +    if (cc->vmsd != NULL) {
> +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +    }
> +}
> +
> +void cpu_vmstate_unregister(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->vmsd != NULL) {
> +        vmstate_unregister(NULL, cc->vmsd, cpu);
> +    }
> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> +    }
> +}
> +
>  #else
>  
>  static int cpu_get_free_index(Error **errp)
> @@ -634,12 +657,18 @@ static void cpu_release_index(CPUState *cpu)
>  {
>      return;
>  }
> +
> +void cpu_vmstate_register(CPUState *cpu)
> +{
> +}
> +
> +void cpu_vmstate_unregister(CPUState *cpu)
> +{
> +}
>  #endif
>  
>  void cpu_exec_exit(CPUState *cpu)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> -
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
>  #endif
> @@ -657,18 +686,10 @@ void cpu_exec_exit(CPUState *cpu)
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
>  #endif
> -
> -    if (cc->vmsd != NULL) {
> -        vmstate_unregister(NULL, cc->vmsd, cpu);
> -    }
> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> -    }
>  }
>  
>  void cpu_exec_init(CPUState *cpu, Error **errp)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
>      Error *local_err = NULL;
>  
>      cpu->as = NULL;
> @@ -705,15 +726,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      }
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>  #if defined(CONFIG_USER_ONLY)
> -    (void) cc;
>      cpu_list_unlock();
> -#else
> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> -    }
> -    if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> -    }
>  #endif
>  }
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 32f3af3..29ccf5c 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -870,4 +870,6 @@ extern const struct VMStateDescription vmstate_cpu_common;
>      .offset = 0,                                                            \
>  }
>  
> +void cpu_vmstate_register(CPUState *cpu);
> +void cpu_vmstate_unregister(CPUState *cpu);
>  #endif
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 751e992..488ecc6 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -310,10 +310,16 @@ static void cpu_common_parse_features(CPUState *cpu, char *features,
>      }
>  }
>  
> +static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
> +{
> +    cpu_vmstate_unregister(CPU(dev));
> +}
> +
>  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cpu = CPU(dev);
>  
> +    cpu_vmstate_register(cpu);
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(cpu);
>          cpu_resume(cpu);
> @@ -367,6 +373,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>      k->cpu_exec_exit = cpu_common_noop;
>      k->cpu_exec_interrupt = cpu_common_exec_interrupt;
>      dc->realize = cpu_common_realizefn;
> +    dc->unrealize = cpu_common_unrealizefn;
>      /*
>       * Reason: CPUs still need special care by board code: wiring up
>       * IRQs, adding reset handlers, halting non-first CPUs, ...
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index 2864105..6ec2fca 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -163,6 +163,7 @@ struct ppc_segment_page_sizes;
>  /**
>   * PowerPCCPUClass:
>   * @parent_realize: The parent class' realize handler.
> + * @parent_unrealize: The parent class' unrealize handler.
>   * @parent_reset: The parent class' reset handler.
>   *
>   * A PowerPC CPU model.
> @@ -173,6 +174,7 @@ typedef struct PowerPCCPUClass {
>      /*< public >*/
>  
>      DeviceRealize parent_realize;
> +    DeviceUnrealize parent_unrealize;
>      void (*parent_reset)(CPUState *cpu);
>  
>      uint32_t pvr;
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 8f257fb..efd6b88 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9773,10 +9773,12 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>  static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(dev);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
>      opc_handler_t **table;
>      int i, j;
>  
> +    pcc->parent_unrealize(dev, errp);
>      cpu_exec_exit(CPU(dev));
>  
>      for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
> @@ -10364,6 +10366,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      pcc->parent_realize = dc->realize;
> +    pcc->parent_unrealize = dc->unrealize;
>      pcc->pvr_match = ppc_pvr_match_default;
>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
>      dc->realize = ppc_cpu_realizefn;

-- 
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] [RFC PATCH v1 2/5] cpu: Introduce CPUState::migration_id

[David Gibson]

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

On Wed, Jul 06, 2016 at 02:29:18PM +0530, Bharata B Rao wrote:
> Add CPUState::migration_id and use that as instance_id in
> vmstate_register() call.
> 
> Introduce use-migration-id property that allows target machines to
> optionally switch to using migration_id instead of cpu_index.
> This will help allow successful migration in cases where holes are
> introduced in cpu_index range after CPU hot removals.
> 
> The default implementation of CPUClass:get_migration_id() continues
> to return cpu_index.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  exec.c            |  5 +++--
>  include/qom/cpu.h |  6 ++++++
>  qom/cpu.c         | 21 +++++++++++++++++++++
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index fb73910..a8afeda 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -619,12 +619,13 @@ static void cpu_release_index(CPUState *cpu)
>  void cpu_vmstate_register(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int instance_id = cc->get_migration_id(cpu);
>  
>      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> +        vmstate_register(NULL, instance_id, &vmstate_cpu_common, cpu);
>      }
>      if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +        vmstate_register(NULL, instance_id, cc->vmsd, cpu);
>      }
>  }
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 29ccf5c..6d00143 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -187,6 +187,7 @@ typedef struct CPUClass {
>      bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
>  
>      void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
> +    int (*get_migration_id)(CPUState *cpu);
>  } CPUClass;
>  
>  #ifdef HOST_WORDS_BIGENDIAN
> @@ -273,6 +274,9 @@ struct qemu_work_item {
>   * @kvm_fd: vCPU file descriptor for KVM.
>   * @work_mutex: Lock to prevent multiple access to queued_work_*.
>   * @queued_work_first: First asynchronous work pending.
> + * @migration_id: Use as instance_id argument in cpu vmstate_register calls
> + * @use_migration_id: Set to enforce the use of CPUClass.get_migration_id()
> + *     over cpu_index during vmstate registration.

I don't really understand the purpose of the use-migration-id bool.
Can't you just leave the migration-id as cpu_index by default if you
don't want a different migration id.  AFAICT this flag is never
tested.

-- 
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] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[David Gibson]

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

On Wed, Jul 06, 2016 at 02:01:14PM +0200, Igor Mammedov wrote:
> On Wed,  6 Jul 2016 14:29:19 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > cpu_index is used as migration_id by default. For machine type
> > versions that set use-migration-id property, cpu_dt_it is returned.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  target-ppc/translate_init.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index efd6b88..9ca2f5e 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
> >  #endif
> >  }
> >  
> > +static int ppc_cpu_get_migration_id(CPUState *cs)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > +    if (cs->use_migration_id) {
> > +        return (int) cpu->cpu_dt_id;
> Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe
> to do so, that's the reason why I'm going to use index in possible_cpus
> on ARM and for the sake of uniformity do the same for x86 (even though
> it's possible to use 32bit APIC ID).

No, I'm pretty sure the DT id fits in a 32-bit field in the device
tree itself, so it should be safe.

> 
> > +    } else {
> > +        return cs->cpu_index;
> > +    }
> > +}
> > +
> >  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
> >  {
> >      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> > @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass
> > *oc, void *data) #ifndef CONFIG_USER_ONLY
> >      cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
> >  #endif
> > +    cc->get_migration_id = ppc_cpu_get_migration_id;
> >  
> >      dc->fw_name = "PowerPC,UNKNOWN";
> >  }
> 

-- 
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] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[David Gibson]

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

On Wed, Jul 06, 2016 at 07:51:01PM +0530, Bharata B Rao wrote:
> On Wed, Jul 06, 2016 at 02:01:14PM +0200, Igor Mammedov wrote:
> > On Wed,  6 Jul 2016 14:29:19 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > cpu_index is used as migration_id by default. For machine type
> > > versions that set use-migration-id property, cpu_dt_it is returned.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  target-ppc/translate_init.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > > index efd6b88..9ca2f5e 100644
> > > --- a/target-ppc/translate_init.c
> > > +++ b/target-ppc/translate_init.c
> > > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
> > >  #endif
> > >  }
> > >  
> > > +static int ppc_cpu_get_migration_id(CPUState *cs)
> > > +{
> > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > +
> > > +    if (cs->use_migration_id) {
> > > +        return (int) cpu->cpu_dt_id;
> > Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe
> > to do so, that's the reason why I'm going to use index in possible_cpus
> > on ARM and for the sake of uniformity do the same for x86 (even though
> > it's possible to use 32bit APIC ID).
> 
> For the existing max_cpus limit, 32 bit should be fine, but obviously
> that is not good and future safe.

I think we should be OK.  The hardware IDs these are modelled on are
32-bit - I'm pretty sure #address-cells in /cpus is 1, not 2.

And even with a lot of wastage for alignment gaps of various sorts,
we're a *long* way off from supporting 4 billion cpus...

> I had a brief look at the implementation around possible_cpus in
> x86, let me see if I can do something similar for generating stable
> ids for PowerPC. I thought device tree ID would be our stable id, but
> that being 64 bit and migration code requiring 32 bit value isn't
> helping :(
> 
> Regards,
> Bharata.
> 

-- 
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] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[David Gibson]

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

On Wed, Jul 06, 2016 at 02:29:19PM +0530, Bharata B Rao wrote:
> cpu_index is used as migration_id by default. For machine type versions
> that set use-migration-id property, cpu_dt_it is returned.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

This seems ceonceptually wrong.  The logic to determine the stable ID
is still coming from the CPU code.  But kind of the point here is that
in most cases the stable IDs really belong to the machine type, not
the CPU.

Would it make more sense to just have migration-id be a settable
property, which the machine type needs to set between init and
realize?  (or leave as default == cpu_index).

That way we could have machine version dependent differences without
this awkward use_migration_id flag (after all, if it's not always used
as the id for migration, 'migration_id' is kind of a bad name...).

> ---
>  target-ppc/translate_init.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index efd6b88..9ca2f5e 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
>  #endif
>  }
>  
> +static int ppc_cpu_get_migration_id(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    if (cs->use_migration_id) {
> +        return (int) cpu->cpu_dt_id;
> +    } else {
> +        return cs->cpu_index;
> +    }
> +}
> +
>  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  #ifndef CONFIG_USER_ONLY
>      cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
>  #endif
> +    cc->get_migration_id = ppc_cpu_get_migration_id;
>  
>      dc->fw_name = "PowerPC,UNKNOWN";
>  }

-- 
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] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[Greg Kurz]

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

On Thu, 7 Jul 2016 10:57:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 06, 2016 at 07:51:01PM +0530, Bharata B Rao wrote:
> > On Wed, Jul 06, 2016 at 02:01:14PM +0200, Igor Mammedov wrote:  
> > > On Wed,  6 Jul 2016 14:29:19 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >   
> > > > cpu_index is used as migration_id by default. For machine type
> > > > versions that set use-migration-id property, cpu_dt_it is returned.
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  target-ppc/translate_init.c | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > > > index efd6b88..9ca2f5e 100644
> > > > --- a/target-ppc/translate_init.c
> > > > +++ b/target-ppc/translate_init.c
> > > > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
> > > >  #endif
> > > >  }
> > > >  
> > > > +static int ppc_cpu_get_migration_id(CPUState *cs)
> > > > +{
> > > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > > +
> > > > +    if (cs->use_migration_id) {
> > > > +        return (int) cpu->cpu_dt_id;  
> > > Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe
> > > to do so, that's the reason why I'm going to use index in possible_cpus
> > > on ARM and for the sake of uniformity do the same for x86 (even though
> > > it's possible to use 32bit APIC ID).  
> > 
> > For the existing max_cpus limit, 32 bit should be fine, but obviously
> > that is not good and future safe.  
> 
> I think we should be OK.  The hardware IDs these are modelled on are
> 32-bit - I'm pretty sure #address-cells in /cpus is 1, not 2.
> 
> And even with a lot of wastage for alignment gaps of various sorts,
> we're a *long* way off from supporting 4 billion cpus...
> 

Indeed ! Even in the "worst" case, which is running a guest with 1 thread
per core on a POWER8 host with full cores (8 HW threads), the current maximum
value for cpu_dt_id is 254 * 8 == 2032 :)

> > I had a brief look at the implementation around possible_cpus in
> > x86, let me see if I can do something similar for generating stable
> > ids for PowerPC. I thought device tree ID would be our stable id, but
> > that being 64 bit and migration code requiring 32 bit value isn't
> > helping :(
> > 
> > Regards,
> > Bharata.
> >   
> 


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

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v1 3/5] spapr: Implement CPUClass::get_migration_id() for PowerPC CPUs

[Mark Cave-Ayland]

On 07/07/16 02:00, David Gibson wrote:

> On Wed, Jul 06, 2016 at 02:29:19PM +0530, Bharata B Rao wrote:
>> cpu_index is used as migration_id by default. For machine type versions
>> that set use-migration-id property, cpu_dt_it is returned.
>>
>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> This seems ceonceptually wrong.  The logic to determine the stable ID
> is still coming from the CPU code.  But kind of the point here is that
> in most cases the stable IDs really belong to the machine type, not
> the CPU.
> 
> Would it make more sense to just have migration-id be a settable
> property, which the machine type needs to set between init and
> realize?  (or leave as default == cpu_index).
> 
> That way we could have machine version dependent differences without
> this awkward use_migration_id flag (after all, if it's not always used
> as the id for migration, 'migration_id' is kind of a bad name...).
> 
>> ---
>>  target-ppc/translate_init.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index efd6b88..9ca2f5e 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
>>  #endif
>>  }
>>  
>> +static int ppc_cpu_get_migration_id(CPUState *cs)
>> +{
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +
>> +    if (cs->use_migration_id) {
>> +        return (int) cpu->cpu_dt_id;
>> +    } else {
>> +        return cs->cpu_index;
>> +    }
>> +}
>> +
>>  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>>  {
>>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>> @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>>  #ifndef CONFIG_USER_ONLY
>>      cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
>>  #endif
>> +    cc->get_migration_id = ppc_cpu_get_migration_id;
>>  
>>      dc->fw_name = "PowerPC,UNKNOWN";
>>  }
> 

Could this be another potential user for the PPCMachineClass solution I
was looking at for timebase migration in
https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg01208.html? It
seems like a similar problem where machine-level state is needed for
migration purposes.


ATB,

Mark.