Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device

[Igor Mammedov]

On Fri, 11 Mar 2016 10:24:35 +0530
Bharata B Rao <address@hidden> 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 <address@hidden>
> ---
>  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 <address@hidden>
> + *
> + * 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 <address@hidden>
> + *
> + * 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