From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55927) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFENZ-00019K-Cf for qemu-devel@nongnu.org; Tue, 21 Jun 2016 01:37:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFENW-0001dU-H5 for qemu-devel@nongnu.org; Tue, 21 Jun 2016 01:37:40 -0400 Date: Tue, 21 Jun 2016 15:10:00 +1000 From: David Gibson Message-ID: <20160621051000.GD14861@voom.fritz.box> References: <1466238846-21365-1-git-send-email-bharata@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="gE7i1rD7pdK0Ng3j" Content-Disposition: inline In-Reply-To: <1466238846-21365-1-git-send-email-bharata@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores 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, imammedo@redhat.com, thuth@redhat.com --gE7i1rD7pdK0Ng3j Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jun 18, 2016 at 02:04:06PM +0530, Bharata B Rao wrote: > Compat CPU type is typically specified on -cpu cmdline option like: > -cpu host,compat=3Dpower7 or -cpu POWER8E,compat=3Dpower7 etc. > With the introduction of sPAPR CPU core devices, we need to support > the same for core devices too. >=20 > Support the specification of CPU compat type on device_add command for > sPAPRCPUCore devices like: > (qemu) device_add POWER8E-spapr-cpu-core,id=3Dcore3,compat=3Dpower7,core-= id=3D24 >=20 > Signed-off-by: Bharata B Rao > --- > Applies on ppc-for-2.7 branch of David Gibson's tree. The implementation looks ok apart from a few nits noted below. There's a larger problem here, though, in that this doesn't advertise the necessary compat=3D property via query-hotpluggable-cpus qmp and hmp interfaces. Which means that management has no good way of knowing it's necessary. >=20 > hw/ppc/spapr.c | 8 +++++ > hw/ppc/spapr_cpu_core.c | 73 +++++++++++++++++++++++++++++++++++= ++++++ > include/hw/ppc/spapr_cpu_core.h | 2 ++ > 3 files changed, 83 insertions(+) >=20 > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 778fa25..2049d7d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState *machine) > if (i < spapr_cores) { > char *type =3D spapr_get_cpu_core_type(machine->cpu_mode= l); > Object *core; > + char *compat; > =20 > if (!object_class_by_name(type)) { > error_report("Unable to find sPAPR CPU Core definiti= on"); > @@ -1818,6 +1819,13 @@ static void ppc_spapr_init(MachineState *machine) > &error_fatal); > object_property_set_int(core, core_dt_id, CPU_CORE_PROP_= CORE_ID, > &error_fatal); > + compat =3D spapr_get_cpu_compat_type(machine->cpu_model); > + if (compat) { > + object_property_set_str(core, compat, "compat", > + &error_fatal); > + g_free(compat); > + } > + > object_property_set_bool(core, true, "realized", &error_= fatal); > } > } > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index 3a5da09..9eb63cc 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model) > return core_type; > } > =20 > +/* > + * Returns the CPU compat type specified in -cpu @model. > + */ > +char *spapr_get_cpu_compat_type(const char *model) > +{ > + char *compat_type =3D NULL; > + gchar **model_pieces =3D g_strsplit(model, ",", 2); > + > + if (model_pieces[1]) { > + gchar **compat_pieces =3D g_strsplit(model_pieces[1], "=3D", 2); > + > + compat_type =3D g_strdup_printf("%s", compat_pieces[1]); > + } > + > + g_strfreev(model_pieces); > + return compat_type; > +} > + > static void spapr_core_release(DeviceState *dev, void *opaque) > { > sPAPRCPUCore *sc =3D SPAPR_CPU_CORE(OBJECT(dev)); > @@ -223,12 +241,31 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_de= v, DeviceState *dev, > CPUCore *cc =3D CPU_CORE(dev); > char *base_core_type =3D spapr_get_cpu_core_type(machine->cpu_model); > const char *type =3D object_get_typename(OBJECT(dev)); > + char *base_compat_type =3D NULL; > + char *compat =3D NULL; > + bool compat_set; > =20 > if (strcmp(base_core_type, type)) { > error_setg(&local_err, "CPU core type should be %s", base_core_t= ype); > goto out; > } > =20 > + base_compat_type =3D spapr_get_cpu_compat_type(machine->cpu_model); This can go in the initializer to match the base_core_type. > + compat =3D object_property_get_str(OBJECT(dev), "compat", NULL); > + compat_set =3D (compat && *compat) ? true : false; You don't need the ?:, the condition is already a boolean. > + > + if (base_compat_type) { > + if ((compat_set && strcmp(base_compat_type, compat)) || > + !compat_set) { > + error_setg(&local_err, "CPU compat type should be %s", > + base_compat_type); > + goto out; > + } > + } else if (compat_set) { > + error_setg(&local_err, "CPU compat type shouldn't be set"); > + goto out; > + } > + > if (!smc->dr_cpu_enabled && dev->hotplugged) { > error_setg(&local_err, "CPU hotplug not supported for this machi= ne"); > goto out; > @@ -256,6 +293,8 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev,= DeviceState *dev, > } > =20 > out: > + g_free(compat); > + g_free(base_compat_type); > g_free(base_core_type); > error_propagate(errp, local_err); > } > @@ -288,6 +327,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, = Error **errp) > Error *local_err =3D NULL; > Object *obj; > int i; > + char *compat =3D object_property_get_str(OBJECT(sc), "compat", NULL); > + bool compat_set =3D (compat && *compat) ? true : false; Again, don't need ?: here. > =20 > sc->threads =3D g_malloc0(size * cc->nr_threads); > for (i =3D 0; i < cc->nr_threads; i++) { > @@ -298,9 +339,19 @@ static void spapr_cpu_core_realize(DeviceState *dev,= Error **errp) > snprintf(id, sizeof(id), "thread[%d]", i); > object_property_add_child(OBJECT(sc), id, obj, &local_err); > if (local_err) { > + g_free(compat); > goto err; > } > + if (compat_set) { > + CPUClass *cc =3D CPU_GET_CLASS(CPU(obj)); > + char *featurestr =3D g_strdup_printf("compat=3D%s", compat); > + > + cc->parse_features(CPU(obj), featurestr, &local_err); Hmm.. would it make more sense to just do an object_property_set() rather than calling into parse_features? > + g_free(featurestr); > + } > } > + > + g_free(compat); > object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &loc= al_err); > if (local_err) { > goto err; > @@ -318,6 +369,27 @@ err: > error_propagate(errp, local_err); > } > =20 > +static char *spapr_cpu_core_prop_get_compat(Object *obj, Error **errp) > +{ > + sPAPRCPUCore *core =3D SPAPR_CPU_CORE(obj); > + > + return g_strdup(core->cpu_compat); > +} > + > +static void spapr_cpu_core_prop_set_compat(Object *obj, const char *val, > + Error **errp) > +{ > + sPAPRCPUCore *core =3D SPAPR_CPU_CORE(obj); > + > + core->cpu_compat =3D g_strdup(val); > +} > + > +static void spapr_cpu_core_instance_init(Object *obj) > +{ > + object_property_add_str(obj, "compat", spapr_cpu_core_prop_get_compa= t, > + spapr_cpu_core_prop_set_compat, NULL); > +} > + > static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc =3D DEVICE_CLASS(oc); > @@ -388,6 +460,7 @@ static const TypeInfo spapr_cpu_core_type_info =3D { > .parent =3D TYPE_CPU_CORE, > .abstract =3D true, > .instance_size =3D sizeof(sPAPRCPUCore), > + .instance_init =3D spapr_cpu_core_instance_init, > .class_init =3D spapr_cpu_core_class_init, > }; > =20 > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_c= ore.h > index 1c9b319..e697dab 100644 > --- a/include/hw/ppc/spapr_cpu_core.h > +++ b/include/hw/ppc/spapr_cpu_core.h > @@ -24,11 +24,13 @@ typedef struct sPAPRCPUCore { > /*< public >*/ > void *threads; > ObjectClass *cpu_class; > + char *cpu_compat; > } sPAPRCPUCore; > =20 > void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp); > char *spapr_get_cpu_core_type(const char *model); > +char *spapr_get_cpu_compat_type(const char *model); > void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp); > void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, --=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 --gE7i1rD7pdK0Ng3j Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXaMwoAAoJEGw4ysog2bOSdl4QAKajWCrr2aE0dqfyWWHOrN/l qOrRS4mTAj3uKkTsjHKE51tQs3JsT95x7qBfg7Tc5VgsQ1i4cNhl0uXlQIXJPydG uZfty9Ha/qrnQ62gS7Yt2xCbQclTR+f90HLiceSKqKWRzkQmWkaFzJnPjtr3XNhq cDBNF6rVg9MLWwATPwZ1mmtXO3PFSXcMaUQuII+kgtdG+jxQ3nuQfSTrGkXSpOCT hujNselJBB9P2SR5At8+ggYGgMtx9gTLx4BKwgXkjxZwKwP0bNdC75wmhzE1RUaP Mjxeckavv6CuCK3yFRI2XajT5MFvNolwY8OrrAE70vD1nzgfyYMH7A1b1PByOaWF Hw+VOiB0bJvQOb5/vqEgjpyBCvplPZeoudL3Im8UxOaiFKRZyJ+Dc16Rm0vWppkM Fh7UkexvrTB5ZhkDMAEeSGfZ0RXBtdxpQcn6GjmXbkhv0US3kHSoODkP+0/T2ykN S4TNhKcOg47YBlqzwabMKSlgugu3IaWXUTB7nktQCNn8GFBvk+Ny38c2Xv/q1XXX bDUn8GP2JI9z4gU7azy/6EvKt5gQpa8E56aZWcrjtOWClBXrR2zHBm6Usc66/gl4 4uPx3tXsBddDfSomwQCmS3o8ODWWOIBVn7YreRieZPSM29pfGlvQ7I8cAAxDengi bjnkkUmaET2ZrFvU4Fv0 =yvG8 -----END PGP SIGNATURE----- --gE7i1rD7pdK0Ng3j--