Patchwork [qom-next,5/6] target-i386: make initialize CPU in QOM way

login
register
mail settings
Submitter Igor Mammedov
Date May 23, 2012, 4:39 p.m.
Message ID <1337791181-27446-6-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/160966/
State New
Headers show

Comments

Igor Mammedov - May 23, 2012, 4:39 p.m.
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)

v2:
  - fix moving of tcg_* initialization into cpu.c from helper.c
      spotted-by: <Jan Kiszka jan.kiszka@siemens.com>
  - make it compile/work on i386-linux-user target

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c              |   45 ++++------------------------
 target-i386/cpu.c    |   81 ++++++++++++++++++++++++++++++++++++++++++++++++-
 target-i386/helper.c |   39 ------------------------
 3 files changed, 85 insertions(+), 80 deletions(-)
Andreas Färber - May 23, 2012, 9:27 p.m.
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/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)
> 
> v2:
>   - fix moving of tcg_* initialization into cpu.c from helper.c
>       spotted-by: <Jan Kiszka jan.kiszka@siemens.com>
>   - make it compile/work on i386-linux-user target
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  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 = cpu_x86_init(cpu_model);
> -    if (cpu == NULL) {
> -        exit(1);
> -    }
> -    env = &cpu->env;
> -    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> -        env->apic_state = 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 = 0; i < smp_cpus; i++) {

If we do changes here, please add the missing space. :)

> -        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 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, 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;
> +    }
> +
> +#ifndef CONFIG_USER_ONLY
> +    if (((env->cpuid_features & CPUID_APIC) || smp_cpus > 1)) {
> +        if (kvm_irqchip_in_kernel()) {
> +            env->apic_state = qdev_create(NULL, "kvm-apic");
> +        } else if (xen_enabled()) {
> +            env->apic_state = qdev_create(NULL, "xen-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);

I'd like to avoid re-adding this set_ptr(). We can cherry-pick my
link<X86CPU> property from QOM CPUState part 4 series.

> +    }
> +#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 = 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);
>      }
>  }
> +#endif
>  
>  void x86_cpu_realize(Object *obj, Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(obj);
> +#ifndef CONFIG_USER_ONLY
> +    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;
> +        }
> +    }
> +#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 that
doesn't use the parameter. Please don't change it back, I guess it's a
mismerge.

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?

