From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35124) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaFyE-0004Vw-R8 for qemu-devel@nongnu.org; Mon, 29 Feb 2016 00:02:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aaFyC-0000vu-IR for qemu-devel@nongnu.org; Mon, 29 Feb 2016 00:02:10 -0500 Date: Mon, 29 Feb 2016 14:44:22 +1100 From: David Gibson Message-ID: <20160229034422.GE5427@voom.redhat.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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="+JUInw4efm7IfTNU" Content-Disposition: inline In-Reply-To: <20160226181339.5592.52630@loki> 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: Michael Roth Cc: mjrosato@linux.vnet.ibm.com, agraf@suse.de, thuth@redhat.com, pkrempa@redhat.com, ehabkost@redhat.com, aik@ozlabs.ru, armbru@redhat.com, qemu-devel@nongnu.org, borntraeger@de.ibm.com, pbonzini@redhat.com, qemu-ppc@nongnu.org, Bharata B Rao , imammedo@redhat.com, afaerber@suse.de --+JUInw4efm7IfTNU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 de= vice. > > Creating this core device will result in creation of all the CPU thread > > devices that are part of this core. > >=20 > > 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 > >=20 > > 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 +=3D ppc.o ppc_booke.o > > obj-$(CONFIG_PSERIES) +=3D spapr.o spapr_vio.o spapr_events.o > > obj-$(CONFIG_PSERIES) +=3D spapr_hcall.o spapr_iommu.o spapr_rtas.o > > obj-$(CONFIG_PSERIES) +=3D spapr_pci.o spapr_rtc.o spapr_drc.o spapr_r= ng.o > > +obj-$(CONFIG_PSERIES) +=3D spapr_cpu_core.o > > ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy) > > obj-y +=3D 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 =3D 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 =3D SPAPR_CPU_CORE(OBJECT(dev)); > > + sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > > + Error *local_err =3D 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, er= rp); > > +} > > + > > +/* > > + * 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 propertie= s. > > + * 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 =3D 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->thre= ads[i]), > > + errp); > > + } > > +} > > + > > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp) > > +{ > > + sPAPRCPUCore *core =3D SPAPR_CPU_CORE(obj); > > + > > + return core->slot; > > +} > > + > > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val, > > + Error **errp) > > +{ > > + sPAPRCPUCore *core =3D SPAPR_CPU_CORE(obj); > > + > > + core->slot =3D g_strdup(val); > > +} > > + > > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error **er= rp) > > +{ > > + sPAPRCPUCore *core =3D 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 =3D SPAPR_CPU_CORE(obj); > > + MachineState *machine =3D 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 =3D 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 =3D SPAPR_CPU_CORE(obj); > > + int64_t value =3D 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 =3D SPAPR_CPU_CORE(obj); > > + Error *local_err =3D 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 !=3D smp_threads) { > > + error_setg(errp, "nr_threads should be %d", smp_threads); > > + return; > > + } > > + > > + core->nr_threads =3D value; > > + core->threads =3D 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); >=20 > "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... >=20 > 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. >=20 > 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. --=20 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 --+JUInw4efm7IfTNU Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW076WAAoJEGw4ysog2bOS1UoQANlYSZinqf+F1hRuWf+hHYwc WGH8sa+7YrzexupdKXeiG12YboEXB9QRtnqU5bhBbqKQPXx9qVuhiCGeg9YHx0sy A73eaV5ZPnuy6zoOzOTC3xHzbHafoG1U+iQGE87roO/frdA5rajTuGVbaBUhfZZv rP7Cm54rVTGCVv+Tgs83kdJGpSxh4q3T92QG63Qp0tMwUfCrtI3axy7tHha0+LsL JbExC9s/pRTTiaW8qGP8MtMpHMv7uHT6BE5NaLW5V59kMiuRz6JnI/jUCEF1IWnG 73N8beANtRri/fXgmAo7XQSmpZgLgxc/Vyp8rYA9y+Tav211VStJKKpwuF3KxmHM 4IMCZfxeb/Uo460PizDLjoJJm+OecNvg5SMff1l43pAh5ge0gTXyOC/rCK8xZ3hk fVsN5F3exoRVwt0a1sIkI+bIzwwDuAZ9+DL2L7D8pwvUVpRid3B2SxdzeNLU6XLa XqNfJY/e9lNXPEiHxAEciQhU3vwn0+C8eh2GYY2GKSLjVP/ppsAWjS6bD9AFO7ml mKn4/m7h4pYu7gPzTXMLM1cvuURnMJw3rLUvOxkDYPDitpQGj2kyoprqs/Qc3kgw iqbMYTkosCzHp3pgu8MII9wX2RE6gL1ad06Vg2sFWUbZbbiXVfYosr9DC9XXIqDw vEsj29FoKGppiSB/Uc3s =7EQh -----END PGP SIGNATURE----- --+JUInw4efm7IfTNU--