From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60124) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFYcj-0006DM-Q7 for qemu-devel@nongnu.org; Tue, 21 Jun 2016 23:14:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFYcf-0003tK-JC for qemu-devel@nongnu.org; Tue, 21 Jun 2016 23:14:41 -0400 Date: Wed, 22 Jun 2016 13:03:44 +1000 From: David Gibson Message-ID: <20160622030344.GL17957@voom.fritz.box> References: <1466238846-21365-1-git-send-email-bharata@linux.vnet.ibm.com> <20160621051000.GD14861@voom.fritz.box> <20160622023650.GD5613@in.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="5LiOUhUlsRX0HDkW" Content-Disposition: inline In-Reply-To: <20160622023650.GD5613@in.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 --5LiOUhUlsRX0HDkW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 22, 2016 at 08:06:50AM +0530, Bharata B Rao wrote: > On Tue, Jun 21, 2016 at 03:10:00PM +1000, David Gibson wrote: > > 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,c= ore-id=3D24 > > >=20 > > > Signed-off-by: Bharata B Rao > > > --- > > > Applies on ppc-for-2.7 branch of David Gibson's tree. > >=20 > > The implementation looks ok apart from a few nits noted below. > >=20 > > 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 > > >=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 *machin= e) > > > if (i < spapr_cores) { > > > char *type =3D spapr_get_cpu_core_type(machine->cpu_= model); > > > Object *core; > > > + char *compat; > > > =20 > > > if (!object_class_by_name(type)) { > > > error_report("Unable to find sPAPR CPU Core defi= nition"); > > > @@ -1818,6 +1819,13 @@ static void ppc_spapr_init(MachineState *machi= ne) > > > &error_fatal); > > > object_property_set_int(core, core_dt_id, CPU_CORE_P= ROP_CORE_ID, > > > &error_fatal); > > > + compat =3D spapr_get_cpu_compat_type(machine->cpu_mo= del); > > > + if (compat) { > > > + object_property_set_str(core, compat, "compat", > > > + &error_fatal); > > > + g_free(compat); > > > + } > > > + > > > object_property_set_bool(core, true, "realized", &er= ror_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 *hotplu= g_dev, DeviceState *dev, > > > CPUCore *cc =3D CPU_CORE(dev); > > > char *base_core_type =3D spapr_get_cpu_core_type(machine->cpu_mo= del); > > > 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_co= re_type); > > > goto out; > > > } > > > =20 > > > + base_compat_type =3D spapr_get_cpu_compat_type(machine->cpu_mode= l); > >=20 > > This can go in the initializer to match the base_core_type. >=20 > Had it that way, but since there was an error exit possibility when > base_core_type is not matching, I thought better to initialize base_compa= t_type > after that check. Ah, good point. I forgot that get_cpu_compat_type() had an implicit allocation that needs cleanup. > > > @@ -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", comp= at); > > > + > > > + cc->parse_features(CPU(obj), featurestr, &local_err); > >=20 > > Hmm.. would it make more sense to just do an object_property_set() > > rather than calling into parse_features? >=20 > It would work, but I guess better to use ->parse_features() to ensure > future additional properties would work seamlessly. Hmm.. except that the string you're forming to pass to parse_features is always just "compat=3D%s". So you'd have to change that bit anyway, so I don't see that using parse_features really buys you anything. --=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 --5LiOUhUlsRX0HDkW Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXagAQAAoJEGw4ysog2bOS64sP/jIvDpt9b8FIIKucUSo1FTP2 AMNgDPih/PENySwxees57FmJdxYssTIaF9n+y8P91Dno4pkzZYA8EVbqFLMqXCP2 NIqX5NXc6mXHj7CyeVX7+OCkkre+Nv+dmaE0w/H8ep5QGjI29673oGcJRx3KvhU/ Y0/r4INyhTr7+oUQ51LhgLGkY8S9HDs9aoJCx4Limuv8AZp2Edc5wXMNVrdb2UbE jPyHY0l35vYPyml1KgrVJgdAeIhnIYRpWiQwq7t9AexT7XGQq+ZCji90TFH5du0B 1fA7Mm7jwJBknwqI08BtixVbi2DxECMYuJ2w5lIsVWj0Lr9MD1qHtJ+tv8bzR5oV DnigdKAD7WDS9BBVhKSGPnsQY/AUxqMGXe4NINkESsJbZiTEoTIKLO45yMASVhqP FMcOo/BmS2Tf1G3MuTGvlK4S6vg1M65Pq7ZSLQx2vEhwYbWJW5YOiscKqY7r/qgB HY/NJDS4FiMra/Kz/tbE5kTmmXrqwd+68P6cOWY8UksVVS4qzQgotaS0bkzFkhfZ FS5otlfD1D0ib3hCcM5TK/tpNM2fQngkZtb2iSamj52qxUOrbP8G5ducH031jLvK +75KK9LIRz76+Ddd2ncj7jhzxokw7M9EraV7EpB1bZ4DSs4K8S2V1TYJt+RHYEEt +r/RmKDuHU4ech7iiPSl =6NYU -----END PGP SIGNATURE----- --5LiOUhUlsRX0HDkW--