[Qemu-devel] [PATCH v2 0/5] spapr: QMP: add query-hotpluggable-cpus

[Qemu-devel] [PATCH v2 0/5] spapr: QMP: add query-hotpluggable-cpus

[Igor Mammedov]

it's a spapr flavor of original RFC with comments taken in account
 https://patchwork.ozlabs.org/patch/583036/

Changes since RFC:
 - drop arch_id
 - move CPU properties into separate structure
 - target implements its own qmp callback version
 - rebased on top of [RFC PATCH v1 00/10] Core based CPU hotplug for PowerPC sPAPR
                      https://www.mail-archive.com/qemu-devel@nongnu.org/msg357567.html
    - convert slot name to core id hack
    - drop links
    - add generic pre hotplug callback
    - implement query-hotpluggable-cpus

Igor Mammedov (5):
  QMP: add query-hotpluggable-cpus
  spapr: convert slot name property to numeric core and links
  qdev: hotplug: introduce HotplugHandler.pre_plug() callback
  spapr: check if cpu core is already present
  spapr: implement query-hotpluggable-cpus QMP command

 hw/core/hotplug.c                   | 11 +++++
 hw/core/qdev.c                      |  9 +++-
 hw/cpu/core.c                       | 32 ++++++++----
 hw/ppc/spapr.c                      | 97 +++++++++++++++++++++----------------
 hw/ppc/spapr_cpu_core.c             | 25 +---------
 include/hw/cpu/core.h               |  4 +-
 include/hw/hotplug.h                | 14 +++++-
 qapi-schema.json                    | 39 +++++++++++++++
 qmp-commands.hx                     | 34 +++++++++++++
 stubs/Makefile.objs                 |  1 +
 stubs/qmp_query_hotpluggable_cpus.c |  9 ++++
 11 files changed, 198 insertions(+), 77 deletions(-)
 create mode 100644 stubs/qmp_query_hotpluggable_cpus.c

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/5] QMP: add query-hotpluggable-cpus

[Igor Mammedov]

it will allow mgmt to query present and possible to hotplug
CPU objects, it is required from a target platform that
wish to support command to implement
 qmp_query_hotpluggable_cpus()
functioni, which will return a list of possible CPU objects
with options that would be needed for hotplugging possible
CPU objects.

For RFC there are:
'type': 'str' - OQOM CPU object type for usage with device_add

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

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 qapi-schema.json                    | 39 +++++++++++++++++++++++++++++++++++++
 qmp-commands.hx                     | 34 ++++++++++++++++++++++++++++++++
 stubs/Makefile.objs                 |  1 +
 stubs/qmp_query_hotpluggable_cpus.c |  9 +++++++++
 4 files changed, 83 insertions(+)
 create mode 100644 stubs/qmp_query_hotpluggable_cpus.c

diff --git a/qapi-schema.json b/qapi-schema.json
index 362c9d8..c59840d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4122,3 +4122,42 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# CpuInstanceProps
+#
+# @node: NUMA node ID the CPU belongs to, optional
+# @socket: socket number within node/board the CPU belongs to, optional
+# @core: core number within socket the CPU belongs to, optional
+# @thread: thread number within core the CPU belongs to, optional
+#
+# Since: 2.7
+{ 'struct': 'CpuInstanceProps',
+  'data': { '*node': 'int',
+            '*socket': 'int',
+            '*core': 'int',
+            '*thread': 'int'
+  }
+}
+
+##
+# @HotpluggableCPU
+#
+# @type: CPU object type for usage with device_add command
+# @qom-path: link to existing CPU object if CPU is present or
+#            omitted if CPU is not present.
+# @props: list of properties to be used for hotplugging CPU
+#
+# Since: 2.7
+{ 'struct': 'HotpluggableCPU',
+  'data': { 'type': 'str',
+            '*qom-path': 'str',
+            '*props': 'CpuInstanceProps'
+          }
+}
+
+##
+# @query-hotpluggable-cpus
+#
+# Since: 2.6
+{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b629673..39687ad 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4853,3 +4853,37 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+    {
+        .name       = "query-hotpluggable-cpus",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_query_hotpluggable_cpus,
+    },
+
+SQMP
+Show  existing/possible CPUs
+-------------------------------
+
+Arguments: None.
+
+Example for x86 target started with -smp 2,sockets=2,cores=1,threads=3,maxcpus=6:
+
+-> { "execute": "query-hotpluggable-cpus" }
+<- {"return": [
+     {"core": 0, "socket": 1, "thread": 2}, "type": "qemu64-x86_64-cpu"},
+     {"core": 0, "socket": 1, "thread": 1}, "type": "qemu64-x86_64-cpu"},
+     {"core": 0, "socket": 1, "thread": 0}, "type": "qemu64-x86_64-cpu"},
+     {"core": 0, "socket": 0, "thread": 2}, "type": "qemu64-x86_64-cpu"},
+     {"core": 0, "socket": 0, "thread": 1}, "type": "qemu64-x86_64-cpu", "qom-path": "/machine/unattached/device[3]"},
+     {"core": 0, "socket": 0, "thread": 0}, "type": "qemu64-x86_64-cpu", "qom-path": "/machine/unattached/device[0]"}
+   ]}'
+
+Example for SPAPR target started with -smp 2,cores=2,maxcpus=4:
+
+-> { "execute": "query-hotpluggable-cpus" }
+<- {"return": [
+     {"core": 1 }, "type": "spapr-cpu-core"},
+     {"core": 0 }, "type": "spapr-cpu-core", "qom-path": "/machine/unattached/device[0]"}
+   ]}'
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index e922de9..0011173 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -39,3 +39,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_hotpluggable_cpus.o
diff --git a/stubs/qmp_query_hotpluggable_cpus.c b/stubs/qmp_query_hotpluggable_cpus.c
new file mode 100644
index 0000000..21a75a3
--- /dev/null
+++ b/stubs/qmp_query_hotpluggable_cpus.c
@@ -0,0 +1,9 @@
+#include "qemu/osdep.h"
+#include "qapi/qmp/qerror.h"
+#include "qmp-commands.h"
+
+HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
+{
+    error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
+    return NULL;
+}
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/5] spapr: convert slot name property to numeric core and links

[Igor Mammedov]

it's just a hack to get qiuck swith to numeric core id
should be split and merged in patches
introducing modified code.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/cpu/core.c           | 32 +++++++++++++++++++++++---------
 hw/ppc/spapr.c          | 39 ++-------------------------------------
 hw/ppc/spapr_cpu_core.c | 25 ++-----------------------
 include/hw/cpu/core.h   |  4 ++--
 4 files changed, 29 insertions(+), 71 deletions(-)

diff --git a/hw/cpu/core.c b/hw/cpu/core.c
index d8caf37..90a9408 100644
--- a/hw/cpu/core.c
+++ b/hw/cpu/core.c
@@ -7,25 +7,39 @@
  * See the COPYING file in the top-level directory.
  */
 #include "hw/cpu/core.h"
+#include "qapi/visitor.h"
 
-static char *core_prop_get_slot(Object *obj, Error **errp)
+static void core_prop_get_core(Object *obj, Visitor *v,
+                               const char *name, void *opaque,
+                               Error **errp)
 {
-    CPUCore *core = CPU_CORE(obj);
+    CPUCore *cc = CPU_CORE(obj);
+    int64_t value = cc->core;
 
-    return g_strdup(core->slot);
+    visit_type_int(v, name, &value, errp);
 }
 
-static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
+static void core_prop_set_core(Object *obj, Visitor *v,
+                               const char *name, void *opaque,
+                               Error **errp)
 {
-    CPUCore *core = CPU_CORE(obj);
-
-    core->slot = g_strdup(val);
+    CPUCore *cc = CPU_CORE(obj);
+    Error *local_err = NULL;
+    int64_t value;
+
+    visit_type_int(v, name, &value, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    cc->core = value;
 }
 
 static void cpu_core_instance_init(Object *obj)
 {
-    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
-                            NULL);
+    object_property_add(obj, CPU_CORE_ID_PROP, "int",
+                            core_prop_get_core, core_prop_set_core,
+                            NULL, NULL, NULL);
 }
 
 static const TypeInfo cpu_core_type_info = {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6173c1b..6890a44 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1755,28 +1755,6 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp)
     }
 }
 
-/*
- * Check to see if core is being hot-plugged into an already populated slot.
- */
-static void spapr_cpu_core_allow_set_link(Object *obj, const char *name,
-                                          Object *val, Error **errp)
-{
-    Object *core = object_property_get_link(qdev_get_machine(), name, NULL);
-
-    /*
-     * Allow the link to be unset when the core is unplugged.
-     */
-    if (!val) {
-        return;
-    }
-
-    if (core) {
-        char *path = object_get_canonical_path(core);
-        error_setg(errp, "Slot %s already populated with %s", name, path);
-        g_free(path);
-    }
-}
-
 /* pSeries LPAR / sPAPR hardware init */
 static void ppc_spapr_init(MachineState *machine)
 {
@@ -1884,21 +1862,8 @@ static void ppc_spapr_init(MachineState *machine)
     spapr->cores = g_new0(Object *, spapr_max_cores);
 
     for (i = 0; i < spapr_max_cores; i++) {
-        char name[32];
-
-        /*
-         * Create links from machine objects to all possible cores.
-         */
-        snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
-        object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
-                                 (Object **)&spapr->cores[i],
-                                 spapr_cpu_core_allow_set_link,
-                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
-                                 &error_fatal);
-
         /*
-         * Create cores and set link from machine object to core object for
-         * boot time CPUs and realize them.
+         * Create cores for boot time CPUs and realize them.
          */
         if (i < spapr_cores) {
             Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
@@ -1907,7 +1872,7 @@ static void ppc_spapr_init(MachineState *machine)
                                     &error_fatal);
             object_property_set_int(core, smp_threads, "nr_threads",
                                     &error_fatal);
-            object_property_set_str(core, name, CPU_CORE_SLOT_PROP,
+            object_property_set_int(core, i, CPU_CORE_ID_PROP,
                                     &error_fatal);
             object_property_set_bool(core, true, "realized", &error_fatal);
         }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 5156eb3..98af840 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -117,19 +117,12 @@ static int spapr_cpu_release(Object *obj, void *opaque)
 static void spapr_core_release(DeviceState *dev, void *opaque)
 {
     struct sPAPRCPUUnplugList unplug_list;
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
-    char *slot = object_property_get_str(OBJECT(dev), CPU_CORE_SLOT_PROP,
-                 &error_fatal);
 
     QLIST_INIT(&unplug_list);
     object_child_foreach(OBJECT(dev), spapr_cpu_release, &unplug_list);
     spapr_cpu_core_cleanup(&unplug_list);
 
-    /* Unset the link from machine object to this core */
-    object_property_set_link(OBJECT(spapr), NULL, slot, NULL);
-    g_free(slot);
-
     g_free(core->threads);
     object_unparent(OBJECT(dev));
 }
@@ -181,9 +174,6 @@ static int spapr_cpu_core_realize_child(Object *child, void *opaque)
 static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 {
     sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
-    char *slot;
-    Error *local_err = NULL;
 
     if (!core->nr_threads) {
         error_setg(errp, "nr_threads property can't be 0");
@@ -199,19 +189,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
      * TODO: If slot isn't specified, plug this core into
      * an existing empty slot.
      */
-    slot = object_property_get_str(OBJECT(dev), CPU_CORE_SLOT_PROP, &local_err);
-    if (!slot) {
-        error_setg(errp, "slot property isn't set");
-        return;
-    }
-
-    object_property_set_link(OBJECT(spapr), OBJECT(core), slot, &local_err);
-    g_free(slot);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
+    /* probably should error out as 'core' should be specified
+       either by board or by user */
     object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
 }
 
diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
index 2daa724..a627969 100644
--- a/include/hw/cpu/core.h
+++ b/include/hw/cpu/core.h
@@ -22,9 +22,9 @@ typedef struct CPUCore {
     DeviceState parent_obj;
 
     /*< public >*/
-    char *slot;
+    int core;
 } CPUCore;
 
-#define CPU_CORE_SLOT_PROP "slot"
+#define CPU_CORE_ID_PROP "core"
 
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/5] qdev: hotplug: introduce HotplugHandler.pre_plug() callback

[Igor Mammedov]

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

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

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

[Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
replaced link set check removed in previous patch
---
 hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6890a44..db33c29 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
     return fdt;
 }
 
+static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
+                                          DeviceState *dev, Error **errp)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
+                                           &error_abort);
+
+        if (!smc->dr_cpu_enabled && dev->hotplugged) {
+            error_setg(errp, "CPU hotplug not supported for this machine");
+            return;
+        }
+        if (spapr->cores[core]) {
+            error_setg(errp, "core %d is already present", core);
+            return;
+        }
+    }
+}
+
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
@@ -2338,10 +2359,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 
         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        if (!smc->dr_cpu_enabled && dev->hotplugged) {
-            error_setg(errp, "CPU hotplug not supported for this machine");
-            return;
-        }
         spapr_core_plug(hotplug_dev, dev, errp);
     }
 }
