From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59634) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZB9O-00057m-8a for qemu-devel@nongnu.org; Fri, 26 Feb 2016 00:41:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aZB9K-00012d-W3 for qemu-devel@nongnu.org; Fri, 26 Feb 2016 00:41:14 -0500 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:42050) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZB9J-0000zC-TC for qemu-devel@nongnu.org; Fri, 26 Feb 2016 00:41:10 -0500 Received: from localhost by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 26 Feb 2016 15:41:06 +1000 Date: Fri, 26 Feb 2016 11:09:37 +0530 From: Bharata B Rao Message-ID: <20160226053937.GA10503@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> <20160226025744.GE20657@voom.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160226025744.GE20657@voom.fritz.box> Subject: Re: [Qemu-devel] [RFC PATCH v0 2/6] spapr: CPU core device Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: mjrosato@linux.vnet.ibm.com, agraf@suse.de, thuth@redhat.com, pkrempa@redhat.com, ehabkost@redhat.com, aik@ozlabs.ru, qemu-devel@nongnu.org, armbru@redhat.com, borntraeger@de.ibm.com, qemu-ppc@nongnu.org, pbonzini@redhat.com, imammedo@redhat.com, afaerber@suse.de, mdroth@linux.vnet.ibm.com On Fri, Feb 26, 2016 at 01:57:44PM +1100, David Gibson wrote: > On Thu, Feb 25, 2016 at 09:52:38PM +0530, Bharata B Rao 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. > > > > Signed-off-by: Bharata B Rao > > Looks good overall, a few minor points below. > > > --- > > 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); > > I think you want to use cpu_class_by_name() rather than explicitly > constructing the class name here. Good point, started using ObjectClass. > > > + 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; > > + } > > I'm not sure if this is the right place to test this. Possibly it > should be tested in the machine hotplug handler instead. Not sure about the right place to validate this and nr_threads property. But it essentially boils down to backing out from property setter after object init vs backing out during realization. Regards, Bharata.