From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59309) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaKfo-0003Yn-IF for qemu-devel@nongnu.org; Mon, 29 Feb 2016 05:03:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aaKfl-00014R-9n for qemu-devel@nongnu.org; Mon, 29 Feb 2016 05:03:28 -0500 Date: Mon, 29 Feb 2016 11:03:16 +0100 From: Igor Mammedov Message-ID: <20160229110316.02ab92d5@nial.brq.redhat.com> In-Reply-To: <20160229055018.GE5756@in.ibm.com> References: <1456417362-20652-1-git-send-email-bharata@linux.vnet.ibm.com> <1456417362-20652-3-git-send-email-bharata@linux.vnet.ibm.com> <20160226181339.5592.52630@loki> <20160229055018.GE5756@in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH v0 2/6] spapr: CPU core device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bharata B Rao Cc: mjrosato@linux.vnet.ibm.com, agraf@suse.de, thuth@redhat.com, pkrempa@redhat.com, ehabkost@redhat.com, borntraeger@de.ibm.com, qemu-devel@nongnu.org, armbru@redhat.com, Michael Roth , aik@ozlabs.ru, qemu-ppc@nongnu.org, pbonzini@redhat.com, afaerber@suse.de, david@gibson.dropbear.id.au On Mon, 29 Feb 2016 11:20:19 +0530 Bharata B Rao wrote: > 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... > > Correct. > > > > > 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? > > Yes makes sense. In fact David has often times said that generic > properties/routines should be pushed to base class wherever possible. > > I didn't do that in this first iteration to keep the generic changes > as minimum as possible, but yes slot should be a property of the > base class of core or socket. Then what will happen to slot if there isn't any core/socket device to query it, i.e. cpu hasn't been plugged in yet? To me slot looks like a machine belonged feature. > > Regards, > Bharata. >