[RFC,v0,2/6] spapr: CPU core device

Add sPAPR specific CPU core device that is based on generic CPU core device.
Creating this core device will result in creation of all the CPU thread
devices that are part of this core.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/Makefile.objs            |   1 +
 hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr_cpu_core.h |  32 ++++++
 3 files changed, 243 insertions(+)
 create mode 100644 hw/ppc/spapr_cpu_core.c
 create mode 100644 include/hw/ppc/spapr_cpu_core.h

On Thu, Feb 25, 2016 at 09:52:38PM +0530, Bharata B Rao wrote:
> Add sPAPR specific CPU core device that is based on generic CPU core device.
> Creating this core device will result in creation of all the CPU thread
> devices that are part of this core.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Looks good overall, a few minor points below.

> ---
>  hw/ppc/Makefile.objs            |   1 +
>  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
>  3 files changed, 243 insertions(+)
>  create mode 100644 hw/ppc/spapr_cpu_core.c
>  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index c1ffc77..5cc6608 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> new file mode 100644
> index 0000000..c44eb61
> --- /dev/null
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -0,0 +1,210 @@
> +/*
> + * sPAPR CPU core device, acts as container of CPU thread devices.
> + *
> + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/cpu/core.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/boards.h"
> +#include "qemu/error-report.h"
> +#include "qapi/visitor.h"
> +#include <sysemu/cpus.h>
> +
> +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> +{
> +    Error **errp = opaque;
> +
> +    object_property_set_bool(child, true, "realized", errp);
> +    if (*errp) {
> +        return 1;
> +    }
> +    return 0;
> +}
> +
> +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    Error *local_err = NULL;
> +
> +    if (!core->nr_threads) {
> +        error_setg(errp, "nr_threads property can't be 0");
> +        return;
> +    }
> +
> +    if (!core->cpu_model) {
> +        error_setg(errp, "cpu_model property isn't set");
> +        return;
> +    }
> +
> +    /*
> +     * TODO: If slot isn't specified, plug this core into
> +     * an existing empty slot.
> +     */
> +    if (!core->slot) {
> +        error_setg(errp, "slot property isn't set");
> +        return;
> +    }
> +
> +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> +                             &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> +}
> +
> +/*
> + * This creates the CPU threads for a given @core.
> + *
> + * In order to create the threads, we need two inputs - number of
> + * threads and the cpu_model. These are set as core object's properties.
> + * When both of them become available/set, this routine will be called from
> + * either property's set handler to create the threads.
> + *
> + * TODO: Dependence of threads creation on two properties is resulting
> + * in this not-so-clean way of creating threads from either of the
> + * property setters based on the order in which they get set. Check if
> + * this can be handled in a better manner.
> + */
> +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> +{
> +    int i;
> +
> +    for (i = 0; i < core->nr_threads; i++) {
> +        char id[32];
> +        char type[32];
> +
> +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> +                 TYPE_POWERPC_CPU);

I think you want to use cpu_class_by_name() rather than explicitly
constructing the class name here.

> +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> +
> +        snprintf(id, sizeof(id), "thread[%d]", i);
> +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> +                                  errp);
> +    }
> +}
> +
> +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    return core->slot;
> +}
> +
> +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> +                                              Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    core->slot = g_strdup(val);
> +}
> +
> +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    return core->cpu_model;
> +}
> +
> +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> +                                              Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +
> +    /*
> +     * cpu_model can't be different from what is specified with -cpu
> +     */
> +    if (strcmp(val, machine->cpu_model)) {
> +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> +       return;
> +    }

I'm not sure if this is the right place to test this.  Possibly it
should be tested in the machine hotplug handler instead.

> +    core->cpu_model = g_strdup(val);
> +    if (core->nr_threads && core->cpu_model) {
> +        spapr_cpu_core_create_threads(core, errp);
> +    }
> +}
> +
> +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> +                                               const char *name, void *opaque,
> +                                               Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +    int64_t value = core->nr_threads;
> +
> +    visit_type_int(v, name, &value, errp);
> +}
> +
> +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> +                                               const char *name, void *opaque,
> +                                               Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_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;
> +    }
> +
> +    /* Allow only homogeneous configuration */
> +    if (value != smp_threads) {
> +        error_setg(errp, "nr_threads should be %d", smp_threads);
> +        return;
> +    }

Likewise, not sure if this is the right place for this check.

> +    core->nr_threads = value;
> +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> +
> +    if (core->nr_threads && core->cpu_model) {
> +        spapr_cpu_core_create_threads(core, errp);
> +    }
> +}
> +
> +static void spapr_cpu_core_instance_init(Object *obj)
> +{
> +    object_property_add(obj, "nr_threads", "int",
> +                        spapr_cpu_core_prop_get_nr_threads,
> +                        spapr_cpu_core_prop_set_nr_threads,
> +                        NULL, NULL, NULL);
> +    object_property_add_str(obj, "cpu_model",
> +                            spapr_cpu_core_prop_get_cpu_model,
> +                            spapr_cpu_core_prop_set_cpu_model,
> +                            NULL);
> +    object_property_add_str(obj, "slot",
> +                            spapr_cpu_core_prop_get_slot,
> +                            spapr_cpu_core_prop_set_slot,
> +                            NULL);
> +}
> +
> +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = spapr_cpu_core_realize;
> +}
> +
> +static const TypeInfo spapr_cpu_core_type_info = {
> +    .name = TYPE_SPAPR_CPU_CORE,
> +    .parent = TYPE_CPU_CORE,
> +    .instance_init = spapr_cpu_core_instance_init,
> +    .instance_size = sizeof(sPAPRCPUCore),
> +    .class_init = spapr_cpu_core_class_init,
> +};
> +
> +static void spapr_cpu_core_register_types(void)
> +{
> +    type_register_static(&spapr_cpu_core_type_info);
> +}
> +
> +type_init(spapr_cpu_core_register_types)
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> new file mode 100644
> index 0000000..ed9bc7f
> --- /dev/null
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -0,0 +1,32 @@
> +/*
> + * sPAPR CPU core device.
> + *
> + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef HW_SPAPR_CPU_CORE_H
> +#define HW_SPAPR_CPU_CORE_H
> +
> +#include "hw/qdev.h"
> +#include "hw/cpu/core.h"
> +
> +#define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
> +#define SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(sPAPRCPUCore, (obj), TYPE_SPAPR_CPU_CORE)
> +
> +typedef struct sPAPRCPUCore {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +
> +    /*< public >*/
> +    int nr_threads;
> +    char *cpu_model;

I think this might be better as an ObjectClass *, rather than a
string.  It avoids strdup()ing above, and works a bit more nicely with
cpu_class_by_name().

> +    char *slot;
> +    PowerPCCPU *threads;
> +} sPAPRCPUCore;
> +
> +#define SPAPR_CPU_CORE_SLOT_PROP "slot"
> +
> +#endif

On Fri, Feb 26, 2016 at 01:57:44PM +1100, David Gibson wrote:
> On Thu, Feb 25, 2016 at 09:52:38PM +0530, Bharata B Rao wrote:
> > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > Creating this core device will result in creation of all the CPU thread
> > devices that are part of this core.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Looks good overall, a few minor points below.
> 
> > ---
> >  hw/ppc/Makefile.objs            |   1 +
> >  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
> >  3 files changed, 243 insertions(+)
> >  create mode 100644 hw/ppc/spapr_cpu_core.c
> >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > 
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index c1ffc77..5cc6608 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >  obj-y += spapr_pci_vfio.o
> >  endif
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > new file mode 100644
> > index 0000000..c44eb61
> > --- /dev/null
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -0,0 +1,210 @@
> > +/*
> > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > + *
> > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#include "hw/cpu/core.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> > +#include "hw/ppc/spapr.h"
> > +#include "hw/boards.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/visitor.h"
> > +#include <sysemu/cpus.h>
> > +
> > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > +{
> > +    Error **errp = opaque;
> > +
> > +    object_property_set_bool(child, true, "realized", errp);
> > +    if (*errp) {
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    Error *local_err = NULL;
> > +
> > +    if (!core->nr_threads) {
> > +        error_setg(errp, "nr_threads property can't be 0");
> > +        return;
> > +    }
> > +
> > +    if (!core->cpu_model) {
> > +        error_setg(errp, "cpu_model property isn't set");
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * TODO: If slot isn't specified, plug this core into
> > +     * an existing empty slot.
> > +     */
> > +    if (!core->slot) {
> > +        error_setg(errp, "slot property isn't set");
> > +        return;
> > +    }
> > +
> > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > +                             &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> > +}
> > +
> > +/*
> > + * This creates the CPU threads for a given @core.
> > + *
> > + * In order to create the threads, we need two inputs - number of
> > + * threads and the cpu_model. These are set as core object's properties.
> > + * When both of them become available/set, this routine will be called from
> > + * either property's set handler to create the threads.
> > + *
> > + * TODO: Dependence of threads creation on two properties is resulting
> > + * in this not-so-clean way of creating threads from either of the
> > + * property setters based on the order in which they get set. Check if
> > + * this can be handled in a better manner.
> > + */
> > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < core->nr_threads; i++) {
> > +        char id[32];
> > +        char type[32];
> > +
> > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > +                 TYPE_POWERPC_CPU);
> 
> I think you want to use cpu_class_by_name() rather than explicitly
> constructing the class name here.

Good point, started using ObjectClass.

> 
> > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> > +
> > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > +                                  errp);
> > +    }
> > +}
> > +
> > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    return core->slot;
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > +                                              Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    core->slot = g_strdup(val);
> > +}
> > +
> > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    return core->cpu_model;
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> > +                                              Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +    MachineState *machine = MACHINE(qdev_get_machine());
> > +
> > +    /*
> > +     * cpu_model can't be different from what is specified with -cpu
> > +     */
> > +    if (strcmp(val, machine->cpu_model)) {
> > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> > +       return;
> > +    }
> 
> I'm not sure if this is the right place to test this.  Possibly it
> should be tested in the machine hotplug handler instead.

Not sure about the right place to validate this and nr_threads property. But
it essentially boils down to backing out from property setter after
object init vs backing out during realization.

Regards,
Bharata.

On 25.02.2016 17:22, Bharata B Rao wrote:
> Add sPAPR specific CPU core device that is based on generic CPU core device.
> Creating this core device will result in creation of all the CPU thread
> devices that are part of this core.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
...
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> new file mode 100644
> index 0000000..c44eb61
> --- /dev/null
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -0,0 +1,210 @@
> +/*
> + * sPAPR CPU core device, acts as container of CPU thread devices.
> + *
> + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/cpu/core.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/boards.h"
> +#include "qemu/error-report.h"
> +#include "qapi/visitor.h"
> +#include <sysemu/cpus.h>
> +
> +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> +{
> +    Error **errp = opaque;
> +
> +    object_property_set_bool(child, true, "realized", errp);
> +    if (*errp) {
> +        return 1;
> +    }
> +    return 0;
> +}
> +
> +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    Error *local_err = NULL;
> +
> +    if (!core->nr_threads) {
> +        error_setg(errp, "nr_threads property can't be 0");
> +        return;
> +    }
> +
> +    if (!core->cpu_model) {
> +        error_setg(errp, "cpu_model property isn't set");
> +        return;
> +    }
> +
> +    /*
> +     * TODO: If slot isn't specified, plug this core into
> +     * an existing empty slot.
> +     */
> +    if (!core->slot) {
> +        error_setg(errp, "slot property isn't set");
> +        return;
> +    }
> +
> +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> +                             &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> +}
> +
> +/*
> + * This creates the CPU threads for a given @core.
> + *
> + * In order to create the threads, we need two inputs - number of
> + * threads and the cpu_model. These are set as core object's properties.
> + * When both of them become available/set, this routine will be called from
> + * either property's set handler to create the threads.
> + *
> + * TODO: Dependence of threads creation on two properties is resulting
> + * in this not-so-clean way of creating threads from either of the
> + * property setters based on the order in which they get set. Check if
> + * this can be handled in a better manner.
> + */
> +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> +{
> +    int i;
> +
> +    for (i = 0; i < core->nr_threads; i++) {
> +        char id[32];
> +        char type[32];
> +
> +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> +                 TYPE_POWERPC_CPU);
> +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> +
> +        snprintf(id, sizeof(id), "thread[%d]", i);
> +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> +                                  errp);

