[Qemu-devel] [PATCH v7 00/13] sPAPR CPU hotplug

[Qemu-devel] [PATCH v7 00/13] sPAPR CPU hotplug

[Bharata B Rao]

Hi,

This is the 7th iteration of patchset that introduces CPU hotplug for
PowerPC sPAPR guests using device_add/device_del commands.

(qemu) device_add powerpc64-cpu-core,id=core1

The main change in this version is about adding "info ppc-cpu-cores"
QMP/HMP support to obtain information about PowerPC CPU cores.

The first 6 patches are generic changes.

1/13  machine: Don't allow CPU toplogies with partially filled cores
2/13  exec: Remove cpu from cpus list during cpu_exec_exit()
3/13  exec: Do vmstate unregistration from cpu_exec_exit()
4/13  cpu: Don't realize CPU from cpu_generic_init()

Above 4 patches can stand on their own and probably can be pushed
ahead of actual hotplug patches when found ready. Let me know if I
should pursue these in a separate patchset.

Out of the above 4, last three (2/13, 3/13, 4/13) are required by
s390 and have been posted in their CPU hotplug patchset.

5/13  cpu: Reclaim vCPU objects

Above patch is needed by x86 as well as s390 and has been posted in their
respective CPU hotplug patchsets.

6/13  cpu: Add a sync version of cpu_remove()

Above patch is needed by s390 and has been posted in their CPU hotplug
patchset.

The remaining patches are ppc/spapr specific. This patchset applies
on top of ppc-for-2.6 branch of David Gibson's tree.

Changes in v7
-------------
- Added two patches (12/13, 13/13) to obtain information about
  ppc cpu cores via QMP/HMP.
- Don't populate MachineClass::validate_smp_config() for TYPE_MACINE
  so that all archs are not affected and don't have to explicitly
  disable the enforcement of partially filled cores requirement
  as per Eduardo Habkost's suggestion. (01/13)
- Some minor code cleanups in 10/13 as per David Gibson's suggestion.
- Store the first thread representing the core in PowerPCCPUCore so
  that it DRC index of the core can be easily found during DRC
  attach (08/13, 11/13).
- Make unplug_list local to core release routine as per David (11/13).

v6: http://lists.gnu.org/archive/html/qemu-ppc/2016-01/msg00060.html

Bharata B Rao (12):
  machine: Don't allow CPU toplogies with partially filled cores
  exec: Remove cpu from cpus list during cpu_exec_exit()
  exec: Do vmstate unregistration from cpu_exec_exit()
  cpu: Don't realize CPU from cpu_generic_init()
  cpu: Add a sync version of cpu_remove()
  xics,xics_kvm: Handle CPU unplug correctly
  target-ppc: Introduce PowerPC specific CPU core device
  spapr: Enable CPU hotplug for pseries-2.6 and add CPU DRC DT entries
  spapr: CPU hotplug support
  spapr: CPU hot unplug support
  qmp: Add query-ppc-cpu-cores command
  hmp: Add "info ppc-cpu-cores" command

Gu Zheng (1):
  cpu: Reclaim vCPU objects

 cpus.c                          |  50 ++++++++
 exec.c                          |  30 +++++
 hmp-commands-info.hx            |  16 +++
 hmp.c                           |  31 +++++
 hmp.h                           |   1 +
 hw/core/machine.c               |  23 ++++
 hw/i386/pc.c                    |   1 +
 hw/i386/pc_piix.c               |   1 +
 hw/i386/pc_q35.c                |   1 +
 hw/intc/xics.c                  |  14 +++
 hw/intc/xics_kvm.c              |   8 +-
 hw/ppc/Makefile.objs            |   1 +
 hw/ppc/cpu-core.c               | 152 +++++++++++++++++++++++
 hw/ppc/spapr.c                  | 260 ++++++++++++++++++++++++++++++++++++++--
 hw/ppc/spapr_events.c           |   3 +
 hw/ppc/spapr_rtas.c             |  24 ++++
 include/hw/boards.h             |   4 +
 include/hw/ppc/cpu-core.h       |  33 +++++
 include/hw/ppc/spapr.h          |   9 ++
 include/hw/ppc/xics.h           |   1 +
 include/qom/cpu.h               |  18 +++
 include/sysemu/kvm.h            |   1 +
 kvm-all.c                       |  57 ++++++++-
 kvm-stub.c                      |   5 +
 qapi-schema.json                |  31 +++++
 qmp-commands.hx                 |  51 ++++++++
 qom/cpu.c                       |   6 -
 stubs/Makefile.objs             |   1 +
 stubs/qmp_query_ppc_cpu_cores.c |  10 ++
 target-arm/helper.c             |  16 ++-
 target-cris/cpu.c               |  16 ++-
 target-lm32/helper.c            |  16 ++-
 target-moxie/cpu.c              |  16 ++-
 target-openrisc/cpu.c           |  16 ++-
 target-ppc/translate_init.c     |  24 +++-
 target-sh4/cpu.c                |  16 ++-
 target-tricore/helper.c         |  16 ++-
 target-unicore32/helper.c       |  16 ++-
 vl.c                            |   5 +
 39 files changed, 973 insertions(+), 27 deletions(-)
 create mode 100644 hw/ppc/cpu-core.c
 create mode 100644 include/hw/ppc/cpu-core.h
 create mode 100644 stubs/qmp_query_ppc_cpu_cores.c

-- 
2.1.0

[Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[Bharata B Rao]

Prevent guests from booting with CPU topologies that have partially
filled CPU cores or can result in partially filled CPU cores after
CPU hotplug like

-smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
-smp 15,sockets=1,cores=4,threads=4,maxcpus=17.

This is enforced by introducing MachineClass::validate_smp_config()
that gets called from generic SMP parsing code. Machine type versions
that want to enforce this can define this to the generic version
provided.

Only sPAPR and PC machine types starting from version 2.6 enforce this in
this patch.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/core/machine.c   | 23 +++++++++++++++++++++++
 hw/i386/pc.c        |  1 +
 hw/i386/pc_piix.c   |  1 +
 hw/i386/pc_q35.c    |  1 +
 hw/ppc/spapr.c      |  2 ++
 include/hw/boards.h |  4 ++++
 vl.c                |  5 +++++
 7 files changed, 37 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c46ddc7..4505995 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -336,6 +336,29 @@ static void machine_init_notify(Notifier *notifier, void *data)
     foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
 }
 
+/*
+ * Machine types that want to prevent starting of guests with
+ * partially filled CPU cores can use this routine as their
+ * MachineClass:validate_smp_config().
+ */
+void validate_smp_config_generic(int smp_cpus, int max_cpus,
+                                 int smp_threads, Error **errp)
+{
+    if (smp_cpus % smp_threads) {
+        error_setg(errp, "cpu topology: "
+                   "smp_cpus (%u) should be multiple of threads (%u) ",
+                   smp_cpus, smp_threads);
+        return;
+    }
+
+    if (max_cpus % smp_threads) {
+        error_setg(errp, "cpu topology: "
+                   "max_cpus (%u) should be multiple of threads (%u) ",
+                   max_cpus, smp_threads);
+        return;
+    }
+}
+
 static void machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 78cf8fa..a54e0a0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1971,6 +1971,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->hot_add_cpu = pc_hot_add_cpu;
     mc->max_cpus = 255;
     mc->reset = pc_machine_reset;
+    mc->validate_smp_config = validate_smp_config_generic;
     hc->plug = pc_machine_device_plug_cb;
     hc->unplug_request = pc_machine_device_unplug_request_cb;
     hc->unplug = pc_machine_device_unplug_cb;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index bc74557..98b8b69 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -435,6 +435,7 @@ static void pc_i440fx_2_5_machine_options(MachineClass *m)
     pc_i440fx_2_6_machine_options(m);
     m->alias = NULL;
     m->is_default = 0;
+    m->validate_smp_config = NULL;
     pcmc->save_tsc_khz = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6128b02..c5f4935 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -362,6 +362,7 @@ static void pc_q35_2_5_machine_options(MachineClass *m)
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_2_6_machine_options(m);
     m->alias = NULL;
+    m->validate_smp_config = NULL;
     pcmc->save_tsc_khz = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
 }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a9c9a95..6ac9f06 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2317,6 +2317,7 @@ 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;
+    mc->validate_smp_config = validate_smp_config_generic;
     hc->plug = spapr_machine_device_plug;
     hc->unplug = spapr_machine_device_unplug;
     mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
@@ -2402,6 +2403,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
     spapr_machine_2_6_class_options(mc);
     smc->use_ohci_by_default = true;
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
+    mc->validate_smp_config = NULL;
 }
 
 DEFINE_SPAPR_MACHINE(2_5, "2.5", false);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 0f30959..435c339 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -40,6 +40,8 @@ int machine_kvm_shadow_mem(MachineState *machine);
 int machine_phandle_start(MachineState *machine);
 bool machine_dump_guest_core(MachineState *machine);
 bool machine_mem_merge(MachineState *machine);
+void validate_smp_config_generic(int smp_cpus, int max_cpus,
+                                 int smp_threads, Error **errp);
 
 /**
  * MachineClass:
@@ -99,6 +101,8 @@ struct MachineClass {
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
     unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
+    void (*validate_smp_config)(int smp_cpus, int max_cpus, int smp_threads,
+                                Error **errp);
 };
 
 /**
diff --git a/vl.c b/vl.c
index f043009..9e4da46 100644
--- a/vl.c
+++ b/vl.c
@@ -4126,6 +4126,11 @@ int main(int argc, char **argv, char **envp)
 
     smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
 
+    if (machine_class->validate_smp_config) {
+        machine_class->validate_smp_config(smp_cpus, max_cpus, smp_threads,
+                                           &error_abort);
+    }
+
     machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
     if (max_cpus > machine_class->max_cpus) {
         error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
-- 
2.1.0

[Qemu-devel] [PATCH v7 02/13] 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_init() is called from generic CPU::instance_finalize and some
archs like PowerPC call it from CPU unrealizefn. So ensure that we
dequeue the cpu only once.

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

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

diff --git a/exec.c b/exec.c
index 7115403..c8da9d4 100644
--- a/exec.c
+++ b/exec.c
@@ -596,6 +596,7 @@ void cpu_exec_exit(CPUState *cpu)
         return;
     }
 
+    QTAILQ_REMOVE(&cpus, cpu, node);
     bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
     cpu->cpu_index = -1;
 }
@@ -614,6 +615,15 @@ static int cpu_get_free_index(Error **errp)
 
 void cpu_exec_exit(CPUState *cpu)
 {
+    cpu_list_lock();
+    if (cpu->cpu_index == -1) {
+        cpu_list_unlock();
+        return;
+    }
+
+    QTAILQ_REMOVE(&cpus, cpu, node);
+    cpu->cpu_index = -1;
+    cpu_list_unlock();
 }
 #endif
 
-- 
2.1.0

[Qemu-devel] [PATCH v7 03/13] exec: Do vmstate unregistration from cpu_exec_exit()

[Bharata B Rao]

cpu_exec_init() does vmstate_register and register_savevm for the CPU device.
These need to be undone from cpu_exec_exit(). These changes are needed to
support CPU hot removal and also to correctly fail hotplug attempts
beyond max_cpus.

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

diff --git a/exec.c b/exec.c
index c8da9d4..aa41032 100644
--- a/exec.c
+++ b/exec.c
@@ -591,6 +591,8 @@ static int cpu_get_free_index(Error **errp)
 
 void cpu_exec_exit(CPUState *cpu)
 {
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
     if (cpu->cpu_index == -1) {
         /* cpu_index was never allocated by this @cpu or was already freed. */
         return;
@@ -599,6 +601,15 @@ void cpu_exec_exit(CPUState *cpu)
     QTAILQ_REMOVE(&cpus, cpu, node);
     bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
     cpu->cpu_index = -1;
+    if (cc->vmsd != NULL) {
+        vmstate_unregister(NULL, cc->vmsd, cpu);
+    }
+#if defined(CPU_SAVE_VERSION)
+    unregister_savevm(NULL, "cpu", cpu->env_ptr);
+#endif
+    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
+        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
+    }
 }
 #else
 
@@ -615,6 +626,8 @@ static int cpu_get_free_index(Error **errp)
 
 void cpu_exec_exit(CPUState *cpu)
 {
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
     cpu_list_lock();
     if (cpu->cpu_index == -1) {
         cpu_list_unlock();
@@ -624,6 +637,13 @@ void cpu_exec_exit(CPUState *cpu)
     QTAILQ_REMOVE(&cpus, cpu, node);
     cpu->cpu_index = -1;
     cpu_list_unlock();
+
+    if (cc->vmsd != NULL) {
+        vmstate_unregister(NULL, cc->vmsd, cpu);
+    }
+    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
+        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
+    }
 }
 #endif
 
-- 
2.1.0

[Qemu-devel] [PATCH v7 04/13] cpu: Don't realize CPU from cpu_generic_init()

[Bharata B Rao]

Don't do CPU realization from cpu_generic_init(). With this
cpu_generic_init() will be used to just create CPU threads and they
should be realized separately from realizefn call.

Convert the existing callers to do explicit realization.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qom/cpu.c                   |  6 ------
 target-arm/helper.c         | 16 +++++++++++++++-
 target-cris/cpu.c           | 16 +++++++++++++++-
 target-lm32/helper.c        | 16 +++++++++++++++-
 target-moxie/cpu.c          | 16 +++++++++++++++-
 target-openrisc/cpu.c       | 16 +++++++++++++++-
 target-ppc/translate_init.c | 16 +++++++++++++++-
 target-sh4/cpu.c            | 16 +++++++++++++++-
 target-tricore/helper.c     | 16 +++++++++++++++-
 target-unicore32/helper.c   | 16 +++++++++++++++-
 10 files changed, 135 insertions(+), 15 deletions(-)

diff --git a/qom/cpu.c b/qom/cpu.c
index 8f537a4..01fd776 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -63,13 +63,7 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
     featurestr = strtok(NULL, ",");
     cc->parse_features(cpu, featurestr, &err);
     g_free(str);
-    if (err != NULL) {
-        goto out;
-    }
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
 
