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

[Qemu-devel] [RFC PATCH v2 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.

- CPUState::stable_cpu_id is newly introduced and used as instance_id when
  registering CPU devices using vmstate_register. stable_cpu_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 stable_cpu_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 (which is stable_cpu_id for pseries-2.7 onwards)
based on core-id and hence is based on ppc-vcpu-dt-id-rework branch of his
tree.

Changes in v2
-------------
- s/migration_id/stable_cpu_id and this is set by the machine code
  after CPU init but before realize.
- s/use-migration-id/has-migration-id and use DEFINE_PROP_BOOL to simplify
  the code.
- Start with use-migration-id turned off by default.
- Consolidate the code that obtains the 'server' in XICS code into a
  separate routine.

v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg384135.html

Bharata B Rao (5):
  cpu,target-ppc: Move cpu_vmstate_[un]register calls to
    cpu_common_[un]realize
  cpu: Introduce CPUState::stable_cpu_id
  spapr: Set stable_cpu_id for threads of CPU cores
  xics: Use stable_cpu_id instead of cpu_index in XICS code
  spapr: Enable the use of stable_cpu_id from pseries-2.7 onwards

 exec.c                      | 55 ++++++++++++++++++++++++++++-----------------
 hw/intc/xics.c              | 21 +++++++++++++----
 hw/intc/xics_kvm.c          | 10 ++++-----
 hw/intc/xics_spapr.c        | 29 ++++++++++++++----------
 hw/ppc/spapr.c              | 14 ++++++++++++
 hw/ppc/spapr_cpu_core.c     |  7 ++++++
 include/hw/compat.h         |  3 +++
 include/hw/ppc/xics.h       |  1 +
 include/qom/cpu.h           |  7 ++++++
 qom/cpu.c                   | 13 +++++++++++
 target-ppc/cpu-qom.h        |  2 ++
 target-ppc/translate_init.c |  3 +++
 12 files changed, 123 insertions(+), 42 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 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().

TODO: Decide if we indeed  want to move vmstate_[un]register()
calls to cpu_common_[un]realize().

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 cacb100..331386f 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 a9727a1..1095ea1 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -325,10 +325,16 @@ static void cpu_common_parse_features(const char *typename, 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);
@@ -382,6 +388,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 976a38b..bc8b767 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9743,10 +9743,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++) {
@@ -10329,6 +10331,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 v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[Bharata B Rao]

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

Introduce has-stable_cpu_id property that allows target machines to
optionally switch to using stable_cpu_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.

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

diff --git a/exec.c b/exec.c
index fb73910..3b36fe5 100644
--- a/exec.c
+++ b/exec.c
@@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
 void cpu_vmstate_register(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
+    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
+                      cpu->cpu_index;
 
     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 331386f..527c021 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -273,6 +273,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.
+ * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
+ * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
+ *     over cpu_index during vmstate registration.
  *
  * State of one CPU core or thread.
  */
@@ -360,6 +363,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 stable_cpu_id;
+    bool has_stable_cpu_id;
 };
 
 QTAILQ_HEAD(CPUTailQ, CPUState);
diff --git a/qom/cpu.c b/qom/cpu.c
index 1095ea1..bae1bf7 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
     return cpu->cpu_index;
 }
 
+static Property cpu_common_properties[] = {
+    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void cpu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
      * IRQs, adding reset handlers, halting non-first CPUs, ...
      */
     dc->cannot_instantiate_with_device_add_yet = true;
+    dc->props = cpu_common_properties;
 }
 
 static const TypeInfo cpu_type_info = {
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[Bharata B Rao]

Conditonally set stable_cpu_id for CPU threads that are created as part
of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
onwards.

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

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index b104778..0ec3513 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < cc->nr_threads; i++) {
         char id[32];
         obj = sc->threads + i * size;
+        CPUState *cs;
 
         object_initialize(obj, size, typename);
+        cs = CPU(obj);
+
+        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */
+        if (cs->has_stable_cpu_id) {
+            cs->stable_cpu_id = cc->core_id + i;
+        }
         snprintf(id, sizeof(id), "thread[%d]", i);
         object_property_add_child(OBJECT(sc), id, obj, &local_err);
         if (local_err) {
-- 
2.7.4

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

[Bharata B Rao]

xics maintains an array of ICPState structures which is indexed
by cpu_index. Optionally change this to index the ICPState array by
stable_cpu_id. When the use of stable_cpu_id is enabled from pseries-2.7
onwards, this allows migration of guest to succeed when there are holes in
cpu_index range due to CPU core hot removal.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/intc/xics.c        | 21 +++++++++++++++++----
 hw/intc/xics_kvm.c    | 10 ++++------
 hw/intc/xics_spapr.c  | 29 +++++++++++++++++------------
 include/hw/ppc/xics.h |  1 +
 4 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index cd48f42..97ff3c5 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -36,6 +36,17 @@
 #include "qemu/error-report.h"
 #include "qapi/visitor.h"
 
+int xics_get_server(PowerPCCPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+
+    if (cs->has_stable_cpu_id) {
+        return cs->stable_cpu_id;
+    } else {
+        return cs->cpu_index;
+    }
+}
+
 int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
 {
     PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
@@ -50,9 +61,10 @@ 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];
+    int server = xics_get_server(cpu);
+    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 +75,11 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
-    ICPState *ss = &xics->ss[cs->cpu_index];
+    int server = xics_get_server(cpu);
+    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..f71b468 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -326,14 +326,12 @@ 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);
+    int server = xics_get_server(cpu);
+    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..5491f82 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -31,6 +31,7 @@
 #include "trace.h"
 #include "qemu/timer.h"
 #include "hw/ppc/spapr.h"
+#include "hw/ppc/spapr_cpu_core.h"
 #include "hw/ppc/xics.h"
 #include "qapi/visitor.h"
 #include "qapi/error.h"
@@ -42,17 +43,19 @@
 static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
-    CPUState *cs = CPU(cpu);
+    int server = xics_get_server(cpu);
     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->has_stable_cpu_id ? args[0] :
+                          xics_get_cpu_index_by_dt_id(args[0]);
     target_ulong mfrr = args[1];
 
     if (server >= spapr->xics->nr_servers) {
@@ -66,8 +69,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 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);
+    int server = xics_get_server(cpu);
+    uint32_t xirr = icp_accept(spapr->xics->ss + server);
 
     args[0] = xirr;
     return H_SUCCESS;
@@ -76,8 +79,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 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];
+    int server = xics_get_server(cpu);
+    ICPState *ss = &spapr->xics->ss[server];
     uint32_t xirr = icp_accept(ss);
 
     args[0] = xirr;
@@ -88,19 +91,19 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           target_ulong opcode, target_ulong *args)
 {
-    CPUState *cs = CPU(cpu);
+    int server = xics_get_server(cpu);
     target_ulong xirr = args[0];
 
-    icp_eoi(spapr->xics, cs->cpu_index, xirr);
+    icp_eoi(spapr->xics, server, xirr);
     return H_SUCCESS;
 }
 
 static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                             target_ulong opcode, target_ulong *args)
 {
-    CPUState *cs = CPU(cpu);
+    int server = xics_get_server(cpu);
     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 +116,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 +126,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->has_stable_cpu_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)
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 6189a3b..aea0678 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -195,5 +195,6 @@ void ics_write_xive(ICSState *ics, int nr, int server,
 void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
 
 int xics_find_source(XICSState *icp, int irq);
+int xics_get_server(PowerPCCPU *cpu);
 
 #endif /* __XICS_H__ */
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 5/5] spapr: Enable the use of stable_cpu_id from pseries-2.7 onwards

[Bharata B Rao]

Starting from pseries-2.7, turn on has-stable-cpu-id property that
switches to using stable_cpu_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.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c      | 14 ++++++++++++++
 include/hw/compat.h |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 89e61b9..4ca6072 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2495,6 +2495,14 @@ static const TypeInfo spapr_machine_info = {
 /*
  * pseries-2.7
  */
+#define SPAPR_COMPAT_2_7 \
+    HW_COMPAT_2_7 \
+    { \
+        .driver   = TYPE_CPU, \
+        .property = "has-stable-cpu-id", \
+        .value    = "on", \
+    },
+
 static void spapr_machine_2_7_instance_options(MachineState *machine)
 {
 }
@@ -2502,6 +2510,7 @@ static void spapr_machine_2_7_instance_options(MachineState *machine)
 static void spapr_machine_2_7_class_options(MachineClass *mc)
 {
     /* Defaults for the latest behaviour inherited from the base class */
+    SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_7);
 }
 
 DEFINE_SPAPR_MACHINE(2_7, "2.7", true);
@@ -2515,6 +2524,11 @@ DEFINE_SPAPR_MACHINE(2_7, "2.7", true);
         .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
         .property = "ddw",\
         .value    = stringify(off),\
+    }, \
+    { \
+        .driver   = TYPE_CPU, \
+        .property = "has-stable-cpu-id", \
+        .value    = "off", \
     },
 
 static void spapr_machine_2_6_instance_options(MachineState *machine)
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 636befe..8e639e2 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -1,6 +1,9 @@
 #ifndef HW_COMPAT_H
 #define HW_COMPAT_H
 
+#define HW_COMPAT_2_7 \
+    /* empty */
+
 #define HW_COMPAT_2_6 \
     /* empty */
 
-- 
2.7.4

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

[Greg Kurz]

On Thu,  7 Jul 2016 20:20:20 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> 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.
> 
> - CPUState::stable_cpu_id is newly introduced and used as instance_id when
>   registering CPU devices using vmstate_register. stable_cpu_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 stable_cpu_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 (which is stable_cpu_id for pseries-2.7 onwards)
> based on core-id and hence is based on ppc-vcpu-dt-id-rework branch of his
> tree.
> 

I'm not very comfortable with this. Shouldn't it be the other way round
actually: cpu_dt_id depending on stable_cpu_id ?

I think we're missing something like a stable_core_id.

I'll illustrate that with a comment on patch 3/5.

> Changes in v2
> -------------
> - s/migration_id/stable_cpu_id and this is set by the machine code
>   after CPU init but before realize.
> - s/use-migration-id/has-migration-id and use DEFINE_PROP_BOOL to simplify
>   the code.
> - Start with use-migration-id turned off by default.
> - Consolidate the code that obtains the 'server' in XICS code into a
>   separate routine.
> 
> v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg384135.html
> 
> Bharata B Rao (5):
>   cpu,target-ppc: Move cpu_vmstate_[un]register calls to
>     cpu_common_[un]realize
>   cpu: Introduce CPUState::stable_cpu_id
>   spapr: Set stable_cpu_id for threads of CPU cores
>   xics: Use stable_cpu_id instead of cpu_index in XICS code
>   spapr: Enable the use of stable_cpu_id from pseries-2.7 onwards
> 
>  exec.c                      | 55 ++++++++++++++++++++++++++++-----------------
>  hw/intc/xics.c              | 21 +++++++++++++----
>  hw/intc/xics_kvm.c          | 10 ++++-----
>  hw/intc/xics_spapr.c        | 29 ++++++++++++++----------
>  hw/ppc/spapr.c              | 14 ++++++++++++
>  hw/ppc/spapr_cpu_core.c     |  7 ++++++
>  include/hw/compat.h         |  3 +++
>  include/hw/ppc/xics.h       |  1 +
>  include/qom/cpu.h           |  7 ++++++
>  qom/cpu.c                   | 13 +++++++++++
>  target-ppc/cpu-qom.h        |  2 ++
>  target-ppc/translate_init.c |  3 +++
>  12 files changed, 123 insertions(+), 42 deletions(-)
> 

