[RFC,v2,6/9] 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.

Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
topologies that result in partially filled cores. Both of these are done
only if CPU core hotplug is supported.

Note: An unrelated change in the call to xics_system_init() is done
in this patch as it makes sense to use the local variable smt introduced
in this patch instead of kvmppc_smt_threads() call here.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/Makefile.objs            |   1 +
 hw/ppc/spapr.c                  |  68 +++++++++++---
 hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   4 +
 include/hw/ppc/spapr_cpu_core.h |  28 ++++++
 5 files changed, 287 insertions(+), 13 deletions(-)
 create mode 100644 hw/ppc/spapr_cpu_core.c
 create mode 100644 include/hw/ppc/spapr_cpu_core.h

On Fri, 11 Mar 2016 10:24:35 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> 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.
> 
> Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> topologies that result in partially filled cores. Both of these are done
> only if CPU core hotplug is supported.
> 
> Note: An unrelated change in the call to xics_system_init() is done
> in this patch as it makes sense to use the local variable smt introduced
> in this patch instead of kvmppc_smt_threads() call here.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/Makefile.objs            |   1 +
>  hw/ppc/spapr.c                  |  68 +++++++++++---
>  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |   4 +
>  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
>  5 files changed, 287 insertions(+), 13 deletions(-)
>  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.c b/hw/ppc/spapr.c
> index 64c4acc..cffe8c8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -64,6 +64,7 @@
>  
>  #include "hw/compat.h"
>  #include "qemu-common.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  
>  #include <libfdt.h>
>  
> @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
>  
>  }
>  
> -static void spapr_cpu_reset(void *opaque)
> +void spapr_cpu_reset(void *opaque)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      PowerPCCPU *cpu = opaque;
> @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>      machine->boot_order = g_strdup(boot_device);
>  }
>  
> -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> -                           Error **errp)
> +/*
> + * TODO: Check if some of these can be moved to rtas_start_cpu() where
> + * a few other things required for hotplugged CPUs are being done.
> + */
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
>  
> @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
>      const char *kernel_filename = machine->kernel_filename;
>      const char *kernel_cmdline = machine->kernel_cmdline;
>      const char *initrd_filename = machine->initrd_filename;
> -    PowerPCCPU *cpu;
>      PCIHostState *phb;
>      int i;
>      MemoryRegion *sysmem = get_system_memory();
> @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
>      long load_limit, fw_size;
>      bool kernel_le = false;
>      char *filename;
> +    int smt = kvmppc_smt_threads();
> +    int spapr_cores = smp_cpus / smp_threads;
> +    int spapr_max_cores = max_cpus / smp_threads;
> +
> +    if (smc->dr_cpu_enabled) {
> +        if (smp_cpus % smp_threads) {
> +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> +                         smp_cpus, smp_threads);
> +            exit(1);
> +        }
> +        if (max_cpus % smp_threads) {
> +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> +                         max_cpus, smp_threads);
> +            exit(1);
> +        }
> +    }
>  
>      msi_supported = true;
>  
> @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
>  
>      /* Set up Interrupt Controller before we create the VCPUs */
>      spapr->icp = xics_system_init(machine,
> -                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> -                                               smp_threads),
> +                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),
is smt == smp_threads, if not then what's a difference?

>                                    XICS_IRQS, &error_fatal);
>  
>      if (smc->dr_lmb_enabled) {
> @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
>      if (machine->cpu_model == NULL) {
>          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
>      }
> -    for (i = 0; i < smp_cpus; i++) {
> -        cpu = cpu_ppc_init(machine->cpu_model);
> -        if (cpu == NULL) {
> -            error_report("Unable to find PowerPC CPU definition");
> -            exit(1);
> +
> +    if (smc->dr_cpu_enabled) {
> +        spapr->cores = g_new0(Object *, spapr_max_cores);
> +
> +        for (i = 0; i < spapr_max_cores; i++) {
> +            int core_dt_id = i * smt;
> +
> +            if (i < spapr_cores) {
> +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> +
> +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> +                                        &error_fatal);
> +                object_property_set_int(core, smp_threads, "threads",
> +                                        &error_fatal);
> +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> +                                        &error_fatal);
> +                object_property_set_bool(core, true, "realized", &error_fatal);
> +            }
>          }
> -        spapr_cpu_init(spapr, cpu, &error_fatal);
> +    } else {
> +        for (i = 0; i < smp_cpus; i++) {
> +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> +            if (cpu == NULL) {
> +                error_report("Unable to find PowerPC CPU definition");
> +                exit(1);
> +            }
> +            spapr_cpu_init(spapr, cpu, &error_fatal);
> +       }
>      }
>  
>      if (kvm_enabled()) {
> @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
>                                               DeviceState *dev)
>  {
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> +        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>          return HOTPLUG_HANDLER(machine);
>      }
>      return NULL;
> @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
>  
>      smc->dr_lmb_enabled = true;
> +    smc->dr_cpu_enabled = true;
>      fwc->get_dev_path = spapr_get_fw_dev_path;
>      nc->nmi_monitor_handler = spapr_nmi;
>  }
> @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
>  
>      spapr_machine_2_6_class_options(mc);
>      smc->use_ohci_by_default = true;
> +    smc->dr_cpu_enabled = false;
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
>  }
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> new file mode 100644
> index 0000000..8c6d71d
> --- /dev/null
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -0,0 +1,199 @@
> +/*
> + * 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>
> +#include "target-ppc/kvm_ppc.h"
> +
> +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> +                                          Error **errp)
> +{
> +    int i;
> +    Error *local_err = NULL;
> +
> +    for (i = 0; i < threads; i++) {
> +        char id[32];
> +
> +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> +                          object_class_get_name(core->oc));
> +        snprintf(id, sizeof(id), "thread[%d]", i);
> +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> +                                  &local_err);
> +        if (local_err) {
> +            goto err;
> +        }
> +    }
> +    return;
> +
> +err:
> +    while (--i) {
> +        object_unparent(OBJECT(&core->threads[i]));
> +    }
> +    error_propagate(errp, local_err);
> +}
> +
> +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> +{
> +    Error **errp = opaque;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    CPUState *cs = CPU(child);
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    object_property_set_bool(child, true, "realized", errp);
> +    if (*errp) {
> +        return 1;
> +    }
> +
> +    spapr_cpu_init(spapr, cpu, errp);
> +    if (*errp) {
> +        return 1;
> +    }
> +
> +    spapr_cpu_reset(cpu);
should it be move to spapr_cpu_init() ?

> +    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());
> +    int spapr_max_cores = max_cpus / smp_threads;
this should be in machine code

> +    Error *local_err = NULL;
> +    int threads = 0;
> +    int core_dt_id, core_id;
> +    int smt = kvmppc_smt_threads();
> +
> +    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);
since spapr_core is inherited from cpu-core,
you can cast to cpu-core and use fields directly here instead of using
property setters.

> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    if (threads != smp_threads) {
> +        error_setg(&local_err, "threads must be %d", smp_threads);
> +        goto out;
> +    }
move to machine handler

> +
> +    if (!core->oc) {
> +        error_setg(&local_err, "cpu_model property isn't set");
> +        goto out;
> +    }
> +
> +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    if (core_dt_id % smt) {
> +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> +        goto out;
> +    }
> +
> +    core_id = core_dt_id / smt;
> +    if (core_id < 0 || core_id >= spapr_max_cores) {
> +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> +        goto out;
> +    }
maybe due to nameing it's a bit confusing,
what's difference between core_id and core_dt_id?

> +
> +    /*
> +     * TODO: This check will be moved to ->pre_plug() as suggested by Igor
> +     * when there is consensus pre_plug hook.
> +     */
> +    if (spapr->cores[core_id]) {
> +        error_setg(&local_err, "core %d already populated", core_dt_id);
> +        goto out;
> +    }
> +
> +    core->threads = g_new0(PowerPCCPU, threads);
> +    spapr_cpu_core_create_threads(core, threads, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    return;
> +
> +out:
> +    if (local_err) {
> +        g_free(core->threads);
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
> +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    /*
> +     * TODO: This returns the full type instead of just cpu_model. For eg,
> +     * host-powerpc64-cpu is returned where just "host" is expected.
> +     */
> +    return g_strdup(object_class_get_name(core->oc));
> +}
> +
> +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());
> +    ObjectClass *oc = cpu_class_by_name(TYPE_POWERPC_CPU, val);
> +    ObjectClass *oc_base = cpu_class_by_name(TYPE_POWERPC_CPU,
> +                                             machine->cpu_model);
> +    if (!oc) {
> +        error_setg(errp, "Unknown CPU model %s", val);
> +        return;
> +    }
> +
> +    /*
> +     * Currently cpu_model can't be different from what is specified with -cpu
> +     */
> +    if (strcmp(object_class_get_name(oc), object_class_get_name(oc_base))) {
> +        error_setg(errp, "cpu_model must be %s", machine->cpu_model);
> +        return;
> +    }
> +
> +    core->oc = oc;
> +}
> +
> +static void spapr_cpu_core_instance_init(Object *obj)
> +{
> +    object_property_add_str(obj, "cpu_model",
> +                            spapr_cpu_core_prop_get_cpu_model,
> +                            spapr_cpu_core_prop_set_cpu_model,
> +                            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.h b/include/hw/ppc/spapr.h
> index 098d85d..c099c3c 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -36,6 +36,7 @@ struct sPAPRMachineClass {
>  
>      /*< public >*/
>      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
> +    bool dr_cpu_enabled;       /* enable dynamic-reconfig/hotplug of CPUs */
>      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
>  };
>  
> @@ -79,6 +80,7 @@ struct sPAPRMachineState {
>      /*< public >*/
>      char *kvm_type;
>      MemoryHotplugState hotplug_memory;
> +    Object **cores;
I'd prefer "Object *cores[0];" as it tells us that it's an array

>  };
>  
>  #define H_SUCCESS         0
> @@ -585,6 +587,8 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
>                                         uint32_t count);
>  void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
>                                            uint32_t count);
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
> +void spapr_cpu_reset(void *opaque);
>  
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> new file mode 100644
> index 0000000..48fb76a
> --- /dev/null
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -0,0 +1,28 @@
> +/*
> + * 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 >*/
> +    CPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *oc;
> +    PowerPCCPU *threads;
> +} sPAPRCPUCore;
> +
> +#endif

