From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35750) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8hgw-0005ga-Rp for qemu-devel@nongnu.org; Fri, 03 Jun 2016 01:30:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8hgt-0003ET-Jl for qemu-devel@nongnu.org; Fri, 03 Jun 2016 01:30:42 -0400 Date: Fri, 3 Jun 2016 15:25:08 +1000 From: David Gibson Message-ID: <20160603052508.GJ1087@voom.fritz.box> References: <1463024905-28401-1-git-send-email-bharata@linux.vnet.ibm.com> <1463024905-28401-8-git-send-email-bharata@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="GvznHscUikHnwW2p" Content-Disposition: inline In-Reply-To: <1463024905-28401-8-git-send-email-bharata@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [for-2.7 PATCH v3 07/15] spapr: Abstract CPU core device and type specific core devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bharata B Rao Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, afaerber@suse.de, imammedo@redhat.com, armbru@redhat.com, thuth@redhat.com, aik@ozlabs.ru, agraf@suse.de, pbonzini@redhat.com, ehabkost@redhat.com, pkrempa@redhat.com, mdroth@linux.vnet.ibm.com, eblake@redhat.com, mjrosato@linux.vnet.ibm.com, borntraeger@de.ibm.com --GvznHscUikHnwW2p Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, May 12, 2016 at 09:18:17AM +0530, Bharata B Rao wrote: > Add sPAPR specific abastract CPU core device that is based on generic > CPU core device. Use this as base type to create sPAPR CPU specific core > devices. >=20 > TODO: > - Add core types for other remaining CPU types > - Handle CPU model alias correctly >=20 > Signed-off-by: Bharata B Rao This is looking pretty cood, but there's some minor changes I'd like to see. > --- > hw/ppc/Makefile.objs | 1 + > hw/ppc/spapr.c | 3 +- > hw/ppc/spapr_cpu_core.c | 168 ++++++++++++++++++++++++++++++++++= ++++++ > include/hw/ppc/spapr.h | 1 + > include/hw/ppc/spapr_cpu_core.h | 28 +++++++ > 5 files changed, 199 insertions(+), 2 deletions(-) > 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_rng= =2Eo > +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.c b/hw/ppc/spapr.c > index b69995e..95db047 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1605,8 +1605,7 @@ static void spapr_boot_set(void *opaque, const char= *boot_device, > machine->boot_order =3D g_strdup(boot_device); > } > =20 > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, > - Error **errp) > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **e= rrp) I think this function should actually move into spapr_cpu_core.c > { > CPUPPCState *env =3D &cpu->env; > =20 > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > new file mode 100644 > index 0000000..af63ed9 > --- /dev/null > +++ b/hw/ppc/spapr_cpu_core.c > @@ -0,0 +1,168 @@ > +/* > + * 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 la= ter. > + * 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 "qapi/error.h" > +#include > +#include "target-ppc/kvm_ppc.h" > + > +static void spapr_cpu_core_create_threads(DeviceState *dev, int threads, > + Error **errp) This function could be folded into spapr_cpu_core_realize(), that's the only caller and they're both fairly short. > +{ > + int i; > + Error *local_err =3D NULL; > + sPAPRCPUCore *core =3D SPAPR_CPU_CORE(OBJECT(dev)); > + > + for (i =3D 0; i < threads; i++) { > + char id[32]; > + > + object_initialize(&core->threads[i], sizeof(core->threads[i]), > + object_class_get_name(core->cpu)); Given we have to go from the class pointer to the class name to actually use it here, maybe we would be better off storing the name rather than a class pointer. Up to you, I'm happy either way. > + snprintf(id, sizeof(id), "thread[%d]", i); > + object_property_add_child(OBJECT(core), id, OBJECT(&core->thread= s[i]), > + &local_err); > + if (local_err) { > + goto err; > + } > + } > + return; > + > +err: > + while (--i) { > + object_unparent(OBJECT(&core->threads[i])); Is this safe if some of the threads haven't been initialized? > + } > + error_propagate(errp, local_err); > +} > + > +static int spapr_cpu_core_realize_child(Object *child, void *opaque) > +{ > + Error **errp =3D opaque; > + sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > + CPUState *cs =3D CPU(child); > + PowerPCCPU *cpu =3D 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; > + } > + return 0; > +} > + > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > +{ > + sPAPRCPUCore *sc =3D SPAPR_CPU_CORE(OBJECT(dev)); > + CPUCore *cc =3D CPU_CORE(OBJECT(dev)); > + Error *local_err =3D NULL; > + > + sc->threads =3D g_new0(PowerPCCPU, cc->threads); This isn't quite safe, because it assume the structure size for all the threads is that of PowerPCCPU. That's true now, but cpu thread subtypes could in theory extend that structure with subtype specific fields. I think we need to actually look up the class instance size here. > + spapr_cpu_core_create_threads(dev, cc->threads, &local_err); > + if (local_err) { > + goto out; > + } > + object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &loc= al_err); > + > +out: > + if (local_err) { > + g_free(sc->threads); > + error_propagate(errp, local_err); > + } > +} > + > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(oc); > + dc->realize =3D spapr_cpu_core_realize; > +} > + > +/* > + * instance_init routines from different flavours of sPAPR CPU cores. > + * TODO: Add support for 'host' core type. > + */ > +#define SPAPR_CPU_CORE_INITFN(_type, _fname) \ > +static void glue(glue(spapr_cpu_core_, _fname), _initfn(Object *obj)) \ > +{ \ > + sPAPRCPUCore *core =3D SPAPR_CPU_CORE(obj); \ > + char *name =3D g_strdup_printf("%s-" TYPE_POWERPC_CPU, stringify(_ty= pe)); \ > + ObjectClass *oc =3D object_class_by_name(name); \ > + g_assert(oc); \ > + g_free((void *)name); \ > + core->cpu =3D oc; \ This all looks like stuff which could be done at class_init time, but this is an instance_init function. I'm also wondering if it might just be simpler to store _type in the subclass and do this in the base class. > +} > + > +SPAPR_CPU_CORE_INITFN(POWER7_v2.3, POWER7); > +SPAPR_CPU_CORE_INITFN(POWER7+_v2.1, POWER7plus); > +SPAPR_CPU_CORE_INITFN(POWER8_v2.0, POWER8); > +SPAPR_CPU_CORE_INITFN(POWER8E_v2.1, POWER8E); > + > +typedef struct SPAPRCoreInfo { > + const char *name; > + void (*initfn)(Object *obj); > +} SPAPRCoreInfo; > + > +static const SPAPRCoreInfo spapr_cores[] =3D { > + /* POWER7 and aliases */ > + { .name =3D "POWER7_v2.3", .initfn =3D spapr_cpu_core_POWER7_initfn = }, > + { .name =3D "POWER7", .initfn =3D spapr_cpu_core_POWER7_initfn }, > + > + /* POWER7+ and aliases */ > + { .name =3D "POWER7+_v2.1", .initfn =3D spapr_cpu_core_POWER7plus_in= itfn }, > + { .name =3D "POWER7+", .initfn =3D spapr_cpu_core_POWER7plus_initfn = }, > + > + /* POWER8 and aliases */ > + { .name =3D "POWER8_v2.0", .initfn =3D spapr_cpu_core_POWER8_initfn = }, > + { .name =3D "POWER8", .initfn =3D spapr_cpu_core_POWER8_initfn }, > + { .name =3D "power8", .initfn =3D spapr_cpu_core_POWER8_initfn }, > + > + /* POWER8E and aliases */ > + { .name =3D "POWER8E_v2.1", .initfn =3D spapr_cpu_core_POWER8E_initf= n }, > + { .name =3D "POWER8E", .initfn =3D spapr_cpu_core_POWER8E_initfn }, > + > + { .name =3D NULL } > +}; > + > +static void spapr_cpu_core_register(const SPAPRCoreInfo *info) > +{ > + TypeInfo type_info =3D { > + .parent =3D TYPE_SPAPR_CPU_CORE, > + .instance_size =3D sizeof(sPAPRCPUCore), > + .instance_init =3D info->initfn, > + }; > + > + type_info.name =3D g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, info->= name); > + type_register(&type_info); > + g_free((void *)type_info.name); > +} The way the above registration works is pretty clunky. I'm not immediately sure how to make it nicer though, so I'm happy enough to merge it this way and hope we can clean up later. > + > +static const TypeInfo spapr_cpu_core_type_info =3D { > + .name =3D TYPE_SPAPR_CPU_CORE, > + .parent =3D TYPE_CPU_CORE, > + .abstract =3D true, > + .instance_size =3D sizeof(sPAPRCPUCore), > + .class_init =3D spapr_cpu_core_class_init, > +}; > + > +static void spapr_cpu_core_register_types(void) > +{ > + const SPAPRCoreInfo *info =3D spapr_cores; > + > + type_register_static(&spapr_cpu_core_type_info); > + while (info->name) { > + spapr_cpu_core_register(info); > + info++; > + } > +} > + > +type_init(spapr_cpu_core_register_types) > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 815d5ee..bcd9de6 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -580,6 +580,7 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorT= ype drc_type, > uint32_t count); > void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type, > uint32_t count); > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **e= rrp); > =20 > /* rtas-configure-connector state */ > struct sPAPRConfigureConnectorState { > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_c= ore.h > new file mode 100644 > index 0000000..b6aa39e > --- /dev/null > +++ b/include/hw/ppc/spapr_cpu_core.h > @@ -0,0 +1,28 @@ > +/* > + * sPAPR CPU core device. > + * > + * Copyright (C) 2016 Bharata B Rao > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or la= ter. > + * See the COPYING file in the top-level directory. > + */ > +#ifndef HW_SPAPR_CPU_CORE_H > +#define HW_SPAPR_CPU_CORE_H > + > +#include "hw/qdev.h" > +#include "hw/cpu/core.h" > + > +#define TYPE_SPAPR_CPU_CORE "spapr-cpu-core" > +#define SPAPR_CPU_CORE(obj) \ > + OBJECT_CHECK(sPAPRCPUCore, (obj), TYPE_SPAPR_CPU_CORE) > + > +typedef struct sPAPRCPUCore { > + /*< private >*/ > + CPUCore parent_obj; > + > + /*< public >*/ > + PowerPCCPU *threads; > + ObjectClass *cpu; I'd prefer to see this called 'cpu_class' or something - seeing 'cpu' makes me think an instance rather than a class. > +} sPAPRCPUCore; > + > +#endif --=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 --GvznHscUikHnwW2p Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXURS0AAoJEGw4ysog2bOSQIQP/Rz9R4/veBRJUyKFlFKDTSD9 BeiwbHQhx28+3HF8bGGD6fktJsVj4mR+MegL48ZVaDOR8dxvw6wJiUNRs2N8nUbe OnpFhr3w07pjPa9U6blXRu+4lMkmjkz9AHMA3wLQSe2siJLGj2GX+0iEndtkQOGB 1Ds87nUaCDmMzUz+qe+n+eGIct4agAh5ncqRYVjSXQdS95gqlx9E501lTRdeK2Ub R3MXvaZe05IGtF85K3cqj3ycmT+PjrGpKr3nZOCZtP4SSAlWO4uYpHqzaOLuBve9 tBbMwaHI6MaHt4ZPivRFdlgiheQ6D04Yaan4+H5MC2ce4UCOldS0IixP8FhTeREw zaKNYNUnu16tnwGVdxCRTFn1x3dJVO/gGU6RewTHyAQz2v8vHowEWiRjcSa9MIGg rIXeW4ra4EcB3LTssNUHpzBP30zW52SvTiljgebvWpXVVWotVKxsddR8vBdPzYEo e3oNRkTp4As0dHBogcy+u3QwsqTXomJHV2re8rg6LTTuX0wurwxZGJHR744S+zKB RkXGwvLPLLvsN4v3jV1WN2xwWeZXRXnFtGe3WWIGjomzwepiWIv8sTwvaUKUHxNg QBGpPWLzShZUvh1pcOWEfiwNP+UscUa/8wtFSjFF2Gp+NNtSKLCsykLmJn+ek1nS jn07GrXxzJAYqYLKYwYW =vDOr -----END PGP SIGNATURE----- --GvznHscUikHnwW2p--