From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38101) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afl3g-0007tC-Eh for qemu-devel@nongnu.org; Tue, 15 Mar 2016 05:14:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afl3c-0002Q3-16 for qemu-devel@nongnu.org; Tue, 15 Mar 2016 05:14:31 -0400 Received: from e28smtp04.in.ibm.com ([125.16.236.4]:50499) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afl3b-0002Pl-Bs for qemu-devel@nongnu.org; Tue, 15 Mar 2016 05:14:27 -0400 Received: from localhost by e28smtp04.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 15 Mar 2016 14:44:23 +0530 Date: Tue, 15 Mar 2016 14:44:01 +0530 From: Bharata B Rao Message-ID: <20160315091401.GB13176@in.ibm.com> References: <1457672078-17307-1-git-send-email-bharata@linux.vnet.ibm.com> <1457672078-17307-7-git-send-email-bharata@linux.vnet.ibm.com> <20160314112523.1c43461f@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160314112523.1c43461f@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: mjrosato@linux.vnet.ibm.com, agraf@suse.de, thuth@redhat.com, pkrempa@redhat.com, ehabkost@redhat.com, aik@ozlabs.ru, armbru@redhat.com, qemu-devel@nongnu.org, borntraeger@de.ibm.com, qemu-ppc@nongnu.org, pbonzini@redhat.com, mdroth@linux.vnet.ibm.com, afaerber@suse.de, david@gibson.dropbear.id.au 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. Regards, Bharata.