From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59736) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bJv1f-0005aK-8p for qemu-devel@nongnu.org; Sun, 03 Jul 2016 23:58:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bJv1b-0001a3-2J for qemu-devel@nongnu.org; Sun, 03 Jul 2016 23:58:26 -0400 Date: Mon, 4 Jul 2016 13:54:55 +1000 From: David Gibson Message-ID: <20160704035455.GE2919@voom.fritz.box> References: <146741287399.948.15988269239450224065.stgit@bahia.lan> <146741290890.948.9125710372347515263.stgit@bahia.lan> <20160702080622.GH21596@in.ibm.com> <20160702103333.55119e79@bahia.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="T6xhMxlHU34Bk0ad" Content-Disposition: inline In-Reply-To: <20160702103333.55119e79@bahia.lan> Subject: Re: [Qemu-devel] [PATCH v2 4/7] ppc: open code cpu creation for machine types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Bharata B Rao , Benjamin Herrenschmidt , qemu-devel@nongnu.org, Alexander Graf , qemu-ppc@nongnu.org, Cedric Le Goater , Scott Wood , Igor Mammedov --T6xhMxlHU34Bk0ad Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote: > On Sat, 2 Jul 2016 13:36:22 +0530 > Bharata B Rao wrote: >=20 > > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote: > > > If we want to generate cpu_dt_id in the machine code, this must occur > > > before the cpu gets realized. We must open code the cpu creation to be > > > able to do this. > > >=20 > > > This patch just does that. It borrows some lines from previous work > > > from Bharata to handle the feature parsing. > > >=20 > > > Signed-off-by: Greg Kurz > > > --- > > > hw/ppc/ppc.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > >=20 > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > > index dc3d214009c5..57f4ddd073d0 100644 > > > --- a/hw/ppc/ppc.c > > > +++ b/hw/ppc/ppc.c > > > @@ -32,6 +32,7 @@ > > > #include "sysemu/cpus.h" > > > #include "hw/timer/m48t59.h" > > > #include "qemu/log.h" > > > +#include "qapi/error.h" > > > #include "qemu/error-report.h" > > > #include "hw/loader.h" > > > #include "sysemu/kvm.h" > > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_i= d) > > >=20 > > > PowerPCCPU *ppc_cpu_init(const char *cpu_model) > > > { > > > - return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model)= ); > > > + PowerPCCPU *cpu; > > > + CPUClass *cc; > > > + ObjectClass *oc; > > > + gchar **model_pieces; > > > + Error *err =3D NULL; > > > + > > > + model_pieces =3D g_strsplit(cpu_model, ",", 2); > > > + if (!model_pieces[0]) { > > > + error_report("Invalid/empty CPU model name"); > > > + return NULL; > > > + } > > > + > > > + oc =3D cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]); > > > + if (oc =3D=3D NULL) { > > > + error_report("Unable to find CPU definition: %s", model_piec= es[0]); > > > + return NULL; > > > + } > > > + > > > + cpu =3D POWERPC_CPU(object_new(object_class_get_name(oc))); > > > + > > > + cc =3D CPU_CLASS(oc); > > > + cc->parse_features(CPU(cpu), model_pieces[1], &err); =20 > >=20 > > Igor is working on a patchset to convert -cpu features into global prop= erties. > > IIUC, after that patchset, it is not recommended to parse the -cpu feat= ures > > for every CPU but do it only once. > >=20 >=20 > cpu_generic_init() in the current code also does the parsing, and as the = title > says, this patch is just about open coding the creation. I don't want to > change behavior yet. >=20 > But yes, I agree that we should only parse features once and I'll be more= than > happy to fix this in a followup patch, based on Igor's work. >=20 > In the meantime, maybe I can add a comment stating that the parsing shoul= d go > away ? Right. But the thing is by open coding here, you're making two copies that need to be fixed instead of one, which increases the chances of error. It seems like it would be safer to change the generic code so there's a new generic function which doesn't do the realize which we can use on ppc (and other platforms when/if they need it). Doing the change just on ppc by making our own copy of cpu_generic_init() seems more like to lead to future mistakes. > > That is what I attempted here in the context of supporting compat cpu t= ype > > for pseries-2.7: > >=20 > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html > >=20 >=20 > Yeah and this is where I borrowed some lines. :) >=20 > > Regards, > > Bharata. > >=20 >=20 > Cheers. >=20 --=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 --T6xhMxlHU34Bk0ad Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXed4PAAoJEGw4ysog2bOS0ToP/2wHV8TY3xzvn1UCdHEEuO2E 5+Bn7aj8nH72XkQX96L01iGu4ACHcyqW3gTojera1YsuABuMzsIVEmE1meTPpL4g fwTtTDvA/TQNQrHhhwZafufZlvZLDLjm1YSEON/TcQR4KLB/haQpCb89W9Hlj/kX OyNoHrtsHvARzRrfX/PlOtz8KvFseF1bk4lxK+9ZwwwvTu64O+ikBUS7u6JB/uY/ +DxOJIrFfhNSRu0Y3hsRpyNBbqT4xASm70ldCEQvXIbmrY4Q0nZj8VzsIbIZyuFD +lHumCqf0QJeRWQInaWmzwepLdS9WwjlBSYf3mBi2irdXTLlEpTaPc+48VnFwSyT 2phBVMY3hUsoVZarq2JnJHak2xAInvISNsmze7q9CzE3K2xtCtcEeMT283Uatl1z ZDa80sJW3AFehZT9iPupO1d3PdVKZTlF0VaXzAM5KCZbvVXlJcC8Ae82MHhK5svu v/DVxVzgN6a4EwbHCrRTGUCNuXJHCI0WZ7vfjnZQGm77810s4e0wGprzqAPn0srJ mue50UawWISK847JnbeUGpJRfKyAN21/DvPQX3SOom84DPBUsOSkAtML2NJaZOCJ TtyZN73SqwaZYZl7cfAWig+4mTjRBzyXqzlIjp6sDaUjOBocEoguCUMytIvi5yoh BR6dpb2+JPgw+/EOaxfT =29uv -----END PGP SIGNATURE----- --T6xhMxlHU34Bk0ad--