From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aog19-0000R2-Qj for qemu-devel@nongnu.org; Fri, 08 Apr 2016 19:40:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aog16-0001ys-BC for qemu-devel@nongnu.org; Fri, 08 Apr 2016 19:40:47 -0400 Received: from e18.ny.us.ibm.com ([129.33.205.208]:34260) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aog16-0001yI-7k for qemu-devel@nongnu.org; Fri, 08 Apr 2016 19:40:44 -0400 Received: from localhost by e18.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 8 Apr 2016 19:40:41 -0400 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <1459413561-30745-10-git-send-email-bharata@linux.vnet.ibm.com> References: <1459413561-30745-1-git-send-email-bharata@linux.vnet.ibm.com> <1459413561-30745-10-git-send-email-bharata@linux.vnet.ibm.com> Message-ID: <20160408233519.30838.52061@loki> Date: Fri, 08 Apr 2016 18:35:19 -0500 Subject: Re: [Qemu-devel] [RFC PATCH v2.1 09/12] spapr: convert boot CPUs into CPU core devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bharata B Rao , qemu-devel@nongnu.org Cc: qemu-ppc@nongnu.org, afaerber@suse.de, david@gibson.dropbear.id.au, imammedo@redhat.com, armbru@redhat.com, thuth@redhat.com, aik@ozlabs.ru, agraf@suse.de, pbonzini@redhat.com, ehabkost@redhat.com, pkrempa@redhat.com, eblake@redhat.com, mjrosato@linux.vnet.ibm.com, borntraeger@de.ibm.com 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 > --- > 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 > = > @@ -1614,6 +1615,10 @@ static void spapr_boot_set(void *opaque, const cha= r *boot_device, > machine->boot_order =3D 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 **e= rrp) > { > CPUPPCState *env =3D &cpu->env; > @@ -1644,6 +1649,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, Power= PCCPU *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 =3D machine->kernel_filename; > const char *kernel_cmdline =3D machine->kernel_cmdline; > const char *initrd_filename =3D machine->initrd_filename; > - PowerPCCPU *cpu; > PCIHostState *phb; > int i; > MemoryRegion *sysmem =3D get_system_memory(); > @@ -1741,6 +1746,22 @@ static void ppc_spapr_init(MachineState *machine) > long load_limit, fw_size; > bool kernel_le =3D false; > char *filename; > + int smt =3D kvmppc_smt_threads(); > + int spapr_cores =3D smp_cpus / smp_threads; > + int spapr_max_cores =3D 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 =3D true; > = > @@ -1787,8 +1808,7 @@ static void ppc_spapr_init(MachineState *machine) > = > /* Set up Interrupt Controller before we create the VCPUs */ > spapr->icp =3D xics_system_init(machine, > - DIV_ROUND_UP(max_cpus * kvmppc_smt_thr= eads(), > - smp_threads), > + DIV_ROUND_UP(max_cpus * smt, smp_threa= ds), > XICS_IRQS, &error_fatal); > = > if (smc->dr_lmb_enabled) { > @@ -1799,13 +1819,34 @@ static void ppc_spapr_init(MachineState *machine) > if (machine->cpu_model =3D=3D NULL) { > machine->cpu_model =3D kvm_enabled() ? "host" : "POWER7"; > } > - for (i =3D 0; i < smp_cpus; i++) { > - cpu =3D cpu_ppc_init(machine->cpu_model); > - if (cpu =3D=3D NULL) { > - error_report("Unable to find PowerPC CPU definition"); > - exit(1); > + > + if (smc->dr_cpu_enabled) { > + spapr->cores =3D g_new0(Object *, spapr_max_cores); > + > + for (i =3D 0; i < spapr_max_cores; i++) { > + int core_dt_id =3D 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. > + char *type =3D spapr_get_cpu_core_type(machine->cpu_mode= l); > + Object *core =3D 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 =3D 0; i < smp_cpus; i++) { > + PowerPCCPU *cpu =3D cpu_ppc_init(machine->cpu_model); > + if (cpu =3D=3D 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 =3D=3D "threads" via pre_plug hook? Maybe it makes sense to loosen that restriction in the special case where DR isn't enabled? > + } > } > = > if (kvm_enabled()) { > @@ -2257,10 +2298,19 @@ static void spapr_machine_device_unplug(HotplugHa= ndler *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; > @@ -2299,11 +2349,13 @@ static void spapr_machine_class_init(ObjectClass = *oc, void *data) > mc->has_dynamic_sysbus =3D true; > mc->pci_allow_0_address =3D true; > mc->get_hotplug_handler =3D spapr_get_hotpug_handler; > + hc->pre_plug =3D spapr_machine_device_pre_plug; > hc->plug =3D spapr_machine_device_plug; > hc->unplug =3D spapr_machine_device_unplug; > mc->cpu_index_to_socket_id =3D spapr_cpu_index_to_socket_id; > = > smc->dr_lmb_enabled =3D true; > + smc->dr_cpu_enabled =3D true; > fwc->get_dev_path =3D spapr_get_fw_dev_path; > nc->nmi_monitor_handler =3D spapr_nmi; > } > @@ -2383,6 +2435,7 @@ static void spapr_machine_2_5_class_options(Machine= Class *mc) > = > spapr_machine_2_6_class_options(mc); > smc->use_ohci_by_default =3D true; > + smc->dr_cpu_enabled =3D 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 3751a54..640d143 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -15,6 +15,40 @@ > #include > #include "target-ppc/kvm_ppc.h" > = > +void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > + Error **errp) > +{ > + sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > + int spapr_max_cores =3D max_cpus / smp_threads; > + int index; > + int smt =3D kvmppc_smt_threads(); > + Error *local_err =3D NULL; > + CPUCore *cc =3D CPU_CORE(dev); > + > + if (cc->threads !=3D smp_threads) { > + error_setg(&local_err, "threads must be %d", smp_threads); > + goto out; > + } > + > + if (cc->core % smt) { > + error_setg(&local_err, "invalid core id %d\n", cc->core); > + goto out; > + } > + > + index =3D cc->core / smt; > + if (index < 0 || index >=3D spapr_max_cores) { > + error_setg(&local_err, "core id %d out of range", cc->core); > + goto out; > + } > + > + if (spapr->cores[index]) { > + error_setg(&local_err, "core %d already populated", cc->core); > + } > + > +out: > + error_propagate(errp, local_err); > +} > + > static const TypeInfo spapr_cpu_core_type_info =3D { > .name =3D TYPE_SPAPR_CPU_CORE, > .parent =3D TYPE_CPU_CORE, > @@ -146,3 +180,14 @@ static void spapr_cpu_core_register_types(void) > } > = > type_init(spapr_cpu_core_register_types) > + > +/* > + * TODO: Looks fragile :( > + */ > +char *spapr_get_cpu_core_type(const char *model) > +{ > + char core_type[32]; > + > + snprintf(core_type, 32, "%s-%s", model, TYPE_SPAPR_CPU_CORE); > + return g_strdup(core_type); > +} > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 0fdf448..a6956c0 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -36,6 +36,7 @@ struct sPAPRMachineClass { > = > /*< public >*/ > bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMB= s */ > + bool dr_cpu_enabled; /* enable dynamic-reconfig/hotplug of CPU= s */ > bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */ > }; > = > @@ -79,6 +80,7 @@ struct sPAPRMachineState { > /*< public >*/ > char *kvm_type; > MemoryHotplugState hotplug_memory; > + Object **cores; > }; > = > #define H_SUCCESS 0 > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_c= ore.h > index 71e69c0..f08f291 100644 > --- a/include/hw/ppc/spapr_cpu_core.h > +++ b/include/hw/ppc/spapr_cpu_core.h > @@ -60,4 +60,7 @@ typedef struct POWER8sPAPRCPUCore { > ObjectClass *cpu; > } POWER8sPAPRCPUCore; > = > +void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > + Error **errp); > +char *spapr_get_cpu_core_type(const char *model); > #endif > -- = > 2.1.0 >=20