@@ -2413,6 +2430,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;
+    hc->pre_plug = spapr_machine_device_pre_plug;
     hc->plug = spapr_machine_device_plug;
     hc->unplug = spapr_machine_device_unplug;
     mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 5/5] spapr: implement query-hotpluggable-cpus QMP command

[Igor Mammedov]

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

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

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
it's only compile tested and needs spapr-cpu-core
to have 'core' property to work with device_add.
---
 hw/ppc/spapr.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index db33c29..cd4b891 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -65,6 +65,7 @@
 #include "hw/compat.h"
 #include "qemu-common.h"
 #include "hw/ppc/spapr_cpu_core.h"
+#include "qmp-commands.h"
 
 #include <libfdt.h>
 
@@ -2404,6 +2405,37 @@ static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
     return cpu_index / smp_threads / smp_cores;
 }
 
+HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
+{
+    int i;
+    HotpluggableCPUList *head = NULL;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    int spapr_max_cores = max_cpus / smp_threads;
+
+    for (i = 0; i < spapr_max_cores; i++) {
+        HotpluggableCPUList *list_item = g_new0(HotpluggableCPUList, 1);
+        HotpluggableCPU *cpu_item = g_new0(HotpluggableCPU, 1);
+        CpuInstanceProps *cpu_props = g_new0(CpuInstanceProps, 1);
+
+        cpu_item->type = g_strdup(TYPE_SPAPR_CPU_CORE);
+        cpu_item->has_props = true;
+        cpu_props->has_core = true;
+        cpu_props->core = i;
+        /* TODO: add 'has_node/node' here to describe
+           to which node core belongs */
+
+        cpu_item->props = cpu_props;
+        if (spapr->cores[i]) {
+            cpu_item->has_qom_path = true;
+            cpu_item->qom_path = object_get_canonical_path(spapr->cores[i]);
+        }
+        list_item->value = cpu_item;
+        list_item->next = head;
+        head = list_item;
+    }
+    return head;
+}
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[Bharata B Rao]

On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> replaced link set check removed in previous patch
> ---
>  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6890a44..db33c29 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
>      return fdt;
>  }
> 
> +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> +                                          DeviceState *dev, Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> +
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> +                                           &error_abort);
> +
> +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> +            error_setg(errp, "CPU hotplug not supported for this machine");
> +            return;
> +        }
> +        if (spapr->cores[core]) {
> +            error_setg(errp, "core %d is already present", core);
> +            return;
> +        }

Wondering why can't we do the above check from core's realizefn and fail
the core hotplug from realizefn ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v2 2/5] spapr: convert slot name property to numeric core and links

[Bharata B Rao]

On Tue, Mar 08, 2016 at 02:18:12PM +0100, Igor Mammedov wrote:
> it's just a hack to get qiuck swith to numeric core id
> should be split and merged in patches
> introducing modified code.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/cpu/core.c           | 32 +++++++++++++++++++++++---------
>  hw/ppc/spapr.c          | 39 ++-------------------------------------
>  hw/ppc/spapr_cpu_core.c | 25 ++-----------------------
>  include/hw/cpu/core.h   |  4 ++--
>  4 files changed, 29 insertions(+), 71 deletions(-)
> 
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index d8caf37..90a9408 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c
> @@ -7,25 +7,39 @@
>   * See the COPYING file in the top-level directory.
>   */
>  #include "hw/cpu/core.h"
> +#include "qapi/visitor.h"
> 
> -static char *core_prop_get_slot(Object *obj, Error **errp)
> +static void core_prop_get_core(Object *obj, Visitor *v,
> +                               const char *name, void *opaque,
> +                               Error **errp)
>  {
> -    CPUCore *core = CPU_CORE(obj);
> +    CPUCore *cc = CPU_CORE(obj);
> +    int64_t value = cc->core;
> 
> -    return g_strdup(core->slot);
> +    visit_type_int(v, name, &value, errp);
>  }
> 
> -static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> +static void core_prop_set_core(Object *obj, Visitor *v,
> +                               const char *name, void *opaque,
> +                               Error **errp)
>  {
> -    CPUCore *core = CPU_CORE(obj);
> -
> -    core->slot = g_strdup(val);
> +    CPUCore *cc = CPU_CORE(obj);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, name, &value, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    cc->core = value;
>  }
> 
>  static void cpu_core_instance_init(Object *obj)
>  {
> -    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> -                            NULL);
> +    object_property_add(obj, CPU_CORE_ID_PROP, "int",
> +                            core_prop_get_core, core_prop_set_core,
> +                            NULL, NULL, NULL);
>  }
> 
>  static const TypeInfo cpu_core_type_info = {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6173c1b..6890a44 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1755,28 +1755,6 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp)
>      }
>  }
> 
> -/*
> - * Check to see if core is being hot-plugged into an already populated slot.
> - */
> -static void spapr_cpu_core_allow_set_link(Object *obj, const char *name,
> -                                          Object *val, Error **errp)
> -{
> -    Object *core = object_property_get_link(qdev_get_machine(), name, NULL);
> -
> -    /*
> -     * Allow the link to be unset when the core is unplugged.
> -     */
> -    if (!val) {
> -        return;
> -    }
> -
> -    if (core) {
> -        char *path = object_get_canonical_path(core);
> -        error_setg(errp, "Slot %s already populated with %s", name, path);
> -        g_free(path);
> -    }
> -}
> -
>  /* pSeries LPAR / sPAPR hardware init */
>  static void ppc_spapr_init(MachineState *machine)
>  {
> @@ -1884,21 +1862,8 @@ static void ppc_spapr_init(MachineState *machine)
>      spapr->cores = g_new0(Object *, spapr_max_cores);
> 
>      for (i = 0; i < spapr_max_cores; i++) {
> -        char name[32];
> -
> -        /*
> -         * Create links from machine objects to all possible cores.
> -         */
> -        snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> -        object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> -                                 (Object **)&spapr->cores[i],
> -                                 spapr_cpu_core_allow_set_link,
> -                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
> -                                 &error_fatal);

With the removal of this link, I don't see any code that explicitly sets
spapr->cores[i] on which you depend upon. However I can add that when I
absorb required bits from this patchset to my sPAPR hotplug series if
this is the agreed way forward.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v2 1/5] QMP: add query-hotpluggable-cpus

[Eric Blake]

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

On 03/08/2016 06:18 AM, Igor Mammedov wrote:
> it will allow mgmt to query present and possible to hotplug

maybe s/possible to hotplug/hotpluggable/

> CPU objects, it is required from a target platform that
> wish to support command to implement
>  qmp_query_hotpluggable_cpus()
> functioni, which will return a list of possible CPU objects

s/functioni/function/

> with options that would be needed for hotplugging possible
> CPU objects.
> 
> For RFC there are:
> 'type': 'str' - OQOM CPU object type for usage with device_add

s/OQOM/QOM/ ?

> 
> and a set of optional fields that are to used for hotplugging
> a CPU objects and would allows mgmt tools to know what/where
> it could be hotplugged;
> [node],[socket],[core],[thread]
> 
> For present CPUs there is a 'qom-path' field which
> would allow mgmt inspect whatever object/abstraction

s/inspect/to inspect/

> the target platform considers as CPU object.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  qapi-schema.json                    | 39 +++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx                     | 34 ++++++++++++++++++++++++++++++++
>  stubs/Makefile.objs                 |  1 +
>  stubs/qmp_query_hotpluggable_cpus.c |  9 +++++++++
>  4 files changed, 83 insertions(+)
>  create mode 100644 stubs/qmp_query_hotpluggable_cpus.c
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 362c9d8..c59840d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4122,3 +4122,42 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# CpuInstanceProps

Worth spelling this as Properties instead of abbreviating?  But the type
names aren't ABI (they don't affect introspection), so I'm not insisting.

> +#
> +# @node: NUMA node ID the CPU belongs to, optional

Elsewhere, we use the tag '#optional', not 'optional, so that when we
finally get Marc-Andre's patches for auto-generating docs, they will
have a sane string to search for.

> +# @socket: socket number within node/board the CPU belongs to, optional
> +# @core: core number within socket the CPU belongs to, optional
> +# @thread: thread number within core the CPU belongs to, optional
> +#
> +# Since: 2.7

Ah, so you've already conceded that this is too much of a feature too
late past 2.6 soft freeze.

> +{ 'struct': 'CpuInstanceProps',
> +  'data': { '*node': 'int',
> +            '*socket': 'int',
> +            '*core': 'int',
> +            '*thread': 'int'
> +  }
> +}
> +
> +##
> +# @HotpluggableCPU
> +#
> +# @type: CPU object type for usage with device_add command
> +# @qom-path: link to existing CPU object if CPU is present or
> +#            omitted if CPU is not present.

Missing '#optional' marker.

> +# @props: list of properties to be used for hotplugging CPU

Is this always going to be present, or should it have an '#optional'
marker?  Right now, it could be present but content-free, as in
"props":{}, if that would help users realize that the particular CPU has
no further tuning available for hotplug purposes.

> +#
> +# Since: 2.7
> +{ 'struct': 'HotpluggableCPU',
> +  'data': { 'type': 'str',
> +            '*qom-path': 'str',
> +            '*props': 'CpuInstanceProps'
> +          }
> +}
> +
> +##
> +# @query-hotpluggable-cpus
> +#
> +# Since: 2.6

Inconsistent - how can the command be 2.6 if the structures it uses are 2.7?

> +{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }

> +Example for x86 target started with -smp 2,sockets=2,cores=1,threads=3,maxcpus=6:
> +
> +-> { "execute": "query-hotpluggable-cpus" }
> +<- {"return": [
> +     {"core": 0, "socket": 1, "thread": 2}, "type": "qemu64-x86_64-cpu"},

Not valid JSON.  You probably meant:

{"return": [
  { "props": {"core"...2}, "type"...},

> +     {"core": 0, "socket": 0, "thread": 1}, "type": "qemu64-x86_64-cpu", "qom-path": "/machine/unattached/device[3]"},

It's okay to line-wrap, to keep 80-column lines.

> +     {"core": 0, "socket": 0, "thread": 0}, "type": "qemu64-x86_64-cpu", "qom-path": "/machine/unattached/device[0]"}
> +   ]}'
> +
> +Example for SPAPR target started with -smp 2,cores=2,maxcpus=4:
> +
> +-> { "execute": "query-hotpluggable-cpus" }
> +<- {"return": [
> +     {"core": 1 }, "type": "spapr-cpu-core"},

Again, not valid JSON.

But useful examples.

-- 
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 v2 2/5] spapr: convert slot name property to numeric core and links

[Eric Blake]

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

On 03/08/2016 06:18 AM, Igor Mammedov wrote:
> it's just a hack to get qiuck swith to numeric core id

s/qiuck swith/a quick switch/

> should be split and merged in patches
> introducing modified code.

Does that mean this series is still RFC, and you plan to respin it
differently?

-- 
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 v2 1/5] QMP: add query-hotpluggable-cpus

[David Gibson]

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

On Tue, Mar 08, 2016 at 09:46:58AM -0700, Eric Blake wrote:
> On 03/08/2016 06:18 AM, Igor Mammedov wrote:
> > it will allow mgmt to query present and possible to hotplug
> 
> maybe s/possible to hotplug/hotpluggable/
> 
> > CPU objects, it is required from a target platform that
> > wish to support command to implement
> >  qmp_query_hotpluggable_cpus()
> > functioni, which will return a list of possible CPU objects
> 
> s/functioni/function/
> 
> > with options that would be needed for hotplugging possible
> > CPU objects.
> > 
> > For RFC there are:
> > 'type': 'str' - OQOM CPU object type for usage with device_add
> 
> s/OQOM/QOM/ ?
> 
> > 
> > and a set of optional fields that are to used for hotplugging
> > a CPU objects and would allows mgmt tools to know what/where
> > it could be hotplugged;
> > [node],[socket],[core],[thread]
> > 
> > For present CPUs there is a 'qom-path' field which
> > would allow mgmt inspect whatever object/abstraction
> 
> s/inspect/to inspect/
> 
> > the target platform considers as CPU object.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  qapi-schema.json                    | 39 +++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx                     | 34 ++++++++++++++++++++++++++++++++
> >  stubs/Makefile.objs                 |  1 +
> >  stubs/qmp_query_hotpluggable_cpus.c |  9 +++++++++
> >  4 files changed, 83 insertions(+)
> >  create mode 100644 stubs/qmp_query_hotpluggable_cpus.c
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 362c9d8..c59840d 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4122,3 +4122,42 @@
> >  ##
> >  { 'enum': 'ReplayMode',
> >    'data': [ 'none', 'record', 'play' ] }
> > +
> > +##
> > +# CpuInstanceProps
> 
> Worth spelling this as Properties instead of abbreviating?  But the type
> names aren't ABI (they don't affect introspection), so I'm not insisting.
> 
> > +#
> > +# @node: NUMA node ID the CPU belongs to, optional
> 
> Elsewhere, we use the tag '#optional', not 'optional, so that when we
> finally get Marc-Andre's patches for auto-generating docs, they will
> have a sane string to search for.
> 
> > +# @socket: socket number within node/board the CPU belongs to, optional
> > +# @core: core number within socket the CPU belongs to, optional
> > +# @thread: thread number within core the CPU belongs to, optional
> > +#
> > +# Since: 2.7
> 
> Ah, so you've already conceded that this is too much of a feature too
> late past 2.6 soft freeze.
> 
> > +{ 'struct': 'CpuInstanceProps',
> > +  'data': { '*node': 'int',
> > +            '*socket': 'int',
> > +            '*core': 'int',
> > +            '*thread': 'int'
> > +  }
> > +}
> > +
> > +##
> > +# @HotpluggableCPU
> > +#
> > +# @type: CPU object type for usage with device_add command
> > +# @qom-path: link to existing CPU object if CPU is present or
> > +#            omitted if CPU is not present.
> 
> Missing '#optional' marker.
> 
> > +# @props: list of properties to be used for hotplugging CPU
> 
> Is this always going to be present, or should it have an '#optional'
> marker?  Right now, it could be present but content-free, as in
> "props":{}, if that would help users realize that the particular CPU has
> no further tuning available for hotplug purposes.

I think it should be always present, even if it can be empty.

> > +#
> > +# Since: 2.7
> > +{ 'struct': 'HotpluggableCPU',
> > +  'data': { 'type': 'str',
> > +            '*qom-path': 'str',
> > +            '*props': 'CpuInstanceProps'
> > +          }
> > +}
> > +
> > +##
> > +# @query-hotpluggable-cpus
> > +#
> > +# Since: 2.6
> 
> Inconsistent - how can the command be 2.6 if the structures it uses are 2.7?
> 
> > +{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
> 
> > +Example for x86 target started with -smp 2,sockets=2,cores=1,threads=3,maxcpus=6:
> > +
> > +-> { "execute": "query-hotpluggable-cpus" }
> > +<- {"return": [
> > +     {"core": 0, "socket": 1, "thread": 2}, "type": "qemu64-x86_64-cpu"},
> 
> Not valid JSON.  You probably meant:
> 
> {"return": [
>   { "props": {"core"...2}, "type"...},
> 
> > +     {"core": 0, "socket": 0, "thread": 1}, "type": "qemu64-x86_64-cpu", "qom-path": "/machine/unattached/device[3]"},
> 
> It's okay to line-wrap, to keep 80-column lines.
> 
> > +     {"core": 0, "socket": 0, "thread": 0}, "type": "qemu64-x86_64-cpu", "qom-path": "/machine/unattached/device[0]"}
> > +   ]}'
> > +
> > +Example for SPAPR target started with -smp 2,cores=2,maxcpus=4:
> > +
> > +-> { "execute": "query-hotpluggable-cpus" }
> > +<- {"return": [
> > +     {"core": 1 }, "type": "spapr-cpu-core"},
> 
> Again, not valid JSON.
> 
> But useful examples.
> 



-- 
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 v2 2/5] spapr: convert slot name property to numeric core and links

[David Gibson]

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

On Tue, Mar 08, 2016 at 02:18:12PM +0100, Igor Mammedov wrote:
> it's just a hack to get qiuck swith to numeric core id
> should be split and merged in patches
> introducing modified code.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/cpu/core.c           | 32 +++++++++++++++++++++++---------
>  hw/ppc/spapr.c          | 39 ++-------------------------------------
>  hw/ppc/spapr_cpu_core.c | 25 ++-----------------------
>  include/hw/cpu/core.h   |  4 ++--
>  4 files changed, 29 insertions(+), 71 deletions(-)
> 
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index d8caf37..90a9408 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c
> @@ -7,25 +7,39 @@
>   * See the COPYING file in the top-level directory.
>   */
>  #include "hw/cpu/core.h"
> +#include "qapi/visitor.h"
>  
> -static char *core_prop_get_slot(Object *obj, Error **errp)
> +static void core_prop_get_core(Object *obj, Visitor *v,
> +                               const char *name, void *opaque,
> +                               Error **errp)
>  {
> -    CPUCore *core = CPU_CORE(obj);
> +    CPUCore *cc = CPU_CORE(obj);
> +    int64_t value = cc->core;
>  
> -    return g_strdup(core->slot);
> +    visit_type_int(v, name, &value, errp);
>  }
>  
> -static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> +static void core_prop_set_core(Object *obj, Visitor *v,
> +                               const char *name, void *opaque,
> +                               Error **errp)
>  {
> -    CPUCore *core = CPU_CORE(obj);
> -
> -    core->slot = g_strdup(val);
> +    CPUCore *cc = CPU_CORE(obj);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, name, &value, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    cc->core = value;
>  }
>  
>  static void cpu_core_instance_init(Object *obj)
>  {
> -    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> -                            NULL);
> +    object_property_add(obj, CPU_CORE_ID_PROP, "int",
> +                            core_prop_get_core, core_prop_set_core,
> +                            NULL, NULL, NULL);
>  }

Something we should clarify at some point: is the core property
intended to be globally unique on its own, or just unique in
combination with other information (e.g. socket and/or node).

