From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41781) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTuHO-0000ZR-IZ for qemu-devel@nongnu.org; Thu, 01 Nov 2012 08:53:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TTuHN-0002vu-0j for qemu-devel@nongnu.org; Thu, 01 Nov 2012 08:53:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29932) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTuHM-0002vo-Nn for qemu-devel@nongnu.org; Thu, 01 Nov 2012 08:53:48 -0400 Date: Thu, 1 Nov 2012 13:53:44 +0100 From: Igor Mammedov Message-ID: <20121101135344.4c622b15@nial.usersys.redhat.com> In-Reply-To: <50915544.6070600@suse.de> References: <1351101001-14589-1-git-send-email-ehabkost@redhat.com> <1351101001-14589-21-git-send-email-ehabkost@redhat.com> <20121031173233.072f8b45@nial.usersys.redhat.com> <50915544.6070600@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 20/27] target-i386: do not call x86_cpu_realize() on cpu_x86_init() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?ISO-8859-1?B?RuRyYmVy?= Cc: Paolo Bonzini , Eduardo Habkost , qemu-devel@nongnu.org On Wed, 31 Oct 2012 17:43:48 +0100 Andreas F=E4rber wrote: > Am 31.10.2012 17:32, schrieb Igor Mammedov: > > On Wed, 24 Oct 2012 15:49:54 -0200 > > Eduardo Habkost wrote: > >=20 > >> The PC code will need to run additional steps when initializing the CPU > >> object, before x86_cpu_realize(). So, make cpu_x86_init() not call > > Killing cpu_x86_init() altogether will make future re-factoring even > > easier. For present its code could be duplicated in cpu_init() and pc.c, > >=20 > > and with cpu subclasses cpu_init () would be reduced to > > cpu =3D object_new(X86CPU.QEMUxx); > > cpu.realize(); > > and pc_cpus_init() > > cpu =3D object_new(X86CPU.QEMUxx); > > make cpu a child of /machine (); > > apply custom properties (); > > cpu.realize(); > >=20 > > I don't see any benefits in keeping cpu_x86_init() around and if we sta= rt > > touching it then just lets get rid of it in one step. >=20 > To my regret, CPU subclasses have moved to the end of your two queues. I > was considering doing a proposal to fast-track that for symmetry with > the other targets and gradually improve that with the pending properties I could revive not qdev based version of cpu properties so we could move forward if Anthony won't object (he argued in favor of qdev based one on IR= C).=20 > series (that now depend on qdev, which I find rather nasty to review), Is there anything that could be done to improve review-ability of cpu-as-qd= ev > but I have my hands quite full currently, no promises... >=20 > Andreas >=20 > >=20 > >> x86_cpu_realize(), and add two x86_cpu_realize() calls: > >> > >> - One on cpu_init(), that is called only by *-user > >> - One on pc_cpu_init(), that will include the more advanced PC CPU > >> initialization steps > >> > >> Signed-off-by: Eduardo Habkost > >> --- > >> hw/pc.c | 12 +++++++++++- > >> target-i386/cpu.h | 14 ++++++++++++++ > >> target-i386/helper.c | 11 ++++------- > >> 3 files changed, 29 insertions(+), 8 deletions(-) > >> > >> diff --git a/hw/pc.c b/hw/pc.c > >> index 85eab04..c209d3d 100644 > >> --- a/hw/pc.c > >> +++ b/hw/pc.c > >> @@ -861,10 +861,20 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, > >> int level)=20 > >> static void pc_cpu_init(PCInitArgs *args, int cpu_index) > >> { > >> - if (!cpu_x86_init(args->qemu_args->cpu_model)) { > >> + Error *err =3D NULL; > >> + X86CPU *cpu; > >> + > >> + cpu =3D cpu_x86_init(args->qemu_args->cpu_model); > >> + if (!cpu) { > >> fprintf(stderr, "Unable to find x86 CPU definition\n"); > >> exit(1); > >> } > >> + > >> + x86_cpu_realize(OBJECT(cpu), &err); > >> + if (err) { > >> + error_report("pc_cpu_init: %s\n", error_get_pretty(err)); > >> + exit(1); > >> + } > >> } > >> =20 > >> void pc_cpus_init(PCInitArgs *args) > >> diff --git a/target-i386/cpu.h b/target-i386/cpu.h > >> index 871c270..6853b17 100644 > >> --- a/target-i386/cpu.h > >> +++ b/target-i386/cpu.h > >> @@ -21,6 +21,7 @@ > >> =20 > >> #include "config.h" > >> #include "qemu-common.h" > >> +#include "qemu-error.h" > >> =20 > >> #ifdef TARGET_X86_64 > >> #define TARGET_LONG_BITS 64 > >> @@ -1008,12 +1009,25 @@ uint64_t cpu_get_tsc(CPUX86State *env); > >> #define TARGET_VIRT_ADDR_SPACE_BITS 32 > >> #endif > >> =20 > >> +/* Helper for simple CPU initialization (for target-independent code) > >> + * > >> + * Note that the PC code doesn't use this function, as it does > >> additional > >> + * initialization steps between cpu_x86_init() and cpu_x86_realize() = is > >> called. > >> + */ > >> static inline CPUX86State *cpu_init(const char *cpu_model) > >> { > >> + Error *err =3D NULL; > >> X86CPU *cpu =3D cpu_x86_init(cpu_model); > >> if (cpu =3D=3D NULL) { > >> return NULL; > >> } > >> + > >> + x86_cpu_realize(OBJECT(cpu), &err); > >> + if (err) { > >> + error_report("cpu_init: %s\n", error_get_pretty(err)); > >> + return NULL; > >> + } > >> + > >> return &cpu->env; > >> } > >> =20 > >> diff --git a/target-i386/helper.c b/target-i386/helper.c > >> index 1e5f61f..87a9221 100644 > >> --- a/target-i386/helper.c > >> +++ b/target-i386/helper.c > >> @@ -1240,11 +1240,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, > >> unsigned int selector, return 1; > >> } > >> =20 > >> +/* Initialize X86CPU object > >> + * > >> + * Callers must eventually call x86_cpu_realize(), to finish > >> initialization. > >> + */ > >> X86CPU *cpu_x86_init(const char *cpu_model) > >> { > >> X86CPU *cpu; > >> CPUX86State *env; > >> - Error *err =3D NULL; > >> =20 > >> cpu =3D X86_CPU(object_new(TYPE_X86_CPU)); > >> env =3D &cpu->env; > >> @@ -1255,12 +1258,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) > >> return NULL; > >> } > >> =20 > >> - x86_cpu_realize(OBJECT(cpu), &err); > >> - if (err) { > >> - error_report("cpu_x86_init: %s\n", error_get_pretty(err)); > >> - return NULL; > >> - } > >> - > >> return cpu; > >> } > >> =20 > >=20 >=20 >=20