Need to check errp here to see whether something went wrong?

> +    }
> +}
> +
> +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    return core->slot;
> +}
> +
> +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> +                                              Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    core->slot = g_strdup(val);
> +}
> +
> +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    return core->cpu_model;
> +}
> +
> +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> +                                              Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +
> +    /*
> +     * cpu_model can't be different from what is specified with -cpu
> +     */
> +    if (strcmp(val, machine->cpu_model)) {
> +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);

s/should/must/

> +       return;
> +    }
> +
> +    core->cpu_model = g_strdup(val);
> +    if (core->nr_threads && core->cpu_model) {
> +        spapr_cpu_core_create_threads(core, errp);
> +    }
> +}
> +
> +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> +                                               const char *name, void *opaque,
> +                                               Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +    int64_t value = core->nr_threads;
> +
> +    visit_type_int(v, name, &value, errp);
> +}
> +
> +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> +                                               const char *name, void *opaque,
> +                                               Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_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;
> +    }
> +
> +    /* Allow only homogeneous configuration */
> +    if (value != smp_threads) {
> +        error_setg(errp, "nr_threads should be %d", smp_threads);

s/should/must/

> +        return;
> +    }
> +
> +    core->nr_threads = value;
> +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));

I think it's preferable to use g_new0 for such allocations instead.

Also, should this memory maybe be freed during instance_finalize again,
so that there is no memory leak here in case the cores are deleted again
one day?

> +    if (core->nr_threads && core->cpu_model) {
> +        spapr_cpu_core_create_threads(core, errp);
> +    }
> +}
> +
> +static void spapr_cpu_core_instance_init(Object *obj)
> +{
> +    object_property_add(obj, "nr_threads", "int",
> +                        spapr_cpu_core_prop_get_nr_threads,
> +                        spapr_cpu_core_prop_set_nr_threads,
> +                        NULL, NULL, NULL);
> +    object_property_add_str(obj, "cpu_model",
> +                            spapr_cpu_core_prop_get_cpu_model,
> +                            spapr_cpu_core_prop_set_cpu_model,
> +                            NULL);
> +    object_property_add_str(obj, "slot",
> +                            spapr_cpu_core_prop_get_slot,
> +                            spapr_cpu_core_prop_set_slot,
> +                            NULL);
> +}
> +
> +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = spapr_cpu_core_realize;
> +}
> +
> +static const TypeInfo spapr_cpu_core_type_info = {
> +    .name = TYPE_SPAPR_CPU_CORE,
> +    .parent = TYPE_CPU_CORE,
> +    .instance_init = spapr_cpu_core_instance_init,
> +    .instance_size = sizeof(sPAPRCPUCore),
> +    .class_init = spapr_cpu_core_class_init,
> +};
> +
> +static void spapr_cpu_core_register_types(void)
> +{
> +    type_register_static(&spapr_cpu_core_type_info);
> +}
> +
> +type_init(spapr_cpu_core_register_types)
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> new file mode 100644
> index 0000000..ed9bc7f
> --- /dev/null
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -0,0 +1,32 @@
> +/*
> + * sPAPR CPU core device.
> + *
> + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef HW_SPAPR_CPU_CORE_H
> +#define HW_SPAPR_CPU_CORE_H
> +
> +#include "hw/qdev.h"
> +#include "hw/cpu/core.h"
> +
> +#define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
> +#define SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(sPAPRCPUCore, (obj), TYPE_SPAPR_CPU_CORE)
> +
> +typedef struct sPAPRCPUCore {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +
> +    /*< public >*/
> +    int nr_threads;
> +    char *cpu_model;
> +    char *slot;

<bikeshedpainting>
I'd maybe call that "slot_name" instead ... if you only call it "slot",
I'd rather think of an integer value than a string here.
</bikeshedpainting>

> +    PowerPCCPU *threads;
> +} sPAPRCPUCore;
> +
> +#define SPAPR_CPU_CORE_SLOT_PROP "slot"
> +
> +#endif
> 

 Thomas

Quoting Bharata B Rao (2016-02-25 10:22:38)
> Add sPAPR specific CPU core device that is based on generic CPU core device.
> Creating this core device will result in creation of all the CPU thread
> devices that are part of this core.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/Makefile.objs            |   1 +
>  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
>  3 files changed, 243 insertions(+)
>  create mode 100644 hw/ppc/spapr_cpu_core.c
>  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index c1ffc77..5cc6608 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> new file mode 100644
> index 0000000..c44eb61
> --- /dev/null
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -0,0 +1,210 @@
> +/*
> + * sPAPR CPU core device, acts as container of CPU thread devices.
> + *
> + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/cpu/core.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/boards.h"
> +#include "qemu/error-report.h"
> +#include "qapi/visitor.h"
> +#include <sysemu/cpus.h>
> +
> +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> +{
> +    Error **errp = opaque;
> +
> +    object_property_set_bool(child, true, "realized", errp);
> +    if (*errp) {
> +        return 1;
> +    }
> +    return 0;
> +}
> +
> +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    Error *local_err = NULL;
> +
> +    if (!core->nr_threads) {
> +        error_setg(errp, "nr_threads property can't be 0");
> +        return;
> +    }
> +
> +    if (!core->cpu_model) {
> +        error_setg(errp, "cpu_model property isn't set");
> +        return;
> +    }
> +
> +    /*
> +     * TODO: If slot isn't specified, plug this core into
> +     * an existing empty slot.
> +     */
> +    if (!core->slot) {
> +        error_setg(errp, "slot property isn't set");
> +        return;
> +    }
> +
> +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> +                             &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> +}
> +
> +/*
> + * This creates the CPU threads for a given @core.
> + *
> + * In order to create the threads, we need two inputs - number of
> + * threads and the cpu_model. These are set as core object's properties.
> + * When both of them become available/set, this routine will be called from
> + * either property's set handler to create the threads.
> + *
> + * TODO: Dependence of threads creation on two properties is resulting
> + * in this not-so-clean way of creating threads from either of the
> + * property setters based on the order in which they get set. Check if
> + * this can be handled in a better manner.
> + */
> +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> +{
> +    int i;
> +
> +    for (i = 0; i < core->nr_threads; i++) {
> +        char id[32];
> +        char type[32];
> +
> +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> +                 TYPE_POWERPC_CPU);
> +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> +
> +        snprintf(id, sizeof(id), "thread[%d]", i);
> +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> +                                  errp);
> +    }
> +}
> +
> +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    return core->slot;
> +}
> +
> +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> +                                              Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    core->slot = g_strdup(val);
> +}
> +
> +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    return core->cpu_model;
> +}
> +
> +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> +                                              Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +
> +    /*
> +     * cpu_model can't be different from what is specified with -cpu
> +     */
> +    if (strcmp(val, machine->cpu_model)) {
> +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> +       return;
> +    }
> +
> +    core->cpu_model = g_strdup(val);
> +    if (core->nr_threads && core->cpu_model) {
> +        spapr_cpu_core_create_threads(core, errp);
> +    }
> +}
> +
> +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> +                                               const char *name, void *opaque,
> +                                               Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +    int64_t value = core->nr_threads;
> +
> +    visit_type_int(v, name, &value, errp);
> +}
> +
> +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> +                                               const char *name, void *opaque,
> +                                               Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_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;
> +    }
> +
> +    /* Allow only homogeneous configuration */
> +    if (value != smp_threads) {
> +        error_setg(errp, "nr_threads should be %d", smp_threads);
> +        return;
> +    }
> +
> +    core->nr_threads = value;
> +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> +
> +    if (core->nr_threads && core->cpu_model) {
> +        spapr_cpu_core_create_threads(core, errp);
> +    }
> +}
> +
> +static void spapr_cpu_core_instance_init(Object *obj)
> +{
> +    object_property_add(obj, "nr_threads", "int",
> +                        spapr_cpu_core_prop_get_nr_threads,
> +                        spapr_cpu_core_prop_set_nr_threads,
> +                        NULL, NULL, NULL);
> +    object_property_add_str(obj, "cpu_model",
> +                            spapr_cpu_core_prop_get_cpu_model,
> +                            spapr_cpu_core_prop_set_cpu_model,
> +                            NULL);
> +    object_property_add_str(obj, "slot",
> +                            spapr_cpu_core_prop_get_slot,
> +                            spapr_cpu_core_prop_set_slot,
> +                            NULL);

"slot" seems intended to be a machine-agnostic of mapping device
types discovered from qmp_query_cpu_slots() to an appropriate
"bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
It seems like maybe TYPE_CPU_CORE is a better place, but then on
x86 I suppose it might be TYPE_CPU_SOCKET or something instead...

It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
right approach, but I don't know how we could expose that as
a property. I guess it's somewhat implied that this "interface"
exists if qmp_query_cpu_slots() returns the type, but I wonder
if something a bit more formal should be modeled to make the
implementation requirements a bit clearer.

Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
class method, expose them via "slot" property, then have the
defaults generate "not implemented" errors?

> +}
> +
> +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = spapr_cpu_core_realize;
> +}
> +
> +static const TypeInfo spapr_cpu_core_type_info = {
> +    .name = TYPE_SPAPR_CPU_CORE,
> +    .parent = TYPE_CPU_CORE,
> +    .instance_init = spapr_cpu_core_instance_init,
> +    .instance_size = sizeof(sPAPRCPUCore),
> +    .class_init = spapr_cpu_core_class_init,
> +};
> +
> +static void spapr_cpu_core_register_types(void)
> +{
> +    type_register_static(&spapr_cpu_core_type_info);
> +}
> +
> +type_init(spapr_cpu_core_register_types)
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> new file mode 100644
> index 0000000..ed9bc7f
> --- /dev/null
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -0,0 +1,32 @@
> +/*
> + * sPAPR CPU core device.
> + *
> + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef HW_SPAPR_CPU_CORE_H
> +#define HW_SPAPR_CPU_CORE_H
> +
> +#include "hw/qdev.h"
> +#include "hw/cpu/core.h"
> +
> +#define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
> +#define SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(sPAPRCPUCore, (obj), TYPE_SPAPR_CPU_CORE)
> +
> +typedef struct sPAPRCPUCore {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +
> +    /*< public >*/
> +    int nr_threads;
> +    char *cpu_model;
> +    char *slot;
> +    PowerPCCPU *threads;
> +} sPAPRCPUCore;
> +
> +#define SPAPR_CPU_CORE_SLOT_PROP "slot"
> +
> +#endif
> -- 
> 2.1.0
>

