[Qemu-devel] [for-2.7 PATCH v3 00/15] Core based CPU hotplug for PowerPC sPAPR

[Qemu-devel] [for-2.7 PATCH v3 00/15] Core based CPU hotplug for PowerPC sPAPR

[Bharata B Rao]

Hi,

This is v3 of "Core based CPU hotplug for PowerPC sPAPR". The hotplug
semantics looks like this:

(qemu) device_add POWER8E-spapr-cpu-core,id=core2,core=16[,threads=4]
(qemu) device_add POWER8E_v2.1-spapr-cpu-core,id=core2,core=16[,threads=4]

Changes in v3
-------------
- Moved CPU ObjectClass pointer from sPAPR specific CPU core type to
  its parent type, the abstract sPAPR CPU core type. This largely reduces
  the use of macros.
- Including Igor's QMP query-hotpluggable-cpus patches in this series.
- Added HMP version for query-hotpluggable-cpus.
- Added a patch to prevent QEMU crash due to DRC detach racing against attach.
- Addressed miscellaneous review comments from previous post.

v2.1: https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00649.html

Bharata B Rao (11):
  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: Abstract CPU core device and type specific core devices
  spapr: convert boot CPUs into CPU core devices
  spapr: CPU hotplug support
  xics,xics_kvm: Handle CPU unplug correctly
  spapr_drc: Prevent detach racing against attach for CPU DR
  spapr: CPU hot unplug support
  hmp: Add 'info hotpluggable-cpus' HMP command

Gu Zheng (1):
  cpu: Reclaim vCPU objects

Igor Mammedov (3):
  qdev: hotplug: Introduce HotplugHandler.pre_plug() callback
  QMP: Add query-hotpluggable-cpus
  spapr: implement query-hotpluggable-cpus callback

 cpus.c                          |  51 +++++-
 exec.c                          |  41 ++++-
 hmp-commands-info.hx            |  14 ++
 hmp.c                           |  41 +++++
 hmp.h                           |   1 +
 hw/core/hotplug.c               |  11 ++
 hw/core/qdev.c                  |   9 +-
 hw/cpu/Makefile.objs            |   1 +
 hw/cpu/core.c                   |  88 ++++++++++
 hw/intc/xics.c                  |  14 ++
 hw/intc/xics_kvm.c              |   8 +-
 hw/ppc/Makefile.objs            |   1 +
 hw/ppc/spapr.c                  | 190 ++++++++++++++++++--
 hw/ppc/spapr_cpu_core.c         | 376 ++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_drc.c              |  12 ++
 hw/ppc/spapr_events.c           |   3 +
 hw/ppc/spapr_rtas.c             |  24 +++
 include/hw/boards.h             |   5 +
 include/hw/cpu/core.h           |  31 ++++
 include/hw/hotplug.h            |  14 +-
 include/hw/ppc/spapr.h          |   6 +
 include/hw/ppc/spapr_cpu_core.h |  44 +++++
 include/hw/ppc/spapr_drc.h      |   1 +
 include/hw/ppc/xics.h           |   1 +
 include/qom/cpu.h               |  18 ++
 include/sysemu/kvm.h            |   1 +
 kvm-all.c                       |  57 +++++-
 kvm-stub.c                      |   5 +
 monitor.c                       |  13 ++
 qapi-schema.json                |  55 ++++++
 qmp-commands.hx                 |  23 +++
 31 files changed, 1130 insertions(+), 29 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] [for-2.7 PATCH v3 01/15] 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 exec.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index c4f9036..da1f09a 100644
--- a/exec.c
+++ b/exec.c
@@ -610,15 +610,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
 
@@ -633,11 +627,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

Re: [Qemu-devel] [for-2.7 PATCH v3 01/15] exec: Remove cpu from cpus list during cpu_exec_exit()

[Paolo Bonzini]

On 12/05/2016 05:48, 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.

I think the better thing would be to call it from CPU::unrealize, but
this patch is okay too.

Thanks,

Paolo

> 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>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [for-2.7 PATCH v3 01/15] exec: Remove cpu from cpus list during cpu_exec_exit()

[David Gibson]

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

On Thu, May 26, 2016 at 12:12:41PM +0200, Paolo Bonzini wrote:
> 
> 
> On 12/05/2016 05:48, 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.
> 
> I think the better thing would be to call it from CPU::unrealize, but
> this patch is okay too.
> 
> Thanks,
> 
> Paolo

Thanks for the review Paolo.

However, what I'm really unclear on is what is the next step towards
merging these.  Will you take them through your tree?  Should Bharata
send a formal pull request with the prelim patches?  If so, to whom?

> > 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>
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

-- 
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] [for-2.7 PATCH v3 01/15] exec: Remove cpu from cpus list during cpu_exec_exit()

[Paolo Bonzini]

On 27/05/2016 05:07, David Gibson wrote:
> On Thu, May 26, 2016 at 12:12:41PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 12/05/2016 05:48, 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.
>>
>> I think the better thing would be to call it from CPU::unrealize, but
>> this patch is okay too.
>>
>> Thanks,
>>
>> Paolo
> 
> Thanks for the review Paolo.
> 
> However, what I'm really unclear on is what is the next step towards
> merging these.  Will you take them through your tree?  Should Bharata
> send a formal pull request with the prelim patches?  If so, to whom?

Feel free to take them and add an Acked-by for me.  The fewer patches I
merge, the better. :)

Thanks,

Paolo

[Qemu-devel] [for-2.7 PATCH v3 02/15] 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>
Reviewed-by: Thomas Huth <thuth@redhat.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 da1f09a..32bd5df 100644
--- a/exec.c
+++ b/exec.c
@@ -635,6 +635,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
@@ -652,6 +654,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

Re: [Qemu-devel] [for-2.7 PATCH v3 02/15] exec: Do vmstate unregistration from cpu_exec_exit()

[Paolo Bonzini]

On 12/05/2016 05:48, 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: Thomas Huth <thuth@redhat.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 da1f09a..32bd5df 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -635,6 +635,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
> @@ -652,6 +654,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)
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH] fixup! exec: Do vmstate unregistration from cpu_exec_exit()

[Igor Mammedov]

with all header changes merged in current master (d6550e9ed2)
above patch breaks compilation with:
  exec.c: In function ‘cpu_exec_exit’:
  exec.c:661:9: error: implicit declaration of function ‘vmstate_unregister’ [-Werror=implicit-function-declaration]
         vmstate_unregister(NULL, cc->vmsd, cpu);

pls squash this fixup in orginal patch

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 exec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/exec.c b/exec.c
index c5dd58e..a0327a9 100644
--- a/exec.c
+++ b/exec.c
@@ -62,6 +62,8 @@
 #include "qemu/mmap-alloc.h"
 #endif
 
+#include "migration/vmstate.h"
+
 //#define DEBUG_SUBPAGE
 
 #if !defined(CONFIG_USER_ONLY)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] fixup! exec: Do vmstate unregistration from cpu_exec_exit()

[David Gibson]

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

On Mon, May 30, 2016 at 05:22:29PM +0200, Igor Mammedov wrote:
> with all header changes merged in current master (d6550e9ed2)
> above patch breaks compilation with:
>   exec.c: In function ‘cpu_exec_exit’:
>   exec.c:661:9: error: implicit declaration of function ‘vmstate_unregister’ [-Werror=implicit-function-declaration]
>          vmstate_unregister(NULL, cc->vmsd, cpu);
> 
> pls squash this fixup in orginal patch
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Already made the same change when I pulled it into my tree.

> ---
>  exec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index c5dd58e..a0327a9 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -62,6 +62,8 @@
>  #include "qemu/mmap-alloc.h"
>  #endif
>  
> +#include "migration/vmstate.h"
> +
>  //#define DEBUG_SUBPAGE
>  
>  #if !defined(CONFIG_USER_ONLY)

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

[Qemu-devel] [for-2.7 PATCH v3 03/15] 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)]
Reviewed-by: Thomas Huth <thuth@redhat.com>
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 cbeb1f6..3e7eada 100644
--- a/cpus.c
+++ b/cpus.c
@@ -970,6 +970,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;
@@ -1059,7 +1071,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) {
@@ -1067,8 +1079,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;
 }
 
@@ -1122,6 +1136,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();
 
@@ -1159,6 +1174,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;
@@ -1315,6 +1340,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
 
@@ -1531,6 +1563,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 b7a10f7..911bc9f 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -235,6 +235,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.
@@ -287,6 +288,7 @@ struct CPUState {
     bool created;
     bool stop;
     bool stopped;
+    bool unplug;
     bool crash_occurred;
     bool exit_request;
     uint32_t interrupt_request;
@@ -754,6 +756,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 0e18f15..4706c37 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 e7b66df..09f1211 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

Re: [Qemu-devel] [for-2.7 PATCH v3 03/15] cpu: Reclaim vCPU objects

[Paolo Bonzini]

On 12/05/2016 05:48, Bharata B Rao wrote:
> @@ -1531,6 +1563,9 @@ static void tcg_exec_all(void)
>                  break;
>              }
>          } else if (cpu->stop || cpu->stopped) {
> +            if (cpu->unplug) {

This "if" is unnecessary.  next_cpu is not used anymore after the
"break", so the store is dead.

Ok without this.

Paolo

> +                next_cpu = CPU_NEXT(cpu);
> +            }
>              break;
>          }
>      }

Re: [Qemu-devel] [for-2.7 PATCH v3 03/15] cpu: Reclaim vCPU objects

[Bharata B Rao]

On Thu, May 26, 2016 at 12:19:05PM +0200, Paolo Bonzini wrote:
> 
> 
> On 12/05/2016 05:48, Bharata B Rao wrote:
> > @@ -1531,6 +1563,9 @@ static void tcg_exec_all(void)
> >                  break;
> >              }
> >          } else if (cpu->stop || cpu->stopped) {
> > +            if (cpu->unplug) {
> 
> This "if" is unnecessary.  next_cpu is not used anymore after the
> "break", so the store is dead.

next_cpu is a global here. Won't tcg_exec_all() continue with running the
next_cpu when it is called the next time ?

Regards,
Bharata.

Re: [Qemu-devel] [for-2.7 PATCH v3 03/15] cpu: Reclaim vCPU objects

[Paolo Bonzini]

On 26/05/2016 12:47, Bharata B Rao wrote:
>>> > >          } else if (cpu->stop || cpu->stopped) {
>>> > > +            if (cpu->unplug) {
>> > 
>> > This "if" is unnecessary.  next_cpu is not used anymore after the
>> > "break", so the store is dead.
> next_cpu is a global here. Won't tcg_exec_all() continue with running the
> next_cpu when it is called the next time ?

You're right, sorry!

Thanks,

Paolo

[Qemu-devel] [for-2.7 PATCH v3 04/15] 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 3e7eada..0a6ae44 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1082,6 +1082,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;
 }
@@ -1182,6 +1184,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;
         }
     }
@@ -1347,6 +1351,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 911bc9f..9c544f6 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -763,6 +763,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

Re: [Qemu-devel] [for-2.7 PATCH v3 04/15] cpu: Add a sync version of cpu_remove()

[Paolo Bonzini]

On 12/05/2016 05:48, Bharata B Rao wrote:
> 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 3e7eada..0a6ae44 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1082,6 +1082,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;
>  }
> @@ -1182,6 +1184,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;
>          }
>      }
> @@ -1347,6 +1351,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 911bc9f..9c544f6 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -763,6 +763,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.
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [for-2.7 PATCH v3 05/15] qdev: hotplug: Introduce HotplugHandler.pre_plug() callback

[Bharata B Rao]

From: Igor Mammedov <imammedo@redhat.com>

pre_plug callback is to be called before device.realize() is executed.
This would allow to check/set device's properties from HotplugHandler.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/core/hotplug.c    | 11 +++++++++++
 hw/core/qdev.c       |  9 ++++++++-
 include/hw/hotplug.h | 14 +++++++++++++-
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 645cfca..17ac986 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -13,6 +13,17 @@
 #include "hw/hotplug.h"
 #include "qemu/module.h"
 
+void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
+                              DeviceState *plugged_dev,
+                              Error **errp)
+{
+    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
+
+    if (hdc->pre_plug) {
+        hdc->pre_plug(plug_handler, plugged_dev, errp);
+    }
+}
+
 void hotplug_handler_plug(HotplugHandler *plug_handler,
                           DeviceState *plugged_dev,
                           Error **errp)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index db41aa1..a0b3aad 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1062,6 +1062,14 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             g_free(name);
         }
 
+        hotplug_ctrl = qdev_get_hotplug_handler(dev);
+        if (hotplug_ctrl) {
+            hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err);
+            if (local_err != NULL) {
+                goto fail;
+            }
+        }
+
         if (dc->realize) {
             dc->realize(dev, &local_err);
         }
@@ -1072,7 +1080,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
 
         DEVICE_LISTENER_CALL(realize, Forward, dev);
 
-        hotplug_ctrl = qdev_get_hotplug_handler(dev);
         if (hotplug_ctrl) {
             hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
         }
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index da1d0e4..c0db869 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -45,7 +45,8 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
  * hardware (un)plug functions.
  *
  * @parent: Opaque parent interface.
- * @plug: plug callback.
+ * @pre_plug: pre plug callback called at start of device.realize(true)
+ * @plug: plug callback called at end of device.realize(true).
  * @unplug_request: unplug request callback.
  *                  Used as a means to initiate device unplug for devices that
  *                  require asynchronous unplug handling.