On 14.03.2016 11:25, Igor Mammedov wrote:
> On Fri, 11 Mar 2016 10:24:35 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> 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.
>>
>> Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
>> CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
>> topologies that result in partially filled cores. Both of these are done
>> only if CPU core hotplug is supported.
>>
>> Note: An unrelated change in the call to xics_system_init() is done
>> in this patch as it makes sense to use the local variable smt introduced
>> in this patch instead of kvmppc_smt_threads() call here.
>>
>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/Makefile.objs            |   1 +
>>  hw/ppc/spapr.c                  |  68 +++++++++++---
>>  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h          |   4 +
>>  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
>>  5 files changed, 287 insertions(+), 13 deletions(-)
>>  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.c b/hw/ppc/spapr.c
>> index 64c4acc..cffe8c8 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
...
>> @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
>>      if (machine->cpu_model == NULL) {
>>          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
>>      }
>> -    for (i = 0; i < smp_cpus; i++) {
>> -        cpu = cpu_ppc_init(machine->cpu_model);
>> -        if (cpu == NULL) {
>> -            error_report("Unable to find PowerPC CPU definition");
>> -            exit(1);
>> +
>> +    if (smc->dr_cpu_enabled) {
>> +        spapr->cores = g_new0(Object *, spapr_max_cores);

The spapr->cores _pointer_ is allocate with g_new0 here ...

>> +        for (i = 0; i < spapr_max_cores; i++) {
>> +            int core_dt_id = i * smt;
>> +
>> +            if (i < spapr_cores) {
>> +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
>> +
>> +                object_property_set_str(core, machine->cpu_model, "cpu_model",
>> +                                        &error_fatal);
>> +                object_property_set_int(core, smp_threads, "threads",
>> +                                        &error_fatal);
>> +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
>> +                                        &error_fatal);
>> +                object_property_set_bool(core, true, "realized", &error_fatal);
>> +            }
>>          }
>> -        spapr_cpu_init(spapr, cpu, &error_fatal);
>> +    } else {
>> +        for (i = 0; i < smp_cpus; i++) {
>> +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
>> +            if (cpu == NULL) {
>> +                error_report("Unable to find PowerPC CPU definition");
>> +                exit(1);
>> +            }
>> +            spapr_cpu_init(spapr, cpu, &error_fatal);
>> +       }
>>      }
...
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 098d85d..c099c3c 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -36,6 +36,7 @@ struct sPAPRMachineClass {
>>  
>>      /*< public >*/
>>      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
>> +    bool dr_cpu_enabled;       /* enable dynamic-reconfig/hotplug of CPUs */
>>      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
>>  };
>>  
>> @@ -79,6 +80,7 @@ struct sPAPRMachineState {
>>      /*< public >*/
>>      char *kvm_type;
>>      MemoryHotplugState hotplug_memory;
>> +    Object **cores;
> I'd prefer "Object *cores[0];" as it tells us that it's an array

... so you can not declare it as an array here, can you??

 Thomas

On Mon, 14 Mar 2016 11:56:52 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 14.03.2016 11:25, Igor Mammedov wrote:
> > On Fri, 11 Mar 2016 10:24:35 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> 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.
> >>
> >> Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> >> CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> >> topologies that result in partially filled cores. Both of these are done
> >> only if CPU core hotplug is supported.
> >>
> >> Note: An unrelated change in the call to xics_system_init() is done
> >> in this patch as it makes sense to use the local variable smt introduced
> >> in this patch instead of kvmppc_smt_threads() call here.
> >>
> >> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/Makefile.objs            |   1 +
> >>  hw/ppc/spapr.c                  |  68 +++++++++++---
> >>  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/spapr.h          |   4 +
> >>  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
> >>  5 files changed, 287 insertions(+), 13 deletions(-)
> >>  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.c b/hw/ppc/spapr.c
> >> index 64c4acc..cffe8c8 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c  
> ...
> >> @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
> >>      if (machine->cpu_model == NULL) {
> >>          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> >>      }
> >> -    for (i = 0; i < smp_cpus; i++) {
> >> -        cpu = cpu_ppc_init(machine->cpu_model);
> >> -        if (cpu == NULL) {
> >> -            error_report("Unable to find PowerPC CPU definition");
> >> -            exit(1);
> >> +
> >> +    if (smc->dr_cpu_enabled) {
> >> +        spapr->cores = g_new0(Object *, spapr_max_cores);  
> 
> The spapr->cores _pointer_ is allocate with g_new0 here ...
> 
> >> +        for (i = 0; i < spapr_max_cores; i++) {
> >> +            int core_dt_id = i * smt;
> >> +
> >> +            if (i < spapr_cores) {
> >> +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> >> +
> >> +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> >> +                                        &error_fatal);
> >> +                object_property_set_int(core, smp_threads, "threads",
> >> +                                        &error_fatal);
> >> +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> >> +                                        &error_fatal);
> >> +                object_property_set_bool(core, true, "realized", &error_fatal);
> >> +            }
> >>          }
> >> -        spapr_cpu_init(spapr, cpu, &error_fatal);
> >> +    } else {
> >> +        for (i = 0; i < smp_cpus; i++) {
> >> +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> >> +            if (cpu == NULL) {
> >> +                error_report("Unable to find PowerPC CPU definition");
> >> +                exit(1);
> >> +            }
> >> +            spapr_cpu_init(spapr, cpu, &error_fatal);
> >> +       }
> >>      }  
> ...
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 098d85d..c099c3c 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -36,6 +36,7 @@ struct sPAPRMachineClass {
> >>  
> >>      /*< public >*/
> >>      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
> >> +    bool dr_cpu_enabled;       /* enable dynamic-reconfig/hotplug of CPUs */
> >>      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
> >>  };
> >>  
> >> @@ -79,6 +80,7 @@ struct sPAPRMachineState {
> >>      /*< public >*/
> >>      char *kvm_type;
> >>      MemoryHotplugState hotplug_memory;
> >> +    Object **cores;  
> > I'd prefer "Object *cores[0];" as it tells us that it's an array  
> 
> ... so you can not declare it as an array here, can you??
I can't

> 
>  Thomas
>

On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote:
> On Fri, 11 Mar 2016 10:24:35 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> 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.
> > 
> > Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> > CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> > topologies that result in partially filled cores. Both of these are done
> > only if CPU core hotplug is supported.
> > 
> > Note: An unrelated change in the call to xics_system_init() is done
> > in this patch as it makes sense to use the local variable smt introduced
> > in this patch instead of kvmppc_smt_threads() call here.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/Makefile.objs            |   1 +
> >  hw/ppc/spapr.c                  |  68 +++++++++++---
> >  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h          |   4 +
> >  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
> >  5 files changed, 287 insertions(+), 13 deletions(-)
> >  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.c b/hw/ppc/spapr.c
> > index 64c4acc..cffe8c8 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -64,6 +64,7 @@
> >  
> >  #include "hw/compat.h"
> >  #include "qemu-common.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> >  
> >  #include <libfdt.h>
> >  
> > @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
> >  
> >  }
> >  
> > -static void spapr_cpu_reset(void *opaque)
> > +void spapr_cpu_reset(void *opaque)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      PowerPCCPU *cpu = opaque;
> > @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> >      machine->boot_order = g_strdup(boot_device);
> >  }
> >  
> > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > -                           Error **errp)
> > +/*
> > + * TODO: Check if some of these can be moved to rtas_start_cpu() where
> > + * a few other things required for hotplugged CPUs are being done.
> > + */
> > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> >  {
> >      CPUPPCState *env = &cpu->env;
> >  
> > @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
> >      const char *kernel_filename = machine->kernel_filename;
> >      const char *kernel_cmdline = machine->kernel_cmdline;
> >      const char *initrd_filename = machine->initrd_filename;
> > -    PowerPCCPU *cpu;
> >      PCIHostState *phb;
> >      int i;
> >      MemoryRegion *sysmem = get_system_memory();
> > @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
> >      long load_limit, fw_size;
> >      bool kernel_le = false;
> >      char *filename;
> > +    int smt = kvmppc_smt_threads();
> > +    int spapr_cores = smp_cpus / smp_threads;
> > +    int spapr_max_cores = max_cpus / smp_threads;
> > +
> > +    if (smc->dr_cpu_enabled) {
> > +        if (smp_cpus % smp_threads) {
> > +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > +                         smp_cpus, smp_threads);
> > +            exit(1);
> > +        }
> > +        if (max_cpus % smp_threads) {
> > +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > +                         max_cpus, smp_threads);
> > +            exit(1);
> > +        }
> > +    }
> >  
> >      msi_supported = true;
> >  
> > @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >      /* Set up Interrupt Controller before we create the VCPUs */
> >      spapr->icp = xics_system_init(machine,
> > -                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> > -                                               smp_threads),
> > +                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),
> is smt == smp_threads, if not then what's a difference?

