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

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

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.

Regards,
Bharata.