Re: [Qemu-devel] [RFC PATCH v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[Greg Kurz]

On Thu,  7 Jul 2016 20:20:23 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Conditonally set stable_cpu_id for CPU threads that are created as part
> of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> onwards.
> 

The last sentence is a bit confusing since the enablement actually happens
in patch 5/5.

> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_cpu_core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index b104778..0ec3513 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      for (i = 0; i < cc->nr_threads; i++) {
>          char id[32];
>          obj = sc->threads + i * size;
> +        CPUState *cs;
>  
>          object_initialize(obj, size, typename);
> +        cs = CPU(obj);
> +
> +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */

It isn't what I had in mind. More something like below:

In ppc_spapr_init():

    for (i = 0; i < spapr_max_cores; i++) {
        spapr->cores[i]->stable_id = i * smp_threads;
    }


In spapr_cpu_core_realize():

    for (j = 0; j < cc->nr_threads; j++) {
        stable_cpu_id = cc->stable_id + j;
    }

So we need to introduce cc->stable_id.

I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id.

Makes sense ?

> +        if (cs->has_stable_cpu_id) {
> +            cs->stable_cpu_id = cc->core_id + i;
> +        }
>          snprintf(id, sizeof(id), "thread[%d]", i);
>          object_property_add_child(OBJECT(sc), id, obj, &local_err);
>          if (local_err) {

Re: [Qemu-devel] [RFC PATCH v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[Greg Kurz]

On Thu,  7 Jul 2016 20:20:22 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Add CPUState::stable_cpu_id and use that as instance_id in
> vmstate_register() call.
> 
> Introduce has-stable_cpu_id property that allows target machines to
> optionally switch to using stable_cpu_id instead of cpu_index.

If stable_cpu_id is in the [0..max_vcpus) range, then it does not really depend
on the machine type or version. In this case, the property is not needed.

> This will help allow successful migration in cases where holes are
> introduced in cpu_index range after CPU hot removals.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  exec.c            | 6 ++++--
>  include/qom/cpu.h | 5 +++++
>  qom/cpu.c         | 6 ++++++
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index fb73910..3b36fe5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
>  void cpu_vmstate_register(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
> +                      cpu->cpu_index;
>  
>      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 331386f..527c021 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -273,6 +273,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.
> + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
> + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
> + *     over cpu_index during vmstate registration.
>   *
>   * State of one CPU core or thread.
>   */
> @@ -360,6 +363,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 stable_cpu_id;
> +    bool has_stable_cpu_id;
>  };
>  
>  QTAILQ_HEAD(CPUTailQ, CPUState);
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 1095ea1..bae1bf7 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
>      return cpu->cpu_index;
>  }
>  
> +static Property cpu_common_properties[] = {
> +    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
>  static void cpu_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>       * IRQs, adding reset handlers, halting non-first CPUs, ...
>       */
>      dc->cannot_instantiate_with_device_add_yet = true;
> +    dc->props = cpu_common_properties;
>  }
>  
>  static const TypeInfo cpu_type_info = {

Re: [Qemu-devel] [RFC PATCH v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[David Gibson]

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

On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote:
> Add CPUState::stable_cpu_id and use that as instance_id in
> vmstate_register() call.
> 
> Introduce has-stable_cpu_id property that allows target machines to
> optionally switch to using stable_cpu_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.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  exec.c            | 6 ++++--
>  include/qom/cpu.h | 5 +++++
>  qom/cpu.c         | 6 ++++++
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index fb73910..3b36fe5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
>  void cpu_vmstate_register(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
> +                      cpu->cpu_index;
>  
>      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 331386f..527c021 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -273,6 +273,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.
> + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
> + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
> + *     over cpu_index during vmstate registration.
>   *
>   * State of one CPU core or thread.
>   */
> @@ -360,6 +363,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 stable_cpu_id;
> +    bool has_stable_cpu_id;
>  };
>  
>  QTAILQ_HEAD(CPUTailQ, CPUState);
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 1095ea1..bae1bf7 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
>      return cpu->cpu_index;
>  }
>  
> +static Property cpu_common_properties[] = {
> +    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),

It seems odd to me that stable_cpu_id itself isn't exposed as a
property.  Even if we don't need to set it externally for now, it
really should be QOM introspectable.

> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
>  static void cpu_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>       * IRQs, adding reset handlers, halting non-first CPUs, ...
>       */
>      dc->cannot_instantiate_with_device_add_yet = true;
> +    dc->props = cpu_common_properties;
>  }
>  
>  static const TypeInfo cpu_type_info = {

-- 
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 v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[David Gibson]

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

On Thu, Jul 07, 2016 at 07:52:32PM +0200, Greg Kurz wrote:
> On Thu,  7 Jul 2016 20:20:22 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Add CPUState::stable_cpu_id and use that as instance_id in
> > vmstate_register() call.
> > 
> > Introduce has-stable_cpu_id property that allows target machines to
> > optionally switch to using stable_cpu_id instead of cpu_index.
> 
> If stable_cpu_id is in the [0..max_vcpus) range, then it does not really depend
> on the machine type or version.

No.. it really does.  The ids need to exactly match, not just be in
the same range in order to maintain migration compatibility with
pre-stable_cpu_id versions of qemu.

> In this case, the property is not needed.
> 
> > This will help allow successful migration in cases where holes are
> > introduced in cpu_index range after CPU hot removals.
> > 
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  exec.c            | 6 ++++--
> >  include/qom/cpu.h | 5 +++++
> >  qom/cpu.c         | 6 ++++++
> >  3 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index fb73910..3b36fe5 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
> >  void cpu_vmstate_register(CPUState *cpu)
> >  {
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > +    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
> > +                      cpu->cpu_index;
> >  
> >      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 331386f..527c021 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -273,6 +273,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.
> > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
> > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
> > + *     over cpu_index during vmstate registration.
> >   *
> >   * State of one CPU core or thread.
> >   */
> > @@ -360,6 +363,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 stable_cpu_id;
> > +    bool has_stable_cpu_id;
> >  };
> >  
> >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > diff --git a/qom/cpu.c b/qom/cpu.c
> > index 1095ea1..bae1bf7 100644
> > --- a/qom/cpu.c
> > +++ b/qom/cpu.c
> > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
> >      return cpu->cpu_index;
> >  }
> >  
> > +static Property cpu_common_properties[] = {
> > +    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),
> > +    DEFINE_PROP_END_OF_LIST()
> > +};
> > +
> >  static void cpu_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
> >       * IRQs, adding reset handlers, halting non-first CPUs, ...
> >       */
> >      dc->cannot_instantiate_with_device_add_yet = true;
> > +    dc->props = cpu_common_properties;
> >  }
> >  
> >  static const TypeInfo cpu_type_info = {
> 

-- 
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 v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[David Gibson]

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

On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote:
> Conditonally set stable_cpu_id for CPU threads that are created as part
> of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> onwards.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_cpu_core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index b104778..0ec3513 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      for (i = 0; i < cc->nr_threads; i++) {
>          char id[32];
>          obj = sc->threads + i * size;
> +        CPUState *cs;
>  
>          object_initialize(obj, size, typename);
> +        cs = CPU(obj);
> +
> +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */
> +        if (cs->has_stable_cpu_id) {
> +            cs->stable_cpu_id = cc->core_id + i;
> +        }

Testing cs->has_stable_cpu_id here in machine type specific code seems
really weird.  It's the machine type that knows whether it has a
stable ID to give to the CPU or not, rather than the other way around.

Since we haven't yet had a release with cpu cores, I think the right
thing is for cpu_core to unconditionally set the stable ID (and set
has_stable_id to true).  The backup path that does thread-based cpu
init, can set has_stable_id to false (if that's not the default).

>          snprintf(id, sizeof(id), "thread[%d]", i);
>          object_property_add_child(OBJECT(sc), id, obj, &local_err);
>          if (local_err) {

-- 
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 v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[David Gibson]

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

On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote:
> On Thu,  7 Jul 2016 20:20:23 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Conditonally set stable_cpu_id for CPU threads that are created as part
> > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > onwards.
> > 
> 
> The last sentence is a bit confusing since the enablement actually happens
> in patch 5/5.
> 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index b104778..0ec3513 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      for (i = 0; i < cc->nr_threads; i++) {
> >          char id[32];
> >          obj = sc->threads + i * size;
> > +        CPUState *cs;
> >  
> >          object_initialize(obj, size, typename);
> > +        cs = CPU(obj);
> > +
> > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */
> 
> It isn't what I had in mind. More something like below:
> 
> In ppc_spapr_init():
> 
>     for (i = 0; i < spapr_max_cores; i++) {
>         spapr->cores[i]->stable_id = i * smp_threads;
>     }
> 
> 
> In spapr_cpu_core_realize():
> 
>     for (j = 0; j < cc->nr_threads; j++) {
>         stable_cpu_id = cc->stable_id + j;
>     }
> 
> So we need to introduce cc->stable_id.

No, we don't.  Cores have had a stable ID since they were introduced.

Instead we should be setting the thread stable ids based on the core
stable id.

> I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id.
> 
> Makes sense ?
> 
> > +        if (cs->has_stable_cpu_id) {
> > +            cs->stable_cpu_id = cc->core_id + i;
> > +        }
> >          snprintf(id, sizeof(id), "thread[%d]", i);
> >          object_property_add_child(OBJECT(sc), id, obj, &local_err);
> >          if (local_err) {
> 

-- 
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 v2 4/5] xics: Use stable_cpu_id instead of cpu_index in XICS code

[David Gibson]

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

On Thu, Jul 07, 2016 at 08:20:24PM +0530, Bharata B Rao wrote:
> xics maintains an array of ICPState structures which is indexed
> by cpu_index. Optionally change this to index the ICPState array by
> stable_cpu_id. When the use of stable_cpu_id is enabled from pseries-2.7
> onwards, this allows migration of guest to succeed when there are holes in
> cpu_index range due to CPU core hot removal.

You haven't changed the allocation path, so this will waste a bunch of
space if the stable IDs aren't dense.  They always are for now, but
that might not be true for the upcoming powernv machine type.

> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/intc/xics.c        | 21 +++++++++++++++++----
>  hw/intc/xics_kvm.c    | 10 ++++------
>  hw/intc/xics_spapr.c  | 29 +++++++++++++++++------------
>  include/hw/ppc/xics.h |  1 +
>  4 files changed, 39 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index cd48f42..97ff3c5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -36,6 +36,17 @@
>  #include "qemu/error-report.h"
>  #include "qapi/visitor.h"
>  
> +int xics_get_server(PowerPCCPU *cpu)
> +{
> +    CPUState *cs = CPU(cpu);
> +
> +    if (cs->has_stable_cpu_id) {
> +        return cs->stable_cpu_id;
> +    } else {
> +        return cs->cpu_index;
> +    }
> +}

I really think we want a generic helper that gets our best guess at a
stable id - the actual stable id if it's present, otherwise
cpu_index.  I think a bunch of things are going to want this.

> +
>  int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
>  {
>      PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
> @@ -50,9 +61,10 @@ 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];
> +    int server = xics_get_server(cpu);
> +    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 +75,11 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> -    ICPState *ss = &xics->ss[cs->cpu_index];
> +    int server = xics_get_server(cpu);
> +    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..f71b468 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -326,14 +326,12 @@ 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);
> +    int server = xics_get_server(cpu);
> +    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..5491f82 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -31,6 +31,7 @@
>  #include "trace.h"
>  #include "qemu/timer.h"
>  #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  #include "hw/ppc/xics.h"
>  #include "qapi/visitor.h"
>  #include "qapi/error.h"
> @@ -42,17 +43,19 @@
>  static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
> -    CPUState *cs = CPU(cpu);
> +    int server = xics_get_server(cpu);
>      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->has_stable_cpu_id ? args[0] :
> +                          xics_get_cpu_index_by_dt_id(args[0]);
>      target_ulong mfrr = args[1];
>  
>      if (server >= spapr->xics->nr_servers) {
> @@ -66,8 +69,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  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);
> +    int server = xics_get_server(cpu);
> +    uint32_t xirr = icp_accept(spapr->xics->ss + server);
>  
>      args[0] = xirr;
>      return H_SUCCESS;
> @@ -76,8 +79,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  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];
> +    int server = xics_get_server(cpu);
> +    ICPState *ss = &spapr->xics->ss[server];
>      uint32_t xirr = icp_accept(ss);
>  
>      args[0] = xirr;
> @@ -88,19 +91,19 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            target_ulong opcode, target_ulong *args)
>  {
> -    CPUState *cs = CPU(cpu);
> +    int server = xics_get_server(cpu);
>      target_ulong xirr = args[0];
>  
> -    icp_eoi(spapr->xics, cs->cpu_index, xirr);
> +    icp_eoi(spapr->xics, server, xirr);
>      return H_SUCCESS;
>  }
>  
>  static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                              target_ulong opcode, target_ulong *args)
>  {
> -    CPUState *cs = CPU(cpu);
> +    int server = xics_get_server(cpu);
>      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 +116,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 +126,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->has_stable_cpu_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)
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6189a3b..aea0678 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -195,5 +195,6 @@ void ics_write_xive(ICSState *ics, int nr, int server,
>  void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
>  
>  int xics_find_source(XICSState *icp, int irq);
> +int xics_get_server(PowerPCCPU *cpu);
>  
>  #endif /* __XICS_H__ */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

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

[David Gibson]

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

On Thu, Jul 07, 2016 at 06:04:10PM +0200, Greg Kurz wrote:
> On Thu,  7 Jul 2016 20:20:20 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > 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.
> > 
> > - CPUState::stable_cpu_id is newly introduced and used as instance_id when
> >   registering CPU devices using vmstate_register. stable_cpu_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 stable_cpu_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 (which is stable_cpu_id for pseries-2.7 onwards)
> > based on core-id and hence is based on ppc-vcpu-dt-id-rework branch of his
> > tree.
> > 
> 
> I'm not very comfortable with this. Shouldn't it be the other way round
> actually: cpu_dt_id depending on stable_cpu_id ?
> 
> I think we're missing something like a stable_core_id.

The core-id is already stable.

Deriving the stable vcpu id from the core id is correct.

cpu_dt_id should actually go away, and we should just use
stable_cpu_id when creating the device tree.

-- 
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 v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[Bharata B Rao]

On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote:
> On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote:
> > Conditonally set stable_cpu_id for CPU threads that are created as part
> > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > onwards.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index b104778..0ec3513 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      for (i = 0; i < cc->nr_threads; i++) {
> >          char id[32];
> >          obj = sc->threads + i * size;
> > +        CPUState *cs;
> >  
> >          object_initialize(obj, size, typename);
> > +        cs = CPU(obj);
> > +
> > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */
> > +        if (cs->has_stable_cpu_id) {
> > +            cs->stable_cpu_id = cc->core_id + i;
> > +        }
> 
> Testing cs->has_stable_cpu_id here in machine type specific code seems
> really weird.  It's the machine type that knows whether it has a
> stable ID to give to the CPU or not, rather than the other way around.
> 
> Since we haven't yet had a release with cpu cores, I think the right
> thing is for cpu_core to unconditionally set the stable ID (and set
> has_stable_id to true).

Right, we can set cpu_stable_id unconditionally here since this code path
(core realize) will be taken only for pseries-2.7 onwards. has_stable_id
will get set as part of the property we defined in SPAPR_COMPAT_2_7.

> The backup path that does thread-based cpu
> init, can set has_stable_id to false (if that's not the default).

Default is off, but turning it on for 2.7 will be inherited by 2.6
and others below. Hence I have code to explicitly disable this prop
for 2.6 and below via SPAPR_COMPAT_2_6.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[David Gibson]

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

On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote:
> On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote:
> > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote:
> > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > onwards.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index b104778..0ec3513 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > >      for (i = 0; i < cc->nr_threads; i++) {
> > >          char id[32];
> > >          obj = sc->threads + i * size;
> > > +        CPUState *cs;
> > >  
> > >          object_initialize(obj, size, typename);
> > > +        cs = CPU(obj);
> > > +
> > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */
> > > +        if (cs->has_stable_cpu_id) {
> > > +            cs->stable_cpu_id = cc->core_id + i;
> > > +        }
> > 
> > Testing cs->has_stable_cpu_id here in machine type specific code seems
> > really weird.  It's the machine type that knows whether it has a
> > stable ID to give to the CPU or not, rather than the other way around.
> > 
> > Since we haven't yet had a release with cpu cores, I think the right
> > thing is for cpu_core to unconditionally set the stable ID (and set
> > has_stable_id to true).
> 
> Right, we can set cpu_stable_id unconditionally here since this code path
> (core realize) will be taken only for pseries-2.7 onwards. has_stable_id
> will get set as part of the property we defined in SPAPR_COMPAT_2_7.

Hrm, that's true.  But when you describe it like that it sounds like a
really non-obvious and fragile dependency between different components.

> > The backup path that does thread-based cpu
> > init, can set has_stable_id to false (if that's not the default).
> 
> Default is off, but turning it on for 2.7 will be inherited by 2.6
> and others below. Hence I have code to explicitly disable this prop
> for 2.6 and below via SPAPR_COMPAT_2_6.

This is all seeming terribly awkward.  Can we try investigating a
different approach:

    1. Rename cpu_index to cpu_id, but it's still used in the same
       places it's used.

    2. Remove assumptions that cpu_id values are contiguous or
       dense
    
    3. Machine type decides whether it wants to populate the cpu_id
       values explicitly, or leave it to generic code to calculate
       them as cpu_index is calculated now.

    4. Ideally, generic code enforces that the machine type populates
       either all or none of the cpu_id values.

Does that seem workable?

-- 
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 v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[Greg Kurz]

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

On Fri, 8 Jul 2016 15:25:33 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote:
> > On Thu,  7 Jul 2016 20:20:23 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > onwards.
> > >   
> > 
> > The last sentence is a bit confusing since the enablement actually happens
> > in patch 5/5.
> >   
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index b104778..0ec3513 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > >      for (i = 0; i < cc->nr_threads; i++) {
> > >          char id[32];
> > >          obj = sc->threads + i * size;
> > > +        CPUState *cs;
> > >  
> > >          object_initialize(obj, size, typename);
> > > +        cs = CPU(obj);
> > > +
> > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */  
> > 
> > It isn't what I had in mind. More something like below:
> > 
> > In ppc_spapr_init():
> > 
> >     for (i = 0; i < spapr_max_cores; i++) {
> >         spapr->cores[i]->stable_id = i * smp_threads;
> >     }
> > 
> > 
> > In spapr_cpu_core_realize():
> > 
> >     for (j = 0; j < cc->nr_threads; j++) {
> >         stable_cpu_id = cc->stable_id + j;
> >     }
> > 
> > So we need to introduce cc->stable_id.  
> 
> No, we don't.  Cores have had a stable ID since they were introduced.
> 

I agree core_dt_id is stable but it is a DT concept.

static void ppc_spapr_init(MachineState *machine)
{
[...]
        for (i = 0; i < spapr_max_cores; i++) {
            int core_dt_id = i * smt;
[...]
                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
                                        &error_fatal);

This patch produces stable_cpu_id in the [0...smt * smp_cores) range. I find it
awkward it depends on the host setup.

I'm suggesting we introduce cc->stable_id to be able to compute a simple
stable_cpu_id in the range [0...max_cpus), like x86 and ARM.

> Instead we should be setting the thread stable ids based on the core
> stable id.
> 
> > I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id.
> > 
> > Makes sense ?
> >   
> > > +        if (cs->has_stable_cpu_id) {
> > > +            cs->stable_cpu_id = cc->core_id + i;
> > > +        }
> > >          snprintf(id, sizeof(id), "thread[%d]", i);
> > >          object_property_add_child(OBJECT(sc), id, obj, &local_err);
> > >          if (local_err) {  
> >   
> 


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

Re: [Qemu-devel] [RFC PATCH v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[David Gibson]

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

On Fri, Jul 08, 2016 at 09:46:47AM +0200, Greg Kurz wrote:
> On Fri, 8 Jul 2016 15:25:33 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote:
> > > On Thu,  7 Jul 2016 20:20:23 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >   
> > > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > > onwards.
> > > >   
> > > 
> > > The last sentence is a bit confusing since the enablement actually happens
> > > in patch 5/5.
> > >   
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > index b104778..0ec3513 100644
> > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > >      for (i = 0; i < cc->nr_threads; i++) {
> > > >          char id[32];
> > > >          obj = sc->threads + i * size;
> > > > +        CPUState *cs;
> > > >  
> > > >          object_initialize(obj, size, typename);
> > > > +        cs = CPU(obj);
> > > > +
> > > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */  
> > > 
> > > It isn't what I had in mind. More something like below:
> > > 
> > > In ppc_spapr_init():
> > > 
> > >     for (i = 0; i < spapr_max_cores; i++) {
> > >         spapr->cores[i]->stable_id = i * smp_threads;
> > >     }
> > > 
> > > 
> > > In spapr_cpu_core_realize():
> > > 
> > >     for (j = 0; j < cc->nr_threads; j++) {
> > >         stable_cpu_id = cc->stable_id + j;
> > >     }
> > > 
> > > So we need to introduce cc->stable_id.  
> > 
> > No, we don't.  Cores have had a stable ID since they were introduced.
> > 
> 
> I agree core_dt_id is stable but it is a DT concept.

There is no core_dt_id.  There's just core-id, which is machine
assigned (via the query hotpluggable cpus interface) and stable.

> static void ppc_spapr_init(MachineState *machine)
> {
> [...]
>         for (i = 0; i < spapr_max_cores; i++) {
>             int core_dt_id = i * smt;

..uh, ok, except for that poorly named variable.  But that's because
this is in the machine type, and it knows it's going to use the same
ids to give to the core object and to put in the device tree.

> [...]
>                 object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
>                                         &error_fatal);
> 
> This patch produces stable_cpu_id in the [0...smt * smp_cores) range. I find it
> awkward it depends on the host setup.

True.  Possibly we should set these as i * (maximum plausible number
of threads).

The gotcha is that currently we're using the same "dt_id" to control
KVM's cpu id and that in turn controls the SMT level.  That's a poor
interface on the kernel side (my bad), but we have to live with it
now.  However we could de-couple that KVM id from the core-id.  It'd
no doubt cause some complications with kvm-xics, but we can probably
handle it.

> I'm suggesting we introduce cc->stable_id to be able to compute a simple
> stable_cpu_id in the range [0...max_cpus), like x86 and ARM.

I really don't see what properties this is supposed to have that are
different from the existing core-id.

> 
> > Instead we should be setting the thread stable ids based on the core
> > stable id.
> > 
> > > I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id.
> > > 
> > > Makes sense ?
> > >   
> > > > +        if (cs->has_stable_cpu_id) {
> > > > +            cs->stable_cpu_id = cc->core_id + i;
> > > > +        }
> > > >          snprintf(id, sizeof(id), "thread[%d]", i);
> > > >          object_property_add_child(OBJECT(sc), id, obj, &local_err);
> > > >          if (local_err) {  
> > >   
> > 
> 



-- 
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 v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[Igor Mammedov]

On Fri, 8 Jul 2016 17:39:52 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote:
> > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote:  
> > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote:  
> > > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > > onwards.
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > index b104778..0ec3513 100644
> > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > >      for (i = 0; i < cc->nr_threads; i++) {
> > > >          char id[32];
> > > >          obj = sc->threads + i * size;
> > > > +        CPUState *cs;
> > > >  
> > > >          object_initialize(obj, size, typename);
> > > > +        cs = CPU(obj);
> > > > +
> > > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */
> > > > +        if (cs->has_stable_cpu_id) {
> > > > +            cs->stable_cpu_id = cc->core_id + i;
> > > > +        }  
> > > 
> > > Testing cs->has_stable_cpu_id here in machine type specific code seems
> > > really weird.  It's the machine type that knows whether it has a
> > > stable ID to give to the CPU or not, rather than the other way around.
> > > 
> > > Since we haven't yet had a release with cpu cores, I think the right
> > > thing is for cpu_core to unconditionally set the stable ID (and set
> > > has_stable_id to true).  
> > 
> > Right, we can set cpu_stable_id unconditionally here since this code path
> > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id
> > will get set as part of the property we defined in SPAPR_COMPAT_2_7.  
> 
> Hrm, that's true.  But when you describe it like that it sounds like a
> really non-obvious and fragile dependency between different components.
that's how compat stuff is typically done for devices,
CPUs shouldn't be an exception. 
(consistency with other devices helps here in long run)
 
> > > The backup path that does thread-based cpu
> > > init, can set has_stable_id to false (if that's not the default).  
> > 
> > Default is off, but turning it on for 2.7 will be inherited by 2.6
> > and others below. Hence I have code to explicitly disable this prop
> > for 2.6 and below via SPAPR_COMPAT_2_6.  
> 
> This is all seeming terribly awkward.
Typically default is set the way so new machine type doesn't have
to enable it explicitly.

However the way it's done here helps not to touch/check every user
that uses cpu_index, limiting series impact only to code that
asks for it, it look a lot safer to got this rout for now.


>  Can we try investigating a
> different approach:
> 
>     1. Rename cpu_index to cpu_id, but it's still used in the same
>        places it's used.
> 
>     2. Remove assumptions that cpu_id values are contiguous or
>        dense
>     
>     3. Machine type decides whether it wants to populate the cpu_id
>        values explicitly, or leave it to generic code to calculate
>        them as cpu_index is calculated now.
> 
>     4. Ideally, generic code enforces that the machine type populates
>        either all or none of the cpu_id values.
> 
> Does that seem workable?
Ideally we will get there some day (and may be get rid of cpu_index altogether),
but for now it seems too invasive with a lot of chances to introduce non obvious
regression.

So I'd keep approach used in this series.

Re: [Qemu-devel] [RFC PATCH v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[Igor Mammedov]

On Fri, 8 Jul 2016 15:19:58 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote:
> > Add CPUState::stable_cpu_id and use that as instance_id in
> > vmstate_register() call.
> > 
> > Introduce has-stable_cpu_id property that allows target machines to
> > optionally switch to using stable_cpu_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.
> > 
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  exec.c            | 6 ++++--
> >  include/qom/cpu.h | 5 +++++
> >  qom/cpu.c         | 6 ++++++
> >  3 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index fb73910..3b36fe5 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
> >  void cpu_vmstate_register(CPUState *cpu)
> >  {
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > +    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
> > +                      cpu->cpu_index;
> >  
> >      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 331386f..527c021 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -273,6 +273,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.
> > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
> > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
> > + *     over cpu_index during vmstate registration.
> >   *
> >   * State of one CPU core or thread.
> >   */
> > @@ -360,6 +363,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 stable_cpu_id;
> > +    bool has_stable_cpu_id;
> >  };
> >  
> >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > diff --git a/qom/cpu.c b/qom/cpu.c
> > index 1095ea1..bae1bf7 100644
> > --- a/qom/cpu.c
> > +++ b/qom/cpu.c
> > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
> >      return cpu->cpu_index;
> >  }
> >  
> > +static Property cpu_common_properties[] = {
> > +    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),  
> 
> It seems odd to me that stable_cpu_id itself isn't exposed as a
> property.  Even if we don't need to set it externally for now, it
> really should be QOM introspectable.
Should it? Why?
It's QEMU internal detail and outside world preferably shouldn't
know anything about it.
As example look at cpu_index which were/is used in HMP and -numa
interfaces and now mgmt tries make some sense of it.

Cleaner way should be teaching -numa to handle cpus specified by
socket/core/thread IDs so that mgmt would actually know what CPUs
it assigns to what nodes and not to look at/invent/generate some
internal cpu_id.

> 
> > +    DEFINE_PROP_END_OF_LIST()
> > +};
> > +
> >  static void cpu_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
> >       * IRQs, adding reset handlers, halting non-first CPUs, ...
> >       */
> >      dc->cannot_instantiate_with_device_add_yet = true;
> > +    dc->props = cpu_common_properties;
> >  }
> >  
> >  static const TypeInfo cpu_type_info = {  
> 

Re: [Qemu-devel] [RFC PATCH v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[Greg Kurz]

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

On Fri, 8 Jul 2016 17:59:07 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jul 08, 2016 at 09:46:47AM +0200, Greg Kurz wrote:
> > On Fri, 8 Jul 2016 15:25:33 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote:  
> > > > On Thu,  7 Jul 2016 20:20:23 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > >     
> > > > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > > > onwards.
> > > > >     
> > > > 
> > > > The last sentence is a bit confusing since the enablement actually happens
> > > > in patch 5/5.
> > > >     
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > index b104778..0ec3513 100644
> > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > >      for (i = 0; i < cc->nr_threads; i++) {
> > > > >          char id[32];
> > > > >          obj = sc->threads + i * size;
> > > > > +        CPUState *cs;
> > > > >  
> > > > >          object_initialize(obj, size, typename);
> > > > > +        cs = CPU(obj);
> > > > > +
> > > > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */    
> > > > 
> > > > It isn't what I had in mind. More something like below:
> > > > 
> > > > In ppc_spapr_init():
> > > > 
> > > >     for (i = 0; i < spapr_max_cores; i++) {
> > > >         spapr->cores[i]->stable_id = i * smp_threads;
> > > >     }
> > > > 
> > > > 
> > > > In spapr_cpu_core_realize():
> > > > 
> > > >     for (j = 0; j < cc->nr_threads; j++) {
> > > >         stable_cpu_id = cc->stable_id + j;
> > > >     }
> > > > 
> > > > So we need to introduce cc->stable_id.    
> > > 
> > > No, we don't.  Cores have had a stable ID since they were introduced.
> > >   
> > 
> > I agree core_dt_id is stable but it is a DT concept.  
> 
> There is no core_dt_id.  There's just core-id, which is machine
> assigned (via the query hotpluggable cpus interface) and stable.
> 
> > static void ppc_spapr_init(MachineState *machine)
> > {
> > [...]
> >         for (i = 0; i < spapr_max_cores; i++) {
> >             int core_dt_id = i * smt;  
> 
> ..uh, ok, except for that poorly named variable.  But that's because
> this is in the machine type, and it knows it's going to use the same
> ids to give to the core object and to put in the device tree.
> 

It is core_id everywhere else.

$ git grep core_id hw/ppc/
hw/ppc/spapr.c:        cpu_props->has_core_id = true;
hw/ppc/spapr.c:        cpu_props->core_id = i * smt;
hw/ppc/spapr_cpu_core.c:    spapr->cores[cc->core_id / smt] = NULL;
hw/ppc/spapr_cpu_core.c:    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id);
hw/ppc/spapr_cpu_core.c:    index = cc->core_id / smt;
hw/ppc/spapr_cpu_core.c:    if (cc->core_id % smt) {
hw/ppc/spapr_cpu_core.c:        error_setg(&local_err, "invalid core id %d\n", cc->core_id);
hw/ppc/spapr_cpu_core.c:    index = cc->core_id / smt;
hw/ppc/spapr_cpu_core.c:        error_setg(&local_err, "core id %d out of range", cc->core_id);
hw/ppc/spapr_cpu_core.c:        error_setg(&local_err, "core %d already populated", cc->core_id);

$ git grep core_dt_id hw/ppc/
hw/ppc/spapr.c:            int core_dt_id = i * smt;
hw/ppc/spapr.c:                                       SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
hw/ppc/spapr.c:                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,

I got confused because the current code still puts cpu_dt_id of thread0 in the
device tree. And since cpu_dt_id is still being computed on cpu_index, it is
a different beast (which needs to be killed since it even crashes simple
hotplug/unplug scenarios).

> > [...]
> >                 object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
> >                                         &error_fatal);
> > 
> > This patch produces stable_cpu_id in the [0...smt * smp_cores) range. I find it
> > awkward it depends on the host setup.  
> 
> True.  Possibly we should set these as i * (maximum plausible number
> of threads).
> 
> The gotcha is that currently we're using the same "dt_id" to control
> KVM's cpu id and that in turn controls the SMT level.  That's a poor
> interface on the kernel side (my bad), but we have to live with it
> now.  However we could de-couple that KVM id from the core-id.  It'd
> no doubt cause some complications with kvm-xics, but we can probably
> handle it.
> 
> > I'm suggesting we introduce cc->stable_id to be able to compute a simple
> > stable_cpu_id in the range [0...max_cpus), like x86 and ARM.  
> 
> I really don't see what properties this is supposed to have that are
> different from the existing core-id.
> 

Simplicity of always having CPU0, 1, 2, 3... max_cpus in QEMU, and try
to hide the "poor interface on the kernel side" from the code that
does not need it... but maybe that is not that important.

> >   
> > > Instead we should be setting the thread stable ids based on the core
> > > stable id.
> > >   
> > > > I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id.
> > > > 
> > > > Makes sense ?
> > > >     
> > > > > +        if (cs->has_stable_cpu_id) {
> > > > > +            cs->stable_cpu_id = cc->core_id + i;
> > > > > +        }
> > > > >          snprintf(id, sizeof(id), "thread[%d]", i);
> > > > >          object_property_add_child(OBJECT(sc), id, obj, &local_err);
> > > > >          if (local_err) {    
> > > >     
> > >   
> >   
> 
> 
> 


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

Re: [Qemu-devel] [RFC PATCH v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[Bharata B Rao]

On Fri, Jul 08, 2016 at 12:59:59PM +0200, Igor Mammedov wrote:
> On Fri, 8 Jul 2016 17:39:52 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote:
> > > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote:  
> > > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote:  
> > > > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > > > onwards.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > index b104778..0ec3513 100644
> > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > >      for (i = 0; i < cc->nr_threads; i++) {
> > > > >          char id[32];
> > > > >          obj = sc->threads + i * size;
> > > > > +        CPUState *cs;
> > > > >  
> > > > >          object_initialize(obj, size, typename);
> > > > > +        cs = CPU(obj);
> > > > > +
> > > > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */
> > > > > +        if (cs->has_stable_cpu_id) {
> > > > > +            cs->stable_cpu_id = cc->core_id + i;
> > > > > +        }  
> > > > 
> > > > Testing cs->has_stable_cpu_id here in machine type specific code seems
> > > > really weird.  It's the machine type that knows whether it has a
> > > > stable ID to give to the CPU or not, rather than the other way around.
> > > > 
> > > > Since we haven't yet had a release with cpu cores, I think the right
> > > > thing is for cpu_core to unconditionally set the stable ID (and set
> > > > has_stable_id to true).  
> > > 
> > > Right, we can set cpu_stable_id unconditionally here since this code path
> > > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id
> > > will get set as part of the property we defined in SPAPR_COMPAT_2_7.  
> > 
> > Hrm, that's true.  But when you describe it like that it sounds like a
> > really non-obvious and fragile dependency between different components.
> that's how compat stuff is typically done for devices,
> CPUs shouldn't be an exception. 
> (consistency with other devices helps here in long run)
> 
> > > > The backup path that does thread-based cpu
> > > > init, can set has_stable_id to false (if that's not the default).  
> > > 
> > > Default is off, but turning it on for 2.7 will be inherited by 2.6
> > > and others below. Hence I have code to explicitly disable this prop
> > > for 2.6 and below via SPAPR_COMPAT_2_6.  
> > 
> > This is all seeming terribly awkward.
> Typically default is set the way so new machine type doesn't have
> to enable it explicitly.
> 
> However the way it's done here helps not to touch/check every user
> that uses cpu_index, limiting series impact only to code that
> asks for it, it look a lot safer to got this rout for now.

David,

- I believe there's a consensus on using core-id as the stable_cpu_id.
- You weren't liking the use of a separate property user-stable-cpu-id to
  control/enable the use of stable_cpu_id. After Igor's reply above, should
  we stick with that approach ?
- I am planning to drop the code that introduces cpu_common_unrealize()
  and that moves vmstate_[un]register() calls to qom/cpu.c as that affects
  all other archs. Instead lets just check for use_stable_cpu_id from exec.c
  itself and use it appropriately.

If you are ok with all the above, I shall post the next version on top
of Greg's patchset.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[David Gibson]

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

On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote:
> On Fri, 8 Jul 2016 15:19:58 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote:
> > > Add CPUState::stable_cpu_id and use that as instance_id in
> > > vmstate_register() call.
> > > 
> > > Introduce has-stable_cpu_id property that allows target machines to
> > > optionally switch to using stable_cpu_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.
> > > 
> > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  exec.c            | 6 ++++--
> > >  include/qom/cpu.h | 5 +++++
> > >  qom/cpu.c         | 6 ++++++
> > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/exec.c b/exec.c
> > > index fb73910..3b36fe5 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
> > >  void cpu_vmstate_register(CPUState *cpu)
> > >  {
> > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > > +    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
> > > +                      cpu->cpu_index;
> > >  
> > >      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 331386f..527c021 100644
> > > --- a/include/qom/cpu.h
> > > +++ b/include/qom/cpu.h
> > > @@ -273,6 +273,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.
> > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
> > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
> > > + *     over cpu_index during vmstate registration.
> > >   *
> > >   * State of one CPU core or thread.
> > >   */
> > > @@ -360,6 +363,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 stable_cpu_id;
> > > +    bool has_stable_cpu_id;
> > >  };
> > >  
> > >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > index 1095ea1..bae1bf7 100644
> > > --- a/qom/cpu.c
> > > +++ b/qom/cpu.c
> > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
> > >      return cpu->cpu_index;
> > >  }
> > >  
> > > +static Property cpu_common_properties[] = {
> > > +    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),  
> > 
> > It seems odd to me that stable_cpu_id itself isn't exposed as a
> > property.  Even if we don't need to set it externally for now, it
> > really should be QOM introspectable.
> Should it? Why?

Well, for one thing it's really strange to have the boolean flag
exposed, but not the value itself.

> It's QEMU internal detail and outside world preferably shouldn't
> know anything about it.

Hrm.. I guess kinda.  But I think it's less an internal detail than
the existing cpu_index is.

> As example look at cpu_index which were/is used in HMP and -numa
> interfaces and now mgmt tries make some sense of it.
> 
> Cleaner way should be teaching -numa to handle cpus specified by
> socket/core/thread IDs so that mgmt would actually know what CPUs
> it assigns to what nodes and not to look at/invent/generate some
> internal cpu_id.
> 
> > 
> > > +    DEFINE_PROP_END_OF_LIST()
> > > +};
> > > +
> > >  static void cpu_class_init(ObjectClass *klass, void *data)
> > >  {
> > >      DeviceClass *dc = DEVICE_CLASS(klass);
> > > @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
> > >       * IRQs, adding reset handlers, halting non-first CPUs, ...
> > >       */
> > >      dc->cannot_instantiate_with_device_add_yet = true;
> > > +    dc->props = cpu_common_properties;
> > >  }
> > >  
> > >  static const TypeInfo cpu_type_info = {  
> > 
> 

-- 
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 v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[David Gibson]

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

On Fri, Jul 08, 2016 at 05:24:24PM +0200, Greg Kurz wrote:
> On Fri, 8 Jul 2016 17:59:07 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jul 08, 2016 at 09:46:47AM +0200, Greg Kurz wrote:
> > > On Fri, 8 Jul 2016 15:25:33 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote:  
> > > > > On Thu,  7 Jul 2016 20:20:23 +0530
> > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > >     
> > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > > > > onwards.
> > > > > >     
> > > > > 
> > > > > The last sentence is a bit confusing since the enablement actually happens
> > > > > in patch 5/5.
> > > > >     
> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > ---
> > > > > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > > > > >  1 file changed, 7 insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > index b104778..0ec3513 100644
> > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > > >      for (i = 0; i < cc->nr_threads; i++) {
> > > > > >          char id[32];
> > > > > >          obj = sc->threads + i * size;
> > > > > > +        CPUState *cs;
> > > > > >  
> > > > > >          object_initialize(obj, size, typename);
> > > > > > +        cs = CPU(obj);
> > > > > > +
> > > > > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */    
> > > > > 
> > > > > It isn't what I had in mind. More something like below:
> > > > > 
> > > > > In ppc_spapr_init():
> > > > > 
> > > > >     for (i = 0; i < spapr_max_cores; i++) {
> > > > >         spapr->cores[i]->stable_id = i * smp_threads;
> > > > >     }
> > > > > 
> > > > > 
> > > > > In spapr_cpu_core_realize():
> > > > > 
> > > > >     for (j = 0; j < cc->nr_threads; j++) {
> > > > >         stable_cpu_id = cc->stable_id + j;
> > > > >     }
> > > > > 
> > > > > So we need to introduce cc->stable_id.    
> > > > 
> > > > No, we don't.  Cores have had a stable ID since they were introduced.
> > > >   
> > > 
> > > I agree core_dt_id is stable but it is a DT concept.  
> > 
> > There is no core_dt_id.  There's just core-id, which is machine
> > assigned (via the query hotpluggable cpus interface) and stable.
> > 
> > > static void ppc_spapr_init(MachineState *machine)
> > > {
> > > [...]
> > >         for (i = 0; i < spapr_max_cores; i++) {
> > >             int core_dt_id = i * smt;  
> > 
> > ..uh, ok, except for that poorly named variable.  But that's because
> > this is in the machine type, and it knows it's going to use the same
> > ids to give to the core object and to put in the device tree.
> > 
> 
> It is core_id everywhere else.
> 
> $ git grep core_id hw/ppc/
> hw/ppc/spapr.c:        cpu_props->has_core_id = true;
> hw/ppc/spapr.c:        cpu_props->core_id = i * smt;
> hw/ppc/spapr_cpu_core.c:    spapr->cores[cc->core_id / smt] = NULL;
> hw/ppc/spapr_cpu_core.c:    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id);
> hw/ppc/spapr_cpu_core.c:    index = cc->core_id / smt;
> hw/ppc/spapr_cpu_core.c:    if (cc->core_id % smt) {
> hw/ppc/spapr_cpu_core.c:        error_setg(&local_err, "invalid core id %d\n", cc->core_id);
> hw/ppc/spapr_cpu_core.c:    index = cc->core_id / smt;
> hw/ppc/spapr_cpu_core.c:        error_setg(&local_err, "core id %d out of range", cc->core_id);
> hw/ppc/spapr_cpu_core.c:        error_setg(&local_err, "core %d already populated", cc->core_id);
> 
> $ git grep core_dt_id hw/ppc/
> hw/ppc/spapr.c:            int core_dt_id = i * smt;
> hw/ppc/spapr.c:                                       SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
> hw/ppc/spapr.c:                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
> 
> I got confused because the current code still puts cpu_dt_id of thread0 in the
> device tree. And since cpu_dt_id is still being computed on cpu_index, it is
> a different beast (which needs to be killed since it even crashes simple
> hotplug/unplug scenarios).
> 
> > > [...]
> > >                 object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
> > >                                         &error_fatal);
> > > 
> > > This patch produces stable_cpu_id in the [0...smt * smp_cores) range. I find it
> > > awkward it depends on the host setup.  
> > 
> > True.  Possibly we should set these as i * (maximum plausible number
> > of threads).
> > 
> > The gotcha is that currently we're using the same "dt_id" to control
> > KVM's cpu id and that in turn controls the SMT level.  That's a poor
> > interface on the kernel side (my bad), but we have to live with it
> > now.  However we could de-couple that KVM id from the core-id.  It'd
> > no doubt cause some complications with kvm-xics, but we can probably
> > handle it.
> > 
> > > I'm suggesting we introduce cc->stable_id to be able to compute a simple
> > > stable_cpu_id in the range [0...max_cpus), like x86 and ARM.  
> > 
> > I really don't see what properties this is supposed to have that are
> > different from the existing core-id.
> > 
> 
> Simplicity of always having CPU0, 1, 2, 3... max_cpus in QEMU, and try
> to hide the "poor interface on the kernel side" from the code that
> does not need it... but maybe that is not that important.

Merely having contiguous values seems like a very poor trade off for
having two different IDs for the same object to get confused between.

-- 
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 v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[David Gibson]

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

On Fri, Jul 08, 2016 at 12:59:59PM +0200, Igor Mammedov wrote:
> On Fri, 8 Jul 2016 17:39:52 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote:
> > > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote:  
> > > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote:  
> > > > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > > > onwards.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > index b104778..0ec3513 100644
> > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > >      for (i = 0; i < cc->nr_threads; i++) {
> > > > >          char id[32];
> > > > >          obj = sc->threads + i * size;
> > > > > +        CPUState *cs;
> > > > >  
> > > > >          object_initialize(obj, size, typename);
> > > > > +        cs = CPU(obj);
> > > > > +
> > > > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */
> > > > > +        if (cs->has_stable_cpu_id) {
> > > > > +            cs->stable_cpu_id = cc->core_id + i;
> > > > > +        }  
> > > > 
> > > > Testing cs->has_stable_cpu_id here in machine type specific code seems
> > > > really weird.  It's the machine type that knows whether it has a
> > > > stable ID to give to the CPU or not, rather than the other way around.
> > > > 
> > > > Since we haven't yet had a release with cpu cores, I think the right
> > > > thing is for cpu_core to unconditionally set the stable ID (and set
> > > > has_stable_id to true).  
> > > 
> > > Right, we can set cpu_stable_id unconditionally here since this code path
> > > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id
> > > will get set as part of the property we defined in SPAPR_COMPAT_2_7.  
> > 
> > Hrm, that's true.  But when you describe it like that it sounds like a
> > really non-obvious and fragile dependency between different components.
> that's how compat stuff is typically done for devices,
> CPUs shouldn't be an exception. 
> (consistency with other devices helps here in long run)
>  
> > > > The backup path that does thread-based cpu
> > > > init, can set has_stable_id to false (if that's not the default).  
> > > 
> > > Default is off, but turning it on for 2.7 will be inherited by 2.6
> > > and others below. Hence I have code to explicitly disable this prop
> > > for 2.6 and below via SPAPR_COMPAT_2_6.  
> > 
> > This is all seeming terribly awkward.
> Typically default is set the way so new machine type doesn't have
> to enable it explicitly.
> 
> However the way it's done here helps not to touch/check every user
> that uses cpu_index, limiting series impact only to code that
> asks for it, it look a lot safer to got this rout for now.
> 
> 
> >  Can we try investigating a
> > different approach:
> > 
> >     1. Rename cpu_index to cpu_id, but it's still used in the same
> >        places it's used.
> > 
> >     2. Remove assumptions that cpu_id values are contiguous or
> >        dense
> >     
> >     3. Machine type decides whether it wants to populate the cpu_id
> >        values explicitly, or leave it to generic code to calculate
> >        them as cpu_index is calculated now.
> > 
> >     4. Ideally, generic code enforces that the machine type populates
> >        either all or none of the cpu_id values.
> > 
> > Does that seem workable?
> Ideally we will get there some day (and may be get rid of cpu_index altogether),
> but for now it seems too invasive with a lot of chances to introduce non obvious
> regression.

Yes, that's a risk.  But I'm basically no longer convinced that it's
any higher than the risk of the same thing with the current approach.

> So I'd keep approach used in this series.

-- 
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 v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[Bharata B Rao]

On Mon, Jul 11, 2016 at 01:22:37PM +1000, David Gibson wrote:
> On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote:
> > On Fri, 8 Jul 2016 15:19:58 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote:
> > > > Add CPUState::stable_cpu_id and use that as instance_id in
> > > > vmstate_register() call.
> > > > 
> > > > Introduce has-stable_cpu_id property that allows target machines to
> > > > optionally switch to using stable_cpu_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.
> > > > 
> > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  exec.c            | 6 ++++--
> > > >  include/qom/cpu.h | 5 +++++
> > > >  qom/cpu.c         | 6 ++++++
> > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/exec.c b/exec.c
> > > > index fb73910..3b36fe5 100644
> > > > --- a/exec.c
> > > > +++ b/exec.c
> > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
> > > >  void cpu_vmstate_register(CPUState *cpu)
> > > >  {
> > > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > +    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
> > > > +                      cpu->cpu_index;
> > > >  
> > > >      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 331386f..527c021 100644
> > > > --- a/include/qom/cpu.h
> > > > +++ b/include/qom/cpu.h
> > > > @@ -273,6 +273,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.
> > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
> > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
> > > > + *     over cpu_index during vmstate registration.
> > > >   *
> > > >   * State of one CPU core or thread.
> > > >   */
> > > > @@ -360,6 +363,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 stable_cpu_id;
> > > > +    bool has_stable_cpu_id;
> > > >  };
> > > >  
> > > >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > > index 1095ea1..bae1bf7 100644
> > > > --- a/qom/cpu.c
> > > > +++ b/qom/cpu.c
> > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
> > > >      return cpu->cpu_index;
> > > >  }
> > > >  
> > > > +static Property cpu_common_properties[] = {
> > > > +    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),  
> > > 
> > > It seems odd to me that stable_cpu_id itself isn't exposed as a
> > > property.  Even if we don't need to set it externally for now, it
> > > really should be QOM introspectable.
> > Should it? Why?
> 
> Well, for one thing it's really strange to have the boolean flag
> exposed, but not the value itself.

How about just the property which starts with a deafult value (-1 ?)
based on which we can either use stable_cpu_id or cpu_index with
vmstate_register() calls ? Machine types that want to use stable_cpu_id
can explicitly set this property to a valid "non -1" value ?

I remember you were suggesting something like this earlier.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[Igor Mammedov]

On Mon, 11 Jul 2016 09:05:07 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, Jul 11, 2016 at 01:22:37PM +1000, David Gibson wrote:
> > On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote:  
> > > On Fri, 8 Jul 2016 15:19:58 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote:  
> > > > > Add CPUState::stable_cpu_id and use that as instance_id in
> > > > > vmstate_register() call.
> > > > > 
> > > > > Introduce has-stable_cpu_id property that allows target machines to
> > > > > optionally switch to using stable_cpu_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.
> > > > > 
> > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > >  exec.c            | 6 ++++--
> > > > >  include/qom/cpu.h | 5 +++++
> > > > >  qom/cpu.c         | 6 ++++++
> > > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/exec.c b/exec.c
> > > > > index fb73910..3b36fe5 100644
> > > > > --- a/exec.c
> > > > > +++ b/exec.c
> > > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
> > > > >  void cpu_vmstate_register(CPUState *cpu)
> > > > >  {
> > > > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > +    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
> > > > > +                      cpu->cpu_index;
> > > > >  
> > > > >      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 331386f..527c021 100644
> > > > > --- a/include/qom/cpu.h
> > > > > +++ b/include/qom/cpu.h
> > > > > @@ -273,6 +273,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.
> > > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
> > > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
> > > > > + *     over cpu_index during vmstate registration.
> > > > >   *
> > > > >   * State of one CPU core or thread.
> > > > >   */
> > > > > @@ -360,6 +363,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 stable_cpu_id;
> > > > > +    bool has_stable_cpu_id;
> > > > >  };
> > > > >  
> > > > >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > > > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > > > index 1095ea1..bae1bf7 100644
> > > > > --- a/qom/cpu.c
> > > > > +++ b/qom/cpu.c
> > > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
> > > > >      return cpu->cpu_index;
> > > > >  }
> > > > >  
> > > > > +static Property cpu_common_properties[] = {
> > > > > +    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),    
> > > > 
> > > > It seems odd to me that stable_cpu_id itself isn't exposed as a
> > > > property.  Even if we don't need to set it externally for now, it
> > > > really should be QOM introspectable.  
> > > Should it? Why?  
> > 
> > Well, for one thing it's really strange to have the boolean flag
> > exposed, but not the value itself.  
> 
> How about just the property which starts with a deafult value (-1 ?)
> based on which we can either use stable_cpu_id or cpu_index with
> vmstate_register() calls ? Machine types that want to use stable_cpu_id
> can explicitly set this property to a valid "non -1" value ?
the main purpose of CPU::has-stable-cpu-id is to provide means to
reuse current compat logic for devices and keep it localized in CPU device.

It could be done the other way around i.e.
 - use -1 to detect not set stable-cpu-id
 - add has-stable-cpu-id field to machine or arch specific subclass
     (you see it doesn't go away, it just spreads compat logic to machine)
 - set has-stable-cpu-id in machine class init depending on version
     (on pc it would be pc_i440fx_X_X_machine_options)
 - do in machine code:
     if (has-stable-cpu-id) {
        assign stable-cpu-id
     }

I think the original device compat way is a bit more cleaner than above
(not to mention that it follows typical pattern)
 
> I remember you were suggesting something like this earlier.
> 
> Regards,
> Bharata.
> 
> 

Re: [Qemu-devel] [RFC PATCH v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[Igor Mammedov]

On Mon, 11 Jul 2016 13:22:37 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote:
> > On Fri, 8 Jul 2016 15:19:58 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote:  
> > > > Add CPUState::stable_cpu_id and use that as instance_id in
> > > > vmstate_register() call.
> > > > 
> > > > Introduce has-stable_cpu_id property that allows target machines to
> > > > optionally switch to using stable_cpu_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.
> > > > 
> > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  exec.c            | 6 ++++--
> > > >  include/qom/cpu.h | 5 +++++
> > > >  qom/cpu.c         | 6 ++++++
> > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/exec.c b/exec.c
> > > > index fb73910..3b36fe5 100644
> > > > --- a/exec.c
> > > > +++ b/exec.c
> > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
> > > >  void cpu_vmstate_register(CPUState *cpu)
> > > >  {
> > > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > +    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
> > > > +                      cpu->cpu_index;
> > > >  
> > > >      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 331386f..527c021 100644
> > > > --- a/include/qom/cpu.h
> > > > +++ b/include/qom/cpu.h
> > > > @@ -273,6 +273,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.
> > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
> > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
> > > > + *     over cpu_index during vmstate registration.
> > > >   *
> > > >   * State of one CPU core or thread.
> > > >   */
> > > > @@ -360,6 +363,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 stable_cpu_id;
> > > > +    bool has_stable_cpu_id;
> > > >  };
> > > >  
> > > >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > > index 1095ea1..bae1bf7 100644
> > > > --- a/qom/cpu.c
> > > > +++ b/qom/cpu.c
> > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
> > > >      return cpu->cpu_index;
> > > >  }
> > > >  
> > > > +static Property cpu_common_properties[] = {
> > > > +    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),    
> > > 
> > > It seems odd to me that stable_cpu_id itself isn't exposed as a
> > > property.  Even if we don't need to set it externally for now, it
> > > really should be QOM introspectable.  
> > Should it? Why?  
> 
> Well, for one thing it's really strange to have the boolean flag
> exposed, but not the value itself.
property doesn't always means that it's intended as an external interface

> 
> > It's QEMU internal detail and outside world preferably shouldn't
> > know anything about it.  
> 
> Hrm.. I guess kinda.  But I think it's less an internal detail than
> the existing cpu_index is.
so it' better not to start to advertise it as an external interface.

Should be add some flag to generic property to mark it as internal?

> 
> > As example look at cpu_index which were/is used in HMP and -numa
> > interfaces and now mgmt tries make some sense of it.
> > 
> > Cleaner way should be teaching -numa to handle cpus specified by
> > socket/core/thread IDs so that mgmt would actually know what CPUs
> > it assigns to what nodes and not to look at/invent/generate some
> > internal cpu_id.
> >   
> > >   
> > > > +    DEFINE_PROP_END_OF_LIST()
> > > > +};
> > > > +
> > > >  static void cpu_class_init(ObjectClass *klass, void *data)
> > > >  {
> > > >      DeviceClass *dc = DEVICE_CLASS(klass);
> > > > @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
> > > >       * IRQs, adding reset handlers, halting non-first CPUs, ...
> > > >       */
> > > >      dc->cannot_instantiate_with_device_add_yet = true;
> > > > +    dc->props = cpu_common_properties;
> > > >  }
> > > >  
> > > >  static const TypeInfo cpu_type_info = {    
> > >   
> >   
> 

Re: [Qemu-devel] [RFC PATCH v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[Igor Mammedov]

On Mon, 11 Jul 2016 13:26:01 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jul 08, 2016 at 12:59:59PM +0200, Igor Mammedov wrote:
> > On Fri, 8 Jul 2016 17:39:52 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote:  
> > > > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote:    
> > > > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote:    
> > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > > > > onwards.
> > > > > > 
> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > ---
> > > > > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > > > > >  1 file changed, 7 insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > index b104778..0ec3513 100644
> > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > > >      for (i = 0; i < cc->nr_threads; i++) {
> > > > > >          char id[32];
> > > > > >          obj = sc->threads + i * size;
> > > > > > +        CPUState *cs;
> > > > > >  
> > > > > >          object_initialize(obj, size, typename);
> > > > > > +        cs = CPU(obj);
> > > > > > +
> > > > > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */
> > > > > > +        if (cs->has_stable_cpu_id) {
> > > > > > +            cs->stable_cpu_id = cc->core_id + i;
> > > > > > +        }    
> > > > > 
> > > > > Testing cs->has_stable_cpu_id here in machine type specific code seems
> > > > > really weird.  It's the machine type that knows whether it has a
> > > > > stable ID to give to the CPU or not, rather than the other way around.
> > > > > 
> > > > > Since we haven't yet had a release with cpu cores, I think the right
> > > > > thing is for cpu_core to unconditionally set the stable ID (and set
> > > > > has_stable_id to true).    
> > > > 
> > > > Right, we can set cpu_stable_id unconditionally here since this code path
> > > > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id
> > > > will get set as part of the property we defined in SPAPR_COMPAT_2_7.    
> > > 
> > > Hrm, that's true.  But when you describe it like that it sounds like a
> > > really non-obvious and fragile dependency between different components.  
> > that's how compat stuff is typically done for devices,
> > CPUs shouldn't be an exception. 
> > (consistency with other devices helps here in long run)
> >    
> > > > > The backup path that does thread-based cpu
> > > > > init, can set has_stable_id to false (if that's not the default).    
> > > > 
> > > > Default is off, but turning it on for 2.7 will be inherited by 2.6
> > > > and others below. Hence I have code to explicitly disable this prop
> > > > for 2.6 and below via SPAPR_COMPAT_2_6.    
> > > 
> > > This is all seeming terribly awkward.  
> > Typically default is set the way so new machine type doesn't have
> > to enable it explicitly.
> > 
> > However the way it's done here helps not to touch/check every user
> > that uses cpu_index, limiting series impact only to code that
> > asks for it, it look a lot safer to got this rout for now.
> > 
> >   
> > >  Can we try investigating a
> > > different approach:
> > > 
> > >     1. Rename cpu_index to cpu_id, but it's still used in the same
> > >        places it's used.
> > > 
> > >     2. Remove assumptions that cpu_id values are contiguous or
> > >        dense
> > >     
> > >     3. Machine type decides whether it wants to populate the cpu_id
> > >        values explicitly, or leave it to generic code to calculate
> > >        them as cpu_index is calculated now.
> > > 
> > >     4. Ideally, generic code enforces that the machine type populates
> > >        either all or none of the cpu_id values.
> > > 
> > > Does that seem workable?  
> > Ideally we will get there some day (and may be get rid of cpu_index altogether),
> > but for now it seems too invasive with a lot of chances to introduce non obvious
> > regression.  
> 
> Yes, that's a risk.  But I'm basically no longer convinced that it's
> any higher than the risk of the same thing with the current approach.
to me it looks much less risky as it affects only migration path and
could be done in time for 2.7

if we go to cpu_index re-factoring path then I'm afraid it quite a bit
more complex and it's not a 2.7 material.


> 
> > So I'd keep approach used in this series.  
> 

[Qemu-devel] [PATCH] VARIANT 1: reuse device compat logic to pick preffered CPU's migration instance_id

[Igor Mammedov]

this approach i I preffer as it uses less per machine migration glue
and follows typical compat pattern for devices

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 exec.c              |  7 +++++--
 hw/i386/pc.c        | 10 +++++++---
 include/hw/compat.h |  6 +++++-
 include/qom/cpu.h   |  3 +++
 qom/cpu.c           | 13 +++++++++++++
 5 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/exec.c b/exec.c
index 0122ef7..79e1dcb 100644
--- a/exec.c
+++ b/exec.c
@@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
+    int migration_id;
 
     cpu->as = NULL;
     cpu->num_ases = 0;
@@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
     (void) cc;
     cpu_list_unlock();
 #else
+    migration_id = cc->use_migration_id > 0 ?
+        cpu->migration_id : cpu->cpu_index;
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
+        vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu);
     }
     if (cc->vmsd != NULL) {
-        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
+        vmstate_register(NULL, migration_id, cc->vmsd, cpu);
     }
 #endif
 }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index cd1745e..f041279 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 }
 
 static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
-                          Error **errp)
+                          Error **errp, int migration_id)
 {
     X86CPU *cpu = NULL;
+    CPUState *cs = CPU(cpu);
     Error *local_err = NULL;
 
     cpu = cpu_x86_create(cpu_model, &local_err);
@@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
         goto out;
     }
 
+    cs->migration_id = migration_id;
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
 
@@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
         return;
     }
 
-    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
+    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms)
         pcms->possible_cpus->len++;
         if (i < smp_cpus) {
             cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
-                             &error_fatal);
+                             &error_fatal, i);
             pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
             object_unref(OBJECT(cpu));
         }
@@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
+    CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU));
 
+    cc->use_migration_id = true;
     pcmc->get_hotplug_handler = mc->get_hotplug_handler;
     pcmc->pci_enabled = true;
     pcmc->has_acpi_build = true;
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 636befe..a66e80d 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,11 @@
 #define HW_COMPAT_H
 
 #define HW_COMPAT_2_6 \
-    /* empty */
+    { \
+        .driver   = TYPE_CPU, \
+        .property = "use-migration-id", \
+        .value    = "off", \
+    },
 
 #define HW_COMPAT_2_5 \
     {\
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 32f3af3..a29a2c9 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -177,6 +177,8 @@ typedef struct CPUClass {
                                 void *opaque);
 
     const struct VMStateDescription *vmsd;
+    int use_migration_id;
+
     int gdb_num_core_regs;
     const char *gdb_core_xml_file;
     gchar * (*gdb_arch_name)(CPUState *cpu);
@@ -360,6 +362,7 @@ 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;
 };
 
 QTAILQ_HEAD(CPUTailQ, CPUState);
diff --git a/qom/cpu.c b/qom/cpu.c
index 751e992..3eb48b9 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -342,11 +342,24 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
     return cpu->cpu_index;
 }
 
+static void cpu_common_set_use_migration_id(Object *obj, bool value,
+                                            Error **err)
+{
+    CPUClass *cc = CPU_GET_CLASS(obj);
+    cc->use_migration_id = value ? 1 : 0;
+}
+
 static void cpu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     CPUClass *k = CPU_CLASS(klass);
 
+    k->use_migration_id = -1;
+    object_class_property_add_bool(klass, "use-migration-id",
+                                   NULL,
+                                   cpu_common_set_use_migration_id,
+                                   &error_abort);
+
     k->class_by_name = cpu_common_class_by_name;
     k->parse_features = cpu_common_parse_features;
     k->reset = cpu_common_reset;
-- 
1.8.3.1

[Qemu-devel] [PATCH] VARIANT 2: use machine specific callback to pick CPU's migration instance_id

[Igor Mammedov]

it uses less CPU only code but needs per machine glue (pc+q53 for x86)
to keep migration instance_id == cpu_index for old machine
types.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 exec.c            |  8 ++++++--
 hw/i386/pc.c      | 16 ++++++++++++++++
 hw/i386/pc_piix.c |  3 +++
 hw/i386/pc_q35.c  |  3 +++
 include/qom/cpu.h |  1 +
 5 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index 0122ef7..60778ab 100644
--- a/exec.c
+++ b/exec.c
@@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
+    int migration_id;
 
     cpu->as = NULL;
     cpu->num_ases = 0;
@@ -708,11 +709,14 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
     (void) cc;
     cpu_list_unlock();
 #else
+    migration_id = cc->get_migration_id ?
+        cc->get_migration_id(cpu) : cpu->cpu_index;
+
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
+        vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu);
     }
     if (cc->vmsd != NULL) {
-        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
+        vmstate_register(NULL, migration_id, cc->vmsd, cpu);
     }
 #endif
 }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index cd1745e..703791f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2006,13 +2006,29 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
     }
 }
 
+static int pc_cpu_get_migration_id(CPUState *cs)
+{
+    CPUArchId apic_id, *found_cpu;
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+
+    apic_id.arch_id = cc->get_arch_id(cs);
+    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
+        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
+        pc_apic_cmp);
+    assert(found_cpu);
+    return found_cpu - pcms->possible_cpus->cpus;
+}
+
 static void pc_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
+    CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU));
 
+    cc->get_migration_id = pc_cpu_get_migration_id;
     pcmc->get_hotplug_handler = mc->get_hotplug_handler;
     pcmc->pci_enabled = true;
     pcmc->has_acpi_build = true;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a07dc81..d834e31 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -452,10 +452,13 @@ DEFINE_I440FX_MACHINE(v2_7, "pc-i440fx-2.7", NULL,
 static void pc_i440fx_2_6_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+    CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU));
+
     pc_i440fx_2_7_machine_options(m);
     m->is_default = 0;
     m->alias = NULL;
     pcmc->legacy_cpu_hotplug = true;
+    cc->get_migration_id = NULL;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_6);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c0b9961..24a556a 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -304,10 +304,13 @@ DEFINE_Q35_MACHINE(v2_7, "pc-q35-2.7", NULL,
 static void pc_q35_2_6_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+    CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU));
+
     pc_q35_2_7_machine_options(m);
     m->alias = NULL;
     pcmc->legacy_cpu_hotplug = true;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_6);
+    cc->get_migration_id = NULL;
 }
 
 DEFINE_Q35_MACHINE(v2_6, "pc-q35-2.6", NULL,
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 32f3af3..664573b 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
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] VARIANT 1: reuse device compat logic to pick preffered CPU's migration instance_id

[Paolo Bonzini]

On 11/07/2016 15:42, Igor Mammedov wrote:
> this approach i I preffer as it uses less per machine migration glue
> and follows typical compat pattern for devices
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  exec.c              |  7 +++++--
>  hw/i386/pc.c        | 10 +++++++---
>  include/hw/compat.h |  6 +++++-
>  include/qom/cpu.h   |  3 +++
>  qom/cpu.c           | 13 +++++++++++++
>  5 files changed, 33 insertions(+), 6 deletions(-)

Much better...

Paolo

Re: [Qemu-devel] [RFC PATCH v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[David Gibson]

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

On Mon, Jul 11, 2016 at 10:15:57AM +0200, Igor Mammedov wrote:
> On Mon, 11 Jul 2016 13:26:01 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jul 08, 2016 at 12:59:59PM +0200, Igor Mammedov wrote:
> > > On Fri, 8 Jul 2016 17:39:52 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote:  
> > > > > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote:    
> > > > > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote:    
> > > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > > > > > onwards.
> > > > > > > 
> > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > > ---
> > > > > > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > > index b104778..0ec3513 100644
> > > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > > > >      for (i = 0; i < cc->nr_threads; i++) {
> > > > > > >          char id[32];
> > > > > > >          obj = sc->threads + i * size;
> > > > > > > +        CPUState *cs;
> > > > > > >  
> > > > > > >          object_initialize(obj, size, typename);
> > > > > > > +        cs = CPU(obj);
> > > > > > > +
> > > > > > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */
> > > > > > > +        if (cs->has_stable_cpu_id) {
> > > > > > > +            cs->stable_cpu_id = cc->core_id + i;
> > > > > > > +        }    
> > > > > > 
> > > > > > Testing cs->has_stable_cpu_id here in machine type specific code seems
> > > > > > really weird.  It's the machine type that knows whether it has a
> > > > > > stable ID to give to the CPU or not, rather than the other way around.
> > > > > > 
> > > > > > Since we haven't yet had a release with cpu cores, I think the right
> > > > > > thing is for cpu_core to unconditionally set the stable ID (and set
> > > > > > has_stable_id to true).    
> > > > > 
> > > > > Right, we can set cpu_stable_id unconditionally here since this code path
> > > > > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id
> > > > > will get set as part of the property we defined in SPAPR_COMPAT_2_7.    
> > > > 
> > > > Hrm, that's true.  But when you describe it like that it sounds like a
> > > > really non-obvious and fragile dependency between different components.  
> > > that's how compat stuff is typically done for devices,
> > > CPUs shouldn't be an exception. 
> > > (consistency with other devices helps here in long run)
> > >    
> > > > > > The backup path that does thread-based cpu
> > > > > > init, can set has_stable_id to false (if that's not the default).    
> > > > > 
> > > > > Default is off, but turning it on for 2.7 will be inherited by 2.6
> > > > > and others below. Hence I have code to explicitly disable this prop
> > > > > for 2.6 and below via SPAPR_COMPAT_2_6.    
> > > > 
> > > > This is all seeming terribly awkward.  
> > > Typically default is set the way so new machine type doesn't have
> > > to enable it explicitly.
> > > 
> > > However the way it's done here helps not to touch/check every user
> > > that uses cpu_index, limiting series impact only to code that
> > > asks for it, it look a lot safer to got this rout for now.
> > > 
> > >   
> > > >  Can we try investigating a
> > > > different approach:
> > > > 
> > > >     1. Rename cpu_index to cpu_id, but it's still used in the same
> > > >        places it's used.
> > > > 
> > > >     2. Remove assumptions that cpu_id values are contiguous or
> > > >        dense
> > > >     
> > > >     3. Machine type decides whether it wants to populate the cpu_id
> > > >        values explicitly, or leave it to generic code to calculate
> > > >        them as cpu_index is calculated now.
> > > > 
> > > >     4. Ideally, generic code enforces that the machine type populates
> > > >        either all or none of the cpu_id values.
> > > > 
> > > > Does that seem workable?  
> > > Ideally we will get there some day (and may be get rid of cpu_index altogether),
> > > but for now it seems too invasive with a lot of chances to introduce non obvious
> > > regression.  
> > 
> > Yes, that's a risk.  But I'm basically no longer convinced that it's
> > any higher than the risk of the same thing with the current approach.
> to me it looks much less risky as it affects only migration path and
> could be done in time for 2.7
> 
> if we go to cpu_index re-factoring path then I'm afraid it quite a bit
> more complex and it's not a 2.7 material.

Yeah, ok.  Having looked in a bit more depth, I agree with you.

> 
> 
> > 
> > > So I'd keep approach used in this series.  
> > 
> 

-- 
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] VARIANT 1: reuse device compat logic to pick preffered CPU's migration instance_id

[David Gibson]

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

On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote:
> this approach i I preffer as it uses less per machine migration glue
> and follows typical compat pattern for devices
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

After our IRC discussion last night, I prefer this version as well.
We can definitely work with this on Power as well.

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

One query regarding the x86 specific implementation below, though.

> ---
>  exec.c              |  7 +++++--
>  hw/i386/pc.c        | 10 +++++++---
>  include/hw/compat.h |  6 +++++-
>  include/qom/cpu.h   |  3 +++
>  qom/cpu.c           | 13 +++++++++++++
>  5 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 0122ef7..79e1dcb 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>      Error *local_err = NULL;
> +    int migration_id;
>  
>      cpu->as = NULL;
>      cpu->num_ases = 0;
> @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      (void) cc;
>      cpu_list_unlock();
>  #else
> +    migration_id = cc->use_migration_id > 0 ?
> +        cpu->migration_id : cpu->cpu_index;
>      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> +        vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu);
>      }
>      if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +        vmstate_register(NULL, migration_id, cc->vmsd, cpu);
>      }
>  #endif
>  }
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index cd1745e..f041279 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>  }
>  
>  static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> -                          Error **errp)
> +                          Error **errp, int migration_id)
>  {
>      X86CPU *cpu = NULL;
> +    CPUState *cs = CPU(cpu);
>      Error *local_err = NULL;
>

Any reason not to re-use the apic_id as the migration_id?  It's 64-bit
here which is no good, but apic_id_t is defined as uint32_t elsewhere,
so is that just a bug with the function signature?

>      cpu = cpu_x86_create(cpu_model, &local_err);
> @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
>          goto out;
>      }
>  
> +    cs->migration_id = migration_id;
>      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>  
> @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>          return;
>      }
>  
> -    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
> +    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms)
>          pcms->possible_cpus->len++;
>          if (i < smp_cpus) {
>              cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
> -                             &error_fatal);
> +                             &error_fatal, i);
>              pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
>              object_unref(OBJECT(cpu));
>          }
> @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>      NMIClass *nc = NMI_CLASS(oc);
> +    CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU));
>  
> +    cc->use_migration_id = true;
>      pcmc->get_hotplug_handler = mc->get_hotplug_handler;
>      pcmc->pci_enabled = true;
>      pcmc->has_acpi_build = true;
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 636befe..a66e80d 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -2,7 +2,11 @@
>  #define HW_COMPAT_H
>  
>  #define HW_COMPAT_2_6 \
> -    /* empty */
> +    { \
> +        .driver   = TYPE_CPU, \
> +        .property = "use-migration-id", \
> +        .value    = "off", \
> +    },
>  
>  #define HW_COMPAT_2_5 \
>      {\
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 32f3af3..a29a2c9 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -177,6 +177,8 @@ typedef struct CPUClass {
>                                  void *opaque);
>  
>      const struct VMStateDescription *vmsd;
> +    int use_migration_id;
> +
>      int gdb_num_core_regs;
>      const char *gdb_core_xml_file;
>      gchar * (*gdb_arch_name)(CPUState *cpu);
> @@ -360,6 +362,7 @@ 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;

Not really in scope for this patch, but I do wonder if
vmstate_register() should actually take a uint32_t instead of an int,
since that's what it is on the wire, IIUC.

>  };
>  
>  QTAILQ_HEAD(CPUTailQ, CPUState);
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 751e992..3eb48b9 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -342,11 +342,24 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
>      return cpu->cpu_index;
>  }
>  
> +static void cpu_common_set_use_migration_id(Object *obj, bool value,
> +                                            Error **err)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(obj);
> +    cc->use_migration_id = value ? 1 : 0;
> +}
> +
>  static void cpu_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      CPUClass *k = CPU_CLASS(klass);
>  
> +    k->use_migration_id = -1;
> +    object_class_property_add_bool(klass, "use-migration-id",
> +                                   NULL,
> +                                   cpu_common_set_use_migration_id,
> +                                   &error_abort);
> +
>      k->class_by_name = cpu_common_class_by_name;
>      k->parse_features = cpu_common_parse_features;
>      k->reset = cpu_common_reset;