smp_threads is the specified SMT mode of the guest, smt defines the max
SMT guest mode that can be supported on this host.

BTW as I noted in the patch description, this change is unrelated to
CPU hotplug. I did this since I introduced a separate variable (smt)
for kvmppc_smt_threads() and hence replaced the above use of
kvmppc_smt_threads() too.

> 
> >                                    XICS_IRQS, &error_fatal);
> >  
> >      if (smc->dr_lmb_enabled) {
> > @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
> >      if (machine->cpu_model == NULL) {
> >          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> >      }
> > -    for (i = 0; i < smp_cpus; i++) {
> > -        cpu = cpu_ppc_init(machine->cpu_model);
> > -        if (cpu == NULL) {
> > -            error_report("Unable to find PowerPC CPU definition");
> > -            exit(1);
> > +
> > +    if (smc->dr_cpu_enabled) {
> > +        spapr->cores = g_new0(Object *, spapr_max_cores);
> > +
> > +        for (i = 0; i < spapr_max_cores; i++) {
> > +            int core_dt_id = i * smt;
> > +
> > +            if (i < spapr_cores) {
> > +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> > +
> > +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> > +                                        &error_fatal);
> > +                object_property_set_int(core, smp_threads, "threads",
> > +                                        &error_fatal);
> > +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> > +                                        &error_fatal);
> > +                object_property_set_bool(core, true, "realized", &error_fatal);
> > +            }
> >          }
> > -        spapr_cpu_init(spapr, cpu, &error_fatal);
> > +    } else {
> > +        for (i = 0; i < smp_cpus; i++) {
> > +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > +            if (cpu == NULL) {
> > +                error_report("Unable to find PowerPC CPU definition");
> > +                exit(1);
> > +            }
> > +            spapr_cpu_init(spapr, cpu, &error_fatal);
> > +       }
> >      }
> >  
> >      if (kvm_enabled()) {
> > @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> >  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> >                                               DeviceState *dev)
> >  {
> > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > +        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> >          return HOTPLUG_HANDLER(machine);
> >      }
> >      return NULL;
> > @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> >  
> >      smc->dr_lmb_enabled = true;
> > +    smc->dr_cpu_enabled = true;
> >      fwc->get_dev_path = spapr_get_fw_dev_path;
> >      nc->nmi_monitor_handler = spapr_nmi;
> >  }
> > @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
> >  
> >      spapr_machine_2_6_class_options(mc);
> >      smc->use_ohci_by_default = true;
> > +    smc->dr_cpu_enabled = false;
> >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> >  }
> >  
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > new file mode 100644
> > index 0000000..8c6d71d
> > --- /dev/null
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -0,0 +1,199 @@
> > +/*
> > + * 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>
> > +#include "target-ppc/kvm_ppc.h"
> > +
> > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> > +                                          Error **errp)
> > +{
> > +    int i;
> > +    Error *local_err = NULL;
> > +
> > +    for (i = 0; i < threads; i++) {
> > +        char id[32];
> > +
> > +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> > +                          object_class_get_name(core->oc));
> > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > +                                  &local_err);
> > +        if (local_err) {
> > +            goto err;
> > +        }
> > +    }
> > +    return;
> > +
> > +err:
> > +    while (--i) {
> > +        object_unparent(OBJECT(&core->threads[i]));
> > +    }
> > +    error_propagate(errp, local_err);
> > +}
> > +
> > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > +{
> > +    Error **errp = opaque;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    CPUState *cs = CPU(child);
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > +    object_property_set_bool(child, true, "realized", errp);
> > +    if (*errp) {
> > +        return 1;
> > +    }
> > +
> > +    spapr_cpu_init(spapr, cpu, errp);
> > +    if (*errp) {
> > +        return 1;
> > +    }
> > +
> > +    spapr_cpu_reset(cpu);
> should it be move to spapr_cpu_init() ?

Could be moved.

> 
> > +    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());
> > +    int spapr_max_cores = max_cpus / smp_threads;
> this should be in machine code

With ->pre_plug(), won't have this here.

> 
> > +    Error *local_err = NULL;
> > +    int threads = 0;
> > +    int core_dt_id, core_id;
> > +    int smt = kvmppc_smt_threads();
> > +
> > +    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);
> since spapr_core is inherited from cpu-core,
> you can cast to cpu-core and use fields directly here instead of using
> property setters.

Ah ok.

> 
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    if (threads != smp_threads) {
> > +        error_setg(&local_err, "threads must be %d", smp_threads);
> > +        goto out;
> > +    }
> move to machine handler

Ok, guess you mean pre_plug handler ?

> 
> > +
> > +    if (!core->oc) {
> > +        error_setg(&local_err, "cpu_model property isn't set");
> > +        goto out;
> > +    }
> > +
> > +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    if (core_dt_id % smt) {
> > +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> > +        goto out;
> > +    }
> > +
> > +    core_id = core_dt_id / smt;
> > +    if (core_id < 0 || core_id >= spapr_max_cores) {
> > +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> > +        goto out;
> > +    }
> maybe due to nameing it's a bit confusing,
> what's difference between core_id and core_dt_id?

core_dt_id is the device tree IDs that we use with PowerPC cores. This is
what we use with "core" property of CPU_CORE. Since core_dt_id doesn't
grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host),
I am translating that to contiguous integer core_id so that I can
store the pointer of the realized core in the appropriate slot of
spapr->cpu_cores[] array.

Regards,
Bharata.

On Tue, Mar 15, 2016 at 02:44:01PM +0530, Bharata B Rao wrote:
> On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote:
> > On Fri, 11 Mar 2016 10:24:35 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> 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.
> > > 
> > > Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> > > CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> > > topologies that result in partially filled cores. Both of these are done
> > > only if CPU core hotplug is supported.
> > > 
> > > Note: An unrelated change in the call to xics_system_init() is done
> > > in this patch as it makes sense to use the local variable smt introduced
> > > in this patch instead of kvmppc_smt_threads() call here.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/Makefile.objs            |   1 +
> > >  hw/ppc/spapr.c                  |  68 +++++++++++---
> > >  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr.h          |   4 +
> > >  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
> > >  5 files changed, 287 insertions(+), 13 deletions(-)
> > >  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.c b/hw/ppc/spapr.c
> > > index 64c4acc..cffe8c8 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -64,6 +64,7 @@
> > >  
> > >  #include "hw/compat.h"
> > >  #include "qemu-common.h"
> > > +#include "hw/ppc/spapr_cpu_core.h"
> > >  
> > >  #include <libfdt.h>
> > >  
> > > @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
> > >  
> > >  }
> > >  
> > > -static void spapr_cpu_reset(void *opaque)
> > > +void spapr_cpu_reset(void *opaque)
> > >  {
> > >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > >      PowerPCCPU *cpu = opaque;
> > > @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> > >      machine->boot_order = g_strdup(boot_device);
> > >  }
> > >  
> > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > > -                           Error **errp)
> > > +/*
> > > + * TODO: Check if some of these can be moved to rtas_start_cpu() where
> > > + * a few other things required for hotplugged CPUs are being done.
> > > + */
> > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > >  {
> > >      CPUPPCState *env = &cpu->env;
> > >  
> > > @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
> > >      const char *kernel_filename = machine->kernel_filename;
> > >      const char *kernel_cmdline = machine->kernel_cmdline;
> > >      const char *initrd_filename = machine->initrd_filename;
> > > -    PowerPCCPU *cpu;
> > >      PCIHostState *phb;
> > >      int i;
> > >      MemoryRegion *sysmem = get_system_memory();
> > > @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
> > >      long load_limit, fw_size;
> > >      bool kernel_le = false;
> > >      char *filename;
> > > +    int smt = kvmppc_smt_threads();
> > > +    int spapr_cores = smp_cpus / smp_threads;
> > > +    int spapr_max_cores = max_cpus / smp_threads;
> > > +
> > > +    if (smc->dr_cpu_enabled) {
> > > +        if (smp_cpus % smp_threads) {
> > > +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > > +                         smp_cpus, smp_threads);
> > > +            exit(1);
> > > +        }
> > > +        if (max_cpus % smp_threads) {
> > > +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > > +                         max_cpus, smp_threads);
> > > +            exit(1);
> > > +        }
> > > +    }
> > >  
> > >      msi_supported = true;
> > >  
> > > @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> > >  
> > >      /* Set up Interrupt Controller before we create the VCPUs */
> > >      spapr->icp = xics_system_init(machine,
> > > -                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> > > -                                               smp_threads),
> > > +                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),
> > is smt == smp_threads, if not then what's a difference?
> 
> smp_threads is the specified SMT mode of the guest, smt defines the max
> SMT guest mode that can be supported on this host.
> 
> BTW as I noted in the patch description, this change is unrelated to
> CPU hotplug. I did this since I introduced a separate variable (smt)
> for kvmppc_smt_threads() and hence replaced the above use of
> kvmppc_smt_threads() too.
> 
> > 
> > >                                    XICS_IRQS, &error_fatal);
> > >  
> > >      if (smc->dr_lmb_enabled) {
> > > @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
> > >      if (machine->cpu_model == NULL) {
> > >          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> > >      }
> > > -    for (i = 0; i < smp_cpus; i++) {
> > > -        cpu = cpu_ppc_init(machine->cpu_model);
> > > -        if (cpu == NULL) {
> > > -            error_report("Unable to find PowerPC CPU definition");
> > > -            exit(1);
> > > +
> > > +    if (smc->dr_cpu_enabled) {
> > > +        spapr->cores = g_new0(Object *, spapr_max_cores);
> > > +
> > > +        for (i = 0; i < spapr_max_cores; i++) {
> > > +            int core_dt_id = i * smt;
> > > +
> > > +            if (i < spapr_cores) {
> > > +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> > > +
> > > +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> > > +                                        &error_fatal);
> > > +                object_property_set_int(core, smp_threads, "threads",
> > > +                                        &error_fatal);
> > > +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> > > +                                        &error_fatal);
> > > +                object_property_set_bool(core, true, "realized", &error_fatal);
> > > +            }
> > >          }
> > > -        spapr_cpu_init(spapr, cpu, &error_fatal);
> > > +    } else {
> > > +        for (i = 0; i < smp_cpus; i++) {
> > > +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > > +            if (cpu == NULL) {
> > > +                error_report("Unable to find PowerPC CPU definition");
> > > +                exit(1);
> > > +            }
> > > +            spapr_cpu_init(spapr, cpu, &error_fatal);
> > > +       }
> > >      }
> > >  
> > >      if (kvm_enabled()) {
> > > @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> > >  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> > >                                               DeviceState *dev)
> > >  {
> > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > > +        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > >          return HOTPLUG_HANDLER(machine);
> > >      }
> > >      return NULL;
> > > @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > >  
> > >      smc->dr_lmb_enabled = true;
> > > +    smc->dr_cpu_enabled = true;
> > >      fwc->get_dev_path = spapr_get_fw_dev_path;
> > >      nc->nmi_monitor_handler = spapr_nmi;
> > >  }
> > > @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
> > >  
> > >      spapr_machine_2_6_class_options(mc);
> > >      smc->use_ohci_by_default = true;
> > > +    smc->dr_cpu_enabled = false;
> > >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> > >  }
> > >  
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > new file mode 100644
> > > index 0000000..8c6d71d
> > > --- /dev/null
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -0,0 +1,199 @@
> > > +/*
> > > + * 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>
> > > +#include "target-ppc/kvm_ppc.h"
> > > +
> > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> > > +                                          Error **errp)
> > > +{
> > > +    int i;
> > > +    Error *local_err = NULL;
> > > +
> > > +    for (i = 0; i < threads; i++) {
> > > +        char id[32];
> > > +
> > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> > > +                          object_class_get_name(core->oc));
> > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > +                                  &local_err);
> > > +        if (local_err) {
> > > +            goto err;
> > > +        }
> > > +    }
> > > +    return;
> > > +
> > > +err:
> > > +    while (--i) {
> > > +        object_unparent(OBJECT(&core->threads[i]));
> > > +    }
> > > +    error_propagate(errp, local_err);
> > > +}
> > > +
> > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > +{
> > > +    Error **errp = opaque;
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > +    CPUState *cs = CPU(child);
> > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > +
> > > +    object_property_set_bool(child, true, "realized", errp);
> > > +    if (*errp) {
> > > +        return 1;
> > > +    }
> > > +
> > > +    spapr_cpu_init(spapr, cpu, errp);
> > > +    if (*errp) {
> > > +        return 1;
> > > +    }
> > > +
> > > +    spapr_cpu_reset(cpu);
> > should it be move to spapr_cpu_init() ?
> 
> Could be moved.
> 
> > 
> > > +    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());
> > > +    int spapr_max_cores = max_cpus / smp_threads;
> > this should be in machine code
> 
> With ->pre_plug(), won't have this here.
> 
> > 
> > > +    Error *local_err = NULL;
> > > +    int threads = 0;
> > > +    int core_dt_id, core_id;
> > > +    int smt = kvmppc_smt_threads();
> > > +
> > > +    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);
> > since spapr_core is inherited from cpu-core,
> > you can cast to cpu-core and use fields directly here instead of using
> > property setters.
> 
> Ah ok.
> 
> > 
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (threads != smp_threads) {
> > > +        error_setg(&local_err, "threads must be %d", smp_threads);
> > > +        goto out;
> > > +    }
> > move to machine handler
> 
> Ok, guess you mean pre_plug handler ?
> 
> > 
> > > +
> > > +    if (!core->oc) {
> > > +        error_setg(&local_err, "cpu_model property isn't set");
> > > +        goto out;
> > > +    }
> > > +
> > > +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (core_dt_id % smt) {
> > > +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> > > +        goto out;
> > > +    }
> > > +
> > > +    core_id = core_dt_id / smt;
> > > +    if (core_id < 0 || core_id >= spapr_max_cores) {
> > > +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> > > +        goto out;
> > > +    }
> > maybe due to nameing it's a bit confusing,
> > what's difference between core_id and core_dt_id?
> 
> core_dt_id is the device tree IDs that we use with PowerPC cores. This is
> what we use with "core" property of CPU_CORE. Since core_dt_id doesn't
> grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host),
> I am translating that to contiguous integer core_id so that I can
> store the pointer of the realized core in the appropriate slot of
> spapr->cpu_cores[] array.

So, I see why the distinction is there, but it is kinda confusing.
I'm wondering if we could do away with the spapr->cores array entirely
and instead just access the core objects via the QOM tree - QOM
"arrays" (i.e. properties named like foo[NNN]) can be sparse, so
there's no need to allocate dense ids.

Alternatively renaming "core_id" to "index" would be marginally less
confusing.

On Tue, 15 Mar 2016 14:44:01 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote:
> > On Fri, 11 Mar 2016 10:24:35 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> 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.
> > > 
> > > Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> > > CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> > > topologies that result in partially filled cores. Both of these are done
> > > only if CPU core hotplug is supported.
> > > 
> > > Note: An unrelated change in the call to xics_system_init() is done
> > > in this patch as it makes sense to use the local variable smt introduced
> > > in this patch instead of kvmppc_smt_threads() call here.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/Makefile.objs            |   1 +
> > >  hw/ppc/spapr.c                  |  68 +++++++++++---
> > >  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr.h          |   4 +
> > >  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
> > >  5 files changed, 287 insertions(+), 13 deletions(-)
> > >  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.c b/hw/ppc/spapr.c
> > > index 64c4acc..cffe8c8 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -64,6 +64,7 @@
> > >  
> > >  #include "hw/compat.h"
> > >  #include "qemu-common.h"
> > > +#include "hw/ppc/spapr_cpu_core.h"
> > >  
> > >  #include <libfdt.h>
> > >  
> > > @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
> > >  
> > >  }
> > >  
> > > -static void spapr_cpu_reset(void *opaque)
> > > +void spapr_cpu_reset(void *opaque)
> > >  {
> > >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > >      PowerPCCPU *cpu = opaque;
> > > @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> > >      machine->boot_order = g_strdup(boot_device);
> > >  }
> > >  
> > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > > -                           Error **errp)
> > > +/*
> > > + * TODO: Check if some of these can be moved to rtas_start_cpu() where
> > > + * a few other things required for hotplugged CPUs are being done.
> > > + */
> > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > >  {
> > >      CPUPPCState *env = &cpu->env;
> > >  
> > > @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
> > >      const char *kernel_filename = machine->kernel_filename;
> > >      const char *kernel_cmdline = machine->kernel_cmdline;
> > >      const char *initrd_filename = machine->initrd_filename;
> > > -    PowerPCCPU *cpu;
> > >      PCIHostState *phb;
> > >      int i;
> > >      MemoryRegion *sysmem = get_system_memory();
> > > @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
> > >      long load_limit, fw_size;
> > >      bool kernel_le = false;
> > >      char *filename;
> > > +    int smt = kvmppc_smt_threads();
> > > +    int spapr_cores = smp_cpus / smp_threads;
> > > +    int spapr_max_cores = max_cpus / smp_threads;
> > > +
> > > +    if (smc->dr_cpu_enabled) {
> > > +        if (smp_cpus % smp_threads) {
> > > +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > > +                         smp_cpus, smp_threads);
> > > +            exit(1);
> > > +        }
> > > +        if (max_cpus % smp_threads) {
> > > +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > > +                         max_cpus, smp_threads);
> > > +            exit(1);
> > > +        }
> > > +    }
> > >  
> > >      msi_supported = true;
> > >  
> > > @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> > >  
> > >      /* Set up Interrupt Controller before we create the VCPUs */
> > >      spapr->icp = xics_system_init(machine,
> > > -                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> > > -                                               smp_threads),
> > > +                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),  
> > is smt == smp_threads, if not then what's a difference?  
> 
> smp_threads is the specified SMT mode of the guest, smt defines the max
> SMT guest mode that can be supported on this host.
> 
> BTW as I noted in the patch description, this change is unrelated to
> CPU hotplug. I did this since I introduced a separate variable (smt)
> for kvmppc_smt_threads() and hence replaced the above use of
> kvmppc_smt_threads() too.
> 
> >   
> > >                                    XICS_IRQS, &error_fatal);
> > >  
> > >      if (smc->dr_lmb_enabled) {
> > > @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
> > >      if (machine->cpu_model == NULL) {
> > >          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> > >      }
> > > -    for (i = 0; i < smp_cpus; i++) {
> > > -        cpu = cpu_ppc_init(machine->cpu_model);
> > > -        if (cpu == NULL) {
> > > -            error_report("Unable to find PowerPC CPU definition");
> > > -            exit(1);
> > > +
> > > +    if (smc->dr_cpu_enabled) {
> > > +        spapr->cores = g_new0(Object *, spapr_max_cores);
> > > +
> > > +        for (i = 0; i < spapr_max_cores; i++) {
> > > +            int core_dt_id = i * smt;
> > > +
> > > +            if (i < spapr_cores) {
> > > +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> > > +
> > > +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> > > +                                        &error_fatal);
> > > +                object_property_set_int(core, smp_threads, "threads",
> > > +                                        &error_fatal);
> > > +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> > > +                                        &error_fatal);
> > > +                object_property_set_bool(core, true, "realized", &error_fatal);
> > > +            }
> > >          }
> > > -        spapr_cpu_init(spapr, cpu, &error_fatal);
> > > +    } else {
> > > +        for (i = 0; i < smp_cpus; i++) {
> > > +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > > +            if (cpu == NULL) {
> > > +                error_report("Unable to find PowerPC CPU definition");
> > > +                exit(1);
> > > +            }
> > > +            spapr_cpu_init(spapr, cpu, &error_fatal);
> > > +       }
> > >      }
> > >  
> > >      if (kvm_enabled()) {
> > > @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> > >  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> > >                                               DeviceState *dev)
> > >  {
> > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > > +        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > >          return HOTPLUG_HANDLER(machine);
> > >      }
> > >      return NULL;
> > > @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > >  
> > >      smc->dr_lmb_enabled = true;
> > > +    smc->dr_cpu_enabled = true;
> > >      fwc->get_dev_path = spapr_get_fw_dev_path;
> > >      nc->nmi_monitor_handler = spapr_nmi;
> > >  }
> > > @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
> > >  
> > >      spapr_machine_2_6_class_options(mc);
> > >      smc->use_ohci_by_default = true;
> > > +    smc->dr_cpu_enabled = false;
> > >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> > >  }
> > >  
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > new file mode 100644
> > > index 0000000..8c6d71d
> > > --- /dev/null
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -0,0 +1,199 @@
> > > +/*
> > > + * 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>
> > > +#include "target-ppc/kvm_ppc.h"
> > > +
> > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> > > +                                          Error **errp)
> > > +{
> > > +    int i;
> > > +    Error *local_err = NULL;
> > > +
> > > +    for (i = 0; i < threads; i++) {
> > > +        char id[32];
> > > +
> > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> > > +                          object_class_get_name(core->oc));
> > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > +                                  &local_err);
> > > +        if (local_err) {
> > > +            goto err;
> > > +        }
> > > +    }
> > > +    return;
> > > +
> > > +err:
> > > +    while (--i) {
> > > +        object_unparent(OBJECT(&core->threads[i]));
> > > +    }
> > > +    error_propagate(errp, local_err);
> > > +}
> > > +
> > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > +{
> > > +    Error **errp = opaque;
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > +    CPUState *cs = CPU(child);
> > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > +
> > > +    object_property_set_bool(child, true, "realized", errp);
> > > +    if (*errp) {
> > > +        return 1;
> > > +    }
> > > +
> > > +    spapr_cpu_init(spapr, cpu, errp);
> > > +    if (*errp) {
> > > +        return 1;
> > > +    }
> > > +
> > > +    spapr_cpu_reset(cpu);  
> > should it be move to spapr_cpu_init() ?  
> 
> Could be moved.
> 
> >   
> > > +    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());
> > > +    int spapr_max_cores = max_cpus / smp_threads;  
> > this should be in machine code  
> 
> With ->pre_plug(), won't have this here.
> 
> >   
> > > +    Error *local_err = NULL;
> > > +    int threads = 0;
> > > +    int core_dt_id, core_id;
> > > +    int smt = kvmppc_smt_threads();
> > > +
> > > +    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);  
> > since spapr_core is inherited from cpu-core,
> > you can cast to cpu-core and use fields directly here instead of using
> > property setters.  
> 
> Ah ok.
> 
> >   
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (threads != smp_threads) {
> > > +        error_setg(&local_err, "threads must be %d", smp_threads);
> > > +        goto out;
> > > +    }  
> > move to machine handler  
> 
> Ok, guess you mean pre_plug handler ?
yep

> 
> >   
> > > +
> > > +    if (!core->oc) {
> > > +        error_setg(&local_err, "cpu_model property isn't set");
> > > +        goto out;
> > > +    }
> > > +
> > > +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (core_dt_id % smt) {
> > > +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> > > +        goto out;
> > > +    }
> > > +
> > > +    core_id = core_dt_id / smt;
> > > +    if (core_id < 0 || core_id >= spapr_max_cores) {
> > > +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> > > +        goto out;
> > > +    }  
> > maybe due to nameing it's a bit confusing,
> > what's difference between core_id and core_dt_id?  
> 
> core_dt_id is the device tree IDs that we use with PowerPC cores. This is
> what we use with "core" property of CPU_CORE. Since core_dt_id doesn't
> grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host),
> I am translating that to contiguous integer core_id so that I can
> store the pointer of the realized core in the appropriate slot of
> spapr->cpu_cores[] array.
So maybe rename core_id to something else so it wouldn't be confusing.
Also I'd move out core_id check to pre_plug handler as it's an internal
machine index (including spapr_max_cores).

> 
> Regards,
> Bharata.
> 
>

On Tue, 15 Mar 2016 20:34:28 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Mar 15, 2016 at 02:44:01PM +0530, Bharata B Rao wrote:
> > On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote:  
> > > On Fri, 11 Mar 2016 10:24:35 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> 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.
> > > > 
> > > > Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for
> > > > CPU core hotplug. Initialize boot time CPUs as core deivces and prevent
> > > > topologies that result in partially filled cores. Both of these are done
> > > > only if CPU core hotplug is supported.
> > > > 
> > > > Note: An unrelated change in the call to xics_system_init() is done
> > > > in this patch as it makes sense to use the local variable smt introduced
> > > > in this patch instead of kvmppc_smt_threads() call here.
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/ppc/Makefile.objs            |   1 +
> > > >  hw/ppc/spapr.c                  |  68 +++++++++++---
> > > >  hw/ppc/spapr_cpu_core.c         | 199 ++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/ppc/spapr.h          |   4 +
> > > >  include/hw/ppc/spapr_cpu_core.h |  28 ++++++
> > > >  5 files changed, 287 insertions(+), 13 deletions(-)
> > > >  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.c b/hw/ppc/spapr.c
> > > > index 64c4acc..cffe8c8 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -64,6 +64,7 @@
> > > >  
> > > >  #include "hw/compat.h"
> > > >  #include "qemu-common.h"
> > > > +#include "hw/ppc/spapr_cpu_core.h"
> > > >  
> > > >  #include <libfdt.h>
> > > >  
> > > > @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void)
> > > >  
> > > >  }
> > > >  
> > > > -static void spapr_cpu_reset(void *opaque)
> > > > +void spapr_cpu_reset(void *opaque)
> > > >  {
> > > >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > >      PowerPCCPU *cpu = opaque;
> > > > @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> > > >      machine->boot_order = g_strdup(boot_device);
> > > >  }
> > > >  
> > > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > > > -                           Error **errp)
> > > > +/*
> > > > + * TODO: Check if some of these can be moved to rtas_start_cpu() where
> > > > + * a few other things required for hotplugged CPUs are being done.
> > > > + */
> > > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > > >  {
> > > >      CPUPPCState *env = &cpu->env;
> > > >  
> > > > @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machine)
> > > >      const char *kernel_filename = machine->kernel_filename;
> > > >      const char *kernel_cmdline = machine->kernel_cmdline;
> > > >      const char *initrd_filename = machine->initrd_filename;
> > > > -    PowerPCCPU *cpu;
> > > >      PCIHostState *phb;
> > > >      int i;
> > > >      MemoryRegion *sysmem = get_system_memory();
> > > > @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machine)
> > > >      long load_limit, fw_size;
> > > >      bool kernel_le = false;
> > > >      char *filename;
> > > > +    int smt = kvmppc_smt_threads();
> > > > +    int spapr_cores = smp_cpus / smp_threads;
> > > > +    int spapr_max_cores = max_cpus / smp_threads;
> > > > +
> > > > +    if (smc->dr_cpu_enabled) {
> > > > +        if (smp_cpus % smp_threads) {
> > > > +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > > > +                         smp_cpus, smp_threads);
> > > > +            exit(1);
> > > > +        }
> > > > +        if (max_cpus % smp_threads) {
> > > > +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > > > +                         max_cpus, smp_threads);
> > > > +            exit(1);
> > > > +        }
> > > > +    }
> > > >  
> > > >      msi_supported = true;
> > > >  
> > > > @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> > > >  
> > > >      /* Set up Interrupt Controller before we create the VCPUs */
> > > >      spapr->icp = xics_system_init(machine,
> > > > -                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> > > > -                                               smp_threads),
> > > > +                                  DIV_ROUND_UP(max_cpus * smt, smp_threads),  
> > > is smt == smp_threads, if not then what's a difference?  
> > 
> > smp_threads is the specified SMT mode of the guest, smt defines the max
> > SMT guest mode that can be supported on this host.
> > 
> > BTW as I noted in the patch description, this change is unrelated to
> > CPU hotplug. I did this since I introduced a separate variable (smt)
> > for kvmppc_smt_threads() and hence replaced the above use of
> > kvmppc_smt_threads() too.
> >   
> > >   
> > > >                                    XICS_IRQS, &error_fatal);
> > > >  
> > > >      if (smc->dr_lmb_enabled) {
> > > > @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *machine)
> > > >      if (machine->cpu_model == NULL) {
> > > >          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> > > >      }
> > > > -    for (i = 0; i < smp_cpus; i++) {
> > > > -        cpu = cpu_ppc_init(machine->cpu_model);
> > > > -        if (cpu == NULL) {
> > > > -            error_report("Unable to find PowerPC CPU definition");
> > > > -            exit(1);
> > > > +
> > > > +    if (smc->dr_cpu_enabled) {
> > > > +        spapr->cores = g_new0(Object *, spapr_max_cores);
> > > > +
> > > > +        for (i = 0; i < spapr_max_cores; i++) {
> > > > +            int core_dt_id = i * smt;
> > > > +
> > > > +            if (i < spapr_cores) {
> > > > +                Object *core  = object_new(TYPE_SPAPR_CPU_CORE);
> > > > +
> > > > +                object_property_set_str(core, machine->cpu_model, "cpu_model",
> > > > +                                        &error_fatal);
> > > > +                object_property_set_int(core, smp_threads, "threads",
> > > > +                                        &error_fatal);
> > > > +                object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
> > > > +                                        &error_fatal);
> > > > +                object_property_set_bool(core, true, "realized", &error_fatal);
> > > > +            }
> > > >          }
> > > > -        spapr_cpu_init(spapr, cpu, &error_fatal);
> > > > +    } else {
> > > > +        for (i = 0; i < smp_cpus; i++) {
> > > > +            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > > > +            if (cpu == NULL) {
> > > > +                error_report("Unable to find PowerPC CPU definition");
> > > > +                exit(1);
> > > > +            }
> > > > +            spapr_cpu_init(spapr, cpu, &error_fatal);
> > > > +       }
> > > >      }
> > > >  
> > > >      if (kvm_enabled()) {
> > > > @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> > > >  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> > > >                                               DeviceState *dev)
> > > >  {
> > > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > > > +        object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > >          return HOTPLUG_HANDLER(machine);
> > > >      }
> > > >      return NULL;
> > > > @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > >      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > > >  
> > > >      smc->dr_lmb_enabled = true;
> > > > +    smc->dr_cpu_enabled = true;
> > > >      fwc->get_dev_path = spapr_get_fw_dev_path;
> > > >      nc->nmi_monitor_handler = spapr_nmi;
> > > >  }
> > > > @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(MachineClass *mc)
> > > >  
> > > >      spapr_machine_2_6_class_options(mc);
> > > >      smc->use_ohci_by_default = true;
> > > > +    smc->dr_cpu_enabled = false;
> > > >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5);
> > > >  }
> > > >  
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > new file mode 100644
> > > > index 0000000..8c6d71d
> > > > --- /dev/null
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -0,0 +1,199 @@
> > > > +/*
> > > > + * 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>
> > > > +#include "target-ppc/kvm_ppc.h"
> > > > +
> > > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads,
> > > > +                                          Error **errp)
> > > > +{
> > > > +    int i;
> > > > +    Error *local_err = NULL;
> > > > +
> > > > +    for (i = 0; i < threads; i++) {
> > > > +        char id[32];
> > > > +
> > > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]),
> > > > +                          object_class_get_name(core->oc));
> > > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > > +        object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]),
> > > > +                                  &local_err);
> > > > +        if (local_err) {
> > > > +            goto err;
> > > > +        }
> > > > +    }
> > > > +    return;
> > > > +
> > > > +err:
> > > > +    while (--i) {
> > > > +        object_unparent(OBJECT(&core->threads[i]));
> > > > +    }
> > > > +    error_propagate(errp, local_err);
> > > > +}
> > > > +
> > > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > > +{
> > > > +    Error **errp = opaque;
> > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > > +    CPUState *cs = CPU(child);
> > > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > > +
> > > > +    object_property_set_bool(child, true, "realized", errp);
> > > > +    if (*errp) {
> > > > +        return 1;
> > > > +    }
> > > > +
> > > > +    spapr_cpu_init(spapr, cpu, errp);
> > > > +    if (*errp) {
> > > > +        return 1;
> > > > +    }
> > > > +
> > > > +    spapr_cpu_reset(cpu);  
> > > should it be move to spapr_cpu_init() ?  
> > 
> > Could be moved.
> >   
> > >   
> > > > +    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());
> > > > +    int spapr_max_cores = max_cpus / smp_threads;  
> > > this should be in machine code  
> > 
> > With ->pre_plug(), won't have this here.
> >   
> > >   
> > > > +    Error *local_err = NULL;
> > > > +    int threads = 0;
> > > > +    int core_dt_id, core_id;
> > > > +    int smt = kvmppc_smt_threads();
> > > > +
> > > > +    threads = object_property_get_int(OBJECT(dev), "threads", &local_err);  
> > > since spapr_core is inherited from cpu-core,
> > > you can cast to cpu-core and use fields directly here instead of using
> > > property setters.  
> > 
> > Ah ok.
> >   
> > >   
> > > > +    if (local_err) {
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    if (threads != smp_threads) {
> > > > +        error_setg(&local_err, "threads must be %d", smp_threads);
> > > > +        goto out;
> > > > +    }  
> > > move to machine handler  
> > 
> > Ok, guess you mean pre_plug handler ?
> >   
> > >   
> > > > +
> > > > +    if (!core->oc) {
> > > > +        error_setg(&local_err, "cpu_model property isn't set");
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> > > > +    if (local_err) {
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    if (core_dt_id % smt) {
> > > > +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    core_id = core_dt_id / smt;
> > > > +    if (core_id < 0 || core_id >= spapr_max_cores) {
> > > > +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> > > > +        goto out;
> > > > +    }  
> > > maybe due to nameing it's a bit confusing,
> > > what's difference between core_id and core_dt_id?  
> > 
> > core_dt_id is the device tree IDs that we use with PowerPC cores. This is
> > what we use with "core" property of CPU_CORE. Since core_dt_id doesn't
> > grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host),
> > I am translating that to contiguous integer core_id so that I can
> > store the pointer of the realized core in the appropriate slot of
> > spapr->cpu_cores[] array.  
> 
> So, I see why the distinction is there, but it is kinda confusing.
> I'm wondering if we could do away with the spapr->cores array entirely
> and instead just access the core objects via the QOM tree - QOM
> "arrays" (i.e. properties named like foo[NNN]) can be sparse, so
> there's no need to allocate dense ids.
Wouldn't be lookups for duplicate in QOM tree take O(N^2)
when hot-plugging N cpus?

It should be less with sorted array at least.

> Alternatively renaming "core_id" to "index" would be marginally less
> confusing.
>

On Tue, Mar 15, 2016 at 02:46:37PM +0100, Igor Mammedov wrote:
> On Tue, 15 Mar 2016 20:34:28 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Tue, Mar 15, 2016 at 02:44:01PM +0530, Bharata B Rao wrote:
> > > On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote:  
> > > > On Fri, 11 Mar 2016 10:24:35 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
[snip]
> > > > > +    if (!core->oc) {
> > > > > +        error_setg(&local_err, "cpu_model property isn't set");
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err);
> > > > > +    if (local_err) {
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    if (core_dt_id % smt) {
> > > > > +        error_setg(&local_err, "invalid core id %d\n", core_dt_id);
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    core_id = core_dt_id / smt;
> > > > > +    if (core_id < 0 || core_id >= spapr_max_cores) {
> > > > > +        error_setg(&local_err, "core id %d out of range", core_dt_id);
> > > > > +        goto out;
> > > > > +    }  
> > > > maybe due to nameing it's a bit confusing,
> > > > what's difference between core_id and core_dt_id?  
> > > 
> > > core_dt_id is the device tree IDs that we use with PowerPC cores. This is
> > > what we use with "core" property of CPU_CORE. Since core_dt_id doesn't
> > > grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host),
> > > I am translating that to contiguous integer core_id so that I can
> > > store the pointer of the realized core in the appropriate slot of
> > > spapr->cpu_cores[] array.  
> > 
> > So, I see why the distinction is there, but it is kinda confusing.
> > I'm wondering if we could do away with the spapr->cores array entirely
> > and instead just access the core objects via the QOM tree - QOM
> > "arrays" (i.e. properties named like foo[NNN]) can be sparse, so
> > there's no need to allocate dense ids.
> Wouldn't be lookups for duplicate in QOM tree take O(N^2)
> when hot-plugging N cpus?

With the present QOM implementation, yes, although I know Paolo has
made noises about changing that to a hash table.

> It should be less with sorted array at least.

It would, but I doubt the O(N^2) will actually be a problem with
realistic numbers of cpus.