Re: [Qemu-devel] [for-2.7 PATCH v3 08/15] spapr: convert boot CPUs into CPU core devices

[Bharata B Rao]

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 <address@hidden>
> 
> 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 <address@hidden>
> 
> > ---
> >  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.