From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39115) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SWoUz-0003Te-7M for qemu-devel@nongnu.org; Tue, 22 May 2012 08:47:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SWoUs-0004WC-1O for qemu-devel@nongnu.org; Tue, 22 May 2012 08:47:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33547) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SWoUr-0004VX-Q5 for qemu-devel@nongnu.org; Tue, 22 May 2012 08:47:29 -0400 Message-ID: <4FBB8ADB.8070508@redhat.com> Date: Tue, 22 May 2012 14:47:23 +0200 From: Igor Mammedov MIME-Version: 1.0 References: <1337682954-20618-1-git-send-email-imammedo@redhat.com> <1337682954-20618-5-git-send-email-imammedo@redhat.com> <4FBB70D6.8070101@siemens.com> In-Reply-To: <4FBB70D6.8070101@siemens.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: Jan Kiszka 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 05/22/2012 12:56 PM, Jan Kiszka wrote: > 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? :) Yep it works :) It's my bad, that due patches reshuffling I've moved this hunk into: [PATCH qom-next 2/5] target-i386: add cpu-model property to x86_cpu Will move it to this patch. Thanks, Igor