@@ -58,6 +59,7 @@ typedef struct HotplugHandlerClass {
     InterfaceClass parent;
 
     /* <public> */
+    hotplug_fn pre_plug;
     hotplug_fn plug;
     hotplug_fn unplug_request;
     hotplug_fn unplug;
@@ -73,6 +75,16 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
                           Error **errp);
 
 /**
+ * hotplug_handler_pre_plug:
+ *
+ * Call #HotplugHandlerClass.pre_plug callback of @plug_handler.
+ */
+void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
+                              DeviceState *plugged_dev,
+                              Error **errp);
+
+
+/**
  * hotplug_handler_unplug_request:
  *
  * Calls #HotplugHandlerClass.unplug_request callback of @plug_handler.
-- 
2.1.0

Re: [Qemu-devel] [for-2.7 PATCH v3 05/15] qdev: hotplug: Introduce HotplugHandler.pre_plug() callback

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:15AM +0530, Bharata B Rao wrote:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> pre_plug callback is to be called before device.realize() is executed.
> This would allow to check/set device's properties from HotplugHandler.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Igor,  do you think we're ready to merge this?

If so, do you want to take it through your tree, or should I take it
through the ppc tree?

> ---
>  hw/core/hotplug.c    | 11 +++++++++++
>  hw/core/qdev.c       |  9 ++++++++-
>  include/hw/hotplug.h | 14 +++++++++++++-
>  3 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 645cfca..17ac986 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -13,6 +13,17 @@
>  #include "hw/hotplug.h"
>  #include "qemu/module.h"
>  
> +void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
> +                              DeviceState *plugged_dev,
> +                              Error **errp)
> +{
> +    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> +
> +    if (hdc->pre_plug) {
> +        hdc->pre_plug(plug_handler, plugged_dev, errp);
> +    }
> +}
> +
>  void hotplug_handler_plug(HotplugHandler *plug_handler,
>                            DeviceState *plugged_dev,
>                            Error **errp)
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index db41aa1..a0b3aad 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1062,6 +1062,14 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              g_free(name);
>          }
>  
> +        hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +        if (hotplug_ctrl) {
> +            hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err);
> +            if (local_err != NULL) {
> +                goto fail;
> +            }
> +        }
> +
>          if (dc->realize) {
>              dc->realize(dev, &local_err);
>          }
> @@ -1072,7 +1080,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>  
>          DEVICE_LISTENER_CALL(realize, Forward, dev);
>  
> -        hotplug_ctrl = qdev_get_hotplug_handler(dev);
>          if (hotplug_ctrl) {
>              hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
>          }
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index da1d0e4..c0db869 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -45,7 +45,8 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>   * hardware (un)plug functions.
>   *
>   * @parent: Opaque parent interface.
> - * @plug: plug callback.
> + * @pre_plug: pre plug callback called at start of device.realize(true)
> + * @plug: plug callback called at end of device.realize(true).
>   * @unplug_request: unplug request callback.
>   *                  Used as a means to initiate device unplug for devices that
>   *                  require asynchronous unplug handling.
> @@ -58,6 +59,7 @@ typedef struct HotplugHandlerClass {
>      InterfaceClass parent;
>  
>      /* <public> */
> +    hotplug_fn pre_plug;
>      hotplug_fn plug;
>      hotplug_fn unplug_request;
>      hotplug_fn unplug;
> @@ -73,6 +75,16 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
>                            Error **errp);
>  
>  /**
> + * hotplug_handler_pre_plug:
> + *
> + * Call #HotplugHandlerClass.pre_plug callback of @plug_handler.
> + */
> +void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
> +                              DeviceState *plugged_dev,
> +                              Error **errp);
> +
> +
> +/**
>   * hotplug_handler_unplug_request:
>   *
>   * Calls #HotplugHandlerClass.unplug_request callback of @plug_handler.

-- 
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] [for-2.7 PATCH v3 05/15] qdev: hotplug: Introduce HotplugHandler.pre_plug() callback

[Igor Mammedov]

On Thu, 2 Jun 2016 11:15:44 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, May 12, 2016 at 09:18:15AM +0530, Bharata B Rao wrote:
> > From: Igor Mammedov <imammedo@redhat.com>
> > 
> > pre_plug callback is to be called before device.realize() is
> > executed. This would allow to check/set device's properties from
> > HotplugHandler.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Igor,  do you think we're ready to merge this?
Yes, I think so.

> 
> If so, do you want to take it through your tree, or should I take it
> through the ppc tree?
Please take it through your tree, along with the rest patches
in this series.

> 
> > ---
> >  hw/core/hotplug.c    | 11 +++++++++++
> >  hw/core/qdev.c       |  9 ++++++++-
> >  include/hw/hotplug.h | 14 +++++++++++++-
> >  3 files changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> > index 645cfca..17ac986 100644
> > --- a/hw/core/hotplug.c
> > +++ b/hw/core/hotplug.c
> > @@ -13,6 +13,17 @@
> >  #include "hw/hotplug.h"
> >  #include "qemu/module.h"
> >  
> > +void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
> > +                              DeviceState *plugged_dev,
> > +                              Error **errp)
> > +{
> > +    HotplugHandlerClass *hdc =
> > HOTPLUG_HANDLER_GET_CLASS(plug_handler); +
> > +    if (hdc->pre_plug) {
> > +        hdc->pre_plug(plug_handler, plugged_dev, errp);
> > +    }
> > +}
> > +
> >  void hotplug_handler_plug(HotplugHandler *plug_handler,
> >                            DeviceState *plugged_dev,
> >                            Error **errp)
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index db41aa1..a0b3aad 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -1062,6 +1062,14 @@ static void device_set_realized(Object *obj,
> > bool value, Error **errp) g_free(name);
> >          }
> >  
> > +        hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > +        if (hotplug_ctrl) {
> > +            hotplug_handler_pre_plug(hotplug_ctrl, dev,
> > &local_err);
> > +            if (local_err != NULL) {
> > +                goto fail;
> > +            }
> > +        }
> > +
> >          if (dc->realize) {
> >              dc->realize(dev, &local_err);
> >          }
> > @@ -1072,7 +1080,6 @@ static void device_set_realized(Object *obj,
> > bool value, Error **errp) 
> >          DEVICE_LISTENER_CALL(realize, Forward, dev);
> >  
> > -        hotplug_ctrl = qdev_get_hotplug_handler(dev);
> >          if (hotplug_ctrl) {
> >              hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
> >          }
> > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> > index da1d0e4..c0db869 100644
> > --- a/include/hw/hotplug.h
> > +++ b/include/hw/hotplug.h
> > @@ -45,7 +45,8 @@ typedef void (*hotplug_fn)(HotplugHandler
> > *plug_handler,
> >   * hardware (un)plug functions.
> >   *
> >   * @parent: Opaque parent interface.
> > - * @plug: plug callback.
> > + * @pre_plug: pre plug callback called at start of
> > device.realize(true)
> > + * @plug: plug callback called at end of device.realize(true).
> >   * @unplug_request: unplug request callback.
> >   *                  Used as a means to initiate device unplug for
> > devices that
> >   *                  require asynchronous unplug handling.
> > @@ -58,6 +59,7 @@ typedef struct HotplugHandlerClass {
> >      InterfaceClass parent;
> >  
> >      /* <public> */
> > +    hotplug_fn pre_plug;
> >      hotplug_fn plug;
> >      hotplug_fn unplug_request;
> >      hotplug_fn unplug;
> > @@ -73,6 +75,16 @@ void hotplug_handler_plug(HotplugHandler
> > *plug_handler, Error **errp);
> >  
> >  /**
> > + * hotplug_handler_pre_plug:
> > + *
> > + * Call #HotplugHandlerClass.pre_plug callback of @plug_handler.
> > + */
> > +void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
> > +                              DeviceState *plugged_dev,
> > +                              Error **errp);
> > +
> > +
> > +/**
> >   * hotplug_handler_unplug_request:
> >   *
> >   * Calls #HotplugHandlerClass.unplug_request callback of
> > @plug_handler.
> 

Re: [Qemu-devel] [for-2.7 PATCH v3 05/15] qdev: hotplug: Introduce HotplugHandler.pre_plug() callback

[David Gibson]

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

On Thu, Jun 02, 2016 at 11:32:13AM +0200, Igor Mammedov wrote:
> On Thu, 2 Jun 2016 11:15:44 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, May 12, 2016 at 09:18:15AM +0530, Bharata B Rao wrote:
> > > From: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > pre_plug callback is to be called before device.realize() is
> > > executed. This would allow to check/set device's properties from
> > > HotplugHandler.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Igor,  do you think we're ready to merge this?
> Yes, I think so.
> 
> > 
> > If so, do you want to take it through your tree, or should I take it
> > through the ppc tree?
> Please take it through your tree, along with the rest patches
> in this series.

Ok, I've made a branch to collate these into, see:
  https://github.com/dgibson/qemu/tree/ppc-cpu-hotplug

For now just patches 5&6 are there (not ppc specific), but I plan to
merge in the rest as I review them.

-- 
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] [for-2.7 PATCH v3 05/15] qdev: hotplug: Introduce HotplugHandler.pre_plug() callback

[Igor Mammedov]

On Fri, 3 Jun 2016 15:10:45 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 02, 2016 at 11:32:13AM +0200, Igor Mammedov wrote:
> > On Thu, 2 Jun 2016 11:15:44 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, May 12, 2016 at 09:18:15AM +0530, Bharata B Rao wrote:  
> > > > From: Igor Mammedov <imammedo@redhat.com>
> > > > 
> > > > pre_plug callback is to be called before device.realize() is
> > > > executed. This would allow to check/set device's properties from
> > > > HotplugHandler.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>  
> > > 
> > > Igor,  do you think we're ready to merge this?  
> > Yes, I think so.
> >   
> > > 
> > > If so, do you want to take it through your tree, or should I take it
> > > through the ppc tree?  
> > Please take it through your tree, along with the rest patches
> > in this series.  
> 
> Ok, I've made a branch to collate these into, see:
>   https://github.com/dgibson/qemu/tree/ppc-cpu-hotplug
Great, thanks!

Looks ok to me.

> 
> For now just patches 5&6 are there (not ppc specific), but I plan to
> merge in the rest as I review them.
> 

[Qemu-devel] [for-2.7 PATCH v3 06/15] 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>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
               [Integer core property]
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/cpu/Makefile.objs  |  1 +
 hw/cpu/core.c         | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/cpu/core.h | 31 ++++++++++++++++++
 3 files changed, 120 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..fa5bc82
--- /dev/null
+++ b/hw/cpu/core.c
@@ -0,0 +1,88 @@
+/*
+ * 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 "qapi/error.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

Re: [Qemu-devel] [for-2.7 PATCH v3 06/15] cpu: Abstract CPU core type

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:16AM +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>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>                [Integer core property]
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Igor, Eduardo,

Do you think we're comfortable enough with this abstract core concept
to merge it now?  If so which tree should it go through?

> ---
>  hw/cpu/Makefile.objs  |  1 +
>  hw/cpu/core.c         | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/cpu/core.h | 31 ++++++++++++++++++
>  3 files changed, 120 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..fa5bc82
> --- /dev/null
> +++ b/hw/cpu/core.c
> @@ -0,0 +1,88 @@
> +/*
> + * 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 "qapi/error.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] [for-2.7 PATCH v3 06/15] cpu: Abstract CPU core type

[Igor Mammedov]

On Thu, 2 Jun 2016 13:38:58 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, May 12, 2016 at 09:18:16AM +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>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >                [Integer core property]
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Igor, Eduardo,
> 
> Do you think we're comfortable enough with this abstract core concept
> to merge it now?  If so which tree should it go through?
spapr is the only user of it, and it makes sense for patch
to be merged as part of series that actually uses it.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> > ---
> >  hw/cpu/Makefile.objs  |  1 +
> >  hw/cpu/core.c         | 88
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/cpu/core.h | 31 ++++++++++++++++++ 3 files changed, 120
> > 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..fa5bc82
> > --- /dev/null
> > +++ b/hw/cpu/core.c
> > @@ -0,0 +1,88 @@
> > +/*
> > + * 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 "qapi/error.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
> 

Re: [Qemu-devel] [for-2.7 PATCH v3 06/15] cpu: Abstract CPU core type

[Eduardo Habkost]

On Thu, Jun 02, 2016 at 01:38:58PM +1000, David Gibson wrote:
> On Thu, May 12, 2016 at 09:18:16AM +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>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >                [Integer core property]
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Igor, Eduardo,
> 
> Do you think we're comfortable enough with this abstract core concept
> to merge it now?  If so which tree should it go through?

TBH, I haven't reviewed the concept carefully. I hoped that
people that spent more time thinking about long-term plans
(especially Andreas) would help move the discussion forward, but
Andreas is moving away from QOM CPU.

I need to review the previous discussions more carefully, but the
concept looks simple enough to me, and I don't think we should
hold spapr work because we want a Grand Plan for generic CPU
abstractions to be finished first. If David, Igor, and the people
working on spapr are happy with it, I trust their judgement.

I just wish the interface was better documented, especially the
meaning of the "core" and "threads" properties. I would prefer to
have "core-id" as the property name instead of "core" (most of
the error messages related to it (in patch 08/15) say "core id").


> 
> > ---
> >  hw/cpu/Makefile.objs  |  1 +
> >  hw/cpu/core.c         | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/cpu/core.h | 31 ++++++++++++++++++
> >  3 files changed, 120 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..fa5bc82
> > --- /dev/null
> > +++ b/hw/cpu/core.c
> > @@ -0,0 +1,88 @@
> > +/*
> > + * 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 "qapi/error.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



-- 
Eduardo

Re: [Qemu-devel] [for-2.7 PATCH v3 06/15] cpu: Abstract CPU core type

[David Gibson]

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

On Thu, Jun 02, 2016 at 03:12:09PM -0300, Eduardo Habkost wrote:
> On Thu, Jun 02, 2016 at 01:38:58PM +1000, David Gibson wrote:
> > On Thu, May 12, 2016 at 09:18:16AM +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>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > >                [Integer core property]
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Igor, Eduardo,
> > 
> > Do you think we're comfortable enough with this abstract core concept
> > to merge it now?  If so which tree should it go through?
> 
> TBH, I haven't reviewed the concept carefully. I hoped that
> people that spent more time thinking about long-term plans
> (especially Andreas) would help move the discussion forward, but
> Andreas is moving away from QOM CPU.
> 
> I need to review the previous discussions more carefully, but the
> concept looks simple enough to me, and I don't think we should
> hold spapr work because we want a Grand Plan for generic CPU
> abstractions to be finished first. If David, Igor, and the people
> working on spapr are happy with it, I trust their judgement.
> 
> I just wish the interface was better documented, especially the
> meaning of the "core" and "threads" properties. I would prefer to
> have "core-id" as the property name instead of "core" (most of
> the error messages related to it (in patch 08/15) say "core id").

Ok, I've renamed the properties to 'core-id' and 'nr-threads' as I
merged, which I think is clearer.

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

[Qemu-devel] [for-2.7 PATCH v3 07/15] spapr: Abstract CPU core device and type specific core devices

[Bharata B Rao]

Add sPAPR specific abastract CPU core device that is based on generic
CPU core device. Use this as base type to create sPAPR CPU specific core
devices.

TODO:
- Add core types for other remaining CPU types
- Handle CPU model alias correctly

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/Makefile.objs            |   1 +
 hw/ppc/spapr.c                  |   3 +-
 hw/ppc/spapr_cpu_core.c         | 168 ++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   1 +
 include/hw/ppc/spapr_cpu_core.h |  28 +++++++
 5 files changed, 199 insertions(+), 2 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 b69995e..95db047 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1605,8 +1605,7 @@ 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)
+void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
 {
     CPUPPCState *env = &cpu->env;
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
new file mode 100644
index 0000000..af63ed9
--- /dev/null
+++ b/hw/ppc/spapr_cpu_core.c
@@ -0,0 +1,168 @@
+/*
+ * 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 "qapi/error.h"
+#include <sysemu/cpus.h>
+#include "target-ppc/kvm_ppc.h"
+
+static void spapr_cpu_core_create_threads(DeviceState *dev, int threads,
+                                          Error **errp)
+{
+    int i;
+    Error *local_err = NULL;
+    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
+
+    for (i = 0; i < threads; i++) {
+        char id[32];
+
+        object_initialize(&core->threads[i], sizeof(core->threads[i]),
+                          object_class_get_name(core->cpu));
+        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;
+    }
+    return 0;
+}
+
+static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
+{
+    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
+    CPUCore *cc = CPU_CORE(OBJECT(dev));
+    Error *local_err = NULL;
+
+    sc->threads = g_new0(PowerPCCPU, cc->threads);
+    spapr_cpu_core_create_threads(dev, cc->threads, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
+
+out:
+    if (local_err) {
+        g_free(sc->threads);
+        error_propagate(errp, local_err);
+    }
+}
+
+static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    dc->realize = spapr_cpu_core_realize;
+}
+
+/*
+ * instance_init routines from different flavours of sPAPR CPU cores.
+ * TODO: Add support for 'host' core type.
+ */
+#define SPAPR_CPU_CORE_INITFN(_type, _fname) \
+static void glue(glue(spapr_cpu_core_, _fname), _initfn(Object *obj)) \
+{ \
+    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); \
+    char *name = g_strdup_printf("%s-" TYPE_POWERPC_CPU, stringify(_type)); \
+    ObjectClass *oc = object_class_by_name(name); \
+    g_assert(oc); \
+    g_free((void *)name); \
+    core->cpu = oc; \
+}
+
+SPAPR_CPU_CORE_INITFN(POWER7_v2.3, POWER7);
+SPAPR_CPU_CORE_INITFN(POWER7+_v2.1, POWER7plus);
+SPAPR_CPU_CORE_INITFN(POWER8_v2.0, POWER8);
+SPAPR_CPU_CORE_INITFN(POWER8E_v2.1, POWER8E);
+
+typedef struct SPAPRCoreInfo {
+    const char *name;
+    void (*initfn)(Object *obj);
+} SPAPRCoreInfo;
+
+static const SPAPRCoreInfo spapr_cores[] = {
+    /* POWER7 and aliases */
+    { .name = "POWER7_v2.3", .initfn = spapr_cpu_core_POWER7_initfn },
+    { .name = "POWER7", .initfn = spapr_cpu_core_POWER7_initfn },
+
+    /* POWER7+ and aliases */
+    { .name = "POWER7+_v2.1", .initfn = spapr_cpu_core_POWER7plus_initfn },
+    { .name = "POWER7+", .initfn = spapr_cpu_core_POWER7plus_initfn },
+
+    /* POWER8 and aliases */
+    { .name = "POWER8_v2.0", .initfn = spapr_cpu_core_POWER8_initfn },
+    { .name = "POWER8", .initfn = spapr_cpu_core_POWER8_initfn },
+    { .name = "power8", .initfn = spapr_cpu_core_POWER8_initfn },
+
+    /* POWER8E and aliases */
+    { .name = "POWER8E_v2.1", .initfn = spapr_cpu_core_POWER8E_initfn },
+    { .name = "POWER8E", .initfn = spapr_cpu_core_POWER8E_initfn },
+
+    { .name = NULL }
+};
+
+static void spapr_cpu_core_register(const SPAPRCoreInfo *info)
+{
+    TypeInfo type_info = {
+        .parent = TYPE_SPAPR_CPU_CORE,
+        .instance_size = sizeof(sPAPRCPUCore),
+        .instance_init = info->initfn,
+    };
+
+    type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, info->name);
+    type_register(&type_info);
+    g_free((void *)type_info.name);
+}
+
+static const TypeInfo spapr_cpu_core_type_info = {
+    .name = TYPE_SPAPR_CPU_CORE,
+    .parent = TYPE_CPU_CORE,
+    .abstract = true,
+    .instance_size = sizeof(sPAPRCPUCore),
+    .class_init = spapr_cpu_core_class_init,
+};
+
+static void spapr_cpu_core_register_types(void)
+{
+    const SPAPRCoreInfo *info = spapr_cores;
+
+    type_register_static(&spapr_cpu_core_type_info);
+    while (info->name) {
+        spapr_cpu_core_register(info);
+        info++;
+    }
+}
+
+type_init(spapr_cpu_core_register_types)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 815d5ee..bcd9de6 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -580,6 +580,7 @@ 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);
 
 /* 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..b6aa39e
--- /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 >*/
+    PowerPCCPU *threads;
+    ObjectClass *cpu;
+} sPAPRCPUCore;
+
+#endif
-- 
2.1.0