-- 
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 v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[David Gibson]

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

On Mon, Jul 11, 2016 at 09:58:21AM +0200, Igor Mammedov wrote:
> On Mon, 11 Jul 2016 13:22:37 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote:
> > > On Fri, 8 Jul 2016 15:19:58 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote:  
> > > > > Add CPUState::stable_cpu_id and use that as instance_id in
> > > > > vmstate_register() call.
> > > > > 
> > > > > Introduce has-stable_cpu_id property that allows target machines to
> > > > > optionally switch to using stable_cpu_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.
> > > > > 
> > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > >  exec.c            | 6 ++++--
> > > > >  include/qom/cpu.h | 5 +++++
> > > > >  qom/cpu.c         | 6 ++++++
> > > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/exec.c b/exec.c
> > > > > index fb73910..3b36fe5 100644
> > > > > --- a/exec.c
> > > > > +++ b/exec.c
> > > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
> > > > >  void cpu_vmstate_register(CPUState *cpu)
> > > > >  {
> > > > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > +    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
> > > > > +                      cpu->cpu_index;
> > > > >  
> > > > >      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 331386f..527c021 100644
> > > > > --- a/include/qom/cpu.h
> > > > > +++ b/include/qom/cpu.h
> > > > > @@ -273,6 +273,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.
> > > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
> > > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
> > > > > + *     over cpu_index during vmstate registration.
> > > > >   *
> > > > >   * State of one CPU core or thread.
> > > > >   */
> > > > > @@ -360,6 +363,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 stable_cpu_id;
> > > > > +    bool has_stable_cpu_id;
> > > > >  };
> > > > >  
> > > > >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > > > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > > > index 1095ea1..bae1bf7 100644
> > > > > --- a/qom/cpu.c
> > > > > +++ b/qom/cpu.c
> > > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
> > > > >      return cpu->cpu_index;
> > > > >  }
> > > > >  
> > > > > +static Property cpu_common_properties[] = {
> > > > > +    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),    
> > > > 
> > > > It seems odd to me that stable_cpu_id itself isn't exposed as a
> > > > property.  Even if we don't need to set it externally for now, it
> > > > really should be QOM introspectable.  
> > > Should it? Why?  
> > 
> > Well, for one thing it's really strange to have the boolean flag
> > exposed, but not the value itself.
> property doesn't always means that it's intended as an external interface
> 
> > 
> > > It's QEMU internal detail and outside world preferably shouldn't
> > > know anything about it.  
> > 
> > Hrm.. I guess kinda.  But I think it's less an internal detail than
> > the existing cpu_index is.
> so it' better not to start to advertise it as an external interface.

