On Wed, 9 Mar 2016 14:19:26 +1100
David Gibson <da...@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 <imamm...@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
>