-out:
     if (err != NULL) {
         error_report_err(err);
         object_unref(OBJECT(cpu));
diff --git a/target-arm/helper.c b/target-arm/helper.c
index ae02486..4a46cdb 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4564,7 +4564,21 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 
 ARMCPU *cpu_arm_init(const char *cpu_model)
 {
-    return ARM_CPU(cpu_generic_init(TYPE_ARM_CPU, cpu_model));
+    CPUState *cpu = cpu_generic_init(TYPE_ARM_CPU, cpu_model);
+    Error *err = NULL;
+
+    if (!cpu) {
+        return NULL;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    } else {
+        return ARM_CPU(cpu);
+    }
 }
 
 void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
diff --git a/target-cris/cpu.c b/target-cris/cpu.c
index 8eaf5a5..d2c0822 100644
--- a/target-cris/cpu.c
+++ b/target-cris/cpu.c
@@ -89,7 +89,21 @@ static ObjectClass *cris_cpu_class_by_name(const char *cpu_model)
 
 CRISCPU *cpu_cris_init(const char *cpu_model)
 {
-    return CRIS_CPU(cpu_generic_init(TYPE_CRIS_CPU, cpu_model));
+    CPUState *cpu = cpu_generic_init(TYPE_CRIS_CPU, cpu_model);
+    Error *err = NULL;
+
+    if (!cpu) {
+        return NULL;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    } else {
+        return CRIS_CPU(cpu);
+    }
 }
 
 /* Sort alphabetically by VR. */
diff --git a/target-lm32/helper.c b/target-lm32/helper.c
index e26c133..49ac960 100644
--- a/target-lm32/helper.c
+++ b/target-lm32/helper.c
@@ -218,7 +218,21 @@ bool lm32_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 
 LM32CPU *cpu_lm32_init(const char *cpu_model)
 {
-    return LM32_CPU(cpu_generic_init(TYPE_LM32_CPU, cpu_model));
+    CPUState *cpu = cpu_generic_init(TYPE_LM32_CPU, cpu_model);
+    Error *err = NULL;
+
+    if (!cpu) {
+        return NULL;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    } else {
+        return LM32_CPU(cpu);
+    }
 }
 
 /* Some soc ignores the MSB on the address bus. Thus creating a shadow memory
diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
index 0c60c65..5989fa6 100644
--- a/target-moxie/cpu.c
+++ b/target-moxie/cpu.c
@@ -152,7 +152,21 @@ static const MoxieCPUInfo moxie_cpus[] = {
 
 MoxieCPU *cpu_moxie_init(const char *cpu_model)
 {
-    return MOXIE_CPU(cpu_generic_init(TYPE_MOXIE_CPU, cpu_model));
+    CPUState *cpu = cpu_generic_init(TYPE_MOXIE_CPU, cpu_model);
+    Error *err = NULL;
+
+    if (!cpu) {
+        return NULL;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    } else {
+        return MOXIE_CPU(cpu);
+    }
 }
 
 static void cpu_register(const MoxieCPUInfo *info)
diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
index cc5e2d1..873eafb 100644
--- a/target-openrisc/cpu.c
+++ b/target-openrisc/cpu.c
@@ -222,7 +222,21 @@ static void openrisc_cpu_register_types(void)
 
 OpenRISCCPU *cpu_openrisc_init(const char *cpu_model)
 {
-    return OPENRISC_CPU(cpu_generic_init(TYPE_OPENRISC_CPU, cpu_model));
+    CPUState *cpu = cpu_generic_init(TYPE_OPENRISC_CPU, cpu_model);
+    Error *err = NULL;
+
+    if (!cpu) {
+        return NULL;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    } else {
+        return OPENRISC_CPU(cpu);
+    }
 }
 
 /* Sort alphabetically by type name, except for "any". */
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index f6babd2..3304ad1 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9428,7 +9428,21 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name)
 
 PowerPCCPU *cpu_ppc_init(const char *cpu_model)
 {
-    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
+    CPUState *cpu = cpu_generic_init(TYPE_POWERPC_CPU, cpu_model);
+    Error *err = NULL;
+
+    if (!cpu) {
+        return NULL;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    } else {
+        return POWERPC_CPU(cpu);
+    }
 }
 
 /* Sort by PVR, ordering special case "host" last. */
diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
index d7e2fbd..e5151a0 100644
--- a/target-sh4/cpu.c
+++ b/target-sh4/cpu.c
@@ -155,7 +155,21 @@ static ObjectClass *superh_cpu_class_by_name(const char *cpu_model)
 
 SuperHCPU *cpu_sh4_init(const char *cpu_model)
 {
-    return SUPERH_CPU(cpu_generic_init(TYPE_SUPERH_CPU, cpu_model));
+    CPUState *cpu = cpu_generic_init(TYPE_SUPERH_CPU, cpu_model);
+    Error *err = NULL;
+
+    if (!cpu) {
+        return NULL;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    } else {
+        return SUPERH_CPU(cpu);
+    }
 }
 
 static void sh7750r_cpu_initfn(Object *obj)
diff --git a/target-tricore/helper.c b/target-tricore/helper.c
index 1b70429..bf164fc 100644
--- a/target-tricore/helper.c
+++ b/target-tricore/helper.c
@@ -83,7 +83,21 @@ int cpu_tricore_handle_mmu_fault(CPUState *cs, target_ulong address,
 
 TriCoreCPU *cpu_tricore_init(const char *cpu_model)
 {
-    return TRICORE_CPU(cpu_generic_init(TYPE_TRICORE_CPU, cpu_model));
+    CPUState *cpu = cpu_generic_init(TYPE_TRICORE_CPU, cpu_model);
+    Error *err = NULL;
+
+    if (!cpu) {
+        return NULL;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    } else {
+        return TRICORE_CPU(cpu);
+    }
 }
 
 static void tricore_cpu_list_entry(gpointer data, gpointer user_data)
diff --git a/target-unicore32/helper.c b/target-unicore32/helper.c
index ae63277..e47bb12 100644
--- a/target-unicore32/helper.c
+++ b/target-unicore32/helper.c
@@ -27,7 +27,21 @@
 
 UniCore32CPU *uc32_cpu_init(const char *cpu_model)
 {
-    return UNICORE32_CPU(cpu_generic_init(TYPE_UNICORE32_CPU, cpu_model));
+    CPUState *cpu = cpu_generic_init(TYPE_UNICORE32_CPU, cpu_model);
+    Error *err = NULL;
+
+    if (!cpu) {
+        return NULL;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    } else {
+        return UNICORE32_CPU(cpu);
+    }
 }
 
 uint32_t HELPER(clo)(uint32_t x)
-- 
2.1.0

[Qemu-devel] [PATCH v7 05/13] 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: David Gibson <david@gibson.dropbear.id.au>
---
 cpus.c               | 38 +++++++++++++++++++++++++++++++++++
 include/qom/cpu.h    | 10 +++++++++
 include/sysemu/kvm.h |  1 +
 kvm-all.c            | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 kvm-stub.c           |  5 +++++
 5 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index 1e97cc4..c5631f0 100644
--- a/cpus.c
+++ b/cpus.c
@@ -953,6 +953,18 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
     qemu_cpu_kick(cpu);
 }
 
+static void qemu_kvm_destroy_vcpu(CPUState *cpu)
+{
+    if (kvm_destroy_vcpu(cpu) < 0) {
+        error_report("kvm_destroy_vcpu failed");
+        exit(EXIT_FAILURE);
+    }
+}
+
+static void qemu_tcg_destroy_vcpu(CPUState *cpu)
+{
+}
+
 static void flush_queued_work(CPUState *cpu)
 {
     struct qemu_work_item *wi;
@@ -1053,6 +1065,11 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
             }
         }
         qemu_kvm_wait_io_event(cpu);
+        if (cpu->exit && !cpu_can_run(cpu)) {
+            qemu_kvm_destroy_vcpu(cpu);
+            qemu_mutex_unlock_iothread();
+            return NULL;
+        }
     }
 
     return NULL;
@@ -1108,6 +1125,7 @@ static void tcg_exec_all(void);
 static void *qemu_tcg_cpu_thread_fn(void *arg)
 {
     CPUState *cpu = arg;
+    CPUState *remove_cpu = NULL;
 
     rcu_register_thread();
 
@@ -1145,6 +1163,16 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
             }
         }
         qemu_tcg_wait_io_event(QTAILQ_FIRST(&cpus));
+        CPU_FOREACH(cpu) {
+            if (cpu->exit && !cpu_can_run(cpu)) {
+                remove_cpu = cpu;
+                break;
+            }
+        }
+        if (remove_cpu) {
+            qemu_tcg_destroy_vcpu(remove_cpu);
+            remove_cpu = NULL;
+        }
     }
 
     return NULL;
@@ -1301,6 +1329,13 @@ void resume_all_vcpus(void)
     }
 }
 
+void cpu_remove(CPUState *cpu)
+{
+    cpu->stop = true;
+    cpu->exit = true;
+    qemu_cpu_kick(cpu);
+}
+
 /* For temporary buffers for forming a name */
 #define VCPU_THREAD_NAME_SIZE 16
 
@@ -1517,6 +1552,9 @@ static void tcg_exec_all(void)
                 break;
             }
         } else if (cpu->stop || cpu->stopped) {
+            if (cpu->exit) {
+                next_cpu = CPU_NEXT(cpu);
+            }
             break;
         }
     }
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 2e5229d..32a2c71 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -232,6 +232,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.
+ * @exit: Indicates the CPU has exited due to an unplug operation.
  * @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.
@@ -284,6 +285,7 @@ struct CPUState {
     bool created;
     bool stop;
     bool stopped;
+    bool exit;
     bool crash_occurred;
     bool exit_request;
     uint32_t interrupt_request;
@@ -751,6 +753,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 7741f91..7324fa9 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -218,6 +218,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 9148889..699c1ce 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -62,6 +62,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;
@@ -95,6 +101,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;
@@ -238,6 +245,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;
@@ -246,7 +300,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;
@@ -1509,6 +1563,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 dc97a5e..0b39456 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -32,6 +32,11 @@ bool kvm_allowed;
 bool kvm_readonly_mem_allowed;
 bool kvm_ioeventfd_any_length_allowed;
 
+int kvm_destroy_vcpu(CPUState *cpu)
+{
+    return -ENOSYS;
+}
+
 int kvm_init_vcpu(CPUState *cpu)
 {
     return -ENOSYS;
-- 
2.1.0

[Qemu-devel] [PATCH v7 06/13] 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 c5631f0..2608ef5 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1067,6 +1067,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
         qemu_kvm_wait_io_event(cpu);
         if (cpu->exit && !cpu_can_run(cpu)) {
             qemu_kvm_destroy_vcpu(cpu);
+            cpu->created = false;
+            qemu_cond_signal(&qemu_cpu_cond);
             qemu_mutex_unlock_iothread();
             return NULL;
         }
@@ -1171,6 +1173,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;
         }
     }
@@ -1336,6 +1340,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 32a2c71..bed8654 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -760,6 +760,14 @@ void cpu_resume(CPUState *cpu);
  */
 void cpu_remove(CPUState *cpu);
 
+ /**
+ * cpu_remove_sync:
+ * @cpu: The CPU to remove.
+ *
+ * Requests the CPU to be removed and waits till it is removed.
+ */
+void cpu_remove_sync(CPUState *cpu);
+
 /**
  * qemu_init_vcpu:
  * @cpu: The vCPU to initialize.
-- 
2.1.0

[Qemu-devel] [PATCH v7 07/13] 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 9ff5796..0782e86 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -44,6 +44,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);
@@ -53,6 +65,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 d58729c..049d4e2 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -109,8 +109,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);
 }
@@ -343,8 +345,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 355a966..640162f 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -166,5 +166,6 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align);
 void xics_free(XICSState *icp, int irq, int num);
 
 void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
+void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu);
 
 #endif /* __XICS_H__ */
-- 
2.1.0

[Qemu-devel] [PATCH v7 08/13] target-ppc: Introduce PowerPC specific CPU core device

[Bharata B Rao]

CPU core device is a container of CPU thread devices.  CPU hotplug is
performed at the granularity of CPU core device. When hotplugged, CPU core
creates CPU thread devices.

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

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index c1ffc77..a6b7cfb 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -21,3 +21,4 @@ obj-$(CONFIG_E500) += e500.o mpc8544ds.o e500plat.o
 obj-$(CONFIG_E500) += mpc8544_guts.o ppce500_spin.o
 # PowerPC 440 Xilinx ML507 reference board.
 obj-$(CONFIG_XILINX) += virtex_ml507.o
+obj-y += cpu-core.o
diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
new file mode 100644
index 0000000..aa96e79
--- /dev/null
+++ b/hw/ppc/cpu-core.c
@@ -0,0 +1,75 @@
+/*
+ * PowerPC 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/ppc/cpu-core.h"
+#include "hw/boards.h"
+#include <sysemu/cpus.h>
+#include "qemu/error-report.h"
+
+static int ppc_cpu_core_realize_child(Object *child, void *opaque)
+{
+    Error **errp = opaque;
+
+    object_property_set_bool(child, true, "realized", errp);
+    if (*errp) {
+        return 1;
+    }
+
+    return 0;
+}
+
+static void ppc_cpu_core_realize(DeviceState *dev, Error **errp)
+{
+    object_child_foreach(OBJECT(dev), ppc_cpu_core_realize_child, errp);
+}
+
+static void ppc_cpu_core_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = ppc_cpu_core_realize;
+    dc->desc = "PowerPC CPU core";
+}
+
+static void ppc_cpu_core_instance_init(Object *obj)
+{
+    int i;
+    CPUState *cpu;
+    MachineState *machine = MACHINE(qdev_get_machine());
+    PowerPCCPUCore *core = POWERPC_CPU_CORE(obj);
+
+    /* Create as many CPU threads as specified in the topology */
+    for (i = 0; i < smp_threads; i++) {
+        cpu = cpu_generic_init(TYPE_POWERPC_CPU, machine->cpu_model);
+        if (!cpu) {
+            error_report("Unable to find CPU definition: %s",
+                          machine->cpu_model);
+            exit(EXIT_FAILURE);
+        }
+        object_property_add_child(obj, "thread[*]", OBJECT(cpu), &error_abort);
+        object_unref(OBJECT(cpu));
+        if (!i) {
+            core->thread0 = POWERPC_CPU(cpu);
+        }
+    }
+}
+
+static const TypeInfo ppc_cpu_core_type_info = {
+    .name = TYPE_POWERPC_CPU_CORE,
+    .parent = TYPE_DEVICE,
+    .class_init = ppc_cpu_core_class_init,
+    .instance_init = ppc_cpu_core_instance_init,
+    .instance_size = sizeof(PowerPCCPUCore),
+};
+
+static void cpu_core_register_types(void)
+{
+    type_register_static(&ppc_cpu_core_type_info);
+}
+
+type_init(cpu_core_register_types)
diff --git a/include/hw/ppc/cpu-core.h b/include/hw/ppc/cpu-core.h
new file mode 100644
index 0000000..ff2ebc2
--- /dev/null
+++ b/include/hw/ppc/cpu-core.h
@@ -0,0 +1,33 @@
+/*
+ * 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_PPC_CPU_CORE_H
+#define HW_PPC_CPU_CORE_H
+
+#include "hw/qdev.h"
+
+#ifdef TARGET_PPC64
+#define TYPE_POWERPC_CPU_CORE "powerpc64-cpu-core"
+#elif defined(TARGET_PPCEMB)
+#define TYPE_POWERPC_CPU_CORE "embedded-powerpc-cpu-core"
+#else
+#define TYPE_POWERPC_CPU_CORE "powerpc-cpu-core"
+#endif
+
+#define POWERPC_CPU_CORE(obj) \
+    OBJECT_CHECK(PowerPCCPUCore, (obj), TYPE_POWERPC_CPU_CORE)
+
+typedef struct PowerPCCPUCore {
+    /*< private >*/
+    DeviceState parent_obj;
+    /*< public >*/
+
+    PowerPCCPU *thread0;
+} PowerPCCPUCore;
+
+#endif
-- 
2.1.0

[Qemu-devel] [PATCH v7 09/13] spapr: Enable CPU hotplug for pseries-2.6 and add CPU DRC DT entries

