From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55593) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXUNB-0006cY-H4 for qemu-devel@nongnu.org; Thu, 24 May 2012 05:30:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SXUMz-0004R9-M3 for qemu-devel@nongnu.org; Thu, 24 May 2012 05:30:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3113) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXUMz-0004PU-F3 for qemu-devel@nongnu.org; Thu, 24 May 2012 05:30:09 -0400 Message-ID: <4FBDFF92.7050601@redhat.com> Date: Thu, 24 May 2012 11:29:54 +0200 From: Igor Mammedov MIME-Version: 1.0 References: <1337791181-27446-1-git-send-email-imammedo@redhat.com> <1337791181-27446-6-git-send-email-imammedo@redhat.com> <4FBD5655.1000603@suse.de> In-Reply-To: <4FBD5655.1000603@suse.de> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH qom-next 5/6] target-i386: make initialize CPU in QOM way List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-15?Q?Andreas_F=E4rber?= Cc: peter.maydell@linaro.org, aliguori@us.ibm.com, wei.liu2@citrix.com, ehabkost@redhat.com, stefano.stabellini@eu.citrix.com, sw@weilnetz.de, mtosatti@redhat.com, qemu-devel@nongnu.org, agraf@suse.de, blauwirbel@gmail.com, avi@redhat.com, jan.kiszka@siemens.com, anthony.perard@citrix.com, pbonzini@redhat.com On 05/23/2012 11:27 PM, Andreas F=E4rber wrote: > Am 23.05.2012 18:39, schrieb Igor Mammedov: >> Make CPU creation/initialization consistent with QOM object >> behavior in this, by moving tcg and apic initialization from board >> level into CPU's initfn/realize calls and cpu_model property setter. >> >> Which makes CPU object self-sufficient in respect of creation/initiali= zation >> and matches a typical object creation sequence, i.e.: >> - create CPU instance >> - set properties >> - realize object - (x86_cpu_realize will be converted into realize >> property setter, when it is implemented) >> >> v2: >> - fix moving of tcg_* initialization into cpu.c from helper.c >> spotted-by: >> - make it compile/work on i386-linux-user target >> >> Signed-off-by: Igor Mammedov >> --- >> hw/pc.c | 45 ++++------------------------ >> target-i386/cpu.c | 81 ++++++++++++++++++++++++++++++++++++++++= ++++++++- >> target-i386/helper.c | 39 ------------------------ >> 3 files changed, 85 insertions(+), 80 deletions(-) >> >> diff --git a/hw/pc.c b/hw/pc.c >> index 1ccfc6e..d7845ea 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c > >> @@ -911,30 +891,17 @@ static void pc_cpu_reset(void *opaque) >> cpu_reset(CPU(cpu)); >> } >> >> -static X86CPU *pc_new_cpu(const char *cpu_model) >> -{ >> - X86CPU *cpu; >> - CPUX86State *env; >> - >> - cpu =3D cpu_x86_init(cpu_model); >> - if (cpu =3D=3D NULL) { >> - exit(1); >> - } >> - env =3D&cpu->env; >> - 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, cpu); >> - pc_cpu_reset(cpu); >> - return cpu; >> -} >> - >> void pc_cpus_init(const char *cpu_model) >> { >> + X86CPU *cpu; >> int i; >> >> for(i =3D 0; i< smp_cpus; i++) { > > If we do changes here, please add the missing space. :) I'll fix it. > >> - pc_new_cpu(cpu_model); >> + cpu =3D cpu_x86_init(cpu_model); >> + if (cpu =3D=3D NULL) { >> + exit(1); >> + } >> + qemu_register_reset(pc_cpu_reset, cpu); >> } >> } >> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index e655129..99ef891 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c > >> @@ -1749,24 +1753,89 @@ static void x86_set_cpu_model(Object *obj, con= st char *value, Error **errp) >> if (cpu_x86_register(cpu, env->cpu_model_str)< 0) { >> fprintf(stderr, "Unable to find x86 CPU definition\n"); >> error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); >> + return; >> + } >> + >> +#ifndef CONFIG_USER_ONLY >> + if (((env->cpuid_features& CPUID_APIC) || smp_cpus> 1)) { >> + if (kvm_irqchip_in_kernel()) { >> + env->apic_state =3D qdev_create(NULL, "kvm-apic"); >> + } else if (xen_enabled()) { >> + env->apic_state =3D qdev_create(NULL, "xen-apic"); >> + } else { >> + env->apic_state =3D qdev_create(NULL, "apic"); >> + } >> + object_property_add_child(OBJECT(cpu), "apic", >> + OBJECT(env->apic_state), NULL); >> + >> + qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id= ); >> + qdev_prop_set_ptr(env->apic_state, "cpu_env", env); > > I'd like to avoid re-adding this set_ptr(). We can cherry-pick my > link property from QOM CPUState part 4 series. sure, I'll add you link patches and rebase on top of it > >> + } >> +#endif >> +} >> + >> +#ifndef CONFIG_USER_ONLY >> +static CPUDebugExcpHandler *prev_debug_excp_handler; >> + >> +static void breakpoint_handler(CPUX86State *env) >> +{ >> + CPUBreakpoint *bp; >> + >> + if (env->watchpoint_hit) { >> + if (env->watchpoint_hit->flags& BP_CPU) { >> + env->watchpoint_hit =3D NULL; >> + if (check_hw_breakpoints(env, 0)) { >> + raise_exception_env(EXCP01_DB, env); >> + } else { >> + cpu_resume_from_signal(env, NULL); >> + } >> + } >> + } else { >> + QTAILQ_FOREACH(bp,&env->breakpoints, entry) >> + if (bp->pc =3D=3D env->eip) { >> + if (bp->flags& BP_CPU) { >> + check_hw_breakpoints(env, 1); >> + raise_exception_env(EXCP01_DB, env); >> + } >> + break; >> + } >> + } >> + if (prev_debug_excp_handler) { >> + prev_debug_excp_handler(env); >> } >> } >> +#endif >> >> void x86_cpu_realize(Object *obj, Error **errp) >> { >> X86CPU *cpu =3D X86_CPU(obj); >> +#ifndef CONFIG_USER_ONLY >> + CPUX86State *env =3D&cpu->env; >> + >> + if (env->apic_state) { >> + if (qdev_init(env->apic_state)< 0) { >> + error_set(errp, QERR_DEVICE_INIT_FAILED, >> + object_get_typename(OBJECT(env->apic_state))); >> + return; >> + } >> + } >> +#endif >> >> mce_init(cpu); >> - qemu_init_vcpu(&cpu->env); >> + qemu_init_vcpu(env); > > This only works because currently qemu_init_vcpu() is a no-op macro tha= t > doesn't use the parameter. Please don't change it back, I guess it's a > mismerge. I'll fix it. > > We can avoid the env variable if I finish merging Paolo's series - by > realizing the CPU the APIC as its child would get realized, too. Is the > ordering before mce_init() mandatory here or is it just to reduce the > #ifndef'ery? mce_init() ordering here is not important and it is less #ifndef-s this w= ay. yep, with Paolo's realize patch whole #ifndef block should be thrown away. I can split it into a separate patch, that could be easily discarded when Paolo's realize is committed. >> + cpu_reset(CPU(cpu)); >> } >> >> static void x86_cpu_initfn(Object *obj) >> { >> X86CPU *cpu =3D X86_CPU(obj); >> CPUX86State *env =3D&cpu->env; >> + static int inited; >> >> cpu_exec_init(env); >> >> + env->cpuid_apic_id =3D env->cpu_index; >> + >> object_property_add(obj, "family", "int", >> x86_cpuid_version_get_family, >> x86_cpuid_version_set_family, NULL, NULL, NU= LL); >> @@ -1795,7 +1864,15 @@ static void x86_cpu_initfn(Object *obj) >> object_property_add_str(obj, "cpu-model", >> x86_get_cpu_model, x86_set_cpu_model, NULL); >> >> - env->cpuid_apic_id =3D env->cpu_index; >> + /* init various static tables used in TCG mode */ >> + if (tcg_enabled()&& !inited) { >> + inited =3D 1; >> + optimize_flags_init(); >> +#ifndef CONFIG_USER_ONLY >> + prev_debug_excp_handler =3D >> + cpu_set_debug_excp_handler(breakpoint_handler); >> +#endif >> + } > > Did you forget to put that into its own patch or did that not work? > My idea was to have it first in the series so that other changes here > and elsewhere can be rebased onto it. > > Also I wonder whether it would better be placed into the class_init? I'= d > tend towards initfn because that will not be invoked during type > enumeration. Ok, I'll split it into separate patch. On the second thought there is no compelling reason to move this hunk in = cpu.c It could be left at board level, just moved in the beginning of pc_cpus_i= nit() and called only once there. > >> } >> >> static void x86_cpu_common_class_init(ObjectClass *oc, void *data) >> diff --git a/target-i386/helper.c b/target-i386/helper.c >> index fbaeeea..38ac25d 100644 >> --- a/target-i386/helper.c >> +++ b/target-i386/helper.c >> @@ -941,34 +941,6 @@ int check_hw_breakpoints(CPUX86State *env, int fo= rce_dr6_update) >> return hit_enabled; >> } >> >> -static CPUDebugExcpHandler *prev_debug_excp_handler; >> - >> -static void breakpoint_handler(CPUX86State *env) >> -{ >> - CPUBreakpoint *bp; >> - >> - if (env->watchpoint_hit) { >> - if (env->watchpoint_hit->flags& BP_CPU) { >> - env->watchpoint_hit =3D NULL; >> - if (check_hw_breakpoints(env, 0)) >> - raise_exception_env(EXCP01_DB, env); >> - else >> - cpu_resume_from_signal(env, NULL); >> - } >> - } else { >> - QTAILQ_FOREACH(bp,&env->breakpoints, entry) >> - if (bp->pc =3D=3D env->eip) { >> - if (bp->flags& BP_CPU) { >> - check_hw_breakpoints(env, 1); >> - raise_exception_env(EXCP01_DB, env); >> - } >> - break; >> - } >> - } >> - if (prev_debug_excp_handler) >> - prev_debug_excp_handler(env); >> -} >> - > > I wonder if that could rather stay here as non-static? Any preference in what header file it should be? --=20 ----- Thanks Igor