Right.  My comments were based on the assumption that this was
intended as some sort of generally useful stable CPU id, rather than
something narrowly focussed on migration.

Having understood things better from our IRC discussion, I withdraw
this objection.  Things also make more sense to me once this is made a
class property instead of an instance property, which you've done in
your proposed patches.

> Should be add some flag to generic property to mark it as internal?

If such a thing exists that would be good.

-- 
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] VARIANT 1: reuse device compat logic to pick preffered CPU's migration instance_id

[Bharata B Rao]

On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote:
> this approach i I preffer as it uses less per machine migration glue
> and follows typical compat pattern for devices
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  exec.c              |  7 +++++--
>  hw/i386/pc.c        | 10 +++++++---
>  include/hw/compat.h |  6 +++++-
>  include/qom/cpu.h   |  3 +++
>  qom/cpu.c           | 13 +++++++++++++
>  5 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 0122ef7..79e1dcb 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>      Error *local_err = NULL;
> +    int migration_id;
> 
>      cpu->as = NULL;
>      cpu->num_ases = 0;
> @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      (void) cc;
>      cpu_list_unlock();
>  #else
> +    migration_id = cc->use_migration_id > 0 ?
> +        cpu->migration_id : cpu->cpu_index;

cc->use_migration_id is always -1 here (See explanation below)