>  static const TypeInfo cpu_core_type_info = {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6173c1b..6890a44 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1755,28 +1755,6 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp)
>      }
>  }
>  
> -/*
> - * Check to see if core is being hot-plugged into an already populated slot.
> - */
> -static void spapr_cpu_core_allow_set_link(Object *obj, const char *name,
> -                                          Object *val, Error **errp)
> -{
> -    Object *core = object_property_get_link(qdev_get_machine(), name, NULL);
> -
> -    /*
> -     * Allow the link to be unset when the core is unplugged.
> -     */
> -    if (!val) {
> -        return;
> -    }
> -
> -    if (core) {
> -        char *path = object_get_canonical_path(core);
> -        error_setg(errp, "Slot %s already populated with %s", name, path);
> -        g_free(path);
> -    }
> -}
> -
>  /* pSeries LPAR / sPAPR hardware init */
>  static void ppc_spapr_init(MachineState *machine)
>  {
> @@ -1884,21 +1862,8 @@ static void ppc_spapr_init(MachineState *machine)
>      spapr->cores = g_new0(Object *, spapr_max_cores);
>  
>      for (i = 0; i < spapr_max_cores; i++) {
> -        char name[32];
> -
> -        /*
> -         * Create links from machine objects to all possible cores.
> -         */
> -        snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> -        object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> -                                 (Object **)&spapr->cores[i],
> -                                 spapr_cpu_core_allow_set_link,
> -                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
> -                                 &error_fatal);
> -
>          /*
> -         * Create cores and set link from machine object to core object for
> -         * boot time CPUs and realize them.
> +         * Create cores for boot time CPUs and realize them.
>           */
>          if (i < spapr_cores) {
>              Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> @@ -1907,7 +1872,7 @@ static void ppc_spapr_init(MachineState *machine)
>                                      &error_fatal);
>              object_property_set_int(core, smp_threads, "nr_threads",
>                                      &error_fatal);
> -            object_property_set_str(core, name, CPU_CORE_SLOT_PROP,
> +            object_property_set_int(core, i, CPU_CORE_ID_PROP,
>                                      &error_fatal);

Not really important for this quick hack, but for spapr it might make
sense to have the "core" property be the dt_id, rather than just an
arbitrary index.

>              object_property_set_bool(core, true, "realized", &error_fatal);
>          }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 5156eb3..98af840 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -117,19 +117,12 @@ static int spapr_cpu_release(Object *obj, void *opaque)
>  static void spapr_core_release(DeviceState *dev, void *opaque)
>  {
>      struct sPAPRCPUUnplugList unplug_list;
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> -    char *slot = object_property_get_str(OBJECT(dev), CPU_CORE_SLOT_PROP,
> -                 &error_fatal);
>  
>      QLIST_INIT(&unplug_list);
>      object_child_foreach(OBJECT(dev), spapr_cpu_release, &unplug_list);
>      spapr_cpu_core_cleanup(&unplug_list);
>  
> -    /* Unset the link from machine object to this core */
> -    object_property_set_link(OBJECT(spapr), NULL, slot, NULL);
> -    g_free(slot);
> -
>      g_free(core->threads);
>      object_unparent(OBJECT(dev));
>  }
> @@ -181,9 +174,6 @@ static int spapr_cpu_core_realize_child(Object *child, void *opaque)
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>  {
>      sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> -    char *slot;
> -    Error *local_err = NULL;
>  
>      if (!core->nr_threads) {
>          error_setg(errp, "nr_threads property can't be 0");
> @@ -199,19 +189,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>       * TODO: If slot isn't specified, plug this core into
>       * an existing empty slot.
>       */
> -    slot = object_property_get_str(OBJECT(dev), CPU_CORE_SLOT_PROP, &local_err);
> -    if (!slot) {
> -        error_setg(errp, "slot property isn't set");
> -        return;
> -    }
> -
> -    object_property_set_link(OBJECT(spapr), OBJECT(core), slot, &local_err);
> -    g_free(slot);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
> +    /* probably should error out as 'core' should be specified
> +       either by board or by user */
>      object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
>  }
>  
> diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> index 2daa724..a627969 100644
> --- a/include/hw/cpu/core.h
> +++ b/include/hw/cpu/core.h
> @@ -22,9 +22,9 @@ typedef struct CPUCore {
>      DeviceState parent_obj;
>  
>      /*< public >*/
> -    char *slot;
> +    int core;
>  } CPUCore;
>  
> -#define CPU_CORE_SLOT_PROP "slot"
> +#define CPU_CORE_ID_PROP "core"
>  
>  #endif

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

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

Re: [Qemu-devel] [PATCH v2 2/5] spapr: convert slot name property to numeric core and links

[Igor Mammedov]

On Tue, 8 Mar 2016 20:39:39 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Mar 08, 2016 at 02:18:12PM +0100, Igor Mammedov wrote:
> > it's just a hack to get qiuck swith to numeric core id
> > should be split and merged in patches
> > introducing modified code.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/cpu/core.c           | 32 +++++++++++++++++++++++---------
> >  hw/ppc/spapr.c          | 39 ++-------------------------------------
> >  hw/ppc/spapr_cpu_core.c | 25 ++-----------------------
> >  include/hw/cpu/core.h   |  4 ++--
> >  4 files changed, 29 insertions(+), 71 deletions(-)
> > 
> > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > index d8caf37..90a9408 100644
> > --- a/hw/cpu/core.c
> > +++ b/hw/cpu/core.c
> > @@ -7,25 +7,39 @@
> >   * See the COPYING file in the top-level directory.
> >   */
> >  #include "hw/cpu/core.h"
> > +#include "qapi/visitor.h"
> > 
> > -static char *core_prop_get_slot(Object *obj, Error **errp)
> > +static void core_prop_get_core(Object *obj, Visitor *v,
> > +                               const char *name, void *opaque,
> > +                               Error **errp)
> >  {
> > -    CPUCore *core = CPU_CORE(obj);
> > +    CPUCore *cc = CPU_CORE(obj);
> > +    int64_t value = cc->core;
> > 
> > -    return g_strdup(core->slot);
> > +    visit_type_int(v, name, &value, errp);
> >  }
> > 
> > -static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > +static void core_prop_set_core(Object *obj, Visitor *v,
> > +                               const char *name, void *opaque,
> > +                               Error **errp)
> >  {
> > -    CPUCore *core = CPU_CORE(obj);
> > -
> > -    core->slot = g_strdup(val);
> > +    CPUCore *cc = CPU_CORE(obj);
> > +    Error *local_err = NULL;
> > +    int64_t value;
> > +
> > +    visit_type_int(v, name, &value, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +    cc->core = value;
> >  }
> > 
> >  static void cpu_core_instance_init(Object *obj)
> >  {
> > -    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > -                            NULL);
> > +    object_property_add(obj, CPU_CORE_ID_PROP, "int",
> > +                            core_prop_get_core, core_prop_set_core,
> > +                            NULL, NULL, NULL);
> >  }
> > 
> >  static const TypeInfo cpu_core_type_info = {
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 6173c1b..6890a44 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1755,28 +1755,6 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp)
> >      }
> >  }
> > 
> > -/*
> > - * Check to see if core is being hot-plugged into an already populated slot.
> > - */
> > -static void spapr_cpu_core_allow_set_link(Object *obj, const char *name,
> > -                                          Object *val, Error **errp)
> > -{
> > -    Object *core = object_property_get_link(qdev_get_machine(), name, NULL);
> > -
> > -    /*
> > -     * Allow the link to be unset when the core is unplugged.
> > -     */
> > -    if (!val) {
> > -        return;
> > -    }
> > -
> > -    if (core) {
> > -        char *path = object_get_canonical_path(core);
> > -        error_setg(errp, "Slot %s already populated with %s", name, path);
> > -        g_free(path);
> > -    }
> > -}
> > -
> >  /* pSeries LPAR / sPAPR hardware init */
> >  static void ppc_spapr_init(MachineState *machine)
> >  {
> > @@ -1884,21 +1862,8 @@ static void ppc_spapr_init(MachineState *machine)
> >      spapr->cores = g_new0(Object *, spapr_max_cores);
> > 
> >      for (i = 0; i < spapr_max_cores; i++) {
> > -        char name[32];
> > -
> > -        /*
> > -         * Create links from machine objects to all possible cores.
> > -         */
> > -        snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > -        object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > -                                 (Object **)&spapr->cores[i],
> > -                                 spapr_cpu_core_allow_set_link,
> > -                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
> > -                                 &error_fatal);  
> 
> With the removal of this link, I don't see any code that explicitly sets
> spapr->cores[i] on which you depend upon. However I can add that when I
> absorb required bits from this patchset to my sPAPR hotplug series if
> this is the agreed way forward.
yep, that's need to be fixed, thanks for noticing


> 
> Regards,
> Bharata.
> 

Re: [Qemu-devel] [PATCH v2 1/5] QMP: add query-hotpluggable-cpus

[Igor Mammedov]

On Tue, 8 Mar 2016 09:46:58 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 03/08/2016 06:18 AM, Igor Mammedov wrote:
> > it will allow mgmt to query present and possible to hotplug  
> 
> maybe s/possible to hotplug/hotpluggable/
> 
> > CPU objects, it is required from a target platform that
> > wish to support command to implement
> >  qmp_query_hotpluggable_cpus()
> > functioni, which will return a list of possible CPU objects  
> 
> s/functioni/function/
> 
> > with options that would be needed for hotplugging possible
> > CPU objects.
> > 
> > For RFC there are:
> > 'type': 'str' - OQOM CPU object type for usage with device_add  
> 
> s/OQOM/QOM/ ?
> 
> > 
> > and a set of optional fields that are to used for hotplugging
> > a CPU objects and would allows mgmt tools to know what/where
> > it could be hotplugged;
> > [node],[socket],[core],[thread]
> > 
> > For present CPUs there is a 'qom-path' field which
> > would allow mgmt inspect whatever object/abstraction  
> 
> s/inspect/to inspect/
> 
> > the target platform considers as CPU object.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  qapi-schema.json                    | 39 +++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx                     | 34 ++++++++++++++++++++++++++++++++
> >  stubs/Makefile.objs                 |  1 +
> >  stubs/qmp_query_hotpluggable_cpus.c |  9 +++++++++
> >  4 files changed, 83 insertions(+)
> >  create mode 100644 stubs/qmp_query_hotpluggable_cpus.c
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 362c9d8..c59840d 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4122,3 +4122,42 @@
> >  ##
> >  { 'enum': 'ReplayMode',
> >    'data': [ 'none', 'record', 'play' ] }
> > +
> > +##
> > +# CpuInstanceProps  
> 
> Worth spelling this as Properties instead of abbreviating?  But the type
> names aren't ABI (they don't affect introspection), so I'm not insisting.
Ok, I'll redo it as CpuInstanceProperties if no one objects.

> 
> > +#
> > +# @node: NUMA node ID the CPU belongs to, optional  
> 
> Elsewhere, we use the tag '#optional', not 'optional, so that when we
> finally get Marc-Andre's patches for auto-generating docs, they will
> have a sane string to search for.
> 
> > +# @socket: socket number within node/board the CPU belongs to, optional
> > +# @core: core number within socket the CPU belongs to, optional
> > +# @thread: thread number within core the CPU belongs to, optional
> > +#
> > +# Since: 2.7  
> 
> Ah, so you've already conceded that this is too much of a feature too
> late past 2.6 soft freeze.
yep' it would be better if we commit command early in 2.7 dev cycle,
so individual targets could push target specific patches on top.


> 
> > +{ 'struct': 'CpuInstanceProps',
> > +  'data': { '*node': 'int',
> > +            '*socket': 'int',
> > +            '*core': 'int',
> > +            '*thread': 'int'
> > +  }
> > +}
> > +
> > +##
> > +# @HotpluggableCPU
> > +#
> > +# @type: CPU object type for usage with device_add command
> > +# @qom-path: link to existing CPU object if CPU is present or
> > +#            omitted if CPU is not present.  
> 
> Missing '#optional' marker.
> 
> > +# @props: list of properties to be used for hotplugging CPU  
> 
> Is this always going to be present, or should it have an '#optional'
> marker?  Right now, it could be present but content-free, as in
> "props":{}, if that would help users realize that the particular CPU has
> no further tuning available for hotplug purposes.
Ok, I'll make it as always present.

> 
> > +#
> > +# Since: 2.7
> > +{ 'struct': 'HotpluggableCPU',
> > +  'data': { 'type': 'str',
> > +            '*qom-path': 'str',
> > +            '*props': 'CpuInstanceProps'
> > +          }
> > +}
> > +
> > +##
> > +# @query-hotpluggable-cpus
> > +#
> > +# Since: 2.6  
> 
> Inconsistent - how can the command be 2.6 if the structures it uses are 2.7?
> 
> > +{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }  
> 
> > +Example for x86 target started with -smp 2,sockets=2,cores=1,threads=3,maxcpus=6:
> > +
> > +-> { "execute": "query-hotpluggable-cpus" }
> > +<- {"return": [
> > +     {"core": 0, "socket": 1, "thread": 2}, "type": "qemu64-x86_64-cpu"},  
> 
> Not valid JSON.  You probably meant:
> 
> {"return": [
>   { "props": {"core"...2}, "type"...},
> 
> > +     {"core": 0, "socket": 0, "thread": 1}, "type": "qemu64-x86_64-cpu", "qom-path": "/machine/unattached/device[3]"},  
> 
> It's okay to line-wrap, to keep 80-column lines.
> 
> > +     {"core": 0, "socket": 0, "thread": 0}, "type": "qemu64-x86_64-cpu", "qom-path": "/machine/unattached/device[0]"}
> > +   ]}'
> > +
> > +Example for SPAPR target started with -smp 2,cores=2,maxcpus=4:
> > +
> > +-> { "execute": "query-hotpluggable-cpus" }
> > +<- {"return": [
> > +     {"core": 1 }, "type": "spapr-cpu-core"},  
> 
> Again, not valid JSON.
> 
> But useful examples.
Thanks for review, the rest of comments I'll fix as you've suggested.

Re: [Qemu-devel] [PATCH v2 2/5] spapr: convert slot name property to numeric core and links

[Igor Mammedov]

On Tue, 8 Mar 2016 09:48:18 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 03/08/2016 06:18 AM, Igor Mammedov wrote:
> > it's just a hack to get qiuck swith to numeric core id  
> 
> s/qiuck swith/a quick switch/
> 
> > should be split and merged in patches
> > introducing modified code.  
> 
> Does that mean this series is still RFC, and you plan to respin it
> differently?
It should have been RFC since it's on top another RFC,
some patches are meant to be absorbed by spapr hotplug patchset.
But generic patch introducing QMP command can be posted before
any target specific patchset if we find consensus earlier.

Re: [Qemu-devel] [PATCH v2 2/5] spapr: convert slot name property to numeric core and links

[Igor Mammedov]

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

> On Tue, Mar 08, 2016 at 02:18:12PM +0100, Igor Mammedov wrote:
> > it's just a hack to get qiuck swith to numeric core id
> > should be split and merged in patches
> > introducing modified code.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/cpu/core.c           | 32 +++++++++++++++++++++++---------
> >  hw/ppc/spapr.c          | 39 ++-------------------------------------
> >  hw/ppc/spapr_cpu_core.c | 25 ++-----------------------
> >  include/hw/cpu/core.h   |  4 ++--
> >  4 files changed, 29 insertions(+), 71 deletions(-)
> > 
> > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > index d8caf37..90a9408 100644
> > --- a/hw/cpu/core.c
> > +++ b/hw/cpu/core.c
> > @@ -7,25 +7,39 @@
> >   * See the COPYING file in the top-level directory.
> >   */
> >  #include "hw/cpu/core.h"
> > +#include "qapi/visitor.h"
> >  
> > -static char *core_prop_get_slot(Object *obj, Error **errp)
> > +static void core_prop_get_core(Object *obj, Visitor *v,
> > +                               const char *name, void *opaque,
> > +                               Error **errp)
> >  {
> > -    CPUCore *core = CPU_CORE(obj);
> > +    CPUCore *cc = CPU_CORE(obj);
> > +    int64_t value = cc->core;
> >  
> > -    return g_strdup(core->slot);
> > +    visit_type_int(v, name, &value, errp);
> >  }
> >  
> > -static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > +static void core_prop_set_core(Object *obj, Visitor *v,
> > +                               const char *name, void *opaque,
> > +                               Error **errp)
> >  {
> > -    CPUCore *core = CPU_CORE(obj);
> > -
> > -    core->slot = g_strdup(val);
> > +    CPUCore *cc = CPU_CORE(obj);
> > +    Error *local_err = NULL;
> > +    int64_t value;
> > +
> > +    visit_type_int(v, name, &value, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +    cc->core = value;
> >  }
> >  
> >  static void cpu_core_instance_init(Object *obj)
> >  {
> > -    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > -                            NULL);
> > +    object_property_add(obj, CPU_CORE_ID_PROP, "int",
> > +                            core_prop_get_core, core_prop_set_core,
> > +                            NULL, NULL, NULL);
> >  }  
> 
> Something we should clarify at some point: is the core property
> intended to be globally unique on its own, or just unique in
> combination with other information (e.g. socket and/or node).
I'd think it should be unique in combination with other information.
However I wouldn't enforce it globally, since it might be platform dependent
and I'd leave this check upto a concrete target. 


> 
> >  static const TypeInfo cpu_core_type_info = {
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 6173c1b..6890a44 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1755,28 +1755,6 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp)
> >      }
> >  }
> >  
> > -/*
> > - * Check to see if core is being hot-plugged into an already populated slot.
> > - */
> > -static void spapr_cpu_core_allow_set_link(Object *obj, const char *name,
> > -                                          Object *val, Error **errp)
> > -{
> > -    Object *core = object_property_get_link(qdev_get_machine(), name, NULL);
> > -
> > -    /*
> > -     * Allow the link to be unset when the core is unplugged.
> > -     */
> > -    if (!val) {
> > -        return;
> > -    }
> > -
> > -    if (core) {
> > -        char *path = object_get_canonical_path(core);
> > -        error_setg(errp, "Slot %s already populated with %s", name, path);
> > -        g_free(path);
> > -    }
> > -}
> > -
> >  /* pSeries LPAR / sPAPR hardware init */
> >  static void ppc_spapr_init(MachineState *machine)
> >  {
> > @@ -1884,21 +1862,8 @@ static void ppc_spapr_init(MachineState *machine)
> >      spapr->cores = g_new0(Object *, spapr_max_cores);
> >  
> >      for (i = 0; i < spapr_max_cores; i++) {
> > -        char name[32];
> > -
> > -        /*
> > -         * Create links from machine objects to all possible cores.
> > -         */
> > -        snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > -        object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > -                                 (Object **)&spapr->cores[i],
> > -                                 spapr_cpu_core_allow_set_link,
> > -                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
> > -                                 &error_fatal);
> > -
> >          /*
> > -         * Create cores and set link from machine object to core object for
> > -         * boot time CPUs and realize them.
> > +         * Create cores for boot time CPUs and realize them.
> >           */
> >          if (i < spapr_cores) {
> >              Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> > @@ -1907,7 +1872,7 @@ static void ppc_spapr_init(MachineState *machine)
> >                                      &error_fatal);
> >              object_property_set_int(core, smp_threads, "nr_threads",
> >                                      &error_fatal);
> > -            object_property_set_str(core, name, CPU_CORE_SLOT_PROP,
> > +            object_property_set_int(core, i, CPU_CORE_ID_PROP,
> >                                      &error_fatal);  
> 
> Not really important for this quick hack, but for spapr it might make
> sense to have the "core" property be the dt_id, rather than just an
> arbitrary index.
anything that hides cpu_index from user interface and guest ABI,
and replaces it with some deterministic id (which makes sense for target),
sounds good tome.

> 
> >              object_property_set_bool(core, true, "realized", &error_fatal);
> >          }
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 5156eb3..98af840 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -117,19 +117,12 @@ static int spapr_cpu_release(Object *obj, void *opaque)
> >  static void spapr_core_release(DeviceState *dev, void *opaque)
> >  {
> >      struct sPAPRCPUUnplugList unplug_list;
> > -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > -    char *slot = object_property_get_str(OBJECT(dev), CPU_CORE_SLOT_PROP,
> > -                 &error_fatal);
> >  
> >      QLIST_INIT(&unplug_list);
> >      object_child_foreach(OBJECT(dev), spapr_cpu_release, &unplug_list);
> >      spapr_cpu_core_cleanup(&unplug_list);
> >  
> > -    /* Unset the link from machine object to this core */
> > -    object_property_set_link(OBJECT(spapr), NULL, slot, NULL);
> > -    g_free(slot);
> > -
> >      g_free(core->threads);
> >      object_unparent(OBJECT(dev));
> >  }
> > @@ -181,9 +174,6 @@ static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> >  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >  {
> >      sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > -    char *slot;
> > -    Error *local_err = NULL;
> >  
> >      if (!core->nr_threads) {
> >          error_setg(errp, "nr_threads property can't be 0");
> > @@ -199,19 +189,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >       * TODO: If slot isn't specified, plug this core into
> >       * an existing empty slot.
> >       */
> > -    slot = object_property_get_str(OBJECT(dev), CPU_CORE_SLOT_PROP, &local_err);
> > -    if (!slot) {
> > -        error_setg(errp, "slot property isn't set");
> > -        return;
> > -    }
> > -
> > -    object_property_set_link(OBJECT(spapr), OBJECT(core), slot, &local_err);
> > -    g_free(slot);
> > -    if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        return;
> > -    }
> > -
> > +    /* probably should error out as 'core' should be specified
> > +       either by board or by user */
> >      object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> >  }
> >  
> > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > index 2daa724..a627969 100644
> > --- a/include/hw/cpu/core.h
> > +++ b/include/hw/cpu/core.h
> > @@ -22,9 +22,9 @@ typedef struct CPUCore {
> >      DeviceState parent_obj;
> >  
> >      /*< public >*/
> > -    char *slot;
> > +    int core;
> >  } CPUCore;
> >  
> > -#define CPU_CORE_SLOT_PROP "slot"
> > +#define CPU_CORE_ID_PROP "core"
> >  
> >  #endif  
> 

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[Igor Mammedov]

On Tue, 8 Mar 2016 20:04:12 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > replaced link set check removed in previous patch
> > ---
> >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 6890a44..db33c29 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> >      return fdt;
> >  }
> > 
> > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > +                                          DeviceState *dev, Error **errp)
> > +{
> > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > +
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > +                                           &error_abort);
> > +
> > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > +            return;
> > +        }
> > +        if (spapr->cores[core]) {
> > +            error_setg(errp, "core %d is already present", core);
> > +            return;
> > +        }  
> 
> Wondering why can't we do the above check from core's realizefn and fail
> the core hotplug from realizefn ?
that's rather simple, in ideal QOM world child shouldn't
poke into parents internal if it could be helped.
So hook provides responsibility separation where
board/or something else(HotplugHandler) can do a necessary
wiring of a component which is being hotplugged, without
forcing hotplugged device being aware about it.

That's what HotplugHandler->plug callback is doing for
post realize and HotplugHandler->pre_plug will do similar
thing but allowing board to execute preliminary tasks
(like check/set properties, amend its internal state)
before object is realized.

That will make realize() cleaner as it won't have to hack
into data it shouldn't and would prevent us calling unrealize()
if we were to check it later at HotplugHandler->plug time.
(i.e. realize() won't even have a chance to introduce side
effects that should be undone with unlealize())


> 
> Regards,
> Bharata.
> 

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[David Gibson]

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

On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:
> On Tue, 8 Mar 2016 20:04:12 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > replaced link set check removed in previous patch
> > > ---
> > >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 6890a44..db33c29 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > >      return fdt;
> > >  }
> > > 
> > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > +                                          DeviceState *dev, Error **errp)
> > > +{
> > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > +
> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > > +                                           &error_abort);
> > > +
> > > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > > +            return;
> > > +        }
> > > +        if (spapr->cores[core]) {
> > > +            error_setg(errp, "core %d is already present", core);
> > > +            return;
> > > +        }  
> > 
> > Wondering why can't we do the above check from core's realizefn and fail
> > the core hotplug from realizefn ?
> that's rather simple, in ideal QOM world child shouldn't
> poke into parents internal if it could be helped.
> So hook provides responsibility separation where
> board/or something else(HotplugHandler) can do a necessary
> wiring of a component which is being hotplugged, without
> forcing hotplugged device being aware about it.

Oh.. yes.  Sorry, somehow I got confused and thought you were
suggesting a 'pre_realize()' method on the *object* rather than a
pre_plug hotplughandler hook.

> That's what HotplugHandler->plug callback is doing for
> post realize and HotplugHandler->pre_plug will do similar
> thing but allowing board to execute preliminary tasks
> (like check/set properties, amend its internal state)
> before object is realized.

> That will make realize() cleaner as it won't have to hack
> into data it shouldn't and would prevent us calling unrealize()
> if we were to check it later at HotplugHandler->plug time.
> (i.e. realize() won't even have a chance to introduce side
> effects that should be undone with unlealize())

Hmm.. how big a deal is it to roll back from the existing plug()
handler?

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

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

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[Bharata B Rao]

