[Qemu-devel] [RFC PATCH v2 0/9] Core based CPU hotplug for PowerPC sPAPR

[Qemu-devel] [RFC PATCH v2 0/9] Core based CPU hotplug for PowerPC sPAPR

[Bharata B Rao]

Hi,

This is the next version of "Core based CPU hotplug for PowerPC sPAPR" that
was posted at
https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html

device_add semantics
--------------------
For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
(qemu) device_add spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]

Major changes in this version
-----------------------------
- Based on the review feedback, removed the links from machine object
  to the core objects.
- With that, the concept of using the links as slots where core object sits
  is gone.
- String slot name which was being used as slot= with device_add now
  becomes an integer core id being specified as core=.
- threads property which indicates the number of threads in the core
  moves from spapr-cpu-core type to cpu-core type.
- Threads creation moves from core's property setter to core's realizefn.
- Igor's proposed pre_plug handler isn't yet used in this patchset, but it
  wouldn't take much effort to switch to it. Waiting for some review/consensus
  on Igor's patchset before switching to it.
- This patchset will now work with Igor's query-hotpluggable-cpus QMP
  interface.

Other changes
-------------
- Core ID that is used with device_add is in fact device tree ID now.
- DRC indexes are based on core_dt_id now. There are a couple of places
  where core device's thread0 is used to fetch the DRC index, but changing
  that requires bigger change of converting the CPUs DT code generation
  to iterate over cores instead of threads.
- Coverted while(1) to do-while() as suggeted by Thomas Huth (3/9).
- Creation of spapr-cpu-core device and conversion of boot CPUs into
  cores merged into a single patch (6/9).
- Topologies with incomplete cores are prevented from booting only with
  machine type versions that support CPU DR (6/9).
- Conversion of boot CPUs into cores is done only for machine type versions
  that support CPU DR (6/9).
- Take care of recovery from failure in plug handler when CPU hotplug isn't
  supported correctly. This will not be needed when we prevent such
  attempts from pre_plug handler (9/9).
Bharata B Rao (8):
  exec: Remove cpu from cpus list during cpu_exec_exit()
  exec: Do vmstate unregistration from cpu_exec_exit()
  cpu: Add a sync version of cpu_remove()
  cpu: Abstract CPU core type
  spapr: CPU core device
  spapr: CPU hotplug support
  xics,xics_kvm: Handle CPU unplug correctly
  spapr: CPU hot unplug support

Gu Zheng (1):
  cpu: Reclaim vCPU objects

 cpus.c                          |  51 +++++-
 exec.c                          |  41 ++++-
 hw/cpu/Makefile.objs            |   1 +
 hw/cpu/core.c                   |  87 ++++++++++
 hw/intc/xics.c                  |  14 ++
 hw/intc/xics_kvm.c              |   8 +-
 hw/ppc/Makefile.objs            |   1 +
 hw/ppc/spapr.c                  | 153 +++++++++++++++--
 hw/ppc/spapr_cpu_core.c         | 354 ++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_events.c           |   3 +
 hw/ppc/spapr_rtas.c             |  24 +++
 include/hw/cpu/core.h           |  31 ++++
 include/hw/ppc/spapr.h          |   7 +
 include/hw/ppc/spapr_cpu_core.h |  42 +++++
 include/hw/ppc/xics.h           |   1 +
 include/qom/cpu.h               |  18 ++
 include/sysemu/kvm.h            |   1 +
 kvm-all.c                       |  57 ++++++-
 kvm-stub.c                      |   5 +
 19 files changed, 871 insertions(+), 28 deletions(-)
 create mode 100644 hw/cpu/core.c
 create mode 100644 hw/ppc/spapr_cpu_core.c
 create mode 100644 include/hw/cpu/core.h
 create mode 100644 include/hw/ppc/spapr_cpu_core.h

-- 
2.1.0

[Qemu-devel] [RFC PATCH v2 1/9] exec: Remove cpu from cpus list during cpu_exec_exit()

[Bharata B Rao]

CPUState *cpu gets added to the cpus list during cpu_exec_init(). It
should be removed from cpu_exec_exit().

cpu_exec_exit() is called from generic CPU::instance_finalize and some
archs like PowerPC call it from CPU unrealizefn. So ensure that we
dequeue the cpu only once.

Now -1 value for cpu->cpu_index indicates that we have already dequeued
the cpu for CONFIG_USER_ONLY case also.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 exec.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index f09dd4e..49ae593 100644
--- a/exec.c
+++ b/exec.c
@@ -609,15 +609,9 @@ static int cpu_get_free_index(Error **errp)
     return cpu;
 }
 
-void cpu_exec_exit(CPUState *cpu)
+static void cpu_release_index(CPUState *cpu)
 {
-    if (cpu->cpu_index == -1) {
-        /* cpu_index was never allocated by this @cpu or was already freed. */
-        return;
-    }
-
     bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
-    cpu->cpu_index = -1;
 }
 #else
 
@@ -632,11 +626,33 @@ static int cpu_get_free_index(Error **errp)
     return cpu_index;
 }
 
-void cpu_exec_exit(CPUState *cpu)
+static void cpu_release_index(CPUState *cpu)
 {
+    return;
 }
 #endif
 
+void cpu_exec_exit(CPUState *cpu)
+{
+#if defined(CONFIG_USER_ONLY)
+    cpu_list_lock();
+#endif
+    if (cpu->cpu_index == -1) {
+        /* cpu_index was never allocated by this @cpu or was already freed. */
+#if defined(CONFIG_USER_ONLY)
+        cpu_list_unlock();
+#endif
+        return;
+    }
+
+    QTAILQ_REMOVE(&cpus, cpu, node);
+    cpu_release_index(cpu);
+    cpu->cpu_index = -1;
+#if defined(CONFIG_USER_ONLY)
+    cpu_list_unlock();
+#endif
+}
+
 void cpu_exec_init(CPUState *cpu, Error **errp)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
-- 
2.1.0

[Qemu-devel] [RFC PATCH v2 2/9] exec: Do vmstate unregistration from cpu_exec_exit()

[Bharata B Rao]

cpu_exec_init() does vmstate_register for the CPU device. This needs to be
undone from cpu_exec_exit(). This change is needed to support CPU hot
removal.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 exec.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/exec.c b/exec.c
index 49ae593..8ddca6b 100644
--- a/exec.c
+++ b/exec.c
@@ -634,6 +634,8 @@ static void cpu_release_index(CPUState *cpu)
 
 void cpu_exec_exit(CPUState *cpu)
 {
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
 #if defined(CONFIG_USER_ONLY)
     cpu_list_lock();
 #endif
@@ -651,6 +653,13 @@ 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)
-- 
2.1.0

[Qemu-devel] [RFC PATCH v2 3/9] cpu: Reclaim vCPU objects

[Bharata B Rao]

From: Gu Zheng <guz.fnst@cn.fujitsu.com>

In order to deal well with the kvm vcpus (which can not be removed without any
protection), we do not close KVM vcpu fd, just record and mark it as stopped
into a list, so that we can reuse it for the appending cpu hot-add request if
possible. It is also the approach that kvm guys suggested:
https://www.mail-archive.com/kvm@vger.kernel.org/msg102839.html

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
               [- Explicit CPU_REMOVE() from qemu_kvm/tcg_destroy_vcpu()
                  isn't needed as it is done from cpu_exec_exit()
                - Use iothread mutex instead of global mutex during
                  destroy
                - Don't cleanup vCPU object from vCPU thread context
                  but leave it to the callers (device_add/device_del)]
---
 cpus.c               | 39 +++++++++++++++++++++++++++++++++--
 include/qom/cpu.h    | 10 +++++++++
 include/sysemu/kvm.h |  1 +
 kvm-all.c            | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 kvm-stub.c           |  5 +++++
 5 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/cpus.c b/cpus.c
index bc774e2..be0ac6a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -953,6 +953,18 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
     qemu_cpu_kick(cpu);
 }
 
+static void qemu_kvm_destroy_vcpu(CPUState *cpu)
+{
+    if (kvm_destroy_vcpu(cpu) < 0) {
+        error_report("kvm_destroy_vcpu failed");
+        exit(EXIT_FAILURE);
+    }
+}
+
+static void qemu_tcg_destroy_vcpu(CPUState *cpu)
+{
+}
+
 static void flush_queued_work(CPUState *cpu)
 {
     struct qemu_work_item *wi;
@@ -1045,7 +1057,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
     cpu->created = true;
     qemu_cond_signal(&qemu_cpu_cond);
 
-    while (1) {
+    do {
         if (cpu_can_run(cpu)) {
             r = kvm_cpu_exec(cpu);
             if (r == EXCP_DEBUG) {
@@ -1053,8 +1065,10 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
             }
         }
         qemu_kvm_wait_io_event(cpu);
-    }
+    } while (!cpu->unplug || cpu_can_run(cpu));
 
+    qemu_kvm_destroy_vcpu(cpu);
+    qemu_mutex_unlock_iothread();
     return NULL;
 }
 
@@ -1108,6 +1122,7 @@ static void tcg_exec_all(void);
 static void *qemu_tcg_cpu_thread_fn(void *arg)
 {
     CPUState *cpu = arg;
+    CPUState *remove_cpu = NULL;
 
     rcu_register_thread();
 
@@ -1145,6 +1160,16 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
             }
         }
         qemu_tcg_wait_io_event(QTAILQ_FIRST(&cpus));
+        CPU_FOREACH(cpu) {
+            if (cpu->unplug && !cpu_can_run(cpu)) {
+                remove_cpu = cpu;
+                break;
+            }
+        }
+        if (remove_cpu) {
+            qemu_tcg_destroy_vcpu(remove_cpu);
+            remove_cpu = NULL;
+        }
     }
 
     return NULL;
@@ -1301,6 +1326,13 @@ void resume_all_vcpus(void)
     }
 }
 
+void cpu_remove(CPUState *cpu)
+{
+    cpu->stop = true;
+    cpu->unplug = true;
+    qemu_cpu_kick(cpu);
+}
+
 /* For temporary buffers for forming a name */
 #define VCPU_THREAD_NAME_SIZE 16
 
@@ -1517,6 +1549,9 @@ static void tcg_exec_all(void)
                 break;
             }
         } else if (cpu->stop || cpu->stopped) {
+            if (cpu->unplug) {
+                next_cpu = CPU_NEXT(cpu);
+            }
             break;
         }
     }
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7052eee..0720dd7 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -237,6 +237,7 @@ struct kvm_run;
  * @halted: Nonzero if the CPU is in suspended state.
  * @stop: Indicates a pending stop request.
  * @stopped: Indicates the CPU has been artificially stopped.
+ * @unplug: Indicates a pending CPU unplug request.
  * @crash_occurred: Indicates the OS reported a crash (panic) for this CPU
  * @tcg_exit_req: Set to force TCG to stop executing linked TBs for this
  *           CPU and return to its top level loop.
@@ -289,6 +290,7 @@ struct CPUState {
     bool created;
     bool stop;
     bool stopped;
+    bool unplug;
     bool crash_occurred;
     bool exit_request;
     uint32_t interrupt_request;
@@ -756,6 +758,14 @@ void cpu_exit(CPUState *cpu);
 void cpu_resume(CPUState *cpu);
 
 /**
+ * cpu_remove:
+ * @cpu: The CPU to remove.
+ *
+ * Requests the CPU to be removed.
+ */
+void cpu_remove(CPUState *cpu);
+
+/**
  * qemu_init_vcpu:
  * @cpu: The vCPU to initialize.
  *
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 6695fa7..5d5b602 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -216,6 +216,7 @@ int kvm_has_intx_set_mask(void);
 
 int kvm_init_vcpu(CPUState *cpu);
 int kvm_cpu_exec(CPUState *cpu);
+int kvm_destroy_vcpu(CPUState *cpu);
 
 #ifdef NEED_CPU_H
 
diff --git a/kvm-all.c b/kvm-all.c
index 44c0464..35c0621 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -61,6 +61,12 @@
 
 #define KVM_MSI_HASHTAB_SIZE    256
 
+struct KVMParkedVcpu {
+    unsigned long vcpu_id;
+    int kvm_fd;
+    QLIST_ENTRY(KVMParkedVcpu) node;
+};
+
 struct KVMState
 {
     AccelState parent_obj;
@@ -94,6 +100,7 @@ struct KVMState
     QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
 #endif
     KVMMemoryListener memory_listener;
+    QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus;
 };
 
 KVMState *kvm_state;
@@ -237,6 +244,53 @@ static int kvm_set_user_memory_region(KVMMemoryListener *kml, KVMSlot *slot)
     return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
 }
 
+int kvm_destroy_vcpu(CPUState *cpu)
+{
+    KVMState *s = kvm_state;
+    long mmap_size;
+    struct KVMParkedVcpu *vcpu = NULL;
+    int ret = 0;
+
+    DPRINTF("kvm_destroy_vcpu\n");
+
+    mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
+    if (mmap_size < 0) {
+        ret = mmap_size;
+        DPRINTF("KVM_GET_VCPU_MMAP_SIZE failed\n");
+        goto err;
+    }
+
+    ret = munmap(cpu->kvm_run, mmap_size);
+    if (ret < 0) {
+        goto err;
+    }
+
+    vcpu = g_malloc0(sizeof(*vcpu));
+    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
+    vcpu->kvm_fd = cpu->kvm_fd;
+    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
+err:
+    return ret;
+}
+
+static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
+{
+    struct KVMParkedVcpu *cpu;
+
+    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
+        if (cpu->vcpu_id == vcpu_id) {
+            int kvm_fd;
+
+            QLIST_REMOVE(cpu, node);
+            kvm_fd = cpu->kvm_fd;
+            g_free(cpu);
+            return kvm_fd;
+        }
+    }
+
+    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
+}
+
 int kvm_init_vcpu(CPUState *cpu)
 {
     KVMState *s = kvm_state;
@@ -245,7 +299,7 @@ int kvm_init_vcpu(CPUState *cpu)
 
     DPRINTF("kvm_init_vcpu\n");
 
-    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)kvm_arch_vcpu_id(cpu));
+    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
     if (ret < 0) {
         DPRINTF("kvm_create_vcpu failed\n");
         goto err;
@@ -1495,6 +1549,7 @@ static int kvm_init(MachineState *ms)
 #ifdef KVM_CAP_SET_GUEST_DEBUG
     QTAILQ_INIT(&s->kvm_sw_breakpoints);
 #endif
+    QLIST_INIT(&s->kvm_parked_vcpus);
     s->vmfd = -1;
     s->fd = qemu_open("/dev/kvm", O_RDWR);
     if (s->fd == -1) {
diff --git a/kvm-stub.c b/kvm-stub.c
index b962b24..61f9d5c 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -33,6 +33,11 @@ bool kvm_allowed;
 bool kvm_readonly_mem_allowed;
 bool kvm_ioeventfd_any_length_allowed;
 
+int kvm_destroy_vcpu(CPUState *cpu)
+{
+    return -ENOSYS;
+}
+
 int kvm_init_vcpu(CPUState *cpu)
 {
     return -ENOSYS;
-- 
2.1.0

[Qemu-devel] [RFC PATCH v2 4/9] cpu: Add a sync version of cpu_remove()

[Bharata B Rao]

This sync API will be used by the CPU hotplug code to wait for the CPU to
completely get removed before flagging the failure to the device_add
command.

Sync version of this call is needed to correctly recover from CPU
realization failures when ->plug() handler fails.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 cpus.c            | 12 ++++++++++++
 include/qom/cpu.h |  8 ++++++++
 2 files changed, 20 insertions(+)

diff --git a/cpus.c b/cpus.c
index be0ac6a..05cb096 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1068,6 +1068,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
     } while (!cpu->unplug || cpu_can_run(cpu));
 
     qemu_kvm_destroy_vcpu(cpu);
+    cpu->created = false;
+    qemu_cond_signal(&qemu_cpu_cond);
     qemu_mutex_unlock_iothread();
     return NULL;
 }
@@ -1168,6 +1170,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
         }
         if (remove_cpu) {
             qemu_tcg_destroy_vcpu(remove_cpu);
+            cpu->created = false;
+            qemu_cond_signal(&qemu_cpu_cond);
             remove_cpu = NULL;
         }
     }
@@ -1333,6 +1337,14 @@ void cpu_remove(CPUState *cpu)
     qemu_cpu_kick(cpu);
 }
 
+void cpu_remove_sync(CPUState *cpu)
+{
+    cpu_remove(cpu);
+    while (cpu->created) {
+        qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+    }
+}
+
 /* For temporary buffers for forming a name */
 #define VCPU_THREAD_NAME_SIZE 16
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 0720dd7..6e20119 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -765,6 +765,14 @@ void cpu_resume(CPUState *cpu);
  */
 void cpu_remove(CPUState *cpu);
 
+ /**
+ * cpu_remove_sync:
+ * @cpu: The CPU to remove.
+ *
+ * Requests the CPU to be removed and waits till it is removed.
+ */
+void cpu_remove_sync(CPUState *cpu);
+
 /**
  * qemu_init_vcpu:
  * @cpu: The vCPU to initialize.
-- 
2.1.0

[Qemu-devel] [RFC PATCH v2 5/9] cpu: Abstract CPU core type

[Bharata B Rao]

Add an abstract CPU core type that could be used by machines that want
to define and hotplug CPUs in core granularity.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/cpu/Makefile.objs  |  1 +
 hw/cpu/core.c         | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/cpu/core.h | 31 ++++++++++++++++++
 3 files changed, 119 insertions(+)
 create mode 100644 hw/cpu/core.c
 create mode 100644 include/hw/cpu/core.h

diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
index 0954a18..942a4bb 100644
--- a/hw/cpu/Makefile.objs
+++ b/hw/cpu/Makefile.objs
@@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
 obj-$(CONFIG_REALVIEW) += realview_mpcore.o
 obj-$(CONFIG_A9MPCORE) += a9mpcore.o
 obj-$(CONFIG_A15MPCORE) += a15mpcore.o
+obj-y += core.o
 
diff --git a/hw/cpu/core.c b/hw/cpu/core.c
new file mode 100644
index 0000000..3faf53d
--- /dev/null
+++ b/hw/cpu/core.c
@@ -0,0 +1,87 @@
+/*
+ * CPU core abstract device
+ *
+ * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "hw/cpu/core.h"
+#include "qapi/visitor.h"
+#include "sysemu/cpus.h"
+
+static void core_prop_get_core(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    CPUCore *core = CPU_CORE(obj);
+    int64_t value = core->core;
+
+    visit_type_int(v, name, &value, errp);
+}
+
+static void core_prop_set_core(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    CPUCore *core = CPU_CORE(obj);
+    Error *local_err = NULL;
+    int64_t value;
+
+    visit_type_int(v, name, &value, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    core->core = value;
+}
+
+static void core_prop_get_threads(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    CPUCore *core = CPU_CORE(obj);
+    int64_t value = core->threads;
+
+    visit_type_int(v, name, &value, errp);
+}
+
+static void core_prop_set_threads(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    CPUCore *core = CPU_CORE(obj);
+    Error *local_err = NULL;
+    int64_t value;
+
+    visit_type_int(v, name, &value, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    core->threads = value;
+}
+
+static void cpu_core_instance_init(Object *obj)
+{
+    CPUCore *core = CPU_CORE(obj);
+
+    object_property_add(obj, "core", "int", core_prop_get_core,
+                        core_prop_set_core, NULL, NULL, NULL);
+    object_property_add(obj, "threads", "int", core_prop_get_threads,
+                        core_prop_set_threads, NULL, NULL, NULL);
+    core->threads = smp_threads;
+}
+
+static const TypeInfo cpu_core_type_info = {
+    .name = TYPE_CPU_CORE,
+    .parent = TYPE_DEVICE,
+    .abstract = true,
+    .instance_size = sizeof(CPUCore),
+    .instance_init = cpu_core_instance_init,
+};
+
+static void cpu_core_register_types(void)
+{
+    type_register_static(&cpu_core_type_info);
+}
+
+type_init(cpu_core_register_types)
diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
new file mode 100644
index 0000000..a2a5a04
--- /dev/null
+++ b/include/hw/cpu/core.h
@@ -0,0 +1,31 @@
+/*
+ * CPU core abstract device
+ *
+ * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef HW_CPU_CORE_H
+#define HW_CPU_CORE_H
+
+#include "qemu/osdep.h"
+#include "hw/qdev.h"
+
+#define TYPE_CPU_CORE "cpu-core"
+
+#define CPU_CORE(obj) \
+    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
+
+typedef struct CPUCore {
+    /*< private >*/
+    DeviceState parent_obj;
+
+    /*< public >*/
+    int core;
+    int threads;
+} CPUCore;
+
+#define CPU_CORE_PROP_CORE "core"
+
+#endif
-- 
2.1.0

[Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device

[Bharata B Rao]

Add sPAPR specific CPU core device that is based on generic CPU core device.
Creating this core device will result in creation of all the CPU thread
devices that are part of this core.

Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
topologies that result in partially filled cores. Both of these are done
only if CPU core hotplug is supported.

Note: An unrelated change in the call to xics_system_init() is done
in this patch as it makes sense to use the local variable smt introduced
in this patch instead of kvmppc_smt_threads() call here.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/Makefile.objs            |   1 +
 hw/ppc/spapr.c                  |  68 +++++++++++---
 hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   4 +
 include/hw/ppc/spapr_cpu_core.h |  28 ++++++
 5 files changed, 287 insertions(+), 13 deletions(-)
 create mode 100644 hw/ppc/spapr_cpu_core.c
 create mode 100644 include/hw/ppc/spapr_cpu_core.h

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index c1ffc77..5cc6608 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
 obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
 obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
 obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
+obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
 ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
 obj-y += spapr_pci_vfio.o
 endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 64c4acc..cffe8c8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -64,6 +64,7 @@
 
 #include "hw/compat.h"
 #include "qemu-common.h"
+#include "hw/ppc/spapr_cpu_core.h"
 
 #include <libfdt.h>
 
@@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
 
 }
 