>      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> +        vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu);
>      }
>      if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +        vmstate_register(NULL, migration_id, cc->vmsd, cpu);
>      }
>  #endif
>  }
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index cd1745e..f041279 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>  }
> 
>  static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> -                          Error **errp)
> +                          Error **errp, int migration_id)
>  {
>      X86CPU *cpu = NULL;
> +    CPUState *cs = CPU(cpu);
>      Error *local_err = NULL;
> 
>      cpu = cpu_x86_create(cpu_model, &local_err);

Assignment to cs should move here after cpu_x86_create.

> @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
>          goto out;
>      }
> 
> +    cs->migration_id = migration_id;
>      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> 
> @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>          return;
>      }
> 
> -    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
> +    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms)
>          pcms->possible_cpus->len++;
>          if (i < smp_cpus) {
>              cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
> -                             &error_fatal);
> +                             &error_fatal, i);
>              pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
>              object_unref(OBJECT(cpu));
>          }
> @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>      NMIClass *nc = NMI_CLASS(oc);
> +    CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU));
> 
> +    cc->use_migration_id = true;

It is not clear as to how setting cc->use_migration_id here in machine class
init would affect all CPUs. What I am observing is that the cc that we
set here is different from the cc that we get via GET_CPU_CLASS(cpu) in
cpu_exec_init(). Hence use_migration_id is never set for any CPUs that
get created.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH] VARIANT 1: reuse device compat logic to pick preffered CPU's migration instance_id