[Bharata B Rao]

Start supporting CPU hotplug from pseries-2.6 onwards. Add CPU
DRC (Dynamic Resource Connector) device tree entries.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 23 +++++++++++++++++++++++
 include/hw/ppc/spapr.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6ac9f06..eeea411 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -985,6 +985,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) {
+            fprintf(stderr, "Couldn't set up CPU DR device tree properties\n");
+            exit(1);
+        }
+    }
+
     _FDT((fdt_pack(fdt)));
 
     if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
@@ -1752,6 +1762,8 @@ static void ppc_spapr_init(MachineState *machine)
     long load_limit, fw_size;
     bool kernel_le = false;
     char *filename;
+    int smt = kvmppc_smt_threads();
+    int smp_max_cores = max_cpus / smp_threads;
 
     msi_supported = true;
 
@@ -1818,6 +1830,15 @@ static void ppc_spapr_init(MachineState *machine)
         spapr_validate_node_memory(machine, &error_fatal);
     }
 
+    if (smc->dr_cpu_enabled) {
+        for (i = 0; i < smp_max_cores; i++) {
+            sPAPRDRConnector *drc =
+                spapr_dr_connector_new(OBJECT(spapr),
+                                       SPAPR_DR_CONNECTOR_TYPE_CPU, i * smt);
+            qemu_register_reset(spapr_drc_reset, drc);
+        }
+    }
+
     /* init CPUs */
     if (machine->cpu_model == NULL) {
         machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
@@ -2323,6 +2344,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
 
     smc->dr_lmb_enabled = true;
+    smc->dr_cpu_enabled = true;
     fwc->get_dev_path = spapr_get_fw_dev_path;
     nc->nmi_monitor_handler = spapr_nmi;
 }
@@ -2402,6 +2424,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);
     mc->validate_smp_config = NULL;
 }
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 1f9e722..a9d98e7 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 */
 };
 
-- 
2.1.0

[Qemu-devel] [PATCH v7 10/13] spapr: CPU hotplug support

[Bharata B Rao]

Support CPU hotplug via device-add command like this:

(qemu) device_add powerpc64-cpu-core,id=core2

In response to device_add, CPU core device will be created. CPU core
device creates and realizes CPU thread devices. If the machine type
supports CPU hotplug, boot-time CPUs are created as CPU core devices
otherwise they continue to be created as individual CPU devices.

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

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c              | 145 +++++++++++++++++++++++++++++++++++++++++---
 hw/ppc/spapr_events.c       |   3 +
 hw/ppc/spapr_rtas.c         |  24 ++++++++
 target-ppc/translate_init.c |   8 +++
 4 files changed, 173 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index eeea411..6ef520d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -63,6 +63,7 @@
 
 #include "hw/compat.h"
 #include "qemu-common.h"
+#include "hw/ppc/cpu-core.h"
 
 #include <libfdt.h>
 
@@ -601,6 +602,18 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
     size_t page_sizes_prop_size;
     uint32_t vcpus_per_socket = smp_threads * smp_cores;
     uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+    int drc_index;