-static void spapr_cpu_reset(void *opaque)
+void spapr_cpu_reset(void *opaque)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     PowerPCCPU *cpu = opaque;
@@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
     machine->boot_order = g_strdup(boot_device);
 }
 
-static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
-                           Error **errp)
+/*
+ * TODO: Check if some of these can be moved to rtas_start_cpu() where
+ * a few other things required for hotplugged CPUs are being done.
+ */
+void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
 {
     CPUPPCState *env = &cpu->env;
 
@@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
-    PowerPCCPU *cpu;
     PCIHostState *phb;
     int i;
     MemoryRegion *sysmem = get_system_memory();
@@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
     long load_limit, fw_size;
     bool kernel_le = false;
     char *filename;
+    int smt = kvmppc_smt_threads();
+    int spapr_cores = smp_cpus / smp_threads;
+    int spapr_max_cores = max_cpus / smp_threads;
+
+    if (smc->dr_cpu_enabled) {
+        if (smp_cpus % smp_threads) {
+            error_report("smp_cpus (%u) must be multiple of threads (%u)",
+                         smp_cpus, smp_threads);
+            exit(1);
+        }
+        if (max_cpus % smp_threads) {
+            error_report("max_cpus (%u) must be multiple of threads (%u)",
+                         max_cpus, smp_threads);
+            exit(1);
+        }
+    }
 
     msi_supported = true;
 
@@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
 
     /* Set up Interrupt Controller before we create the VCPUs */
     spapr->icp = xics_system_init(machine,
-                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
-                                               smp_threads),
+                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),
                                   XICS_IRQS, &error_fatal);
 
     if (smc->dr_lmb_enabled) {
@@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
     if (machine->cpu_model == NULL) {
         machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
     }
-    for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(machine->cpu_model);
-        if (cpu == NULL) {
-            error_report("Unable to find PowerPC CPU definition");
-            exit(1);
+
+    if (smc->dr_cpu_enabled) {
+        spapr->cores = g_new0(Object *, spapr_max_cores);
+
+        for (i = 0; i < spapr_max_cores; i++) {
+            int core_dt_id = i * smt;
+
+            if (i < spapr_cores) {
+                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
+
+                object_property_set_str(core, machine->cpu_model, "cpu_model",
+                                        &error_fatal);
+                object_property_set_int(core, smp_threads, "threads",
+                                        &error_fatal);
+                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
+                                        &error_fatal);
+                object_property_set_bool(core, true, "realized", &error_fatal);
+            }
         }
-        spapr_cpu_init(spapr, cpu, &error_fatal);
+    } else {
+        for (i = 0; i < smp_cpus; i++) {
+            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
+            if (cpu == NULL) {
+                error_report("Unable to find PowerPC CPU definition");
+                exit(1);
+            }
+            spapr_cpu_init(spapr, cpu, &error_fatal);
+       }
     }
 
     if (kvm_enabled()) {
@@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
 static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
                                              DeviceState *dev)
 {
-    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         return HOTPLUG_HANDLER(machine);
     }
     return NULL;
@@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
 
     smc->dr_lmb_enabled = true;
+    smc->dr_cpu_enabled = true;
     fwc->get_dev_path = spapr_get_fw_dev_path;
     nc->nmi_monitor_handler = spapr_nmi;
 }
@@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
 
     spapr_machine_2_6_class_options(mc);
     smc->use_ohci_by_default = true;
+    smc->dr_cpu_enabled = false;
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
 }
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
new file mode 100644
index 0000000..8c6d71d
--- /dev/null
+++ b/hw/ppc/spapr_cpu_core.c
@@ -0,0 +1,199 @@
+/*
+ * sPAPR CPU core device, acts as container of CPU thread devices.
+ *
+ * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "hw/cpu/core.h"
+#include "hw/ppc/spapr_cpu_core.h"
+#include "hw/ppc/spapr.h"
+#include "hw/boards.h"
+#include "qemu/error-report.h"
+#include "qapi/visitor.h"
+#include <sysemu/cpus.h>
+#include "target-ppc/kvm_ppc.h"
+
+static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
+                                          Error **errp)
+{
+    int i;
+    Error *local_err = NULL;
+
+    for (i = 0; i < threads; i++) {
+        char id[32];
+
+        object_initialize(&core->threads[i], sizeof(core->threads[i]),
+                          object_class_get_name(core->oc));
+        snprintf(id, sizeof(id), "thread[%d]", i);
+        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
+                                  &local_err);
+        if (local_err) {
+            goto err;
+        }
+    }
+    return;
+
+err:
+    while (--i) {
+        object_unparent(OBJECT(&core->threads[i]));
+    }
+    error_propagate(errp, local_err);
+}
+
+static int spapr_cpu_core_realize_child(Object *child, void *opaque)
+{
+    Error **errp = opaque;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    CPUState *cs = CPU(child);
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+    object_property_set_bool(child, true, "realized", errp);
+    if (*errp) {
+        return 1;
+    }
+
+    spapr_cpu_init(spapr, cpu, errp);
+    if (*errp) {
+        return 1;
+    }
+
+    spapr_cpu_reset(cpu);
+    return 0;
+}
+
+static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
+{
+    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    int spapr_max_cores = max_cpus / smp_threads;
+    Error *local_err = NULL;
+    int threads = 0;
+    int core_dt_id, core_id;
+    int smt = kvmppc_smt_threads();
+
+    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    if (threads != smp_threads) {
+        error_setg(&local_err, "threads must be %d", smp_threads);
+        goto out;
+    }
+
+    if (!core->oc) {
+        error_setg(&local_err, "cpu_model property isn't set");
+        goto out;
+    }
+
+    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    if (core_dt_id % smt) {
+        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
+        goto out;
+    }
+
+    core_id = core_dt_id / smt;
+    if (core_id < 0 || core_id >= spapr_max_cores) {
+        error_setg(&local_err, "core id %d out of range", core_dt_id);
+        goto out;
+    }
+
+    /*
+     * TODO: This check will be moved to ->pre_plug() as suggested by Igor
+     * when there is consensus pre_plug hook.
+     */
+    if (spapr->cores[core_id]) {
+        error_setg(&local_err, "core %d already populated", core_dt_id);
+        goto out;
+    }
+
+    core->threads = g_new0(PowerPCCPU, threads);
+    spapr_cpu_core_create_threads(core, threads, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    return;
+
+out:
+    if (local_err) {
+        g_free(core->threads);
+        error_propagate(errp, local_err);
+    }
+}
+
+static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
+{
+    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
+
+    /*
+     * TODO: This returns the full type instead of just cpu_model. For eg,
+     * host-powerpc64-cpu is returned where just "host" is expected.
+     */
+    return g_strdup(object_class_get_name(core->oc));
+}
+
+static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
+                                              Error **errp)
+{
+    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
+    MachineState *machine = MACHINE(qdev_get_machine());
+    ObjectClass *oc = cpu_class_by_name(TYPE_POWERPC_CPU, val);
+    ObjectClass *oc_base = cpu_class_by_name(TYPE_POWERPC_CPU,
+                                             machine->cpu_model);
+    if (!oc) {
+        error_setg(errp, "Unknown CPU model %s", val);
+        return;
+    }
+
+    /*
+     * Currently cpu_model can't be different from what is specified with -cpu
+     */
+    if (strcmp(object_class_get_name(oc), object_class_get_name(oc_base))) {
+        error_setg(errp, "cpu_model must be %s", machine->cpu_model);
+        return;
+    }
+
+    core->oc = oc;
+}
+
+static void spapr_cpu_core_instance_init(Object *obj)
+{
+    object_property_add_str(obj, "cpu_model",
+                            spapr_cpu_core_prop_get_cpu_model,
+                            spapr_cpu_core_prop_set_cpu_model,
+                            NULL);
+}
+
+static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = spapr_cpu_core_realize;
+}
+
+static const TypeInfo spapr_cpu_core_type_info = {
+    .name = TYPE_SPAPR_CPU_CORE,
+    .parent = TYPE_CPU_CORE,
+    .instance_init = spapr_cpu_core_instance_init,
+    .instance_size = sizeof(sPAPRCPUCore),
+    .class_init = spapr_cpu_core_class_init,
+};
+
+static void spapr_cpu_core_register_types(void)
+{
+    type_register_static(&spapr_cpu_core_type_info);
+}
+
+type_init(spapr_cpu_core_register_types)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 098d85d..c099c3c 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -36,6 +36,7 @@ struct sPAPRMachineClass {
 
     /*< public >*/
     bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
+    bool dr_cpu_enabled;       /* enable dynamic-reconfig/hotplug of CPUs */
     bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
 };
 
@@ -79,6 +80,7 @@ struct sPAPRMachineState {
     /*< public >*/
     char *kvm_type;
     MemoryHotplugState hotplug_memory;
+    Object **cores;
 };
 
 #define H_SUCCESS         0
@@ -585,6 +587,8 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
                                        uint32_t count);
 void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
                                           uint32_t count);
+void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
+void spapr_cpu_reset(void *opaque);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
new file mode 100644
index 0000000..48fb76a
--- /dev/null
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -0,0 +1,28 @@
+/*
+ * sPAPR CPU core device.
+ *
+ * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef HW_SPAPR_CPU_CORE_H
+#define HW_SPAPR_CPU_CORE_H
+
+#include "hw/qdev.h"
+#include "hw/cpu/core.h"
+
+#define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
+#define SPAPR_CPU_CORE(obj) \
+    OBJECT_CHECK(sPAPRCPUCore, (obj), TYPE_SPAPR_CPU_CORE)
+
+typedef struct sPAPRCPUCore {
+    /*< private >*/
+    CPUCore parent_obj;
+
+    /*< public >*/
+    ObjectClass *oc;
+    PowerPCCPU *threads;
+} sPAPRCPUCore;
+
+#endif
-- 
2.1.0

[Qemu-devel] [RFC PATCH v2 7/9] spapr: CPU hotplug support

[Bharata B Rao]

Set up device tree entries for the hotplugged CPU core and use the
exising RTAS event logging infrastructure to send CPU hotplug notification
to the guest.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c                  | 64 ++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_cpu_core.c         | 69 +++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_events.c           |  3 ++
 hw/ppc/spapr_rtas.c             | 24 ++++++++++++++
 include/hw/ppc/spapr.h          |  2 ++
 include/hw/ppc/spapr_cpu_core.h |  2 ++
 6 files changed, 164 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cffe8c8..822c87d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -603,6 +603,18 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
     size_t page_sizes_prop_size;
     uint32_t vcpus_per_socket = smp_threads * smp_cores;
     uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+    int drc_index;
+
+    if (smc->dr_cpu_enabled) {
+        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
+        g_assert(drc);
+        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+        drc_index = drck->get_index(drc);
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
+    }
 
     /* Note: we keep CI large pages off for now because a 64K capable guest
      * provisioned with large pages might otherwise try to map a qemu
@@ -987,6 +999,16 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
         _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
     }
 
+    if (smc->dr_cpu_enabled) {
+        int offset = fdt_path_offset(fdt, "/cpus");
+        ret = spapr_drc_populate_dt(fdt, offset, NULL,
+                                    SPAPR_DR_CONNECTOR_TYPE_CPU);
+        if (ret < 0) {
+            error_report("Couldn't set up CPU DR device tree properties");
+            exit(1);
+        }
+    }
+
     _FDT((fdt_pack(fdt)));
 
     if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
@@ -1622,6 +1644,8 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
 void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
 {
     CPUPPCState *env = &cpu->env;
+    CPUState *cs = CPU(cpu);
+    int i;
 
     /* Set time-base frequency to 512 MHz */
     cpu_ppc_tb_init(env, TIMEBASE_FREQ);
@@ -1646,6 +1670,14 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
         }
     }
 
+    /* Set NUMA node for the added CPUs  */
+    for (i = 0; i < nb_numa_nodes; i++) {
+        if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) {
+            cs->numa_node = i;
+            break;
+        }
+    }
+
     xics_cpu_setup(spapr->icp, cpu);
 
     qemu_register_reset(spapr_cpu_reset, cpu);
@@ -1824,6 +1856,11 @@ static void ppc_spapr_init(MachineState *machine)
 
         for (i = 0; i < spapr_max_cores; i++) {
             int core_dt_id = i * smt;
+            sPAPRDRConnector *drc =
+                spapr_dr_connector_new(OBJECT(spapr),
+                                       SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
+
+            qemu_register_reset(spapr_drc_reset, drc);
 
             if (i < spapr_cores) {
                 Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
@@ -2246,6 +2283,27 @@ out:
     error_propagate(errp, local_err);
 }
 
+void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
+                                    int *fdt_offset, sPAPRMachineState *spapr)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    DeviceClass *dc = DEVICE_GET_CLASS(cs);
+    int id = ppc_get_vcpu_dt_id(cpu);
+    void *fdt;
+    int offset, fdt_size;
+    char *nodename;
+
+    fdt = create_device_tree(&fdt_size);
+    nodename = g_strdup_printf("%s@%x", dc->fw_name, id);
+    offset = fdt_add_subnode(fdt, 0, nodename);
+
+    spapr_populate_cpu_dt(cs, fdt, offset, spapr);
+    g_free(nodename);
+
+    *fdt_offset = offset;
+    return fdt;
+}
+
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
@@ -2286,6 +2344,12 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
         }
 
         spapr_memory_plug(hotplug_dev, dev, node, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        if (!smc->dr_cpu_enabled && dev->hotplugged) {
+            error_setg(errp, "CPU hotplug not supported for this machine");
+            return;
+        }
+        spapr_core_plug(hotplug_dev, dev, errp);
     }
 }
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 8c6d71d..db8de32 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -15,6 +15,75 @@
 #include <sysemu/cpus.h>
 #include "target-ppc/kvm_ppc.h"
 
+void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                     Error **errp)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
+    CPUState *cs = CPU(&core->threads[0]);
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+    Error *local_err = NULL;
+    void *fdt = NULL;
+    int fdt_offset = 0;
+    int core_id, core_dt_id;
+    int smt = kvmppc_smt_threads();
+
+    /* TODO: Should we cache core_id in sPAPRCPUCore ? */
+    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
+    core_id = core_dt_id / smt;
+    spapr->cores[core_id] = OBJECT(dev);
+
+    if (!smc->dr_cpu_enabled) {
+        /*
+         * This is a cold plugged CPU core but the machine doesn't support
+         * DR. So skip the hotplug path ensuring that the core is brought
+         * up online with out an associated DR connector.
+         */
+        return;
+    }
+
+    g_assert(drc);
+
+    /*
+     * Setup CPU DT entries only for hotplugged CPUs. For boot time or
+     * coldplugged CPUs DT entries are setup in spapr_finalize_fdt().
+     */
+    if (dev->hotplugged) {
+        fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, spapr);
+        dev->hotplugged = true;
+    }
+
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
+    if (local_err) {
+        g_free(fdt);
+        spapr->cores[core_id] = NULL;
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (dev->hotplugged) {
+        /*
+         * Send hotplug notification interrupt to the guest only in case
+         * of hotplugged CPUs.
+         */
+        spapr_hotplug_req_add_by_index(drc);
+    } else {
+        /*
+         * Set the right DRC states for cold plugged CPU.
+         */
+        drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
+        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
+    }
+}
+
 static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
                                           Error **errp)
 {
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 39f4682..10340e1 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -437,6 +437,9 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
     case SPAPR_DR_CONNECTOR_TYPE_LMB:
         hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
         break;
+    case SPAPR_DR_CONNECTOR_TYPE_CPU:
+        hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_CPU;
+        break;
     default:
         /* we shouldn't be signaling hotplug events for resources
          * that don't support them
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index b7c5ebd..cc0369e 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -34,6 +34,7 @@
 
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
+#include "hw/ppc/ppc.h"
 #include "qapi-event.h"
 #include "hw/boards.h"
 
@@ -161,6 +162,27 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
     rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
 }
 
+/*
+ * Set the timebase offset of the CPU to that of first CPU.
+ * This helps hotplugged CPU to have the correct timebase offset.
+ */
+static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
+{
+    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
+
+    cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
+}
+
+static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
+{
+    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
+
+    if (!pcc->interrupts_big_endian(fcpu)) {
+        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
+    }
+}
+
 static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
                            uint32_t token, uint32_t nargs,
                            target_ulong args,
@@ -197,6 +219,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
         env->nip = start;
         env->gpr[3] = r3;
         cs->halted = 0;
+        spapr_cpu_set_endianness(cpu);
+        spapr_cpu_update_tb_offset(cpu);
 
         qemu_cpu_kick(cs);
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index c099c3c..8957072 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -589,6 +589,8 @@ void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
                                           uint32_t count);
 void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
 void spapr_cpu_reset(void *opaque);
+void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
+                                    int *fdt_offset, sPAPRMachineState *spapr);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 48fb76a..980d8ae 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -25,4 +25,6 @@ typedef struct sPAPRCPUCore {
     PowerPCCPU *threads;
 } sPAPRCPUCore;
 
+void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                     Error **errp);
 #endif
-- 
2.1.0

[Qemu-devel] [RFC PATCH v2 8/9] xics, xics_kvm: Handle CPU unplug correctly

[Bharata B Rao]

XICS is setup for each CPU during initialization. Provide a routine
to undo the same when CPU is unplugged. While here, move ss->cs management
into xics from xics_kvm since there is nothing KVM specific in it.
Also ensure xics reset doesn't set irq for CPUs that are already unplugged.

This allows reboot of a VM that has undergone CPU hotplug and unplug
to work correctly.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/intc/xics.c        | 14 ++++++++++++++
 hw/intc/xics_kvm.c    |  8 ++++----
 include/hw/ppc/xics.h |  1 +
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 213a370..9fdb551 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -45,6 +45,18 @@ static int get_cpu_index_by_dt_id(int cpu_dt_id)
     return -1;
 }
 
+void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+    ICPState *ss = &icp->ss[cs->cpu_index];
+
+    assert(cs->cpu_index < icp->nr_servers);
+    assert(cs == ss->cs);
+
+    ss->output = NULL;
+    ss->cs = NULL;
+}
+
 void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
@@ -54,6 +66,8 @@ void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
 
     assert(cs->cpu_index < icp->nr_servers);
 
+    ss->cs = cs;
+
     if (info->cpu_setup) {
         info->cpu_setup(icp, cpu);
     }
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 9fe0667..7aab4a1 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -110,8 +110,10 @@ static void icp_kvm_reset(DeviceState *dev)
     icp->pending_priority = 0xff;
     icp->mfrr = 0xff;
 
-    /* Make all outputs are deasserted */
-    qemu_set_irq(icp->output, 0);
+    /* Make all outputs as deasserted only if the CPU thread is in use */
+    if (icp->output) {
+        qemu_set_irq(icp->output, 0);
+    }
 
     icp_set_kvm_state(icp, 1);
 }
@@ -344,8 +346,6 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
     if (icpkvm->kernel_xics_fd != -1) {
         int ret;
 
-        ss->cs = cs;
-
         ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0,
                                   icpkvm->kernel_xics_fd, kvm_arch_vcpu_id(cs));
         if (ret < 0) {
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index f60b06a..9091054 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -167,5 +167,6 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align,
 void xics_free(XICSState *icp, int irq, int num);
 
 void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
+void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu);
 
 #endif /* __XICS_H__ */
-- 
2.1.0

[Qemu-devel] [RFC PATCH v2 9/9] spapr: CPU hot unplug support

[Bharata B Rao]

Remove the CPU core device by removing the underlying CPU thread devices.
Hot removal of CPU for sPAPR guests is achieved by sending the hot unplug
notification to the guest. Release the vCPU object after CPU hot unplug so
that vCPU fd can be parked and reused.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c                  | 21 ++++++++++
 hw/ppc/spapr_cpu_core.c         | 86 +++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |  1 +
 include/hw/ppc/spapr_cpu_core.h | 12 ++++++
 4 files changed, 120 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 822c87d..b1e9ba2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2345,7 +2345,12 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 
         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        /*
+         * TODO: Move this check to pre_plug handler at which point
+         * spapr_core_release() won't be necessary.
+         */
         if (!smc->dr_cpu_enabled && dev->hotplugged) {
+            spapr_core_release(dev);
             error_setg(errp, "CPU hotplug not supported for this machine");
             return;
         }
@@ -2353,11 +2358,27 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     }
 }
 
+void spapr_cpu_destroy(PowerPCCPU *cpu)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+
+    xics_cpu_destroy(spapr->icp, cpu);
+    qemu_unregister_reset(spapr_cpu_reset, cpu);
+}
+
 static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         error_setg(errp, "Memory hot unplug not supported by sPAPR");
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        if (!smc->dr_cpu_enabled) {
+            error_setg(errp, "CPU hot unplug not supported on this machine");
+            return;
+        }
+        spapr_core_unplug(hotplug_dev, dev, errp);
     }
 }
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index db8de32..dd391bd 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -84,6 +84,92 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     }
 }
 
