On Tue, 15 Mar 2016 20:34:28 +1100 David Gibson <da...@gibson.dropbear.id.au> wrote:
> 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 <bhar...@linux.vnet.ibm.com> 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 <bhar...@linux.vnet.ibm.com> > > > > --- > > > > 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 <libfdt.h> > > > > > > > > @@ -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 <bhar...@linux.vnet.ibm.com> > > > > + * > > > > + * 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 <sysemu/cpus.h> > > > > +#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. Wouldn't be lookups for duplicate in QOM tree take O(N^2) when hot-plugging N cpus? It should be less with sorted array at least. > Alternatively renaming "core_id" to "index" would be marginally less > confusing. >