[Igor Mammedov]

On Tue, 12 Jul 2016 15:07:09 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote:
> > this approach i I preffer as it uses less per machine migration glue
> > and follows typical compat pattern for devices
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>  
> 
> After our IRC discussion last night, I prefer this version as well.
> We can definitely work with this on Power as well.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Bharata,

Could you take this patch but strip out PC parts which were meant
as usage example only?

I'll post a separate PC patch on top of it as part of or on top of
cpu-hotplug for PC, as that series changes pc.c significantly and
resulting PC part of patch will look a bit simpler/different.

> 
> One query regarding the x86 specific implementation below, though.
> 
> > ---
> >  exec.c              |  7 +++++--
> >  hw/i386/pc.c        | 10 +++++++---
> >  include/hw/compat.h |  6 +++++-
> >  include/qom/cpu.h   |  3 +++
> >  qom/cpu.c           | 13 +++++++++++++
> >  5 files changed, 33 insertions(+), 6 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index 0122ef7..79e1dcb 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
> >  {
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
> >      Error *local_err = NULL;
> > +    int migration_id;
> >  
> >      cpu->as = NULL;
> >      cpu->num_ases = 0;
> > @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
> >      (void) cc;
> >      cpu_list_unlock();
> >  #else
> > +    migration_id = cc->use_migration_id > 0 ?
> > +        cpu->migration_id : cpu->cpu_index;
> >      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> > +        vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu);
> >      }
> >      if (cc->vmsd != NULL) {
> > -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > +        vmstate_register(NULL, migration_id, cc->vmsd, cpu);
> >      }
> >  #endif
> >  }
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index cd1745e..f041279 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
> >  }
> >  
> >  static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> > -                          Error **errp)
> > +                          Error **errp, int migration_id)
> >  {
> >      X86CPU *cpu = NULL;
> > +    CPUState *cs = CPU(cpu);
> >      Error *local_err = NULL;
> >  
> 
> Any reason not to re-use the apic_id as the migration_id?  It's 64-bit
> here which is no good, but apic_id_t is defined as uint32_t elsewhere,
> so is that just a bug with the function signature?
It could be apic_id but I chose to use possible_cpus index to make code
similar as much as possible so that when I start adding cpu-hotplug to ARM
it would be possible to reuse some of x86 code that could be generalized
shared between both archs.

> >      cpu = cpu_x86_create(cpu_model, &local_err);
> > @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> >          goto out;
> >      }
> >  
> > +    cs->migration_id = migration_id;
> >      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
> >      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> >  
> > @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
> >          return;
> >      }
> >  
> > -    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
> > +    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> > @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms)
> >          pcms->possible_cpus->len++;
> >          if (i < smp_cpus) {
> >              cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
> > -                             &error_fatal);
> > +                             &error_fatal, i);
> >              pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
> >              object_unref(OBJECT(cpu));
> >          }
> > @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >      PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
> >      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> >      NMIClass *nc = NMI_CLASS(oc);
> > +    CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU));
> >  
> > +    cc->use_migration_id = true;
> >      pcmc->get_hotplug_handler = mc->get_hotplug_handler;
> >      pcmc->pci_enabled = true;
> >      pcmc->has_acpi_build = true;
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 636befe..a66e80d 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -2,7 +2,11 @@
> >  #define HW_COMPAT_H
> >  
> >  #define HW_COMPAT_2_6 \
> > -    /* empty */
> > +    { \
> > +        .driver   = TYPE_CPU, \
> > +        .property = "use-migration-id", \
> > +        .value    = "off", \
> > +    },
> >  
> >  #define HW_COMPAT_2_5 \
> >      {\
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 32f3af3..a29a2c9 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -177,6 +177,8 @@ typedef struct CPUClass {
> >                                  void *opaque);
> >  
> >      const struct VMStateDescription *vmsd;
> > +    int use_migration_id;
> > +
> >      int gdb_num_core_regs;
> >      const char *gdb_core_xml_file;
> >      gchar * (*gdb_arch_name)(CPUState *cpu);
> > @@ -360,6 +362,7 @@ 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;  
> 
> Not really in scope for this patch, but I do wonder if
> vmstate_register() should actually take a uint32_t instead of an int,
> since that's what it is on the wire, IIUC.
I think vmstate_register() uses -1 as a way to detect if it should
do instance_id auto assignment.
Any way I'll leave answer to this this question to migration experts.


> 
> >  };
> >  
> >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > diff --git a/qom/cpu.c b/qom/cpu.c
> > index 751e992..3eb48b9 100644
> > --- a/qom/cpu.c
> > +++ b/qom/cpu.c
> > @@ -342,11 +342,24 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
> >      return cpu->cpu_index;
> >  }
> >  
> > +static void cpu_common_set_use_migration_id(Object *obj, bool value,
> > +                                            Error **err)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(obj);
> > +    cc->use_migration_id = value ? 1 : 0;
> > +}
> > +
> >  static void cpu_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >      CPUClass *k = CPU_CLASS(klass);
> >  
> > +    k->use_migration_id = -1;
> > +    object_class_property_add_bool(klass, "use-migration-id",
> > +                                   NULL,
> > +                                   cpu_common_set_use_migration_id,
> > +                                   &error_abort);
> > +
> >      k->class_by_name = cpu_common_class_by_name;
> >      k->parse_features = cpu_common_parse_features;
> >      k->reset = cpu_common_reset;  
> 