+static void spapr_cpu_core_cleanup(struct sPAPRCPUUnplugList *unplug_list)
+{
+    sPAPRCPUUnplug *unplug, *next;
+    Object *cpu;
+
+    QLIST_FOREACH_SAFE(unplug, unplug_list, node, next) {
+        cpu = unplug->cpu;
+        object_unparent(cpu);
+        QLIST_REMOVE(unplug, node);
+        g_free(unplug);
+    }
+}
+
+static void spapr_add_cpu_to_unplug_list(Object *cpu,
+                                         struct sPAPRCPUUnplugList *unplug_list)
+{
+    sPAPRCPUUnplug *unplug = g_malloc(sizeof(*unplug));
+
+    unplug->cpu = cpu;
+    QLIST_INSERT_HEAD(unplug_list, unplug, node);
+}
+
+static int spapr_cpu_release(Object *obj, void *opaque)
+{
+    DeviceState *dev = DEVICE(obj);
+    CPUState *cs = CPU(dev);
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    struct sPAPRCPUUnplugList *unplug_list = opaque;
+
+    spapr_cpu_destroy(cpu);
+    cpu_remove_sync(cs);
+
+    /*
+     * We are still walking the core object's children list, and
+     * hence can't cleanup this CPU thread object just yet. Put
+     * it on a list for later removal.
+     */
+    spapr_add_cpu_to_unplug_list(obj, unplug_list);
+    return 0;
+}
+
+void spapr_core_release(DeviceState *dev)
+{
+    struct sPAPRCPUUnplugList unplug_list;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
+    int core_dt_id = object_property_get_int(OBJECT(dev), "core", NULL);
+    int smt = kvmppc_smt_threads();
+
+    QLIST_INIT(&unplug_list);
+    object_child_foreach(OBJECT(dev), spapr_cpu_release, &unplug_list);
+    spapr_cpu_core_cleanup(&unplug_list);
+    spapr->cores[core_dt_id / smt] = NULL;
+
+    g_free(core->threads);
+}
+
+static void spapr_core_release_unparent(DeviceState *dev, void *opaque)
+{
+    spapr_core_release(dev);
+    object_unparent(OBJECT(dev));
+}
+
+void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                       Error **errp)
+{
+    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
+    PowerPCCPU *cpu = &core->threads[0];
+    int id = ppc_get_vcpu_dt_id(cpu);
+    sPAPRDRConnector *drc =
+        spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
+    sPAPRDRConnectorClass *drck;
+    Error *local_err = NULL;
+
+    g_assert(drc);
+
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    drck->detach(drc, dev, spapr_core_release_unparent, NULL, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    spapr_hotplug_req_remove_by_index(drc);
+}
+
 static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
                                           Error **errp)
 {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 8957072..41d0928 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -591,6 +591,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
 void spapr_cpu_reset(void *opaque);
 void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
                                     int *fdt_offset, sPAPRMachineState *spapr);
+void spapr_cpu_destroy(PowerPCCPU *cpu);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 980d8ae..75c6c58 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -27,4 +27,16 @@ typedef struct sPAPRCPUCore {
 
 void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                      Error **errp);
+void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                       Error **errp);
+void spapr_core_release(DeviceState *dev);
+
+/* List to store unplugged CPU objects for cleanup during unplug */
+typedef struct sPAPRCPUUnplug {
+    Object *cpu;
+    QLIST_ENTRY(sPAPRCPUUnplug) node;
+} sPAPRCPUUnplug;
+
+QLIST_HEAD(sPAPRCPUUnplugList, sPAPRCPUUnplug);
+
 #endif
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH v2 1/9] exec: Remove cpu from cpus list during cpu_exec_exit()

[Thomas Huth]

On 11.03.2016 05:54, Bharata B Rao wrote:
> CPUState *cpu gets added to the cpus list during cpu_exec_init(). It
> should be removed from cpu_exec_exit().
> 
> cpu_exec_exit() is called from generic CPU::instance_finalize and some
> archs like PowerPC call it from CPU unrealizefn. So ensure that we
> dequeue the cpu only once.
> 
> Now -1 value for cpu->cpu_index indicates that we have already dequeued
> the cpu for CONFIG_USER_ONLY case also.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  exec.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index f09dd4e..49ae593 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -609,15 +609,9 @@ static int cpu_get_free_index(Error **errp)
>      return cpu;
>  }
>  
> -void cpu_exec_exit(CPUState *cpu)
> +static void cpu_release_index(CPUState *cpu)
>  {
> -    if (cpu->cpu_index == -1) {
> -        /* cpu_index was never allocated by this @cpu or was already freed. */
> -        return;
> -    }
> -
>      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> -    cpu->cpu_index = -1;
>  }
>  #else
>  
> @@ -632,11 +626,33 @@ static int cpu_get_free_index(Error **errp)
>      return cpu_index;
>  }
>  
> -void cpu_exec_exit(CPUState *cpu)
> +static void cpu_release_index(CPUState *cpu)
>  {
> +    return;
>  }
>  #endif
>  
> +void cpu_exec_exit(CPUState *cpu)
> +{
> +#if defined(CONFIG_USER_ONLY)
> +    cpu_list_lock();
> +#endif
> +    if (cpu->cpu_index == -1) {
> +        /* cpu_index was never allocated by this @cpu or was already freed. */
> +#if defined(CONFIG_USER_ONLY)
> +        cpu_list_unlock();
> +#endif
> +        return;
> +    }
> +
> +    QTAILQ_REMOVE(&cpus, cpu, node);
> +    cpu_release_index(cpu);
> +    cpu->cpu_index = -1;
> +#if defined(CONFIG_USER_ONLY)
> +    cpu_list_unlock();
> +#endif
> +}
> +
>  void cpu_exec_init(CPUState *cpu, Error **errp)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [RFC PATCH v2 2/9] exec: Do vmstate unregistration from cpu_exec_exit()

[Thomas Huth]

On 11.03.2016 05:54, Bharata B Rao wrote:
> cpu_exec_init() does vmstate_register for the CPU device. This needs to be
> undone from cpu_exec_exit(). This change is needed to support CPU hot
> removal.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  exec.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 49ae593..8ddca6b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -634,6 +634,8 @@ static void cpu_release_index(CPUState *cpu)
>  
>  void cpu_exec_exit(CPUState *cpu)
>  {
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
>  #endif
> @@ -651,6 +653,13 @@ 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)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [RFC PATCH v2 3/9] cpu: Reclaim vCPU objects

[Thomas Huth]

On 11.03.2016 05:54, Bharata B Rao wrote:
> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> 
> In order to deal well with the kvm vcpus (which can not be removed without any
> protection), we do not close KVM vcpu fd, just record and mark it as stopped
> into a list, so that we can reuse it for the appending cpu hot-add request if
> possible. It is also the approach that kvm guys suggested:
> https://www.mail-archive.com/kvm@vger.kernel.org/msg102839.html
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>                [- Explicit CPU_REMOVE() from qemu_kvm/tcg_destroy_vcpu()
>                   isn't needed as it is done from cpu_exec_exit()
>                 - Use iothread mutex instead of global mutex during
>                   destroy
>                 - Don't cleanup vCPU object from vCPU thread context
>                   but leave it to the callers (device_add/device_del)]
> ---
>  cpus.c               | 39 +++++++++++++++++++++++++++++++++--
>  include/qom/cpu.h    | 10 +++++++++
>  include/sysemu/kvm.h |  1 +
>  kvm-all.c            | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  kvm-stub.c           |  5 +++++
>  5 files changed, 109 insertions(+), 3 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [RFC PATCH v2 0/9] Core based CPU hotplug for PowerPC sPAPR

[Igor Mammedov]

On Fri, 11 Mar 2016 10:24:29 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Hi,
> 
> This is the next version of "Core based CPU hotplug for PowerPC sPAPR" that
> was posted at
> https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html
> 
> device_add semantics
> --------------------
> For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
> (qemu) device_add spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]
do you plan to allow user to hotplug different cpu_models?
If not it would be better to hide cpu_model from user
and set it from machine pre_plug handler.

> 
> Major changes in this version
> -----------------------------
> - Based on the review feedback, removed the links from machine object
>   to the core objects.
> - With that, the concept of using the links as slots where core object sits
>   is gone.
> - String slot name which was being used as slot= with device_add now
>   becomes an integer core id being specified as core=.
> - threads property which indicates the number of threads in the core
>   moves from spapr-cpu-core type to cpu-core type.
> - Threads creation moves from core's property setter to core's realizefn.
> - Igor's proposed pre_plug handler isn't yet used in this patchset, but it
>   wouldn't take much effort to switch to it. Waiting for some review/consensus
>   on Igor's patchset before switching to it.
> - This patchset will now work with Igor's query-hotpluggable-cpus QMP
>   interface.
> 
> Other changes
> -------------
> - Core ID that is used with device_add is in fact device tree ID now.
> - DRC indexes are based on core_dt_id now. There are a couple of places
>   where core device's thread0 is used to fetch the DRC index, but changing
>   that requires bigger change of converting the CPUs DT code generation
>   to iterate over cores instead of threads.
> - Coverted while(1) to do-while() as suggeted by Thomas Huth (3/9).
> - Creation of spapr-cpu-core device and conversion of boot CPUs into
>   cores merged into a single patch (6/9).
> - Topologies with incomplete cores are prevented from booting only with
>   machine type versions that support CPU DR (6/9).
> - Conversion of boot CPUs into cores is done only for machine type versions
>   that support CPU DR (6/9).
> - Take care of recovery from failure in plug handler when CPU hotplug isn't
>   supported correctly. This will not be needed when we prevent such
>   attempts from pre_plug handler (9/9).
> Bharata B Rao (8):
>   exec: Remove cpu from cpus list during cpu_exec_exit()
>   exec: Do vmstate unregistration from cpu_exec_exit()
>   cpu: Add a sync version of cpu_remove()
>   cpu: Abstract CPU core type
>   spapr: CPU core device
>   spapr: CPU hotplug support
>   xics,xics_kvm: Handle CPU unplug correctly
>   spapr: CPU hot unplug support
> 
> Gu Zheng (1):
>   cpu: Reclaim vCPU objects
> 
>  cpus.c                          |  51 +++++-
>  exec.c                          |  41 ++++-
>  hw/cpu/Makefile.objs            |   1 +
>  hw/cpu/core.c                   |  87 ++++++++++
>  hw/intc/xics.c                  |  14 ++
>  hw/intc/xics_kvm.c              |   8 +-
>  hw/ppc/Makefile.objs            |   1 +
>  hw/ppc/spapr.c                  | 153 +++++++++++++++--
>  hw/ppc/spapr_cpu_core.c         | 354 ++++++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_events.c           |   3 +
>  hw/ppc/spapr_rtas.c             |  24 +++
>  include/hw/cpu/core.h           |  31 ++++
>  include/hw/ppc/spapr.h          |   7 +
>  include/hw/ppc/spapr_cpu_core.h |  42 +++++
>  include/hw/ppc/xics.h           |   1 +
>  include/qom/cpu.h               |  18 ++
>  include/sysemu/kvm.h            |   1 +
>  kvm-all.c                       |  57 ++++++-
>  kvm-stub.c                      |   5 +
>  19 files changed, 871 insertions(+), 28 deletions(-)
>  create mode 100644 hw/cpu/core.c
>  create mode 100644 hw/ppc/spapr_cpu_core.c
>  create mode 100644 include/hw/cpu/core.h
>  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> 

Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device

[Igor Mammedov]

On Fri, 11 Mar 2016 10:24:35 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Add sPAPR specific CPU core device that is based on generic CPU core device.
> Creating this core device will result in creation of all the CPU thread
> devices that are part of this core.
> 
> Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> topologies that result in partially filled cores. Both of these are done
> only if CPU core hotplug is supported.
> 
> Note: An unrelated change in the call to xics_system_init() is done
> in this patch as it makes sense to use the local variable smt introduced
> in this patch instead of kvmppc_smt_threads() call here.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/Makefile.objs            |   1 +
>  hw/ppc/spapr.c                  |  68 +++++++++++---
>  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |   4 +
>  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
>  5 files changed, 287 insertions(+), 13 deletions(-)
>  create mode 100644 hw/ppc/spapr_cpu_core.c
>  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index c1ffc77..5cc6608 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 64c4acc..cffe8c8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -64,6 +64,7 @@
>  
>  #include "hw/compat.h"
>  #include "qemu-common.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  
>  #include <libfdt.h>
>  
> @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
>  
>  }
>  
> -static void spapr_cpu_reset(void *opaque)
> +void spapr_cpu_reset(void *opaque)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      PowerPCCPU *cpu = opaque;
> @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>      machine->boot_order = g_strdup(boot_device);
>  }
>  
> -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> -                           Error **errp)
> +/*
> + * TODO: Check if some of these can be moved to rtas_start_cpu() where
> + * a few other things required for hotplugged CPUs are being done.
> + */
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
>  
> @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
>      const char *kernel_filename = machine->kernel_filename;
>      const char *kernel_cmdline = machine->kernel_cmdline;
>      const char *initrd_filename = machine->initrd_filename;
> -    PowerPCCPU *cpu;
>      PCIHostState *phb;
>      int i;
>      MemoryRegion *sysmem = get_system_memory();
> @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
>      long load_limit, fw_size;
>      bool kernel_le = false;
>      char *filename;
> +    int smt = kvmppc_smt_threads();
> +    int spapr_cores = smp_cpus / smp_threads;
> +    int spapr_max_cores = max_cpus / smp_threads;
> +
> +    if (smc->dr_cpu_enabled) {
> +        if (smp_cpus % smp_threads) {
> +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> +                         smp_cpus, smp_threads);
> +            exit(1);
> +        }
> +        if (max_cpus % smp_threads) {
> +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> +                         max_cpus, smp_threads);
> +            exit(1);
> +        }
> +    }
>  
>      msi_supported = true;
>  
> @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
>  
>      /* Set up Interrupt Controller before we create the VCPUs */
>      spapr->icp = xics_system_init(machine,
> -                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> -                                               smp_threads),
> +                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),
is smt == smp_threads, if not then what's a difference?

>                                    XICS_IRQS, &error_fatal);
>  
>      if (smc->dr_lmb_enabled) {
> @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
>      if (machine->cpu_model == NULL) {
>          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
>      }
> -    for (i = 0; i < smp_cpus; i++) {
> -        cpu = cpu_ppc_init(machine->cpu_model);
> -        if (cpu == NULL) {
> -            error_report("Unable to find PowerPC CPU definition");
> -            exit(1);
> +
> +    if (smc->dr_cpu_enabled) {
> +        spapr->cores = g_new0(Object *, spapr_max_cores);
> +
> +        for (i = 0; i < spapr_max_cores; i++) {
> +            int core_dt_id = i * smt;
> +
> +            if (i < spapr_cores) {
> +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> +
> +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> +                                        &error_fatal);
> +                object_property_set_int(core, smp_threads, "threads",
> +                                        &error_fatal);
> +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> +                                        &error_fatal);
> +                object_property_set_bool(core, true, "realized", &error_fatal);
> +            }
>          }
> -        spapr_cpu_init(spapr, cpu, &error_fatal);
> +    } else {
> +        for (i = 0; i < smp_cpus; i++) {
> +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> +            if (cpu == NULL) {
> +                error_report("Unable to find PowerPC CPU definition");
> +                exit(1);
> +            }
> +            spapr_cpu_init(spapr, cpu, &error_fatal);
> +       }
>      }
>  
>      if (kvm_enabled()) {
> @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
>                                               DeviceState *dev)
>  {
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> +        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>          return HOTPLUG_HANDLER(machine);
>      }
>      return NULL;
> @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
>  
>      smc->dr_lmb_enabled = true;
> +    smc->dr_cpu_enabled = true;
>      fwc->get_dev_path = spapr_get_fw_dev_path;
>      nc->nmi_monitor_handler = spapr_nmi;
>  }
> @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
>  
>      spapr_machine_2_6_class_options(mc);
>      smc->use_ohci_by_default = true;
> +    smc->dr_cpu_enabled = false;
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
>  }
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> new file mode 100644
> index 0000000..8c6d71d
> --- /dev/null
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -0,0 +1,199 @@
> +/*
> + * sPAPR CPU core device, acts as container of CPU thread devices.
> + *
> + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/cpu/core.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/boards.h"
> +#include "qemu/error-report.h"
> +#include "qapi/visitor.h"
> +#include <sysemu/cpus.h>
> +#include "target-ppc/kvm_ppc.h"
> +
> +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> +                                          Error **errp)
> +{
> +    int i;
> +    Error *local_err = NULL;
> +
> +    for (i = 0; i < threads; i++) {
> +        char id[32];
> +
> +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> +                          object_class_get_name(core->oc));
> +        snprintf(id, sizeof(id), "thread[%d]", i);
> +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> +                                  &local_err);
> +        if (local_err) {
> +            goto err;
> +        }
> +    }
> +    return;
> +
> +err:
> +    while (--i) {
> +        object_unparent(OBJECT(&core->threads[i]));
> +    }
> +    error_propagate(errp, local_err);
> +}
> +
> +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> +{
> +    Error **errp = opaque;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    CPUState *cs = CPU(child);
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    object_property_set_bool(child, true, "realized", errp);
> +    if (*errp) {
> +        return 1;
> +    }
> +
> +    spapr_cpu_init(spapr, cpu, errp);
> +    if (*errp) {
> +        return 1;
> +    }
> +
> +    spapr_cpu_reset(cpu);
should it be move to spapr_cpu_init() ?

> +    return 0;
> +}
> +
> +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    int spapr_max_cores = max_cpus / smp_threads;
this should be in machine code

> +    Error *local_err = NULL;
> +    int threads = 0;
> +    int core_dt_id, core_id;
> +    int smt = kvmppc_smt_threads();
> +
> +    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);
since spapr_core is inherited from cpu-core,
you can cast to cpu-core and use fields directly here instead of using
property setters.

> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    if (threads != smp_threads) {
> +        error_setg(&local_err, "threads must be %d", smp_threads);
> +        goto out;
> +    }
move to machine handler

> +
> +    if (!core->oc) {
> +        error_setg(&local_err, "cpu_model property isn't set");
> +        goto out;
> +    }
> +
> +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    if (core_dt_id % smt) {
> +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> +        goto out;
> +    }
> +
> +    core_id = core_dt_id / smt;
> +    if (core_id < 0 || core_id >= spapr_max_cores) {
> +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> +        goto out;
> +    }
maybe due to nameing it's a bit confusing,
what's difference between core_id and core_dt_id?

> +
> +    /*
> +     * TODO: This check will be moved to ->pre_plug() as suggested by Igor
> +     * when there is consensus pre_plug hook.
> +     */
> +    if (spapr->cores[core_id]) {
> +        error_setg(&local_err, "core %d already populated", core_dt_id);
> +        goto out;
> +    }
> +
> +    core->threads = g_new0(PowerPCCPU, threads);
> +    spapr_cpu_core_create_threads(core, threads, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    return;
> +
> +out:
> +    if (local_err) {
> +        g_free(core->threads);
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
> +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    /*
> +     * TODO: This returns the full type instead of just cpu_model. For eg,
> +     * host-powerpc64-cpu is returned where just "host" is expected.
> +     */
> +    return g_strdup(object_class_get_name(core->oc));
> +}
> +
> +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> +                                              Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    ObjectClass *oc = cpu_class_by_name(TYPE_POWERPC_CPU, val);
> +    ObjectClass *oc_base = cpu_class_by_name(TYPE_POWERPC_CPU,
> +                                             machine->cpu_model);
> +    if (!oc) {
> +        error_setg(errp, "Unknown CPU model %s", val);
> +        return;
> +    }
> +
> +    /*
> +     * Currently cpu_model can't be different from what is specified with -cpu
> +     */
> +    if (strcmp(object_class_get_name(oc), object_class_get_name(oc_base))) {
> +        error_setg(errp, "cpu_model must be %s", machine->cpu_model);
> +        return;
> +    }
> +
> +    core->oc = oc;
> +}
> +
> +static void spapr_cpu_core_instance_init(Object *obj)
> +{
> +    object_property_add_str(obj, "cpu_model",
> +                            spapr_cpu_core_prop_get_cpu_model,
> +                            spapr_cpu_core_prop_set_cpu_model,
> +                            NULL);
> +}
> +
> +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = spapr_cpu_core_realize;
> +}
> +
> +static const TypeInfo spapr_cpu_core_type_info = {
> +    .name = TYPE_SPAPR_CPU_CORE,
> +    .parent = TYPE_CPU_CORE,
> +    .instance_init = spapr_cpu_core_instance_init,
> +    .instance_size = sizeof(sPAPRCPUCore),
> +    .class_init = spapr_cpu_core_class_init,
> +};
> +
> +static void spapr_cpu_core_register_types(void)
> +{
> +    type_register_static(&spapr_cpu_core_type_info);
> +}
> +
> +type_init(spapr_cpu_core_register_types)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 098d85d..c099c3c 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -36,6 +36,7 @@ struct sPAPRMachineClass {
>  
>      /*< public >*/
>      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
> +    bool dr_cpu_enabled;       /* enable dynamic-reconfig/hotplug of CPUs */
>      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
>  };
>  
> @@ -79,6 +80,7 @@ struct sPAPRMachineState {
>      /*< public >*/
>      char *kvm_type;
>      MemoryHotplugState hotplug_memory;
> +    Object **cores;
I'd prefer "Object *cores[0];" as it tells us that it's an array

>  };
>  
>  #define H_SUCCESS         0
> @@ -585,6 +587,8 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
>                                         uint32_t count);
>  void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
>                                            uint32_t count);
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
> +void spapr_cpu_reset(void *opaque);
>  
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> new file mode 100644
> index 0000000..48fb76a
> --- /dev/null
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -0,0 +1,28 @@
> +/*
> + * sPAPR CPU core device.
> + *
> + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef HW_SPAPR_CPU_CORE_H
> +#define HW_SPAPR_CPU_CORE_H
> +
> +#include "hw/qdev.h"
> +#include "hw/cpu/core.h"
> +
> +#define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
> +#define SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(sPAPRCPUCore, (obj), TYPE_SPAPR_CPU_CORE)
> +
> +typedef struct sPAPRCPUCore {
> +    /*< private >*/
> +    CPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *oc;
> +    PowerPCCPU *threads;
> +} sPAPRCPUCore;
> +
> +#endif

Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device

[Thomas Huth]