On Fri, Feb 26, 2016 at 12:13:39PM -0600, Michael Roth wrote:
> Quoting Bharata B Rao (2016-02-25 10:22:38)
> > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > Creating this core device will result in creation of all the CPU thread
> > devices that are part of this core.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/Makefile.objs            |   1 +
> >  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
> >  3 files changed, 243 insertions(+)
> >  create mode 100644 hw/ppc/spapr_cpu_core.c
> >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > 
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index c1ffc77..5cc6608 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >  obj-y += spapr_pci_vfio.o
> >  endif
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > new file mode 100644
> > index 0000000..c44eb61
> > --- /dev/null
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -0,0 +1,210 @@
> > +/*
> > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > + *
> > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#include "hw/cpu/core.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> > +#include "hw/ppc/spapr.h"
> > +#include "hw/boards.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/visitor.h"
> > +#include <sysemu/cpus.h>
> > +
> > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > +{
> > +    Error **errp = opaque;
> > +
> > +    object_property_set_bool(child, true, "realized", errp);
> > +    if (*errp) {
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    Error *local_err = NULL;
> > +
> > +    if (!core->nr_threads) {
> > +        error_setg(errp, "nr_threads property can't be 0");
> > +        return;
> > +    }
> > +
> > +    if (!core->cpu_model) {
> > +        error_setg(errp, "cpu_model property isn't set");
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * TODO: If slot isn't specified, plug this core into
> > +     * an existing empty slot.
> > +     */
> > +    if (!core->slot) {
> > +        error_setg(errp, "slot property isn't set");
> > +        return;
> > +    }
> > +
> > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > +                             &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> > +}
> > +
> > +/*
> > + * This creates the CPU threads for a given @core.
> > + *
> > + * In order to create the threads, we need two inputs - number of
> > + * threads and the cpu_model. These are set as core object's properties.
> > + * When both of them become available/set, this routine will be called from
> > + * either property's set handler to create the threads.
> > + *
> > + * TODO: Dependence of threads creation on two properties is resulting
> > + * in this not-so-clean way of creating threads from either of the
> > + * property setters based on the order in which they get set. Check if
> > + * this can be handled in a better manner.
> > + */
> > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < core->nr_threads; i++) {
> > +        char id[32];
> > +        char type[32];
> > +
> > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > +                 TYPE_POWERPC_CPU);
> > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> > +
> > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > +                                  errp);
> > +    }
> > +}
> > +
> > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    return core->slot;
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > +                                              Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    core->slot = g_strdup(val);
> > +}
> > +
> > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    return core->cpu_model;
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> > +                                              Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +    MachineState *machine = MACHINE(qdev_get_machine());
> > +
> > +    /*
> > +     * cpu_model can't be different from what is specified with -cpu
> > +     */
> > +    if (strcmp(val, machine->cpu_model)) {
> > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> > +       return;
> > +    }
> > +
> > +    core->cpu_model = g_strdup(val);
> > +    if (core->nr_threads && core->cpu_model) {
> > +        spapr_cpu_core_create_threads(core, errp);
> > +    }
> > +}
> > +
> > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> > +                                               const char *name, void *opaque,
> > +                                               Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +    int64_t value = core->nr_threads;
> > +
> > +    visit_type_int(v, name, &value, errp);
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> > +                                               const char *name, void *opaque,
> > +                                               Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_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;
> > +    }
> > +
> > +    /* Allow only homogeneous configuration */
> > +    if (value != smp_threads) {
> > +        error_setg(errp, "nr_threads should be %d", smp_threads);
> > +        return;
> > +    }
> > +
> > +    core->nr_threads = value;
> > +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> > +
> > +    if (core->nr_threads && core->cpu_model) {
> > +        spapr_cpu_core_create_threads(core, errp);
> > +    }
> > +}
> > +
> > +static void spapr_cpu_core_instance_init(Object *obj)
> > +{
> > +    object_property_add(obj, "nr_threads", "int",
> > +                        spapr_cpu_core_prop_get_nr_threads,
> > +                        spapr_cpu_core_prop_set_nr_threads,
> > +                        NULL, NULL, NULL);
> > +    object_property_add_str(obj, "cpu_model",
> > +                            spapr_cpu_core_prop_get_cpu_model,
> > +                            spapr_cpu_core_prop_set_cpu_model,
> > +                            NULL);
> > +    object_property_add_str(obj, "slot",
> > +                            spapr_cpu_core_prop_get_slot,
> > +                            spapr_cpu_core_prop_set_slot,
> > +                            NULL);
> 
> "slot" seems intended to be a machine-agnostic of mapping device
> types discovered from qmp_query_cpu_slots() to an appropriate
> "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> It seems like maybe TYPE_CPU_CORE is a better place, but then on
> x86 I suppose it might be TYPE_CPU_SOCKET or something instead...
> 
> It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> right approach, but I don't know how we could expose that as
> a property. I guess it's somewhat implied that this "interface"
> exists if qmp_query_cpu_slots() returns the type, but I wonder
> if something a bit more formal should be modeled to make the
> implementation requirements a bit clearer.
> 
> Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> class method, expose them via "slot" property, then have the
> defaults generate "not implemented" errors?

Yeah, TBH the "slot" stuff has me a bit confused still.  I'm intending
to have a closer look at it once the more obvious cleanups are
addressed.

On Fri, Feb 26, 2016 at 11:46:19AM +0100, Thomas Huth wrote:
> On 25.02.2016 17:22, Bharata B Rao wrote:
> > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > Creating this core device will result in creation of all the CPU thread
> > devices that are part of this core.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> ...
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > new file mode 100644
> > index 0000000..c44eb61
> > --- /dev/null
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -0,0 +1,210 @@
> > +/*
> > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > + *
> > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#include "hw/cpu/core.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> > +#include "hw/ppc/spapr.h"
> > +#include "hw/boards.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/visitor.h"
> > +#include <sysemu/cpus.h>
> > +
> > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > +{
> > +    Error **errp = opaque;
> > +
> > +    object_property_set_bool(child, true, "realized", errp);
> > +    if (*errp) {
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    Error *local_err = NULL;
> > +
> > +    if (!core->nr_threads) {
> > +        error_setg(errp, "nr_threads property can't be 0");
> > +        return;
> > +    }
> > +
> > +    if (!core->cpu_model) {
> > +        error_setg(errp, "cpu_model property isn't set");
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * TODO: If slot isn't specified, plug this core into
> > +     * an existing empty slot.
> > +     */
> > +    if (!core->slot) {
> > +        error_setg(errp, "slot property isn't set");
> > +        return;
> > +    }
> > +
> > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > +                             &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> > +}
> > +
> > +/*
> > + * This creates the CPU threads for a given @core.
> > + *
> > + * In order to create the threads, we need two inputs - number of
> > + * threads and the cpu_model. These are set as core object's properties.
> > + * When both of them become available/set, this routine will be called from
> > + * either property's set handler to create the threads.
> > + *
> > + * TODO: Dependence of threads creation on two properties is resulting
> > + * in this not-so-clean way of creating threads from either of the
> > + * property setters based on the order in which they get set. Check if
> > + * this can be handled in a better manner.
> > + */
> > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < core->nr_threads; i++) {
> > +        char id[32];
> > +        char type[32];
> > +
> > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > +                 TYPE_POWERPC_CPU);
> > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> > +
> > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > +                                  errp);
> 
> Need to check errp here to see whether something went wrong?

Yes, I will use local_err and them propagate.

> 
> > +    }
> > +}
> > +
> > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    return core->slot;
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > +                                              Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    core->slot = g_strdup(val);
> > +}
> > +
> > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    return core->cpu_model;
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> > +                                              Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +    MachineState *machine = MACHINE(qdev_get_machine());
> > +
> > +    /*
> > +     * cpu_model can't be different from what is specified with -cpu
> > +     */
> > +    if (strcmp(val, machine->cpu_model)) {
> > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> 
> s/should/must/

sure :)

> 
> > +       return;
> > +    }
> > +
> > +    core->cpu_model = g_strdup(val);
> > +    if (core->nr_threads && core->cpu_model) {
> > +        spapr_cpu_core_create_threads(core, errp);
> > +    }
> > +}
> > +
> > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> > +                                               const char *name, void *opaque,
> > +                                               Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +    int64_t value = core->nr_threads;
> > +
> > +    visit_type_int(v, name, &value, errp);
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> > +                                               const char *name, void *opaque,
> > +                                               Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_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;
> > +    }
> > +
> > +    /* Allow only homogeneous configuration */
> > +    if (value != smp_threads) {
> > +        error_setg(errp, "nr_threads should be %d", smp_threads);
> 
> s/should/must/
> 
> > +        return;
> > +    }
> > +
> > +    core->nr_threads = value;
> > +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> 
> I think it's preferable to use g_new0 for such allocations instead.

Ok.

> 
> Also, should this memory maybe be freed during instance_finalize again,
> so that there is no memory leak here in case the cores are deleted again
> one day?

I will be forced to care of this when I add hot removal in the next version.

> 
> > +    if (core->nr_threads && core->cpu_model) {
> > +        spapr_cpu_core_create_threads(core, errp);
> > +    }
> > +}
> > +
> > +static void spapr_cpu_core_instance_init(Object *obj)
> > +{
> > +    object_property_add(obj, "nr_threads", "int",
> > +                        spapr_cpu_core_prop_get_nr_threads,
> > +                        spapr_cpu_core_prop_set_nr_threads,
> > +                        NULL, NULL, NULL);
> > +    object_property_add_str(obj, "cpu_model",
> > +                            spapr_cpu_core_prop_get_cpu_model,
> > +                            spapr_cpu_core_prop_set_cpu_model,
> > +                            NULL);
> > +    object_property_add_str(obj, "slot",
> > +                            spapr_cpu_core_prop_get_slot,
> > +                            spapr_cpu_core_prop_set_slot,
> > +                            NULL);
> > +}
> > +
> > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > +
> > +    dc->realize = spapr_cpu_core_realize;
> > +}
> > +
> > +static const TypeInfo spapr_cpu_core_type_info = {
> > +    .name = TYPE_SPAPR_CPU_CORE,
> > +    .parent = TYPE_CPU_CORE,
> > +    .instance_init = spapr_cpu_core_instance_init,
> > +    .instance_size = sizeof(sPAPRCPUCore),
> > +    .class_init = spapr_cpu_core_class_init,
> > +};
> > +
> > +static void spapr_cpu_core_register_types(void)
> > +{
> > +    type_register_static(&spapr_cpu_core_type_info);
> > +}
> > +
> > +type_init(spapr_cpu_core_register_types)
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > new file mode 100644
> > index 0000000..ed9bc7f
> > --- /dev/null
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -0,0 +1,32 @@
> > +/*
> > + * sPAPR CPU core device.
> > + *
> > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#ifndef HW_SPAPR_CPU_CORE_H
> > +#define HW_SPAPR_CPU_CORE_H
> > +
> > +#include "hw/qdev.h"
> > +#include "hw/cpu/core.h"
> > +
> > +#define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
> > +#define SPAPR_CPU_CORE(obj) \
> > +    OBJECT_CHECK(sPAPRCPUCore, (obj), TYPE_SPAPR_CPU_CORE)
> > +
> > +typedef struct sPAPRCPUCore {
> > +    /*< private >*/
> > +    DeviceState parent_obj;
> > +
> > +    /*< public >*/
> > +    int nr_threads;
> > +    char *cpu_model;
> > +    char *slot;
> 
> <bikeshedpainting>
> I'd maybe call that "slot_name" instead ... if you only call it "slot",
> I'd rather think of an integer value than a string here.
> </bikeshedpainting>

Point taken, pc-dimm has slot which is a number :)

Regards,
Bharata.