> +    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);
>  
> +    env->cpuid_apic_id = env->cpu_index;
> +
>      object_property_add(obj, "family", "int",
>                          x86_cpuid_version_get_family,
>                          x86_cpuid_version_set_family, NULL, NULL, NULL);
> @@ -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 = env->cpu_index;
> +    /* 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
> +    }

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.

>  }
>  
>  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 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);
> -}
> -

I wonder if that could rather stay here as non-static?

>  typedef struct MCEInjectionParams {
>      Monitor *mon;
>      CPUX86State *env;
> @@ -1155,20 +1127,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
> -    }
> -
>      if (cpu_model) {
>          object_property_set_str(OBJECT(cpu), cpu_model, "cpu-model", &errp);
>      } else {

/-F
Igor Mammedov - May 24, 2012, 9:29 a.m.
On 05/23/2012 11:27 PM, Andreas Färber 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/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)
>>
>> v2:
>>    - fix moving of tcg_* initialization into cpu.c from helper.c
>>        spotted-by:<Jan Kiszka jan.kiszka@siemens.com>
>>    - make it compile/work on i386-linux-user target
>>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>> ---
>>   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 = cpu_x86_init(cpu_model);
>> -    if (cpu == NULL) {
>> -        exit(1);
>> -    }
>> -    env =&cpu->env;
>> -    if ((env->cpuid_features&  CPUID_APIC) || smp_cpus>  1) {
>> -        env->apic_state = 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 = 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 = 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 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, 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;
>> +    }
>> +
>> +#ifndef CONFIG_USER_ONLY
>> +    if (((env->cpuid_features&  CPUID_APIC) || smp_cpus>  1)) {
>> +        if (kvm_irqchip_in_kernel()) {
>> +            env->apic_state = qdev_create(NULL, "kvm-apic");
>> +        } else if (xen_enabled()) {
>> +            env->apic_state = qdev_create(NULL, "xen-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);
>
> I'd like to avoid re-adding this set_ptr(). We can cherry-pick my
> link<X86CPU>  property from QOM CPUState part 4 series.
sure, I'll add you link<X86CPU> 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 = 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);
>>       }
>>   }
>> +#endif
>>
>>   void x86_cpu_realize(Object *obj, Error **errp)
>>   {
>>       X86CPU *cpu = X86_CPU(obj);
>> +#ifndef CONFIG_USER_ONLY
>> +    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;
>> +        }
>> +    }
>> +#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 that
> 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 way.
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 = X86_CPU(obj);
>>       CPUX86State *env =&cpu->env;
>> +    static int inited;
>>
>>       cpu_exec_init(env);
>>
>> +    env->cpuid_apic_id = env->cpu_index;
>> +
>>       object_property_add(obj, "family", "int",
>>                           x86_cpuid_version_get_family,
>>                           x86_cpuid_version_set_family, NULL, NULL, NULL);
>> @@ -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 = env->cpu_index;
>> +    /* 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
>> +    }
>
> 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_init()
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 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);
>> -}
>> -
>
> I wonder if that could rather stay here as non-static?
Any preference in what header file it should be?

Patch

diff --git a/hw/pc.c b/hw/pc.c
index 1ccfc6e..d7845ea 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -42,7 +42,6 @@ 
 #include "sysbus.h"
 #include "sysemu.h"
 #include "kvm.h"
-#include "xen.h"
 #include "blockdev.h"
 #include "ui/qemu-spice.h"
 #include "memory.h"
@@ -877,25 +876,6 @@  DeviceState *cpu_get_current_apic(void)
     }
 }
 
-static DeviceState *apic_init(void *env, uint8_t apic_id)
-{
-    DeviceState *dev;
-
-    if (kvm_irqchip_in_kernel()) {
-        dev = qdev_create(NULL, "kvm-apic");
-    } else if (xen_enabled()) {
-        dev = qdev_create(NULL, "xen-apic");
-    } else {
-        dev = qdev_create(NULL, "apic");
-    }
-
-    qdev_prop_set_uint8(dev, "id", apic_id);
-    qdev_prop_set_ptr(dev, "cpu_env", env);
-    qdev_init_nofail(dev);
-
-    return dev;
-}
-
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 {
     CPUX86State *s = opaque;
@@ -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 = cpu_x86_init(cpu_model);
-    if (cpu == NULL) {
-        exit(1);
-    }
-    env = &cpu->env;
-    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
-        env->apic_state = 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 = 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 e655129..99ef891 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -23,6 +23,7 @@ 
 
 #include "cpu.h"
 #include "kvm.h"
+#include "hw/xen.h"
 
 #include "qemu-option.h"
 #include "qemu-config.h"
@@ -31,6 +32,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.
@@ -1749,24 +1753,89 @@  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;
+    }
+
+#ifndef CONFIG_USER_ONLY
+    if (((env->cpuid_features & CPUID_APIC) || smp_cpus > 1)) {
+        if (kvm_irqchip_in_kernel()) {
+            env->apic_state = qdev_create(NULL, "kvm-apic");
+        } else if (xen_enabled()) {
+            env->apic_state = qdev_create(NULL, "xen-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);
+    }
+#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 = 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);
     }
 }
+#endif
 
 void x86_cpu_realize(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
+#ifndef CONFIG_USER_ONLY
+    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;
+        }
+    }
+#endif
 
     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);
 
+    env->cpuid_apic_id = env->cpu_index;
+
     object_property_add(obj, "family", "int",
                         x86_cpuid_version_get_family,
                         x86_cpuid_version_set_family, NULL, NULL, NULL);
@@ -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 = env->cpu_index;
+    /* 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
+    }
 }
 
 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 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;
@@ -1155,20 +1127,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
-    }
-
     if (cpu_model) {
         object_property_set_str(OBJECT(cpu), cpu_model, "cpu-model", &errp);
     } else {