On 14.03.2016 11:25, Igor Mammedov wrote:
> On Fri, 11 Mar 2016 10:24:35 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
>> Add sPAPR specific CPU core device that is based on generic CPU core device.
>> Creating this core device will result in creation of all the CPU thread
>> devices that are part of this core.
>>
>> Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
>> CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
>> topologies that result in partially filled cores. Both of these are done
>> only if CPU core hotplug is supported.
>>
>> Note: An unrelated change in the call to xics_system_init() is done
>> in this patch as it makes sense to use the local variable smt introduced
>> in this patch instead of kvmppc_smt_threads() call here.
>>
>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/Makefile.objs            |   1 +
>>  hw/ppc/spapr.c                  |  68 +++++++++++---
>>  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h          |   4 +
>>  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
>>  5 files changed, 287 insertions(+), 13 deletions(-)
>>  create mode 100644 hw/ppc/spapr_cpu_core.c
>>  create mode 100644 include/hw/ppc/spapr_cpu_core.h
>>
>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>> index c1ffc77..5cc6608 100644
>> --- a/hw/ppc/Makefile.objs
>> +++ b/hw/ppc/Makefile.objs
>> @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
>>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>> +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>>  obj-y += spapr_pci_vfio.o
>>  endif
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 64c4acc..cffe8c8 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
...
>> @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
>>      if (machine->cpu_model == NULL) {
>>          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
>>      }
>> -    for (i = 0; i < smp_cpus; i++) {
>> -        cpu = cpu_ppc_init(machine->cpu_model);
>> -        if (cpu == NULL) {
>> -            error_report("Unable to find PowerPC CPU definition");
>> -            exit(1);
>> +
>> +    if (smc->dr_cpu_enabled) {
>> +        spapr->cores = g_new0(Object *, spapr_max_cores);

The spapr->cores _pointer_ is allocate with g_new0 here ...

>> +        for (i = 0; i < spapr_max_cores; i++) {
>> +            int core_dt_id = i * smt;
>> +
>> +            if (i < spapr_cores) {
>> +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
>> +
>> +                object_property_set_str(core, machine->cpu_model, "cpu_model",
>> +                                        &error_fatal);
>> +                object_property_set_int(core, smp_threads, "threads",
>> +                                        &error_fatal);
>> +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
>> +                                        &error_fatal);
>> +                object_property_set_bool(core, true, "realized", &error_fatal);
>> +            }
>>          }
>> -        spapr_cpu_init(spapr, cpu, &error_fatal);
>> +    } else {
>> +        for (i = 0; i < smp_cpus; i++) {
>> +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
>> +            if (cpu == NULL) {
>> +                error_report("Unable to find PowerPC CPU definition");
>> +                exit(1);
>> +            }
>> +            spapr_cpu_init(spapr, cpu, &error_fatal);
>> +       }
>>      }
...
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 098d85d..c099c3c 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -36,6 +36,7 @@ struct sPAPRMachineClass {
>>  
>>      /*< public >*/
>>      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
>> +    bool dr_cpu_enabled;       /* enable dynamic-reconfig/hotplug of CPUs */
>>      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
>>  };
>>  
>> @@ -79,6 +80,7 @@ struct sPAPRMachineState {
>>      /*< public >*/
>>      char *kvm_type;
>>      MemoryHotplugState hotplug_memory;
>> +    Object **cores;
> I'd prefer "Object *cores[0];" as it tells us that it's an array

... so you can not declare it as an array here, can you??

 Thomas

Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device

[Igor Mammedov]

On Mon, 14 Mar 2016 11:56:52 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 14.03.2016 11:25, Igor Mammedov wrote:
> > On Fri, 11 Mar 2016 10:24:35 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> >> Add sPAPR specific CPU core device that is based on generic CPU core device.
> >> Creating this core device will result in creation of all the CPU thread
> >> devices that are part of this core.
> >>
> >> Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> >> CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> >> topologies that result in partially filled cores. Both of these are done
> >> only if CPU core hotplug is supported.
> >>
> >> Note: An unrelated change in the call to xics_system_init() is done
> >> in this patch as it makes sense to use the local variable smt introduced
> >> in this patch instead of kvmppc_smt_threads() call here.
> >>
> >> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/Makefile.objs            |   1 +
> >>  hw/ppc/spapr.c                  |  68 +++++++++++---
> >>  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/spapr.h          |   4 +
> >>  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
> >>  5 files changed, 287 insertions(+), 13 deletions(-)
> >>  create mode 100644 hw/ppc/spapr_cpu_core.c
> >>  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> >>
> >> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> >> index c1ffc77..5cc6608 100644
> >> --- a/hw/ppc/Makefile.objs
> >> +++ b/hw/ppc/Makefile.objs
> >> @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> >>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> >>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> >> +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> >>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >>  obj-y += spapr_pci_vfio.o
> >>  endif
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 64c4acc..cffe8c8 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c  
> ...
> >> @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
> >>      if (machine->cpu_model == NULL) {
> >>          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> >>      }
> >> -    for (i = 0; i < smp_cpus; i++) {
> >> -        cpu = cpu_ppc_init(machine->cpu_model);
> >> -        if (cpu == NULL) {
> >> -            error_report("Unable to find PowerPC CPU definition");
> >> -            exit(1);
> >> +
> >> +    if (smc->dr_cpu_enabled) {
> >> +        spapr->cores = g_new0(Object *, spapr_max_cores);  
> 
> The spapr->cores _pointer_ is allocate with g_new0 here ...
> 
> >> +        for (i = 0; i < spapr_max_cores; i++) {
> >> +            int core_dt_id = i * smt;
> >> +
> >> +            if (i < spapr_cores) {
> >> +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> >> +
> >> +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> >> +                                        &error_fatal);
> >> +                object_property_set_int(core, smp_threads, "threads",
> >> +                                        &error_fatal);
> >> +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> >> +                                        &error_fatal);
> >> +                object_property_set_bool(core, true, "realized", &error_fatal);
> >> +            }
> >>          }
> >> -        spapr_cpu_init(spapr, cpu, &error_fatal);
> >> +    } else {
> >> +        for (i = 0; i < smp_cpus; i++) {
> >> +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> >> +            if (cpu == NULL) {
> >> +                error_report("Unable to find PowerPC CPU definition");
> >> +                exit(1);
> >> +            }
> >> +            spapr_cpu_init(spapr, cpu, &error_fatal);
> >> +       }
> >>      }  
> ...
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 098d85d..c099c3c 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -36,6 +36,7 @@ struct sPAPRMachineClass {
> >>  
> >>      /*< public >*/
> >>      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
> >> +    bool dr_cpu_enabled;       /* enable dynamic-reconfig/hotplug of CPUs */
> >>      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
> >>  };
> >>  
> >> @@ -79,6 +80,7 @@ struct sPAPRMachineState {
> >>      /*< public >*/
> >>      char *kvm_type;
> >>      MemoryHotplugState hotplug_memory;
> >> +    Object **cores;  
> > I'd prefer "Object *cores[0];" as it tells us that it's an array  
> 
> ... so you can not declare it as an array here, can you??
I can't

> 
>  Thomas
> 

Re: [Qemu-devel] [RFC PATCH v2 2/9] exec: Do vmstate unregistration from cpu_exec_exit()

[David Gibson]

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

On Fri, Mar 11, 2016 at 10:24:31AM +0530, Bharata B Rao wrote:
> cpu_exec_init() does vmstate_register for the CPU device. This needs to be
> undone from cpu_exec_exit(). This change is needed to support CPU hot
> removal.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

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

> ---
>  exec.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 49ae593..8ddca6b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -634,6 +634,8 @@ static void cpu_release_index(CPUState *cpu)
>  
>  void cpu_exec_exit(CPUState *cpu)
>  {
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
>  #endif
> @@ -651,6 +653,13 @@ 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)

-- 
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/9] cpu: Reclaim vCPU objects

[David Gibson]

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

On Fri, Mar 11, 2016 at 10:24:32AM +0530, Bharata B Rao wrote:
> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> 
> In order to deal well with the kvm vcpus (which can not be removed without any
> protection), we do not close KVM vcpu fd, just record and mark it as stopped
> into a list, so that we can reuse it for the appending cpu hot-add request if
> possible. It is also the approach that kvm guys suggested:
> https://www.mail-archive.com/kvm@vger.kernel.org/msg102839.html
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>                [- Explicit CPU_REMOVE() from qemu_kvm/tcg_destroy_vcpu()
>                   isn't needed as it is done from cpu_exec_exit()
>                 - Use iothread mutex instead of global mutex during
>                   destroy
>                 - Don't cleanup vCPU object from vCPU thread context
>                   but leave it to the callers (device_add/device_del)]

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

> ---
>  cpus.c               | 39 +++++++++++++++++++++++++++++++++--
>  include/qom/cpu.h    | 10 +++++++++
>  include/sysemu/kvm.h |  1 +
>  kvm-all.c            | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  kvm-stub.c           |  5 +++++
>  5 files changed, 109 insertions(+), 3 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index bc774e2..be0ac6a 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -953,6 +953,18 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
>      qemu_cpu_kick(cpu);
>  }
>  
> +static void qemu_kvm_destroy_vcpu(CPUState *cpu)
> +{
> +    if (kvm_destroy_vcpu(cpu) < 0) {
> +        error_report("kvm_destroy_vcpu failed");
> +        exit(EXIT_FAILURE);
> +    }
> +}
> +
> +static void qemu_tcg_destroy_vcpu(CPUState *cpu)
> +{
> +}
> +
>  static void flush_queued_work(CPUState *cpu)
>  {
>      struct qemu_work_item *wi;
> @@ -1045,7 +1057,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>      cpu->created = true;
>      qemu_cond_signal(&qemu_cpu_cond);
>  
> -    while (1) {
> +    do {
>          if (cpu_can_run(cpu)) {
>              r = kvm_cpu_exec(cpu);
>              if (r == EXCP_DEBUG) {
> @@ -1053,8 +1065,10 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>              }
>          }
>          qemu_kvm_wait_io_event(cpu);
> -    }
> +    } while (!cpu->unplug || cpu_can_run(cpu));
>  
> +    qemu_kvm_destroy_vcpu(cpu);
> +    qemu_mutex_unlock_iothread();
>      return NULL;
>  }
>  
> @@ -1108,6 +1122,7 @@ static void tcg_exec_all(void);
>  static void *qemu_tcg_cpu_thread_fn(void *arg)
>  {
>      CPUState *cpu = arg;
> +    CPUState *remove_cpu = NULL;
>  
>      rcu_register_thread();
>  
> @@ -1145,6 +1160,16 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>              }
>          }
>          qemu_tcg_wait_io_event(QTAILQ_FIRST(&cpus));
> +        CPU_FOREACH(cpu) {
> +            if (cpu->unplug && !cpu_can_run(cpu)) {
> +                remove_cpu = cpu;
> +                break;
> +            }
> +        }
> +        if (remove_cpu) {
> +            qemu_tcg_destroy_vcpu(remove_cpu);
> +            remove_cpu = NULL;
> +        }
>      }
>  
>      return NULL;
> @@ -1301,6 +1326,13 @@ void resume_all_vcpus(void)
>      }
>  }
>  
> +void cpu_remove(CPUState *cpu)
> +{
> +    cpu->stop = true;
> +    cpu->unplug = true;
> +    qemu_cpu_kick(cpu);
> +}
> +
>  /* For temporary buffers for forming a name */
>  #define VCPU_THREAD_NAME_SIZE 16
>  
> @@ -1517,6 +1549,9 @@ static void tcg_exec_all(void)
>                  break;
>              }
>          } else if (cpu->stop || cpu->stopped) {
> +            if (cpu->unplug) {
> +                next_cpu = CPU_NEXT(cpu);
> +            }
>              break;
>          }
>      }
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 7052eee..0720dd7 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -237,6 +237,7 @@ struct kvm_run;
>   * @halted: Nonzero if the CPU is in suspended state.
>   * @stop: Indicates a pending stop request.
>   * @stopped: Indicates the CPU has been artificially stopped.
> + * @unplug: Indicates a pending CPU unplug request.
>   * @crash_occurred: Indicates the OS reported a crash (panic) for this CPU
>   * @tcg_exit_req: Set to force TCG to stop executing linked TBs for this
>   *           CPU and return to its top level loop.
> @@ -289,6 +290,7 @@ struct CPUState {
>      bool created;
>      bool stop;
>      bool stopped;
> +    bool unplug;
>      bool crash_occurred;
>      bool exit_request;
>      uint32_t interrupt_request;
> @@ -756,6 +758,14 @@ void cpu_exit(CPUState *cpu);
>  void cpu_resume(CPUState *cpu);
>  
>  /**
> + * cpu_remove:
> + * @cpu: The CPU to remove.
> + *
> + * Requests the CPU to be removed.
> + */
> +void cpu_remove(CPUState *cpu);
> +
> +/**
>   * qemu_init_vcpu:
>   * @cpu: The vCPU to initialize.
>   *
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 6695fa7..5d5b602 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -216,6 +216,7 @@ int kvm_has_intx_set_mask(void);
>  
>  int kvm_init_vcpu(CPUState *cpu);
>  int kvm_cpu_exec(CPUState *cpu);
> +int kvm_destroy_vcpu(CPUState *cpu);
>  
>  #ifdef NEED_CPU_H
>  
> diff --git a/kvm-all.c b/kvm-all.c
> index 44c0464..35c0621 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -61,6 +61,12 @@
>  
>  #define KVM_MSI_HASHTAB_SIZE    256
>  
> +struct KVMParkedVcpu {
> +    unsigned long vcpu_id;
> +    int kvm_fd;
> +    QLIST_ENTRY(KVMParkedVcpu) node;
> +};
> +
>  struct KVMState
>  {
>      AccelState parent_obj;
> @@ -94,6 +100,7 @@ struct KVMState
>      QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
>  #endif
>      KVMMemoryListener memory_listener;
> +    QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus;
>  };
>  
>  KVMState *kvm_state;
> @@ -237,6 +244,53 @@ static int kvm_set_user_memory_region(KVMMemoryListener *kml, KVMSlot *slot)
>      return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
>  }
>  
> +int kvm_destroy_vcpu(CPUState *cpu)
> +{
> +    KVMState *s = kvm_state;
> +    long mmap_size;
> +    struct KVMParkedVcpu *vcpu = NULL;
> +    int ret = 0;
> +
> +    DPRINTF("kvm_destroy_vcpu\n");
> +
> +    mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
> +    if (mmap_size < 0) {
> +        ret = mmap_size;
> +        DPRINTF("KVM_GET_VCPU_MMAP_SIZE failed\n");
> +        goto err;
> +    }
> +
> +    ret = munmap(cpu->kvm_run, mmap_size);
> +    if (ret < 0) {
> +        goto err;
> +    }
> +
> +    vcpu = g_malloc0(sizeof(*vcpu));
> +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
> +    vcpu->kvm_fd = cpu->kvm_fd;
> +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> +err:
> +    return ret;
> +}
> +
> +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
> +{
> +    struct KVMParkedVcpu *cpu;
> +
> +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> +        if (cpu->vcpu_id == vcpu_id) {
> +            int kvm_fd;
> +
> +            QLIST_REMOVE(cpu, node);
> +            kvm_fd = cpu->kvm_fd;
> +            g_free(cpu);
> +            return kvm_fd;
> +        }
> +    }
> +
> +    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> +}
> +
>  int kvm_init_vcpu(CPUState *cpu)
>  {
>      KVMState *s = kvm_state;
> @@ -245,7 +299,7 @@ int kvm_init_vcpu(CPUState *cpu)
>  
>      DPRINTF("kvm_init_vcpu\n");
>  
> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)kvm_arch_vcpu_id(cpu));
> +    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>      if (ret < 0) {
>          DPRINTF("kvm_create_vcpu failed\n");
>          goto err;
> @@ -1495,6 +1549,7 @@ static int kvm_init(MachineState *ms)
>  #ifdef KVM_CAP_SET_GUEST_DEBUG
>      QTAILQ_INIT(&s->kvm_sw_breakpoints);
>  #endif
> +    QLIST_INIT(&s->kvm_parked_vcpus);
>      s->vmfd = -1;
>      s->fd = qemu_open("/dev/kvm", O_RDWR);
>      if (s->fd == -1) {
> diff --git a/kvm-stub.c b/kvm-stub.c
> index b962b24..61f9d5c 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -33,6 +33,11 @@ bool kvm_allowed;
>  bool kvm_readonly_mem_allowed;
>  bool kvm_ioeventfd_any_length_allowed;
>  
> +int kvm_destroy_vcpu(CPUState *cpu)
> +{
> +    return -ENOSYS;
> +}
> +
>  int kvm_init_vcpu(CPUState *cpu)
>  {
>      return -ENOSYS;

-- 
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 5/9] cpu: Abstract CPU core type

[David Gibson]

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

On Fri, Mar 11, 2016 at 10:24:34AM +0530, Bharata B Rao wrote:
> Add an abstract CPU core type that could be used by machines that want
> to define and hotplug CPUs in core granularity.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

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

> ---
>  hw/cpu/Makefile.objs  |  1 +
>  hw/cpu/core.c         | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/cpu/core.h | 31 ++++++++++++++++++
>  3 files changed, 119 insertions(+)
>  create mode 100644 hw/cpu/core.c
>  create mode 100644 include/hw/cpu/core.h
> 
> diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> index 0954a18..942a4bb 100644
> --- a/hw/cpu/Makefile.objs
> +++ b/hw/cpu/Makefile.objs
> @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
>  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
>  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
>  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> +obj-y += core.o
>  
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> new file mode 100644
> index 0000000..3faf53d
> --- /dev/null
> +++ b/hw/cpu/core.c
> @@ -0,0 +1,87 @@
> +/*
> + * CPU core abstract device
> + *
> + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/cpu/core.h"
> +#include "qapi/visitor.h"
> +#include "sysemu/cpus.h"
> +
> +static void core_prop_get_core(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
> +{
> +    CPUCore *core = CPU_CORE(obj);
> +    int64_t value = core->core;
> +
> +    visit_type_int(v, name, &value, errp);
> +}
> +
> +static void core_prop_set_core(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
> +{
> +    CPUCore *core = CPU_CORE(obj);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, name, &value, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    core->core = value;
> +}
> +
> +static void core_prop_get_threads(Object *obj, Visitor *v, const char *name,
> +                                  void *opaque, Error **errp)
> +{
> +    CPUCore *core = CPU_CORE(obj);
> +    int64_t value = core->threads;
> +
> +    visit_type_int(v, name, &value, errp);
> +}
> +
> +static void core_prop_set_threads(Object *obj, Visitor *v, const char *name,
> +                                  void *opaque, Error **errp)
> +{
> +    CPUCore *core = CPU_CORE(obj);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, name, &value, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    core->threads = value;
> +}
> +
> +static void cpu_core_instance_init(Object *obj)
> +{
> +    CPUCore *core = CPU_CORE(obj);
> +
> +    object_property_add(obj, "core", "int", core_prop_get_core,
> +                        core_prop_set_core, NULL, NULL, NULL);
> +    object_property_add(obj, "threads", "int", core_prop_get_threads,
> +                        core_prop_set_threads, NULL, NULL, NULL);
> +    core->threads = smp_threads;
> +}
> +
> +static const TypeInfo cpu_core_type_info = {
> +    .name = TYPE_CPU_CORE,
> +    .parent = TYPE_DEVICE,
> +    .abstract = true,
> +    .instance_size = sizeof(CPUCore),
> +    .instance_init = cpu_core_instance_init,
> +};
> +
> +static void cpu_core_register_types(void)
> +{
> +    type_register_static(&cpu_core_type_info);
> +}
> +
> +type_init(cpu_core_register_types)
> diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> new file mode 100644
> index 0000000..a2a5a04
> --- /dev/null
> +++ b/include/hw/cpu/core.h
> @@ -0,0 +1,31 @@
> +/*
> + * CPU core abstract device
> + *
> + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef HW_CPU_CORE_H
> +#define HW_CPU_CORE_H
> +
> +#include "qemu/osdep.h"
> +#include "hw/qdev.h"
> +
> +#define TYPE_CPU_CORE "cpu-core"
> +
> +#define CPU_CORE(obj) \
> +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> +
> +typedef struct CPUCore {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +
> +    /*< public >*/
> +    int core;
> +    int threads;
> +} CPUCore;
> +
> +#define CPU_CORE_PROP_CORE "core"
> +
> +#endif

-- 
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 6/9] spapr: CPU core device

[Bharata B Rao]

On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote:
> On Fri, 11 Mar 2016 10:24:35 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > Creating this core device will result in creation of all the CPU thread
> > devices that are part of this core.
> > 
> > Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> > CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> > topologies that result in partially filled cores. Both of these are done
> > only if CPU core hotplug is supported.
> > 
> > Note: An unrelated change in the call to xics_system_init() is done
> > in this patch as it makes sense to use the local variable smt introduced
> > in this patch instead of kvmppc_smt_threads() call here.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/Makefile.objs            |   1 +
> >  hw/ppc/spapr.c                  |  68 +++++++++++---
> >  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h          |   4 +
> >  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
> >  5 files changed, 287 insertions(+), 13 deletions(-)
> >  create mode 100644 hw/ppc/spapr_cpu_core.c
> >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > 
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index c1ffc77..5cc6608 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >  obj-y += spapr_pci_vfio.o
> >  endif
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 64c4acc..cffe8c8 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -64,6 +64,7 @@
> >  
> >  #include "hw/compat.h"
> >  #include "qemu-common.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> >  
> >  #include <libfdt.h>
> >  
> > @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
> >  
> >  }
> >  
> > -static void spapr_cpu_reset(void *opaque)
> > +void spapr_cpu_reset(void *opaque)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      PowerPCCPU *cpu = opaque;
> > @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> >      machine->boot_order = g_strdup(boot_device);
> >  }
> >  
> > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > -                           Error **errp)
> > +/*
> > + * TODO: Check if some of these can be moved to rtas_start_cpu() where
> > + * a few other things required for hotplugged CPUs are being done.
> > + */
> > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> >  {
> >      CPUPPCState *env = &cpu->env;
> >  
> > @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
> >      const char *kernel_filename = machine->kernel_filename;
> >      const char *kernel_cmdline = machine->kernel_cmdline;
> >      const char *initrd_filename = machine->initrd_filename;
> > -    PowerPCCPU *cpu;
> >      PCIHostState *phb;
> >      int i;
> >      MemoryRegion *sysmem = get_system_memory();
> > @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
> >      long load_limit, fw_size;
> >      bool kernel_le = false;
> >      char *filename;
> > +    int smt = kvmppc_smt_threads();
> > +    int spapr_cores = smp_cpus / smp_threads;
> > +    int spapr_max_cores = max_cpus / smp_threads;
> > +
> > +    if (smc->dr_cpu_enabled) {
> > +        if (smp_cpus % smp_threads) {
> > +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > +                         smp_cpus, smp_threads);
> > +            exit(1);
> > +        }
> > +        if (max_cpus % smp_threads) {
> > +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > +                         max_cpus, smp_threads);
> > +            exit(1);
> > +        }
> > +    }
> >  
> >      msi_supported = true;
> >  
> > @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >      /* Set up Interrupt Controller before we create the VCPUs */
> >      spapr->icp = xics_system_init(machine,
> > -                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> > -                                               smp_threads),
> > +                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),
> is smt == smp_threads, if not then what's a difference?

