From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57356) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTbOZ-0005tb-J8 for qemu-devel@nongnu.org; Wed, 31 Oct 2012 12:44:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TTbOS-0007Wm-HH for qemu-devel@nongnu.org; Wed, 31 Oct 2012 12:43:59 -0400 Received: from cantor2.suse.de ([195.135.220.15]:59776 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTbOS-0007WW-6r for qemu-devel@nongnu.org; Wed, 31 Oct 2012 12:43:52 -0400 Message-ID: <50915544.6070600@suse.de> Date: Wed, 31 Oct 2012 17:43:48 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 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> In-Reply-To: <20121031173233.072f8b45@nial.usersys.redhat.com> 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: Igor Mammedov Cc: Paolo Bonzini , Eduardo Habkost , qemu-devel@nongnu.org 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 CP= U >> object, before x86_cpu_realize(). So, make cpu_x86_init() not call > Killing cpu_x86_init() altogether will make future re-factoring even ea= sier. > 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. 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 series (that now depend on qdev, which I find rather nasty to review), but I have my hands quite full currently, no promises... Andreas >=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 additi= onal >> + * 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 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg