From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57771) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ry3ta-00085X-7r for qemu-devel@nongnu.org; Thu, 16 Feb 2012 11:09:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ry3tU-0001sY-0R for qemu-devel@nongnu.org; Thu, 16 Feb 2012 11:09:22 -0500 Received: from cantor2.suse.de ([195.135.220.15]:53110 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ry3tT-0001sR-Oh for qemu-devel@nongnu.org; Thu, 16 Feb 2012 11:09:15 -0500 Message-ID: <4F3D2A2A.9040002@suse.de> Date: Thu, 16 Feb 2012 17:09:14 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1329347774-23262-1-git-send-email-imammedo@redhat.com> <1329347774-23262-3-git-send-email-imammedo@redhat.com> In-Reply-To: <1329347774-23262-3-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: jan.kiszka@siemens.com, qemu-devel@nongnu.org, gleb@redhat.com Am 16.02.2012 00:16, schrieb Igor Mammedov: > Convert pc cpu to qdev device that is attached to icc bus, later > hot-plug ability of icc bus will allow to implement cpu hot-plug. >=20 > Signed-off-by: Igor Mammedov This conflicts with CPU QOM'ification across targets (and no longer applies due to type_init() introduction). > --- > hw/pc.c | 62 ++++++++++++++++++++++++++++++++++++++++++= +------ > target-i386/cpu.h | 1 + > target-i386/helper.c | 26 ++++++++++++++------ > 3 files changed, 73 insertions(+), 16 deletions(-) >=20 > diff --git a/hw/pc.c b/hw/pc.c > index 33d8090..b8db5dc 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, = int level) > } > } > =20 > +typedef struct CPUPC { > + ICCBusDevice busdev; > + char *model; > + CPUState state; > +} CPUPC; I don't like this approach. For starters, using CPUState as field rather than pointer seems a bad idea to me (CPUX86State would only be slightly better). We should have a single code path through which we instantiate CPUs, currently: cpu_$arch_init(const char *cpu_model) With my series completed, this would return an X86CPU object. Depending on our liking, we could either place some ICC / APIC / whatever fields directly into that, or embed the X86CPU in an object such as yours above as a link. I do feel however that the model string is misplaced there. Question is whether this ICC stuff is actually part of the CPU or part of the CPU wiring on the mainboard - I vaguely remember someone saying that this changed over time...? Having both depending on CPU subclass might also be an option, but I'd rather leave such decisions as a follow-up to the core QOM'ification. Andreas > + > static void pc_cpu_reset(void *opaque) > { > CPUState *env =3D opaque; > @@ -930,23 +936,63 @@ static void pc_cpu_reset(void *opaque) > env->halted =3D !cpu_is_bsp(env); > } > =20 > -static CPUState *pc_new_cpu(const char *cpu_model) > +static DeviceState *pc_new_cpu(const char *cpu_model) > { > - CPUState *env; > + DeviceState *dev; > + BusState *b; > =20 > - env =3D cpu_init(cpu_model); > - if (!env) { > - fprintf(stderr, "Unable to find x86 CPU definition\n"); > - exit(1); > + b =3D get_icc_bus(); > + dev =3D qdev_create(b, "cpu-pc"); > + if (!dev) { > + return NULL; > + } > + qdev_prop_set_string(dev, "model", g_strdup(cpu_model)); > + if (qdev_init(dev) < 0) { > + return NULL; > + } > + return dev; > +} > + > +static int cpu_device_init(ICCBusDevice *dev) > +{ > + CPUPC* cpu =3D DO_UPCAST(CPUPC, busdev, dev); > + CPUState *env =3D &cpu->state; > + > + if (cpu_x86_init_inplace(env, cpu->model) < 0) { > + return -1; > } > + > if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { > env->apic_state =3D apic_init(env, env->cpuid_apic_id); > } > - qemu_register_reset(pc_cpu_reset, env); > + > + return 0; > +} > + > +static void cpu_device_reset(DeviceState *dev) { > + CPUPC *cpu =3D DO_UPCAST(CPUPC, busdev.qdev, dev); > + CPUState *env =3D &cpu->state; > pc_cpu_reset(env); > - return env; > } > =20 > +static ICCBusDeviceInfo cpu_device_info =3D { > + .qdev.name =3D "cpu-pc", > + .qdev.size =3D sizeof(CPUPC), > + .qdev.reset =3D cpu_device_reset, > + .init =3D cpu_device_init, > + .qdev.props =3D (Property[]) { > + DEFINE_PROP_STRING("model", CPUPC, model), > + DEFINE_PROP_END_OF_LIST(), > + }, > +}; > + > +static void pc_register_devices(void) > +{ > + iccbus_register_devinfo(&cpu_device_info); > +} > + > +device_init(pc_register_devices); > + > void pc_cpus_init(const char *cpu_model) > { > int i; > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 7e66bcf..830b65e 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -775,6 +775,7 @@ typedef struct CPUX86State { > } CPUX86State; > =20 > CPUX86State *cpu_x86_init(const char *cpu_model); > +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model); > int cpu_x86_exec(CPUX86State *s); > void cpu_x86_close(CPUX86State *s); > void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *= optarg); > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 2586aff..df2f5ba 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1235,12 +1235,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, u= nsigned int selector, > return 1; > } > =20 > -CPUX86State *cpu_x86_init(const char *cpu_model) > +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model) > { > - CPUX86State *env; > static int inited; > =20 > - env =3D g_malloc0(sizeof(CPUX86State)); > + if (cpu_x86_register(env, cpu_model) < 0) { > + fprintf(stderr, "Unable to find x86 CPU definition\n"); > + return -1; > + } > cpu_exec_init(env); > env->cpu_model_str =3D cpu_model; > =20 > @@ -1253,18 +1255,26 @@ CPUX86State *cpu_x86_init(const char *cpu_model= ) > cpu_set_debug_excp_handler(breakpoint_handler); > #endif > } > - if (cpu_x86_register(env, cpu_model) < 0) { > - cpu_x86_close(env); > - return NULL; > - } > env->cpuid_apic_id =3D env->cpu_index; > mce_init(env); > =20 > qemu_init_vcpu(env); > =20 > - return env; > + return 0; > } > =20 > +CPUX86State *cpu_x86_init(const char *cpu_model) > +{ > + CPUX86State *env; > + > + env =3D g_malloc0(sizeof(CPUX86State)); > + if (cpu_x86_init_inplace(env, cpu_model) < 0) { > + g_free(env); > + return NULL; > + } > + > + return env; > +} > #if !defined(CONFIG_USER_ONLY) > void do_cpu_init(CPUState *env) > { --=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