Re: [Qemu-devel] [RFC PATCH v2.1 09/12] spapr: convert boot CPUs into CPU core devices

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Bharata B Rao (2016-03-31 03:39:18)
> 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/spapr.c                  | 73 +++++++++++++++++++++++++++++++++++------
>  hw/ppc/spapr_cpu_core.c         | 45 +++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |  2 ++
>  include/hw/ppc/spapr_cpu_core.h |  3 ++
>  4 files changed, 113 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 45ac5dc..1ead043 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>
> 
> @@ -1614,6 +1615,10 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>      machine->boot_order = g_strdup(boot_device);
>  }
> 
> +/*
> + * 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;
> @@ -1644,6 +1649,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
>      xics_cpu_setup(spapr->icp, cpu);
> 
>      qemu_register_reset(spapr_cpu_reset, cpu);
> +    spapr_cpu_reset(cpu);
>  }
> 
>  /*
> @@ -1727,7 +1733,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();
> @@ -1741,6 +1746,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;
> 
> @@ -1787,8 +1808,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),
>                                    XICS_IRQS, &error_fatal);
> 
>      if (smc->dr_lmb_enabled) {
> @@ -1799,13 +1819,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) {

Is there any reason to not just have the for() loop stop at spapr_cores?
Maybe I missed something in the subsequent patches, but it seems like we
never end up doing anything beyond i < spapr_cores.

> +                char *type = spapr_get_cpu_core_type(machine->cpu_model);
> +                Object *core  = object_new(type);
> +
> +                g_free(type);
> +                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);

Is there anything that prevents us using core modeling in the non-DR
case as well? Having management of invididual threads/spapr_cpu_init
contained in within one place seems beneficial beyond hotplug. Is it
because cores are currently limited to cpus == "threads" via pre_plug
hook? Maybe it makes sense to loosen that restriction in the special
case where DR isn't enabled?

> +       }
>      }
> 
>      if (kvm_enabled()) {
> @@ -2257,10 +2298,19 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>      }
>  }
> 
> +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> +                                          DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> +        spapr_core_pre_plug(hotplug_dev, dev, errp);
> +    }
> +}
> +
>  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;
> @@ -2299,11 +2349,13 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->has_dynamic_sysbus = true;
>      mc->pci_allow_0_address = true;
>      mc->get_hotplug_handler = spapr_get_hotpug_handler;
> +    hc->pre_plug = spapr_machine_device_pre_plug;
>      hc->plug = spapr_machine_device_plug;
>      hc->unplug = spapr_machine_device_unplug;
>      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;
>  }
> @@ -2383,6 +2435,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
> index 3751a54..640d143 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -15,6 +15,40 @@
>  #include <sysemu/cpus.h>
>  #include "target-ppc/kvm_ppc.h"
> 
> +void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                         Error **errp)
> +{
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    int spapr_max_cores = max_cpus / smp_threads;
> +    int index;
> +    int smt = kvmppc_smt_threads();
> +    Error *local_err = NULL;
> +    CPUCore *cc = CPU_CORE(dev);
> +
> +    if (cc->threads != smp_threads) {
> +        error_setg(&local_err, "threads must be %d", smp_threads);
> +        goto out;
> +    }
> +
> +    if (cc->core % smt) {
> +        error_setg(&local_err, "invalid core id %d\n", cc->core);
> +        goto out;
> +    }
> +
> +    index = cc->core / smt;
> +    if (index < 0 || index >= spapr_max_cores) {
> +        error_setg(&local_err, "core id %d out of range", cc->core);
> +        goto out;
> +    }
> +
> +    if (spapr->cores[index]) {
> +        error_setg(&local_err, "core %d already populated", cc->core);
> +    }
> +
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
>  static const TypeInfo spapr_cpu_core_type_info = {
>      .name = TYPE_SPAPR_CPU_CORE,
>      .parent = TYPE_CPU_CORE,
> @@ -146,3 +180,14 @@ static void spapr_cpu_core_register_types(void)
>  }
> 
>  type_init(spapr_cpu_core_register_types)
> +
> +/*
> + * TODO: Looks fragile :(
> + */
> +char *spapr_get_cpu_core_type(const char *model)
> +{
> +    char core_type[32];
> +
> +    snprintf(core_type, 32, "%s-%s", model, TYPE_SPAPR_CPU_CORE);
> +    return g_strdup(core_type);
> +}
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 0fdf448..a6956c0 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;
>  };
> 
>  #define H_SUCCESS         0
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 71e69c0..f08f291 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -60,4 +60,7 @@ typedef struct POWER8sPAPRCPUCore {
>      ObjectClass *cpu;
>  } POWER8sPAPRCPUCore;
> 
> +void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                         Error **errp);
> +char *spapr_get_cpu_core_type(const char *model);
>  #endif
> -- 
> 2.1.0
>