Re: [Qemu-devel] [for-2.7 PATCH v3 07/15] spapr: Abstract CPU core device and type specific core devices

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:17AM +0530, Bharata B Rao wrote:
> Add sPAPR specific abastract CPU core device that is based on generic
> CPU core device. Use this as base type to create sPAPR CPU specific core
> devices.
> 
> TODO:
> - Add core types for other remaining CPU types
> - Handle CPU model alias correctly
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

This is looking pretty cood, but there's some minor changes I'd like
to see.

> ---
>  hw/ppc/Makefile.objs            |   1 +
>  hw/ppc/spapr.c                  |   3 +-
>  hw/ppc/spapr_cpu_core.c         | 168 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |   1 +
>  include/hw/ppc/spapr_cpu_core.h |  28 +++++++
>  5 files changed, 199 insertions(+), 2 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 b69995e..95db047 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1605,8 +1605,7 @@ 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)
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)

I think this function should actually move into spapr_cpu_core.c

>  {
>      CPUPPCState *env = &cpu->env;
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> new file mode 100644
> index 0000000..af63ed9
> --- /dev/null
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -0,0 +1,168 @@
> +/*
> + * 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 "qapi/error.h"
> +#include <sysemu/cpus.h>
> +#include "target-ppc/kvm_ppc.h"
> +
> +static void spapr_cpu_core_create_threads(DeviceState *dev, int threads,
> +                                          Error **errp)

This function could be folded into spapr_cpu_core_realize(), that's
the only caller and they're both fairly short.

> +{
> +    int i;
> +    Error *local_err = NULL;
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +
> +    for (i = 0; i < threads; i++) {
> +        char id[32];
> +
> +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> +                          object_class_get_name(core->cpu));

Given we have to go from the class pointer to the class name to
actually use it here, maybe we would be better off storing the name
rather than a class pointer.  Up to you, I'm happy either way.

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

Is this safe if some of the threads haven't been initialized?

> +    }
> +    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;
> +    }
> +    return 0;
> +}
> +
> +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> +    CPUCore *cc = CPU_CORE(OBJECT(dev));
> +    Error *local_err = NULL;
> +
> +    sc->threads = g_new0(PowerPCCPU, cc->threads);

This isn't quite safe, because it assume the structure size for all
the threads is that of PowerPCCPU.  That's true now, but cpu thread
subtypes could in theory extend that structure with subtype specific
fields.  I think we need to actually look up the class instance size
here.

> +    spapr_cpu_core_create_threads(dev, cc->threads, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
> +
> +out:
> +    if (local_err) {
> +        g_free(sc->threads);
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
> +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    dc->realize = spapr_cpu_core_realize;
> +}
> +
> +/*
> + * instance_init routines from different flavours of sPAPR CPU cores.
> + * TODO: Add support for 'host' core type.
> + */
> +#define SPAPR_CPU_CORE_INITFN(_type, _fname) \
> +static void glue(glue(spapr_cpu_core_, _fname), _initfn(Object *obj)) \
> +{ \
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); \
> +    char *name = g_strdup_printf("%s-" TYPE_POWERPC_CPU, stringify(_type)); \
> +    ObjectClass *oc = object_class_by_name(name); \
> +    g_assert(oc); \
> +    g_free((void *)name); \
> +    core->cpu = oc; \

This all looks like stuff which could be done at class_init time, but
this is an instance_init function.

I'm also wondering if it might just be simpler to store _type in the
subclass and do this in the base class.

> +}
> +
> +SPAPR_CPU_CORE_INITFN(POWER7_v2.3, POWER7);
> +SPAPR_CPU_CORE_INITFN(POWER7+_v2.1, POWER7plus);
> +SPAPR_CPU_CORE_INITFN(POWER8_v2.0, POWER8);
> +SPAPR_CPU_CORE_INITFN(POWER8E_v2.1, POWER8E);
> +
> +typedef struct SPAPRCoreInfo {
> +    const char *name;
> +    void (*initfn)(Object *obj);
> +} SPAPRCoreInfo;
> +
> +static const SPAPRCoreInfo spapr_cores[] = {
> +    /* POWER7 and aliases */
> +    { .name = "POWER7_v2.3", .initfn = spapr_cpu_core_POWER7_initfn },
> +    { .name = "POWER7", .initfn = spapr_cpu_core_POWER7_initfn },
> +
> +    /* POWER7+ and aliases */
> +    { .name = "POWER7+_v2.1", .initfn = spapr_cpu_core_POWER7plus_initfn },
> +    { .name = "POWER7+", .initfn = spapr_cpu_core_POWER7plus_initfn },
> +
> +    /* POWER8 and aliases */
> +    { .name = "POWER8_v2.0", .initfn = spapr_cpu_core_POWER8_initfn },
> +    { .name = "POWER8", .initfn = spapr_cpu_core_POWER8_initfn },
> +    { .name = "power8", .initfn = spapr_cpu_core_POWER8_initfn },
> +
> +    /* POWER8E and aliases */
> +    { .name = "POWER8E_v2.1", .initfn = spapr_cpu_core_POWER8E_initfn },
> +    { .name = "POWER8E", .initfn = spapr_cpu_core_POWER8E_initfn },
> +
> +    { .name = NULL }
> +};

> +
> +static void spapr_cpu_core_register(const SPAPRCoreInfo *info)
> +{
> +    TypeInfo type_info = {
> +        .parent = TYPE_SPAPR_CPU_CORE,
> +        .instance_size = sizeof(sPAPRCPUCore),
> +        .instance_init = info->initfn,
> +    };
> +
> +    type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, info->name);
> +    type_register(&type_info);
> +    g_free((void *)type_info.name);
> +}

The way the above registration works is pretty clunky.  I'm not
immediately sure how to make it nicer though, so I'm happy enough to
merge it this way and hope we can clean up later.

> +
> +static const TypeInfo spapr_cpu_core_type_info = {
> +    .name = TYPE_SPAPR_CPU_CORE,
> +    .parent = TYPE_CPU_CORE,
> +    .abstract = true,
> +    .instance_size = sizeof(sPAPRCPUCore),
> +    .class_init = spapr_cpu_core_class_init,
> +};
> +
> +static void spapr_cpu_core_register_types(void)
> +{
> +    const SPAPRCoreInfo *info = spapr_cores;
> +
> +    type_register_static(&spapr_cpu_core_type_info);
> +    while (info->name) {
> +        spapr_cpu_core_register(info);
> +        info++;
> +    }
> +}
> +
> +type_init(spapr_cpu_core_register_types)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 815d5ee..bcd9de6 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -580,6 +580,7 @@ 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);
>  
>  /* 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..b6aa39e
> --- /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 >*/
> +    PowerPCCPU *threads;
> +    ObjectClass *cpu;

I'd prefer to see this called 'cpu_class' or something - seeing 'cpu'
makes me think an instance rather than a class.

> +} sPAPRCPUCore;
> +
> +#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] [for-2.7 PATCH v3 07/15] spapr: Abstract CPU core device and type specific core devices

[Bharata B Rao]

On Fri, Jun 03, 2016 at 03:25:08PM +1000, David Gibson wrote:
> On Thu, May 12, 2016 at 09:18:17AM +0530, Bharata B Rao wrote:
> > Add sPAPR specific abastract CPU core device that is based on generic
> > CPU core device. Use this as base type to create sPAPR CPU specific core
> > devices.
> > 
> > TODO:
> > - Add core types for other remaining CPU types
> > - Handle CPU model alias correctly
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> This is looking pretty cood, but there's some minor changes I'd like
> to see.
> 
> > ---
> >  hw/ppc/Makefile.objs            |   1 +
> >  hw/ppc/spapr.c                  |   3 +-
> >  hw/ppc/spapr_cpu_core.c         | 168 ++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h          |   1 +
> >  include/hw/ppc/spapr_cpu_core.h |  28 +++++++
> >  5 files changed, 199 insertions(+), 2 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 b69995e..95db047 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1605,8 +1605,7 @@ 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)
> > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> 
> I think this function should actually move into spapr_cpu_core.c

Actually, this is a CPU thread specific routine and will be called from
spapr.c too. But I moved this to spapr_cpu_core.c as you suggest.

