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 . However
> > we don't take care of "compat=" feature yet for boot time as well
> > as hotplug CPUs.
> > 
> > Signed-off-by: Bharata B Rao 
> 
> 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 
> 
> > ---
> >  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 
> >  
> > @@ -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 = >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, _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 

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

[David Gibson]

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 . However
> we don't take care of "compat=" feature yet for boot time as well
> as hotplug CPUs.
> 
> Signed-off-by: Bharata B Rao 

This will need some tweaking for the changes I made to earlier
patches, otherwise only a couple of tiny nits.

Reviewed-by: David Gibson 

> ---
>  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 
>  
> @@ -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 = >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, _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.

> +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",
> +_fatal);
> +

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

[Bharata B Rao]

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 . However
we don't take care of "compat=" feature yet for boot time as well
as hotplug CPUs.

Signed-off-by: Bharata B Rao 
---
 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 
 
@@ -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 = >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, _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);
+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",
+_fatal);
+object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE,
+_fatal);
+object_property_set_bool(core, true, "realized", _fatal);
 }
-spapr_cpu_init(spapr, cpu, _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, _fatal);