On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:
> On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:
> > On Tue, 8 Mar 2016 20:04:12 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > replaced link set check removed in previous patch
> > > > ---
> > > >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> > > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 6890a44..db33c29 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > > >      return fdt;
> > > >  }
> > > > 
> > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > > +                                          DeviceState *dev, Error **errp)
> > > > +{
> > > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > +
> > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > > > +                                           &error_abort);
> > > > +
> > > > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > > > +            return;
> > > > +        }
> > > > +        if (spapr->cores[core]) {
> > > > +            error_setg(errp, "core %d is already present", core);
> > > > +            return;
> > > > +        }  
> > > 
> > > Wondering why can't we do the above check from core's realizefn and fail
> > > the core hotplug from realizefn ?
> > that's rather simple, in ideal QOM world child shouldn't
> > poke into parents internal if it could be helped.
> > So hook provides responsibility separation where
> > board/or something else(HotplugHandler) can do a necessary
> > wiring of a component which is being hotplugged, without
> > forcing hotplugged device being aware about it.
> 
> Oh.. yes.  Sorry, somehow I got confused and thought you were
> suggesting a 'pre_realize()' method on the *object* rather than a
> pre_plug hotplughandler hook.
> 
> > That's what HotplugHandler->plug callback is doing for
> > post realize and HotplugHandler->pre_plug will do similar
> > thing but allowing board to execute preliminary tasks
> > (like check/set properties, amend its internal state)
> > before object is realized.
> 
> > That will make realize() cleaner as it won't have to hack
> > into data it shouldn't and would prevent us calling unrealize()
> > if we were to check it later at HotplugHandler->plug time.
> > (i.e. realize() won't even have a chance to introduce side
> > effects that should be undone with unlealize())
> 
> Hmm.. how big a deal is it to roll back from the existing plug()
> handler?

Since plug() handler is post-realize, rolling back involves
deleting the threads of the core we created and finally deleting the core
itself. We aleady do this kind of roll back when core hotplug is attemptedi
on machine type version that don't support hotplug.

For the present case of rejecting the hotplug for duplicate core_ids,
are you in fact hinting that instead of failing the hotplug in pre_plug()
lets realize then and then roll back from plug() ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[Igor Mammedov]

On Thu, 10 Mar 2016 11:32:44 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:
> > On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:  
> > > On Tue, 8 Mar 2016 20:04:12 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >   
> > > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:  
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > > replaced link set check removed in previous patch
> > > > > ---
> > > > >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> > > > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 6890a44..db33c29 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > > > >      return fdt;
> > > > >  }
> > > > > 
> > > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > > > +                                          DeviceState *dev, Error **errp)
> > > > > +{
> > > > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > > +
> > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > > > > +                                           &error_abort);
> > > > > +
> > > > > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > > > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > > > > +            return;
> > > > > +        }
> > > > > +        if (spapr->cores[core]) {
> > > > > +            error_setg(errp, "core %d is already present", core);
> > > > > +            return;
> > > > > +        }    
> > > > 
> > > > Wondering why can't we do the above check from core's realizefn and fail
> > > > the core hotplug from realizefn ?  
> > > that's rather simple, in ideal QOM world child shouldn't
> > > poke into parents internal if it could be helped.
> > > So hook provides responsibility separation where
> > > board/or something else(HotplugHandler) can do a necessary
> > > wiring of a component which is being hotplugged, without
> > > forcing hotplugged device being aware about it.  
> > 
> > Oh.. yes.  Sorry, somehow I got confused and thought you were
> > suggesting a 'pre_realize()' method on the *object* rather than a
> > pre_plug hotplughandler hook.
> >   
> > > That's what HotplugHandler->plug callback is doing for
> > > post realize and HotplugHandler->pre_plug will do similar
> > > thing but allowing board to execute preliminary tasks
> > > (like check/set properties, amend its internal state)
> > > before object is realized.  
> >   
> > > That will make realize() cleaner as it won't have to hack
> > > into data it shouldn't and would prevent us calling unrealize()
> > > if we were to check it later at HotplugHandler->plug time.
> > > (i.e. realize() won't even have a chance to introduce side
> > > effects that should be undone with unlealize())  
> > 
> > Hmm.. how big a deal is it to roll back from the existing plug()
> > handler?
realize shouldn't complete without error if object properties are
wrong /for ex: i.e. you create kvm vcpu thread, configure it
as already existing vcpu and have a lot fun afterwards/.

For example: now on x86 we do duplicate CPU check wrong way
by checking for duplicate of apic property from CPU code by
looping through existing CPUs. Instead it would be much cleaner
to move that check to machine which owns apic id assignment
and make it check for duplicate in pre_plug() handler.


> Since plug() handler is post-realize, rolling back involves
> deleting the threads of the core we created and finally deleting the core
> itself.
Even rolling back will leave some after effects, like created
KVM VCPU thread which can't be deleted and who know what else.

>We aleady do this kind of roll back when core hotplug is attemptedi
> on machine type version that don't support hotplug.
that's seems to be wrong, it shouldn't even come to cpu.realize()
if hotplug is not supported.

> 
> For the present case of rejecting the hotplug for duplicate core_ids,
> are you in fact hinting that instead of failing the hotplug in pre_plug()
> lets realize then and then roll back from plug() ?
> 
> Regards,
> Bharata.
> 

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[Bharata B Rao]

On Thu, Mar 10, 2016 at 11:39:46AM +0100, Igor Mammedov wrote:
> On Thu, 10 Mar 2016 11:32:44 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:
> > > On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:  
> > > > On Tue, 8 Mar 2016 20:04:12 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > >   
> > > > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:  
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > ---
> > > > > > replaced link set check removed in previous patch
> > > > > > ---
> > > > > >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> > > > > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index 6890a44..db33c29 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > > > > >      return fdt;
> > > > > >  }
> > > > > > 
> > > > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > > > > +                                          DeviceState *dev, Error **errp)
> > > > > > +{
> > > > > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > > > +
> > > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > > > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > > > > > +                                           &error_abort);
> > > > > > +
> > > > > > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > > > > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > > > > > +            return;
> > > > > > +        }
> > > > > > +        if (spapr->cores[core]) {
> > > > > > +            error_setg(errp, "core %d is already present", core);
> > > > > > +            return;
> > > > > > +        }    
> > > > > 
> > > > > Wondering why can't we do the above check from core's realizefn and fail
> > > > > the core hotplug from realizefn ?  
> > > > that's rather simple, in ideal QOM world child shouldn't
> > > > poke into parents internal if it could be helped.
> > > > So hook provides responsibility separation where
> > > > board/or something else(HotplugHandler) can do a necessary
> > > > wiring of a component which is being hotplugged, without
> > > > forcing hotplugged device being aware about it.  
> > > 
> > > Oh.. yes.  Sorry, somehow I got confused and thought you were
> > > suggesting a 'pre_realize()' method on the *object* rather than a
> > > pre_plug hotplughandler hook.
> > >   
> > > > That's what HotplugHandler->plug callback is doing for
> > > > post realize and HotplugHandler->pre_plug will do similar
> > > > thing but allowing board to execute preliminary tasks
> > > > (like check/set properties, amend its internal state)
> > > > before object is realized.  
> > >   
> > > > That will make realize() cleaner as it won't have to hack
> > > > into data it shouldn't and would prevent us calling unrealize()
> > > > if we were to check it later at HotplugHandler->plug time.
> > > > (i.e. realize() won't even have a chance to introduce side
> > > > effects that should be undone with unlealize())  
> > > 
> > > Hmm.. how big a deal is it to roll back from the existing plug()
> > > handler?
> realize shouldn't complete without error if object properties are
> wrong /for ex: i.e. you create kvm vcpu thread, configure it
> as already existing vcpu and have a lot fun afterwards/.
> 
> For example: now on x86 we do duplicate CPU check wrong way
> by checking for duplicate of apic property from CPU code by
> looping through existing CPUs. Instead it would be much cleaner
> to move that check to machine which owns apic id assignment
> and make it check for duplicate in pre_plug() handler.
> 
> 
> > Since plug() handler is post-realize, rolling back involves
> > deleting the threads of the core we created and finally deleting the core
> > itself.
> Even rolling back will leave some after effects, like created
> KVM VCPU thread which can't be deleted and who know what else.
> 
> >We aleady do this kind of roll back when core hotplug is attemptedi
> > on machine type version that don't support hotplug.
> that's seems to be wrong, it shouldn't even come to cpu.realize()
> if hotplug is not supported.

Hmm that's how we dis-allowed memory hotplug on machine type versions
that don't support memory hotplug, i,e., we failed hotplug from
->plug() handler. So ->pre_plug() seems to be the ideal place for
such early failures ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[Igor Mammedov]

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

> On Thu, Mar 10, 2016 at 11:39:46AM +0100, Igor Mammedov wrote:
> > On Thu, 10 Mar 2016 11:32:44 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:  
> > > > On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:    
> > > > > On Tue, 8 Mar 2016 20:04:12 +0530
> > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > >     
> > > > > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:    
> > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > > ---
> > > > > > > replaced link set check removed in previous patch
> > > > > > > ---
> > > > > > >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> > > > > > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > > index 6890a44..db33c29 100644
> > > > > > > --- a/hw/ppc/spapr.c
> > > > > > > +++ b/hw/ppc/spapr.c
> > > > > > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > > > > > >      return fdt;
> > > > > > >  }
> > > > > > > 
> > > > > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > > > > > +                                          DeviceState *dev, Error **errp)
> > > > > > > +{
> > > > > > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > > > > +
> > > > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > > > > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > > > > > > +                                           &error_abort);
> > > > > > > +
> > > > > > > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > > > > > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > > > > > > +            return;
> > > > > > > +        }
> > > > > > > +        if (spapr->cores[core]) {
> > > > > > > +            error_setg(errp, "core %d is already present", core);
> > > > > > > +            return;
> > > > > > > +        }      
> > > > > > 
> > > > > > Wondering why can't we do the above check from core's realizefn and fail
> > > > > > the core hotplug from realizefn ?    
> > > > > that's rather simple, in ideal QOM world child shouldn't
> > > > > poke into parents internal if it could be helped.
> > > > > So hook provides responsibility separation where
> > > > > board/or something else(HotplugHandler) can do a necessary
> > > > > wiring of a component which is being hotplugged, without
> > > > > forcing hotplugged device being aware about it.    
> > > > 
> > > > Oh.. yes.  Sorry, somehow I got confused and thought you were
> > > > suggesting a 'pre_realize()' method on the *object* rather than a
> > > > pre_plug hotplughandler hook.
> > > >     
> > > > > That's what HotplugHandler->plug callback is doing for
> > > > > post realize and HotplugHandler->pre_plug will do similar
> > > > > thing but allowing board to execute preliminary tasks
> > > > > (like check/set properties, amend its internal state)
> > > > > before object is realized.    
> > > >     
> > > > > That will make realize() cleaner as it won't have to hack
> > > > > into data it shouldn't and would prevent us calling unrealize()
> > > > > if we were to check it later at HotplugHandler->plug time.
> > > > > (i.e. realize() won't even have a chance to introduce side
> > > > > effects that should be undone with unlealize())    
> > > > 
> > > > Hmm.. how big a deal is it to roll back from the existing plug()
> > > > handler?  
> > realize shouldn't complete without error if object properties are
> > wrong /for ex: i.e. you create kvm vcpu thread, configure it
> > as already existing vcpu and have a lot fun afterwards/.
> > 
> > For example: now on x86 we do duplicate CPU check wrong way
> > by checking for duplicate of apic property from CPU code by
> > looping through existing CPUs. Instead it would be much cleaner
> > to move that check to machine which owns apic id assignment
> > and make it check for duplicate in pre_plug() handler.
> > 
> >   
> > > Since plug() handler is post-realize, rolling back involves
> > > deleting the threads of the core we created and finally deleting the core
> > > itself.  
> > Even rolling back will leave some after effects, like created
> > KVM VCPU thread which can't be deleted and who know what else.
> >   
> > >We aleady do this kind of roll back when core hotplug is attemptedi
> > > on machine type version that don't support hotplug.  
> > that's seems to be wrong, it shouldn't even come to cpu.realize()
> > if hotplug is not supported.  
> 
> Hmm that's how we dis-allowed memory hotplug on machine type versions
> that don't support memory hotplug, i,e., we failed hotplug from
> ->plug() handler. So ->pre_plug() seems to be the ideal place for  
> such early failures ?
It sure looks like it's.
most of pc_dimm_memory_plug() should be done at pre_plug()
except of following hunk which should stay a part of plug()