smp_threads is the specified SMT mode of the guest, smt defines the max
SMT guest mode that can be supported on this host.

BTW as I noted in the patch description, this change is unrelated to
CPU hotplug. I did this since I introduced a separate variable (smt)
for kvmppc_smt_threads() and hence replaced the above use of
kvmppc_smt_threads() too.

> 
> >                                    XICS_IRQS, &error_fatal);
> >  
> >      if (smc->dr_lmb_enabled) {
> > @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
> >      if (machine->cpu_model == NULL) {
> >          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> >      }
> > -    for (i = 0; i < smp_cpus; i++) {
> > -        cpu = cpu_ppc_init(machine->cpu_model);
> > -        if (cpu == NULL) {
> > -            error_report("Unable to find PowerPC CPU definition");
> > -            exit(1);
> > +
> > +    if (smc->dr_cpu_enabled) {
> > +        spapr->cores = g_new0(Object *, spapr_max_cores);
> > +
> > +        for (i = 0; i < spapr_max_cores; i++) {
> > +            int core_dt_id = i * smt;
> > +
> > +            if (i < spapr_cores) {
> > +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> > +
> > +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> > +                                        &error_fatal);
> > +                object_property_set_int(core, smp_threads, "threads",
> > +                                        &error_fatal);
> > +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> > +                                        &error_fatal);
> > +                object_property_set_bool(core, true, "realized", &error_fatal);
> > +            }
> >          }
> > -        spapr_cpu_init(spapr, cpu, &error_fatal);
> > +    } else {
> > +        for (i = 0; i < smp_cpus; i++) {
> > +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > +            if (cpu == NULL) {
> > +                error_report("Unable to find PowerPC CPU definition");
> > +                exit(1);
> > +            }
> > +            spapr_cpu_init(spapr, cpu, &error_fatal);
> > +       }
> >      }
> >  
> >      if (kvm_enabled()) {
> > @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> >  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> >                                               DeviceState *dev)
> >  {
> > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > +        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> >          return HOTPLUG_HANDLER(machine);
> >      }
> >      return NULL;
> > @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> >  
> >      smc->dr_lmb_enabled = true;
> > +    smc->dr_cpu_enabled = true;
> >      fwc->get_dev_path = spapr_get_fw_dev_path;
> >      nc->nmi_monitor_handler = spapr_nmi;
> >  }
> > @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
> >  
> >      spapr_machine_2_6_class_options(mc);
> >      smc->use_ohci_by_default = true;
> > +    smc->dr_cpu_enabled = false;
> >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> >  }
> >  
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > new file mode 100644
> > index 0000000..8c6d71d
> > --- /dev/null
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -0,0 +1,199 @@
> > +/*
> > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > + *
> > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#include "hw/cpu/core.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> > +#include "hw/ppc/spapr.h"
> > +#include "hw/boards.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/visitor.h"
> > +#include <sysemu/cpus.h>
> > +#include "target-ppc/kvm_ppc.h"
> > +
> > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> > +                                          Error **errp)
> > +{
> > +    int i;
> > +    Error *local_err = NULL;
> > +
> > +    for (i = 0; i < threads; i++) {
> > +        char id[32];
> > +
> > +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> > +                          object_class_get_name(core->oc));
> > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > +                                  &local_err);
> > +        if (local_err) {
> > +            goto err;
> > +        }
> > +    }
> > +    return;
> > +
> > +err:
> > +    while (--i) {
> > +        object_unparent(OBJECT(&core->threads[i]));
> > +    }
> > +    error_propagate(errp, local_err);
> > +}
> > +
> > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > +{
> > +    Error **errp = opaque;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    CPUState *cs = CPU(child);
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > +    object_property_set_bool(child, true, "realized", errp);
> > +    if (*errp) {
> > +        return 1;
> > +    }
> > +
> > +    spapr_cpu_init(spapr, cpu, errp);
> > +    if (*errp) {
> > +        return 1;
> > +    }
> > +
> > +    spapr_cpu_reset(cpu);
> should it be move to spapr_cpu_init() ?

Could be moved.

> 
> > +    return 0;
> > +}
> > +
> > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    int spapr_max_cores = max_cpus / smp_threads;
> this should be in machine code

With ->pre_plug(), won't have this here.

> 
> > +    Error *local_err = NULL;
> > +    int threads = 0;
> > +    int core_dt_id, core_id;
> > +    int smt = kvmppc_smt_threads();
> > +
> > +    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);
> since spapr_core is inherited from cpu-core,
> you can cast to cpu-core and use fields directly here instead of using
> property setters.

Ah ok.

> 
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    if (threads != smp_threads) {
> > +        error_setg(&local_err, "threads must be %d", smp_threads);
> > +        goto out;
> > +    }
> move to machine handler

Ok, guess you mean pre_plug handler ?

> 
> > +
> > +    if (!core->oc) {
> > +        error_setg(&local_err, "cpu_model property isn't set");
> > +        goto out;
> > +    }
> > +
> > +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    if (core_dt_id % smt) {
> > +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> > +        goto out;
> > +    }
> > +
> > +    core_id = core_dt_id / smt;
> > +    if (core_id < 0 || core_id >= spapr_max_cores) {
> > +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> > +        goto out;
> > +    }
> maybe due to nameing it's a bit confusing,
> what's difference between core_id and core_dt_id?

core_dt_id is the device tree IDs that we use with PowerPC cores. This is
what we use with "core" property of CPU_CORE. Since core_dt_id doesn't
grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host),
I am translating that to contiguous integer core_id so that I can
store the pointer of the realized core in the appropriate slot of
spapr->cpu_cores[] array.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device

[David Gibson]

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

On Tue, Mar 15, 2016 at 02:44:01PM +0530, Bharata B Rao wrote:
> On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote:
> > On Fri, 11 Mar 2016 10:24:35 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > > Creating this core device will result in creation of all the CPU thread
> > > devices that are part of this core.
> > > 
> > > Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> > > CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> > > topologies that result in partially filled cores. Both of these are done
> > > only if CPU core hotplug is supported.
> > > 
> > > Note: An unrelated change in the call to xics_system_init() is done
> > > in this patch as it makes sense to use the local variable smt introduced
> > > in this patch instead of kvmppc_smt_threads() call here.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/Makefile.objs            |   1 +
> > >  hw/ppc/spapr.c                  |  68 +++++++++++---
> > >  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr.h          |   4 +
> > >  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
> > >  5 files changed, 287 insertions(+), 13 deletions(-)
> > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > 
> > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > index c1ffc77..5cc6608 100644
> > > --- a/hw/ppc/Makefile.objs
> > > +++ b/hw/ppc/Makefile.objs
> > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > >  obj-y += spapr_pci_vfio.o
> > >  endif
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 64c4acc..cffe8c8 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -64,6 +64,7 @@
> > >  
> > >  #include "hw/compat.h"
> > >  #include "qemu-common.h"
> > > +#include "hw/ppc/spapr_cpu_core.h"
> > >  
> > >  #include <libfdt.h>
> > >  
> > > @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
> > >  
> > >  }
> > >  
> > > -static void spapr_cpu_reset(void *opaque)
> > > +void spapr_cpu_reset(void *opaque)
> > >  {
> > >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > >      PowerPCCPU *cpu = opaque;
> > > @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> > >      machine->boot_order = g_strdup(boot_device);
> > >  }
> > >  
> > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > > -                           Error **errp)
> > > +/*
> > > + * TODO: Check if some of these can be moved to rtas_start_cpu() where
> > > + * a few other things required for hotplugged CPUs are being done.
> > > + */
> > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > >  {
> > >      CPUPPCState *env = &cpu->env;
> > >  
> > > @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
> > >      const char *kernel_filename = machine->kernel_filename;
> > >      const char *kernel_cmdline = machine->kernel_cmdline;
> > >      const char *initrd_filename = machine->initrd_filename;
> > > -    PowerPCCPU *cpu;
> > >      PCIHostState *phb;
> > >      int i;
> > >      MemoryRegion *sysmem = get_system_memory();
> > > @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
> > >      long load_limit, fw_size;
> > >      bool kernel_le = false;
> > >      char *filename;
> > > +    int smt = kvmppc_smt_threads();
> > > +    int spapr_cores = smp_cpus / smp_threads;
> > > +    int spapr_max_cores = max_cpus / smp_threads;
> > > +
> > > +    if (smc->dr_cpu_enabled) {
> > > +        if (smp_cpus % smp_threads) {
> > > +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > > +                         smp_cpus, smp_threads);
> > > +            exit(1);
> > > +        }
> > > +        if (max_cpus % smp_threads) {
> > > +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > > +                         max_cpus, smp_threads);
> > > +            exit(1);
> > > +        }
> > > +    }
> > >  
> > >      msi_supported = true;
> > >  
> > > @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> > >  
> > >      /* Set up Interrupt Controller before we create the VCPUs */
> > >      spapr->icp = xics_system_init(machine,
> > > -                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> > > -                                               smp_threads),
> > > +                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),
> > is smt == smp_threads, if not then what's a difference?
> 
> smp_threads is the specified SMT mode of the guest, smt defines the max
> SMT guest mode that can be supported on this host.
> 
> BTW as I noted in the patch description, this change is unrelated to
> CPU hotplug. I did this since I introduced a separate variable (smt)
> for kvmppc_smt_threads() and hence replaced the above use of
> kvmppc_smt_threads() too.
> 
> > 
> > >                                    XICS_IRQS, &error_fatal);
> > >  
> > >      if (smc->dr_lmb_enabled) {
> > > @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
> > >      if (machine->cpu_model == NULL) {
> > >          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> > >      }
> > > -    for (i = 0; i < smp_cpus; i++) {
> > > -        cpu = cpu_ppc_init(machine->cpu_model);
> > > -        if (cpu == NULL) {
> > > -            error_report("Unable to find PowerPC CPU definition");
> > > -            exit(1);
> > > +
> > > +    if (smc->dr_cpu_enabled) {
> > > +        spapr->cores = g_new0(Object *, spapr_max_cores);
> > > +
> > > +        for (i = 0; i < spapr_max_cores; i++) {
> > > +            int core_dt_id = i * smt;
> > > +
> > > +            if (i < spapr_cores) {
> > > +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> > > +
> > > +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> > > +                                        &error_fatal);
> > > +                object_property_set_int(core, smp_threads, "threads",
> > > +                                        &error_fatal);
> > > +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> > > +                                        &error_fatal);
> > > +                object_property_set_bool(core, true, "realized", &error_fatal);
> > > +            }
> > >          }
> > > -        spapr_cpu_init(spapr, cpu, &error_fatal);
> > > +    } else {
> > > +        for (i = 0; i < smp_cpus; i++) {
> > > +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > > +            if (cpu == NULL) {
> > > +                error_report("Unable to find PowerPC CPU definition");
> > > +                exit(1);
> > > +            }
> > > +            spapr_cpu_init(spapr, cpu, &error_fatal);
> > > +       }
> > >      }
> > >  
> > >      if (kvm_enabled()) {
> > > @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> > >  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> > >                                               DeviceState *dev)
> > >  {
> > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > > +        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > >          return HOTPLUG_HANDLER(machine);
> > >      }
> > >      return NULL;
> > > @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > >  
> > >      smc->dr_lmb_enabled = true;
> > > +    smc->dr_cpu_enabled = true;
> > >      fwc->get_dev_path = spapr_get_fw_dev_path;
> > >      nc->nmi_monitor_handler = spapr_nmi;
> > >  }
> > > @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
> > >  
> > >      spapr_machine_2_6_class_options(mc);
> > >      smc->use_ohci_by_default = true;
> > > +    smc->dr_cpu_enabled = false;
> > >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> > >  }
> > >  
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > new file mode 100644
> > > index 0000000..8c6d71d
> > > --- /dev/null
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -0,0 +1,199 @@
> > > +/*
> > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > + *
> > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +#include "hw/cpu/core.h"
> > > +#include "hw/ppc/spapr_cpu_core.h"
> > > +#include "hw/ppc/spapr.h"
> > > +#include "hw/boards.h"
> > > +#include "qemu/error-report.h"
> > > +#include "qapi/visitor.h"
> > > +#include <sysemu/cpus.h>
> > > +#include "target-ppc/kvm_ppc.h"
> > > +
> > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> > > +                                          Error **errp)
> > > +{
> > > +    int i;
> > > +    Error *local_err = NULL;
> > > +
> > > +    for (i = 0; i < threads; i++) {
> > > +        char id[32];
> > > +
> > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> > > +                          object_class_get_name(core->oc));
> > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > +                                  &local_err);
> > > +        if (local_err) {
> > > +            goto err;
> > > +        }
> > > +    }
> > > +    return;
> > > +
> > > +err:
> > > +    while (--i) {
> > > +        object_unparent(OBJECT(&core->threads[i]));
> > > +    }
> > > +    error_propagate(errp, local_err);
> > > +}
> > > +
> > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > +{
> > > +    Error **errp = opaque;
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > +    CPUState *cs = CPU(child);
> > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > +
> > > +    object_property_set_bool(child, true, "realized", errp);
> > > +    if (*errp) {
> > > +        return 1;
> > > +    }
> > > +
> > > +    spapr_cpu_init(spapr, cpu, errp);
> > > +    if (*errp) {
> > > +        return 1;
> > > +    }
> > > +
> > > +    spapr_cpu_reset(cpu);
> > should it be move to spapr_cpu_init() ?
> 
> Could be moved.
> 
> > 
> > > +    return 0;
> > > +}
> > > +
> > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > +    int spapr_max_cores = max_cpus / smp_threads;
> > this should be in machine code
> 
> With ->pre_plug(), won't have this here.
> 
> > 
> > > +    Error *local_err = NULL;
> > > +    int threads = 0;
> > > +    int core_dt_id, core_id;
> > > +    int smt = kvmppc_smt_threads();
> > > +
> > > +    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);
> > since spapr_core is inherited from cpu-core,
> > you can cast to cpu-core and use fields directly here instead of using
> > property setters.
> 
> Ah ok.
> 
> > 
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (threads != smp_threads) {
> > > +        error_setg(&local_err, "threads must be %d", smp_threads);
> > > +        goto out;
> > > +    }
> > move to machine handler
> 
> Ok, guess you mean pre_plug handler ?
> 
> > 
> > > +
> > > +    if (!core->oc) {
> > > +        error_setg(&local_err, "cpu_model property isn't set");
> > > +        goto out;
> > > +    }
> > > +
> > > +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (core_dt_id % smt) {
> > > +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> > > +        goto out;
> > > +    }
> > > +
> > > +    core_id = core_dt_id / smt;
> > > +    if (core_id < 0 || core_id >= spapr_max_cores) {
> > > +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> > > +        goto out;
> > > +    }
> > maybe due to nameing it's a bit confusing,
> > what's difference between core_id and core_dt_id?
> 
> core_dt_id is the device tree IDs that we use with PowerPC cores. This is
> what we use with "core" property of CPU_CORE. Since core_dt_id doesn't
> grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host),
> I am translating that to contiguous integer core_id so that I can
> store the pointer of the realized core in the appropriate slot of
> spapr->cpu_cores[] array.

So, I see why the distinction is there, but it is kinda confusing.
I'm wondering if we could do away with the spapr->cores array entirely
and instead just access the core objects via the QOM tree - QOM
"arrays" (i.e. properties named like foo[NNN]) can be sparse, so
there's no need to allocate dense ids.

Alternatively renaming "core_id" to "index" would be marginally less
confusing.

-- 
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 6/9] spapr: CPU core device

[Igor Mammedov]

On Tue, 15 Mar 2016 14:44:01 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote:
> > On Fri, 11 Mar 2016 10:24:35 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > > Creating this core device will result in creation of all the CPU thread
> > > devices that are part of this core.
> > > 
> > > Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> > > CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> > > topologies that result in partially filled cores. Both of these are done
> > > only if CPU core hotplug is supported.
> > > 
> > > Note: An unrelated change in the call to xics_system_init() is done
> > > in this patch as it makes sense to use the local variable smt introduced
> > > in this patch instead of kvmppc_smt_threads() call here.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/Makefile.objs            |   1 +
> > >  hw/ppc/spapr.c                  |  68 +++++++++++---
> > >  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr.h          |   4 +
> > >  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
> > >  5 files changed, 287 insertions(+), 13 deletions(-)
> > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > 
> > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > index c1ffc77..5cc6608 100644
> > > --- a/hw/ppc/Makefile.objs
> > > +++ b/hw/ppc/Makefile.objs
> > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > >  obj-y += spapr_pci_vfio.o
> > >  endif
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 64c4acc..cffe8c8 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -64,6 +64,7 @@
> > >  
> > >  #include "hw/compat.h"
> > >  #include "qemu-common.h"
> > > +#include "hw/ppc/spapr_cpu_core.h"
> > >  
> > >  #include <libfdt.h>
> > >  
> > > @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
> > >  
> > >  }
> > >  
> > > -static void spapr_cpu_reset(void *opaque)
> > > +void spapr_cpu_reset(void *opaque)
> > >  {
> > >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > >      PowerPCCPU *cpu = opaque;
> > > @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> > >      machine->boot_order = g_strdup(boot_device);
> > >  }
> > >  
> > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > > -                           Error **errp)
> > > +/*
> > > + * TODO: Check if some of these can be moved to rtas_start_cpu() where
> > > + * a few other things required for hotplugged CPUs are being done.
> > > + */
> > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > >  {
> > >      CPUPPCState *env = &cpu->env;
> > >  
> > > @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
> > >      const char *kernel_filename = machine->kernel_filename;
> > >      const char *kernel_cmdline = machine->kernel_cmdline;
> > >      const char *initrd_filename = machine->initrd_filename;
> > > -    PowerPCCPU *cpu;
> > >      PCIHostState *phb;
> > >      int i;
> > >      MemoryRegion *sysmem = get_system_memory();
> > > @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
> > >      long load_limit, fw_size;
> > >      bool kernel_le = false;
> > >      char *filename;
> > > +    int smt = kvmppc_smt_threads();
> > > +    int spapr_cores = smp_cpus / smp_threads;
> > > +    int spapr_max_cores = max_cpus / smp_threads;
> > > +
> > > +    if (smc->dr_cpu_enabled) {
> > > +        if (smp_cpus % smp_threads) {
> > > +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > > +                         smp_cpus, smp_threads);
> > > +            exit(1);
> > > +        }
> > > +        if (max_cpus % smp_threads) {
> > > +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > > +                         max_cpus, smp_threads);
> > > +            exit(1);
> > > +        }
> > > +    }
> > >  
> > >      msi_supported = true;
> > >  
> > > @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> > >  
> > >      /* Set up Interrupt Controller before we create the VCPUs */
> > >      spapr->icp = xics_system_init(machine,
> > > -                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> > > -                                               smp_threads),
> > > +                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),  
> > is smt == smp_threads, if not then what's a difference?  
> 
> smp_threads is the specified SMT mode of the guest, smt defines the max
> SMT guest mode that can be supported on this host.
> 
> BTW as I noted in the patch description, this change is unrelated to
> CPU hotplug. I did this since I introduced a separate variable (smt)
> for kvmppc_smt_threads() and hence replaced the above use of
> kvmppc_smt_threads() too.
> 
> >   
> > >                                    XICS_IRQS, &error_fatal);
> > >  
> > >      if (smc->dr_lmb_enabled) {
> > > @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
> > >      if (machine->cpu_model == NULL) {
> > >          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> > >      }
> > > -    for (i = 0; i < smp_cpus; i++) {
> > > -        cpu = cpu_ppc_init(machine->cpu_model);
> > > -        if (cpu == NULL) {
> > > -            error_report("Unable to find PowerPC CPU definition");
> > > -            exit(1);
> > > +
> > > +    if (smc->dr_cpu_enabled) {
> > > +        spapr->cores = g_new0(Object *, spapr_max_cores);
> > > +
> > > +        for (i = 0; i < spapr_max_cores; i++) {
> > > +            int core_dt_id = i * smt;
> > > +
> > > +            if (i < spapr_cores) {
> > > +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> > > +
> > > +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> > > +                                        &error_fatal);
> > > +                object_property_set_int(core, smp_threads, "threads",
> > > +                                        &error_fatal);
> > > +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> > > +                                        &error_fatal);
> > > +                object_property_set_bool(core, true, "realized", &error_fatal);
> > > +            }
> > >          }
> > > -        spapr_cpu_init(spapr, cpu, &error_fatal);
> > > +    } else {
> > > +        for (i = 0; i < smp_cpus; i++) {
> > > +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > > +            if (cpu == NULL) {
> > > +                error_report("Unable to find PowerPC CPU definition");
> > > +                exit(1);
> > > +            }
> > > +            spapr_cpu_init(spapr, cpu, &error_fatal);
> > > +       }
> > >      }
> > >  
> > >      if (kvm_enabled()) {
> > > @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> > >  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> > >                                               DeviceState *dev)
> > >  {
> > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > > +        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > >          return HOTPLUG_HANDLER(machine);
> > >      }
> > >      return NULL;
> > > @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > >  
> > >      smc->dr_lmb_enabled = true;
> > > +    smc->dr_cpu_enabled = true;
> > >      fwc->get_dev_path = spapr_get_fw_dev_path;
> > >      nc->nmi_monitor_handler = spapr_nmi;
> > >  }
> > > @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
> > >  
> > >      spapr_machine_2_6_class_options(mc);
> > >      smc->use_ohci_by_default = true;
> > > +    smc->dr_cpu_enabled = false;
> > >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> > >  }
> > >  
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > new file mode 100644
> > > index 0000000..8c6d71d
> > > --- /dev/null
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -0,0 +1,199 @@
> > > +/*
> > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > + *
> > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +#include "hw/cpu/core.h"
> > > +#include "hw/ppc/spapr_cpu_core.h"
> > > +#include "hw/ppc/spapr.h"
> > > +#include "hw/boards.h"
> > > +#include "qemu/error-report.h"
> > > +#include "qapi/visitor.h"
> > > +#include <sysemu/cpus.h>
> > > +#include "target-ppc/kvm_ppc.h"
> > > +
> > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> > > +                                          Error **errp)
> > > +{
> > > +    int i;
> > > +    Error *local_err = NULL;
> > > +
> > > +    for (i = 0; i < threads; i++) {
> > > +        char id[32];
> > > +
> > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> > > +                          object_class_get_name(core->oc));
> > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > +                                  &local_err);
> > > +        if (local_err) {
> > > +            goto err;
> > > +        }
> > > +    }
> > > +    return;
> > > +
> > > +err:
> > > +    while (--i) {
> > > +        object_unparent(OBJECT(&core->threads[i]));
> > > +    }
> > > +    error_propagate(errp, local_err);
> > > +}
> > > +
> > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > +{
> > > +    Error **errp = opaque;
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > +    CPUState *cs = CPU(child);
> > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > +
> > > +    object_property_set_bool(child, true, "realized", errp);
> > > +    if (*errp) {
> > > +        return 1;
> > > +    }
> > > +
> > > +    spapr_cpu_init(spapr, cpu, errp);
> > > +    if (*errp) {
> > > +        return 1;
> > > +    }
> > > +
> > > +    spapr_cpu_reset(cpu);  
> > should it be move to spapr_cpu_init() ?  
> 
> Could be moved.
> 
> >   
> > > +    return 0;
> > > +}
> > > +
> > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > +    int spapr_max_cores = max_cpus / smp_threads;  
> > this should be in machine code  
> 
> With ->pre_plug(), won't have this here.
> 
> >   
> > > +    Error *local_err = NULL;
> > > +    int threads = 0;
> > > +    int core_dt_id, core_id;
> > > +    int smt = kvmppc_smt_threads();
> > > +
> > > +    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);  
> > since spapr_core is inherited from cpu-core,
> > you can cast to cpu-core and use fields directly here instead of using
> > property setters.  
> 
> Ah ok.
> 
> >   
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (threads != smp_threads) {
> > > +        error_setg(&local_err, "threads must be %d", smp_threads);
> > > +        goto out;
> > > +    }  
> > move to machine handler  
> 
> Ok, guess you mean pre_plug handler ?
yep

> 
> >   
> > > +
> > > +    if (!core->oc) {
> > > +        error_setg(&local_err, "cpu_model property isn't set");
> > > +        goto out;
> > > +    }
> > > +
> > > +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (core_dt_id % smt) {
> > > +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> > > +        goto out;
> > > +    }
> > > +
> > > +    core_id = core_dt_id / smt;
> > > +    if (core_id < 0 || core_id >= spapr_max_cores) {
> > > +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> > > +        goto out;
> > > +    }  
> > maybe due to nameing it's a bit confusing,
> > what's difference between core_id and core_dt_id?  
> 
> core_dt_id is the device tree IDs that we use with PowerPC cores. This is
> what we use with "core" property of CPU_CORE. Since core_dt_id doesn't
> grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host),
> I am translating that to contiguous integer core_id so that I can
> store the pointer of the realized core in the appropriate slot of
> spapr->cpu_cores[] array.
So maybe rename core_id to something else so it wouldn't be confusing.
Also I'd move out core_id check to pre_plug handler as it's an internal
machine index (including spapr_max_cores).

> 
> Regards,
> Bharata.
> 
> 

Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device

[Igor Mammedov]

On Tue, 15 Mar 2016 20:34:28 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Mar 15, 2016 at 02:44:01PM +0530, Bharata B Rao wrote:
> > On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote:  
> > > On Fri, 11 Mar 2016 10:24:35 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >   
> > > > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > > > Creating this core device will result in creation of all the CPU thread
> > > > devices that are part of this core.
> > > > 
> > > > Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> > > > CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> > > > topologies that result in partially filled cores. Both of these are done
> > > > only if CPU core hotplug is supported.
> > > > 
> > > > Note: An unrelated change in the call to xics_system_init() is done
> > > > in this patch as it makes sense to use the local variable smt introduced
> > > > in this patch instead of kvmppc_smt_threads() call here.
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/ppc/Makefile.objs            |   1 +
> > > >  hw/ppc/spapr.c                  |  68 +++++++++++---
> > > >  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/ppc/spapr.h          |   4 +
> > > >  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
> > > >  5 files changed, 287 insertions(+), 13 deletions(-)
> > > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > > 
> > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > index c1ffc77..5cc6608 100644
> > > > --- a/hw/ppc/Makefile.objs
> > > > +++ b/hw/ppc/Makefile.objs
> > > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > > >  obj-y += spapr_pci_vfio.o
> > > >  endif
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 64c4acc..cffe8c8 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -64,6 +64,7 @@
> > > >  
> > > >  #include "hw/compat.h"
> > > >  #include "qemu-common.h"
> > > > +#include "hw/ppc/spapr_cpu_core.h"
> > > >  
> > > >  #include <libfdt.h>
> > > >  
> > > > @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
> > > >  
> > > >  }
> > > >  
> > > > -static void spapr_cpu_reset(void *opaque)
> > > > +void spapr_cpu_reset(void *opaque)
> > > >  {
> > > >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > >      PowerPCCPU *cpu = opaque;
> > > > @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> > > >      machine->boot_order = g_strdup(boot_device);
> > > >  }
> > > >  
> > > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > > > -                           Error **errp)
> > > > +/*
> > > > + * TODO: Check if some of these can be moved to rtas_start_cpu() where
> > > > + * a few other things required for hotplugged CPUs are being done.
> > > > + */
> > > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > > >  {
> > > >      CPUPPCState *env = &cpu->env;
> > > >  
> > > > @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
> > > >      const char *kernel_filename = machine->kernel_filename;
> > > >      const char *kernel_cmdline = machine->kernel_cmdline;
> > > >      const char *initrd_filename = machine->initrd_filename;
> > > > -    PowerPCCPU *cpu;
> > > >      PCIHostState *phb;
> > > >      int i;
> > > >      MemoryRegion *sysmem = get_system_memory();
> > > > @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
> > > >      long load_limit, fw_size;
> > > >      bool kernel_le = false;
> > > >      char *filename;
> > > > +    int smt = kvmppc_smt_threads();
> > > > +    int spapr_cores = smp_cpus / smp_threads;
> > > > +    int spapr_max_cores = max_cpus / smp_threads;
> > > > +
> > > > +    if (smc->dr_cpu_enabled) {
> > > > +        if (smp_cpus % smp_threads) {
> > > > +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > > > +                         smp_cpus, smp_threads);
> > > > +            exit(1);
> > > > +        }
> > > > +        if (max_cpus % smp_threads) {
> > > > +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > > > +                         max_cpus, smp_threads);
> > > > +            exit(1);
> > > > +        }
> > > > +    }
> > > >  
> > > >      msi_supported = true;
> > > >  
> > > > @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> > > >  
> > > >      /* Set up Interrupt Controller before we create the VCPUs */
> > > >      spapr->icp = xics_system_init(machine,
> > > > -                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> > > > -                                               smp_threads),
> > > > +                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),  
> > > is smt == smp_threads, if not then what's a difference?  
> > 
> > smp_threads is the specified SMT mode of the guest, smt defines the max
> > SMT guest mode that can be supported on this host.
> > 
> > BTW as I noted in the patch description, this change is unrelated to
> > CPU hotplug. I did this since I introduced a separate variable (smt)
> > for kvmppc_smt_threads() and hence replaced the above use of
> > kvmppc_smt_threads() too.
> >   
> > >   
> > > >                                    XICS_IRQS, &error_fatal);
> > > >  
> > > >      if (smc->dr_lmb_enabled) {
> > > > @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
> > > >      if (machine->cpu_model == NULL) {
> > > >          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> > > >      }
> > > > -    for (i = 0; i < smp_cpus; i++) {
> > > > -        cpu = cpu_ppc_init(machine->cpu_model);
> > > > -        if (cpu == NULL) {
> > > > -            error_report("Unable to find PowerPC CPU definition");
> > > > -            exit(1);
> > > > +
> > > > +    if (smc->dr_cpu_enabled) {
> > > > +        spapr->cores = g_new0(Object *, spapr_max_cores);
> > > > +
> > > > +        for (i = 0; i < spapr_max_cores; i++) {
> > > > +            int core_dt_id = i * smt;
> > > > +
> > > > +            if (i < spapr_cores) {
> > > > +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> > > > +
> > > > +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> > > > +                                        &error_fatal);
> > > > +                object_property_set_int(core, smp_threads, "threads",
> > > > +                                        &error_fatal);
> > > > +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> > > > +                                        &error_fatal);
> > > > +                object_property_set_bool(core, true, "realized", &error_fatal);
> > > > +            }
> > > >          }
> > > > -        spapr_cpu_init(spapr, cpu, &error_fatal);
> > > > +    } else {
> > > > +        for (i = 0; i < smp_cpus; i++) {
> > > > +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > > > +            if (cpu == NULL) {
> > > > +                error_report("Unable to find PowerPC CPU definition");
> > > > +                exit(1);
> > > > +            }
> > > > +            spapr_cpu_init(spapr, cpu, &error_fatal);
> > > > +       }
> > > >      }
> > > >  
> > > >      if (kvm_enabled()) {
> > > > @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> > > >  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> > > >                                               DeviceState *dev)
> > > >  {
> > > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > > > +        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > >          return HOTPLUG_HANDLER(machine);
> > > >      }
> > > >      return NULL;
> > > > @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > > >  
> > > >      smc->dr_lmb_enabled = true;
> > > > +    smc->dr_cpu_enabled = true;
> > > >      fwc->get_dev_path = spapr_get_fw_dev_path;
> > > >      nc->nmi_monitor_handler = spapr_nmi;
> > > >  }
> > > > @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
> > > >  
> > > >      spapr_machine_2_6_class_options(mc);
> > > >      smc->use_ohci_by_default = true;
> > > > +    smc->dr_cpu_enabled = false;
> > > >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> > > >  }
> > > >  
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > new file mode 100644
> > > > index 0000000..8c6d71d
> > > > --- /dev/null
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -0,0 +1,199 @@
> > > > +/*
> > > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > > + *
> > > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > + * See the COPYING file in the top-level directory.
> > > > + */
> > > > +#include "hw/cpu/core.h"
> > > > +#include "hw/ppc/spapr_cpu_core.h"
> > > > +#include "hw/ppc/spapr.h"
> > > > +#include "hw/boards.h"
> > > > +#include "qemu/error-report.h"
> > > > +#include "qapi/visitor.h"
> > > > +#include <sysemu/cpus.h>
> > > > +#include "target-ppc/kvm_ppc.h"
> > > > +
> > > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> > > > +                                          Error **errp)
> > > > +{
> > > > +    int i;
> > > > +    Error *local_err = NULL;
> > > > +
> > > > +    for (i = 0; i < threads; i++) {
> > > > +        char id[32];
> > > > +
> > > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> > > > +                          object_class_get_name(core->oc));
> > > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > > +                                  &local_err);
> > > > +        if (local_err) {
> > > > +            goto err;
> > > > +        }
> > > > +    }
> > > > +    return;
> > > > +
> > > > +err:
> > > > +    while (--i) {
> > > > +        object_unparent(OBJECT(&core->threads[i]));
> > > > +    }
> > > > +    error_propagate(errp, local_err);
> > > > +}
> > > > +
> > > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > > +{
> > > > +    Error **errp = opaque;
> > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > > +    CPUState *cs = CPU(child);
> > > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > > +
> > > > +    object_property_set_bool(child, true, "realized", errp);
> > > > +    if (*errp) {
> > > > +        return 1;
> > > > +    }
> > > > +
> > > > +    spapr_cpu_init(spapr, cpu, errp);
> > > > +    if (*errp) {
> > > > +        return 1;
> > > > +    }
> > > > +
> > > > +    spapr_cpu_reset(cpu);  
> > > should it be move to spapr_cpu_init() ?  
> > 
> > Could be moved.
> >   
> > >   
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > > +    int spapr_max_cores = max_cpus / smp_threads;  
> > > this should be in machine code  
> > 
> > With ->pre_plug(), won't have this here.
> >   
> > >   
> > > > +    Error *local_err = NULL;
> > > > +    int threads = 0;
> > > > +    int core_dt_id, core_id;
> > > > +    int smt = kvmppc_smt_threads();
> > > > +
> > > > +    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);  
> > > since spapr_core is inherited from cpu-core,
> > > you can cast to cpu-core and use fields directly here instead of using
> > > property setters.  
> > 
> > Ah ok.
> >   
> > >   
> > > > +    if (local_err) {
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    if (threads != smp_threads) {
> > > > +        error_setg(&local_err, "threads must be %d", smp_threads);
> > > > +        goto out;
> > > > +    }  
> > > move to machine handler  
> > 
> > Ok, guess you mean pre_plug handler ?
> >   
> > >   
> > > > +
> > > > +    if (!core->oc) {
> > > > +        error_setg(&local_err, "cpu_model property isn't set");
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> > > > +    if (local_err) {
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    if (core_dt_id % smt) {
> > > > +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    core_id = core_dt_id / smt;
> > > > +    if (core_id < 0 || core_id >= spapr_max_cores) {
> > > > +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> > > > +        goto out;
> > > > +    }  
> > > maybe due to nameing it's a bit confusing,
> > > what's difference between core_id and core_dt_id?  
> > 
> > core_dt_id is the device tree IDs that we use with PowerPC cores. This is
> > what we use with "core" property of CPU_CORE. Since core_dt_id doesn't
> > grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host),
> > I am translating that to contiguous integer core_id so that I can
> > store the pointer of the realized core in the appropriate slot of
> > spapr->cpu_cores[] array.  
> 
> So, I see why the distinction is there, but it is kinda confusing.
> I'm wondering if we could do away with the spapr->cores array entirely
> and instead just access the core objects via the QOM tree - QOM
> "arrays" (i.e. properties named like foo[NNN]) can be sparse, so
> there's no need to allocate dense ids.
Wouldn't be lookups for duplicate in QOM tree take O(N^2)
when hot-plugging N cpus?

It should be less with sorted array at least.

> Alternatively renaming "core_id" to "index" would be marginally less
> confusing.
> 

Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device

[David Gibson]

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

On Tue, Mar 15, 2016 at 02:46:37PM +0100, Igor Mammedov wrote:
> On Tue, 15 Mar 2016 20:34:28 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Tue, Mar 15, 2016 at 02:44:01PM +0530, Bharata B Rao wrote:
> > > On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote:  
> > > > On Fri, 11 Mar 2016 10:24:35 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
[snip]
> > > > > +    if (!core->oc) {
> > > > > +        error_setg(&local_err, "cpu_model property isn't set");
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> > > > > +    if (local_err) {
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    if (core_dt_id % smt) {
> > > > > +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    core_id = core_dt_id / smt;
> > > > > +    if (core_id < 0 || core_id >= spapr_max_cores) {
> > > > > +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> > > > > +        goto out;
> > > > > +    }  
> > > > maybe due to nameing it's a bit confusing,
> > > > what's difference between core_id and core_dt_id?  
> > > 
> > > core_dt_id is the device tree IDs that we use with PowerPC cores. This is
> > > what we use with "core" property of CPU_CORE. Since core_dt_id doesn't
> > > grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host),
> > > I am translating that to contiguous integer core_id so that I can
> > > store the pointer of the realized core in the appropriate slot of
> > > spapr->cpu_cores[] array.  
> > 
> > So, I see why the distinction is there, but it is kinda confusing.
> > I'm wondering if we could do away with the spapr->cores array entirely
> > and instead just access the core objects via the QOM tree - QOM
> > "arrays" (i.e. properties named like foo[NNN]) can be sparse, so
> > there's no need to allocate dense ids.
> Wouldn't be lookups for duplicate in QOM tree take O(N^2)
> when hot-plugging N cpus?

With the present QOM implementation, yes, although I know Paolo has
made noises about changing that to a hash table.

> It should be less with sorted array at least.

It would, but I doubt the O(N^2) will actually be a problem with
realistic numbers of cpus.

-- 
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/9] Core based CPU hotplug for PowerPC sPAPR

[Bharata B Rao]

On Mon, Mar 14, 2016 at 10:47:28AM +0100, Igor Mammedov wrote:
> On Fri, 11 Mar 2016 10:24:29 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Hi,
> > 
> > This is the next version of "Core based CPU hotplug for PowerPC sPAPR" that
> > was posted at
> > https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html
> > 
> > device_add semantics
> > --------------------
> > For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
> > (qemu) device_add spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]
> do you plan to allow user to hotplug different cpu_models?
> If not it would be better to hide cpu_model from user
> and set it from machine pre_plug handler.

In my earlier implementations I derived cpu model from -cpu and threads from
-smp,threads= commandline options and never exposed them to device_add
command.

Though we don't support heterogenous systems (different cpu models and/or
threads) now, it was felt that it should be easy enough to support such
systems if required in future, that's how cpu_model and threads became
options for device_add.

One of the things that David felt was missing from my earlier QMP query
command (and which is true in your QMP query implementation also) is that
we aren't exporting cpu_model at all, at least for not-yet-plugged cores.
So should we include that or let management figure that out since it
would already know about the CPU model.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v2 7/9] spapr: CPU hotplug support

[David Gibson]

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

On Fri, Mar 11, 2016 at 10:24:36AM +0530, Bharata B Rao wrote:
> Set up device tree entries for the hotplugged CPU core and use the
> exising RTAS event logging infrastructure to send CPU hotplug notification
> to the guest.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c                  | 64 ++++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_cpu_core.c         | 69 +++++++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_events.c           |  3 ++
>  hw/ppc/spapr_rtas.c             | 24 ++++++++++++++
>  include/hw/ppc/spapr.h          |  2 ++
>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>  6 files changed, 164 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cffe8c8..822c87d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -603,6 +603,18 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>      size_t page_sizes_prop_size;
>      uint32_t vcpus_per_socket = smp_threads * smp_cores;
>      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +    int drc_index;
> +
> +    if (smc->dr_cpu_enabled) {
> +        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
> +        g_assert(drc);

Small nit: rather than asserting here it might be simpler to just
check for drc != NULL instead of checking smc->dr_cpu_enabled.  That
should have the same effect for now, and will be correct if we ever
have some pluggable and some non-pluggable CPUs...

> +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +        drc_index = drck->get_index(drc);
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
> +    }
>  
>      /* Note: we keep CI large pages off for now because a 64K capable guest
>       * provisioned with large pages might otherwise try to map a qemu
> @@ -987,6 +999,16 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>          _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
>      }
>  
> +    if (smc->dr_cpu_enabled) {
> +        int offset = fdt_path_offset(fdt, "/cpus");
> +        ret = spapr_drc_populate_dt(fdt, offset, NULL,
> +                                    SPAPR_DR_CONNECTOR_TYPE_CPU);
> +        if (ret < 0) {
> +            error_report("Couldn't set up CPU DR device tree properties");
> +            exit(1);
> +        }
> +    }
> +
>      _FDT((fdt_pack(fdt)));
>  
>      if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> @@ -1622,6 +1644,8 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>  void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
> +    CPUState *cs = CPU(cpu);
> +    int i;
>  
>      /* Set time-base frequency to 512 MHz */
>      cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> @@ -1646,6 +1670,14 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
>          }
>      }
>  
> +    /* Set NUMA node for the added CPUs  */
> +    for (i = 0; i < nb_numa_nodes; i++) {
> +        if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) {
> +            cs->numa_node = i;
> +            break;
> +        }
> +    }
> +
>      xics_cpu_setup(spapr->icp, cpu);
>  
>      qemu_register_reset(spapr_cpu_reset, cpu);
> @@ -1824,6 +1856,11 @@ static void ppc_spapr_init(MachineState *machine)
>  
>          for (i = 0; i < spapr_max_cores; i++) {
>              int core_dt_id = i * smt;
> +            sPAPRDRConnector *drc =
> +                spapr_dr_connector_new(OBJECT(spapr),
> +                                       SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
> +
> +            qemu_register_reset(spapr_drc_reset, drc);

...at least it will be if you make construction of the DRC object
conditional on dr_cpu_enabled.

>              if (i < spapr_cores) {
>                  Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> @@ -2246,6 +2283,27 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> +void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> +                                    int *fdt_offset, sPAPRMachineState *spapr)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    DeviceClass *dc = DEVICE_GET_CLASS(cs);
> +    int id = ppc_get_vcpu_dt_id(cpu);
> +    void *fdt;
> +    int offset, fdt_size;
> +    char *nodename;
> +
> +    fdt = create_device_tree(&fdt_size);
> +    nodename = g_strdup_printf("%s@%x", dc->fw_name, id);
> +    offset = fdt_add_subnode(fdt, 0, nodename);
> +
> +    spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> +    g_free(nodename);
> +
> +    *fdt_offset = offset;
> +    return fdt;
> +}
> +
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> @@ -2286,6 +2344,12 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>          }
>  
>          spapr_memory_plug(hotplug_dev, dev, node, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> +            error_setg(errp, "CPU hotplug not supported for this machine");
> +            return;
> +        }
> +        spapr_core_plug(hotplug_dev, dev, errp);
>      }
>  }
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 8c6d71d..db8de32 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -15,6 +15,75 @@
>  #include <sysemu/cpus.h>
>  #include "target-ppc/kvm_ppc.h"
>  
> +void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                     Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +    CPUState *cs = CPU(&core->threads[0]);
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +    Error *local_err = NULL;
> +    void *fdt = NULL;
> +    int fdt_offset = 0;
> +    int core_id, core_dt_id;
> +    int smt = kvmppc_smt_threads();
> +
> +    /* TODO: Should we cache core_id in sPAPRCPUCore ? */
> +    core_dt_id = object_property_get_int(OBJECT(dev), "core",
> &local_err);