On Fri, Feb 26, 2016 at 12:13:39PM -0600, Michael Roth wrote:
> Quoting Bharata B Rao (2016-02-25 10:22:38)
> > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > Creating this core device will result in creation of all the CPU thread
> > devices that are part of this core.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/Makefile.objs            |   1 +
> >  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
> >  3 files changed, 243 insertions(+)
> >  create mode 100644 hw/ppc/spapr_cpu_core.c
> >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > 
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index c1ffc77..5cc6608 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >  obj-y += spapr_pci_vfio.o
> >  endif
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > new file mode 100644
> > index 0000000..c44eb61
> > --- /dev/null
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -0,0 +1,210 @@
> > +/*
> > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > + *
> > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#include "hw/cpu/core.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> > +#include "hw/ppc/spapr.h"
> > +#include "hw/boards.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/visitor.h"
> > +#include <sysemu/cpus.h>
> > +
> > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > +{
> > +    Error **errp = opaque;
> > +
> > +    object_property_set_bool(child, true, "realized", errp);
> > +    if (*errp) {
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    Error *local_err = NULL;
> > +
> > +    if (!core->nr_threads) {
> > +        error_setg(errp, "nr_threads property can't be 0");
> > +        return;
> > +    }
> > +
> > +    if (!core->cpu_model) {
> > +        error_setg(errp, "cpu_model property isn't set");
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * TODO: If slot isn't specified, plug this core into
> > +     * an existing empty slot.
> > +     */
> > +    if (!core->slot) {
> > +        error_setg(errp, "slot property isn't set");
> > +        return;
> > +    }
> > +
> > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > +                             &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> > +}
> > +
> > +/*
> > + * This creates the CPU threads for a given @core.
> > + *
> > + * In order to create the threads, we need two inputs - number of
> > + * threads and the cpu_model. These are set as core object's properties.
> > + * When both of them become available/set, this routine will be called from
> > + * either property's set handler to create the threads.
> > + *
> > + * TODO: Dependence of threads creation on two properties is resulting
> > + * in this not-so-clean way of creating threads from either of the
> > + * property setters based on the order in which they get set. Check if
> > + * this can be handled in a better manner.
> > + */
> > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < core->nr_threads; i++) {
> > +        char id[32];
> > +        char type[32];
> > +
> > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > +                 TYPE_POWERPC_CPU);
> > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> > +
> > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > +                                  errp);
> > +    }
> > +}
> > +
> > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    return core->slot;
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > +                                              Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    core->slot = g_strdup(val);
> > +}
> > +
> > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +
> > +    return core->cpu_model;
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> > +                                              Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +    MachineState *machine = MACHINE(qdev_get_machine());
> > +
> > +    /*
> > +     * cpu_model can't be different from what is specified with -cpu
> > +     */
> > +    if (strcmp(val, machine->cpu_model)) {
> > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> > +       return;
> > +    }
> > +
> > +    core->cpu_model = g_strdup(val);
> > +    if (core->nr_threads && core->cpu_model) {
> > +        spapr_cpu_core_create_threads(core, errp);
> > +    }
> > +}
> > +
> > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> > +                                               const char *name, void *opaque,
> > +                                               Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > +    int64_t value = core->nr_threads;
> > +
> > +    visit_type_int(v, name, &value, errp);
> > +}
> > +
> > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> > +                                               const char *name, void *opaque,
> > +                                               Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_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;
> > +    }
> > +
> > +    /* Allow only homogeneous configuration */
> > +    if (value != smp_threads) {
> > +        error_setg(errp, "nr_threads should be %d", smp_threads);
> > +        return;
> > +    }
> > +
> > +    core->nr_threads = value;
> > +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> > +
> > +    if (core->nr_threads && core->cpu_model) {
> > +        spapr_cpu_core_create_threads(core, errp);
> > +    }
> > +}
> > +
> > +static void spapr_cpu_core_instance_init(Object *obj)
> > +{
> > +    object_property_add(obj, "nr_threads", "int",
> > +                        spapr_cpu_core_prop_get_nr_threads,
> > +                        spapr_cpu_core_prop_set_nr_threads,
> > +                        NULL, NULL, NULL);
> > +    object_property_add_str(obj, "cpu_model",
> > +                            spapr_cpu_core_prop_get_cpu_model,
> > +                            spapr_cpu_core_prop_set_cpu_model,
> > +                            NULL);
> > +    object_property_add_str(obj, "slot",
> > +                            spapr_cpu_core_prop_get_slot,
> > +                            spapr_cpu_core_prop_set_slot,
> > +                            NULL);
> 
> "slot" seems intended to be a machine-agnostic of mapping device
> types discovered from qmp_query_cpu_slots() to an appropriate
> "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> It seems like maybe TYPE_CPU_CORE is a better place, but then on
> x86 I suppose it might be TYPE_CPU_SOCKET or something instead...

Correct.

> 
> It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> right approach, but I don't know how we could expose that as
> a property. I guess it's somewhat implied that this "interface"
> exists if qmp_query_cpu_slots() returns the type, but I wonder
> if something a bit more formal should be modeled to make the
> implementation requirements a bit clearer.
> 
> Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> class method, expose them via "slot" property, then have the
> defaults generate "not implemented" errors?

Yes makes sense. In fact David has often times said that generic
properties/routines should be pushed to base class wherever possible.

I didn't do that in this first iteration to keep the generic changes
as minimum as possible, but yes slot should be a property of the
base class of core or socket.

Regards,
Bharata.

On Mon, 29 Feb 2016 11:20:19 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Fri, Feb 26, 2016 at 12:13:39PM -0600, Michael Roth wrote:
> > Quoting Bharata B Rao (2016-02-25 10:22:38)  
> > > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > > Creating this core device will result in creation of all the CPU thread
> > > devices that are part of this core.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/Makefile.objs            |   1 +
> > >  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
> > >  3 files changed, 243 insertions(+)
> > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > 
> > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > index c1ffc77..5cc6608 100644
> > > --- a/hw/ppc/Makefile.objs
> > > +++ b/hw/ppc/Makefile.objs
> > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > >  obj-y += spapr_pci_vfio.o
> > >  endif
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > new file mode 100644
> > > index 0000000..c44eb61
> > > --- /dev/null
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -0,0 +1,210 @@
> > > +/*
> > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > + *
> > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +#include "hw/cpu/core.h"
> > > +#include "hw/ppc/spapr_cpu_core.h"
> > > +#include "hw/ppc/spapr.h"
> > > +#include "hw/boards.h"
> > > +#include "qemu/error-report.h"
> > > +#include "qapi/visitor.h"
> > > +#include <sysemu/cpus.h>
> > > +
> > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > +{
> > > +    Error **errp = opaque;
> > > +
> > > +    object_property_set_bool(child, true, "realized", errp);
> > > +    if (*errp) {
> > > +        return 1;
> > > +    }
> > > +    return 0;
> > > +}
> > > +
> > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > +    Error *local_err = NULL;
> > > +
> > > +    if (!core->nr_threads) {
> > > +        error_setg(errp, "nr_threads property can't be 0");
> > > +        return;
> > > +    }
> > > +
> > > +    if (!core->cpu_model) {
> > > +        error_setg(errp, "cpu_model property isn't set");
> > > +        return;
> > > +    }
> > > +
> > > +    /*
> > > +     * TODO: If slot isn't specified, plug this core into
> > > +     * an existing empty slot.
> > > +     */
> > > +    if (!core->slot) {
> > > +        error_setg(errp, "slot property isn't set");
> > > +        return;
> > > +    }
> > > +
> > > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > > +                             &local_err);
> > > +    if (local_err) {
> > > +        error_propagate(errp, local_err);
> > > +        return;
> > > +    }
> > > +
> > > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> > > +}
> > > +
> > > +/*
> > > + * This creates the CPU threads for a given @core.
> > > + *
> > > + * In order to create the threads, we need two inputs - number of
> > > + * threads and the cpu_model. These are set as core object's properties.
> > > + * When both of them become available/set, this routine will be called from
> > > + * either property's set handler to create the threads.
> > > + *
> > > + * TODO: Dependence of threads creation on two properties is resulting
> > > + * in this not-so-clean way of creating threads from either of the
> > > + * property setters based on the order in which they get set. Check if
> > > + * this can be handled in a better manner.
> > > + */
> > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> > > +{
> > > +    int i;
> > > +
> > > +    for (i = 0; i < core->nr_threads; i++) {
> > > +        char id[32];
> > > +        char type[32];
> > > +
> > > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > > +                 TYPE_POWERPC_CPU);
> > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> > > +
> > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > +                                  errp);
> > > +    }
> > > +}
> > > +
> > > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > > +{
> > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > +
> > > +    return core->slot;
> > > +}
> > > +
> > > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > > +                                              Error **errp)
> > > +{
> > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > +
> > > +    core->slot = g_strdup(val);
> > > +}
> > > +
> > > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> > > +{
> > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > +
> > > +    return core->cpu_model;
> > > +}
> > > +
> > > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> > > +                                              Error **errp)
> > > +{
> > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > +
> > > +    /*
> > > +     * cpu_model can't be different from what is specified with -cpu
> > > +     */
> > > +    if (strcmp(val, machine->cpu_model)) {
> > > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> > > +       return;
> > > +    }
> > > +
> > > +    core->cpu_model = g_strdup(val);
> > > +    if (core->nr_threads && core->cpu_model) {
> > > +        spapr_cpu_core_create_threads(core, errp);
> > > +    }
> > > +}
> > > +
> > > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> > > +                                               const char *name, void *opaque,
> > > +                                               Error **errp)
> > > +{
> > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > +    int64_t value = core->nr_threads;
> > > +
> > > +    visit_type_int(v, name, &value, errp);
> > > +}
> > > +
> > > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> > > +                                               const char *name, void *opaque,
> > > +                                               Error **errp)
> > > +{
> > > +    sPAPRCPUCore *core = SPAPR_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;
> > > +    }
> > > +
> > > +    /* Allow only homogeneous configuration */
> > > +    if (value != smp_threads) {
> > > +        error_setg(errp, "nr_threads should be %d", smp_threads);
> > > +        return;
> > > +    }
> > > +
> > > +    core->nr_threads = value;
> > > +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> > > +
> > > +    if (core->nr_threads && core->cpu_model) {
> > > +        spapr_cpu_core_create_threads(core, errp);
> > > +    }
> > > +}
> > > +
> > > +static void spapr_cpu_core_instance_init(Object *obj)
> > > +{
> > > +    object_property_add(obj, "nr_threads", "int",
> > > +                        spapr_cpu_core_prop_get_nr_threads,
> > > +                        spapr_cpu_core_prop_set_nr_threads,
> > > +                        NULL, NULL, NULL);
> > > +    object_property_add_str(obj, "cpu_model",
> > > +                            spapr_cpu_core_prop_get_cpu_model,
> > > +                            spapr_cpu_core_prop_set_cpu_model,
> > > +                            NULL);
> > > +    object_property_add_str(obj, "slot",
> > > +                            spapr_cpu_core_prop_get_slot,
> > > +                            spapr_cpu_core_prop_set_slot,
> > > +                            NULL);  
> > 
> > "slot" seems intended to be a machine-agnostic of mapping device
> > types discovered from qmp_query_cpu_slots() to an appropriate
> > "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> > It seems like maybe TYPE_CPU_CORE is a better place, but then on
> > x86 I suppose it might be TYPE_CPU_SOCKET or something instead...  
> 
> Correct.
> 
> > 
> > It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> > right approach, but I don't know how we could expose that as
> > a property. I guess it's somewhat implied that this "interface"
> > exists if qmp_query_cpu_slots() returns the type, but I wonder
> > if something a bit more formal should be modeled to make the
> > implementation requirements a bit clearer.
> > 
> > Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> > class method, expose them via "slot" property, then have the
> > defaults generate "not implemented" errors?  
> 
> Yes makes sense. In fact David has often times said that generic
> properties/routines should be pushed to base class wherever possible.
> 
> I didn't do that in this first iteration to keep the generic changes
> as minimum as possible, but yes slot should be a property of the
> base class of core or socket.
Then what will happen to slot if there isn't any core/socket device
to query it, i.e. cpu hasn't been plugged in yet?
To me slot looks like a machine belonged feature.

> 
> Regards,
> Bharata.
>

On Mon, Feb 29, 2016 at 11:03:16AM +0100, Igor Mammedov wrote:
> On Mon, 29 Feb 2016 11:20:19 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Fri, Feb 26, 2016 at 12:13:39PM -0600, Michael Roth wrote:
> > > Quoting Bharata B Rao (2016-02-25 10:22:38)  
> > > > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > > > Creating this core device will result in creation of all the CPU thread
> > > > devices that are part of this core.
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/ppc/Makefile.objs            |   1 +
> > > >  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
> > > >  3 files changed, 243 insertions(+)
> > > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > > 
> > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > index c1ffc77..5cc6608 100644
> > > > --- a/hw/ppc/Makefile.objs
> > > > +++ b/hw/ppc/Makefile.objs
> > > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > > >  obj-y += spapr_pci_vfio.o
> > > >  endif
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > new file mode 100644
> > > > index 0000000..c44eb61
> > > > --- /dev/null
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -0,0 +1,210 @@
> > > > +/*
> > > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > > + *
> > > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > + * See the COPYING file in the top-level directory.
> > > > + */
> > > > +#include "hw/cpu/core.h"
> > > > +#include "hw/ppc/spapr_cpu_core.h"
> > > > +#include "hw/ppc/spapr.h"
> > > > +#include "hw/boards.h"
> > > > +#include "qemu/error-report.h"
> > > > +#include "qapi/visitor.h"
> > > > +#include <sysemu/cpus.h>
> > > > +
> > > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > > +{
> > > > +    Error **errp = opaque;
> > > > +
> > > > +    object_property_set_bool(child, true, "realized", errp);
> > > > +    if (*errp) {
> > > > +        return 1;
> > > > +    }
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > > +    Error *local_err = NULL;
> > > > +
> > > > +    if (!core->nr_threads) {
> > > > +        error_setg(errp, "nr_threads property can't be 0");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (!core->cpu_model) {
> > > > +        error_setg(errp, "cpu_model property isn't set");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    /*
> > > > +     * TODO: If slot isn't specified, plug this core into
> > > > +     * an existing empty slot.
> > > > +     */
> > > > +    if (!core->slot) {
> > > > +        error_setg(errp, "slot property isn't set");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > > > +                             &local_err);
> > > > +    if (local_err) {
> > > > +        error_propagate(errp, local_err);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> > > > +}
> > > > +
> > > > +/*
> > > > + * This creates the CPU threads for a given @core.
> > > > + *
> > > > + * In order to create the threads, we need two inputs - number of
> > > > + * threads and the cpu_model. These are set as core object's properties.
> > > > + * When both of them become available/set, this routine will be called from
> > > > + * either property's set handler to create the threads.
> > > > + *
> > > > + * TODO: Dependence of threads creation on two properties is resulting
> > > > + * in this not-so-clean way of creating threads from either of the
> > > > + * property setters based on the order in which they get set. Check if
> > > > + * this can be handled in a better manner.
> > > > + */
> > > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> > > > +{
> > > > +    int i;
> > > > +
> > > > +    for (i = 0; i < core->nr_threads; i++) {
> > > > +        char id[32];
> > > > +        char type[32];
> > > > +
> > > > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > > > +                 TYPE_POWERPC_CPU);
> > > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> > > > +
> > > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > > +                                  errp);
> > > > +    }
> > > > +}
> > > > +
> > > > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +
> > > > +    return core->slot;
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > > > +                                              Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +
> > > > +    core->slot = g_strdup(val);
> > > > +}
> > > > +
> > > > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +
> > > > +    return core->cpu_model;
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> > > > +                                              Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > > +
> > > > +    /*
> > > > +     * cpu_model can't be different from what is specified with -cpu
> > > > +     */
> > > > +    if (strcmp(val, machine->cpu_model)) {
> > > > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> > > > +       return;
> > > > +    }
> > > > +
> > > > +    core->cpu_model = g_strdup(val);
> > > > +    if (core->nr_threads && core->cpu_model) {
> > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> > > > +                                               const char *name, void *opaque,
> > > > +                                               Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +    int64_t value = core->nr_threads;
> > > > +
> > > > +    visit_type_int(v, name, &value, errp);
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> > > > +                                               const char *name, void *opaque,
> > > > +                                               Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_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;
> > > > +    }
> > > > +
> > > > +    /* Allow only homogeneous configuration */
> > > > +    if (value != smp_threads) {
> > > > +        error_setg(errp, "nr_threads should be %d", smp_threads);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    core->nr_threads = value;
> > > > +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> > > > +
> > > > +    if (core->nr_threads && core->cpu_model) {
> > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_instance_init(Object *obj)
> > > > +{
> > > > +    object_property_add(obj, "nr_threads", "int",
> > > > +                        spapr_cpu_core_prop_get_nr_threads,
> > > > +                        spapr_cpu_core_prop_set_nr_threads,
> > > > +                        NULL, NULL, NULL);
> > > > +    object_property_add_str(obj, "cpu_model",
> > > > +                            spapr_cpu_core_prop_get_cpu_model,
> > > > +                            spapr_cpu_core_prop_set_cpu_model,
> > > > +                            NULL);
> > > > +    object_property_add_str(obj, "slot",
> > > > +                            spapr_cpu_core_prop_get_slot,
> > > > +                            spapr_cpu_core_prop_set_slot,
> > > > +                            NULL);  
> > > 
> > > "slot" seems intended to be a machine-agnostic of mapping device
> > > types discovered from qmp_query_cpu_slots() to an appropriate
> > > "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> > > It seems like maybe TYPE_CPU_CORE is a better place, but then on
> > > x86 I suppose it might be TYPE_CPU_SOCKET or something instead...  
> > 
> > Correct.
> > 
> > > 
> > > It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> > > right approach, but I don't know how we could expose that as
> > > a property. I guess it's somewhat implied that this "interface"
> > > exists if qmp_query_cpu_slots() returns the type, but I wonder
> > > if something a bit more formal should be modeled to make the
> > > implementation requirements a bit clearer.
> > > 
> > > Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> > > class method, expose them via "slot" property, then have the
> > > defaults generate "not implemented" errors?  
> > 
> > Yes makes sense. In fact David has often times said that generic
> > properties/routines should be pushed to base class wherever possible.
> > 
> > I didn't do that in this first iteration to keep the generic changes
> > as minimum as possible, but yes slot should be a property of the
> > base class of core or socket.
> Then what will happen to slot if there isn't any core/socket device
> to query it, i.e. cpu hasn't been plugged in yet?
> To me slot looks like a machine belonged feature.

Yes slot belongs to the machine and it is represented by a link that
is created b/n the machine object and the core object that sits in
the slot.

In the context of this thread, slot is actually the slot name that
identifies the machine slot which the core occupies or will occupy after
hotplug. Thus slot name which is named slot here, it is a property of the
core device.

(qemu) device_add spapr-cpu-core,slot=core[2]
                                 ^
Regards,
Bharata.
> 
> > 
> > Regards,
> > Bharata.
> >

On Mon, 29 Feb 2016 18:25:25 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, Feb 29, 2016 at 11:03:16AM +0100, Igor Mammedov wrote:
> > On Mon, 29 Feb 2016 11:20:19 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > On Fri, Feb 26, 2016 at 12:13:39PM -0600, Michael Roth wrote:  
> > > > Quoting Bharata B Rao (2016-02-25 10:22:38)    
> > > > > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > > > > Creating this core device will result in creation of all the CPU thread
> > > > > devices that are part of this core.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > >  hw/ppc/Makefile.objs            |   1 +
> > > > >  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
> > > > >  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
> > > > >  3 files changed, 243 insertions(+)
> > > > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > > > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > > > 
> > > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > > index c1ffc77..5cc6608 100644
> > > > > --- a/hw/ppc/Makefile.objs
> > > > > +++ b/hw/ppc/Makefile.objs
> > > > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > > > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > > > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > > > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > > > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > > > >  obj-y += spapr_pci_vfio.o
> > > > >  endif
> > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > new file mode 100644
> > > > > index 0000000..c44eb61
> > > > > --- /dev/null
> > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > @@ -0,0 +1,210 @@
> > > > > +/*
> > > > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > > > + *
> > > > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > + *
> > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > + * See the COPYING file in the top-level directory.
> > > > > + */
> > > > > +#include "hw/cpu/core.h"
> > > > > +#include "hw/ppc/spapr_cpu_core.h"
> > > > > +#include "hw/ppc/spapr.h"
> > > > > +#include "hw/boards.h"
> > > > > +#include "qemu/error-report.h"
> > > > > +#include "qapi/visitor.h"
> > > > > +#include <sysemu/cpus.h>
> > > > > +
> > > > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > > > +{
> > > > > +    Error **errp = opaque;
> > > > > +
> > > > > +    object_property_set_bool(child, true, "realized", errp);
> > > > > +    if (*errp) {
> > > > > +        return 1;
> > > > > +    }
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > > +{
> > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > > > +    Error *local_err = NULL;
> > > > > +
> > > > > +    if (!core->nr_threads) {
> > > > > +        error_setg(errp, "nr_threads property can't be 0");
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    if (!core->cpu_model) {
> > > > > +        error_setg(errp, "cpu_model property isn't set");
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    /*
> > > > > +     * TODO: If slot isn't specified, plug this core into
> > > > > +     * an existing empty slot.
> > > > > +     */
> > > > > +    if (!core->slot) {
> > > > > +        error_setg(errp, "slot property isn't set");
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > > > > +                             &local_err);
> > > > > +    if (local_err) {
> > > > > +        error_propagate(errp, local_err);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * This creates the CPU threads for a given @core.
> > > > > + *
> > > > > + * In order to create the threads, we need two inputs - number of
> > > > > + * threads and the cpu_model. These are set as core object's properties.
> > > > > + * When both of them become available/set, this routine will be called from
> > > > > + * either property's set handler to create the threads.
> > > > > + *
> > > > > + * TODO: Dependence of threads creation on two properties is resulting
> > > > > + * in this not-so-clean way of creating threads from either of the
> > > > > + * property setters based on the order in which they get set. Check if
> > > > > + * this can be handled in a better manner.
> > > > > + */
> > > > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> > > > > +{
> > > > > +    int i;
> > > > > +
> > > > > +    for (i = 0; i < core->nr_threads; i++) {
> > > > > +        char id[32];
> > > > > +        char type[32];
> > > > > +
> > > > > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > > > > +                 TYPE_POWERPC_CPU);
> > > > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> > > > > +
> > > > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > > > +                                  errp);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > > > > +{
> > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > +
> > > > > +    return core->slot;
> > > > > +}
> > > > > +
> > > > > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > > > > +                                              Error **errp)
> > > > > +{
> > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > +
> > > > > +    core->slot = g_strdup(val);
> > > > > +}
> > > > > +
> > > > > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> > > > > +{
> > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > +
> > > > > +    return core->cpu_model;
> > > > > +}
> > > > > +
> > > > > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> > > > > +                                              Error **errp)
> > > > > +{
> > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > > > +
> > > > > +    /*
> > > > > +     * cpu_model can't be different from what is specified with -cpu
> > > > > +     */
> > > > > +    if (strcmp(val, machine->cpu_model)) {
> > > > > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> > > > > +       return;
> > > > > +    }
> > > > > +
> > > > > +    core->cpu_model = g_strdup(val);
> > > > > +    if (core->nr_threads && core->cpu_model) {
> > > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> > > > > +                                               const char *name, void *opaque,
> > > > > +                                               Error **errp)
> > > > > +{
> > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > +    int64_t value = core->nr_threads;
> > > > > +
> > > > > +    visit_type_int(v, name, &value, errp);
> > > > > +}
> > > > > +
> > > > > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> > > > > +                                               const char *name, void *opaque,
> > > > > +                                               Error **errp)
> > > > > +{
> > > > > +    sPAPRCPUCore *core = SPAPR_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;
> > > > > +    }
> > > > > +
> > > > > +    /* Allow only homogeneous configuration */
> > > > > +    if (value != smp_threads) {
> > > > > +        error_setg(errp, "nr_threads should be %d", smp_threads);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    core->nr_threads = value;
> > > > > +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> > > > > +
> > > > > +    if (core->nr_threads && core->cpu_model) {
> > > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static void spapr_cpu_core_instance_init(Object *obj)
> > > > > +{
> > > > > +    object_property_add(obj, "nr_threads", "int",
> > > > > +                        spapr_cpu_core_prop_get_nr_threads,
> > > > > +                        spapr_cpu_core_prop_set_nr_threads,
> > > > > +                        NULL, NULL, NULL);
> > > > > +    object_property_add_str(obj, "cpu_model",
> > > > > +                            spapr_cpu_core_prop_get_cpu_model,
> > > > > +                            spapr_cpu_core_prop_set_cpu_model,
> > > > > +                            NULL);
> > > > > +    object_property_add_str(obj, "slot",
> > > > > +                            spapr_cpu_core_prop_get_slot,
> > > > > +                            spapr_cpu_core_prop_set_slot,
> > > > > +                            NULL);    
> > > > 
> > > > "slot" seems intended to be a machine-agnostic of mapping device
> > > > types discovered from qmp_query_cpu_slots() to an appropriate
> > > > "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> > > > It seems like maybe TYPE_CPU_CORE is a better place, but then on
> > > > x86 I suppose it might be TYPE_CPU_SOCKET or something instead...    
> > > 
> > > Correct.
> > >   
> > > > 
> > > > It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> > > > right approach, but I don't know how we could expose that as
> > > > a property. I guess it's somewhat implied that this "interface"
> > > > exists if qmp_query_cpu_slots() returns the type, but I wonder
> > > > if something a bit more formal should be modeled to make the
> > > > implementation requirements a bit clearer.
> > > > 
> > > > Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> > > > class method, expose them via "slot" property, then have the
> > > > defaults generate "not implemented" errors?    
> > > 
> > > Yes makes sense. In fact David has often times said that generic
> > > properties/routines should be pushed to base class wherever possible.
> > > 
> > > I didn't do that in this first iteration to keep the generic changes
> > > as minimum as possible, but yes slot should be a property of the
> > > base class of core or socket.  
> > Then what will happen to slot if there isn't any core/socket device
> > to query it, i.e. cpu hasn't been plugged in yet?
> > To me slot looks like a machine belonged feature.  
> 
> Yes slot belongs to the machine and it is represented by a link that
> is created b/n the machine object and the core object that sits in
> the slot.
> 
> In the context of this thread, slot is actually the slot name that
> identifies the machine slot which the core occupies or will occupy after
> hotplug. Thus slot name which is named slot here, it is a property of the
> core device.
> 
> (qemu) device_add spapr-cpu-core,slot=core[2]
>                                  ^
Is 'slot' a term used by SPAPR on real hardware?
I'd thought that it's 'core', that's why I suggested to use
'core' for POWER as that matched real world concept, see
my other reply in "[RFC PATCH v0 4/6] spapr: CPU hotplug support" thread
of this series.


> Regards,
> Bharata.
> >   
> > > 
> > > Regards,
> > > Bharata.
> > >   
>

On Mon, Feb 29, 2016 at 04:15:25PM +0100, Igor Mammedov wrote:
> On Mon, 29 Feb 2016 18:25:25 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > On Mon, Feb 29, 2016 at 11:03:16AM +0100, Igor Mammedov wrote:
> > > On Mon, 29 Feb 2016 11:20:19 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
[snip]
> > > > > "slot" seems intended to be a machine-agnostic of mapping device
> > > > > types discovered from qmp_query_cpu_slots() to an appropriate
> > > > > "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> > > > > It seems like maybe TYPE_CPU_CORE is a better place, but then on
> > > > > x86 I suppose it might be TYPE_CPU_SOCKET or something instead...    
> > > > 
> > > > Correct.
> > > >   
> > > > > 
> > > > > It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> > > > > right approach, but I don't know how we could expose that as
> > > > > a property. I guess it's somewhat implied that this "interface"
> > > > > exists if qmp_query_cpu_slots() returns the type, but I wonder
> > > > > if something a bit more formal should be modeled to make the
> > > > > implementation requirements a bit clearer.
> > > > > 
> > > > > Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> > > > > class method, expose them via "slot" property, then have the
> > > > > defaults generate "not implemented" errors?    
> > > > 
> > > > Yes makes sense. In fact David has often times said that generic
> > > > properties/routines should be pushed to base class wherever possible.
> > > > 
> > > > I didn't do that in this first iteration to keep the generic changes
> > > > as minimum as possible, but yes slot should be a property of the
> > > > base class of core or socket.  
> > > Then what will happen to slot if there isn't any core/socket device
> > > to query it, i.e. cpu hasn't been plugged in yet?
> > > To me slot looks like a machine belonged feature.  
> > 
> > Yes slot belongs to the machine and it is represented by a link that
> > is created b/n the machine object and the core object that sits in
> > the slot.
> > 
> > In the context of this thread, slot is actually the slot name that
> > identifies the machine slot which the core occupies or will occupy after
> > hotplug. Thus slot name which is named slot here, it is a property of the
> > core device.
> > 
> > (qemu) device_add spapr-cpu-core,slot=core[2]
> >                                  ^
> Is 'slot' a term used by SPAPR on real hardware?

So.. PAPR is a para-virtualized interface, so it never appears on real
hardware.

But, no, "slot" is not a term used by PAPR.

> I'd thought that it's 'core', that's why I suggested to use
> 'core' for POWER as that matched real world concept, see
> my other reply in "[RFC PATCH v0 4/6] spapr: CPU hotplug support" thread
> of this series.

I don't think it uses "core" either, I believe it uses just "cpu" but
meaning a multi-thread core, rather than a single logical cpu thread.

On Mon, Feb 29, 2016 at 04:15:25PM +0100, Igor Mammedov wrote:
> On Mon, 29 Feb 2016 18:25:25 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Mon, Feb 29, 2016 at 11:03:16AM +0100, Igor Mammedov wrote:
> > > On Mon, 29 Feb 2016 11:20:19 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >   
> > > > On Fri, Feb 26, 2016 at 12:13:39PM -0600, Michael Roth wrote:  
> > > > > Quoting Bharata B Rao (2016-02-25 10:22:38)    
> > > > > > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > > > > > Creating this core device will result in creation of all the CPU thread
> > > > > > devices that are part of this core.
> > > > > > 
> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > ---
> > > > > >  hw/ppc/Makefile.objs            |   1 +
> > > > > >  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
> > > > > >  3 files changed, 243 insertions(+)
> > > > > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > > > > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > > > > 
> > > > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > > > index c1ffc77..5cc6608 100644
> > > > > > --- a/hw/ppc/Makefile.objs
> > > > > > +++ b/hw/ppc/Makefile.objs
> > > > > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > > > > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > > > > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > > > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > > > > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > > > > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > > > > >  obj-y += spapr_pci_vfio.o
> > > > > >  endif
> > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > new file mode 100644
> > > > > > index 0000000..c44eb61
> > > > > > --- /dev/null
> > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > @@ -0,0 +1,210 @@
> > > > > > +/*
> > > > > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > > > > + *
> > > > > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > + *
> > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > + * See the COPYING file in the top-level directory.
> > > > > > + */
> > > > > > +#include "hw/cpu/core.h"
> > > > > > +#include "hw/ppc/spapr_cpu_core.h"
> > > > > > +#include "hw/ppc/spapr.h"
> > > > > > +#include "hw/boards.h"
> > > > > > +#include "qemu/error-report.h"
> > > > > > +#include "qapi/visitor.h"
> > > > > > +#include <sysemu/cpus.h>
> > > > > > +
> > > > > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > > > > +{
> > > > > > +    Error **errp = opaque;
> > > > > > +
> > > > > > +    object_property_set_bool(child, true, "realized", errp);
> > > > > > +    if (*errp) {
> > > > > > +        return 1;
> > > > > > +    }
> > > > > > +    return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > > > +{
> > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > > > > +    Error *local_err = NULL;
> > > > > > +
> > > > > > +    if (!core->nr_threads) {
> > > > > > +        error_setg(errp, "nr_threads property can't be 0");
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (!core->cpu_model) {
> > > > > > +        error_setg(errp, "cpu_model property isn't set");
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    /*
> > > > > > +     * TODO: If slot isn't specified, plug this core into
> > > > > > +     * an existing empty slot.
> > > > > > +     */
> > > > > > +    if (!core->slot) {
> > > > > > +        error_setg(errp, "slot property isn't set");
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > > > > > +                             &local_err);
> > > > > > +    if (local_err) {
> > > > > > +        error_propagate(errp, local_err);
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * This creates the CPU threads for a given @core.
> > > > > > + *
> > > > > > + * In order to create the threads, we need two inputs - number of
> > > > > > + * threads and the cpu_model. These are set as core object's properties.
> > > > > > + * When both of them become available/set, this routine will be called from
> > > > > > + * either property's set handler to create the threads.
> > > > > > + *
> > > > > > + * TODO: Dependence of threads creation on two properties is resulting
> > > > > > + * in this not-so-clean way of creating threads from either of the
> > > > > > + * property setters based on the order in which they get set. Check if
> > > > > > + * this can be handled in a better manner.
> > > > > > + */
> > > > > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> > > > > > +{
> > > > > > +    int i;
> > > > > > +
> > > > > > +    for (i = 0; i < core->nr_threads; i++) {
> > > > > > +        char id[32];
> > > > > > +        char type[32];
> > > > > > +
> > > > > > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > > > > > +                 TYPE_POWERPC_CPU);
> > > > > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> > > > > > +
> > > > > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > > > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > > > > +                                  errp);
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > > > > > +{
> > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > +
> > > > > > +    return core->slot;
> > > > > > +}
> > > > > > +
> > > > > > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > > > > > +                                              Error **errp)
> > > > > > +{
> > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > +
> > > > > > +    core->slot = g_strdup(val);
> > > > > > +}
> > > > > > +
> > > > > > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> > > > > > +{
> > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > +
> > > > > > +    return core->cpu_model;
> > > > > > +}
> > > > > > +
> > > > > > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> > > > > > +                                              Error **errp)
> > > > > > +{
> > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > > > > +
> > > > > > +    /*
> > > > > > +     * cpu_model can't be different from what is specified with -cpu
> > > > > > +     */
> > > > > > +    if (strcmp(val, machine->cpu_model)) {
> > > > > > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> > > > > > +       return;
> > > > > > +    }
> > > > > > +
> > > > > > +    core->cpu_model = g_strdup(val);
> > > > > > +    if (core->nr_threads && core->cpu_model) {
> > > > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> > > > > > +                                               const char *name, void *opaque,
> > > > > > +                                               Error **errp)
> > > > > > +{
> > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > +    int64_t value = core->nr_threads;
> > > > > > +
> > > > > > +    visit_type_int(v, name, &value, errp);
> > > > > > +}
> > > > > > +
> > > > > > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> > > > > > +                                               const char *name, void *opaque,
> > > > > > +                                               Error **errp)
> > > > > > +{
> > > > > > +    sPAPRCPUCore *core = SPAPR_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;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* Allow only homogeneous configuration */
> > > > > > +    if (value != smp_threads) {
> > > > > > +        error_setg(errp, "nr_threads should be %d", smp_threads);
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    core->nr_threads = value;
> > > > > > +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> > > > > > +
> > > > > > +    if (core->nr_threads && core->cpu_model) {
> > > > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > > +static void spapr_cpu_core_instance_init(Object *obj)
> > > > > > +{
> > > > > > +    object_property_add(obj, "nr_threads", "int",
> > > > > > +                        spapr_cpu_core_prop_get_nr_threads,
> > > > > > +                        spapr_cpu_core_prop_set_nr_threads,
> > > > > > +                        NULL, NULL, NULL);
> > > > > > +    object_property_add_str(obj, "cpu_model",
> > > > > > +                            spapr_cpu_core_prop_get_cpu_model,
> > > > > > +                            spapr_cpu_core_prop_set_cpu_model,
> > > > > > +                            NULL);
> > > > > > +    object_property_add_str(obj, "slot",
> > > > > > +                            spapr_cpu_core_prop_get_slot,
> > > > > > +                            spapr_cpu_core_prop_set_slot,
> > > > > > +                            NULL);    
> > > > > 
> > > > > "slot" seems intended to be a machine-agnostic of mapping device
> > > > > types discovered from qmp_query_cpu_slots() to an appropriate
> > > > > "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> > > > > It seems like maybe TYPE_CPU_CORE is a better place, but then on
> > > > > x86 I suppose it might be TYPE_CPU_SOCKET or something instead...    
> > > > 
> > > > Correct.
> > > >   
> > > > > 
> > > > > It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> > > > > right approach, but I don't know how we could expose that as
> > > > > a property. I guess it's somewhat implied that this "interface"
> > > > > exists if qmp_query_cpu_slots() returns the type, but I wonder
> > > > > if something a bit more formal should be modeled to make the
> > > > > implementation requirements a bit clearer.
> > > > > 
> > > > > Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> > > > > class method, expose them via "slot" property, then have the
> > > > > defaults generate "not implemented" errors?    
> > > > 
> > > > Yes makes sense. In fact David has often times said that generic
> > > > properties/routines should be pushed to base class wherever possible.
> > > > 
> > > > I didn't do that in this first iteration to keep the generic changes
> > > > as minimum as possible, but yes slot should be a property of the
> > > > base class of core or socket.  
> > > Then what will happen to slot if there isn't any core/socket device
> > > to query it, i.e. cpu hasn't been plugged in yet?
> > > To me slot looks like a machine belonged feature.  
> > 
> > Yes slot belongs to the machine and it is represented by a link that
> > is created b/n the machine object and the core object that sits in
> > the slot.
> > 
> > In the context of this thread, slot is actually the slot name that
> > identifies the machine slot which the core occupies or will occupy after
> > hotplug. Thus slot name which is named slot here, it is a property of the
> > core device.
> > 
> > (qemu) device_add spapr-cpu-core,slot=core[2]
> >                                  ^
> Is 'slot' a term used by SPAPR on real hardware?
> I'd thought that it's 'core', that's why I suggested to use
> 'core' for POWER as that matched real world concept, see
> my other reply in "[RFC PATCH v0 4/6] spapr: CPU hotplug support" thread
> of this series.

I don't see any reply from you to that thread. I guess you mean
"[RFC PATCH v0 3/6] spapr: Represent boot CPUs as spapr-cpu-core devices"
thread where you suggest to use CORE type rather than TYPE_CPU and
enumerate threads under CORE type.

In case, it didn't come out clearly, I am indeed using core object and
hotplugging at core granularity for sPAPR. The core sits in the machine slot.

Regards,
Bharata.

On Tue, 1 Mar 2016 13:47:27 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, Feb 29, 2016 at 04:15:25PM +0100, Igor Mammedov wrote:
> > On Mon, 29 Feb 2016 18:25:25 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > On Mon, Feb 29, 2016 at 11:03:16AM +0100, Igor Mammedov wrote:  
> > > > On Mon, 29 Feb 2016 11:20:19 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > >     
> > > > > On Fri, Feb 26, 2016 at 12:13:39PM -0600, Michael Roth wrote:    
> > > > > > Quoting Bharata B Rao (2016-02-25 10:22:38)      
> > > > > > > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > > > > > > Creating this core device will result in creation of all the CPU thread
> > > > > > > devices that are part of this core.
> > > > > > > 
> > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > > ---
> > > > > > >  hw/ppc/Makefile.objs            |   1 +
> > > > > > >  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
> > > > > > >  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
> > > > > > >  3 files changed, 243 insertions(+)
> > > > > > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > > > > > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > > > > > 
> > > > > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > > > > index c1ffc77..5cc6608 100644
> > > > > > > --- a/hw/ppc/Makefile.objs
> > > > > > > +++ b/hw/ppc/Makefile.objs
> > > > > > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > > > > > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > > > > > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > > > > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > > > > > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > > > > > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > > > > > >  obj-y += spapr_pci_vfio.o
> > > > > > >  endif
> > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > > new file mode 100644
> > > > > > > index 0000000..c44eb61
> > > > > > > --- /dev/null
> > > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > > @@ -0,0 +1,210 @@
> > > > > > > +/*
> > > > > > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > > > > > + *
> > > > > > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > > + *
> > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > + */
> > > > > > > +#include "hw/cpu/core.h"
> > > > > > > +#include "hw/ppc/spapr_cpu_core.h"
> > > > > > > +#include "hw/ppc/spapr.h"
> > > > > > > +#include "hw/boards.h"
> > > > > > > +#include "qemu/error-report.h"
> > > > > > > +#include "qapi/visitor.h"
> > > > > > > +#include <sysemu/cpus.h>
> > > > > > > +
> > > > > > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > > > > > +{
> > > > > > > +    Error **errp = opaque;
> > > > > > > +
> > > > > > > +    object_property_set_bool(child, true, "realized", errp);
> > > > > > > +    if (*errp) {
> > > > > > > +        return 1;
> > > > > > > +    }
> > > > > > > +    return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > > > > +{
> > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > > > > > +    Error *local_err = NULL;
> > > > > > > +
> > > > > > > +    if (!core->nr_threads) {
> > > > > > > +        error_setg(errp, "nr_threads property can't be 0");
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    if (!core->cpu_model) {
> > > > > > > +        error_setg(errp, "cpu_model property isn't set");
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    /*
> > > > > > > +     * TODO: If slot isn't specified, plug this core into
> > > > > > > +     * an existing empty slot.
> > > > > > > +     */
> > > > > > > +    if (!core->slot) {
> > > > > > > +        error_setg(errp, "slot property isn't set");
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > > > > > > +                             &local_err);
> > > > > > > +    if (local_err) {
> > > > > > > +        error_propagate(errp, local_err);
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * This creates the CPU threads for a given @core.
> > > > > > > + *
> > > > > > > + * In order to create the threads, we need two inputs - number of
> > > > > > > + * threads and the cpu_model. These are set as core object's properties.
> > > > > > > + * When both of them become available/set, this routine will be called from
> > > > > > > + * either property's set handler to create the threads.
> > > > > > > + *
> > > > > > > + * TODO: Dependence of threads creation on two properties is resulting
> > > > > > > + * in this not-so-clean way of creating threads from either of the
> > > > > > > + * property setters based on the order in which they get set. Check if
> > > > > > > + * this can be handled in a better manner.
> > > > > > > + */
> > > > > > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> > > > > > > +{
> > > > > > > +    int i;
> > > > > > > +
> > > > > > > +    for (i = 0; i < core->nr_threads; i++) {
> > > > > > > +        char id[32];
> > > > > > > +        char type[32];
> > > > > > > +
> > > > > > > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > > > > > > +                 TYPE_POWERPC_CPU);
> > > > > > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> > > > > > > +
> > > > > > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > > > > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > > > > > +                                  errp);
> > > > > > > +    }
> > > > > > > +}
> > > > > > > +
> > > > > > > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > > > > > > +{
> > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > +
> > > > > > > +    return core->slot;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > > > > > > +                                              Error **errp)
> > > > > > > +{
> > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > +
> > > > > > > +    core->slot = g_strdup(val);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> > > > > > > +{
> > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > +
> > > > > > > +    return core->cpu_model;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> > > > > > > +                                              Error **errp)
> > > > > > > +{
> > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > > > > > +
> > > > > > > +    /*
> > > > > > > +     * cpu_model can't be different from what is specified with -cpu
> > > > > > > +     */
> > > > > > > +    if (strcmp(val, machine->cpu_model)) {
> > > > > > > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> > > > > > > +       return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    core->cpu_model = g_strdup(val);
> > > > > > > +    if (core->nr_threads && core->cpu_model) {
> > > > > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > > > > +    }
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> > > > > > > +                                               const char *name, void *opaque,
> > > > > > > +                                               Error **errp)
> > > > > > > +{
> > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > +    int64_t value = core->nr_threads;
> > > > > > > +
> > > > > > > +    visit_type_int(v, name, &value, errp);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> > > > > > > +                                               const char *name, void *opaque,
> > > > > > > +                                               Error **errp)
> > > > > > > +{
> > > > > > > +    sPAPRCPUCore *core = SPAPR_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;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    /* Allow only homogeneous configuration */
> > > > > > > +    if (value != smp_threads) {
> > > > > > > +        error_setg(errp, "nr_threads should be %d", smp_threads);
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    core->nr_threads = value;
> > > > > > > +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> > > > > > > +
> > > > > > > +    if (core->nr_threads && core->cpu_model) {
> > > > > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > > > > +    }
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void spapr_cpu_core_instance_init(Object *obj)
> > > > > > > +{
> > > > > > > +    object_property_add(obj, "nr_threads", "int",
> > > > > > > +                        spapr_cpu_core_prop_get_nr_threads,
> > > > > > > +                        spapr_cpu_core_prop_set_nr_threads,
> > > > > > > +                        NULL, NULL, NULL);
> > > > > > > +    object_property_add_str(obj, "cpu_model",
> > > > > > > +                            spapr_cpu_core_prop_get_cpu_model,
> > > > > > > +                            spapr_cpu_core_prop_set_cpu_model,
> > > > > > > +                            NULL);
> > > > > > > +    object_property_add_str(obj, "slot",
> > > > > > > +                            spapr_cpu_core_prop_get_slot,
> > > > > > > +                            spapr_cpu_core_prop_set_slot,
> > > > > > > +                            NULL);      
> > > > > > 
> > > > > > "slot" seems intended to be a machine-agnostic of mapping device
> > > > > > types discovered from qmp_query_cpu_slots() to an appropriate
> > > > > > "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> > > > > > It seems like maybe TYPE_CPU_CORE is a better place, but then on
> > > > > > x86 I suppose it might be TYPE_CPU_SOCKET or something instead...      
> > > > > 
> > > > > Correct.
> > > > >     
> > > > > > 
> > > > > > It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> > > > > > right approach, but I don't know how we could expose that as
> > > > > > a property. I guess it's somewhat implied that this "interface"
> > > > > > exists if qmp_query_cpu_slots() returns the type, but I wonder
> > > > > > if something a bit more formal should be modeled to make the
> > > > > > implementation requirements a bit clearer.
> > > > > > 
> > > > > > Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> > > > > > class method, expose them via "slot" property, then have the
> > > > > > defaults generate "not implemented" errors?      
> > > > > 
> > > > > Yes makes sense. In fact David has often times said that generic
> > > > > properties/routines should be pushed to base class wherever possible.
> > > > > 
> > > > > I didn't do that in this first iteration to keep the generic changes
> > > > > as minimum as possible, but yes slot should be a property of the
> > > > > base class of core or socket.    
> > > > Then what will happen to slot if there isn't any core/socket device
> > > > to query it, i.e. cpu hasn't been plugged in yet?
> > > > To me slot looks like a machine belonged feature.    
> > > 
> > > Yes slot belongs to the machine and it is represented by a link that
> > > is created b/n the machine object and the core object that sits in
> > > the slot.
> > > 
> > > In the context of this thread, slot is actually the slot name that
> > > identifies the machine slot which the core occupies or will occupy after
> > > hotplug. Thus slot name which is named slot here, it is a property of the
> > > core device.
> > > 
> > > (qemu) device_add spapr-cpu-core,slot=core[2]
> > >                                  ^  
> > Is 'slot' a term used by SPAPR on real hardware?
> > I'd thought that it's 'core', that's why I suggested to use
> > 'core' for POWER as that matched real world concept, see
> > my other reply in "[RFC PATCH v0 4/6] spapr: CPU hotplug support" thread
> > of this series.  
> 
> I don't see any reply from you to that thread. I guess you mean
> "[RFC PATCH v0 3/6] spapr: Represent boot CPUs as spapr-cpu-core devices"
no it's another thread, sorry for mixing things up.
it was thread about QMP interface:
http://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06540.html

> thread where you suggest to use CORE type rather than TYPE_CPU and
> enumerate threads under CORE type.
> 
> In case, it didn't come out clearly, I am indeed using core object and
> hotplugging at core granularity for sPAPR. The core sits in the machine slot.
There I was suggesting to handle only CORE at spapr_machine_device_plug,
making core.realize() to complete underlying threads initialization if possible,
and if it you have to do some wiring at machine level for threads then
do it while handling CORE branch.

That way hotplug call-chain would look like:
 device_add(core)
    -> device_realize(core)
        -> core_realize()-> {realize threads}
        -> spapr_machine_device_plug(core)
              connect to board internals for core (maybe threads) 

vs current way:
  device_add(core)
    -> device_realize(core)
        -> core_realize()-> {realize threads}
                             -> device_realize(thread)
                                   -> spapr_machine_device_plug(thread)
                                         some board specific wiring
                             ...
        -> spapr_machine_device_plug(core)
              connect to board internals 

the former is easier to read/follow
> 
> Regards,
> Bharata.
>

On Tue, 1 Mar 2016 12:21:27 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Feb 29, 2016 at 04:15:25PM +0100, Igor Mammedov wrote:
> > On Mon, 29 Feb 2016 18:25:25 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:  
> > > On Mon, Feb 29, 2016 at 11:03:16AM +0100, Igor Mammedov wrote:  
> > > > On Mon, 29 Feb 2016 11:20:19 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:  
> [snip]
> > > > > > "slot" seems intended to be a machine-agnostic of mapping device
> > > > > > types discovered from qmp_query_cpu_slots() to an appropriate
> > > > > > "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> > > > > > It seems like maybe TYPE_CPU_CORE is a better place, but then on
> > > > > > x86 I suppose it might be TYPE_CPU_SOCKET or something instead...      
> > > > > 
> > > > > Correct.
> > > > >     
> > > > > > 
> > > > > > It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> > > > > > right approach, but I don't know how we could expose that as
> > > > > > a property. I guess it's somewhat implied that this "interface"
> > > > > > exists if qmp_query_cpu_slots() returns the type, but I wonder
> > > > > > if something a bit more formal should be modeled to make the
> > > > > > implementation requirements a bit clearer.
> > > > > > 
> > > > > > Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> > > > > > class method, expose them via "slot" property, then have the
> > > > > > defaults generate "not implemented" errors?      
> > > > > 
> > > > > Yes makes sense. In fact David has often times said that generic
> > > > > properties/routines should be pushed to base class wherever possible.
> > > > > 
> > > > > I didn't do that in this first iteration to keep the generic changes
> > > > > as minimum as possible, but yes slot should be a property of the
> > > > > base class of core or socket.    
> > > > Then what will happen to slot if there isn't any core/socket device
> > > > to query it, i.e. cpu hasn't been plugged in yet?
> > > > To me slot looks like a machine belonged feature.    
> > > 
> > > Yes slot belongs to the machine and it is represented by a link that
> > > is created b/n the machine object and the core object that sits in
> > > the slot.
> > > 
> > > In the context of this thread, slot is actually the slot name that
> > > identifies the machine slot which the core occupies or will occupy after
> > > hotplug. Thus slot name which is named slot here, it is a property of the
> > > core device.
> > > 
> > > (qemu) device_add spapr-cpu-core,slot=core[2]
> > >                                  ^  
> > Is 'slot' a term used by SPAPR on real hardware?  
> 
> So.. PAPR is a para-virtualized interface, so it never appears on real
> hardware.
> 
> But, no, "slot" is not a term used by PAPR.
> 
> > I'd thought that it's 'core', that's why I suggested to use
> > 'core' for POWER as that matched real world concept, see
> > my other reply in "[RFC PATCH v0 4/6] spapr: CPU hotplug support" thread
> > of this series.  
> 
> I don't think it uses "core" either, I believe it uses just "cpu" but
> meaning a multi-thread core, rather than a single logical cpu thread.
then calling property 'cpu' is fine or one could go by meaning and
use 'core' property (reusing 'core' property)

On Tue, Mar 01, 2016 at 10:16:18AM +0100, Igor Mammedov wrote:
> On Tue, 1 Mar 2016 13:47:27 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Mon, Feb 29, 2016 at 04:15:25PM +0100, Igor Mammedov wrote:
> > > On Mon, 29 Feb 2016 18:25:25 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >   
> > > > On Mon, Feb 29, 2016 at 11:03:16AM +0100, Igor Mammedov wrote:  
> > > > > On Mon, 29 Feb 2016 11:20:19 +0530
> > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > >     
> > > > > > On Fri, Feb 26, 2016 at 12:13:39PM -0600, Michael Roth wrote:    
> > > > > > > Quoting Bharata B Rao (2016-02-25 10:22:38)      
> > > > > > > > Add sPAPR specific CPU core device that is based on generic CPU core device.
> > > > > > > > Creating this core device will result in creation of all the CPU thread
> > > > > > > > devices that are part of this core.
> > > > > > > > 
> > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > > > ---
> > > > > > > >  hw/ppc/Makefile.objs            |   1 +
> > > > > > > >  hw/ppc/spapr_cpu_core.c         | 210 ++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
> > > > > > > >  3 files changed, 243 insertions(+)
> > > > > > > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > > > > > > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > > > > > > 
> > > > > > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > > > > > index c1ffc77..5cc6608 100644
> > > > > > > > --- a/hw/ppc/Makefile.objs
> > > > > > > > +++ b/hw/ppc/Makefile.objs
> > > > > > > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > > > > > > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > > > > > > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > > > > > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > > > > > > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > > > > > > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > > > > > > >  obj-y += spapr_pci_vfio.o
> > > > > > > >  endif
> > > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > > > new file mode 100644
> > > > > > > > index 0000000..c44eb61
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > > > @@ -0,0 +1,210 @@
> > > > > > > > +/*
> > > > > > > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > > > > > > + *
> > > > > > > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > > > + *
> > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > + */
> > > > > > > > +#include "hw/cpu/core.h"
> > > > > > > > +#include "hw/ppc/spapr_cpu_core.h"
> > > > > > > > +#include "hw/ppc/spapr.h"
> > > > > > > > +#include "hw/boards.h"
> > > > > > > > +#include "qemu/error-report.h"
> > > > > > > > +#include "qapi/visitor.h"
> > > > > > > > +#include <sysemu/cpus.h>
> > > > > > > > +
> > > > > > > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > > > > > > +{
> > > > > > > > +    Error **errp = opaque;
> > > > > > > > +
> > > > > > > > +    object_property_set_bool(child, true, "realized", errp);
> > > > > > > > +    if (*errp) {
> > > > > > > > +        return 1;
> > > > > > > > +    }
> > > > > > > > +    return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > > > > > +{
> > > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > > > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > > > > > > +    Error *local_err = NULL;
> > > > > > > > +
> > > > > > > > +    if (!core->nr_threads) {
> > > > > > > > +        error_setg(errp, "nr_threads property can't be 0");
> > > > > > > > +        return;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    if (!core->cpu_model) {
> > > > > > > > +        error_setg(errp, "cpu_model property isn't set");
> > > > > > > > +        return;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    /*
> > > > > > > > +     * TODO: If slot isn't specified, plug this core into
> > > > > > > > +     * an existing empty slot.
> > > > > > > > +     */
> > > > > > > > +    if (!core->slot) {
> > > > > > > > +        error_setg(errp, "slot property isn't set");
> > > > > > > > +        return;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > > > > > > > +                             &local_err);
> > > > > > > > +    if (local_err) {
> > > > > > > > +        error_propagate(errp, local_err);
> > > > > > > > +        return;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * This creates the CPU threads for a given @core.
> > > > > > > > + *
> > > > > > > > + * In order to create the threads, we need two inputs - number of
> > > > > > > > + * threads and the cpu_model. These are set as core object's properties.
> > > > > > > > + * When both of them become available/set, this routine will be called from
> > > > > > > > + * either property's set handler to create the threads.
> > > > > > > > + *
> > > > > > > > + * TODO: Dependence of threads creation on two properties is resulting
> > > > > > > > + * in this not-so-clean way of creating threads from either of the
> > > > > > > > + * property setters based on the order in which they get set. Check if
> > > > > > > > + * this can be handled in a better manner.
> > > > > > > > + */
> > > > > > > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp)
> > > > > > > > +{
> > > > > > > > +    int i;
> > > > > > > > +
> > > > > > > > +    for (i = 0; i < core->nr_threads; i++) {
> > > > > > > > +        char id[32];
> > > > > > > > +        char type[32];
> > > > > > > > +
> > > > > > > > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > > > > > > > +                 TYPE_POWERPC_CPU);
> > > > > > > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), type);
> > > > > > > > +
> > > > > > > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > > > > > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > > > > > > +                                  errp);
> > > > > > > > +    }
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > > > > > > > +{
> > > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > > +
> > > > > > > > +    return core->slot;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > > > > > > > +                                              Error **errp)
> > > > > > > > +{
> > > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > > +
> > > > > > > > +    core->slot = g_strdup(val);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> > > > > > > > +{
> > > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > > +
> > > > > > > > +    return core->cpu_model;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val,
> > > > > > > > +                                              Error **errp)
> > > > > > > > +{
> > > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > > > > > > +
> > > > > > > > +    /*
> > > > > > > > +     * cpu_model can't be different from what is specified with -cpu
> > > > > > > > +     */
> > > > > > > > +    if (strcmp(val, machine->cpu_model)) {
> > > > > > > > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> > > > > > > > +       return;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    core->cpu_model = g_strdup(val);
> > > > > > > > +    if (core->nr_threads && core->cpu_model) {
> > > > > > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > > > > > +    }
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> > > > > > > > +                                               const char *name, void *opaque,
> > > > > > > > +                                               Error **errp)
> > > > > > > > +{
> > > > > > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > > +    int64_t value = core->nr_threads;
> > > > > > > > +
> > > > > > > > +    visit_type_int(v, name, &value, errp);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> > > > > > > > +                                               const char *name, void *opaque,
> > > > > > > > +                                               Error **errp)
> > > > > > > > +{
> > > > > > > > +    sPAPRCPUCore *core = SPAPR_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;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    /* Allow only homogeneous configuration */
> > > > > > > > +    if (value != smp_threads) {
> > > > > > > > +        error_setg(errp, "nr_threads should be %d", smp_threads);
> > > > > > > > +        return;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    core->nr_threads = value;
> > > > > > > > +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> > > > > > > > +
> > > > > > > > +    if (core->nr_threads && core->cpu_model) {
> > > > > > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > > > > > +    }
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void spapr_cpu_core_instance_init(Object *obj)
> > > > > > > > +{
> > > > > > > > +    object_property_add(obj, "nr_threads", "int",
> > > > > > > > +                        spapr_cpu_core_prop_get_nr_threads,
> > > > > > > > +                        spapr_cpu_core_prop_set_nr_threads,
> > > > > > > > +                        NULL, NULL, NULL);
> > > > > > > > +    object_property_add_str(obj, "cpu_model",
> > > > > > > > +                            spapr_cpu_core_prop_get_cpu_model,
> > > > > > > > +                            spapr_cpu_core_prop_set_cpu_model,
> > > > > > > > +                            NULL);
> > > > > > > > +    object_property_add_str(obj, "slot",
> > > > > > > > +                            spapr_cpu_core_prop_get_slot,
> > > > > > > > +                            spapr_cpu_core_prop_set_slot,
> > > > > > > > +                            NULL);      
> > > > > > > 
> > > > > > > "slot" seems intended to be a machine-agnostic of mapping device
> > > > > > > types discovered from qmp_query_cpu_slots() to an appropriate
> > > > > > > "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> > > > > > > It seems like maybe TYPE_CPU_CORE is a better place, but then on
> > > > > > > x86 I suppose it might be TYPE_CPU_SOCKET or something instead...      
> > > > > > 
> > > > > > Correct.
> > > > > >     
> > > > > > > 
> > > > > > > It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> > > > > > > right approach, but I don't know how we could expose that as
> > > > > > > a property. I guess it's somewhat implied that this "interface"
> > > > > > > exists if qmp_query_cpu_slots() returns the type, but I wonder
> > > > > > > if something a bit more formal should be modeled to make the
> > > > > > > implementation requirements a bit clearer.
> > > > > > > 
> > > > > > > Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> > > > > > > class method, expose them via "slot" property, then have the
> > > > > > > defaults generate "not implemented" errors?      
> > > > > > 
> > > > > > Yes makes sense. In fact David has often times said that generic
> > > > > > properties/routines should be pushed to base class wherever possible.
> > > > > > 
> > > > > > I didn't do that in this first iteration to keep the generic changes
> > > > > > as minimum as possible, but yes slot should be a property of the
> > > > > > base class of core or socket.    
> > > > > Then what will happen to slot if there isn't any core/socket device
> > > > > to query it, i.e. cpu hasn't been plugged in yet?
> > > > > To me slot looks like a machine belonged feature.    
> > > > 
> > > > Yes slot belongs to the machine and it is represented by a link that
> > > > is created b/n the machine object and the core object that sits in
> > > > the slot.
> > > > 
> > > > In the context of this thread, slot is actually the slot name that
> > > > identifies the machine slot which the core occupies or will occupy after
> > > > hotplug. Thus slot name which is named slot here, it is a property of the
> > > > core device.
> > > > 
> > > > (qemu) device_add spapr-cpu-core,slot=core[2]
> > > >                                  ^  
> > > Is 'slot' a term used by SPAPR on real hardware?
> > > I'd thought that it's 'core', that's why I suggested to use
> > > 'core' for POWER as that matched real world concept, see
> > > my other reply in "[RFC PATCH v0 4/6] spapr: CPU hotplug support" thread
> > > of this series.  
> > 
> > I don't see any reply from you to that thread. I guess you mean
> > "[RFC PATCH v0 3/6] spapr: Represent boot CPUs as spapr-cpu-core devices"
> no it's another thread, sorry for mixing things up.
> it was thread about QMP interface:
> http://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06540.html
> 
> > thread where you suggest to use CORE type rather than TYPE_CPU and
> > enumerate threads under CORE type.
> > 
> > In case, it didn't come out clearly, I am indeed using core object and
> > hotplugging at core granularity for sPAPR. The core sits in the machine slot.
> There I was suggesting to handle only CORE at spapr_machine_device_plug,
> making core.realize() to complete underlying threads initialization if possible,
> and if it you have to do some wiring at machine level for threads then
> do it while handling CORE branch.
> 
> That way hotplug call-chain would look like:
>  device_add(core)
>     -> device_realize(core)
>         -> core_realize()-> {realize threads}
>         -> spapr_machine_device_plug(core)
>               connect to board internals for core (maybe threads) 
> 
> vs current way:
>   device_add(core)
>     -> device_realize(core)
>         -> core_realize()-> {realize threads}
>                              -> device_realize(thread)
>                                    -> spapr_machine_device_plug(thread)
>                                          some board specific wiring
>                              ...
>         -> spapr_machine_device_plug(core)
>               connect to board internals 

Sure, I am changing this in next version.

Regards,
Bharata.