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 <address@hidden> 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 <address@hidden>
> > ---
> >  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.
>