+
+    if (smc->dr_cpu_enabled) {
+        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
+        g_assert(drc);
+        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+        drc_index = drck->get_index(drc);
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
+    }
 
     /* Note: we keep CI large pages off for now because a 64K capable guest
      * provisioned with large pages might otherwise try to map a qemu
@@ -1764,6 +1777,8 @@ static void ppc_spapr_init(MachineState *machine)
     char *filename;
     int smt = kvmppc_smt_threads();
     int smp_max_cores = max_cpus / smp_threads;
+    int spapr_smp_cores = smp_cpus / smp_threads;
+    Object *core;
 
     msi_supported = true;
 
@@ -1843,13 +1858,22 @@ 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) {
+        for (i = 0; i < spapr_smp_cores; i++) {
+            core = object_new(TYPE_POWERPC_CPU_CORE);
+            object_property_set_bool(core, true, "realized", &error_abort);
+        }
+    } else {
+        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);
+            }
+            object_property_set_bool(OBJECT(cpu), true, "realized",
+                                     &error_abort);
         }
-        spapr_cpu_init(spapr, cpu, &error_fatal);
     }
 
     if (kvm_enabled()) {
@@ -2245,10 +2269,92 @@ out:
     error_propagate(errp, local_err);
 }
 
+static void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
+                                           int *fdt_offset,
+                                           sPAPRMachineState *spapr)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    DeviceClass *dc = DEVICE_GET_CLASS(cs);
+    int id = ppc_get_vcpu_dt_id(cpu);
+    void *fdt;
+    int offset, fdt_size;
+    char *nodename;
+
+    fdt = create_device_tree(&fdt_size);
+    nodename = g_strdup_printf("%s@%x", dc->fw_name, id);
+    offset = fdt_add_subnode(fdt, 0, nodename);
+
+    spapr_populate_cpu_dt(cs, fdt, offset, spapr);
+    g_free(nodename);
+
+    *fdt_offset = offset;
+    return fdt;
+}
+
+static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                            Error **errp)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+    sPAPRMachineState *ms = SPAPR_MACHINE(qdev_get_machine());
+    PowerPCCPUCore *core = POWERPC_CPU_CORE(OBJECT(dev));
+    PowerPCCPU *cpu = core->thread0;
+    CPUState *cs = CPU(cpu);
+    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;
+    void *fdt = NULL;
+    int fdt_offset = 0;
+
+    if (!smc->dr_cpu_enabled) {
+        /*
+         * This is a cold plugged CPU core but the machine doesn't support
+         * DR. So skip the hotplug path ensuring that the core is brought
+         * up online with out an associated DR connector.
+         */
+        return;
+    }
+
+    g_assert(drc);
+
+    /*
+     * Setup CPU DT entries only for hotplugged CPUs. For boot time or
+     * coldplugged CPUs DT entries are setup in spapr_finalize_fdt().
+     */
+    if (dev->hotplugged) {
+        fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, ms);
+    }
+
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
+    if (local_err) {
+        g_free(fdt);
+        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);
+    }
+    return;
+}
+
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         int node;
@@ -2285,6 +2391,29 @@ 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_CPU)) {
+        CPUState *cs = CPU(dev);
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+        int i;
+
+        if (!smc->dr_cpu_enabled && dev->hotplugged) {
+            error_setg(errp, "CPU hotplug not supported for this machine");
+            cpu_remove_sync(cs);
+            return;
+        }
+
+        /* 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;
+            }
+        }
+
+        spapr_cpu_init(ms, cpu, errp);
+        spapr_cpu_reset(cpu);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
+        spapr_core_plug(hotplug_dev, dev, errp);
     }
 }
 
@@ -2299,7 +2428,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
 static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
                                              DeviceState *dev)
 {
-    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
         return HOTPLUG_HANDLER(machine);
     }
     return NULL;
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 333e6ff..9493c7c 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -436,6 +436,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 130c917..cee9960 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -33,6 +33,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"
 
@@ -159,6 +160,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,
@@ -195,6 +217,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/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 3304ad1..f18489f 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -30,6 +30,9 @@
 #include "qemu/error-report.h"
 #include "qapi/visitor.h"
 #include "hw/qdev-properties.h"
+#if !defined(CONFIG_USER_ONLY)
+#include "sysemu/sysemu.h"
+#endif
 
 //#define PPC_DUMP_CPU
 //#define PPC_DEBUG_SPR
@@ -8983,6 +8986,11 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
 #if !defined(CONFIG_USER_ONLY)
+    if (cs->cpu_index >= max_cpus) {
+        error_setg(errp, "Cannot have more than %d CPUs", max_cpus);
+        return;
+    }
+
     cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
         + (cs->cpu_index % smp_threads);
 #endif
-- 
2.1.0

[Qemu-devel] [PATCH v7 11/13] 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 supported by sending the hot unplug
notification to the guest via EPOW interrupt. 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         | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  8 +++++
 2 files changed, 98 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6ef520d..0a112d8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2417,11 +2417,101 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     }
 }
 
+static 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_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;
+
+    QLIST_INIT(&unplug_list);
+    object_child_foreach(OBJECT(dev), spapr_cpu_release, &unplug_list);
+    spapr_cpu_core_cleanup(&unplug_list);
+    object_unparent(OBJECT(dev));
+}
+
+static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                             Error **errp)
+{
+    PowerPCCPUCore *core = POWERPC_CPU_CORE(OBJECT(dev));
+    PowerPCCPU *cpu = core->thread0;
+    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);
+}
+
 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_POWERPC_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/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index a9d98e7..e161f8f 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -630,4 +630,12 @@ int spapr_rng_populate_dt(void *fdt);
  */
 #define SPAPR_LMB_FLAGS_ASSIGNED 0x00000008
 
+/* 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 /* !defined (__HW_SPAPR_H__) */
-- 
2.1.0

[Qemu-devel] [PATCH v7 12/13] qmp: Add query-ppc-cpu-cores command

[Bharata B Rao]

Show the details of PPC CPU cores via a new QMP command.

TODO: update qmp-commands.hx with example

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/cpu-core.c               | 77 +++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json                | 31 +++++++++++++++++
 qmp-commands.hx                 | 51 +++++++++++++++++++++++++++
 stubs/Makefile.objs             |  1 +
 stubs/qmp_query_ppc_cpu_cores.c | 10 ++++++
 5 files changed, 170 insertions(+)
 create mode 100644 stubs/qmp_query_ppc_cpu_cores.c

diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
index aa96e79..652a5aa 100644
--- a/hw/ppc/cpu-core.c
+++ b/hw/ppc/cpu-core.c
@@ -9,7 +9,84 @@
 #include "hw/ppc/cpu-core.h"
 #include "hw/boards.h"
 #include <sysemu/cpus.h>
+#include <sysemu/kvm.h>
 #include "qemu/error-report.h"
+#include "qmp-commands.h"
+
+/*
+ * QMP: info ppc-cpu-cores
+ */
+static int qmp_ppc_cpu_list(Object *obj, void *opaque)
+{
+    CpuInfoList ***prev = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_POWERPC_CPU)) {
+        CpuInfoList *elem = g_new0(CpuInfoList, 1);
+        CpuInfo *s = g_new0(CpuInfo, 1);
+        CPUState *cs = CPU(obj);
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+        CPUPPCState *env = &cpu->env;
+
+        cpu_synchronize_state(cs);
+        s->arch = CPU_INFO_ARCH_PPC;
+        s->current = (cs == first_cpu);
+        s->CPU = cs->cpu_index;
+        s->qom_path = object_get_canonical_path(obj);
+        s->halted = cs->halted;
+        s->thread_id = cs->thread_id;
+        s->u.ppc = g_new0(CpuInfoPPC, 1);
+        s->u.ppc->nip = env->nip;
+
+        elem->value = s;
+        elem->next = NULL;
+        **prev = elem;
+        *prev = &elem->next;
+    }
+    object_child_foreach(obj, qmp_ppc_cpu_list, opaque);
+    return 0;
+}
+
+static int qmp_ppc_cpu_core_list(Object *obj, void *opaque)
+{
+    PPCCPUCoreList ***prev = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_POWERPC_CPU_CORE)) {
+        DeviceClass *dc = DEVICE_GET_CLASS(obj);
+        DeviceState *dev = DEVICE(obj);
+
+        if (dev->realized) {
+            PPCCPUCoreList *elem = g_new0(PPCCPUCoreList, 1);
+            PPCCPUCore *s = g_new0(PPCCPUCore, 1);
+            CpuInfoList *cpu_head = NULL;
+            CpuInfoList **cpu_prev = &cpu_head;
+
+            if (dev->id) {
+                s->has_id = true;
+                s->id = g_strdup(dev->id);
+            }
+            s->hotplugged = dev->hotplugged;
+            s->hotpluggable = dc->hotpluggable;
+            qmp_ppc_cpu_list(obj, &cpu_prev);
+            s->threads = cpu_head;
+            elem->value = s;
+            elem->next = NULL;
+            **prev = elem;
+            *prev = &elem->next;
+        }
+    }
+
+    object_child_foreach(obj, qmp_ppc_cpu_core_list, opaque);
+    return 0;
+}
+
+PPCCPUCoreList *qmp_query_ppc_cpu_cores(Error **errp)
+{
+    PPCCPUCoreList *head = NULL;
+    PPCCPUCoreList **prev = &head;
+
+    qmp_ppc_cpu_core_list(qdev_get_machine(), &prev);
+    return head;
+}
 
 static int ppc_cpu_core_realize_child(Object *child, void *opaque)
 {
diff --git a/qapi-schema.json b/qapi-schema.json
index 8d04897..0902697 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4083,3 +4083,34 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @PPCCPUCore:
+#
+# Information about PPC CPU core devices
+#
+# @hotplugged: true if device was hotplugged
+#
+# @hotpluggable: true if device if could be added/removed while machine is running
+#
+# Since: 2.6
+##
+
+{ 'struct': 'PPCCPUCore',
+  'data': { '*id': 'str',
+            'hotplugged': 'bool',
+            'hotpluggable': 'bool',
+            'threads' : ['CpuInfo']
+          }
+}
+
+##
+# @query-ppc-cpu-core:
+#
+# Returns information for all PPC CPU core devices
+#
+# Returns: a list of @PPCCPUCore.
+#
+# Since: 2.6
+##
+{ 'command': 'query-ppc-cpu-cores', 'returns': ['PPCCPUCore'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index db072a6..77cda3c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4795,3 +4795,54 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+#if defined TARGET_PPC64
+    {
+        .name       = "query-ppc-cpu-cores",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_query_ppc_cpu_cores,
+    },
+#endif
+
+SQMP
+@query-ppc-cpu-cores
+--------------------
+
+Show PowerPC CPU core devices information.
+
+Example:
+-> { "execute": "query-ppc-cpu-cores" }
+<- {"return": [{"threads": [
+                 {"arch": "ppc",
+                  "current": false,
+                  "CPU": 16,
+                  "nip": -4611686018426944644,
+                  "qom_path": "/machine/peripheral/core2/thread[0]",
+                  "halted": false,
+                  "thread_id": 32636},
+                 {"arch": "ppc",
+                  "current": false",
+                  "CPU": 17,
+                  "nip": -4611686018426944644,
+                  "qom_path": "/machine/peripheral/core2/thread[1]",
+                  "halted": false, "thread_id": 32637},
+                 {"arch": "ppc",
+                  "current": false,
+                  "CPU": 18,
+                  "nip": -4611686018426944644,
+                  "qom_path": "/machine/peripheral/core2/thread[2]",
+                  "halted": false,
+                  "thread_id": 32638},
+                 {"arch": "ppc",
+                  "current": false,
+                  "CPU": 19,
+                  "nip": -4611686018426944644,
+                  "qom_path": "/machine/peripheral/core2/thread[3]",
+                  "halted": false,
+                  "thread_id": 32639}],
+                "hotplugged": false,
+                "hotpluggable": true,
+                "id": "core2"}
+              ]}
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index d7898a0..1d65999 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -38,3 +38,4 @@ stub-obj-y += qmp_pc_dimm_device_list.o
 stub-obj-y += target-monitor-defs.o
 stub-obj-y += target-get-monitor-def.o
 stub-obj-y += vhost.o
+stub-obj-y += qmp_query_ppc_cpu_cores.o
diff --git a/stubs/qmp_query_ppc_cpu_cores.c b/stubs/qmp_query_ppc_cpu_cores.c
new file mode 100644
index 0000000..6a875f0
--- /dev/null
+++ b/stubs/qmp_query_ppc_cpu_cores.c
@@ -0,0 +1,10 @@
+#include "qom/object.h"
+#include "qapi/qmp/qerror.h"
+#include "qemu/typedefs.h"
+#include "qmp-commands.h"
+
+PPCCPUCoreList *qmp_query_ppc_cpu_cores(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return 0;
+}
-- 
2.1.0

[Qemu-devel] [PATCH v7 13/13] hmp: Add "info ppc-cpu-cores" command

[Bharata B Rao]

This is the hmp equivalent of "query ppc-cpu-cores"

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

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 9b71351..cd9a42e 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -786,6 +786,22 @@ STEXI
 Display the value of a storage key (s390 only)
 ETEXI
 
+#if defined(TARGET_PPC64)
+    {
+        .name       = "ppc-cpu-cores",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show PowerPC CPU core devices",
+        .mhandler.cmd = hmp_info_ppc_cpu_cores,
+    },
+#endif
+
+STEXI
+@item info ppc-cpu-cores
+@findex ppc-cpu-cores
+Show PowerPC CPU core devices.
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index 54f2620..ae75aa1 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2375,3 +2375,34 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
 
     qapi_free_RockerOfDpaGroupList(list);
 }
+
+void hmp_info_ppc_cpu_cores(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    PPCCPUCoreList *ppc_cpu_core_list = qmp_query_ppc_cpu_cores(&err);
+    PPCCPUCoreList *s = ppc_cpu_core_list;
+    CpuInfoList *thread;
+
+    while (s) {
+        monitor_printf(mon, "PowerPC CPU device: \"%s\"\n",
+                       s->value->id ? s->value->id : "");
+        monitor_printf(mon, "  hotplugged: %s\n",
+                           s->value->hotplugged ? "true" : "false");
+        monitor_printf(mon, "  hotpluggable: %s\n",
+                           s->value->hotpluggable ? "true" : "false");
+        monitor_printf(mon, "  Threads:\n");
+        for (thread = s->value->threads; thread; thread = thread->next) {
+            monitor_printf(mon, "    CPU #%" PRId64 ":", thread->value->CPU);
+            monitor_printf(mon, " nip=0x%016" PRIx64,
+                           thread->value->u.ppc->nip);
+            if (thread->value->halted) {
+                monitor_printf(mon, " (halted)");
+            }
+            monitor_printf(mon, " thread_id=%" PRId64 "\n",
+                           thread->value->thread_id);
+        }
+        s = s->next;
+    }
+
+    qapi_free_PPCCPUCoreList(ppc_cpu_core_list);
+}
diff --git a/hmp.h b/hmp.h
index a8c5b5a..a31e3d2 100644
--- a/hmp.h
+++ b/hmp.h
@@ -131,5 +131,6 @@ void hmp_rocker(Monitor *mon, const QDict *qdict);
 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_ppc_cpu_cores(Monitor *mon, const QDict *qdict);
 
 #endif
-- 
2.1.0

Re: [Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[Eduardo Habkost]

On Thu, Jan 28, 2016 at 11:19:43AM +0530, Bharata B Rao wrote:
> Prevent guests from booting with CPU topologies that have partially
> filled CPU cores or can result in partially filled CPU cores after
> CPU hotplug like
> 
> -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> 
> This is enforced by introducing MachineClass::validate_smp_config()
> that gets called from generic SMP parsing code. Machine type versions
> that want to enforce this can define this to the generic version
> provided.
> 
> Only sPAPR and PC machine types starting from version 2.6 enforce this in
> this patch.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
[...]
> diff --git a/vl.c b/vl.c
> index f043009..9e4da46 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4126,6 +4126,11 @@ int main(int argc, char **argv, char **envp)
>  
>      smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
>  
> +    if (machine_class->validate_smp_config) {
> +        machine_class->validate_smp_config(smp_cpus, max_cpus, smp_threads,
> +                                           &error_abort);

Invalid SMP config should make QEMU just exit with an error, not
abort(). Please use &error_fatal instead.

The rest looks good.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 02/13] exec: Remove cpu from cpus list during cpu_exec_exit()

[Eduardo Habkost]

On Thu, Jan 28, 2016 at 11:19:44AM +0530, 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_init() is called from generic CPU::instance_finalize and some
> archs like PowerPC call it from CPU unrealizefn. So ensure that we
> dequeue the cpu only once.
> 
> Now -1 value for cpu->cpu_index indicates that we have already dequeued
> the cpu for CONFIG_USER_ONLY case also.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  exec.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 7115403..c8da9d4 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -596,6 +596,7 @@ void cpu_exec_exit(CPUState *cpu)
>          return;
>      }
>  
> +    QTAILQ_REMOVE(&cpus, cpu, node);
>      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
>      cpu->cpu_index = -1;
>  }
> @@ -614,6 +615,15 @@ static int cpu_get_free_index(Error **errp)
>  
>  void cpu_exec_exit(CPUState *cpu)
>  {
> +    cpu_list_lock();
> +    if (cpu->cpu_index == -1) {
> +        cpu_list_unlock();
> +        return;
> +    }
> +
> +    QTAILQ_REMOVE(&cpus, cpu, node);
> +    cpu->cpu_index = -1;
> +    cpu_list_unlock();

With this, the only differences between the two cpu_exec_exit()
implementations are:

* cpu_list_lock()/cpu_list_unlock() functions.
  * We can add !CONFIG_USER_ONLY stubs for them.
* The bitmap_clear() call.
  * It can be abstracted away in a cpu_release_index() function,
    just like we already have a CONFIG_USER_ONLY version of
    cpu_get_free_index().
    * I was going to suggest using cpu_index_map on
      CONFIG_USER_ONLY too, but I assume we don't want to limit
      the number of threads in *-user to MAX_CPUMASK_BITS.

This way we won't need to duplicate cpu_exec_exit() code.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 12/13] qmp: Add query-ppc-cpu-cores command

[Eric Blake]

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

On 01/27/2016 10:49 PM, Bharata B Rao wrote:
> Show the details of PPC CPU cores via a new QMP command.
> 
> TODO: update qmp-commands.hx with example

Is this a stale comment? [1]

> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---

>  #include <sysemu/cpus.h>
> +#include <sysemu/kvm.h>
>  #include "qemu/error-report.h"
> +#include "qmp-commands.h"
> +
> +/*
> + * QMP: info ppc-cpu-cores
> + */
> +static int qmp_ppc_cpu_list(Object *obj, void *opaque)

Comment is a bit off - 'info ...' is an HMP command; this callback is
helping implement the QMP function query-ppc-cpu-cores.

> +++ b/qapi-schema.json
> @@ -4083,3 +4083,34 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @PPCCPUCore:
> +#
> +# Information about PPC CPU core devices
> +#
> +# @hotplugged: true if device was hotplugged
> +#
> +# @hotpluggable: true if device if could be added/removed while machine is running
> +#
> +# Since: 2.6

Missing docs on 'id' and 'threads'.

> +##
> +
> +{ 'struct': 'PPCCPUCore',
> +  'data': { '*id': 'str',
> +            'hotplugged': 'bool',
> +            'hotpluggable': 'bool',
> +            'threads' : ['CpuInfo']
> +          }
> +}
> +
> +##
> +# @query-ppc-cpu-core:
> +#
> +# Returns information for all PPC CPU core devices
> +#
> +# Returns: a list of @PPCCPUCore.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'query-ppc-cpu-cores', 'returns': ['PPCCPUCore'] }

Interface seems okay.

> +++ b/qmp-commands.hx
> @@ -4795,3 +4795,54 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +#if defined TARGET_PPC64
> +    {
> +        .name       = "query-ppc-cpu-cores",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_query_ppc_cpu_cores,
> +    },
> +#endif

Hmm. Conditional compilation. Does the command show up in
'query-commands' and introspection output, even when the target is not
ppc64?  We may need to fix qapi introspection to support conditionals
better; maybe some of Marc-Andre's patches towards eliminating
qmp-commands.hx will come into play here.

> +
> +SQMP
> +@query-ppc-cpu-cores
> +--------------------
> +
> +Show PowerPC CPU core devices information.
> +
> +Example:
> +-> { "execute": "query-ppc-cpu-cores" }
> +<- {"return": [{"threads": [
> +                 {"arch": "ppc",

[1] looks like you provided an example after all.  Is it worth
documenting that this command is only conditionally available?


> +++ b/stubs/qmp_query_ppc_cpu_cores.c
> @@ -0,0 +1,10 @@
> +#include "qom/object.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qemu/typedefs.h"
> +#include "qmp-commands.h"
> +
> +PPCCPUCoreList *qmp_query_ppc_cpu_cores(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return 0;
> +}

Hmm - will the stub even be used, since you used an #ifdef in the .hx file?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v7 13/13] hmp: Add "info ppc-cpu-cores" command

[Eric Blake]

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

On 01/27/2016 10:49 PM, Bharata B Rao wrote:
> This is the hmp equivalent of "query ppc-cpu-cores"

The QMP command is spelled "query-ppc-cpu-cores".

Most HMP commands prefer '_' over '-'; so this should be 'info
ppc_cpu_cores'.

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

> +++ b/hmp.c
> @@ -2375,3 +2375,34 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
>  
>      qapi_free_RockerOfDpaGroupList(list);
>  }
> +
> +void hmp_info_ppc_cpu_cores(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    PPCCPUCoreList *ppc_cpu_core_list = qmp_query_ppc_cpu_cores(&err);
> +    PPCCPUCoreList *s = ppc_cpu_core_list;
> +    CpuInfoList *thread;
> +
> +    while (s) {
> +        monitor_printf(mon, "PowerPC CPU device: \"%s\"\n",
> +                       s->value->id ? s->value->id : "");

This should probably be checking s->value->has_id rather than assuming
that s->value->id will be NULL when not present (well, I'd like to clean
up qapi to avoid the need for has_FOO when FOO  is a pointer, but we're
not there yet).

> +        monitor_printf(mon, "  hotplugged: %s\n",
> +                           s->value->hotplugged ? "true" : "false");
> +        monitor_printf(mon, "  hotpluggable: %s\n",
> +                           s->value->hotpluggable ? "true" : "false");
> +        monitor_printf(mon, "  Threads:\n");
> +        for (thread = s->value->threads; thread; thread = thread->next) {
> +            monitor_printf(mon, "    CPU #%" PRId64 ":", thread->value->CPU);
> +            monitor_printf(mon, " nip=0x%016" PRIx64,
> +                           thread->value->u.ppc->nip);

This uses value->u.ppc without first checking that the discriminator
value->arch is set to CPU_INFO_ARCH_PPC; could that be a problem down
the road?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[David Gibson]

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

On Thu, Jan 28, 2016 at 11:19:43AM +0530, Bharata B Rao wrote:
> Prevent guests from booting with CPU topologies that have partially
> filled CPU cores or can result in partially filled CPU cores after
> CPU hotplug like
> 
> -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> 
> This is enforced by introducing MachineClass::validate_smp_config()
> that gets called from generic SMP parsing code. Machine type versions
> that want to enforce this can define this to the generic version
> provided.
> 
> Only sPAPR and PC machine types starting from version 2.6 enforce this in
> this patch.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

I've been kind of lost in the back and forth about
threads/cores/sockets.

What, in the end, is the rationale for allowing partially filled
sockets, but not partially filled cores?

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

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

Re: [Qemu-devel] [PATCH v7 02/13] exec: Remove cpu from cpus list during cpu_exec_exit()

[Bharata B Rao]

On Thu, Jan 28, 2016 at 05:19:33PM -0200, Eduardo Habkost wrote:
> On Thu, Jan 28, 2016 at 11:19:44AM +0530, 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_init() is called from generic CPU::instance_finalize and some
> > archs like PowerPC call it from CPU unrealizefn. So ensure that we
> > dequeue the cpu only once.
> > 
> > Now -1 value for cpu->cpu_index indicates that we have already dequeued
> > the cpu for CONFIG_USER_ONLY case also.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  exec.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/exec.c b/exec.c
> > index 7115403..c8da9d4 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -596,6 +596,7 @@ void cpu_exec_exit(CPUState *cpu)
> >          return;
> >      }
> >  
> > +    QTAILQ_REMOVE(&cpus, cpu, node);
> >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> >      cpu->cpu_index = -1;
> >  }
> > @@ -614,6 +615,15 @@ static int cpu_get_free_index(Error **errp)
> >  
> >  void cpu_exec_exit(CPUState *cpu)
> >  {
> > +    cpu_list_lock();
> > +    if (cpu->cpu_index == -1) {
> > +        cpu_list_unlock();
> > +        return;
> > +    }
> > +
> > +    QTAILQ_REMOVE(&cpus, cpu, node);
> > +    cpu->cpu_index = -1;
> > +    cpu_list_unlock();
> 
> With this, the only differences between the two cpu_exec_exit()
> implementations are:
> 
> * cpu_list_lock()/cpu_list_unlock() functions.
>   * We can add !CONFIG_USER_ONLY stubs for them.
> * The bitmap_clear() call.
>   * It can be abstracted away in a cpu_release_index() function,
>     just like we already have a CONFIG_USER_ONLY version of
>     cpu_get_free_index().

Ok, made those changes so that cpu_exec_exit() will be a common routine
with some CONFIG_USER_ONLY ifdefs in between.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v7 12/13] qmp: Add query-ppc-cpu-cores command

[Bharata B Rao]

On Thu, Jan 28, 2016 at 01:52:26PM -0700, Eric Blake wrote:
> On 01/27/2016 10:49 PM, Bharata B Rao wrote:
> > Show the details of PPC CPU cores via a new QMP command.
> > 
> > TODO: update qmp-commands.hx with example
> 
> Is this a stale comment? [1]

Yes, I missed removing it after I put the example in qmp-commands.hx.

> 
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> 
> >  #include <sysemu/cpus.h>
> > +#include <sysemu/kvm.h>
> >  #include "qemu/error-report.h"
> > +#include "qmp-commands.h"
> > +
> > +/*
> > + * QMP: info ppc-cpu-cores
> > + */
> > +static int qmp_ppc_cpu_list(Object *obj, void *opaque)
> 
> Comment is a bit off - 'info ...' is an HMP command; this callback is
> helping implement the QMP function query-ppc-cpu-cores.

Ok, will make it "QMP: query-ppc-cpu-cores"

> 
> > +++ b/qapi-schema.json
> > @@ -4083,3 +4083,34 @@
> >  ##
> >  { 'enum': 'ReplayMode',
> >    'data': [ 'none', 'record', 'play' ] }
> > +
> > +##
> > +# @PPCCPUCore:
> > +#
> > +# Information about PPC CPU core devices
> > +#
> > +# @hotplugged: true if device was hotplugged
> > +#
> > +# @hotpluggable: true if device if could be added/removed while machine is running
> > +#
> > +# Since: 2.6
> 
> Missing docs on 'id' and 'threads'.

Will add.

> 
> > +##
> > +
> > +{ 'struct': 'PPCCPUCore',
> > +  'data': { '*id': 'str',
> > +            'hotplugged': 'bool',
> > +            'hotpluggable': 'bool',
> > +            'threads' : ['CpuInfo']
> > +          }
> > +}
> > +
> > +##
> > +# @query-ppc-cpu-core:
> > +#
> > +# Returns information for all PPC CPU core devices
> > +#
> > +# Returns: a list of @PPCCPUCore.
> > +#
> > +# Since: 2.6
> > +##
> > +{ 'command': 'query-ppc-cpu-cores', 'returns': ['PPCCPUCore'] }
> 
> Interface seems okay.
> 
> > +++ b/qmp-commands.hx
> > @@ -4795,3 +4795,54 @@ Example:
> >                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
> >                    "pop-vlan": 1, "id": 251658240}
> >     ]}
> > +
> > +EQMP
> > +
> > +#if defined TARGET_PPC64
> > +    {
> > +        .name       = "query-ppc-cpu-cores",
> > +        .args_type  = "",
> > +        .mhandler.cmd_new = qmp_marshal_query_ppc_cpu_cores,
> > +    },
> > +#endif
> 
> Hmm. Conditional compilation. Does the command show up in
> 'query-commands' and introspection output, even when the target is not
> ppc64?  We may need to fix qapi introspection to support conditionals
> better; maybe some of Marc-Andre's patches towards eliminating
> qmp-commands.hx will come into play here.

For targets other than ppc64, query-commands doesn't list
query-ppc-cpu-cores.

> 
> > +
> > +SQMP
> > +@query-ppc-cpu-cores
> > +--------------------
> > +
> > +Show PowerPC CPU core devices information.
> > +
> > +Example:
> > +-> { "execute": "query-ppc-cpu-cores" }
> > +<- {"return": [{"threads": [
> > +                 {"arch": "ppc",
> 
> [1] looks like you provided an example after all.  Is it worth
> documenting that this command is only conditionally available?

Will add

# Note: This command is available only for PowerPC targets

> 
> 
> > +++ b/stubs/qmp_query_ppc_cpu_cores.c
> > @@ -0,0 +1,10 @@
> > +#include "qom/object.h"
> > +#include "qapi/qmp/qerror.h"
> > +#include "qemu/typedefs.h"
> > +#include "qmp-commands.h"
> > +
> > +PPCCPUCoreList *qmp_query_ppc_cpu_cores(Error **errp)
> > +{
> > +    error_setg(errp, QERR_UNSUPPORTED);
> > +    return 0;
> > +}
> 
> Hmm - will the stub even be used, since you used an #ifdef in the .hx file?

Though I have put

.mhandler.cmd_new = qmp_marshal_query_ppc_cpu_cores,

under ifdef in .hx, qmp_marshal_query_ppc_cpu_cores() is getting defined in
qmp-marshal.c and hence we need this stub file so that qmp_query_ppc_cpu_cores()
gets resolved from qmp-marshal.c:qmp_marshal_query_ppc_cpu_cores().

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v7 13/13] hmp: Add "info ppc-cpu-cores" command

[Bharata B Rao]

On Thu, Jan 28, 2016 at 02:56:41PM -0700, Eric Blake wrote:
> On 01/27/2016 10:49 PM, Bharata B Rao wrote:
> > This is the hmp equivalent of "query ppc-cpu-cores"
> 
> The QMP command is spelled "query-ppc-cpu-cores".
> 
> Most HMP commands prefer '_' over '-'; so this should be 'info
> ppc_cpu_cores'.

I see that a few commands have '-' but as you note, majority of them use
'_'. Though I personally prefer '-', if HMP convention is to go with '_',
will change in the next iteration.

> 
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hmp-commands-info.hx | 16 ++++++++++++++++
> >  hmp.c                | 31 +++++++++++++++++++++++++++++++
> >  hmp.h                |  1 +
> >  3 files changed, 48 insertions(+)
> > 
> 
> > +++ b/hmp.c
> > @@ -2375,3 +2375,34 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
> >  
> >      qapi_free_RockerOfDpaGroupList(list);
> >  }
> > +
> > +void hmp_info_ppc_cpu_cores(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    PPCCPUCoreList *ppc_cpu_core_list = qmp_query_ppc_cpu_cores(&err);
> > +    PPCCPUCoreList *s = ppc_cpu_core_list;
> > +    CpuInfoList *thread;
> > +
> > +    while (s) {
> > +        monitor_printf(mon, "PowerPC CPU device: \"%s\"\n",
> > +                       s->value->id ? s->value->id : "");
> 
> This should probably be checking s->value->has_id rather than assuming
> that s->value->id will be NULL when not present (well, I'd like to clean
> up qapi to avoid the need for has_FOO when FOO  is a pointer, but we're
> not there yet).

Ok, will switch to s->value->has_id ? s->value->id : "")

> 
> > +        monitor_printf(mon, "  hotplugged: %s\n",
> > +                           s->value->hotplugged ? "true" : "false");
> > +        monitor_printf(mon, "  hotpluggable: %s\n",
> > +                           s->value->hotpluggable ? "true" : "false");
> > +        monitor_printf(mon, "  Threads:\n");
> > +        for (thread = s->value->threads; thread; thread = thread->next) {
> > +            monitor_printf(mon, "    CPU #%" PRId64 ":", thread->value->CPU);
> > +            monitor_printf(mon, " nip=0x%016" PRIx64,
> > +                           thread->value->u.ppc->nip);
> 
> This uses value->u.ppc without first checking that the discriminator
> value->arch is set to CPU_INFO_ARCH_PPC; could that be a problem down
> the road?

Can't think of any potential problems as this command is PowerPC
specific.

BTW can you please let me know what else is needed from QEMU end to
drive this PowerPC CPU core device hotplug from libvirt ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[Eduardo Habkost]

On Fri, Jan 29, 2016 at 02:52:30PM +1100, David Gibson wrote:
> On Thu, Jan 28, 2016 at 11:19:43AM +0530, Bharata B Rao wrote:
> > Prevent guests from booting with CPU topologies that have partially
> > filled CPU cores or can result in partially filled CPU cores after
> > CPU hotplug like
> > 
> > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> > 
> > This is enforced by introducing MachineClass::validate_smp_config()
> > that gets called from generic SMP parsing code. Machine type versions
> > that want to enforce this can define this to the generic version
> > provided.
> > 
> > Only sPAPR and PC machine types starting from version 2.6 enforce this in
> > this patch.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> I've been kind of lost in the back and forth about
> threads/cores/sockets.
> 
> What, in the end, is the rationale for allowing partially filled
> sockets, but not partially filled cores?

I don't think there's a good reason for that (at least for PC).

It's easier to relax the requirements later if necessary, than
dealing with compatibility issues again when making the code more
strict. So I suggest we make validate_smp_config_generic() also
check if smp_cpus % (smp_threads * smp_cores) == 0.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[Igor Mammedov]

On Fri, 29 Jan 2016 12:24:18 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jan 29, 2016 at 02:52:30PM +1100, David Gibson wrote:
> > On Thu, Jan 28, 2016 at 11:19:43AM +0530, Bharata B Rao wrote:  
> > > Prevent guests from booting with CPU topologies that have partially
> > > filled CPU cores or can result in partially filled CPU cores after
> > > CPU hotplug like
> > > 
> > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> > > 
> > > This is enforced by introducing MachineClass::validate_smp_config()
> > > that gets called from generic SMP parsing code. Machine type versions
> > > that want to enforce this can define this to the generic version
> > > provided.
> > > 
> > > Only sPAPR and PC machine types starting from version 2.6 enforce this in
> > > this patch.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>  
> > 
> > I've been kind of lost in the back and forth about
> > threads/cores/sockets.
> > 
> > What, in the end, is the rationale for allowing partially filled
> > sockets, but not partially filled cores?  
> 
> I don't think there's a good reason for that (at least for PC).
> 
> It's easier to relax the requirements later if necessary, than
> dealing with compatibility issues again when making the code more
> strict. So I suggest we make validate_smp_config_generic() also
> check if smp_cpus % (smp_threads * smp_cores) == 0.

that would break exiting setups.

Also in case of cpu hotplug this patch will break migration
as target QEMU might refuse starting with hotplugged CPU thread.

Perhaps this check should be enforced per target/machine if
arch requires it.

Re: [Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[Eduardo Habkost]

On Fri, Jan 29, 2016 at 04:10:47PM +0100, Igor Mammedov wrote:
> On Fri, 29 Jan 2016 12:24:18 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Jan 29, 2016 at 02:52:30PM +1100, David Gibson wrote:
> > > On Thu, Jan 28, 2016 at 11:19:43AM +0530, Bharata B Rao wrote:  
> > > > Prevent guests from booting with CPU topologies that have partially
> > > > filled CPU cores or can result in partially filled CPU cores after
> > > > CPU hotplug like
> > > > 
> > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> > > > 
> > > > This is enforced by introducing MachineClass::validate_smp_config()
> > > > that gets called from generic SMP parsing code. Machine type versions
> > > > that want to enforce this can define this to the generic version
> > > > provided.
> > > > 
> > > > Only sPAPR and PC machine types starting from version 2.6 enforce this in
> > > > this patch.
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>  
> > > 
> > > I've been kind of lost in the back and forth about
> > > threads/cores/sockets.
> > > 
> > > What, in the end, is the rationale for allowing partially filled
> > > sockets, but not partially filled cores?  
> > 
> > I don't think there's a good reason for that (at least for PC).
> > 
> > It's easier to relax the requirements later if necessary, than
> > dealing with compatibility issues again when making the code more
> > strict. So I suggest we make validate_smp_config_generic() also
> > check if smp_cpus % (smp_threads * smp_cores) == 0.
> 
> that would break exiting setups.

Not if we do that only on newer machine classes.
validate_smp_config_generic() will be used only on *-2.6 and
newer.


> 
> Also in case of cpu hotplug this patch will break migration
> as target QEMU might refuse starting with hotplugged CPU thread.

This won't change older machine-types.

But I think you are right: it can break migration on pc-2.6, too.
But: isn't migration already broken when creating other sets of
CPUs that can't represented using -smp?

How exactly would you migrate a machine today, if you run:

  $ qemu-system-x86_64 -smp 16,sockets=2,cores=2,threads=2,maxcpus=32
  (QMP) cpu-add id=31


> 
> Perhaps this check should be enforced per target/machine if
> arch requires it.

It is. Please see the patch. It introduces a validate_smp_config
method.

But we need your input to clarify if
validate_smp_config_generic() is safe for pc-2.6 too.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 12/13] qmp: Add query-ppc-cpu-cores command

[Igor Mammedov]

On Thu, 28 Jan 2016 11:19:54 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Show the details of PPC CPU cores via a new QMP command.
> 
> TODO: update qmp-commands.hx with example
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/cpu-core.c               | 77 +++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json                | 31 +++++++++++++++++
>  qmp-commands.hx                 | 51 +++++++++++++++++++++++++++
>  stubs/Makefile.objs             |  1 +
>  stubs/qmp_query_ppc_cpu_cores.c | 10 ++++++
>  5 files changed, 170 insertions(+)
>  create mode 100644 stubs/qmp_query_ppc_cpu_cores.c
> 
> diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
> index aa96e79..652a5aa 100644
> --- a/hw/ppc/cpu-core.c
> +++ b/hw/ppc/cpu-core.c
> @@ -9,7 +9,84 @@
>  #include "hw/ppc/cpu-core.h"
>  #include "hw/boards.h"
>  #include <sysemu/cpus.h>
> +#include <sysemu/kvm.h>
>  #include "qemu/error-report.h"
> +#include "qmp-commands.h"
> +
> +/*
> + * QMP: info ppc-cpu-cores
> + */
> +static int qmp_ppc_cpu_list(Object *obj, void *opaque)
> +{
> +    CpuInfoList ***prev = opaque;
> +
> +    if (object_dynamic_cast(obj, TYPE_POWERPC_CPU)) {
> +        CpuInfoList *elem = g_new0(CpuInfoList, 1);
> +        CpuInfo *s = g_new0(CpuInfo, 1);
> +        CPUState *cs = CPU(obj);
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +        CPUPPCState *env = &cpu->env;
> +
> +        cpu_synchronize_state(cs);
> +        s->arch = CPU_INFO_ARCH_PPC;
> +        s->current = (cs == first_cpu);
> +        s->CPU = cs->cpu_index;
> +        s->qom_path = object_get_canonical_path(obj);
> +        s->halted = cs->halted;
> +        s->thread_id = cs->thread_id;
> +        s->u.ppc = g_new0(CpuInfoPPC, 1);
> +        s->u.ppc->nip = env->nip;
> +
> +        elem->value = s;
> +        elem->next = NULL;
> +        **prev = elem;
> +        *prev = &elem->next;
> +    }
> +    object_child_foreach(obj, qmp_ppc_cpu_list, opaque);
> +    return 0;
> +}
> +
> +static int qmp_ppc_cpu_core_list(Object *obj, void *opaque)
> +{
> +    PPCCPUCoreList ***prev = opaque;
> +
> +    if (object_dynamic_cast(obj, TYPE_POWERPC_CPU_CORE)) {
> +        DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +        DeviceState *dev = DEVICE(obj);
> +
> +        if (dev->realized) {
> +            PPCCPUCoreList *elem = g_new0(PPCCPUCoreList, 1);
> +            PPCCPUCore *s = g_new0(PPCCPUCore, 1);
> +            CpuInfoList *cpu_head = NULL;
> +            CpuInfoList **cpu_prev = &cpu_head;
> +
> +            if (dev->id) {
> +                s->has_id = true;
> +                s->id = g_strdup(dev->id);
> +            }
> +            s->hotplugged = dev->hotplugged;
> +            s->hotpluggable = dc->hotpluggable;
> +            qmp_ppc_cpu_list(obj, &cpu_prev);
> +            s->threads = cpu_head;
> +            elem->value = s;
> +            elem->next = NULL;
> +            **prev = elem;
> +            *prev = &elem->next;
> +        }
> +    }
> +
> +    object_child_foreach(obj, qmp_ppc_cpu_core_list, opaque);
> +    return 0;
> +}
> +
> +PPCCPUCoreList *qmp_query_ppc_cpu_cores(Error **errp)
> +{
> +    PPCCPUCoreList *head = NULL;
> +    PPCCPUCoreList **prev = &head;
> +
> +    qmp_ppc_cpu_core_list(qdev_get_machine(), &prev);
> +    return head;
> +}
>  
>  static int ppc_cpu_core_realize_child(Object *child, void *opaque)
>  {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8d04897..0902697 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4083,3 +4083,34 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @PPCCPUCore:
> +#
> +# Information about PPC CPU core devices
> +#
> +# @hotplugged: true if device was hotplugged
> +#
> +# @hotpluggable: true if device if could be added/removed while machine is running
> +#
> +# Since: 2.6
> +##
> +
> +{ 'struct': 'PPCCPUCore',
> +  'data': { '*id': 'str',
> +            'hotplugged': 'bool',
> +            'hotpluggable': 'bool',
> +            'threads' : ['CpuInfo']
> +          }
> +}
Could it be made more arch independent?
Perhaps it might make sense to replace 'threads'
with qom-path so tools could inspect it in more detail
if needed?

Also looking from cpu hotplug pov it would be nice
to have at top level
  - device type that tools could use with device_add
  - display supported least granularity from topology pov
    like node,socket[,core,[thread]] 'address' parameters
  - display in CPU list also possible CPUs where only
    'type' and 'address' parameters are present.

so above could look like:
{ 'struct': 'CPU',
  'data': {
            'type': 'str'
            'node': 'int',
            'socket': 'int',
            '*core' : 'int',
            '*thread' : 'int',
            '*id': 'str',
            '*hotplugged': 'bool',
            '*hotpluggable': 'bool',
            '*qom-path' : 'str'
          }
}

in addition qom-path could replaced with generic {CPUCore{CPUThread,...},...},
where CPUThread is CPUInfo, I'm not sure if CPUCore could be made generic.

> +
> +##
> +# @query-ppc-cpu-core:
> +#
> +# Returns information for all PPC CPU core devices
> +#
> +# Returns: a list of @PPCCPUCore.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'query-ppc-cpu-cores', 'returns': ['PPCCPUCore'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index db072a6..77cda3c 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4795,3 +4795,54 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +#if defined TARGET_PPC64
> +    {
> +        .name       = "query-ppc-cpu-cores",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_query_ppc_cpu_cores,
> +    },
> +#endif
> +
> +SQMP
> +@query-ppc-cpu-cores
> +--------------------
> +
> +Show PowerPC CPU core devices information.
> +
> +Example:
> +-> { "execute": "query-ppc-cpu-cores" }
> +<- {"return": [{"threads": [
> +                 {"arch": "ppc",
> +                  "current": false,
> +                  "CPU": 16,
> +                  "nip": -4611686018426944644,
> +                  "qom_path": "/machine/peripheral/core2/thread[0]",
> +                  "halted": false,
> +                  "thread_id": 32636},
> +                 {"arch": "ppc",
> +                  "current": false",
> +                  "CPU": 17,
> +                  "nip": -4611686018426944644,
> +                  "qom_path": "/machine/peripheral/core2/thread[1]",
> +                  "halted": false, "thread_id": 32637},
> +                 {"arch": "ppc",
> +                  "current": false,
> +                  "CPU": 18,
> +                  "nip": -4611686018426944644,
> +                  "qom_path": "/machine/peripheral/core2/thread[2]",
> +                  "halted": false,
> +                  "thread_id": 32638},
> +                 {"arch": "ppc",
> +                  "current": false,
> +                  "CPU": 19,
> +                  "nip": -4611686018426944644,
> +                  "qom_path": "/machine/peripheral/core2/thread[3]",
> +                  "halted": false,
> +                  "thread_id": 32639}],
> +                "hotplugged": false,
> +                "hotpluggable": true,
> +                "id": "core2"}
> +              ]}
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index d7898a0..1d65999 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -38,3 +38,4 @@ stub-obj-y += qmp_pc_dimm_device_list.o
>  stub-obj-y += target-monitor-defs.o
>  stub-obj-y += target-get-monitor-def.o
>  stub-obj-y += vhost.o
> +stub-obj-y += qmp_query_ppc_cpu_cores.o
> diff --git a/stubs/qmp_query_ppc_cpu_cores.c b/stubs/qmp_query_ppc_cpu_cores.c
> new file mode 100644
> index 0000000..6a875f0
> --- /dev/null
> +++ b/stubs/qmp_query_ppc_cpu_cores.c
> @@ -0,0 +1,10 @@
> +#include "qom/object.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qemu/typedefs.h"
> +#include "qmp-commands.h"
> +
> +PPCCPUCoreList *qmp_query_ppc_cpu_cores(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return 0;
> +}

Re: [Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[Igor Mammedov]

On Fri, 29 Jan 2016 13:36:05 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jan 29, 2016 at 04:10:47PM +0100, Igor Mammedov wrote:
> > On Fri, 29 Jan 2016 12:24:18 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Fri, Jan 29, 2016 at 02:52:30PM +1100, David Gibson wrote:  
> > > > On Thu, Jan 28, 2016 at 11:19:43AM +0530, Bharata B Rao wrote:    
> > > > > Prevent guests from booting with CPU topologies that have partially
> > > > > filled CPU cores or can result in partially filled CPU cores after
> > > > > CPU hotplug like
> > > > > 
> > > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> > > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> > > > > 
> > > > > This is enforced by introducing MachineClass::validate_smp_config()
> > > > > that gets called from generic SMP parsing code. Machine type versions
> > > > > that want to enforce this can define this to the generic version
> > > > > provided.
> > > > > 
> > > > > Only sPAPR and PC machine types starting from version 2.6 enforce this in
> > > > > this patch.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>    
> > > > 
> > > > I've been kind of lost in the back and forth about
> > > > threads/cores/sockets.
> > > > 
> > > > What, in the end, is the rationale for allowing partially filled
> > > > sockets, but not partially filled cores?    
> > > 
> > > I don't think there's a good reason for that (at least for PC).
> > > 
> > > It's easier to relax the requirements later if necessary, than
> > > dealing with compatibility issues again when making the code more
> > > strict. So I suggest we make validate_smp_config_generic() also
> > > check if smp_cpus % (smp_threads * smp_cores) == 0.  
> > 
> > that would break exiting setups.  
> 
> Not if we do that only on newer machine classes.
> validate_smp_config_generic() will be used only on *-2.6 and
> newer.
> 
> 
> > 
> > Also in case of cpu hotplug this patch will break migration
> > as target QEMU might refuse starting with hotplugged CPU thread.  
> 
> This won't change older machine-types.
> 
> But I think you are right: it can break migration on pc-2.6, too.
> But: isn't migration already broken when creating other sets of
> CPUs that can't represented using -smp?
> 
> How exactly would you migrate a machine today, if you run:
> 
>   $ qemu-system-x86_64 -smp 16,sockets=2,cores=2,threads=2,maxcpus=32
>   (QMP) cpu-add id=31
that's invalid topology and should exit with error at start-up,
however it shouldn't be smp_cpus vs sockets,cores,threads check
but rather max_cpus vs sockets,cores,threads,maxcpus check.
something like this:

diff --git a/vl.c b/vl.c
index f043009..3afa0b6 100644
--- a/vl.c
+++ b/vl.c
@@ -1239,9 +1239,9 @@ static void smp_parse(QemuOpts *opts)
         }
 
         max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
-        if (sockets * cores * threads > max_cpus) {
-            error_report("cpu topology: "
-                         "sockets (%u) * cores (%u) * threads (%u) > "
+        if (sockets * cores * threads == max_cpus) {
+            error_report("invalid cpu topology: "
+                         "sockets (%u) * cores (%u) * threads (%u) not equal "
                          "maxcpus (%u)",
                          sockets, cores, threads, max_cpus);
             exit(1);

> 
> 
> > 
> > Perhaps this check should be enforced per target/machine if
> > arch requires it.  
> 
> It is. Please see the patch. It introduces a validate_smp_config
> method.
> 
> But we need your input to clarify if
> validate_smp_config_generic() is safe for pc-2.6 too.
it breaks migration as it could prevent target from starting if
there is hotplugged CPUs on source side.

Re: [Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[Eduardo Habkost]

On Fri, Jan 29, 2016 at 05:52:15PM +0100, Igor Mammedov wrote:
> On Fri, 29 Jan 2016 13:36:05 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Jan 29, 2016 at 04:10:47PM +0100, Igor Mammedov wrote:
> > > On Fri, 29 Jan 2016 12:24:18 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Fri, Jan 29, 2016 at 02:52:30PM +1100, David Gibson wrote:  
> > > > > On Thu, Jan 28, 2016 at 11:19:43AM +0530, Bharata B Rao wrote:    
> > > > > > Prevent guests from booting with CPU topologies that have partially
> > > > > > filled CPU cores or can result in partially filled CPU cores after
> > > > > > CPU hotplug like
> > > > > > 
> > > > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> > > > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> > > > > > 
> > > > > > This is enforced by introducing MachineClass::validate_smp_config()
> > > > > > that gets called from generic SMP parsing code. Machine type versions
> > > > > > that want to enforce this can define this to the generic version
> > > > > > provided.
> > > > > > 
> > > > > > Only sPAPR and PC machine types starting from version 2.6 enforce this in
> > > > > > this patch.
> > > > > > 
> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>    
> > > > > 
> > > > > I've been kind of lost in the back and forth about
> > > > > threads/cores/sockets.
> > > > > 
> > > > > What, in the end, is the rationale for allowing partially filled
> > > > > sockets, but not partially filled cores?    
> > > > 
> > > > I don't think there's a good reason for that (at least for PC).
> > > > 
> > > > It's easier to relax the requirements later if necessary, than
> > > > dealing with compatibility issues again when making the code more
> > > > strict. So I suggest we make validate_smp_config_generic() also
> > > > check if smp_cpus % (smp_threads * smp_cores) == 0.  
> > > 
> > > that would break exiting setups.  
> > 
> > Not if we do that only on newer machine classes.
> > validate_smp_config_generic() will be used only on *-2.6 and
> > newer.
> > 
> > 
> > > 
> > > Also in case of cpu hotplug this patch will break migration
> > > as target QEMU might refuse starting with hotplugged CPU thread.  
> > 
> > This won't change older machine-types.
> > 
> > But I think you are right: it can break migration on pc-2.6, too.
> > But: isn't migration already broken when creating other sets of
> > CPUs that can't represented using -smp?
> > 
> > How exactly would you migrate a machine today, if you run:
> > 
> >   $ qemu-system-x86_64 -smp 16,sockets=2,cores=2,threads=2,maxcpus=32
> >   (QMP) cpu-add id=31
> that's invalid topology and should exit with error at start-up,

Oops, my mistake. Now the same question with the right numbers:

How exactly would you migrate a machine today, if you do the
following?

  $ qemu-system-x86_64 -smp 8,sockets=2,cores=2,threads=2,maxcpus=16
  (QMP) cpu-add id=15


> however it shouldn't be smp_cpus vs sockets,cores,threads check
> but rather max_cpus vs sockets,cores,threads,maxcpus check.
> something like this:
> 
> diff --git a/vl.c b/vl.c
> index f043009..3afa0b6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1239,9 +1239,9 @@ static void smp_parse(QemuOpts *opts)
>          }
>  
>          max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> -        if (sockets * cores * threads > max_cpus) {
> -            error_report("cpu topology: "
> -                         "sockets (%u) * cores (%u) * threads (%u) > "
> +        if (sockets * cores * threads == max_cpus) {
> +            error_report("invalid cpu topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) not equal "
>                           "maxcpus (%u)",
>                           sockets, cores, threads, max_cpus);
>              exit(1);
> 
> > 
> > 
> > > 
> > > Perhaps this check should be enforced per target/machine if
> > > arch requires it.  
> > 
> > It is. Please see the patch. It introduces a validate_smp_config
> > method.
> > 
> > But we need your input to clarify if
> > validate_smp_config_generic() is safe for pc-2.6 too.
> it breaks migration as it could prevent target from starting if
> there is hotplugged CPUs on source side.

It looks like this is a problem only if the machine allows
hotplug of individual threads. What if we just add this to the
beginning of validate_smp_config_generic():

    if (mc->hot_add_cpu && max_cpus > smp_cpus) {
        /* hot_add_cpu() allows hotplug of individual threads,
         * allowing incomplete cores/sockets, so we can't prevent
         * it from running.
         */
         return 0;
    }

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 08/13] target-ppc: Introduce PowerPC specific CPU core device

[David Gibson]

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

On Thu, Jan 28, 2016 at 11:19:50AM +0530, Bharata B Rao wrote:
> CPU core device is a container of CPU thread devices.  CPU hotplug is
> performed at the granularity of CPU core device. When hotplugged, CPU core
> creates CPU thread devices.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

The basic logic here seems fine.

However, I'm not really convinced that a "powerpc generic" device
makes sense here.  Given the diversity of powerpc platforms, it's
pretty plausible they could have very different restrictions about how
hotplug needs to operate.

So, I think it would make more sense to go for more specific devices
that either represent real hardware chips / modules, or something else
that makes sense for a specific platform.  So, in the first instance a
"PAPR virtual module" device.

It would make sense to have that derive from a more-or-less generic
base class with essentially the logic below, since even if we have
significantly different variants they're likely to share the basic
logic.  But I don't think that base type should be directly
instantiable.

Of course, once that's the case, there's really nothing ppc specific
about the base device either.

> ---
>  hw/ppc/Makefile.objs      |  1 +
>  hw/ppc/cpu-core.c         | 75 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/cpu-core.h | 33 +++++++++++++++++++++
>  3 files changed, 109 insertions(+)
>  create mode 100644 hw/ppc/cpu-core.c
>  create mode 100644 include/hw/ppc/cpu-core.h
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index c1ffc77..a6b7cfb 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -21,3 +21,4 @@ obj-$(CONFIG_E500) += e500.o mpc8544ds.o e500plat.o
>  obj-$(CONFIG_E500) += mpc8544_guts.o ppce500_spin.o
>  # PowerPC 440 Xilinx ML507 reference board.
>  obj-$(CONFIG_XILINX) += virtex_ml507.o
> +obj-y += cpu-core.o
> diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
> new file mode 100644
> index 0000000..aa96e79
> --- /dev/null
> +++ b/hw/ppc/cpu-core.c
> @@ -0,0 +1,75 @@
> +/*
> + * PowerPC 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/ppc/cpu-core.h"
> +#include "hw/boards.h"
> +#include <sysemu/cpus.h>
> +#include "qemu/error-report.h"
> +
> +static int ppc_cpu_core_realize_child(Object *child, void *opaque)
> +{
> +    Error **errp = opaque;
> +
> +    object_property_set_bool(child, true, "realized", errp);
> +    if (*errp) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static void ppc_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> +    object_child_foreach(OBJECT(dev), ppc_cpu_core_realize_child, errp);
> +}
> +
> +static void ppc_cpu_core_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = ppc_cpu_core_realize;
> +    dc->desc = "PowerPC CPU core";
> +}
> +
> +static void ppc_cpu_core_instance_init(Object *obj)
> +{
> +    int i;
> +    CPUState *cpu;
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    PowerPCCPUCore *core = POWERPC_CPU_CORE(obj);
> +
> +    /* Create as many CPU threads as specified in the topology */
> +    for (i = 0; i < smp_threads; i++) {
> +        cpu = cpu_generic_init(TYPE_POWERPC_CPU, machine->cpu_model);
> +        if (!cpu) {
> +            error_report("Unable to find CPU definition: %s",
> +                          machine->cpu_model);
> +            exit(EXIT_FAILURE);
> +        }
> +        object_property_add_child(obj, "thread[*]", OBJECT(cpu), &error_abort);
> +        object_unref(OBJECT(cpu));
> +        if (!i) {
> +            core->thread0 = POWERPC_CPU(cpu);
> +        }
> +    }
> +}
> +
> +static const TypeInfo ppc_cpu_core_type_info = {
> +    .name = TYPE_POWERPC_CPU_CORE,
> +    .parent = TYPE_DEVICE,
> +    .class_init = ppc_cpu_core_class_init,
> +    .instance_init = ppc_cpu_core_instance_init,
> +    .instance_size = sizeof(PowerPCCPUCore),
> +};
> +
> +static void cpu_core_register_types(void)
> +{
> +    type_register_static(&ppc_cpu_core_type_info);
> +}
> +
> +type_init(cpu_core_register_types)
> diff --git a/include/hw/ppc/cpu-core.h b/include/hw/ppc/cpu-core.h
> new file mode 100644
> index 0000000..ff2ebc2
> --- /dev/null
> +++ b/include/hw/ppc/cpu-core.h
> @@ -0,0 +1,33 @@
> +/*
> + * 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_PPC_CPU_CORE_H
> +#define HW_PPC_CPU_CORE_H
> +
> +#include "hw/qdev.h"
> +
> +#ifdef TARGET_PPC64
> +#define TYPE_POWERPC_CPU_CORE "powerpc64-cpu-core"
> +#elif defined(TARGET_PPCEMB)
> +#define TYPE_POWERPC_CPU_CORE "embedded-powerpc-cpu-core"
> +#else
> +#define TYPE_POWERPC_CPU_CORE "powerpc-cpu-core"
> +#endif
> +
> +#define POWERPC_CPU_CORE(obj) \
> +    OBJECT_CHECK(PowerPCCPUCore, (obj), TYPE_POWERPC_CPU_CORE)
> +
> +typedef struct PowerPCCPUCore {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +    /*< public >*/
> +
> +    PowerPCCPU *thread0;
> +} PowerPCCPUCore;
> +
> +#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] [PATCH v7 10/13] spapr: CPU hotplug support

[David Gibson]

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

On Thu, Jan 28, 2016 at 11:19:52AM +0530, Bharata B Rao wrote:
> Support CPU hotplug via device-add command like this:
> 
> (qemu) device_add powerpc64-cpu-core,id=core2
> 
> In response to device_add, CPU core device will be created. CPU core
> device creates and realizes CPU thread devices. If the machine type
> supports CPU hotplug, boot-time CPUs are created as CPU core devices
> otherwise they continue to be created as individual CPU devices.
> 
> Set up device tree entries for the hotplugged CPU core and use the
> exising EPOW event 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>

The logic here looks basically sound.  Elswhere however I have
comments on the socket / core / package / whatnot interfaces that I'm
not entirely convinced by.  Changes there are likely to require
changes here, although with luck not too big.

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

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

Re: [Qemu-devel] [PATCH v7 11/13] spapr: CPU hot unplug support

[David Gibson]

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

On Thu, Jan 28, 2016 at 11:19:53AM +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 supported by sending the hot unplug
> notification to the guest via EPOW interrupt. 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>

Again, the PAPR specific and DRC logic looks sound, but I'm not
entirely convinced by the overall structure it fits into.

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

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

Re: [Qemu-devel] [PATCH v7 12/13] qmp: Add query-ppc-cpu-cores command

[Bharata B Rao]

On Fri, Jan 29, 2016 at 04:45:06PM +0100, Igor Mammedov wrote:
> On Thu, 28 Jan 2016 11:19:54 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Show the details of PPC CPU cores via a new QMP command.
> > 
> > TODO: update qmp-commands.hx with example
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/cpu-core.c               | 77 +++++++++++++++++++++++++++++++++++++++++
> >  qapi-schema.json                | 31 +++++++++++++++++
> >  qmp-commands.hx                 | 51 +++++++++++++++++++++++++++
> >  stubs/Makefile.objs             |  1 +
> >  stubs/qmp_query_ppc_cpu_cores.c | 10 ++++++
> >  5 files changed, 170 insertions(+)
> >  create mode 100644 stubs/qmp_query_ppc_cpu_cores.c
> > 
> > diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
> > index aa96e79..652a5aa 100644
> > --- a/hw/ppc/cpu-core.c
> > +++ b/hw/ppc/cpu-core.c
> > @@ -9,7 +9,84 @@
> >  #include "hw/ppc/cpu-core.h"
> >  #include "hw/boards.h"
> >  #include <sysemu/cpus.h>
> > +#include <sysemu/kvm.h>
> >  #include "qemu/error-report.h"
> > +#include "qmp-commands.h"
> > +
> > +/*
> > + * QMP: info ppc-cpu-cores
> > + */
> > +static int qmp_ppc_cpu_list(Object *obj, void *opaque)
> > +{
> > +    CpuInfoList ***prev = opaque;
> > +
> > +    if (object_dynamic_cast(obj, TYPE_POWERPC_CPU)) {
> > +        CpuInfoList *elem = g_new0(CpuInfoList, 1);
> > +        CpuInfo *s = g_new0(CpuInfo, 1);
> > +        CPUState *cs = CPU(obj);
> > +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +        CPUPPCState *env = &cpu->env;
> > +
> > +        cpu_synchronize_state(cs);
> > +        s->arch = CPU_INFO_ARCH_PPC;
> > +        s->current = (cs == first_cpu);
> > +        s->CPU = cs->cpu_index;
> > +        s->qom_path = object_get_canonical_path(obj);
> > +        s->halted = cs->halted;
> > +        s->thread_id = cs->thread_id;
> > +        s->u.ppc = g_new0(CpuInfoPPC, 1);
> > +        s->u.ppc->nip = env->nip;
> > +
> > +        elem->value = s;
> > +        elem->next = NULL;
> > +        **prev = elem;
> > +        *prev = &elem->next;
> > +    }
> > +    object_child_foreach(obj, qmp_ppc_cpu_list, opaque);
> > +    return 0;
> > +}
> > +
> > +static int qmp_ppc_cpu_core_list(Object *obj, void *opaque)
> > +{
> > +    PPCCPUCoreList ***prev = opaque;
> > +
> > +    if (object_dynamic_cast(obj, TYPE_POWERPC_CPU_CORE)) {
> > +        DeviceClass *dc = DEVICE_GET_CLASS(obj);
> > +        DeviceState *dev = DEVICE(obj);
> > +
> > +        if (dev->realized) {
> > +            PPCCPUCoreList *elem = g_new0(PPCCPUCoreList, 1);
> > +            PPCCPUCore *s = g_new0(PPCCPUCore, 1);
> > +            CpuInfoList *cpu_head = NULL;
> > +            CpuInfoList **cpu_prev = &cpu_head;
> > +
> > +            if (dev->id) {
> > +                s->has_id = true;
> > +                s->id = g_strdup(dev->id);
> > +            }
> > +            s->hotplugged = dev->hotplugged;
> > +            s->hotpluggable = dc->hotpluggable;
> > +            qmp_ppc_cpu_list(obj, &cpu_prev);
> > +            s->threads = cpu_head;
> > +            elem->value = s;
> > +            elem->next = NULL;
> > +            **prev = elem;
> > +            *prev = &elem->next;
> > +        }
> > +    }
> > +
> > +    object_child_foreach(obj, qmp_ppc_cpu_core_list, opaque);
> > +    return 0;
> > +}
> > +
> > +PPCCPUCoreList *qmp_query_ppc_cpu_cores(Error **errp)
> > +{
> > +    PPCCPUCoreList *head = NULL;
> > +    PPCCPUCoreList **prev = &head;
> > +
> > +    qmp_ppc_cpu_core_list(qdev_get_machine(), &prev);
> > +    return head;
> > +}
> >  
> >  static int ppc_cpu_core_realize_child(Object *child, void *opaque)
> >  {
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 8d04897..0902697 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4083,3 +4083,34 @@
> >  ##
> >  { 'enum': 'ReplayMode',
> >    'data': [ 'none', 'record', 'play' ] }
> > +
> > +##
> > +# @PPCCPUCore:
> > +#
> > +# Information about PPC CPU core devices
> > +#
> > +# @hotplugged: true if device was hotplugged
> > +#
> > +# @hotpluggable: true if device if could be added/removed while machine is running
> > +#
> > +# Since: 2.6
> > +##
> > +
> > +{ 'struct': 'PPCCPUCore',
> > +  'data': { '*id': 'str',
> > +            'hotplugged': 'bool',
> > +            'hotpluggable': 'bool',
> > +            'threads' : ['CpuInfo']
> > +          }
> > +}
> Could it be made more arch independent?

Except that it is called PPCCPUCore, the fields are actually arch
neutral with 'threads' element defined as CpuInfo that gets interpreted
as arch-specific CpuInfo based on the target architecture.

In any case, this patchset adds PowerPC specific CPU core device that
sPAPR target implements. This is kept arch-specific in order to make it
more acceptable in short term in case arch-neutral, generic CPU hotplug
solutions take long time for reaching consensus.

> Perhaps it might make sense to replace 'threads'
> with qom-path so tools could inspect it in more detail
> if needed?

Hmm 'threads' is of CpuInfo type which already has qom-path inside it.
Did I get your question right ?

> 
> Also looking from cpu hotplug pov it would be nice
> to have at top level
>   - device type that tools could use with device_add
>   - display supported least granularity from topology pov
>     like node,socket[,core,[thread]] 'address' parameters
>   - display in CPU list also possible CPUs where only
>     'type' and 'address' parameters are present.
> 
> so above could look like:
> { 'struct': 'CPU',
>   'data': {
>             'type': 'str'
>             'node': 'int',
>             'socket': 'int',
>             '*core' : 'int',
>             '*thread' : 'int',
>             '*id': 'str',
>             '*hotplugged': 'bool',
>             '*hotpluggable': 'bool',
>             '*qom-path' : 'str'
>           }
> }

This is PowerPC specific where only core granularity hotplug makes
sense, but if it helps libvirt or other tools, I could add those fields.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[Igor Mammedov]

On Fri, 29 Jan 2016 15:24:12 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jan 29, 2016 at 05:52:15PM +0100, Igor Mammedov wrote:
> > On Fri, 29 Jan 2016 13:36:05 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Fri, Jan 29, 2016 at 04:10:47PM +0100, Igor Mammedov wrote:  
> > > > On Fri, 29 Jan 2016 12:24:18 -0200
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Fri, Jan 29, 2016 at 02:52:30PM +1100, David Gibson wrote:    
> > > > > > On Thu, Jan 28, 2016 at 11:19:43AM +0530, Bharata B Rao wrote:      
> > > > > > > Prevent guests from booting with CPU topologies that have partially
> > > > > > > filled CPU cores or can result in partially filled CPU cores after
> > > > > > > CPU hotplug like
> > > > > > > 
> > > > > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=16 or
> > > > > > > -smp 15,sockets=1,cores=4,threads=4,maxcpus=17.
> > > > > > > 
> > > > > > > This is enforced by introducing MachineClass::validate_smp_config()
> > > > > > > that gets called from generic SMP parsing code. Machine type versions
> > > > > > > that want to enforce this can define this to the generic version
> > > > > > > provided.
> > > > > > > 
> > > > > > > Only sPAPR and PC machine types starting from version 2.6 enforce this in
> > > > > > > this patch.
> > > > > > > 
> > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>      
> > > > > > 
> > > > > > I've been kind of lost in the back and forth about
> > > > > > threads/cores/sockets.
> > > > > > 
> > > > > > What, in the end, is the rationale for allowing partially filled
> > > > > > sockets, but not partially filled cores?      
> > > > > 
> > > > > I don't think there's a good reason for that (at least for PC).
> > > > > 
> > > > > It's easier to relax the requirements later if necessary, than
> > > > > dealing with compatibility issues again when making the code more
> > > > > strict. So I suggest we make validate_smp_config_generic() also
> > > > > check if smp_cpus % (smp_threads * smp_cores) == 0.    
> > > > 
> > > > that would break exiting setups.    
> > > 
> > > Not if we do that only on newer machine classes.
> > > validate_smp_config_generic() will be used only on *-2.6 and
> > > newer.
> > > 
> > >   
> > > > 
> > > > Also in case of cpu hotplug this patch will break migration
> > > > as target QEMU might refuse starting with hotplugged CPU thread.    
> > > 
> > > This won't change older machine-types.
> > > 
> > > But I think you are right: it can break migration on pc-2.6, too.
> > > But: isn't migration already broken when creating other sets of
> > > CPUs that can't represented using -smp?
> > > 
> > > How exactly would you migrate a machine today, if you run:
> > > 
> > >   $ qemu-system-x86_64 -smp 16,sockets=2,cores=2,threads=2,maxcpus=32
> > >   (QMP) cpu-add id=31  
> > that's invalid topology and should exit with error at start-up,  
> 
> Oops, my mistake. Now the same question with the right numbers:
> 
> How exactly would you migrate a machine today, if you do the
> following?
> 
>   $ qemu-system-x86_64 -smp 8,sockets=2,cores=2,threads=2,maxcpus=16
>   (QMP) cpu-add id=15
isn't it the same broken topology? 
  sockets*cores*threads != maxcpus
But if you ask if it's possible to migrate machine with non-sequentially
hotplugged CPUs than answer is no it's not possible with cpu-add.

> > however it shouldn't be smp_cpus vs sockets,cores,threads check
> > but rather max_cpus vs sockets,cores,threads,maxcpus check.
> > something like this:
> > 
> > diff --git a/vl.c b/vl.c
> > index f043009..3afa0b6 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1239,9 +1239,9 @@ static void smp_parse(QemuOpts *opts)
> >          }
> >  
> >          max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> > -        if (sockets * cores * threads > max_cpus) {
> > -            error_report("cpu topology: "
> > -                         "sockets (%u) * cores (%u) * threads (%u) > "
> > +        if (sockets * cores * threads == max_cpus) {
> > +            error_report("invalid cpu topology: "
> > +                         "sockets (%u) * cores (%u) * threads (%u) not equal "
> >                           "maxcpus (%u)",
> >                           sockets, cores, threads, max_cpus);
> >              exit(1);
> >   
> > > 
> > >   
> > > > 
> > > > Perhaps this check should be enforced per target/machine if
> > > > arch requires it.    
> > > 
> > > It is. Please see the patch. It introduces a validate_smp_config
> > > method.
> > > 
> > > But we need your input to clarify if
> > > validate_smp_config_generic() is safe for pc-2.6 too.  
> > it breaks migration as it could prevent target from starting if
> > there is hotplugged CPUs on source side.  
> 
> It looks like this is a problem only if the machine allows
> hotplug of individual threads. What if we just add this to the
> beginning of validate_smp_config_generic():
> 
>     if (mc->hot_add_cpu && max_cpus > smp_cpus) {
It would break migration after hotplugging CPUs upto max_cpus
and then trying to migrate.

Also when we move x86 to device_add that will regress since
hot_add_cpu() won't be used in that case.

I think there is not much point enforcing restrictions
in this patch as generic. We should leave hook empty and allow
target to override it. Then SPAPR could enforce it's own limit
on partial cores.

>         /* hot_add_cpu() allows hotplug of individual threads,
>          * allowing incomplete cores/sockets, so we can't prevent
>          * it from running.
>          */
>          return 0;
>     }

Re: [Qemu-devel] [PATCH v7 12/13] qmp: Add query-ppc-cpu-cores command

[Igor Mammedov]

On Mon, 1 Feb 2016 14:13:58 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Fri, Jan 29, 2016 at 04:45:06PM +0100, Igor Mammedov wrote:
> > On Thu, 28 Jan 2016 11:19:54 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > Show the details of PPC CPU cores via a new QMP command.
> > > 
> > > TODO: update qmp-commands.hx with example
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/cpu-core.c               | 77 +++++++++++++++++++++++++++++++++++++++++
> > >  qapi-schema.json                | 31 +++++++++++++++++
> > >  qmp-commands.hx                 | 51 +++++++++++++++++++++++++++
> > >  stubs/Makefile.objs             |  1 +
> > >  stubs/qmp_query_ppc_cpu_cores.c | 10 ++++++
> > >  5 files changed, 170 insertions(+)
> > >  create mode 100644 stubs/qmp_query_ppc_cpu_cores.c
> > > 
> > > diff --git a/hw/ppc/cpu-core.c b/hw/ppc/cpu-core.c
> > > index aa96e79..652a5aa 100644
> > > --- a/hw/ppc/cpu-core.c
> > > +++ b/hw/ppc/cpu-core.c
> > > @@ -9,7 +9,84 @@
> > >  #include "hw/ppc/cpu-core.h"
> > >  #include "hw/boards.h"
> > >  #include <sysemu/cpus.h>
> > > +#include <sysemu/kvm.h>
> > >  #include "qemu/error-report.h"
> > > +#include "qmp-commands.h"
> > > +
> > > +/*
> > > + * QMP: info ppc-cpu-cores
> > > + */
> > > +static int qmp_ppc_cpu_list(Object *obj, void *opaque)
> > > +{
> > > +    CpuInfoList ***prev = opaque;
> > > +
> > > +    if (object_dynamic_cast(obj, TYPE_POWERPC_CPU)) {
> > > +        CpuInfoList *elem = g_new0(CpuInfoList, 1);
> > > +        CpuInfo *s = g_new0(CpuInfo, 1);
> > > +        CPUState *cs = CPU(obj);
> > > +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > +        CPUPPCState *env = &cpu->env;
> > > +
> > > +        cpu_synchronize_state(cs);
> > > +        s->arch = CPU_INFO_ARCH_PPC;
> > > +        s->current = (cs == first_cpu);
> > > +        s->CPU = cs->cpu_index;
> > > +        s->qom_path = object_get_canonical_path(obj);
> > > +        s->halted = cs->halted;
> > > +        s->thread_id = cs->thread_id;
> > > +        s->u.ppc = g_new0(CpuInfoPPC, 1);
> > > +        s->u.ppc->nip = env->nip;
> > > +
> > > +        elem->value = s;
> > > +        elem->next = NULL;
> > > +        **prev = elem;
> > > +        *prev = &elem->next;
> > > +    }
> > > +    object_child_foreach(obj, qmp_ppc_cpu_list, opaque);
> > > +    return 0;
> > > +}
> > > +
> > > +static int qmp_ppc_cpu_core_list(Object *obj, void *opaque)
> > > +{
> > > +    PPCCPUCoreList ***prev = opaque;
> > > +
> > > +    if (object_dynamic_cast(obj, TYPE_POWERPC_CPU_CORE)) {
> > > +        DeviceClass *dc = DEVICE_GET_CLASS(obj);
> > > +        DeviceState *dev = DEVICE(obj);
> > > +
> > > +        if (dev->realized) {
> > > +            PPCCPUCoreList *elem = g_new0(PPCCPUCoreList, 1);
> > > +            PPCCPUCore *s = g_new0(PPCCPUCore, 1);
> > > +            CpuInfoList *cpu_head = NULL;
> > > +            CpuInfoList **cpu_prev = &cpu_head;
> > > +
> > > +            if (dev->id) {
> > > +                s->has_id = true;
> > > +                s->id = g_strdup(dev->id);
> > > +            }
> > > +            s->hotplugged = dev->hotplugged;
> > > +            s->hotpluggable = dc->hotpluggable;
> > > +            qmp_ppc_cpu_list(obj, &cpu_prev);
> > > +            s->threads = cpu_head;
> > > +            elem->value = s;
> > > +            elem->next = NULL;
> > > +            **prev = elem;
> > > +            *prev = &elem->next;
> > > +        }
> > > +    }
> > > +
> > > +    object_child_foreach(obj, qmp_ppc_cpu_core_list, opaque);
> > > +    return 0;
> > > +}
> > > +
> > > +PPCCPUCoreList *qmp_query_ppc_cpu_cores(Error **errp)
> > > +{
> > > +    PPCCPUCoreList *head = NULL;
> > > +    PPCCPUCoreList **prev = &head;
> > > +
> > > +    qmp_ppc_cpu_core_list(qdev_get_machine(), &prev);
> > > +    return head;
> > > +}
> > >  
> > >  static int ppc_cpu_core_realize_child(Object *child, void *opaque)
> > >  {
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 8d04897..0902697 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -4083,3 +4083,34 @@
> > >  ##
> > >  { 'enum': 'ReplayMode',
> > >    'data': [ 'none', 'record', 'play' ] }
> > > +
> > > +##
> > > +# @PPCCPUCore:
> > > +#
> > > +# Information about PPC CPU core devices
> > > +#
> > > +# @hotplugged: true if device was hotplugged
> > > +#
> > > +# @hotpluggable: true if device if could be added/removed while machine is running
> > > +#
> > > +# Since: 2.6
> > > +##
> > > +
> > > +{ 'struct': 'PPCCPUCore',
> > > +  'data': { '*id': 'str',
> > > +            'hotplugged': 'bool',
> > > +            'hotpluggable': 'bool',
> > > +            'threads' : ['CpuInfo']
> > > +          }
> > > +}  
> > Could it be made more arch independent?  
> 
> Except that it is called PPCCPUCore, the fields are actually arch
> neutral with 'threads' element defined as CpuInfo that gets interpreted
> as arch-specific CpuInfo based on the target architecture.
> 
> In any case, this patchset adds PowerPC specific CPU core device that
> sPAPR target implements. This is kept arch-specific in order to make it
> more acceptable in short term in case arch-neutral, generic CPU hotplug
> solutions take long time for reaching consensus.
Yep it's not easy to agree on but this PPC specific you'd have to
keep around and support pretty much forever once it's released,
I hope it would be easier to agree on CPU hotplug specific qmp/hmp
interface.

Wouldn't 'struct' I've suggested below work for you?
It's more or less generic and would work for x86 as well
so less target specific stuff to do in libvirt would also
be a plus.

> 
> > Perhaps it might make sense to replace 'threads'
> > with qom-path so tools could inspect it in more detail
> > if needed?  
> 
> Hmm 'threads' is of CpuInfo type which already has qom-path inside it.
> Did I get your question right ?
> 
> > 
> > Also looking from cpu hotplug pov it would be nice
> > to have at top level
> >   - device type that tools could use with device_add
> >   - display supported least granularity from topology pov
> >     like node,socket[,core,[thread]] 'address' parameters
> >   - display in CPU list also possible CPUs where only
> >     'type' and 'address' parameters are present.
> > 
> > so above could look like:
> > { 'struct': 'CPU',
> >   'data': {
> >             'type': 'str'
> >             'node': 'int',
> >             'socket': 'int',
> >             '*core' : 'int',
> >             '*thread' : 'int',
> >             '*id': 'str',
> >             '*hotplugged': 'bool',
> >             '*hotpluggable': 'bool',
> >             '*qom-path' : 'str'
> >           }
> > }  
> 
> This is PowerPC specific where only core granularity hotplug makes
> sense, but if it helps libvirt or other tools, I could add those fields.
then PPC won't display/allow 'thread' parameter, while x86 will
since it allows thread granularity. For targets that allow only
sockets neither core/thread would be shown.

> 
> Regards,
> Bharata.
> 

Re: [Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[Eduardo Habkost]

On Mon, Feb 01, 2016 at 10:41:58AM +0100, Igor Mammedov wrote:
[...]
> > 
> > How exactly would you migrate a machine today, if you do the
> > following?
> > 
> >   $ qemu-system-x86_64 -smp 8,sockets=2,cores=2,threads=2,maxcpus=16
> >   (QMP) cpu-add id=15
> isn't it the same broken topology? 
>   sockets*cores*threads != maxcpus
> But if you ask if it's possible to migrate machine with non-sequentially
> hotplugged CPUs than answer is no it's not possible with cpu-add.

I was asking about migration with non-sequential hotplugged CPUs
(e.g. with -smp 8,sockets=4,cores=2,threads=2,maxcpus=16).


[...]
> > > > > 
> > > > > Perhaps this check should be enforced per target/machine if
> > > > > arch requires it.    
> > > > 
> > > > It is. Please see the patch. It introduces a validate_smp_config
> > > > method.
> > > > 
> > > > But we need your input to clarify if
> > > > validate_smp_config_generic() is safe for pc-2.6 too.  
> > > it breaks migration as it could prevent target from starting if
> > > there is hotplugged CPUs on source side.  
> > 
> > It looks like this is a problem only if the machine allows
> > hotplug of individual threads. What if we just add this to the
> > beginning of validate_smp_config_generic():
> > 
> >     if (mc->hot_add_cpu && max_cpus > smp_cpus) {
> It would break migration after hotplugging CPUs upto max_cpus
> and then trying to migrate.
> 
> Also when we move x86 to device_add that will regress since
> hot_add_cpu() won't be used in that case.
> 
> I think there is not much point enforcing restrictions
> in this patch as generic. We should leave hook empty and allow
> target to override it. Then SPAPR could enforce it's own limit
> on partial cores.

Agreed. It looks like we won't get rid of thread-based CPU
hotplug in x86 in the foreseable future.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v7 01/13] machine: Don't allow CPU toplogies with partially filled cores

[Igor Mammedov]

On Wed, 3 Feb 2016 15:38:09 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Feb 01, 2016 at 10:41:58AM +0100, Igor Mammedov wrote:
> [...]
> > > 
> > > How exactly would you migrate a machine today, if you do the
> > > following?
> > > 
> > >   $ qemu-system-x86_64 -smp 8,sockets=2,cores=2,threads=2,maxcpus=16
> > >   (QMP) cpu-add id=15  
> > isn't it the same broken topology? 
> >   sockets*cores*threads != maxcpus
> > But if you ask if it's possible to migrate machine with non-sequentially
> > hotplugged CPUs than answer is no it's not possible with cpu-add.  
> 
> I was asking about migration with non-sequential hotplugged CPUs
> (e.g. with -smp 8,sockets=4,cores=2,threads=2,maxcpus=16).
machine with non-sequential hotplugged CPUs can't be migrated currently
since there is not any way to say which CPUs should be created on target.

That should be though possible to do so with -device/device_add
interface once we enable it for x86 CPU threads.
Stumble block there is where to get APIC ID for a CPU (i.e. address
thing which tells where to plug CPU at).
Adding socket,core,thread properties to CPU would help to solve it,
and they look generic enough to put them in CPUClass.

> 
> 
> [...]
> > > > > > 
> > > > > > Perhaps this check should be enforced per target/machine if
> > > > > > arch requires it.      
> > > > > 
> > > > > It is. Please see the patch. It introduces a validate_smp_config
> > > > > method.
> > > > > 
> > > > > But we need your input to clarify if
> > > > > validate_smp_config_generic() is safe for pc-2.6 too.    
> > > > it breaks migration as it could prevent target from starting if
> > > > there is hotplugged CPUs on source side.    
> > > 
> > > It looks like this is a problem only if the machine allows
> > > hotplug of individual threads. What if we just add this to the
> > > beginning of validate_smp_config_generic():
> > > 
> > >     if (mc->hot_add_cpu && max_cpus > smp_cpus) {  
> > It would break migration after hotplugging CPUs upto max_cpus
> > and then trying to migrate.
> > 
> > Also when we move x86 to device_add that will regress since
> > hot_add_cpu() won't be used in that case.
> > 
> > I think there is not much point enforcing restrictions
> > in this patch as generic. We should leave hook empty and allow
> > target to override it. Then SPAPR could enforce it's own limit
> > on partial cores.  
> 
> Agreed. It looks like we won't get rid of thread-based CPU
> hotplug in x86 in the foreseable future.
> 

Re: [Qemu-devel] [PATCH v7 05/13] cpu: Reclaim vCPU objects

[Thomas Huth]

On 28.01.2016 06:49, Bharata B Rao wrote:
> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> 
> In order to deal well with the kvm vcpus (which can not be removed without any
> protection), we do not close KVM vcpu fd, just record and mark it as stopped
> into a list, so that we can reuse it for the appending cpu hot-add request if
> possible. It is also the approach that kvm guys suggested:
> https://www.mail-archive.com/kvm@vger.kernel.org/msg102839.html
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>                [- Explicit CPU_REMOVE() from qemu_kvm/tcg_destroy_vcpu()
>                   isn't needed as it is done from cpu_exec_exit()
>                 - Use iothread mutex instead of global mutex during
>                   destroy
>                 - Don't cleanup vCPU object from vCPU thread context
>                   but leave it to the callers (device_add/device_del)]
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  cpus.c               | 38 +++++++++++++++++++++++++++++++++++
>  include/qom/cpu.h    | 10 +++++++++
>  include/sysemu/kvm.h |  1 +
>  kvm-all.c            | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  kvm-stub.c           |  5 +++++
>  5 files changed, 110 insertions(+), 1 deletion(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 1e97cc4..c5631f0 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -953,6 +953,18 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
>      qemu_cpu_kick(cpu);
>  }
>  
> +static void qemu_kvm_destroy_vcpu(CPUState *cpu)
> +{
> +    if (kvm_destroy_vcpu(cpu) < 0) {
> +        error_report("kvm_destroy_vcpu failed");
> +        exit(EXIT_FAILURE);
> +    }
> +}
> +
> +static void qemu_tcg_destroy_vcpu(CPUState *cpu)
> +{

Will this be populated by a later patch in your series? If not, maybe
add a debugging statement here so that it is clear that there is a TODO
left here?

> +}
> +
>  static void flush_queued_work(CPUState *cpu)
>  {
>      struct qemu_work_item *wi;
> @@ -1053,6 +1065,11 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>              }
>          }
>          qemu_kvm_wait_io_event(cpu);
> +        if (cpu->exit && !cpu_can_run(cpu)) {
> +            qemu_kvm_destroy_vcpu(cpu);
> +            qemu_mutex_unlock_iothread();
> +            return NULL;
> +        }
>      }

You could increase readability of the code by changing the condition of
the loop instead - currently it is a "while (1)" ... you could turn that
into a "do { ... } while (!cpu->exit || cpu_can_run(cpu))" and then
destroy the cpu after the loop.

>      return NULL;
...
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 2e5229d..32a2c71 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -232,6 +232,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.
> + * @exit: Indicates the CPU has exited due to an unplug operation.

There is also a "exit_request" member in this struct already ... maybe
you could name your new variable differently to avoid confusion?
Something like "remove_request" or "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.
> @@ -284,6 +285,7 @@ struct CPUState {
>      bool created;
>      bool stop;
>      bool stopped;
> +    bool exit;
>      bool crash_occurred;
>      bool exit_request;
>      uint32_t interrupt_request;
...

Apart from that, the patch looks fine to me.

 Thomas