pc_dimm_memory_plug():
...
    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
    vmstate_register_ram(mr, dev);
    numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
...

Ok, I'll try to repost QMP series on top of the latest SPAPR today,
taking in account libvirt feedback I've got this week.

> 
> Regards,
> Bharata.
> 
> 

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[David Gibson]

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

On Thu, Mar 10, 2016 at 11:39:46AM +0100, Igor Mammedov wrote:
> On Thu, 10 Mar 2016 11:32:44 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:
> > > On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:  
> > > > On Tue, 8 Mar 2016 20:04:12 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > >   
> > > > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:  
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > ---
> > > > > > replaced link set check removed in previous patch
> > > > > > ---
> > > > > >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> > > > > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index 6890a44..db33c29 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > > > > >      return fdt;
> > > > > >  }
> > > > > > 
> > > > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > > > > +                                          DeviceState *dev, Error **errp)
> > > > > > +{
> > > > > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > > > +
> > > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > > > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > > > > > +                                           &error_abort);
> > > > > > +
> > > > > > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > > > > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > > > > > +            return;
> > > > > > +        }
> > > > > > +        if (spapr->cores[core]) {
> > > > > > +            error_setg(errp, "core %d is already present", core);
> > > > > > +            return;
> > > > > > +        }    
> > > > > 
> > > > > Wondering why can't we do the above check from core's realizefn and fail
> > > > > the core hotplug from realizefn ?  
> > > > that's rather simple, in ideal QOM world child shouldn't
> > > > poke into parents internal if it could be helped.
> > > > So hook provides responsibility separation where
> > > > board/or something else(HotplugHandler) can do a necessary
> > > > wiring of a component which is being hotplugged, without
> > > > forcing hotplugged device being aware about it.  
> > > 
> > > Oh.. yes.  Sorry, somehow I got confused and thought you were
> > > suggesting a 'pre_realize()' method on the *object* rather than a
> > > pre_plug hotplughandler hook.
> > >   
> > > > That's what HotplugHandler->plug callback is doing for
> > > > post realize and HotplugHandler->pre_plug will do similar
> > > > thing but allowing board to execute preliminary tasks
> > > > (like check/set properties, amend its internal state)
> > > > before object is realized.  
> > >   
> > > > That will make realize() cleaner as it won't have to hack
> > > > into data it shouldn't and would prevent us calling unrealize()
> > > > if we were to check it later at HotplugHandler->plug time.
> > > > (i.e. realize() won't even have a chance to introduce side
> > > > effects that should be undone with unlealize())  
> > > 
> > > Hmm.. how big a deal is it to roll back from the existing plug()
> > > handler?
> realize shouldn't complete without error if object properties are
> wrong /for ex: i.e. you create kvm vcpu thread, configure it
> as already existing vcpu and have a lot fun afterwards/.

It seems to me there are two sorts of checks.  (1) properties that are
wrong simply with reference to the CPU core itself (e.g. unsupported
CPU model, impossible number of threads).  (2) properties that are
wrong only in the context of other CPUs or devices (e.g. core id
already populated, too many cores, impossible core id).

Is it really a problem for realize() to complete if (1) is checked,
but not (2)?

If it's so essential, I'm surprised we haven't hit this already.  What
happens if you try to device_add two PCI devices in the same slot?
Where is that checked?

> For example: now on x86 we do duplicate CPU check wrong way
> by checking for duplicate of apic property from CPU code by
> looping through existing CPUs. Instead it would be much cleaner
> to move that check to machine which owns apic id assignment
> and make it check for duplicate in pre_plug() handler.
> 
> 
> > Since plug() handler is post-realize, rolling back involves
> > deleting the threads of the core we created and finally deleting the core
> > itself.
> Even rolling back will leave some after effects, like created
> KVM VCPU thread which can't be deleted and who know what else.
> 
> >We aleady do this kind of roll back when core hotplug is attemptedi
> > on machine type version that don't support hotplug.
> that's seems to be wrong, it shouldn't even come to cpu.realize()
> if hotplug is not supported.

To be clear here, I'm not saying I think pre_plug() is a bad idea.
I'm just wondering if we can treat that change to the core hotplug
APIs as a clean up for later, rather than a prereq for CPU hotplug.

-- 
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 v2 4/5] spapr: check if cpu core is already present

[Igor Mammedov]

On Tue, 15 Mar 2016 17:10:27 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Mar 10, 2016 at 11:39:46AM +0100, Igor Mammedov wrote:
> > On Thu, 10 Mar 2016 11:32:44 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:  
> > > > On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:    
> > > > > On Tue, 8 Mar 2016 20:04:12 +0530
> > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > >     
> > > > > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:    
> > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > > ---
> > > > > > > replaced link set check removed in previous patch
> > > > > > > ---
> > > > > > >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> > > > > > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > > index 6890a44..db33c29 100644
> > > > > > > --- a/hw/ppc/spapr.c
> > > > > > > +++ b/hw/ppc/spapr.c
> > > > > > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > > > > > >      return fdt;
> > > > > > >  }
> > > > > > > 
> > > > > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > > > > > +                                          DeviceState *dev, Error **errp)
> > > > > > > +{
> > > > > > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > > > > +
> > > > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > > > > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > > > > > > +                                           &error_abort);
> > > > > > > +
> > > > > > > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > > > > > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > > > > > > +            return;
> > > > > > > +        }
> > > > > > > +        if (spapr->cores[core]) {
> > > > > > > +            error_setg(errp, "core %d is already present", core);
> > > > > > > +            return;
> > > > > > > +        }      
> > > > > > 
> > > > > > Wondering why can't we do the above check from core's realizefn and fail
> > > > > > the core hotplug from realizefn ?    
> > > > > that's rather simple, in ideal QOM world child shouldn't
> > > > > poke into parents internal if it could be helped.
> > > > > So hook provides responsibility separation where
> > > > > board/or something else(HotplugHandler) can do a necessary
> > > > > wiring of a component which is being hotplugged, without
> > > > > forcing hotplugged device being aware about it.    
> > > > 
> > > > Oh.. yes.  Sorry, somehow I got confused and thought you were
> > > > suggesting a 'pre_realize()' method on the *object* rather than a
> > > > pre_plug hotplughandler hook.
> > > >     
> > > > > That's what HotplugHandler->plug callback is doing for
> > > > > post realize and HotplugHandler->pre_plug will do similar
> > > > > thing but allowing board to execute preliminary tasks
> > > > > (like check/set properties, amend its internal state)
> > > > > before object is realized.    
> > > >     
> > > > > That will make realize() cleaner as it won't have to hack
> > > > > into data it shouldn't and would prevent us calling unrealize()
> > > > > if we were to check it later at HotplugHandler->plug time.
> > > > > (i.e. realize() won't even have a chance to introduce side
> > > > > effects that should be undone with unlealize())    
> > > > 
> > > > Hmm.. how big a deal is it to roll back from the existing plug()
> > > > handler?  
> > realize shouldn't complete without error if object properties are
> > wrong /for ex: i.e. you create kvm vcpu thread, configure it
> > as already existing vcpu and have a lot fun afterwards/.  
(*1 ^^^)

> 
> It seems to me there are two sorts of checks.  (1) properties that are
> wrong simply with reference to the CPU core itself (e.g. unsupported
> CPU model, impossible number of threads).  (2) properties that are
> wrong only in the context of other CPUs or devices (e.g. core id
> already populated, too many cores, impossible core id).
> 
> Is it really a problem for realize() to complete if (1) is checked,
> but not (2)?
skipping 2 would do *1, (it's hard to tell what complications would
be if CPU object with incorrect properties are created)
 
> If it's so essential, I'm surprised we haven't hit this already.  What
> happens if you try to device_add two PCI devices in the same slot?
> Where is that checked?

PCI device has 2 'address' properties, 'addr' and 'bus'
checking for valid address /including busy slot/
happens as the first step in:

pci_qdev_realize()->
  do_pci_register_device()


