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

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

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 <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.

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.