As Igor pointed out elsewhere you should just be able to do
CPU_CORE(dev)->core.

> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
> +    core_id = core_dt_id / smt;
> +    spapr->cores[core_id] = OBJECT(dev);
> +
> +    if (!smc->dr_cpu_enabled) {
> +        /*
> +         * This is a cold plugged CPU core but the machine doesn't support
> +         * DR. So skip the hotplug path ensuring that the core is brought
> +         * up online with out an associated DR connector.
> +         */
> +        return;
> +    }
> +
> +    g_assert(drc);
> +
> +    /*
> +     * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> +     * coldplugged CPUs DT entries are setup in spapr_finalize_fdt().
> +     */
> +    if (dev->hotplugged) {
> +        fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, spapr);
> +        dev->hotplugged = true;
> +    }
> +
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> +    if (local_err) {
> +        g_free(fdt);
> +        spapr->cores[core_id] = NULL;
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (dev->hotplugged) {
> +        /*
> +         * Send hotplug notification interrupt to the guest only in case
> +         * of hotplugged CPUs.
> +         */
> +        spapr_hotplug_req_add_by_index(drc);
> +    } else {
> +        /*
> +         * Set the right DRC states for cold plugged CPU.
> +         */
> +        drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> +        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> +    }
> +}
> +
>  static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
>                                            Error **errp)
>  {
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 39f4682..10340e1 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -437,6 +437,9 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>      case SPAPR_DR_CONNECTOR_TYPE_LMB:
>          hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
>          break;
> +    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> +        hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_CPU;
> +        break;
>      default:
>          /* we shouldn't be signaling hotplug events for resources
>           * that don't support them
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index b7c5ebd..cc0369e 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -34,6 +34,7 @@
>  
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_vio.h"
> +#include "hw/ppc/ppc.h"
>  #include "qapi-event.h"
>  #include "hw/boards.h"
>  
> @@ -161,6 +162,27 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
>      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>  }
>  
> +/*
> + * Set the timebase offset of the CPU to that of first CPU.
> + * This helps hotplugged CPU to have the correct timebase offset.
> + */
> +static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
> +{
> +    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> +
> +    cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
> +}
> +
> +static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> +{
> +    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> +
> +    if (!pcc->interrupts_big_endian(fcpu)) {
> +        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> +    }
> +}
> +
>  static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
>                             uint32_t token, uint32_t nargs,
>                             target_ulong args,
> @@ -197,6 +219,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
>          env->nip = start;
>          env->gpr[3] = r3;
>          cs->halted = 0;
> +        spapr_cpu_set_endianness(cpu);
> +        spapr_cpu_update_tb_offset(cpu);
>  
>          qemu_cpu_kick(cs);
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index c099c3c..8957072 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -589,6 +589,8 @@ void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
>                                            uint32_t count);
>  void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
>  void spapr_cpu_reset(void *opaque);
> +void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> +                                    int *fdt_offset, sPAPRMachineState *spapr);
>  
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 48fb76a..980d8ae 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -25,4 +25,6 @@ typedef struct sPAPRCPUCore {
>      PowerPCCPU *threads;
>  } sPAPRCPUCore;
>  
> +void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                     Error **errp);
>  #endif

