On Fri, Jun 03, 2016 at 03:32:10PM +1000, David Gibson wrote:
> On Thu, May 12, 2016 at 09:18:18AM +0530, Bharata B Rao wrote:
> > 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.
> >
> > TODO: We derive sPAPR core type by looking at -cpu <model>. However
> > we don't take care of "compat=" feature yet for boot time as well
> > as hotplug CPUs.
> >
> > Signed-off-by: Bharata B Rao <bhar...@linux.vnet.ibm.com>
>
> This will need some tweaking for the changes I made to earlier
> patches, otherwise only a couple of tiny nits.
Took care of it.
>
> Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
>
> > ---
> > hw/ppc/spapr.c | 76
> > +++++++++++++++++++++++++++++++++++------
> > hw/ppc/spapr_cpu_core.c | 58 +++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr.h | 2 ++
> > include/hw/ppc/spapr_cpu_core.h | 3 ++
> > 4 files changed, 129 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 95db047..0f64218 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -65,6 +65,7 @@
> >
> > #include "hw/compat.h"
> > #include "qemu/cutils.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> >
> > #include <libfdt.h>
> >
> > @@ -1605,6 +1606,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;
> > @@ -1628,6 +1633,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);
> > }
> >
> > /*
> > @@ -1711,7 +1717,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();
> > @@ -1725,6 +1730,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_nonbroken = true;
> >
> > @@ -1771,8 +1792,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) {
> > @@ -1783,13 +1803,37 @@ 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_cores; i++) {
> > + int core_dt_id = i * smt;
> > + char *type = spapr_get_cpu_core_type(machine->cpu_model);
>
> Probably makes sense to move this lookup outside the loop.
Yes.
>
> > + Object *core;
> > +
> > + if (!object_class_by_name(type)) {
> > + error_report("Unable to find sPAPR CPU Core definition");
> > + exit(1);
> > + }
> > +
> > + 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);
> > + }
> > }
> >
> > if (kvm_enabled()) {
> > @@ -2245,10 +2289,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;
> > @@ -2287,11 +2340,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;
> > }
> > @@ -2376,6 +2431,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 af63ed9..4450362 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -14,6 +14,64 @@
> > #include <sysemu/cpus.h>
> > #include "target-ppc/kvm_ppc.h"
> >
> > +/*
> > + * Return the sPAPR CPU core type for @model which essentially is the CPU
> > + * model specified with -cpu cmdline option.
> > + */
> > +char *spapr_get_cpu_core_type(const char *model)
> > +{
> > + char core_type[32];
> > + gchar **model_pieces = g_strsplit(model, ",", 2);
> > +
> > + snprintf(core_type, 32, "%s-%s", model_pieces[0], TYPE_SPAPR_CPU_CORE);
> > + g_strfreev(model_pieces);
> > + return g_strdup(core_type);
>
> You could use g_strdup_printf() here.
Yes, incorporated the above changes.
Regards,
Bharata.