> 
> >  {
> >      CPUPPCState *env = &cpu->env;
> >  
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > new file mode 100644
> > index 0000000..af63ed9
> > --- /dev/null
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -0,0 +1,168 @@
> > +/*
> > + * 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 "qapi/error.h"
> > +#include <sysemu/cpus.h>
> > +#include "target-ppc/kvm_ppc.h"
> > +
> > +static void spapr_cpu_core_create_threads(DeviceState *dev, int threads,
> > +                                          Error **errp)
> 
> This function could be folded into spapr_cpu_core_realize(), that's
> the only caller and they're both fairly short.

Ok done.

> 
> > +{
> > +    int i;
> > +    Error *local_err = NULL;
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > +
> > +    for (i = 0; i < threads; i++) {
> > +        char id[32];
> > +
> > +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> > +                          object_class_get_name(core->cpu));
> 
> Given we have to go from the class pointer to the class name to
> actually use it here, maybe we would be better off storing the name
> rather than a class pointer.  Up to you, I'm happy either way.

I was doing typename initially and there were suggestions to move to
ObjectClass. May be I will leave it like this while I fix all the other
things and finally revisit this ?

> 
> > +        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]));
> 
> Is this safe if some of the threads haven't been initialized?

This is in the error path and only those threads which have been
initialized will be unparented.

> 
> > +    }
> > +    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;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > +{
> > +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > +    CPUCore *cc = CPU_CORE(OBJECT(dev));
> > +    Error *local_err = NULL;
> > +
> > +    sc->threads = g_new0(PowerPCCPU, cc->threads);
> 
> This isn't quite safe, because it assume the structure size for all
> the threads is that of PowerPCCPU.  That's true now, but cpu thread
> subtypes could in theory extend that structure with subtype specific
> fields.  I think we need to actually look up the class instance size
> here.

I couldn't find a way to obtain the instance_size of a type. I had to
introduce a QOM API like this:

diff --git a/qom/object.c b/qom/object.c
index 3bc8a00..0e75877 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -202,6 +202,14 @@ static size_t type_object_get_size(TypeImpl *ti)
     return 0;
 }
 
+size_t object_type_get_size(const char *typename)
+{
+    TypeImpl *type = type_get_by_name(typename);
+
+    g_assert(type != NULL);
+    return type_object_get_size(type);
+}
+

With the above I can obtain the size and use the same with
object_initialize(). With this we don't have to make
sc->threads as PowerPCCPU pointer.

> 
> > +    spapr_cpu_core_create_threads(dev, cc->threads, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
> > +
> > +out:
> > +    if (local_err) {
> > +        g_free(sc->threads);
> > +        error_propagate(errp, local_err);
> > +    }
> > +}
> > +
> > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > +    dc->realize = spapr_cpu_core_realize;
> > +}
> > +
> > +/*
> > + * instance_init routines from different flavours of sPAPR CPU cores.
> > + * TODO: Add support for 'host' core type.
> > + */
> > +#define SPAPR_CPU_CORE_INITFN(_type, _fname) \
> > +static void glue(glue(spapr_cpu_core_, _fname), _initfn(Object *obj)) \
> > +{ \
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); \
> > +    char *name = g_strdup_printf("%s-" TYPE_POWERPC_CPU, stringify(_type)); \
> > +    ObjectClass *oc = object_class_by_name(name); \
> > +    g_assert(oc); \
> > +    g_free((void *)name); \
> > +    core->cpu = oc; \
> 
> This all looks like stuff which could be done at class_init time, but
> this is an instance_init function.

Essentially what we do here is to store the CPU thread ObjectClass within the
abstract sPAPRCPUCore object so that we can initialize the CPU threads
of right type from abstract sPAPRCPUCore object's realize routine. I am not
sure if core object is available in class_init routine to achieve this.

> 
> I'm also wondering if it might just be simpler to store _type in the
> subclass and do this in the base class.

Not sure if I really understand this suggestion correctly.

However, if you see, I am not yet defining a separate structures for the
subtypes of sPAPRCPUCore where _type can be stored, but instead using
sPAPRCPUCore as the instance_size for sub core types too.

> 
> > +}
> > +
> > +SPAPR_CPU_CORE_INITFN(POWER7_v2.3, POWER7);
> > +SPAPR_CPU_CORE_INITFN(POWER7+_v2.1, POWER7plus);
> > +SPAPR_CPU_CORE_INITFN(POWER8_v2.0, POWER8);
> > +SPAPR_CPU_CORE_INITFN(POWER8E_v2.1, POWER8E);
> > +
> > +typedef struct SPAPRCoreInfo {
> > +    const char *name;
> > +    void (*initfn)(Object *obj);
> > +} SPAPRCoreInfo;
> > +
> > +static const SPAPRCoreInfo spapr_cores[] = {
> > +    /* POWER7 and aliases */
> > +    { .name = "POWER7_v2.3", .initfn = spapr_cpu_core_POWER7_initfn },
> > +    { .name = "POWER7", .initfn = spapr_cpu_core_POWER7_initfn },
> > +
> > +    /* POWER7+ and aliases */
> > +    { .name = "POWER7+_v2.1", .initfn = spapr_cpu_core_POWER7plus_initfn },
> > +    { .name = "POWER7+", .initfn = spapr_cpu_core_POWER7plus_initfn },
> > +
> > +    /* POWER8 and aliases */
> > +    { .name = "POWER8_v2.0", .initfn = spapr_cpu_core_POWER8_initfn },
> > +    { .name = "POWER8", .initfn = spapr_cpu_core_POWER8_initfn },
> > +    { .name = "power8", .initfn = spapr_cpu_core_POWER8_initfn },
> > +
> > +    /* POWER8E and aliases */
> > +    { .name = "POWER8E_v2.1", .initfn = spapr_cpu_core_POWER8E_initfn },
> > +    { .name = "POWER8E", .initfn = spapr_cpu_core_POWER8E_initfn },
> > +
> > +    { .name = NULL }
> > +};
> 
> > +
> > +static void spapr_cpu_core_register(const SPAPRCoreInfo *info)
> > +{
> > +    TypeInfo type_info = {
> > +        .parent = TYPE_SPAPR_CPU_CORE,
> > +        .instance_size = sizeof(sPAPRCPUCore),
> > +        .instance_init = info->initfn,
> > +    };
> > +
> > +    type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, info->name);
> > +    type_register(&type_info);
> > +    g_free((void *)type_info.name);
> > +}
> 
> The way the above registration works is pretty clunky.  I'm not
> immediately sure how to make it nicer though, so I'm happy enough to
> merge it this way and hope we can clean up later.

There is a precedence for this in target-arm/cpu.c :)

> 
> > +
> > +static const TypeInfo spapr_cpu_core_type_info = {
> > +    .name = TYPE_SPAPR_CPU_CORE,
> > +    .parent = TYPE_CPU_CORE,
> > +    .abstract = true,
> > +    .instance_size = sizeof(sPAPRCPUCore),
> > +    .class_init = spapr_cpu_core_class_init,
> > +};
> > +
> > +static void spapr_cpu_core_register_types(void)
> > +{
> > +    const SPAPRCoreInfo *info = spapr_cores;
> > +
> > +    type_register_static(&spapr_cpu_core_type_info);
> > +    while (info->name) {
> > +        spapr_cpu_core_register(info);
> > +        info++;
> > +    }
> > +}
> > +
> > +type_init(spapr_cpu_core_register_types)
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 815d5ee..bcd9de6 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -580,6 +580,7 @@ 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);
> >  
> >  /* 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..b6aa39e
> > --- /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 >*/
> > +    PowerPCCPU *threads;
> > +    ObjectClass *cpu;
> 
> I'd prefer to see this called 'cpu_class' or something - seeing 'cpu'
> makes me think an instance rather than a class.

Changed to cpu_class.

Regards,
Bharata.

Re: [Qemu-devel] [for-2.7 PATCH v3 07/15] spapr: Abstract CPU core device and type specific core devices

[David Gibson]

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

On Wed, Jun 08, 2016 at 03:12:42PM +0530, Bharata B Rao wrote:
> On Fri, Jun 03, 2016 at 03:25:08PM +1000, David Gibson wrote:
> > On Thu, May 12, 2016 at 09:18:17AM +0530, Bharata B Rao wrote:
> > > Add sPAPR specific abastract CPU core device that is based on generic
> > > CPU core device. Use this as base type to create sPAPR CPU specific core
> > > devices.
> > > 
> > > TODO:
> > > - Add core types for other remaining CPU types
> > > - Handle CPU model alias correctly
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > This is looking pretty cood, but there's some minor changes I'd like
> > to see.
> > 
> > > ---
> > >  hw/ppc/Makefile.objs            |   1 +
> > >  hw/ppc/spapr.c                  |   3 +-
> > >  hw/ppc/spapr_cpu_core.c         | 168 ++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr.h          |   1 +
> > >  include/hw/ppc/spapr_cpu_core.h |  28 +++++++
> > >  5 files changed, 199 insertions(+), 2 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 b69995e..95db047 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1605,8 +1605,7 @@ 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)
> > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > 
> > I think this function should actually move into spapr_cpu_core.c
> 
> Actually, this is a CPU thread specific routine and will be called from
> spapr.c too. But I moved this to spapr_cpu_core.c as you suggest.

Yes, I realize it will be called from spapr.c as well.  The idea is to
make the core based initialization the clean path, even if the
thread-based initialization needed for backwards compat needs some
awkward cross-module calls and exports.

> > >  {
> > >      CPUPPCState *env = &cpu->env;
> > >  
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > new file mode 100644
> > > index 0000000..af63ed9
> > > --- /dev/null
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -0,0 +1,168 @@
> > > +/*
> > > + * 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 "qapi/error.h"
> > > +#include <sysemu/cpus.h>
> > > +#include "target-ppc/kvm_ppc.h"
> > > +
> > > +static void spapr_cpu_core_create_threads(DeviceState *dev, int threads,
> > > +                                          Error **errp)
> > 
> > This function could be folded into spapr_cpu_core_realize(), that's
> > the only caller and they're both fairly short.
> 
> Ok done.
> 
> > 
> > > +{
> > > +    int i;
> > > +    Error *local_err = NULL;
> > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > +
> > > +    for (i = 0; i < threads; i++) {
> > > +        char id[32];
> > > +
> > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> > > +                          object_class_get_name(core->cpu));
> > 
> > Given we have to go from the class pointer to the class name to
> > actually use it here, maybe we would be better off storing the name
> > rather than a class pointer.  Up to you, I'm happy either way.
> 
> I was doing typename initially and there were suggestions to move to
> ObjectClass. May be I will leave it like this while I fix all the other
> things and finally revisit this ?

Yes, that makes sense.  As I say it's not really a big deal either
way.

Could you use object_initialize_with_type() to avoid going via the
type name?

> > > +        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]));
> > 
> > Is this safe if some of the threads haven't been initialized?
> 
> This is in the error path and only those threads which have been
> initialized will be unparented.

Ah, I misread the loop condition.  Except... there are other problems
with that loop condition.

First, (--i) means i will be decremented before you do anything, so
you won't clean up the cpu which actually triggered the error in the
first place.  And the if condition also means the loop body will never
be called with (i == 0).  So both the first and last threads
initialized won't actually be cleaned up.

> > > +    }
> > > +    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;
> > > +    }
> > > +    return 0;
> > > +}
> > > +
> > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > > +    CPUCore *cc = CPU_CORE(OBJECT(dev));
> > > +    Error *local_err = NULL;
> > > +
> > > +    sc->threads = g_new0(PowerPCCPU, cc->threads);
> > 
> > This isn't quite safe, because it assume the structure size for all
> > the threads is that of PowerPCCPU.  That's true now, but cpu thread
> > subtypes could in theory extend that structure with subtype specific
> > fields.  I think we need to actually look up the class instance size
> > here.
> 
> I couldn't find a way to obtain the instance_size of a type. I had to
> introduce a QOM API like this:
> 
> diff --git a/qom/object.c b/qom/object.c
> index 3bc8a00..0e75877 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -202,6 +202,14 @@ static size_t type_object_get_size(TypeImpl *ti)
>      return 0;
>  }
>  
> +size_t object_type_get_size(const char *typename)
> +{
> +    TypeImpl *type = type_get_by_name(typename);
> +
> +    g_assert(type != NULL);
> +    return type_object_get_size(type);
> +}
> +
> 
> With the above I can obtain the size and use the same with
> object_initialize(). With this we don't have to make
> sc->threads as PowerPCCPU pointer.
> 
> > 
> > > +    spapr_cpu_core_create_threads(dev, cc->threads, &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
> > > +
> > > +out:
> > > +    if (local_err) {
> > > +        g_free(sc->threads);
> > > +        error_propagate(errp, local_err);
> > > +    }
> > > +}
> > > +
> > > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> > > +{
> > > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > > +    dc->realize = spapr_cpu_core_realize;
> > > +}
> > > +
> > > +/*
> > > + * instance_init routines from different flavours of sPAPR CPU cores.
> > > + * TODO: Add support for 'host' core type.
> > > + */
> > > +#define SPAPR_CPU_CORE_INITFN(_type, _fname) \
> > > +static void glue(glue(spapr_cpu_core_, _fname), _initfn(Object *obj)) \
> > > +{ \
> > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); \
> > > +    char *name = g_strdup_printf("%s-" TYPE_POWERPC_CPU, stringify(_type)); \
> > > +    ObjectClass *oc = object_class_by_name(name); \
> > > +    g_assert(oc); \
> > > +    g_free((void *)name); \
> > > +    core->cpu = oc; \
> > 
> > This all looks like stuff which could be done at class_init time, but
> > this is an instance_init function.
> 
> Essentially what we do here is to store the CPU thread ObjectClass within the
> abstract sPAPRCPUCore object so that we can initialize the CPU threads
> of right type from abstract sPAPRCPUCore object's realize routine. I am not
> sure if core object is available in class_init routine to achieve this.

It's not, but my point is you can move the ObjectClass * field from
the instance to the class structure, and initialize it from class_init
instead of instance_init.

> > I'm also wondering if it might just be simpler to store _type in the
> > subclass and do this in the base class.
> 
> Not sure if I really understand this suggestion correctly.
> 
> However, if you see, I am not yet defining a separate structures for the
> subtypes of sPAPRCPUCore where _type can be stored, but instead using
> sPAPRCPUCore as the instance_size for sub core types too.

Uh.. not relevant to the point I'm making.

> > > +}
> > > +
> > > +SPAPR_CPU_CORE_INITFN(POWER7_v2.3, POWER7);
> > > +SPAPR_CPU_CORE_INITFN(POWER7+_v2.1, POWER7plus);
> > > +SPAPR_CPU_CORE_INITFN(POWER8_v2.0, POWER8);
> > > +SPAPR_CPU_CORE_INITFN(POWER8E_v2.1, POWER8E);
> > > +
> > > +typedef struct SPAPRCoreInfo {
> > > +    const char *name;
> > > +    void (*initfn)(Object *obj);
> > > +} SPAPRCoreInfo;
> > > +
> > > +static const SPAPRCoreInfo spapr_cores[] = {
> > > +    /* POWER7 and aliases */
> > > +    { .name = "POWER7_v2.3", .initfn = spapr_cpu_core_POWER7_initfn },
> > > +    { .name = "POWER7", .initfn = spapr_cpu_core_POWER7_initfn },
> > > +
> > > +    /* POWER7+ and aliases */
> > > +    { .name = "POWER7+_v2.1", .initfn = spapr_cpu_core_POWER7plus_initfn },
> > > +    { .name = "POWER7+", .initfn = spapr_cpu_core_POWER7plus_initfn },
> > > +
> > > +    /* POWER8 and aliases */
> > > +    { .name = "POWER8_v2.0", .initfn = spapr_cpu_core_POWER8_initfn },
> > > +    { .name = "POWER8", .initfn = spapr_cpu_core_POWER8_initfn },
> > > +    { .name = "power8", .initfn = spapr_cpu_core_POWER8_initfn },
> > > +
> > > +    /* POWER8E and aliases */
> > > +    { .name = "POWER8E_v2.1", .initfn = spapr_cpu_core_POWER8E_initfn },
> > > +    { .name = "POWER8E", .initfn = spapr_cpu_core_POWER8E_initfn },
> > > +
> > > +    { .name = NULL }
> > > +};
> > 
> > > +
> > > +static void spapr_cpu_core_register(const SPAPRCoreInfo *info)
> > > +{
> > > +    TypeInfo type_info = {
> > > +        .parent = TYPE_SPAPR_CPU_CORE,
> > > +        .instance_size = sizeof(sPAPRCPUCore),
> > > +        .instance_init = info->initfn,
> > > +    };
> > > +
> > > +    type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, info->name);
> > > +    type_register(&type_info);
> > > +    g_free((void *)type_info.name);
> > > +}
> > 
> > The way the above registration works is pretty clunky.  I'm not
> > immediately sure how to make it nicer though, so I'm happy enough to
> > merge it this way and hope we can clean up later.
> 
> There is a precedence for this in target-arm/cpu.c :)

Ok, fair enough.

