| Message ID | 1457443095-213125-3-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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?
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
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. >
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.
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 >
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
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(-)