From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43191) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bJyv7-000573-Ah for qemu-devel@nongnu.org; Mon, 04 Jul 2016 04:07:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bJyv2-0002bu-7S for qemu-devel@nongnu.org; Mon, 04 Jul 2016 04:07:56 -0400 Date: Mon, 4 Jul 2016 18:09:12 +1000 From: David Gibson Message-ID: <20160704080912.GK2919@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> <20160704035455.GE2919@voom.fritz.box> <20160704093747.12ed6e8a@172-15-182-60.lightspeed.austtx.sbcglobal.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="NhBACjNc9vV+/oop" Content-Disposition: inline In-Reply-To: <20160704093747.12ed6e8a@172-15-182-60.lightspeed.austtx.sbcglobal.net> 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: Igor Mammedov Cc: Greg Kurz , Bharata B Rao , Benjamin Herrenschmidt , qemu-devel@nongnu.org, Alexander Graf , qemu-ppc@nongnu.org, Cedric Le Goater , Scott Wood --NhBACjNc9vV+/oop Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jul 04, 2016 at 09:37:47AM +0200, Igor Mammedov wrote: > On Mon, 4 Jul 2016 13:54:55 +1000 > David Gibson wrote: >=20 > > 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_id) > > > > >=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_pieces[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 properties. IIUC, after that patchset, it is not > > > > recommended to parse the -cpu features 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 > > > should go away ? > >=20 > > 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. > this patch matches what has been done for x86 target as a pert of > decoupling *-user mode from machine emulation. >=20 > > 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). > We've had it in x86 until recently but I've replaced it > cpu_generic_init(), please don't go that route. >=20 > There is not much to generalize here so far it's basically following > code > cpu =3D object_new(cpu-type) > parse-features(cpu) >=20 > set_properties(cpu) /* optional machine specific */ >=20 > cpu->realize() >=20 > once parse-features refactoring is merged, there won't be anything cpu > specific left as parse-features(cpu) could be moved to generic code > and only very machine specific set_properties would be left which are > not generalizable. Ok. Greg, belay that suggestion :). --=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 --NhBACjNc9vV+/oop Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXehmoAAoJEGw4ysog2bOSkKQP/Ru57fw2wi3HTsCMOsnRPH2p KhZ9mr3VQ756/NZEBY5cScfGmpBriUjEwgOCvJ1zsHk9sKQ5qqe4jc+igymAvHq4 DYwev7XBbjrybwEZbHvQp12fV2SDkld8bLJVQ/PbYeCM2tYpxRNIYhTNApMZo2sM gwWFLRfyosDrxUgbM++7cK4BagdCuJ/y5agvbzcyehzpEEmUluBI/ncoKoQzMriI KX9lAl2NjEoDHA66Jxnc7eETvlkZMdk55U9oM5NJzQ138YNYZc13qYH8oFFw7/Oj OHqwCuz7XRLXEwU4lf/x7lPjLrJuFx3QEk82EH4cYu4HW9CIijZLKnN7u2f0KOSP 7sSmiJfibn6qTZNg3y5DlUXil3BghB4KV+2QDqvwDWfPmhNzz05bMDe9sHLp3DqW 8O6j9Uv5PzdAC4kHVvGWkIZygw3VAArKq9AZkblA90KI1Mer5bh/JjmY1LFVkuat gaHszlgDSp6Vsz5F64gI/UKOy3xIcqqVYGvPKv/ZShJm4Yh2cYbfEKxWDYYX0QQe 7KQ0wM6B8JHPV6tPps65J8QOh/CWe0hX/Or5eqYbu0YoJeQSJy3TGJLoTcH8zyt4 asY46ZNdlywwQijC0i5gNo+FFruyQ+SQGNq9AYgjjW/1cKuSILUT6/qJSTr1izTj AcldDR++iCWkBYhjaY56 =Vzrq -----END PGP SIGNATURE----- --NhBACjNc9vV+/oop--