> > > +
> > > +static const TypeInfo spapr_cpu_core_type_info = {
> > > +    .name = TYPE_SPAPR_CPU_CORE,
> > > +    .parent = TYPE_CPU_CORE,
> > > +    .abstract = true,
> > > +    .instance_size = sizeof(sPAPRCPUCore),
> > > +    .class_init = spapr_cpu_core_class_init,
> > > +};
> > > +
> > > +static void spapr_cpu_core_register_types(void)
> > > +{
> > > +    const SPAPRCoreInfo *info = spapr_cores;
> > > +
> > > +    type_register_static(&spapr_cpu_core_type_info);
> > > +    while (info->name) {
> > > +        spapr_cpu_core_register(info);
> > > +        info++;
> > > +    }
> > > +}
> > > +
> > > +type_init(spapr_cpu_core_register_types)
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index 815d5ee..bcd9de6 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -580,6 +580,7 @@ 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);
> > >  
> > >  /* 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..b6aa39e
> > > --- /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 >*/
> > > +    PowerPCCPU *threads;
> > > +    ObjectClass *cpu;
> > 
> > I'd prefer to see this called 'cpu_class' or something - seeing 'cpu'
> > makes me think an instance rather than a class.
> 
> Changed to cpu_class.

Thanks.

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

[Qemu-devel] [for-2.7 PATCH v3 08/15] spapr: convert boot CPUs into CPU core devices

[Bharata B Rao]

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.

TODO: We derive sPAPR core type by looking at -cpu <model>. However
we don't take care of "compat=" feature yet for boot time as well
as hotplug CPUs.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c                  | 76 +++++++++++++++++++++++++++++++++++------
 hw/ppc/spapr_cpu_core.c         | 58 +++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |  2 ++
 include/hw/ppc/spapr_cpu_core.h |  3 ++
 4 files changed, 129 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 95db047..0f64218 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -65,6 +65,7 @@
 
 #include "hw/compat.h"
 #include "qemu/cutils.h"
+#include "hw/ppc/spapr_cpu_core.h"
 
 #include <libfdt.h>
 
@@ -1605,6 +1606,10 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
     machine->boot_order = g_strdup(boot_device);
 }
 
+/*
+ * 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;
@@ -1628,6 +1633,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
     xics_cpu_setup(spapr->icp, cpu);
 
     qemu_register_reset(spapr_cpu_reset, cpu);
+    spapr_cpu_reset(cpu);
 }
 
 /*
@@ -1711,7 +1717,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();
@@ -1725,6 +1730,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_nonbroken = true;
 
@@ -1771,8 +1792,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) {
@@ -1783,13 +1803,37 @@ 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_cores; i++) {
+            int core_dt_id = i * smt;
+            char *type = spapr_get_cpu_core_type(machine->cpu_model);
+            Object *core;
+
+            if (!object_class_by_name(type)) {
+                error_report("Unable to find sPAPR CPU Core definition");
+                exit(1);
+            }
+
+            core  = object_new(type);
+            g_free(type);
+            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()) {
@@ -2245,10 +2289,19 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
     }
 }
 
+static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
+                                          DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        spapr_core_pre_plug(hotplug_dev, dev, errp);
+    }
+}
+
 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;
@@ -2287,11 +2340,13 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->has_dynamic_sysbus = true;
     mc->pci_allow_0_address = true;
     mc->get_hotplug_handler = spapr_get_hotpug_handler;
+    hc->pre_plug = spapr_machine_device_pre_plug;
     hc->plug = spapr_machine_device_plug;
     hc->unplug = spapr_machine_device_unplug;
     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;
 }
@@ -2376,6 +2431,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
index af63ed9..4450362 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -14,6 +14,64 @@
 #include <sysemu/cpus.h>
 #include "target-ppc/kvm_ppc.h"
 
+/*
+ * Return the sPAPR CPU core type for @model which essentially is the CPU
+ * model specified with -cpu cmdline option.
+ */
+char *spapr_get_cpu_core_type(const char *model)
+{
+    char core_type[32];
+    gchar **model_pieces = g_strsplit(model, ",", 2);
+
+    snprintf(core_type, 32, "%s-%s", model_pieces[0], TYPE_SPAPR_CPU_CORE);
+    g_strfreev(model_pieces);
+    return g_strdup(core_type);
+}
+
+void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                         Error **errp)
+{
+    MachineState *machine = MACHINE(OBJECT(hotplug_dev));
+    sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
+    int spapr_max_cores = max_cpus / smp_threads;
+    int index;
+    int smt = kvmppc_smt_threads();
+    Error *local_err = NULL;
+    CPUCore *cc = CPU_CORE(dev);
+    char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
+    const char *type = object_get_typename(OBJECT(dev));
+
+    if (strcmp(base_core_type, type)) {
+        error_setg(&local_err, "CPU core type should be %s", base_core_type);
+        goto out;
+    }
+
+    if (cc->threads != smp_threads) {
+        error_setg(&local_err, "threads must be %d", smp_threads);
+        goto out;
+    }
+
+    if (cc->core % smt) {
+        error_setg(&local_err, "invalid core id %d\n", cc->core);
+        goto out;
+    }
+
+    index = cc->core / smt;
+    if (index < 0 || index >= spapr_max_cores) {
+        error_setg(&local_err, "core id %d out of range", cc->core);
+        goto out;
+    }
+
+    if (spapr->cores[index]) {
+        error_setg(&local_err, "core %d already populated", cc->core);
+        goto out;
+    }
+
+out:
+    g_free(base_core_type);
+    error_propagate(errp, local_err);
+}
+
 static void spapr_cpu_core_create_threads(DeviceState *dev, int threads,
                                           Error **errp)
 {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index bcd9de6..1b3eeee 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
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index b6aa39e..f1ebd85 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -25,4 +25,7 @@ typedef struct sPAPRCPUCore {
     ObjectClass *cpu;
 } sPAPRCPUCore;
 
+void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                         Error **errp);
+char *spapr_get_cpu_core_type(const char *model);
 #endif
-- 
2.1.0

Re: [Qemu-devel] [for-2.7 PATCH v3 08/15] spapr: convert boot CPUs into CPU core devices

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:18AM +0530, Bharata B Rao wrote:
> 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.
> 
> TODO: We derive sPAPR core type by looking at -cpu <model>. However
> we don't take care of "compat=" feature yet for boot time as well
> as hotplug CPUs.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

This will need some tweaking for the changes I made to earlier
patches, otherwise only a couple of tiny nits.

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

> ---
>  hw/ppc/spapr.c                  | 76 +++++++++++++++++++++++++++++++++++------
>  hw/ppc/spapr_cpu_core.c         | 58 +++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |  2 ++
>  include/hw/ppc/spapr_cpu_core.h |  3 ++
>  4 files changed, 129 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 95db047..0f64218 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -65,6 +65,7 @@
>  
>  #include "hw/compat.h"
>  #include "qemu/cutils.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  
>  #include <libfdt.h>
>  
> @@ -1605,6 +1606,10 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>      machine->boot_order = g_strdup(boot_device);
>  }
>  
> +/*
> + * 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;
> @@ -1628,6 +1633,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
>      xics_cpu_setup(spapr->icp, cpu);
>  
>      qemu_register_reset(spapr_cpu_reset, cpu);
> +    spapr_cpu_reset(cpu);
>  }
>  
>  /*
> @@ -1711,7 +1717,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();
> @@ -1725,6 +1730,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_nonbroken = true;
>  
> @@ -1771,8 +1792,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) {
> @@ -1783,13 +1803,37 @@ 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_cores; i++) {
> +            int core_dt_id = i * smt;
> +            char *type = spapr_get_cpu_core_type(machine->cpu_model);

Probably makes sense to move this lookup outside the loop.

> +            Object *core;
> +
> +            if (!object_class_by_name(type)) {
> +                error_report("Unable to find sPAPR CPU Core definition");
> +                exit(1);
> +            }
> +
> +            core  = object_new(type);
> +            g_free(type);
> +            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()) {
> @@ -2245,10 +2289,19 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>      }
>  }
>  
> +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> +                                          DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> +        spapr_core_pre_plug(hotplug_dev, dev, errp);
> +    }
> +}
> +
>  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;
> @@ -2287,11 +2340,13 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->has_dynamic_sysbus = true;
>      mc->pci_allow_0_address = true;
>      mc->get_hotplug_handler = spapr_get_hotpug_handler;
> +    hc->pre_plug = spapr_machine_device_pre_plug;
>      hc->plug = spapr_machine_device_plug;
>      hc->unplug = spapr_machine_device_unplug;
>      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;
>  }
> @@ -2376,6 +2431,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
> index af63ed9..4450362 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -14,6 +14,64 @@
>  #include <sysemu/cpus.h>
>  #include "target-ppc/kvm_ppc.h"
>  
> +/*
> + * Return the sPAPR CPU core type for @model which essentially is the CPU
> + * model specified with -cpu cmdline option.
> + */
> +char *spapr_get_cpu_core_type(const char *model)
> +{
> +    char core_type[32];
> +    gchar **model_pieces = g_strsplit(model, ",", 2);
> +
> +    snprintf(core_type, 32, "%s-%s", model_pieces[0], TYPE_SPAPR_CPU_CORE);
> +    g_strfreev(model_pieces);
> +    return g_strdup(core_type);

You could use g_strdup_printf() here.

> +}
> +
> +void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                         Error **errp)
> +{
> +    MachineState *machine = MACHINE(OBJECT(hotplug_dev));
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> +    int spapr_max_cores = max_cpus / smp_threads;
> +    int index;
> +    int smt = kvmppc_smt_threads();
> +    Error *local_err = NULL;
> +    CPUCore *cc = CPU_CORE(dev);
> +    char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> +    const char *type = object_get_typename(OBJECT(dev));
> +
> +    if (strcmp(base_core_type, type)) {
> +        error_setg(&local_err, "CPU core type should be %s", base_core_type);
> +        goto out;
> +    }
> +
> +    if (cc->threads != smp_threads) {
> +        error_setg(&local_err, "threads must be %d", smp_threads);
> +        goto out;
> +    }
> +
> +    if (cc->core % smt) {
> +        error_setg(&local_err, "invalid core id %d\n", cc->core);
> +        goto out;
> +    }
> +
> +    index = cc->core / smt;
> +    if (index < 0 || index >= spapr_max_cores) {
> +        error_setg(&local_err, "core id %d out of range", cc->core);
> +        goto out;
> +    }
> +
> +    if (spapr->cores[index]) {
> +        error_setg(&local_err, "core %d already populated", cc->core);
> +        goto out;
> +    }
> +
> +out:
> +    g_free(base_core_type);
> +    error_propagate(errp, local_err);
> +}
> +
>  static void spapr_cpu_core_create_threads(DeviceState *dev, int threads,
>                                            Error **errp)
>  {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index bcd9de6..1b3eeee 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
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index b6aa39e..f1ebd85 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -25,4 +25,7 @@ typedef struct sPAPRCPUCore {
>      ObjectClass *cpu;
>  } sPAPRCPUCore;
>  
> +void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                         Error **errp);
> +char *spapr_get_cpu_core_type(const char *model);
>  #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] [for-2.7 PATCH v3 08/15] spapr: convert boot CPUs into CPU core devices

[Bharata B Rao]

On Fri, Jun 03, 2016 at 03:32:10PM +1000, David Gibson wrote:
> On Thu, May 12, 2016 at 09:18:18AM +0530, Bharata B Rao wrote:
> > 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.
> > 
> > TODO: We derive sPAPR core type by looking at -cpu <model>. However
> > we don't take care of "compat=" feature yet for boot time as well
> > as hotplug CPUs.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> This will need some tweaking for the changes I made to earlier
> patches, otherwise only a couple of tiny nits.

Took care of it.

> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> > ---
> >  hw/ppc/spapr.c                  | 76 +++++++++++++++++++++++++++++++++++------
> >  hw/ppc/spapr_cpu_core.c         | 58 +++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h          |  2 ++
> >  include/hw/ppc/spapr_cpu_core.h |  3 ++
> >  4 files changed, 129 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 95db047..0f64218 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -65,6 +65,7 @@
> >  
> >  #include "hw/compat.h"
> >  #include "qemu/cutils.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> >  
> >  #include <libfdt.h>
> >  
> > @@ -1605,6 +1606,10 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> >      machine->boot_order = g_strdup(boot_device);
> >  }
> >  
> > +/*
> > + * 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;
> > @@ -1628,6 +1633,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> >      xics_cpu_setup(spapr->icp, cpu);
> >  
> >      qemu_register_reset(spapr_cpu_reset, cpu);
> > +    spapr_cpu_reset(cpu);
> >  }
> >  
> >  /*
> > @@ -1711,7 +1717,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();
> > @@ -1725,6 +1730,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_nonbroken = true;
> >  
> > @@ -1771,8 +1792,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) {
> > @@ -1783,13 +1803,37 @@ 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_cores; i++) {
> > +            int core_dt_id = i * smt;
> > +            char *type = spapr_get_cpu_core_type(machine->cpu_model);
> 
> Probably makes sense to move this lookup outside the loop.

Yes.

> 
> > +            Object *core;
> > +
> > +            if (!object_class_by_name(type)) {
> > +                error_report("Unable to find sPAPR CPU Core definition");
> > +                exit(1);
> > +            }
> > +
> > +            core  = object_new(type);
> > +            g_free(type);
> > +            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()) {
> > @@ -2245,10 +2289,19 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> >      }
> >  }
> >  
> > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > +                                          DeviceState *dev, Error **errp)
> > +{
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > +        spapr_core_pre_plug(hotplug_dev, dev, errp);
> > +    }
> > +}
> > +
> >  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;
> > @@ -2287,11 +2340,13 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> >      mc->has_dynamic_sysbus = true;
> >      mc->pci_allow_0_address = true;
> >      mc->get_hotplug_handler = spapr_get_hotpug_handler;
> > +    hc->pre_plug = spapr_machine_device_pre_plug;
> >      hc->plug = spapr_machine_device_plug;
> >      hc->unplug = spapr_machine_device_unplug;
> >      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;
> >  }
> > @@ -2376,6 +2431,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
> > index af63ed9..4450362 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -14,6 +14,64 @@
> >  #include <sysemu/cpus.h>
> >  #include "target-ppc/kvm_ppc.h"
> >  
> > +/*
> > + * Return the sPAPR CPU core type for @model which essentially is the CPU
> > + * model specified with -cpu cmdline option.
> > + */
> > +char *spapr_get_cpu_core_type(const char *model)
> > +{
> > +    char core_type[32];
> > +    gchar **model_pieces = g_strsplit(model, ",", 2);
> > +
> > +    snprintf(core_type, 32, "%s-%s", model_pieces[0], TYPE_SPAPR_CPU_CORE);
> > +    g_strfreev(model_pieces);
> > +    return g_strdup(core_type);
> 
> You could use g_strdup_printf() here.

Yes, incorporated the above changes.

Regards,
Bharata.

[Qemu-devel] [for-2.7 PATCH v3 09/15] 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                  | 91 ++++++++++++++++++++++++++++++++++-------
 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, 176 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0f64218..8c3100d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -605,6 +605,16 @@ 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)};
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+    int drc_index;
+
+    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
+    if (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
@@ -988,6 +998,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) {
@@ -1613,6 +1633,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);
@@ -1630,6 +1652,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);
@@ -1807,23 +1837,31 @@ static void ppc_spapr_init(MachineState *machine)
     if (smc->dr_cpu_enabled) {
         spapr->cores = g_new0(Object *, spapr_max_cores);
 
-        for (i = 0; i < spapr_cores; i++) {
+        for (i = 0; i < spapr_max_cores; i++) {
             int core_dt_id = i * smt;
-            char *type = spapr_get_cpu_core_type(machine->cpu_model);
-            Object *core;
-
-            if (!object_class_by_name(type)) {
-                error_report("Unable to find sPAPR CPU Core definition");
-                exit(1);
+            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) {
+                char *type = spapr_get_cpu_core_type(machine->cpu_model);
+                Object *core;
+
+                if (!object_class_by_name(type)) {
+                    error_report("Unable to find sPAPR CPU Core definition");
+                    exit(1);
+                }
+
+                core  = object_new(type);
+                g_free(type);
+                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);
             }
-
-            core  = object_new(type);
-            g_free(type);
-            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);
         }
     } else {
         for (i = 0; i < smp_cpus; i++) {
@@ -2234,6 +2272,27 @@ out:
     error_propagate(errp, local_err);
 }
 
+void *spapr_populate_hotplug_cpu_dt(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)
 {
@@ -2278,6 +2337,8 @@ 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)) {
+        spapr_core_plug(hotplug_dev, dev, errp);
     }
 }
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 4450362..ed36beb 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -28,10 +28,74 @@ char *spapr_get_cpu_core_type(const char *model)
     return g_strdup(core_type);
 }
 
