[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device |
Date: |
Tue, 15 Mar 2016 20:34:28 +1100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden> 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 <address@hidden>
> > > ---
> > > 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 <address@hidden>
> > > + *
> > > + * 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.
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
signature.asc
Description: PGP signature
[Qemu-devel] [RFC PATCH v2 7/9] spapr: CPU hotplug support, Bharata B Rao, 2016/03/10
[Qemu-devel] [RFC PATCH v2 8/9] xics, xics_kvm: Handle CPU unplug correctly, Bharata B Rao, 2016/03/10
[Qemu-devel] [RFC PATCH v2 9/9] spapr: CPU hot unplug support, Bharata B Rao, 2016/03/10