On Fri, Feb 26, 2016 at 12:13:39PM -0600, Michael Roth wrote: > Quoting Bharata B Rao (2016-02-25 10:22:38) > > 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. > > > > Signed-off-by: Bharata B Rao > > --- > > hw/ppc/Makefile.objs | 1 + > > hw/ppc/spapr_cpu_core.c | 210 ++++++++++++++++++++++++++++++++++++++++ > > include/hw/ppc/spapr_cpu_core.h | 32 ++++++ > > 3 files changed, 243 insertions(+) > > 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_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > new file mode 100644 > > index 0000000..c44eb61 > > --- /dev/null > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -0,0 +1,210 @@ > > +/* > > + * 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 > > + > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque) > > +{ > > + Error **errp = opaque; > > + > > + object_property_set_bool(child, true, "realized", errp); > > + if (*errp) { > > + return 1; > > + } > > + 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()); > > + Error *local_err = NULL; > > + > > + if (!core->nr_threads) { > > + error_setg(errp, "nr_threads property can't be 0"); > > + return; > > + } > > + > > + if (!core->cpu_model) { > > + error_setg(errp, "cpu_model property isn't set"); > > + return; > > + } > > + > > + /* > > + * TODO: If slot isn't specified, plug this core into > > + * an existing empty slot. > > + */ > > + if (!core->slot) { > > + error_setg(errp, "slot property isn't set"); > > + return; > > + } > > + > > + object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot, > > + &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, errp); > > +} > > + > > +/* > > + * This creates the CPU threads for a given @core. > > + * > > + * In order to create the threads, we need two inputs - number of > > + * threads and the cpu_model. These are set as core object's properties. > > + * When both of them become available/set, this routine will be called from > > + * either property's set handler to create the threads. > > + * > > + * TODO: Dependence of threads creation on two properties is resulting > > + * in this not-so-clean way of creating threads from either of the > > + * property setters based on the order in which they get set. Check if > > + * this can be handled in a better manner. > > + */ > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error **errp) > > +{ > > + int i; > > + > > + for (i = 0; i < core->nr_threads; i++) { > > + char id[32]; > > + char type[32]; > > + > > + snprintf(type, sizeof(type), "%s-%s", core->cpu_model, > > + TYPE_POWERPC_CPU); > > + object_initialize(&core->threads[i], sizeof(core->threads[i]), type); > > + > > + snprintf(id, sizeof(id), "thread[%d]", i); > > + object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]), > > + errp); > > + } > > +} > > + > > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp) > > +{ > > + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); > > + > > + return core->slot; > > +} > > + > > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val, > > + Error **errp) > > +{ > > + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); > > + > > + core->slot = g_strdup(val); > > +} > > + > > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **errp) > > +{ > > + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); > > + > > + return core->cpu_model; > > +} > > + > > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char *val, > > + Error **errp) > > +{ > > + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); > > + MachineState *machine = MACHINE(qdev_get_machine()); > > + > > + /* > > + * cpu_model can't be different from what is specified with -cpu > > + */ > > + if (strcmp(val, machine->cpu_model)) { > > + error_setg(errp, "cpu_model should be %s", machine->cpu_model); > > + return; > > + } > > + > > + core->cpu_model = g_strdup(val); > > + if (core->nr_threads && core->cpu_model) { > > + spapr_cpu_core_create_threads(core, errp); > > + } > > +} > > + > > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v, > > + const char *name, void *opaque, > > + Error **errp) > > +{ > > + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); > > + int64_t value = core->nr_threads; > > + > > + visit_type_int(v, name, &value, errp); > > +} > > + > > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v, > > + const char *name, void *opaque, > > + Error **errp) > > +{ > > + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); > > + Error *local_err = NULL; > > + int64_t value; > > + > > + visit_type_int(v, name, &value, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + /* Allow only homogeneous configuration */ > > + if (value != smp_threads) { > > + error_setg(errp, "nr_threads should be %d", smp_threads); > > + return; > > + } > > + > > + core->nr_threads = value; > > + core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU)); > > + > > + if (core->nr_threads && core->cpu_model) { > > + spapr_cpu_core_create_threads(core, errp); > > + } > > +} > > + > > +static void spapr_cpu_core_instance_init(Object *obj) > > +{ > > + object_property_add(obj, "nr_threads", "int", > > + spapr_cpu_core_prop_get_nr_threads, > > + spapr_cpu_core_prop_set_nr_threads, > > + NULL, NULL, NULL); > > + object_property_add_str(obj, "cpu_model", > > + spapr_cpu_core_prop_get_cpu_model, > > + spapr_cpu_core_prop_set_cpu_model, > > + NULL); > > + object_property_add_str(obj, "slot", > > + spapr_cpu_core_prop_get_slot, > > + spapr_cpu_core_prop_set_slot, > > + NULL); > > "slot" seems intended to be a machine-agnostic of mapping device > types discovered from qmp_query_cpu_slots() to an appropriate > "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE. > It seems like maybe TYPE_CPU_CORE is a better place, but then on > x86 I suppose it might be TYPE_CPU_SOCKET or something instead... > > It almost seems like a TYPE_INTERFACE_SLOTABLE would be the > right approach, but I don't know how we could expose that as > a property. I guess it's somewhat implied that this "interface" > exists if qmp_query_cpu_slots() returns the type, but I wonder > if something a bit more formal should be modeled to make the > implementation requirements a bit clearer. > > Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot > class method, expose them via "slot" property, then have the > defaults generate "not implemented" errors? Yeah, TBH the "slot" stuff has me a bit confused still. I'm intending to have a closer look at it once the more obvious cleanups are addressed. -- 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