+void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                     Error **errp)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(OBJECT(hotplug_dev));
+    sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
+    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
+    CPUCore *cc = CPU_CORE(dev);
+    CPUState *cs = CPU(&core->threads[0]);
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+    Error *local_err = NULL;
+    void *fdt = NULL;
+    int fdt_offset = 0;
+    int index;
+    int smt = kvmppc_smt_threads();
+
+    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core);
+    index = cc->core / smt;
+    spapr->cores[index] = 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(cs, &fdt_offset, spapr);
+    }
+
+    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[index] = 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);
+    }
+}
+
 void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                          Error **errp)
 {
     MachineState *machine = MACHINE(OBJECT(hotplug_dev));
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(OBJECT(hotplug_dev));
     sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
     int spapr_max_cores = max_cpus / smp_threads;
     int index;
@@ -46,6 +110,11 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out;
     }
 
+    if (!smc->dr_cpu_enabled && dev->hotplugged) {
+        error_setg(&local_err, "CPU hotplug not supported for this machine");
+        goto out;
+    }
+
     if (cc->threads != smp_threads) {
         error_setg(&local_err, "threads must be %d", smp_threads);
         goto out;
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 049fb1b..af80992 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -449,6 +449,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 f073258..e53ecc0 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"
 
@@ -162,6 +163,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,
@@ -198,6 +220,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 1b3eeee..ca4ae3e 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -583,6 +583,8 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
 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_populate_hotplug_cpu_dt(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 f1ebd85..4cd837e 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -28,4 +28,6 @@ typedef struct sPAPRCPUCore {
 void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                          Error **errp);
 char *spapr_get_cpu_core_type(const char *model);
+void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                     Error **errp);
 #endif
-- 
2.1.0

Re: [Qemu-devel] [for-2.7 PATCH v3 09/15] spapr: CPU hotplug support

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:19AM +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>

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

> ---
>  hw/ppc/spapr.c                  | 91 ++++++++++++++++++++++++++++++++++-------
>  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, 176 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0f64218..8c3100d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -605,6 +605,16 @@ 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)};
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +    int drc_index;
> +
> +    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
> +    if (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
> @@ -988,6 +998,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) {
> @@ -1613,6 +1633,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);
> @@ -1630,6 +1652,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);
> @@ -1807,23 +1837,31 @@ static void ppc_spapr_init(MachineState *machine)
>      if (smc->dr_cpu_enabled) {
>          spapr->cores = g_new0(Object *, spapr_max_cores);
>  
> -        for (i = 0; i < spapr_cores; i++) {
> +        for (i = 0; i < spapr_max_cores; i++) {
>              int core_dt_id = i * smt;
> -            char *type = spapr_get_cpu_core_type(machine->cpu_model);
> -            Object *core;
> -
> -            if (!object_class_by_name(type)) {
> -                error_report("Unable to find sPAPR CPU Core definition");
> -                exit(1);
> +            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) {
> +                char *type = spapr_get_cpu_core_type(machine->cpu_model);
> +                Object *core;
> +
> +                if (!object_class_by_name(type)) {
> +                    error_report("Unable to find sPAPR CPU Core definition");
> +                    exit(1);
> +                }
> +
> +                core  = object_new(type);
> +                g_free(type);
> +                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);
>              }
> -
> -            core  = object_new(type);
> -            g_free(type);
> -            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);
>          }
>      } else {
>          for (i = 0; i < smp_cpus; i++) {
> @@ -2234,6 +2272,27 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> +void *spapr_populate_hotplug_cpu_dt(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)
>  {
> @@ -2278,6 +2337,8 @@ 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)) {
> +        spapr_core_plug(hotplug_dev, dev, errp);
>      }
>  }
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4450362..ed36beb 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -28,10 +28,74 @@ char *spapr_get_cpu_core_type(const char *model)
>      return g_strdup(core_type);
>  }
>  
> +void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                     Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(OBJECT(hotplug_dev));
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +    CPUCore *cc = CPU_CORE(dev);
> +    CPUState *cs = CPU(&core->threads[0]);
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +    Error *local_err = NULL;
> +    void *fdt = NULL;
> +    int fdt_offset = 0;
> +    int index;
> +    int smt = kvmppc_smt_threads();
> +
> +    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core);
> +    index = cc->core / smt;
> +    spapr->cores[index] = 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(cs, &fdt_offset, spapr);
> +    }
> +
> +    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[index] = 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);
> +    }
> +}
> +
>  void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                           Error **errp)
>  {
>      MachineState *machine = MACHINE(OBJECT(hotplug_dev));
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(OBJECT(hotplug_dev));
>      sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
>      int spapr_max_cores = max_cpus / smp_threads;
>      int index;
> @@ -46,6 +110,11 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> +    if (!smc->dr_cpu_enabled && dev->hotplugged) {
> +        error_setg(&local_err, "CPU hotplug not supported for this machine");
> +        goto out;
> +    }
> +
>      if (cc->threads != smp_threads) {
>          error_setg(&local_err, "threads must be %d", smp_threads);
>          goto out;
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 049fb1b..af80992 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -449,6 +449,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 f073258..e53ecc0 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"
>  
> @@ -162,6 +163,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,
> @@ -198,6 +220,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 1b3eeee..ca4ae3e 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -583,6 +583,8 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
>  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_populate_hotplug_cpu_dt(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 f1ebd85..4cd837e 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -28,4 +28,6 @@ typedef struct sPAPRCPUCore {
>  void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                           Error **errp);
>  char *spapr_get_cpu_core_type(const char *model);
> +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 --]

[Qemu-devel] [for-2.7 PATCH v3 10/15] 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 8659be0..cce7f3d 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -48,6 +48,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);
@@ -57,6 +69,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 9029d9e..a2daeae 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -113,8 +113,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);
 }
@@ -347,8 +349,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

Re: [Qemu-devel] [for-2.7 PATCH v3 10/15] xics, xics_kvm: Handle CPU unplug correctly

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:20AM +0530, Bharata B Rao wrote:
> 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>

This patch stands on its own, so I've merged it to the ppc-cpu-hotplug branch.

> ---
>  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 8659be0..cce7f3d 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -48,6 +48,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);
> @@ -57,6 +69,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 9029d9e..a2daeae 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -113,8 +113,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);
>  }
> @@ -347,8 +349,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__ */

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

[Qemu-devel] [for-2.7 PATCH v3 11/15] spapr_drc: Prevent detach racing against attach for CPU DR

[Bharata B Rao]

If a CPU is hot removed while hotplug of the same is still in progress,
the guest crashes. Prevent this by ensuring that detach is done only
after attach has completed.

The existing code already prevents such race for PCI hotplug. However
given that CPU is a logical DR unlike PCI and starts with ISOLATED
state, we need a logic that works for CPU too.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
               [Don't set awaiting_attach for PCI devices]
---
 hw/ppc/spapr_drc.c         | 12 ++++++++++++
 include/hw/ppc/spapr_drc.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1f5f1d7..e146267 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -140,6 +140,8 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
             DPRINTFN("finalizing device removal");
             drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
                          drc->detach_cb_opaque, NULL);
+        } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
+            drc->awaiting_allocation = false;
         }
     }
     return RTAS_OUT_SUCCESS;
@@ -376,6 +378,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
     drc->signalled = (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI)
                      ? true : coldplug;
 
+    if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
+        drc->awaiting_allocation = true;
+    }
+
     object_property_add_link(OBJECT(drc), "device",
                              object_get_typename(OBJECT(drc->dev)),
                              (Object **)(&drc->dev),
@@ -424,6 +430,12 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d,
         return;
     }
 