Re: [Qemu-devel] [PATCH] VARIANT 1: reuse device compat logic to pick preffered CPU's migration instance_id

[Igor Mammedov]

On Tue, 12 Jul 2016 12:36:17 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote:
> > this approach i I preffer as it uses less per machine migration glue
> > and follows typical compat pattern for devices
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  exec.c              |  7 +++++--
> >  hw/i386/pc.c        | 10 +++++++---
> >  include/hw/compat.h |  6 +++++-
> >  include/qom/cpu.h   |  3 +++
> >  qom/cpu.c           | 13 +++++++++++++
> >  5 files changed, 33 insertions(+), 6 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index 0122ef7..79e1dcb 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
> >  {
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
> >      Error *local_err = NULL;
> > +    int migration_id;
> > 
> >      cpu->as = NULL;
> >      cpu->num_ases = 0;
> > @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
> >      (void) cc;
> >      cpu_list_unlock();
> >  #else
> > +    migration_id = cc->use_migration_id > 0 ?
> > +        cpu->migration_id : cpu->cpu_index;  
> 
> cc->use_migration_id is always -1 here (See explanation below)
> 
> >      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> > +        vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu);
> >      }
> >      if (cc->vmsd != NULL) {
> > -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > +        vmstate_register(NULL, migration_id, cc->vmsd, cpu);
> >      }
> >  #endif
> >  }
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index cd1745e..f041279 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
> >  }
> > 
> >  static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> > -                          Error **errp)
> > +                          Error **errp, int migration_id)
> >  {
> >      X86CPU *cpu = NULL;
> > +    CPUState *cs = CPU(cpu);
> >      Error *local_err = NULL;
> > 
> >      cpu = cpu_x86_create(cpu_model, &local_err);  
> 
> Assignment to cs should move here after cpu_x86_create.
thanks, I'll fix it, though there won't be this hunk in at all
in the end PC version in top of cpu-hotplug series.

> 
> > @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> >          goto out;
> >      }
> > 
> > +    cs->migration_id = migration_id;
> >      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
> >      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> > 
> > @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
> >          return;
> >      }
> > 
> > -    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
> > +    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> > @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms)
> >          pcms->possible_cpus->len++;
> >          if (i < smp_cpus) {
> >              cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
> > -                             &error_fatal);
> > +                             &error_fatal, i);
> >              pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
> >              object_unref(OBJECT(cpu));
> >          }
> > @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >      PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
> >      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> >      NMIClass *nc = NMI_CLASS(oc);
> > +    CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU));
> > 
> > +    cc->use_migration_id = true;  
> 
> It is not clear as to how setting cc->use_migration_id here in machine class
> init would affect all CPUs. What I am observing is that the cc that we
> set here is different from the cc that we get via GET_CPU_CLASS(cpu) in
> cpu_exec_init(). Hence use_migration_id is never set for any CPUs that
> get created.
It look like inherited types are initialized before this is called,
I'll try find solution and post fixed patch shortly.

