On Fri, Apr 08, 2016 at 06:35:19PM -0500, Michael Roth wrote:
> 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 <bhar...@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.
In the next patch, I create DR connectors for all spapr_max_cores but
create only spapr_cores number of cores (boot time cores). Having said that
I will have the for loop changed to spapr_cores in this patch and
move to spapr_max_cores in the next patch.
>
> > + 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?
CPU core devices are created only when CPU DR capability is present. To support
migration of older guests that don't know about CPU core devices, I retain the
CPU thread initialization code as above.
Regards,
Bharata.