-- 
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 9/9] spapr: CPU hot unplug support

[David Gibson]

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

On Fri, Mar 11, 2016 at 10:24:38AM +0530, Bharata B Rao wrote:
> Remove the CPU core device by removing the underlying CPU thread devices.
> Hot removal of CPU for sPAPR guests is achieved by sending the hot unplug
> notification to the guest. Release the vCPU object after CPU hot unplug so
> that vCPU fd can be parked and reused.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c                  | 21 ++++++++++
>  hw/ppc/spapr_cpu_core.c         | 86 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |  1 +
>  include/hw/ppc/spapr_cpu_core.h | 12 ++++++
>  4 files changed, 120 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 822c87d..b1e9ba2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2345,7 +2345,12 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>  
>          spapr_memory_plug(hotplug_dev, dev, node, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> +        /*
> +         * TODO: Move this check to pre_plug handler at which point
> +         * spapr_core_release() won't be necessary.
> +         */
>          if (!smc->dr_cpu_enabled && dev->hotplugged) {
> +            spapr_core_release(dev);
>              error_setg(errp, "CPU hotplug not supported for this machine");
>              return;
>          }

This hunk doesn't look like its related to unplug.  Did it belong in
another patch?

> @@ -2353,11 +2358,27 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>      }
>  }
>  
> +void spapr_cpu_destroy(PowerPCCPU *cpu)
> +{
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +
> +    xics_cpu_destroy(spapr->icp, cpu);
> +    qemu_unregister_reset(spapr_cpu_reset, cpu);
> +}
> +
>  static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          error_setg(errp, "Memory hot unplug not supported by sPAPR");
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> +        if (!smc->dr_cpu_enabled) {
> +            error_setg(errp, "CPU hot unplug not supported on this machine");
> +            return;
> +        }
> +        spapr_core_unplug(hotplug_dev, dev, errp);
>      }
>  }
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index db8de32..dd391bd 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -84,6 +84,92 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      }
>  }
>  
> +static void spapr_cpu_core_cleanup(struct sPAPRCPUUnplugList *unplug_list)
> +{
> +    sPAPRCPUUnplug *unplug, *next;
> +    Object *cpu;
> +
> +    QLIST_FOREACH_SAFE(unplug, unplug_list, node, next) {
> +        cpu = unplug->cpu;
> +        object_unparent(cpu);
> +        QLIST_REMOVE(unplug, node);
> +        g_free(unplug);
> +    }
> +}
> +
> +static void spapr_add_cpu_to_unplug_list(Object *cpu,
> +                                         struct sPAPRCPUUnplugList *unplug_list)
> +{
> +    sPAPRCPUUnplug *unplug = g_malloc(sizeof(*unplug));
> +
> +    unplug->cpu = cpu;
> +    QLIST_INSERT_HEAD(unplug_list, unplug, node);
> +}
> +
> +static int spapr_cpu_release(Object *obj, void *opaque)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    CPUState *cs = CPU(dev);
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    struct sPAPRCPUUnplugList *unplug_list = opaque;
> +
> +    spapr_cpu_destroy(cpu);
> +    cpu_remove_sync(cs);
> +
> +    /*
> +     * We are still walking the core object's children list, and
> +     * hence can't cleanup this CPU thread object just yet. Put
> +     * it on a list for later removal.
> +     */
> +    spapr_add_cpu_to_unplug_list(obj, unplug_list);
> +    return 0;
> +}
> +
> +void spapr_core_release(DeviceState *dev)
> +{
> +    struct sPAPRCPUUnplugList unplug_list;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +    int core_dt_id = object_property_get_int(OBJECT(dev), "core", NULL);
> +    int smt = kvmppc_smt_threads();
> +
> +    QLIST_INIT(&unplug_list);
> +    object_child_foreach(OBJECT(dev), spapr_cpu_release, &unplug_list);
> +    spapr_cpu_core_cleanup(&unplug_list);
> +    spapr->cores[core_dt_id / smt] = NULL;
> +
> +    g_free(core->threads);
> +}
> +
> +static void spapr_core_release_unparent(DeviceState *dev, void *opaque)
> +{
> +    spapr_core_release(dev);
> +    object_unparent(OBJECT(dev));
> +}
> +
> +void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                       Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +    PowerPCCPU *cpu = &core->threads[0];
> +    int id = ppc_get_vcpu_dt_id(cpu);
> +    sPAPRDRConnector *drc =
> +        spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> +    sPAPRDRConnectorClass *drck;
> +    Error *local_err = NULL;
> +
> +    g_assert(drc);
> +
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    drck->detach(drc, dev, spapr_core_release_unparent, NULL, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    spapr_hotplug_req_remove_by_index(drc);
> +}
> +
>  static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
>                                            Error **errp)
>  {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 8957072..41d0928 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -591,6 +591,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
>  void spapr_cpu_reset(void *opaque);
>  void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
>                                      int *fdt_offset, sPAPRMachineState *spapr);
> +void spapr_cpu_destroy(PowerPCCPU *cpu);
>  
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 980d8ae..75c6c58 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -27,4 +27,16 @@ typedef struct sPAPRCPUCore {
>  
>  void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                       Error **errp);
> +void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                       Error **errp);
> +void spapr_core_release(DeviceState *dev);
> +
> +/* List to store unplugged CPU objects for cleanup during unplug */
> +typedef struct sPAPRCPUUnplug {
> +    Object *cpu;
> +    QLIST_ENTRY(sPAPRCPUUnplug) node;
> +} sPAPRCPUUnplug;
> +
> +QLIST_HEAD(sPAPRCPUUnplugList, sPAPRCPUUnplug);
> +
>  #endif

-- 
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 9/9] spapr: CPU hot unplug support

[Bharata B Rao]

On Wed, Mar 16, 2016 at 04:27:04PM +1100, David Gibson wrote:
> On Fri, Mar 11, 2016 at 10:24:38AM +0530, Bharata B Rao wrote:
> > Remove the CPU core device by removing the underlying CPU thread devices.
> > Hot removal of CPU for sPAPR guests is achieved by sending the hot unplug
> > notification to the guest. Release the vCPU object after CPU hot unplug so
> > that vCPU fd can be parked and reused.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c                  | 21 ++++++++++
> >  hw/ppc/spapr_cpu_core.c         | 86 +++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h          |  1 +
> >  include/hw/ppc/spapr_cpu_core.h | 12 ++++++
> >  4 files changed, 120 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 822c87d..b1e9ba2 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2345,7 +2345,12 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >  
> >          spapr_memory_plug(hotplug_dev, dev, node, errp);
> >      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > +        /*
> > +         * TODO: Move this check to pre_plug handler at which point
> > +         * spapr_core_release() won't be necessary.
> > +         */
> >          if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > +            spapr_core_release(dev);
> >              error_setg(errp, "CPU hotplug not supported for this machine");
> >              return;
> >          }
> 
> This hunk doesn't look like its related to unplug.  Did it belong in
> another patch?

Yes, it actually belongs to hot-plug but the whole infrastructure to release
the core and associated threads get introduced in this patch, hence put
this hunk here.

However, if pre_plug is the way to go forward, we woudn't need this
altogether as roll-back is much easier from there than from here.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v2 7/9] spapr: CPU hotplug support

[Igor Mammedov]

On Wed, 16 Mar 2016 16:19:33 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Mar 11, 2016 at 10:24:36AM +0530, Bharata B Rao wrote:
> > Set up device tree entries for the hotplugged CPU core and use the
> > exising RTAS event logging infrastructure to send CPU hotplug notification
> > to the guest.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c                  | 64 ++++++++++++++++++++++++++++++++++++++
> >  hw/ppc/spapr_cpu_core.c         | 69 +++++++++++++++++++++++++++++++++++++++++
> >  hw/ppc/spapr_events.c           |  3 ++
> >  hw/ppc/spapr_rtas.c             | 24 ++++++++++++++
> >  include/hw/ppc/spapr.h          |  2 ++
> >  include/hw/ppc/spapr_cpu_core.h |  2 ++
> >  6 files changed, 164 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index cffe8c8..822c87d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -603,6 +603,18 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> >      size_t page_sizes_prop_size;
> >      uint32_t vcpus_per_socket = smp_threads * smp_cores;
> >      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> > +    sPAPRDRConnector *drc;
> > +    sPAPRDRConnectorClass *drck;
> > +    int drc_index;
> > +
> > +    if (smc->dr_cpu_enabled) {
> > +        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
> > +        g_assert(drc);  
> 
> Small nit: rather than asserting here it might be simpler to just
> check for drc != NULL instead of checking smc->dr_cpu_enabled.  That
> should have the same effect for now, and will be correct if we ever
> have some pluggable and some non-pluggable CPUs...
> 
> > +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +        drc_index = drck->get_index(drc);
> > +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
> > +    }
> >  
> >      /* Note: we keep CI large pages off for now because a 64K capable guest
> >       * provisioned with large pages might otherwise try to map a qemu
> > @@ -987,6 +999,16 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
> >          _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
> >      }
> >  
> > +    if (smc->dr_cpu_enabled) {
> > +        int offset = fdt_path_offset(fdt, "/cpus");
> > +        ret = spapr_drc_populate_dt(fdt, offset, NULL,
> > +                                    SPAPR_DR_CONNECTOR_TYPE_CPU);
> > +        if (ret < 0) {
> > +            error_report("Couldn't set up CPU DR device tree properties");
> > +            exit(1);
> > +        }
> > +    }
> > +
> >      _FDT((fdt_pack(fdt)));
> >  
> >      if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> > @@ -1622,6 +1644,8 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> >  void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > +    CPUState *cs = CPU(cpu);
> > +    int i;
> >  
> >      /* Set time-base frequency to 512 MHz */
> >      cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> > @@ -1646,6 +1670,14 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> >          }
> >      }
> >  
> > +    /* Set NUMA node for the added CPUs  */
> > +    for (i = 0; i < nb_numa_nodes; i++) {
> > +        if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) {
> > +            cs->numa_node = i;
> > +            break;
> > +        }
> > +    }
> > +
> >      xics_cpu_setup(spapr->icp, cpu);
> >  
> >      qemu_register_reset(spapr_cpu_reset, cpu);
> > @@ -1824,6 +1856,11 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >          for (i = 0; i < spapr_max_cores; i++) {
> >              int core_dt_id = i * smt;
> > +            sPAPRDRConnector *drc =
> > +                spapr_dr_connector_new(OBJECT(spapr),
> > +                                       SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
> > +
> > +            qemu_register_reset(spapr_drc_reset, drc);  
> 
> ...at least it will be if you make construction of the DRC object
> conditional on dr_cpu_enabled.
> 
> >              if (i < spapr_cores) {
> >                  Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> > @@ -2246,6 +2283,27 @@ out:
> >      error_propagate(errp, local_err);
> >  }
> >  
> > +void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > +                                    int *fdt_offset, sPAPRMachineState *spapr)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    DeviceClass *dc = DEVICE_GET_CLASS(cs);
> > +    int id = ppc_get_vcpu_dt_id(cpu);
> > +    void *fdt;
> > +    int offset, fdt_size;
> > +    char *nodename;
> > +
> > +    fdt = create_device_tree(&fdt_size);
> > +    nodename = g_strdup_printf("%s@%x", dc->fw_name, id);
> > +    offset = fdt_add_subnode(fdt, 0, nodename);
> > +
> > +    spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> > +    g_free(nodename);
> > +
> > +    *fdt_offset = offset;
> > +    return fdt;
> > +}
> > +
> >  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >                                        DeviceState *dev, Error **errp)
> >  {
> > @@ -2286,6 +2344,12 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >          }
> >  
> >          spapr_memory_plug(hotplug_dev, dev, node, errp);
> > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > +            return;
> > +        }
> > +        spapr_core_plug(hotplug_dev, dev, errp);
> >      }
> >  }
> >  
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 8c6d71d..db8de32 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -15,6 +15,75 @@
> >  #include <sysemu/cpus.h>
> >  #include "target-ppc/kvm_ppc.h"
> >  
> > +void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > +                     Error **errp)
> > +{
> > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > +    CPUState *cs = CPU(&core->threads[0]);
> > +    sPAPRDRConnector *drc;
> > +    sPAPRDRConnectorClass *drck;
> > +    Error *local_err = NULL;
> > +    void *fdt = NULL;
> > +    int fdt_offset = 0;
> > +    int core_id, core_dt_id;
> > +    int smt = kvmppc_smt_threads();
> > +
> > +    /* TODO: Should we cache core_id in sPAPRCPUCore ? */
> > +    core_dt_id = object_property_get_int(OBJECT(dev), "core",
> > &local_err);  
> 
> As Igor pointed out elsewhere you should just be able to do
> CPU_CORE(dev)->core.
not exactly like this, from what I know that's not approved style,
it should be something like this:

CPUCore *cc = CPU_CORE(dev);

  ... cc->core ...

> 
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
> > +    core_id = core_dt_id / smt;
> > +    spapr->cores[core_id] = OBJECT(dev);
> > +
> > +    if (!smc->dr_cpu_enabled) {
> > +        /*
> > +         * This is a cold plugged CPU core but the machine doesn't support
> > +         * DR. So skip the hotplug path ensuring that the core is brought
> > +         * up online with out an associated DR connector.
> > +         */
> > +        return;
> > +    }
> > +
> > +    g_assert(drc);
> > +
> > +    /*
> > +     * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> > +     * coldplugged CPUs DT entries are setup in spapr_finalize_fdt().
> > +     */
> > +    if (dev->hotplugged) {
> > +        fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, spapr);
> > +        dev->hotplugged = true;
> > +    }
> > +
> > +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +    drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> > +    if (local_err) {
> > +        g_free(fdt);
> > +        spapr->cores[core_id] = NULL;
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    if (dev->hotplugged) {
> > +        /*
> > +         * Send hotplug notification interrupt to the guest only in case
> > +         * of hotplugged CPUs.
> > +         */
> > +        spapr_hotplug_req_add_by_index(drc);
> > +    } else {
> > +        /*
> > +         * Set the right DRC states for cold plugged CPU.
> > +         */
> > +        drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> > +        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> > +    }
> > +}
> > +
> >  static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> >                                            Error **errp)
> >  {
> > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> > index 39f4682..10340e1 100644
> > --- a/hw/ppc/spapr_events.c
> > +++ b/hw/ppc/spapr_events.c
> > @@ -437,6 +437,9 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
> >      case SPAPR_DR_CONNECTOR_TYPE_LMB:
> >          hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
> >          break;
> > +    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> > +        hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_CPU;
> > +        break;
> >      default:
> >          /* we shouldn't be signaling hotplug events for resources
> >           * that don't support them
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index b7c5ebd..cc0369e 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -34,6 +34,7 @@
> >  
> >  #include "hw/ppc/spapr.h"
> >  #include "hw/ppc/spapr_vio.h"
> > +#include "hw/ppc/ppc.h"
> >  #include "qapi-event.h"
> >  #include "hw/boards.h"
> >  
> > @@ -161,6 +162,27 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
> >      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >  }
> >  
> > +/*
> > + * Set the timebase offset of the CPU to that of first CPU.
> > + * This helps hotplugged CPU to have the correct timebase offset.
> > + */
> > +static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
> > +{
> > +    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> > +
> > +    cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
> > +}
> > +
> > +static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> > +{
> > +    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> > +
> > +    if (!pcc->interrupts_big_endian(fcpu)) {
> > +        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> > +    }
> > +}
> > +
> >  static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> >                             uint32_t token, uint32_t nargs,
> >                             target_ulong args,
> > @@ -197,6 +219,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> >          env->nip = start;
> >          env->gpr[3] = r3;
> >          cs->halted = 0;
> > +        spapr_cpu_set_endianness(cpu);
> > +        spapr_cpu_update_tb_offset(cpu);
> >  
> >          qemu_cpu_kick(cs);
> >  
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index c099c3c..8957072 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -589,6 +589,8 @@ void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
> >                                            uint32_t count);
> >  void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
> >  void spapr_cpu_reset(void *opaque);
> > +void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > +                                    int *fdt_offset, sPAPRMachineState *spapr);
> >  
> >  /* rtas-configure-connector state */
> >  struct sPAPRConfigureConnectorState {
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index 48fb76a..980d8ae 100644
> > --- a/include/hw/ppc/spapr_cpu_core.h
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -25,4 +25,6 @@ typedef struct sPAPRCPUCore {
> >      PowerPCCPU *threads;
> >  } sPAPRCPUCore;
> >  
> > +void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > +                     Error **errp);
> >  #endif  
> 

Re: [Qemu-devel] [RFC PATCH v2 0/9] Core based CPU hotplug for PowerPC sPAPR

[Igor Mammedov]

On Wed, 16 Mar 2016 09:18:03 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, Mar 14, 2016 at 10:47:28AM +0100, Igor Mammedov wrote:
> > On Fri, 11 Mar 2016 10:24:29 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > Hi,
> > > 
> > > This is the next version of "Core based CPU hotplug for PowerPC sPAPR" that
> > > was posted at
> > > https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html
> > > 
> > > device_add semantics
> > > --------------------
> > > For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
> > > (qemu) device_add spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]  
> > do you plan to allow user to hotplug different cpu_models?
> > If not it would be better to hide cpu_model from user
> > and set it from machine pre_plug handler.  
> 
> In my earlier implementations I derived cpu model from -cpu and threads from
> -smp,threads= commandline options and never exposed them to device_add
> command.
> 
> Though we don't support heterogenous systems (different cpu models and/or
> threads) now, it was felt that it should be easy enough to support such
> systems if required in future, that's how cpu_model and threads became
> options for device_add.
> 
> One of the things that David felt was missing from my earlier QMP query
> command (and which is true in your QMP query implementation also) is that
> we aren't exporting cpu_model at all, at least for not-yet-plugged cores.
> So should we include that or let management figure that out since it
> would already know about the CPU model.
1.
so since you are not planning supporting heterogeneous setup yet,
I'd suggest to refrain from making user to provide cpu_model at
device_add time. Instead make machine code to set it for cores it
creates before core.realize() (yet another use for pre_plug()).