+    if (drc->awaiting_allocation) {
+        drc->awaiting_release = true;
+        DPRINTFN("awaiting allocation to complete before removal");
+        return;
+    }
+
     drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
 
     if (drc->detach_cb) {
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index fa21ba0..08e8411 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -152,6 +152,7 @@ typedef struct sPAPRDRConnector {
 
     bool awaiting_release;
     bool signalled;
+    bool awaiting_allocation;
 
     /* device pointer, via link property */
     DeviceState *dev;
-- 
2.1.0

Re: [Qemu-devel] [for-2.7 PATCH v3 11/15] spapr_drc: Prevent detach racing against attach for CPU DR

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:21AM +0530, Bharata B Rao wrote:
> If a CPU is hot removed while hotplug of the same is still in progress,
> the guest crashes. Prevent this by ensuring that detach is done only
> after attach has completed.
> 
> The existing code already prevents such race for PCI hotplug. However
> given that CPU is a logical DR unlike PCI and starts with ISOLATED
> state, we need a logic that works for CPU too.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>                [Don't set awaiting_attach for PCI devices]

Applied to ppc-cpu-hotplug branch.

> ---
>  hw/ppc/spapr_drc.c         | 12 ++++++++++++
>  include/hw/ppc/spapr_drc.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 1f5f1d7..e146267 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -140,6 +140,8 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
>              DPRINTFN("finalizing device removal");
>              drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
>                           drc->detach_cb_opaque, NULL);
> +        } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
> +            drc->awaiting_allocation = false;
>          }
>      }
>      return RTAS_OUT_SUCCESS;
> @@ -376,6 +378,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>      drc->signalled = (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI)
>                       ? true : coldplug;
>  
> +    if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> +        drc->awaiting_allocation = true;
> +    }
> +
>      object_property_add_link(OBJECT(drc), "device",
>                               object_get_typename(OBJECT(drc->dev)),
>                               (Object **)(&drc->dev),
> @@ -424,6 +430,12 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d,
>          return;
>      }
>  
> +    if (drc->awaiting_allocation) {
> +        drc->awaiting_release = true;
> +        DPRINTFN("awaiting allocation to complete before removal");
> +        return;
> +    }
> +
>      drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
>  
>      if (drc->detach_cb) {
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index fa21ba0..08e8411 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -152,6 +152,7 @@ typedef struct sPAPRDRConnector {
>  
>      bool awaiting_release;
>      bool signalled;
> +    bool awaiting_allocation;
>  
>      /* device pointer, via link property */
>      DeviceState *dev;

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

[Qemu-devel] [for-2.7 PATCH v3 12/15] 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                  | 16 ++++++++
 hw/ppc/spapr_cpu_core.c         | 81 +++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |  1 +
 include/hw/ppc/spapr_cpu_core.h | 11 ++++++
 4 files changed, 109 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8c3100d..a8cd74d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2342,11 +2342,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 ed36beb..0b62456 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -28,6 +28,87 @@ char *spapr_get_cpu_core_type(const char *model)
     return g_strdup(core_type);
 }
 
+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;
+}
+
+static void spapr_core_release(DeviceState *dev, void *opaque)
+{
+    struct sPAPRCPUUnplugList unplug_list;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
+    CPUCore *cc = CPU_CORE(dev);
+    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[cc->core / smt] = NULL;
+
+    g_free(core->threads);
+    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, NULL, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    spapr_hotplug_req_remove_by_index(drc);
+}
+
 void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                      Error **errp)
 {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index ca4ae3e..a443693 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -585,6 +585,7 @@ void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
 void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
 void *spapr_populate_hotplug_cpu_dt(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 4cd837e..3d8d6ac 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -30,4 +30,15 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 char *spapr_get_cpu_core_type(const char *model);
 void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                      Error **errp);
+void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                       Error **errp);
+
+/* 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] [for-2.7 PATCH v3 12/15] spapr: CPU hot unplug support

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:22AM +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>

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

Couple of suggestions for minor improvements below, though.

> ---
>  hw/ppc/spapr.c                  | 16 ++++++++
>  hw/ppc/spapr_cpu_core.c         | 81 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |  1 +
>  include/hw/ppc/spapr_cpu_core.h | 11 ++++++
>  4 files changed, 109 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 8c3100d..a8cd74d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2342,11 +2342,27 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>      }
>  }
>  
> +void spapr_cpu_destroy(PowerPCCPU *cpu)

I think this would make more sense in spapr_cpu_core.c (where it could
be static).

> +{
> +    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 ed36beb..0b62456 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -28,6 +28,87 @@ char *spapr_get_cpu_core_type(const char *model)
>      return g_strdup(core_type);
>  }
>  
> +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;
> +}
> +
> +static void spapr_core_release(DeviceState *dev, void *opaque)
> +{
> +    struct sPAPRCPUUnplugList unplug_list;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +    CPUCore *cc = CPU_CORE(dev);
> +    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[cc->core / smt] = NULL;

Could you avoid the awkwardness with the two phase release by stepping
through the cc->threads array instead of using object_child_foreach()?

> +
> +    g_free(core->threads);
> +    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, NULL, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    spapr_hotplug_req_remove_by_index(drc);
> +}
> +
>  void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                       Error **errp)
>  {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ca4ae3e..a443693 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -585,6 +585,7 @@ void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
>  void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
>  void *spapr_populate_hotplug_cpu_dt(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 4cd837e..3d8d6ac 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -30,4 +30,15 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  char *spapr_get_cpu_core_type(const char *model);
>  void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                       Error **errp);
> +void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                       Error **errp);
> +
> +/* 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 --]

[Qemu-devel] [for-2.7 PATCH v3 13/15] QMP: Add query-hotpluggable-cpus

[Bharata B Rao]

From: Igor Mammedov <imammedo@redhat.com>

It will allow mgmt to query present and hotpluggable CPU objects,
it is required from a target platform that wishes to support command
to implement and set MachineClass.query_hotpluggable_cpus callback,
which will return a list of possible CPU objects with options that
would be needed for hotplugging possible CPU objects.

There are:
'type': 'str' - QOM CPU object type for usage with device_add
'vcpus-count': 'int' - number of logical VCPU threads per
                        CPU object (mgmt needs to know)

and a set of optional fields that are to used for hotplugging a CPU
objects and would allows mgmt tools to know what/where it could be
hotplugged;
[node],[socket],[core],[thread]

For present CPUs there is a 'qom-path' field which would allow mgmt to
inspect whatever object/abstraction the target platform considers
as CPU object.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 include/hw/boards.h |  5 +++++
 monitor.c           | 13 +++++++++++++
 qapi-schema.json    | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx     | 23 ++++++++++++++++++++++
 4 files changed, 96 insertions(+)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 8d4fe56..0db0c1e 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -81,6 +81,10 @@ typedef struct {
  *    Returns an array of @CPUArchId architecture-dependent CPU IDs
  *    which includes CPU IDs for present and possible to hotplug CPUs.
  *    Caller is responsible for freeing returned list.
+ * @query_hotpluggable_cpus:
+ *    Returns a @HotpluggableCPUList, which describes CPUs objects which
+ *    could be added with -device/device_add.
+ *    Caller is responsible for freeing returned list.
  */
 struct MachineClass {
     /*< private >*/
@@ -124,6 +128,7 @@ struct MachineClass {
                                            DeviceState *dev);
     unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
     CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
+    HotpluggableCPUList *(*query_hotpluggable_cpus)(MachineState *machine);
 };
 
 /**
diff --git a/monitor.c b/monitor.c
index d1c1930..b469225 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4267,3 +4267,16 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
     return NULL;
 }
 #endif
+
+HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+    if (!mc->query_hotpluggable_cpus) {
+        error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
+        return NULL;
+    }
+
+    return mc->query_hotpluggable_cpus(ms);
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 54634c4..137e532 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4178,3 +4178,58 @@
 # Since: 2.6
 ##
 { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
+
+##
+# CpuInstanceProperties
+#
+# List of properties to be used for hotplugging a CPU instance,
+# it should be passed by management with device_add command when
+# a CPU is being hotplugged.
+#
+# Note: currently there are 4 properties that could be present
+# but management should be prepared to pass through other
+# properties with device_add command to allow for future
+# interface extension.
+#
+# @node: #optional NUMA node ID the CPU belongs to
+# @socket: #optional socket number within node/board the CPU belongs to
+# @core: #optional core number within socket the CPU belongs to
+# @thread: #optional thread number within core the CPU belongs to
+#
+# Since: 2.7
+##
+{ 'struct': 'CpuInstanceProperties',
+  'data': { '*node': 'int',
+            '*socket': 'int',
+            '*core': 'int',
+            '*thread': 'int'
+  }
+}
+
+##
+# @HotpluggableCPU
+#
+# @type: CPU object type for usage with device_add command
+# @props: list of properties to be used for hotplugging CPU
+# @vcpus-count: number of logical VCPU threads @HotpluggableCPU provides
+# @qom-path: #optional link to existing CPU object if CPU is present or
+#            omitted if CPU is not present.
+#
+# Since: 2.7
+##
+{ 'struct': 'HotpluggableCPU',
+  'data': { 'type': 'str',
+            'vcpus-count': 'int',
+            'props': 'CpuInstanceProperties',
+            '*qom-path': 'str'
+          }
+}
+
+##
+# @query-hotpluggable-cpus
+#
+# Returns: a list of HotpluggableCPU objects.
+#
+# Since: 2.7
+##
+{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index de896a5..5640147 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4880,3 +4880,26 @@ Example:
                 { "version": 3, "emulated": false, "kernel": true } ] }
 
 EQMP
+
+    {
+        .name       = "query-hotpluggable-cpus",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_query_hotpluggable_cpus,
+    },
+
+SQMP
+Show existing/possible CPUs
+---------------------------
+
+Arguments: None.
+
+Example for pseries machine type started with
+-smp 2,cores=2,maxcpus=4 -cpu POWER8:
+
+-> { "execute": "query-hotpluggable-cpus" }
+<- {"return": [
+     { "props": { "core": 8 }, "type": "POWER8-spapr-cpu-core",
+       "vcpus-count": 1 },
+     { "props": { "core": 0 }, "type": "POWER8-spapr-cpu-core",
+       "vcpus-count": 1, "qom-path": "/machine/unattached/device[0]"}
+   ]}'
-- 
2.1.0

Re: [Qemu-devel] [for-2.7 PATCH v3 13/15] QMP: Add query-hotpluggable-cpus

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:23AM +0530, Bharata B Rao wrote:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> It will allow mgmt to query present and hotpluggable CPU objects,
> it is required from a target platform that wishes to support command
> to implement and set MachineClass.query_hotpluggable_cpus callback,
> which will return a list of possible CPU objects with options that
> would be needed for hotplugging possible CPU objects.
> 
> There are:
> 'type': 'str' - QOM CPU object type for usage with device_add
> 'vcpus-count': 'int' - number of logical VCPU threads per
>                         CPU object (mgmt needs to know)
> 
> and a set of optional fields that are to used for hotplugging a CPU
> objects and would allows mgmt tools to know what/where it could be
> hotplugged;
> [node],[socket],[core],[thread]
> 
> For present CPUs there is a 'qom-path' field which would allow mgmt to
> inspect whatever object/abstraction the target platform considers
> as CPU object.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

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

Igor, if you're ready for this to go ahead, I'll take it through my tree.

> ---
>  include/hw/boards.h |  5 +++++
>  monitor.c           | 13 +++++++++++++
>  qapi-schema.json    | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx     | 23 ++++++++++++++++++++++
>  4 files changed, 96 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 8d4fe56..0db0c1e 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -81,6 +81,10 @@ typedef struct {
>   *    Returns an array of @CPUArchId architecture-dependent CPU IDs
>   *    which includes CPU IDs for present and possible to hotplug CPUs.
>   *    Caller is responsible for freeing returned list.
> + * @query_hotpluggable_cpus:
> + *    Returns a @HotpluggableCPUList, which describes CPUs objects which
> + *    could be added with -device/device_add.
> + *    Caller is responsible for freeing returned list.
>   */
>  struct MachineClass {
>      /*< private >*/
> @@ -124,6 +128,7 @@ struct MachineClass {
>                                             DeviceState *dev);
>      unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
>      CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
> +    HotpluggableCPUList *(*query_hotpluggable_cpus)(MachineState *machine);
>  };
>  
>  /**
> diff --git a/monitor.c b/monitor.c
> index d1c1930..b469225 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4267,3 +4267,16 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
>      return NULL;
>  }
>  #endif
> +
> +HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> +    if (!mc->query_hotpluggable_cpus) {
> +        error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
> +        return NULL;
> +    }
> +
> +    return mc->query_hotpluggable_cpus(ms);
> +}
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 54634c4..137e532 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4178,3 +4178,58 @@
>  # Since: 2.6
>  ##
>  { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
> +
> +##
> +# CpuInstanceProperties
> +#
> +# List of properties to be used for hotplugging a CPU instance,
> +# it should be passed by management with device_add command when
> +# a CPU is being hotplugged.
> +#
> +# Note: currently there are 4 properties that could be present
> +# but management should be prepared to pass through other
> +# properties with device_add command to allow for future
> +# interface extension.
> +#
> +# @node: #optional NUMA node ID the CPU belongs to
> +# @socket: #optional socket number within node/board the CPU belongs to
> +# @core: #optional core number within socket the CPU belongs to
> +# @thread: #optional thread number within core the CPU belongs to
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'CpuInstanceProperties',
> +  'data': { '*node': 'int',
> +            '*socket': 'int',
> +            '*core': 'int',
> +            '*thread': 'int'
> +  }
> +}
> +
> +##
> +# @HotpluggableCPU
> +#
> +# @type: CPU object type for usage with device_add command
> +# @props: list of properties to be used for hotplugging CPU
> +# @vcpus-count: number of logical VCPU threads @HotpluggableCPU provides
> +# @qom-path: #optional link to existing CPU object if CPU is present or
> +#            omitted if CPU is not present.
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'HotpluggableCPU',
> +  'data': { 'type': 'str',
> +            'vcpus-count': 'int',
> +            'props': 'CpuInstanceProperties',
> +            '*qom-path': 'str'
> +          }
> +}
> +
> +##
> +# @query-hotpluggable-cpus
> +#
> +# Returns: a list of HotpluggableCPU objects.
> +#
> +# Since: 2.7
> +##
> +{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index de896a5..5640147 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4880,3 +4880,26 @@ Example:
>                  { "version": 3, "emulated": false, "kernel": true } ] }
>  
>  EQMP
> +
> +    {
> +        .name       = "query-hotpluggable-cpus",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_query_hotpluggable_cpus,
> +    },
> +
> +SQMP
> +Show existing/possible CPUs
> +---------------------------
> +
> +Arguments: None.
> +
> +Example for pseries machine type started with
> +-smp 2,cores=2,maxcpus=4 -cpu POWER8:
> +
> +-> { "execute": "query-hotpluggable-cpus" }
> +<- {"return": [
> +     { "props": { "core": 8 }, "type": "POWER8-spapr-cpu-core",
> +       "vcpus-count": 1 },
> +     { "props": { "core": 0 }, "type": "POWER8-spapr-cpu-core",
> +       "vcpus-count": 1, "qom-path": "/machine/unattached/device[0]"}
> +   ]}'

-- 
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] [for-2.7 PATCH v3 13/15] QMP: Add query-hotpluggable-cpus

[Igor Mammedov]

On Mon, 6 Jun 2016 15:28:09 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, May 12, 2016 at 09:18:23AM +0530, Bharata B Rao wrote:
> > From: Igor Mammedov <imammedo@redhat.com>
> > 
> > It will allow mgmt to query present and hotpluggable CPU objects,
> > it is required from a target platform that wishes to support command
> > to implement and set MachineClass.query_hotpluggable_cpus callback,
> > which will return a list of possible CPU objects with options that
> > would be needed for hotplugging possible CPU objects.
> > 
> > There are:
> > 'type': 'str' - QOM CPU object type for usage with device_add
> > 'vcpus-count': 'int' - number of logical VCPU threads per
> >                         CPU object (mgmt needs to know)
> > 
> > and a set of optional fields that are to used for hotplugging a CPU
> > objects and would allows mgmt tools to know what/where it could be
> > hotplugged;
> > [node],[socket],[core],[thread]
> > 
> > For present CPUs there is a 'qom-path' field which would allow mgmt to
> > inspect whatever object/abstraction the target platform considers
> > as CPU object.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>  
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Igor, if you're ready for this to go ahead, I'll take it through my tree.
yep, it will allow me drop the same patch(es) from my ACPI CPU hotplug refactoring,
please take it through your tree.

> 
> > ---
> >  include/hw/boards.h |  5 +++++
> >  monitor.c           | 13 +++++++++++++
> >  qapi-schema.json    | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx     | 23 ++++++++++++++++++++++
> >  4 files changed, 96 insertions(+)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 8d4fe56..0db0c1e 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -81,6 +81,10 @@ typedef struct {
> >   *    Returns an array of @CPUArchId architecture-dependent CPU IDs
> >   *    which includes CPU IDs for present and possible to hotplug CPUs.
> >   *    Caller is responsible for freeing returned list.
> > + * @query_hotpluggable_cpus:
> > + *    Returns a @HotpluggableCPUList, which describes CPUs objects which
> > + *    could be added with -device/device_add.
> > + *    Caller is responsible for freeing returned list.
> >   */
> >  struct MachineClass {
> >      /*< private >*/
> > @@ -124,6 +128,7 @@ struct MachineClass {
> >                                             DeviceState *dev);
> >      unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> >      CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
> > +    HotpluggableCPUList *(*query_hotpluggable_cpus)(MachineState *machine);
> >  };
> >  
> >  /**
> > diff --git a/monitor.c b/monitor.c
> > index d1c1930..b469225 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4267,3 +4267,16 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
> >      return NULL;
> >  }
> >  #endif
> > +
> > +HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +
> > +    if (!mc->query_hotpluggable_cpus) {
> > +        error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
> > +        return NULL;
> > +    }
> > +
> > +    return mc->query_hotpluggable_cpus(ms);
> > +}
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 54634c4..137e532 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4178,3 +4178,58 @@
> >  # Since: 2.6
> >  ##
> >  { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
> > +
> > +##
> > +# CpuInstanceProperties
> > +#
> > +# List of properties to be used for hotplugging a CPU instance,
> > +# it should be passed by management with device_add command when
> > +# a CPU is being hotplugged.
> > +#
> > +# Note: currently there are 4 properties that could be present
> > +# but management should be prepared to pass through other
> > +# properties with device_add command to allow for future
> > +# interface extension.
> > +#
> > +# @node: #optional NUMA node ID the CPU belongs to
> > +# @socket: #optional socket number within node/board the CPU belongs to
> > +# @core: #optional core number within socket the CPU belongs to
> > +# @thread: #optional thread number within core the CPU belongs to
> > +#
> > +# Since: 2.7
> > +##
> > +{ 'struct': 'CpuInstanceProperties',
> > +  'data': { '*node': 'int',
> > +            '*socket': 'int',
> > +            '*core': 'int',
> > +            '*thread': 'int'
> > +  }
> > +}
> > +
> > +##
> > +# @HotpluggableCPU
> > +#
> > +# @type: CPU object type for usage with device_add command
> > +# @props: list of properties to be used for hotplugging CPU
> > +# @vcpus-count: number of logical VCPU threads @HotpluggableCPU provides
> > +# @qom-path: #optional link to existing CPU object if CPU is present or
> > +#            omitted if CPU is not present.
> > +#
> > +# Since: 2.7
> > +##
> > +{ 'struct': 'HotpluggableCPU',
> > +  'data': { 'type': 'str',
> > +            'vcpus-count': 'int',
> > +            'props': 'CpuInstanceProperties',
> > +            '*qom-path': 'str'
> > +          }
> > +}
> > +
> > +##
> > +# @query-hotpluggable-cpus
> > +#
> > +# Returns: a list of HotpluggableCPU objects.
> > +#
> > +# Since: 2.7
> > +##
> > +{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index de896a5..5640147 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -4880,3 +4880,26 @@ Example:
> >                  { "version": 3, "emulated": false, "kernel": true } ] }
> >  
> >  EQMP
> > +
> > +    {
> > +        .name       = "query-hotpluggable-cpus",
> > +        .args_type  = "",
> > +        .mhandler.cmd_new = qmp_marshal_query_hotpluggable_cpus,
> > +    },
> > +
> > +SQMP
> > +Show existing/possible CPUs
> > +---------------------------
> > +
> > +Arguments: None.
> > +
> > +Example for pseries machine type started with
> > +-smp 2,cores=2,maxcpus=4 -cpu POWER8:
> > +
> > +-> { "execute": "query-hotpluggable-cpus" }
> > +<- {"return": [
> > +     { "props": { "core": 8 }, "type": "POWER8-spapr-cpu-core",
> > +       "vcpus-count": 1 },
> > +     { "props": { "core": 0 }, "type": "POWER8-spapr-cpu-core",
> > +       "vcpus-count": 1, "qom-path": "/machine/unattached/device[0]"}
> > +   ]}'  
> 

[Qemu-devel] [for-2.7 PATCH v3 14/15] hmp: Add 'info hotpluggable-cpus' HMP command

[Bharata B Rao]

This is the HMP equivalent for QMP query-hotpluggable-cpus.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hmp-commands-info.hx | 14 ++++++++++++++
 hmp.c                | 41 +++++++++++++++++++++++++++++++++++++++++
 hmp.h                |  1 +
 3 files changed, 56 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 52539c3..7da9e6c 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -800,6 +800,20 @@ STEXI
 Display the latest dump status.
 ETEXI
 
+    {
+        .name       = "hotpluggable-cpus",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Show information about hotpluggable CPUs",
+        .mhandler.cmd = hmp_hotpluggable_cpus,
+    },
+
+STEXI
+@item info hotpluggable-cpus
+@findex hotpluggable-cpus
+Show information about hotpluggable CPUs
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index d510236..6480ae7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2381,3 +2381,44 @@ void hmp_info_dump(Monitor *mon, const QDict *qdict)
 
     qapi_free_DumpQueryResult(result);
 }
+
+void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    HotpluggableCPUList *l = qmp_query_hotpluggable_cpus(&err);
+    HotpluggableCPUList *saved = l;
+    CpuInstanceProperties *c;
+
+    if (err != NULL) {
+        hmp_handle_error(mon, &err);
+        return;
+    }
+
+    monitor_printf(mon, "Hotpluggable CPUs:\n");
+    while (l) {
+        monitor_printf(mon, "  type: \"%s\"\n", l->value->type);
+        monitor_printf(mon, "  vcpus_count: \"%ld\"\n", l->value->vcpus_count);
+        if (l->value->has_qom_path) {
+            monitor_printf(mon, "  qom_path: \"%s\"\n", l->value->qom_path);
+        }
+
+        c = l->value->props;
+        monitor_printf(mon, "  CPUInstance Properties:\n");
+        if (c->has_node) {
+            monitor_printf(mon, "    node: \"%ld\"\n", c->node);
+        }
+        if (c->has_socket) {
+            monitor_printf(mon, "    socket: \"%ld\"\n", c->socket);
+        }
+        if (c->has_core) {
+            monitor_printf(mon, "    core: \"%ld\"\n", c->core);
+        }
+        if (c->has_thread) {
+            monitor_printf(mon, "    thread: \"%ld\"\n", c->thread);
+        }
+
+        l = l->next;
+    }
+
+    qapi_free_HotpluggableCPUList(saved);
+}
diff --git a/hmp.h b/hmp.h
index 093d65f..f5d9749 100644
--- a/hmp.h
+++ b/hmp.h
@@ -132,5 +132,6 @@ void hmp_rocker_ports(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
 void hmp_info_dump(Monitor *mon, const QDict *qdict);
+void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
 
 #endif
-- 
2.1.0

Re: [Qemu-devel] [for-2.7 PATCH v3 14/15] hmp: Add 'info hotpluggable-cpus' HMP command

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:24AM +0530, Bharata B Rao wrote:
> This is the HMP equivalent for QMP query-hotpluggable-cpus.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

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

> ---
>  hmp-commands-info.hx | 14 ++++++++++++++
>  hmp.c                | 41 +++++++++++++++++++++++++++++++++++++++++
>  hmp.h                |  1 +
>  3 files changed, 56 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 52539c3..7da9e6c 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -800,6 +800,20 @@ STEXI
>  Display the latest dump status.
>  ETEXI
>  
> +    {
> +        .name       = "hotpluggable-cpus",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "Show information about hotpluggable CPUs",
> +        .mhandler.cmd = hmp_hotpluggable_cpus,
> +    },
> +
> +STEXI
> +@item info hotpluggable-cpus
> +@findex hotpluggable-cpus
> +Show information about hotpluggable CPUs
> +ETEXI
> +
>  STEXI
>  @end table
>  ETEXI
> diff --git a/hmp.c b/hmp.c
> index d510236..6480ae7 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2381,3 +2381,44 @@ void hmp_info_dump(Monitor *mon, const QDict *qdict)
>  
>      qapi_free_DumpQueryResult(result);
>  }
> +
> +void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    HotpluggableCPUList *l = qmp_query_hotpluggable_cpus(&err);
> +    HotpluggableCPUList *saved = l;
> +    CpuInstanceProperties *c;
> +
> +    if (err != NULL) {
> +        hmp_handle_error(mon, &err);
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Hotpluggable CPUs:\n");
> +    while (l) {
> +        monitor_printf(mon, "  type: \"%s\"\n", l->value->type);
> +        monitor_printf(mon, "  vcpus_count: \"%ld\"\n", l->value->vcpus_count);
> +        if (l->value->has_qom_path) {
> +            monitor_printf(mon, "  qom_path: \"%s\"\n", l->value->qom_path);
> +        }
> +
> +        c = l->value->props;
> +        monitor_printf(mon, "  CPUInstance Properties:\n");
> +        if (c->has_node) {
> +            monitor_printf(mon, "    node: \"%ld\"\n", c->node);
> +        }
> +        if (c->has_socket) {
> +            monitor_printf(mon, "    socket: \"%ld\"\n", c->socket);
> +        }
> +        if (c->has_core) {
> +            monitor_printf(mon, "    core: \"%ld\"\n", c->core);
> +        }
> +        if (c->has_thread) {
> +            monitor_printf(mon, "    thread: \"%ld\"\n", c->thread);
> +        }
> +
> +        l = l->next;
> +    }
> +
> +    qapi_free_HotpluggableCPUList(saved);
> +}
> diff --git a/hmp.h b/hmp.h
> index 093d65f..f5d9749 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -132,5 +132,6 @@ void hmp_rocker_ports(Monitor *mon, const QDict *qdict);
>  void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
>  void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
>  void hmp_info_dump(Monitor *mon, const QDict *qdict);
> +void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
>  
>  #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 --]

[Qemu-devel] [for-2.7 PATCH v3 15/15] spapr: implement query-hotpluggable-cpus callback

[Bharata B Rao]

From: Igor Mammedov <imammedo@redhat.com>

It returns a list of present/possible to hotplug CPU
objects with a list of properties to use with
device_add.

in spapr case returned list would looks like:
-> { "execute": "query-hotpluggable-cpus" }
<- {"return": [
     { "props": { "core": 8 }, "type": "POWER8-spapr-cpu-core",
       "vcpus-count": 2 },
     { "props": { "core": 0 }, "type": "POWER8-spapr-cpu-core",
       "vcpus-count": 2,
       "qom-path": "/machine/unattached/device[0]"}
   ]}'

TODO:
  add 'node' property for core <-> numa node mapping

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a8cd74d..ae95580 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -66,6 +66,7 @@
 #include "hw/compat.h"
 #include "qemu/cutils.h"
 #include "hw/ppc/spapr_cpu_core.h"
+#include "qmp-commands.h"
 
 #include <libfdt.h>
 
@@ -2391,6 +2392,38 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
     return cpu_index / smp_threads / smp_cores;
 }
 
+static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
+{
+    int i;
+    HotpluggableCPUList *head = NULL;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+    int spapr_max_cores = max_cpus / smp_threads;
+    int smt = kvmppc_smt_threads();
+
+    for (i = 0; i < spapr_max_cores; i++) {
+        HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
+        HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
+        CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
+
+        cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
+        cpu_item->vcpus_count = smp_threads;
+        cpu_props->has_core = true;
+        cpu_props->core = i * smt;
+        /* TODO: add 'has_node/node' here to describe
+           to which node core belongs */
+
+        cpu_item->props = cpu_props;
+        if (spapr->cores[i]) {
+            cpu_item->has_qom_path = true;
+            cpu_item->qom_path = object_get_canonical_path(spapr->cores[i]);
+        }
+        list_item->value = cpu_item;
+        list_item->next = head;
+        head = list_item;
+    }
+    return head;
+}
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -2421,6 +2454,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     hc->plug = spapr_machine_device_plug;
     hc->unplug = spapr_machine_device_unplug;
     mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
