Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 14254 bytes --]

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 <bharata@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 <bharata@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 <bharata@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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]