That way mgmt doesn't have to figure out what cpu_model to set at
device_add time and doesn't have find out what property to use for it.

2.
If you still insist on providing cpu_model property at
device_add time, you'd better extend QMP command query-hotpluggable-cpus
to provide it in 'props' list along with valid value.

But I'd go with #1 as questions of using cpu_model-s vs QOM types and
discovery of mapping of cpu-model to QOM types is not clear yet
and need to be discussed further.


> 
> Regards,
> Bharata.
> 
> 

Re: [Qemu-devel] [RFC PATCH v2 0/9] Core based CPU hotplug for PowerPC sPAPR

[David Gibson]

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

On Wed, Mar 16, 2016 at 04:48:50PM +0100, Igor Mammedov wrote:
> On Wed, 16 Mar 2016 09:18:03 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Mon, Mar 14, 2016 at 10:47:28AM +0100, Igor Mammedov wrote:
> > > On Fri, 11 Mar 2016 10:24:29 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >   
> > > > Hi,
> > > > 
> > > > This is the next version of "Core based CPU hotplug for PowerPC sPAPR" that
> > > > was posted at
> > > > https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html
> > > > 
> > > > device_add semantics
> > > > --------------------
> > > > For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
> > > > (qemu) device_add spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]  
> > > do you plan to allow user to hotplug different cpu_models?
> > > If not it would be better to hide cpu_model from user
> > > and set it from machine pre_plug handler.  
> > 
> > In my earlier implementations I derived cpu model from -cpu and threads from
> > -smp,threads= commandline options and never exposed them to device_add
> > command.
> > 
> > Though we don't support heterogenous systems (different cpu models and/or
> > threads) now, it was felt that it should be easy enough to support such
> > systems if required in future, that's how cpu_model and threads became
> > options for device_add.
> > 
> > One of the things that David felt was missing from my earlier QMP query
> > command (and which is true in your QMP query implementation also) is that
> > we aren't exporting cpu_model at all, at least for not-yet-plugged cores.
> > So should we include that or let management figure that out since it
> > would already know about the CPU model.
> 1.
> so since you are not planning supporting heterogeneous setup yet,
> I'd suggest to refrain from making user to provide cpu_model at
> device_add time. Instead make machine code to set it for cores it
> creates before core.realize() (yet another use for pre_plug()).
> 
> That way mgmt doesn't have to figure out what cpu_model to set at
> device_add time and doesn't have find out what property to use for it.

Yes.. of course you could also do the same thing for nr_threads, so
I'm wondering whether there's a good argument to keep one in
pre_plug() and one in query-hotpluggable-cpus.

> 2.
> If you still insist on providing cpu_model property at
> device_add time, you'd better extend QMP command query-hotpluggable-cpus
> to provide it in 'props' list along with valid value.

Yes, that's definitely true.

> But I'd go with #1 as questions of using cpu_model-s vs QOM types and
> discovery of mapping of cpu-model to QOM types is not clear yet
> and need to be discussed further.

Hm, I suppose so.

-- 
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/9] Core based CPU hotplug for PowerPC sPAPR

[Bharata B Rao]

On Thu, Mar 17, 2016 at 09:03:43PM +1100, David Gibson wrote:
> On Wed, Mar 16, 2016 at 04:48:50PM +0100, Igor Mammedov wrote:
> > On Wed, 16 Mar 2016 09:18:03 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > On Mon, Mar 14, 2016 at 10:47:28AM +0100, Igor Mammedov wrote:
> > > > On Fri, 11 Mar 2016 10:24:29 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > >   
> > > > > Hi,
> > > > > 
> > > > > This is the next version of "Core based CPU hotplug for PowerPC sPAPR" that
> > > > > was posted at
> > > > > https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html
> > > > > 
> > > > > device_add semantics
> > > > > --------------------
> > > > > For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
> > > > > (qemu) device_add spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]  
> > > > do you plan to allow user to hotplug different cpu_models?
> > > > If not it would be better to hide cpu_model from user
> > > > and set it from machine pre_plug handler.  
> > > 
> > > In my earlier implementations I derived cpu model from -cpu and threads from
> > > -smp,threads= commandline options and never exposed them to device_add
> > > command.
> > > 
> > > Though we don't support heterogenous systems (different cpu models and/or
> > > threads) now, it was felt that it should be easy enough to support such
> > > systems if required in future, that's how cpu_model and threads became
> > > options for device_add.
> > > 
> > > One of the things that David felt was missing from my earlier QMP query
> > > command (and which is true in your QMP query implementation also) is that
> > > we aren't exporting cpu_model at all, at least for not-yet-plugged cores.
> > > So should we include that or let management figure that out since it
> > > would already know about the CPU model.
> > 1.
> > so since you are not planning supporting heterogeneous setup yet,
> > I'd suggest to refrain from making user to provide cpu_model at
> > device_add time. Instead make machine code to set it for cores it
> > creates before core.realize() (yet another use for pre_plug()).
> > 
> > That way mgmt doesn't have to figure out what cpu_model to set at
> > device_add time and doesn't have find out what property to use for it.
> 
> Yes.. of course you could also do the same thing for nr_threads, so
> I'm wondering whether there's a good argument to keep one in
> pre_plug() and one in query-hotpluggable-cpus.

Right, so what should be the way forward ? Should we keep cpu_model= and
threads= options with device_add or just threads=  or neither ?

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v2 0/9] Core based CPU hotplug for PowerPC sPAPR

[David Gibson]

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

On Fri, Mar 18, 2016 at 08:59:32AM +0530, Bharata B Rao wrote:
> On Thu, Mar 17, 2016 at 09:03:43PM +1100, David Gibson wrote:
> > On Wed, Mar 16, 2016 at 04:48:50PM +0100, Igor Mammedov wrote:
> > > On Wed, 16 Mar 2016 09:18:03 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > 
> > > > On Mon, Mar 14, 2016 at 10:47:28AM +0100, Igor Mammedov wrote:
> > > > > On Fri, 11 Mar 2016 10:24:29 +0530
> > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > >   
> > > > > > Hi,
> > > > > > 
> > > > > > This is the next version of "Core based CPU hotplug for PowerPC sPAPR" that
> > > > > > was posted at
> > > > > > https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html
> > > > > > 
> > > > > > device_add semantics
> > > > > > --------------------
> > > > > > For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
> > > > > > (qemu) device_add spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]  
> > > > > do you plan to allow user to hotplug different cpu_models?
> > > > > If not it would be better to hide cpu_model from user
> > > > > and set it from machine pre_plug handler.  
> > > > 
> > > > In my earlier implementations I derived cpu model from -cpu and threads from
> > > > -smp,threads= commandline options and never exposed them to device_add
> > > > command.
> > > > 
> > > > Though we don't support heterogenous systems (different cpu models and/or
> > > > threads) now, it was felt that it should be easy enough to support such
> > > > systems if required in future, that's how cpu_model and threads became
> > > > options for device_add.
> > > > 
> > > > One of the things that David felt was missing from my earlier QMP query
> > > > command (and which is true in your QMP query implementation also) is that
> > > > we aren't exporting cpu_model at all, at least for not-yet-plugged cores.
> > > > So should we include that or let management figure that out since it
> > > > would already know about the CPU model.
> > > 1.
> > > so since you are not planning supporting heterogeneous setup yet,
> > > I'd suggest to refrain from making user to provide cpu_model at
> > > device_add time. Instead make machine code to set it for cores it
> > > creates before core.realize() (yet another use for pre_plug()).
> > > 
> > > That way mgmt doesn't have to figure out what cpu_model to set at
> > > device_add time and doesn't have find out what property to use for it.
> > 
> > Yes.. of course you could also do the same thing for nr_threads, so
> > I'm wondering whether there's a good argument to keep one in
> > pre_plug() and one in query-hotpluggable-cpus.
> 
> Right, so what should be the way forward ? Should we keep cpu_model= and
> threads= options with device_add or just threads=  or neither ?

I'm inclined to keep them both in device_add - I like the idea of
having an example on day 0 of advertising extra properties (beyond
nr_threads and location) to set from query-hotpluggable-cpus.

But, I'd probably change my mind if Igor or someone has a stronger
opinion.

If we advertise cpu_model, however, it should probably be changed to
cpu thread class name, since IIUC that's an existing advertised part
of the QOM interface, but cpu_model isn't.

-- 
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/9] Core based CPU hotplug for PowerPC sPAPR

[Igor Mammedov]

On Mon, 21 Mar 2016 14:57:53 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Mar 18, 2016 at 08:59:32AM +0530, Bharata B Rao wrote:
> > On Thu, Mar 17, 2016 at 09:03:43PM +1100, David Gibson wrote:  
> > > On Wed, Mar 16, 2016 at 04:48:50PM +0100, Igor Mammedov wrote:  
> > > > On Wed, 16 Mar 2016 09:18:03 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > >   
> > > > > On Mon, Mar 14, 2016 at 10:47:28AM +0100, Igor Mammedov wrote:  
> > > > > > On Fri, 11 Mar 2016 10:24:29 +0530
> > > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > > >     
> > > > > > > Hi,
> > > > > > > 
> > > > > > > This is the next version of "Core based CPU hotplug for PowerPC sPAPR" that
> > > > > > > was posted at
> > > > > > > https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html
> > > > > > > 
> > > > > > > device_add semantics
> > > > > > > --------------------
> > > > > > > For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
> > > > > > > (qemu) device_add spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]    
> > > > > > do you plan to allow user to hotplug different cpu_models?
> > > > > > If not it would be better to hide cpu_model from user
> > > > > > and set it from machine pre_plug handler.    
> > > > > 
> > > > > In my earlier implementations I derived cpu model from -cpu and threads from
> > > > > -smp,threads= commandline options and never exposed them to device_add
> > > > > command.
> > > > > 
> > > > > Though we don't support heterogenous systems (different cpu models and/or
> > > > > threads) now, it was felt that it should be easy enough to support such
> > > > > systems if required in future, that's how cpu_model and threads became
> > > > > options for device_add.
> > > > > 
> > > > > One of the things that David felt was missing from my earlier QMP query
> > > > > command (and which is true in your QMP query implementation also) is that
> > > > > we aren't exporting cpu_model at all, at least for not-yet-plugged cores.
> > > > > So should we include that or let management figure that out since it
> > > > > would already know about the CPU model.  
> > > > 1.
> > > > so since you are not planning supporting heterogeneous setup yet,
> > > > I'd suggest to refrain from making user to provide cpu_model at
> > > > device_add time. Instead make machine code to set it for cores it
> > > > creates before core.realize() (yet another use for pre_plug()).
> > > > 
> > > > That way mgmt doesn't have to figure out what cpu_model to set at
> > > > device_add time and doesn't have find out what property to use for it.  
> > > 
> > > Yes.. of course you could also do the same thing for nr_threads, so
> > > I'm wondering whether there's a good argument to keep one in
> > > pre_plug() and one in query-hotpluggable-cpus.  
> > 
> > Right, so what should be the way forward ? Should we keep cpu_model= and
> > threads= options with device_add or just threads=  or neither ?  
> 
> I'm inclined to keep them both in device_add - I like the idea of
> having an example on day 0 of advertising extra properties (beyond
> nr_threads and location) to set from query-hotpluggable-cpus.
> 
> But, I'd probably change my mind if Igor or someone has a stronger
> opinion.
I don't have a strong opinion on this, but you have to keep in mind
that one you make it ABI you probably would have to maintain it forever.

So far 'threads' and 'cpu_model' look like a constant values,
fixed at start-up time for every core.
Taking in account that user is not supposed to change them during
hotplug time and that they are the same for every core,
I'd go for conservative route and hide them in pre_plug() for now.
You always can expose them later if needed.

> If we advertise cpu_model, however, it should probably be changed to
> cpu thread class name, since IIUC that's an existing advertised part
> of the QOM interface, but cpu_model isn't.
I still think that spapr-cpu-core should be an abstract type
with a concrete set of derived cores types per each thread type.
But this question is not related to hotplug, but rather to
start-up of QEMU from scratch with -device and supported types
discovery. So I'd postpone question for later and that's yet another
reason why I'd like to hide cpu_model from user for now.

Re: [Qemu-devel] [RFC PATCH v2 0/9] Core based CPU hotplug for PowerPC sPAPR

[David Gibson]

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

On Mon, Mar 21, 2016 at 11:43:34AM +0100, Igor Mammedov wrote:
> On Mon, 21 Mar 2016 14:57:53 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Mar 18, 2016 at 08:59:32AM +0530, Bharata B Rao wrote:
> > > On Thu, Mar 17, 2016 at 09:03:43PM +1100, David Gibson wrote:  
> > > > On Wed, Mar 16, 2016 at 04:48:50PM +0100, Igor Mammedov wrote:  
> > > > > On Wed, 16 Mar 2016 09:18:03 +0530
> > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > >   
> > > > > > On Mon, Mar 14, 2016 at 10:47:28AM +0100, Igor Mammedov wrote:  
> > > > > > > On Fri, 11 Mar 2016 10:24:29 +0530
> > > > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > > > >     
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > This is the next version of "Core based CPU hotplug for PowerPC sPAPR" that
> > > > > > > > was posted at
> > > > > > > > https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html
> > > > > > > > 
> > > > > > > > device_add semantics
> > > > > > > > --------------------
> > > > > > > > For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
> > > > > > > > (qemu) device_add spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]    
> > > > > > > do you plan to allow user to hotplug different cpu_models?
> > > > > > > If not it would be better to hide cpu_model from user
> > > > > > > and set it from machine pre_plug handler.    
> > > > > > 
> > > > > > In my earlier implementations I derived cpu model from -cpu and threads from
> > > > > > -smp,threads= commandline options and never exposed them to device_add
> > > > > > command.
> > > > > > 
> > > > > > Though we don't support heterogenous systems (different cpu models and/or
> > > > > > threads) now, it was felt that it should be easy enough to support such
> > > > > > systems if required in future, that's how cpu_model and threads became
> > > > > > options for device_add.
> > > > > > 
> > > > > > One of the things that David felt was missing from my earlier QMP query
> > > > > > command (and which is true in your QMP query implementation also) is that
> > > > > > we aren't exporting cpu_model at all, at least for not-yet-plugged cores.
> > > > > > So should we include that or let management figure that out since it
> > > > > > would already know about the CPU model.  
> > > > > 1.
> > > > > so since you are not planning supporting heterogeneous setup yet,
> > > > > I'd suggest to refrain from making user to provide cpu_model at
> > > > > device_add time. Instead make machine code to set it for cores it
> > > > > creates before core.realize() (yet another use for pre_plug()).
> > > > > 
> > > > > That way mgmt doesn't have to figure out what cpu_model to set at
> > > > > device_add time and doesn't have find out what property to use for it.  
> > > > 
> > > > Yes.. of course you could also do the same thing for nr_threads, so
> > > > I'm wondering whether there's a good argument to keep one in
> > > > pre_plug() and one in query-hotpluggable-cpus.  
> > > 
> > > Right, so what should be the way forward ? Should we keep cpu_model= and
> > > threads= options with device_add or just threads=  or neither ?  
> > 
> > I'm inclined to keep them both in device_add - I like the idea of
> > having an example on day 0 of advertising extra properties (beyond
> > nr_threads and location) to set from query-hotpluggable-cpus.
> > 
> > But, I'd probably change my mind if Igor or someone has a stronger
> > opinion.
> I don't have a strong opinion on this, but you have to keep in mind
> that one you make it ABI you probably would have to maintain it forever.
> 
> So far 'threads' and 'cpu_model' look like a constant values,
> fixed at start-up time for every core.
> Taking in account that user is not supposed to change them during
> hotplug time and that they are the same for every core,
> I'd go for conservative route and hide them in pre_plug() for now.
> You always can expose them later if needed.

Hm, yes, I see your point.  Hiding them in pre-plug does also make
life more convenient for someone (not libvirt) manually experimenting
with this - less to type on their device-add command.

> > If we advertise cpu_model, however, it should probably be changed to
> > cpu thread class name, since IIUC that's an existing advertised part
> > of the QOM interface, but cpu_model isn't.
> I still think that spapr-cpu-core should be an abstract type
> with a concrete set of derived cores types per each thread type.
> But this question is not related to hotplug, but rather to
> start-up of QEMU from scratch with -device and supported types
> discovery. So I'd postpone question for later and that's yet another
> reason why I'd like to hide cpu_model from user for now.

Ah.. yes, I think you're probably right, and we should have derived
types.  It's a little bit awkward for spapr, but we can probably do
some macro magic to make it not too bad.

-- 
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/9] Core based CPU hotplug for PowerPC sPAPR

[Igor Mammedov]

On Tue, 22 Mar 2016 11:22:56 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Mar 21, 2016 at 11:43:34AM +0100, Igor Mammedov wrote:
> > On Mon, 21 Mar 2016 14:57:53 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Mar 18, 2016 at 08:59:32AM +0530, Bharata B Rao wrote:  
> > > > On Thu, Mar 17, 2016 at 09:03:43PM +1100, David Gibson wrote:    
> > > > > On Wed, Mar 16, 2016 at 04:48:50PM +0100, Igor Mammedov wrote:    
> > > > > > On Wed, 16 Mar 2016 09:18:03 +0530
> > > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > > >     
> > > > > > > On Mon, Mar 14, 2016 at 10:47:28AM +0100, Igor Mammedov wrote:    
> > > > > > > > On Fri, 11 Mar 2016 10:24:29 +0530
> > > > > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > > > > >       
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > This is the next version of "Core based CPU hotplug for PowerPC sPAPR" that
> > > > > > > > > was posted at
> > > > > > > > > https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html
> > > > > > > > > 
> > > > > > > > > device_add semantics
> > > > > > > > > --------------------
> > > > > > > > > For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
> > > > > > > > > (qemu) device_add spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]      
> > > > > > > > do you plan to allow user to hotplug different cpu_models?
> > > > > > > > If not it would be better to hide cpu_model from user
> > > > > > > > and set it from machine pre_plug handler.      
> > > > > > > 
> > > > > > > In my earlier implementations I derived cpu model from -cpu and threads from
> > > > > > > -smp,threads= commandline options and never exposed them to device_add
> > > > > > > command.
> > > > > > > 
> > > > > > > Though we don't support heterogenous systems (different cpu models and/or
> > > > > > > threads) now, it was felt that it should be easy enough to support such
> > > > > > > systems if required in future, that's how cpu_model and threads became
> > > > > > > options for device_add.
> > > > > > > 
> > > > > > > One of the things that David felt was missing from my earlier QMP query
> > > > > > > command (and which is true in your QMP query implementation also) is that
> > > > > > > we aren't exporting cpu_model at all, at least for not-yet-plugged cores.
> > > > > > > So should we include that or let management figure that out since it
> > > > > > > would already know about the CPU model.    
> > > > > > 1.
> > > > > > so since you are not planning supporting heterogeneous setup yet,
> > > > > > I'd suggest to refrain from making user to provide cpu_model at
> > > > > > device_add time. Instead make machine code to set it for cores it
> > > > > > creates before core.realize() (yet another use for pre_plug()).
> > > > > > 
> > > > > > That way mgmt doesn't have to figure out what cpu_model to set at
> > > > > > device_add time and doesn't have find out what property to use for it.    
> > > > > 
> > > > > Yes.. of course you could also do the same thing for nr_threads, so
> > > > > I'm wondering whether there's a good argument to keep one in
> > > > > pre_plug() and one in query-hotpluggable-cpus.    
> > > > 
> > > > Right, so what should be the way forward ? Should we keep cpu_model= and
> > > > threads= options with device_add or just threads=  or neither ?    
> > > 
> > > I'm inclined to keep them both in device_add - I like the idea of
> > > having an example on day 0 of advertising extra properties (beyond
> > > nr_threads and location) to set from query-hotpluggable-cpus.
> > > 
> > > But, I'd probably change my mind if Igor or someone has a stronger
> > > opinion.  
> > I don't have a strong opinion on this, but you have to keep in mind
> > that one you make it ABI you probably would have to maintain it forever.
> > 
> > So far 'threads' and 'cpu_model' look like a constant values,
> > fixed at start-up time for every core.
> > Taking in account that user is not supposed to change them during
> > hotplug time and that they are the same for every core,
> > I'd go for conservative route and hide them in pre_plug() for now.
> > You always can expose them later if needed.  
> 
> Hm, yes, I see your point.  Hiding them in pre-plug does also make
> life more convenient for someone (not libvirt) manually experimenting
> with this - less to type on their device-add command.
> 
> > > If we advertise cpu_model, however, it should probably be changed to
> > > cpu thread class name, since IIUC that's an existing advertised part
> > > of the QOM interface, but cpu_model isn't.  
> > I still think that spapr-cpu-core should be an abstract type
> > with a concrete set of derived cores types per each thread type.
> > But this question is not related to hotplug, but rather to
> > start-up of QEMU from scratch with -device and supported types
> > discovery. So I'd postpone question for later and that's yet another
> > reason why I'd like to hide cpu_model from user for now.  
> 
> Ah.. yes, I think you're probably right, and we should have derived
> types.  It's a little bit awkward for spapr, but we can probably do
> some macro magic to make it not too bad.
Just to note derived types are not must have for hotplug but
if they are introduced later (i.e. not in the same QEMU release),
we would be forced to keep spapr-core-cpu as non abstract class
for compat reasons wrt migration.