> 
> Regards,
> Bharata.
> 

[Qemu-devel] [PATCH v2 1/2] cpu: add migration_id to allow board to provide migration instance_id

[Igor Mammedov]

Migration breaks if not the last added CPU is removed due to fact
that when target QEMU starts it uses cpu_index - 1 as migration
instance_id for all CPUs that where added after the removed CPU.

Introduce compat property use-migration-id and CPUState::migration_id
field to allow board provide an id that doesn't change regardless
of the order CPUs are created so that instance_ids on source and
target QEMU will match.

By default use-migration-id is enabled however CPUState::migration_id
isn't set (-1) which forces cpu_exec_init() to use legacy cpu_index
so that we don't have to fix all the targets that doesn't support
device_add/device_del for CPUs and/or device compat logic.

And only targets that whish to support device_add/del for CPUs
will set CPUState::migration_id explicitly allwoing them to use
stable migration id.

If boards sets CPUState::migration_id explictly then for compatibility
with old machine types use-migration-id property should be turned off
to force usage of cpu_index as instance_id for machine types
that used cpu_index for instance_id.

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

diff --git a/exec.c b/exec.c
index 0122ef7..0eda24b 100644
--- a/exec.c
+++ b/exec.c
@@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
+    int migration_id;
 
     cpu->as = NULL;
     cpu->num_ases = 0;
@@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
     (void) cc;
     cpu_list_unlock();
 #else
+    migration_id = cc->use_migration_id  && cpu->migration_id != -1 ?
+        cpu->migration_id : cpu->cpu_index;
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
+        vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu);
     }
     if (cc->vmsd != NULL) {
-        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
+        vmstate_register(NULL, migration_id, cc->vmsd, cpu);
     }
 #endif
 }
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index cacb100..7058fc4 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -115,6 +115,8 @@ struct TranslationBlock;
  * @write_elf32_qemunote: Callback for writing a CPU- and QEMU-specific ELF
  * note to a 32-bit VM coredump.
  * @vmsd: State description for migration.
+ * @use_migration_id: Set to use @migration_id if has been set instead of
+ *      cpu_index during vmstate registration.
  * @gdb_num_core_regs: Number of core registers accessible to GDB.
  * @gdb_core_xml_file: File name for core registers GDB XML description.
  * @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
@@ -177,6 +179,8 @@ typedef struct CPUClass {
                                 void *opaque);
 
     const struct VMStateDescription *vmsd;
+    bool use_migration_id;
+
     int gdb_num_core_regs;
     const char *gdb_core_xml_file;
     gchar * (*gdb_arch_name)(CPUState *cpu);
@@ -273,6 +277,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
+ *      by default -1, which forces fallback to using legacy cpu_index even if
+ *      use_migration_id is true
  *
  * State of one CPU core or thread.
  */
@@ -360,6 +367,7 @@ 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;
 };
 
 QTAILQ_HEAD(CPUTailQ, CPUState);
diff --git a/qom/cpu.c b/qom/cpu.c
index a9727a1..480e0c6 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -357,11 +357,24 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
     return cpu->cpu_index;
 }
 
+static void cpu_common_set_use_migration_id(Object *obj, bool value,
+                                            Error **err)
+{
+    CPUClass *cc = CPU_GET_CLASS(obj);
+    cc->use_migration_id = value;
+}
+
 static void cpu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     CPUClass *k = CPU_CLASS(klass);
 
+    k->use_migration_id = true;
+    object_class_property_add_bool(klass, "use-migration-id",
+                                   NULL,
+                                   cpu_common_set_use_migration_id,
+                                   &error_abort);
+
     k->class_by_name = cpu_common_class_by_name;
     k->parse_features = cpu_common_parse_features;
     k->reset = cpu_common_reset;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/2] pc: fix migration failure after cpu hot-unplung

[Igor Mammedov]

As described in commit:
 cpu: add migration_id to allow board to provide migration instance_id
set CPUState::migration_id explictly to stable index in pcms->possible_cpus
so that CPU's instance_id on target would match instance_id on source
even if not the last added CPU is removed.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c         | 9 ++++++---
 include/hw/i386/pc.h | 5 +++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f56e225..9e72eb3 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1040,13 +1040,16 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 }
 
 static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
-                          Error **errp)
+                          Error **errp, int migration_id)
 {
     X86CPU *cpu = NULL;
+    CPUState *cs;
     Error *local_err = NULL;
 
     cpu = X86_CPU(object_new(typename));
 
+    cs = CPU(cpu);
+    cs->migration_id = migration_id;
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
 
@@ -1092,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 
     assert(pcms->possible_cpus->cpus[0].cpu); /* BSP is always present */
     oc = OBJECT_CLASS(CPU_GET_CLASS(pcms->possible_cpus->cpus[0].cpu));
-    cpu = pc_new_cpu(object_class_get_name(oc), apic_id, &local_err);
+    cpu = pc_new_cpu(object_class_get_name(oc), apic_id, &local_err, id);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -1156,7 +1159,7 @@ void pc_cpus_init(PCMachineState *pcms)
         pcms->possible_cpus->len++;
         if (i < smp_cpus) {
             cpu = pc_new_cpu(typename, x86_cpu_apic_id_from_index(i),
-                             &error_fatal);
+                             &error_fatal, i);
             pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
             object_unref(OBJECT(cpu));
         }
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 2123532..fea2961 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -373,6 +373,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = "vmxnet3",\
         .property = "romfile",\
         .value    = "",\
+    },\
+    { \
+        .driver   = TYPE_CPU, \
+        .property = "use-migration-id", \
+        .value    = "off", \
     },
 
 #define PC_COMPAT_2_5 \
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] VARIANT 1: reuse device compat logic to pick preffered CPU's migration instance_id

[David Gibson]

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

On Tue, Jul 12, 2016 at 10:11:58AM +0200, Igor Mammedov wrote:
> On Tue, 12 Jul 2016 15:07:09 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote:
> > > this approach i I preffer as it uses less per machine migration glue
> > > and follows typical compat pattern for devices
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>  
> > 
> > After our IRC discussion last night, I prefer this version as well.
> > We can definitely work with this on Power as well.
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Bharata,
> 
> Could you take this patch but strip out PC parts which were meant
> as usage example only?
> 
> I'll post a separate PC patch on top of it as part of or on top of
> cpu-hotplug for PC, as that series changes pc.c significantly and
> resulting PC part of patch will look a bit simpler/different.
> 
> > 
> > One query regarding the x86 specific implementation below, though.
> > 
> > > ---
> > >  exec.c              |  7 +++++--
> > >  hw/i386/pc.c        | 10 +++++++---
> > >  include/hw/compat.h |  6 +++++-
> > >  include/qom/cpu.h   |  3 +++
> > >  qom/cpu.c           | 13 +++++++++++++
> > >  5 files changed, 33 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/exec.c b/exec.c
> > > index 0122ef7..79e1dcb 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
> > >  {
> > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > >      Error *local_err = NULL;
> > > +    int migration_id;
> > >  
> > >      cpu->as = NULL;
> > >      cpu->num_ases = 0;
> > > @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
> > >      (void) cc;
> > >      cpu_list_unlock();
> > >  #else
> > > +    migration_id = cc->use_migration_id > 0 ?
> > > +        cpu->migration_id : cpu->cpu_index;
> > >      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> > > +        vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu);
> > >      }
> > >      if (cc->vmsd != NULL) {
> > > -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > > +        vmstate_register(NULL, migration_id, cc->vmsd, cpu);
> > >      }
> > >  #endif
> > >  }
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index cd1745e..f041279 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
> > >  }
> > >  
> > >  static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> > > -                          Error **errp)
> > > +                          Error **errp, int migration_id)
> > >  {
> > >      X86CPU *cpu = NULL;
> > > +    CPUState *cs = CPU(cpu);
> > >      Error *local_err = NULL;
> > >  
> > 
> > Any reason not to re-use the apic_id as the migration_id?  It's 64-bit
> > here which is no good, but apic_id_t is defined as uint32_t elsewhere,
> > so is that just a bug with the function signature?
> It could be apic_id but I chose to use possible_cpus index to make code
> similar as much as possible so that when I start adding cpu-hotplug to ARM
> it would be possible to reuse some of x86 code that could be generalized
> shared between both archs.

Ok.

> > >      cpu = cpu_x86_create(cpu_model, &local_err);
> > > @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> > >          goto out;
> > >      }
> > >  
> > > +    cs->migration_id = migration_id;
> > >      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
> > >      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> > >  
> > > @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
> > >          return;
> > >      }
> > >  
> > > -    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
> > > +    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id);
> > >      if (local_err) {
> > >          error_propagate(errp, local_err);
> > >          return;
> > > @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms)
> > >          pcms->possible_cpus->len++;
> > >          if (i < smp_cpus) {
> > >              cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
> > > -                             &error_fatal);
> > > +                             &error_fatal, i);
> > >              pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
> > >              object_unref(OBJECT(cpu));
> > >          }
> > > @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> > >      PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
> > >      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> > >      NMIClass *nc = NMI_CLASS(oc);
> > > +    CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU));
> > >  
> > > +    cc->use_migration_id = true;
> > >      pcmc->get_hotplug_handler = mc->get_hotplug_handler;
> > >      pcmc->pci_enabled = true;
> > >      pcmc->has_acpi_build = true;
> > > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > > index 636befe..a66e80d 100644
> > > --- a/include/hw/compat.h
> > > +++ b/include/hw/compat.h
> > > @@ -2,7 +2,11 @@
> > >  #define HW_COMPAT_H
> > >  
> > >  #define HW_COMPAT_2_6 \
> > > -    /* empty */
> > > +    { \
> > > +        .driver   = TYPE_CPU, \
> > > +        .property = "use-migration-id", \
> > > +        .value    = "off", \
> > > +    },
> > >  
> > >  #define HW_COMPAT_2_5 \
> > >      {\
> > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > > index 32f3af3..a29a2c9 100644
> > > --- a/include/qom/cpu.h
> > > +++ b/include/qom/cpu.h
> > > @@ -177,6 +177,8 @@ typedef struct CPUClass {
> > >                                  void *opaque);
> > >  
> > >      const struct VMStateDescription *vmsd;
> > > +    int use_migration_id;
> > > +
> > >      int gdb_num_core_regs;
> > >      const char *gdb_core_xml_file;
> > >      gchar * (*gdb_arch_name)(CPUState *cpu);
> > > @@ -360,6 +362,7 @@ 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;  
> > 
> > Not really in scope for this patch, but I do wonder if
> > vmstate_register() should actually take a uint32_t instead of an int,
> > since that's what it is on the wire, IIUC.
> I think vmstate_register() uses -1 as a way to detect if it should
> do instance_id auto assignment.
> Any way I'll leave answer to this this question to migration
> > experts.


Ah, good point.

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