> 
> > For example: now on x86 we do duplicate CPU check wrong way
> > by checking for duplicate of apic property from CPU code by
> > looping through existing CPUs. Instead it would be much cleaner
> > to move that check to machine which owns apic id assignment
> > and make it check for duplicate in pre_plug() handler.
> > 
> >   
> > > Since plug() handler is post-realize, rolling back involves
> > > deleting the threads of the core we created and finally deleting the core
> > > itself.  
> > Even rolling back will leave some after effects, like created
> > KVM VCPU thread which can't be deleted and who know what else.
> >   
> > >We aleady do this kind of roll back when core hotplug is attemptedi
> > > on machine type version that don't support hotplug.  
> > that's seems to be wrong, it shouldn't even come to cpu.realize()
> > if hotplug is not supported.  
> 
> To be clear here, I'm not saying I think pre_plug() is a bad idea.
> I'm just wondering if we can treat that change to the core hotplug
> APIs as a clean up for later, rather than a prereq for CPU hotplug.
I's too late for core hotplug being merged into 2.6
(it's still RFC and QEMU is in soft-freeze).
It would be better to fix series so that hotplug would be
done in a clean way and be ready for merging by 2.7 dev cycle opens.

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[David Gibson]

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

On Tue, Mar 15, 2016 at 12:05:06PM +0100, Igor Mammedov wrote:
> On Tue, 15 Mar 2016 17:10:27 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Mar 10, 2016 at 11:39:46AM +0100, Igor Mammedov wrote:
> > > On Thu, 10 Mar 2016 11:32:44 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >   
> > > > On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:  
> > > > > On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:    
> > > > > > On Tue, 8 Mar 2016 20:04:12 +0530
> > > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > > >     
> > > > > > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:    
> > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > > > ---
> > > > > > > > replaced link set check removed in previous patch
> > > > > > > > ---
> > > > > > > >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> > > > > > > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > > > index 6890a44..db33c29 100644
> > > > > > > > --- a/hw/ppc/spapr.c
> > > > > > > > +++ b/hw/ppc/spapr.c
> > > > > > > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > > > > > > >      return fdt;
> > > > > > > >  }
> > > > > > > > 
> > > > > > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > > > > > > +                                          DeviceState *dev, Error **errp)
> > > > > > > > +{
> > > > > > > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > > > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > > > > > +
> > > > > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > > > > > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > > > > > > > +                                           &error_abort);
> > > > > > > > +
> > > > > > > > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > > > > > > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > > > > > > > +            return;
> > > > > > > > +        }
> > > > > > > > +        if (spapr->cores[core]) {
> > > > > > > > +            error_setg(errp, "core %d is already present", core);
> > > > > > > > +            return;
> > > > > > > > +        }      
> > > > > > > 
> > > > > > > Wondering why can't we do the above check from core's realizefn and fail
> > > > > > > the core hotplug from realizefn ?    
> > > > > > that's rather simple, in ideal QOM world child shouldn't
> > > > > > poke into parents internal if it could be helped.
> > > > > > So hook provides responsibility separation where
> > > > > > board/or something else(HotplugHandler) can do a necessary
> > > > > > wiring of a component which is being hotplugged, without
> > > > > > forcing hotplugged device being aware about it.    
> > > > > 
> > > > > Oh.. yes.  Sorry, somehow I got confused and thought you were
> > > > > suggesting a 'pre_realize()' method on the *object* rather than a
> > > > > pre_plug hotplughandler hook.
> > > > >     
> > > > > > That's what HotplugHandler->plug callback is doing for
> > > > > > post realize and HotplugHandler->pre_plug will do similar
> > > > > > thing but allowing board to execute preliminary tasks
> > > > > > (like check/set properties, amend its internal state)
> > > > > > before object is realized.    
> > > > >     
> > > > > > That will make realize() cleaner as it won't have to hack
> > > > > > into data it shouldn't and would prevent us calling unrealize()
> > > > > > if we were to check it later at HotplugHandler->plug time.
> > > > > > (i.e. realize() won't even have a chance to introduce side
> > > > > > effects that should be undone with unlealize())    
> > > > > 
> > > > > Hmm.. how big a deal is it to roll back from the existing plug()
> > > > > handler?  
> > > realize shouldn't complete without error if object properties are
> > > wrong /for ex: i.e. you create kvm vcpu thread, configure it
> > > as already existing vcpu and have a lot fun afterwards/.  
> (*1 ^^^)
> 
> > 
> > It seems to me there are two sorts of checks.  (1) properties that are
> > wrong simply with reference to the CPU core itself (e.g. unsupported
> > CPU model, impossible number of threads).  (2) properties that are
> > wrong only in the context of other CPUs or devices (e.g. core id
> > already populated, too many cores, impossible core id).
> > 
> > Is it really a problem for realize() to complete if (1) is checked,
> > but not (2)?
> skipping 2 would do *1, (it's hard to tell what complications would
> be if CPU object with incorrect properties are created)

Hm, ok.

> > If it's so essential, I'm surprised we haven't hit this already.  What
> > happens if you try to device_add two PCI devices in the same slot?
> > Where is that checked?
> 
> PCI device has 2 'address' properties, 'addr' and 'bus'
> checking for valid address /including busy slot/
> happens as the first step in:
> 
> pci_qdev_realize()->
>   do_pci_register_device()

Ah...!

So the trick here is that the PCI device registers with its bus during
realize().  So now I'm wondering if we should be doing an equivalent
thing for CPUs: e.g. calling spapr_register_core() or something from
realize().

Or is there a fundamental difference between the cases which means
pre_plug() is a better choice here.

> > > For example: now on x86 we do duplicate CPU check wrong way
> > > by checking for duplicate of apic property from CPU code by
> > > looping through existing CPUs. Instead it would be much cleaner
> > > to move that check to machine which owns apic id assignment
> > > and make it check for duplicate in pre_plug() handler.
> > > 
> > >   
> > > > Since plug() handler is post-realize, rolling back involves
> > > > deleting the threads of the core we created and finally deleting the core
> > > > itself.  
> > > Even rolling back will leave some after effects, like created
> > > KVM VCPU thread which can't be deleted and who know what else.
> > >   
> > > >We aleady do this kind of roll back when core hotplug is attemptedi
> > > > on machine type version that don't support hotplug.  
> > > that's seems to be wrong, it shouldn't even come to cpu.realize()
> > > if hotplug is not supported.  
> > 
> > To be clear here, I'm not saying I think pre_plug() is a bad idea.
> > I'm just wondering if we can treat that change to the core hotplug
> > APIs as a clean up for later, rather than a prereq for CPU hotplug.
> I's too late for core hotplug being merged into 2.6
> (it's still RFC and QEMU is in soft-freeze).
> It would be better to fix series so that hotplug would be
> done in a clean way and be ready for merging by 2.7 dev cycle opens.

Yes, I know.  But I'm worried that even in the 2.7 timeframe that
adding callbacks to the core hotplug model could cause long arguments
and delays.

-- 
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 v2 4/5] spapr: check if cpu core is already present

[Igor Mammedov]

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

> On Tue, Mar 15, 2016 at 12:05:06PM +0100, Igor Mammedov wrote:
> > On Tue, 15 Mar 2016 17:10:27 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, Mar 10, 2016 at 11:39:46AM +0100, Igor Mammedov wrote:  
> > > > On Thu, 10 Mar 2016 11:32:44 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > >     
> > > > > On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:    
> > > > > > On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:      
> > > > > > > On Tue, 8 Mar 2016 20:04:12 +0530
> > > > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > > > >       
> > > > > > > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:      
> > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > > > > ---
> > > > > > > > > replaced link set check removed in previous patch
> > > > > > > > > ---
> > > > > > > > >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> > > > > > > > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > > > > index 6890a44..db33c29 100644
> > > > > > > > > --- a/hw/ppc/spapr.c
> > > > > > > > > +++ b/hw/ppc/spapr.c
> > > > > > > > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > > > > > > > >      return fdt;
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > > > > > > > +                                          DeviceState *dev, Error **errp)
> > > > > > > > > +{
> > > > > > > > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > > > > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > > > > > > +
> > > > > > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > > > > > > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > > > > > > > > +                                           &error_abort);
> > > > > > > > > +
> > > > > > > > > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > > > > > > > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > > > > > > > > +            return;
> > > > > > > > > +        }
> > > > > > > > > +        if (spapr->cores[core]) {
> > > > > > > > > +            error_setg(errp, "core %d is already present", core);
> > > > > > > > > +            return;
> > > > > > > > > +        }        
> > > > > > > > 
> > > > > > > > Wondering why can't we do the above check from core's realizefn and fail
> > > > > > > > the core hotplug from realizefn ?      
> > > > > > > that's rather simple, in ideal QOM world child shouldn't
> > > > > > > poke into parents internal if it could be helped.
> > > > > > > So hook provides responsibility separation where
> > > > > > > board/or something else(HotplugHandler) can do a necessary
> > > > > > > wiring of a component which is being hotplugged, without
> > > > > > > forcing hotplugged device being aware about it.      
> > > > > > 
> > > > > > Oh.. yes.  Sorry, somehow I got confused and thought you were
> > > > > > suggesting a 'pre_realize()' method on the *object* rather than a
> > > > > > pre_plug hotplughandler hook.
> > > > > >       
> > > > > > > That's what HotplugHandler->plug callback is doing for
> > > > > > > post realize and HotplugHandler->pre_plug will do similar
> > > > > > > thing but allowing board to execute preliminary tasks
> > > > > > > (like check/set properties, amend its internal state)
> > > > > > > before object is realized.      
> > > > > >       
> > > > > > > That will make realize() cleaner as it won't have to hack
> > > > > > > into data it shouldn't and would prevent us calling unrealize()
> > > > > > > if we were to check it later at HotplugHandler->plug time.
> > > > > > > (i.e. realize() won't even have a chance to introduce side
> > > > > > > effects that should be undone with unlealize())      
> > > > > > 
> > > > > > Hmm.. how big a deal is it to roll back from the existing plug()
> > > > > > handler?    
> > > > realize shouldn't complete without error if object properties are
> > > > wrong /for ex: i.e. you create kvm vcpu thread, configure it
> > > > as already existing vcpu and have a lot fun afterwards/.    
> > (*1 ^^^)
> >   
> > > 
> > > It seems to me there are two sorts of checks.  (1) properties that are
> > > wrong simply with reference to the CPU core itself (e.g. unsupported
> > > CPU model, impossible number of threads).  (2) properties that are
> > > wrong only in the context of other CPUs or devices (e.g. core id
> > > already populated, too many cores, impossible core id).
> > > 
> > > Is it really a problem for realize() to complete if (1) is checked,
> > > but not (2)?  
> > skipping 2 would do *1, (it's hard to tell what complications would
> > be if CPU object with incorrect properties are created)  
> 
> Hm, ok.
> 
> > > If it's so essential, I'm surprised we haven't hit this already.  What
> > > happens if you try to device_add two PCI devices in the same slot?
> > > Where is that checked?  
> > 
> > PCI device has 2 'address' properties, 'addr' and 'bus'
> > checking for valid address /including busy slot/
> > happens as the first step in:
> > 
> > pci_qdev_realize()->
> >   do_pci_register_device()  
> 
> Ah...!
> 
> So the trick here is that the PCI device registers with its bus during
> realize().  So now I'm wondering if we should be doing an equivalent
> thing for CPUs: e.g. calling spapr_register_core() or something from
> realize().
When I introduced HotplugHandler, I've only did minor changes to bus
based hotplug so it might use HotplugHandler transparently without
rewriting QEMU. And since goal was to introduce missing hotplug
infrastructure for bus-less devices not much of legacy bus hotplug
had been touched. (patches as usual are welcome if someone has time for it)

> Or is there a fundamental difference between the cases which means
> pre_plug() is a better choice here.
PCI device has access to its parent bus
while CPUs are bus-less devices and don't have any supporting
infrastructure to access its parent/owner. That's the reason
why HotplugHandler has been introduced to allow hotplug of bus-less
devices and still maintain clear role separation between device models.
Apparent I've haven't noticed a need for pre_plug() hook back then.

> > > > For example: now on x86 we do duplicate CPU check wrong way
> > > > by checking for duplicate of apic property from CPU code by
> > > > looping through existing CPUs. Instead it would be much cleaner
> > > > to move that check to machine which owns apic id assignment
> > > > and make it check for duplicate in pre_plug() handler.
> > > > 
> > > >     
> > > > > Since plug() handler is post-realize, rolling back involves
> > > > > deleting the threads of the core we created and finally deleting the core
> > > > > itself.    
> > > > Even rolling back will leave some after effects, like created
> > > > KVM VCPU thread which can't be deleted and who know what else.
> > > >     
> > > > >We aleady do this kind of roll back when core hotplug is attemptedi
> > > > > on machine type version that don't support hotplug.    
> > > > that's seems to be wrong, it shouldn't even come to cpu.realize()
> > > > if hotplug is not supported.    
> > > 
> > > To be clear here, I'm not saying I think pre_plug() is a bad idea.
> > > I'm just wondering if we can treat that change to the core hotplug
> > > APIs as a clean up for later, rather than a prereq for CPU hotplug.  
> > I's too late for core hotplug being merged into 2.6
> > (it's still RFC and QEMU is in soft-freeze).
> > It would be better to fix series so that hotplug would be
> > done in a clean way and be ready for merging by 2.7 dev cycle opens.  
> 
> Yes, I know.  But I'm worried that even in the 2.7 timeframe that
> adding callbacks to the core hotplug model could cause long arguments
> and delays.
So far no one has argued against of the idea so that should be fine,
and as Bharata pointed out memory hotplug code could
benefit from pre_plug() hook as well.