+    mc->query_hotpluggable_cpus = spapr_query_hotpluggable_cpus;
 
     smc->dr_lmb_enabled = true;
     smc->dr_cpu_enabled = true;
-- 
2.1.0

Re: [Qemu-devel] [for-2.7 PATCH v3 15/15] spapr: implement query-hotpluggable-cpus callback

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:25AM +0530, Bharata B Rao wrote:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> It returns a list of present/possible to hotplug CPU
> objects with a list of properties to use with
> device_add.
> 
> in spapr case returned list would looks like:
> -> { "execute": "query-hotpluggable-cpus" }
> <- {"return": [
>      { "props": { "core": 8 }, "type": "POWER8-spapr-cpu-core",
>        "vcpus-count": 2 },
>      { "props": { "core": 0 }, "type": "POWER8-spapr-cpu-core",
>        "vcpus-count": 2,
>        "qom-path": "/machine/unattached/device[0]"}
>    ]}'
> 
> TODO:
>   add 'node' property for core <-> numa node mapping
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a8cd74d..ae95580 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -66,6 +66,7 @@
>  #include "hw/compat.h"
>  #include "qemu/cutils.h"
>  #include "hw/ppc/spapr_cpu_core.h"
> +#include "qmp-commands.h"
>  
>  #include <libfdt.h>
>  
> @@ -2391,6 +2392,38 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
>      return cpu_index / smp_threads / smp_cores;
>  }
>  
> +static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
> +{
> +    int i;
> +    HotpluggableCPUList *head = NULL;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +    int spapr_max_cores = max_cpus / smp_threads;
> +    int smt = kvmppc_smt_threads();
> +
> +    for (i = 0; i < spapr_max_cores; i++) {
> +        HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
> +        HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
> +        CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
> +
> +        cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
> +        cpu_item->vcpus_count = smp_threads;
> +        cpu_props->has_core = true;
> +        cpu_props->core = i * smt;

I'm thinking we should probably use the existing vcpu_dt_id logic
here, to make sure we don't change things if we ever alter that.

> +        /* TODO: add 'has_node/node' here to describe
> +           to which node core belongs */
> +
> +        cpu_item->props = cpu_props;
> +        if (spapr->cores[i]) {
> +            cpu_item->has_qom_path = true;
> +            cpu_item->qom_path = object_get_canonical_path(spapr->cores[i]);
> +        }
> +        list_item->value = cpu_item;
> +        list_item->next = head;
> +        head = list_item;
> +    }
> +    return head;
> +}
> +
>  static void spapr_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -2421,6 +2454,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      hc->plug = spapr_machine_device_plug;
>      hc->unplug = spapr_machine_device_unplug;
>      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> +    mc->query_hotpluggable_cpus = spapr_query_hotpluggable_cpus;
>  
>      smc->dr_lmb_enabled = true;
>      smc->dr_cpu_enabled = true;

-- 
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] [for-2.7 PATCH v3 00/15] Core based CPU hotplug for PowerPC sPAPR

[Thomas Huth]

On 12.05.2016 05:48, Bharata B Rao wrote:
> Hi,
> 
> This is v3 of "Core based CPU hotplug for PowerPC sPAPR". The hotplug
> semantics looks like this:
> 
> (qemu) device_add POWER8E-spapr-cpu-core,id=core2,core=16[,threads=4]
> (qemu) device_add POWER8E_v2.1-spapr-cpu-core,id=core2,core=16[,threads=4]

*ping*

Andreas, could you please, please review this series? At least the
generic patches (patch 1-6 and patch 13-15)?

Thanks!

  Thomas

Re: [Qemu-devel] [for-2.7 PATCH v3 00/15] Core based CPU hotplug for PowerPC sPAPR

[Andreas Färber]

Am 25.05.2016 um 08:54 schrieb Thomas Huth:
> On 12.05.2016 05:48, Bharata B Rao wrote:
>> Hi,
>>
>> This is v3 of "Core based CPU hotplug for PowerPC sPAPR". The hotplug
>> semantics looks like this:
>>
>> (qemu) device_add POWER8E-spapr-cpu-core,id=core2,core=16[,threads=4]
>> (qemu) device_add POWER8E_v2.1-spapr-cpu-core,id=core2,core=16[,threads=4]
> 
> *ping*
> 
> Andreas, could you please, please review this series? At least the
> generic patches (patch 1-6 and patch 13-15)?

No. (And I still don't see any qtest additions in the diffstat.)

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [for-2.7 PATCH v3 00/15] Core based CPU hotplug for PowerPC sPAPR

[David Gibson]

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

On Thu, May 12, 2016 at 09:18:10AM +0530, Bharata B Rao wrote:
> Hi,
> 
> This is v3 of "Core based CPU hotplug for PowerPC sPAPR". The hotplug
> semantics looks like this:
> 
> (qemu) device_add POWER8E-spapr-cpu-core,id=core2,core=16[,threads=4]
> (qemu) device_add POWER8E_v2.1-spapr-cpu-core,id=core2,core=16[,threads=4]
> 
> Changes in v3
> -------------
> - Moved CPU ObjectClass pointer from sPAPR specific CPU core type to
>   its parent type, the abstract sPAPR CPU core type. This largely reduces
>   the use of macros.
> - Including Igor's QMP query-hotpluggable-cpus patches in this series.
> - Added HMP version for query-hotpluggable-cpus.
> - Added a patch to prevent QEMU crash due to DRC detach racing against attach.
> - Addressed miscellaneous review comments from previous post.
> 
> v2.1: https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00649.html
> 
> Bharata B Rao (11):
>   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: Abstract CPU core device and type specific core devices
>   spapr: convert boot CPUs into CPU core devices
>   spapr: CPU hotplug support
>   xics,xics_kvm: Handle CPU unplug correctly
>   spapr_drc: Prevent detach racing against attach for CPU DR
>   spapr: CPU hot unplug support
>   hmp: Add 'info hotpluggable-cpus' HMP command

Paolo,

Patches 1-4 of this series stand on their own, are AFAICT in your
bailiwick, and I think they're ready to go.  Are you ready to merge
these yet?


Igor,

Patches 5-6 are not ppc specific.  Do you think they're ready to go as
a first step towards new cpu hotplug infrastructure?

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