On Tue, Mar 15, 2016 at 02:44:01PM +0530, Bharata B Rao wrote: > On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote: > > On Fri, 11 Mar 2016 10:24:35 +0530 > > Bharata B Rao wrote: > > > > > Add sPAPR specific CPU core device that is based on generic CPU core device. > > > Creating this core device will result in creation of all the CPU thread > > > devices that are part of this core. > > > > > > 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/Makefile.objs | 1 + > > > hw/ppc/spapr.c | 68 +++++++++++--- > > > hw/ppc/spapr_cpu_core.c | 199 ++++++++++++++++++++++++++++++++++++++++ > > > include/hw/ppc/spapr.h | 4 + > > > include/hw/ppc/spapr_cpu_core.h | 28 ++++++ > > > 5 files changed, 287 insertions(+), 13 deletions(-) > > > create mode 100644 hw/ppc/spapr_cpu_core.c > > > create mode 100644 include/hw/ppc/spapr_cpu_core.h > > > > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs > > > index c1ffc77..5cc6608 100644 > > > --- a/hw/ppc/Makefile.objs > > > +++ b/hw/ppc/Makefile.objs > > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o > > > obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o > > > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o > > > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o > > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o > > > ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy) > > > obj-y += spapr_pci_vfio.o > > > endif > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 64c4acc..cffe8c8 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 > > > > > > @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void) > > > > > > } > > > > > > -static void spapr_cpu_reset(void *opaque) > > > +void spapr_cpu_reset(void *opaque) > > > { > > > sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > > PowerPCCPU *cpu = opaque; > > > @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const char *boot_device, > > > machine->boot_order = g_strdup(boot_device); > > > } > > > > > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, > > > - Error **errp) > > > +/* > > > + * 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; > > > > > > @@ -1728,7 +1732,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(); > > > @@ -1742,6 +1745,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; > > > > > > @@ -1788,8 +1807,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), > > is smt == smp_threads, if not then what's a difference? > > smp_threads is the specified SMT mode of the guest, smt defines the max > SMT guest mode that can be supported on this host. > > BTW as I noted in the patch description, this change is unrelated to > CPU hotplug. I did this since I introduced a separate variable (smt) > for kvmppc_smt_threads() and hence replaced the above use of > kvmppc_smt_threads() too. > > > > > > XICS_IRQS, &error_fatal); > > > > > > if (smc->dr_lmb_enabled) { > > > @@ -1800,13 +1818,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) { > > > + Object *core = object_new(TYPE_SPAPR_CPU_CORE); > > > + > > > + object_property_set_str(core, machine->cpu_model, "cpu_model", > > > + &error_fatal); > > > + 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()) { > > > @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > > > 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; > > > @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > > > 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; > > > } > > > @@ -2384,6 +2425,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 > > > new file mode 100644 > > > index 0000000..8c6d71d > > > --- /dev/null > > > +++ b/hw/ppc/spapr_cpu_core.c > > > @@ -0,0 +1,199 @@ > > > +/* > > > + * sPAPR CPU core device, acts as container of CPU thread devices. > > > + * > > > + * Copyright (C) 2016 Bharata B Rao > > > + * > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > > > + * See the COPYING file in the top-level directory. > > > + */ > > > +#include "hw/cpu/core.h" > > > +#include "hw/ppc/spapr_cpu_core.h" > > > +#include "hw/ppc/spapr.h" > > > +#include "hw/boards.h" > > > +#include "qemu/error-report.h" > > > +#include "qapi/visitor.h" > > > +#include > > > +#include "target-ppc/kvm_ppc.h" > > > + > > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int threads, > > > + Error **errp) > > > +{ > > > + int i; > > > + Error *local_err = NULL; > > > + > > > + for (i = 0; i < threads; i++) { > > > + char id[32]; > > > + > > > + object_initialize(&core->threads[i], sizeof(core->threads[i]), > > > + object_class_get_name(core->oc)); > > > + snprintf(id, sizeof(id), "thread[%d]", i); > > > + object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]), > > > + &local_err); > > > + if (local_err) { > > > + goto err; > > > + } > > > + } > > > + return; > > > + > > > +err: > > > + while (--i) { > > > + object_unparent(OBJECT(&core->threads[i])); > > > + } > > > + error_propagate(errp, local_err); > > > +} > > > + > > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque) > > > +{ > > > + Error **errp = opaque; > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > > + CPUState *cs = CPU(child); > > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > > + > > > + object_property_set_bool(child, true, "realized", errp); > > > + if (*errp) { > > > + return 1; > > > + } > > > + > > > + spapr_cpu_init(spapr, cpu, errp); > > > + if (*errp) { > > > + return 1; > > > + } > > > + > > > + spapr_cpu_reset(cpu); > > should it be move to spapr_cpu_init() ? > > Could be moved. > > > > > > + return 0; > > > +} > > > + > > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > +{ > > > + sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev)); > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > > + int spapr_max_cores = max_cpus / smp_threads; > > this should be in machine code > > With ->pre_plug(), won't have this here. > > > > > > + Error *local_err = NULL; > > > + int threads = 0; > > > + int core_dt_id, core_id; > > > + int smt = kvmppc_smt_threads(); > > > + > > > + threads = object_property_get_int(OBJECT(dev), "threads", &local_err); > > since spapr_core is inherited from cpu-core, > > you can cast to cpu-core and use fields directly here instead of using > > property setters. > > Ah ok. > > > > > > + if (local_err) { > > > + goto out; > > > + } > > > + > > > + if (threads != smp_threads) { > > > + error_setg(&local_err, "threads must be %d", smp_threads); > > > + goto out; > > > + } > > move to machine handler > > Ok, guess you mean pre_plug handler ? > > > > > > + > > > + if (!core->oc) { > > > + error_setg(&local_err, "cpu_model property isn't set"); > > > + goto out; > > > + } > > > + > > > + core_dt_id = object_property_get_int(OBJECT(dev), "core", &local_err); > > > + if (local_err) { > > > + goto out; > > > + } > > > + > > > + if (core_dt_id % smt) { > > > + error_setg(&local_err, "invalid core id %d\n", core_dt_id); > > > + goto out; > > > + } > > > + > > > + core_id = core_dt_id / smt; > > > + if (core_id < 0 || core_id >= spapr_max_cores) { > > > + error_setg(&local_err, "core id %d out of range", core_dt_id); > > > + goto out; > > > + } > > maybe due to nameing it's a bit confusing, > > what's difference between core_id and core_dt_id? > > core_dt_id is the device tree IDs that we use with PowerPC cores. This is > what we use with "core" property of CPU_CORE. Since core_dt_id doesn't > grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8 host), > I am translating that to contiguous integer core_id so that I can > store the pointer of the realized core in the appropriate slot of > spapr->cpu_cores[] array. So, I see why the distinction is there, but it is kinda confusing. I'm wondering if we could do away with the spapr->cores array entirely and instead just access the core objects via the QOM tree - QOM "arrays" (i.e. properties named like foo[NNN]) can be sparse, so there's no need to allocate dense ids. Alternatively renaming "core_id" to "index" would be marginally less confusing. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson