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

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]