From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38510) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SWmla-0003eU-OF for qemu-devel@nongnu.org; Tue, 22 May 2012 06:56:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SWmlT-0002hB-VZ for qemu-devel@nongnu.org; Tue, 22 May 2012 06:56:38 -0400 Received: from goliath.siemens.de ([192.35.17.28]:33798) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SWmlT-0002gr-LB for qemu-devel@nongnu.org; Tue, 22 May 2012 06:56:31 -0400 Message-ID: <4FBB70D6.8070101@siemens.com> Date: Tue, 22 May 2012 07:56:22 -0300 From: Jan Kiszka MIME-Version: 1.0 References: <1337682954-20618-1-git-send-email-imammedo@redhat.com> <1337682954-20618-5-git-send-email-imammedo@redhat.com> In-Reply-To: <1337682954-20618-5-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qom-next 4/5] target-i386: make initialize CPU in QOM way List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: "aliguori@us.ibm.com" , "ehabkost@redhat.com" , "sw@weilnetz.de" , "mtosatti@redhat.com" , "qemu-devel@nongnu.org" , "blauwirbel@gmail.com" , "avi@redhat.com" , "pbonzini@redhat.com" , "afaerber@suse.de" On 2012-05-22 07:35, Igor Mammedov wrote: > 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/initialization > 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) > > Signed-off-by: Igor Mammedov > --- > hw/pc.c | 32 +++++--------------------- > target-i386/cpu.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++- > target-i386/helper.c | 39 -------------------------------- > 3 files changed, 65 insertions(+), 66 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 0eb0b73..677f9e0 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -894,37 +894,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 = cpu_x86_init(cpu_model); > - if (cpu == NULL) { > - exit(1); > - } > - env = &cpu->env; > - if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { > - if (kvm_irqchip_in_kernel()) { > - env->apic_state = qdev_create(NULL, "kvm-apic"); > - } else { > - env->apic_state = qdev_create(NULL, "apic"); > - } > - qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id); > - qdev_prop_set_ptr(env->apic_state, "cpu_env", env); > - qdev_init_nofail(env->apic_state); > - } > - 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 = 0; i < smp_cpus; i++) { > - pc_new_cpu(cpu_model); > + cpu = cpu_x86_init(cpu_model); > + if (cpu == NULL) { > + exit(1); > + } > + qemu_register_reset(pc_cpu_reset, cpu); > } > } > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 538892d..0e804ea 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -31,6 +31,9 @@ > > #include "hyperv.h" > > +#include "hw/qdev.h" > +#include "sysemu.h" > + > /* feature flags taken from "Intel Processor Identification and the CPUID > * Instruction" and AMD's "CPUID Specification". In cases of disagreement > * between feature naming conventions, aliases may be added. > @@ -1747,21 +1750,76 @@ static void x86_set_cpu_model(Object *obj, const 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; > + } > + > + if (((env->cpuid_features & CPUID_APIC) || smp_cpus > 1)) { > + if (kvm_irqchip_in_kernel()) { > + env->apic_state = qdev_create(NULL, "kvm-apic"); > + } else { > + env->apic_state = 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); > + } > +} > + > +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 = 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 == 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); > } > } > > void x86_cpu_realize(Object *obj, Error **errp) > { > X86CPU *cpu = X86_CPU(obj); > + CPUX86State *env = &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; > + } > + } > > mce_init(cpu); > - qemu_init_vcpu(&cpu->env); > + qemu_init_vcpu(env); > + cpu_reset(CPU(cpu)); > } > > static void x86_cpu_initfn(Object *obj) > { > X86CPU *cpu = X86_CPU(obj); > CPUX86State *env = &cpu->env; > + static int inited; > > cpu_exec_init(env); > > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 6fc67a9..443092e 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -947,34 +947,6 @@ int check_hw_breakpoints(CPUX86State *env, int force_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 = 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 == 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); > -} > - > typedef struct MCEInjectionParams { > Monitor *mon; > CPUX86State *env; > @@ -1161,20 +1133,9 @@ X86CPU *cpu_x86_init(const char *cpu_model) > { > X86CPU *cpu; > Error *errp = NULL; > - static int inited; > > cpu = X86_CPU(object_new(TYPE_X86_CPU)); > > - /* init various static tables used in TCG mode */ > - if (tcg_enabled() && !inited) { > - inited = 1; > - optimize_flags_init(); > -#ifndef CONFIG_USER_ONLY > - prev_debug_excp_handler = > - cpu_set_debug_excp_handler(breakpoint_handler); > -#endif > - } > - Where does this hunk go to? Did you test the result against TCG? :) Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux