Re: [Qemu-devel] [PATCH qom-next 11/12] target-i386: initialize APIC at CPU level

2012-05-30 Thread Jan Kiszka
On 2012-05-30 00:10, Igor Mammedov wrote:
 (L)APIC is a part of cpu [1] so move APIC initialization inside of
 x86_cpu object. Since cpu_model and override flags currently specify
 whether APIC should be created or not, APIC creation is moved into
 cpu_model property setter. And APIC initialization is moved into
 x86_cpu_apic_init() which is called from x86_cpu_realize().
 
 [1] - all x86 cpus have integrated APIC if we overlook existence of i486,
 and it's more convenient to model after majority of them.
 
 v2:
  * init APIC mapping at cpu level, due to Peter's objection to putting
it into APIC's initfn and Jan's suggestion to do it inside cpu.
 
 Signed-off-by: Igor Mammedov imamm...@redhat.com
 ---
  hw/pc.c   |   44 -
  target-i386/cpu.c |   62 
 +
  2 files changed, 62 insertions(+), 44 deletions(-)
 
 diff --git a/hw/pc.c b/hw/pc.c
 index d178801..986f119 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
 @@ -70,8 +69,6 @@
  #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
  #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
  
 -#define MSI_ADDR_BASE 0xfee0
 -
  #define E820_NR_ENTRIES  16
  
  struct e820_entry {
 @@ -879,42 +876,6 @@ DeviceState *cpu_get_current_apic(void)
  }
  }
  
 -static DeviceState *apic_init(void *env, uint8_t apic_id)
 -{
 -DeviceState *dev;
 -Error *error = NULL;
 -static int apic_mapped;
 -
 -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);
 -object_property_set_link(OBJECT(dev), OBJECT(ENV_GET_CPU(env)), cpu,
 - error);
 -if (error_is_set(error)) {
 -qerror_report_err(error);
 -error_free(error);
 -exit(1);
 -}
 -qdev_init_nofail(dev);
 -
 -/* XXX: mapping more APICs at the same memory location */
 -if (apic_mapped == 0) {
 -/* NOTE: the APIC is directly connected to the CPU - it is not
 -   on the global memory bus. */
 -/* XXX: what if the base changes? */
 -sysbus_mmio_map(sysbus_from_qdev(dev), 0, MSI_ADDR_BASE);
 -apic_mapped = 1;
 -}
 -
 -return dev;
 -}
 -
  void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
  {
  CPUX86State *s = opaque;
 @@ -933,17 +894,12 @@ static void pc_cpu_reset(void *opaque)
  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;
 diff --git a/target-i386/cpu.c b/target-i386/cpu.c
 index 2610d96..b649904 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,11 @@
  
  #include hyperv.h
  
 +#include sysemu.h
 +#ifndef CONFIG_USER_ONLY
 +#include hw/sysbus.h
 +#endif
 +
  /* 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,13 +1755,69 @@ 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);
 +object_property_set_link(OBJECT(env-apic_state), OBJECT(cpu), cpu,
 + errp);
 +if (error_is_set(errp)) {
 +return;
 +}
 +}
 +#endif
 +}
 +
 +#ifndef CONFIG_USER_ONLY
 +#define MSI_ADDR_BASE 0xfee0
 +
 +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
 +{
 +static int apic_mapped;
 +

Re: [Qemu-devel] [PATCH qom-next 11/12] target-i386: initialize APIC at CPU level

2012-05-30 Thread Igor Mammedov

On 05/30/2012 12:41 PM, Jan Kiszka wrote:

On 2012-05-30 00:10, Igor Mammedov wrote:

(L)APIC is a part of cpu [1] so move APIC initialization inside of
x86_cpu object. Since cpu_model and override flags currently specify
whether APIC should be created or not, APIC creation is moved into
cpu_model property setter. And APIC initialization is moved into
x86_cpu_apic_init() which is called from x86_cpu_realize().

[1] - all x86 cpus have integrated APIC if we overlook existence of i486,
and it's more convenient to model after majority of them.

v2:
  * init APIC mapping at cpu level, due to Peter's objection to putting
it into APIC's initfn and Jan's suggestion to do it inside cpu.

Signed-off-by: Igor Mammedovimamm...@redhat.com
---
  hw/pc.c   |   44 -
  target-i386/cpu.c |   62 +
  2 files changed, 62 insertions(+), 44 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index d178801..986f119 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
@@ -70,8 +69,6 @@
  #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
  #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)

-#define MSI_ADDR_BASE 0xfee0
-
  #define E820_NR_ENTRIES   16

  struct e820_entry {
@@ -879,42 +876,6 @@ DeviceState *cpu_get_current_apic(void)
  }
  }

-static DeviceState *apic_init(void *env, uint8_t apic_id)
-{
-DeviceState *dev;
-Error *error = NULL;
-static int apic_mapped;
-
-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);
-object_property_set_link(OBJECT(dev), OBJECT(ENV_GET_CPU(env)), cpu,
-error);
-if (error_is_set(error)) {
-qerror_report_err(error);
-error_free(error);
-exit(1);
-}
-qdev_init_nofail(dev);
-
-/* XXX: mapping more APICs at the same memory location */
-if (apic_mapped == 0) {
-/* NOTE: the APIC is directly connected to the CPU - it is not
-   on the global memory bus. */
-/* XXX: what if the base changes? */
-sysbus_mmio_map(sysbus_from_qdev(dev), 0, MSI_ADDR_BASE);
-apic_mapped = 1;
-}
-
-return dev;
-}
-
  void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
  {
  CPUX86State *s = opaque;
@@ -933,17 +894,12 @@ static void pc_cpu_reset(void *opaque)
  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;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2610d96..b649904 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,11 @@

  #include hyperv.h

+#include sysemu.h
+#ifndef CONFIG_USER_ONLY
+#include hw/sysbus.h
+#endif
+
  /* 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,13 +1755,69 @@ 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);
+object_property_set_link(OBJECT(env-apic_state), OBJECT(cpu), cpu,
+ errp);
+if (error_is_set(errp)) {
+return;
+}
+}
+#endif
+}
+
+#ifndef CONFIG_USER_ONLY
+#define MSI_ADDR_BASE 0xfee0
+
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+static int apic_mapped;
+CPUX86State *env =cpu-env;
+
+if (env-apic_state == NULL) {
+return;
+}
+
+if 

Re: [Qemu-devel] [PATCH qom-next 11/12] target-i386: initialize APIC at CPU level

2012-05-30 Thread Jan Kiszka
On 2012-05-30 16:27, Igor Mammedov wrote:
 +
 +#ifndef CONFIG_USER_ONLY
 +#define MSI_ADDR_BASE 0xfee0
 +
 +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
 +{
 +static int apic_mapped;
 +CPUX86State *env =cpu-env;
 +
 +if (env-apic_state == NULL) {
 +return;
 +}
 +
 +if (qdev_init(env-apic_state)) {
 +error_set(errp, QERR_DEVICE_INIT_FAILED,
 +  object_get_typename(OBJECT(env-apic_state)));
 +return;
 +}
 +
 +/* XXX: mapping more APICs at the same memory location */
 +if (apic_mapped == 0) {
 +/* NOTE: the APIC is directly connected to the CPU - it is not
 +   on the global memory bus. */
 +/* XXX: what if the base changes? */
 +sysbus_mmio_map(sysbus_from_qdev(env-apic_state), 0, 
 MSI_ADDR_BASE);
 +apic_mapped = 1;
   }
   }
 +#endif

   void x86_cpu_realize(Object *obj, Error **errp)
   {
   X86CPU *cpu = X86_CPU(obj);

 +x86_cpu_apic_init(cpu, errp);
 +if (error_is_set(errp)) {
 +return;
 +}
 +

 I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the
 sake of bisectability). Or, likely better, let x86_cpu_apic_init do
 nothing for usermode emulation.
 initially x86_cpu_apic_init() was doing nothing for usermode, hence lack of
 #ifndef CONFIG_USER_ONLY, but then for patch 12/12 #ifndef CONFIG_USER_ONLY
 required here any way so I've removed usermode stub x86_cpu_apic_init() and
 squashed change in wrong patch :(.
 
 And since I need #ifndef in initfn anyway then probably there is no point
 in having x86_cpu_apic_init(), I'll move its body in initfn then.

I think a function is cleaner, and we have some other examples for this
already in this context. Its body could easily be #ifdef'ed out (the
other reason for #ifdef in the initfn are temporary, no?).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux



Re: [Qemu-devel] [PATCH qom-next 11/12] target-i386: initialize APIC at CPU level

2012-05-30 Thread Igor Mammedov

On 05/30/2012 04:38 PM, Jan Kiszka wrote:

On 2012-05-30 16:27, Igor Mammedov wrote:

+
+#ifndef CONFIG_USER_ONLY
+#define MSI_ADDR_BASE 0xfee0
+
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+static int apic_mapped;
+CPUX86State *env =cpu-env;
+
+if (env-apic_state == NULL) {
+return;
+}
+
+if (qdev_init(env-apic_state)) {
+error_set(errp, QERR_DEVICE_INIT_FAILED,
+  object_get_typename(OBJECT(env-apic_state)));
+return;
+}
+
+/* XXX: mapping more APICs at the same memory location */
+if (apic_mapped == 0) {
+/* NOTE: the APIC is directly connected to the CPU - it is not
+   on the global memory bus. */
+/* XXX: what if the base changes? */
+sysbus_mmio_map(sysbus_from_qdev(env-apic_state), 0, MSI_ADDR_BASE);
+apic_mapped = 1;
   }
   }
+#endif

   void x86_cpu_realize(Object *obj, Error **errp)
   {
   X86CPU *cpu = X86_CPU(obj);

+x86_cpu_apic_init(cpu, errp);
+if (error_is_set(errp)) {
+return;
+}
+


I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the
sake of bisectability). Or, likely better, let x86_cpu_apic_init do
nothing for usermode emulation.

initially x86_cpu_apic_init() was doing nothing for usermode, hence lack of
#ifndef CONFIG_USER_ONLY, but then for patch 12/12 #ifndef CONFIG_USER_ONLY
required here any way so I've removed usermode stub x86_cpu_apic_init() and
squashed change in wrong patch :(.

And since I need #ifndef in initfn anyway then probably there is no point
in having x86_cpu_apic_init(), I'll move its body in initfn then.


I think a function is cleaner, and we have some other examples for this
already in this context. Its body could easily be #ifdef'ed out (the
other reason for #ifdef in the initfn are temporary, no?).

Yes, is temporary.
However if Peter could be persuaded not to object for putting mapping of APIC 
base
into apic's initfn then x86_cpu_apic_init() would be temporary as well 
(qdev_init part
goes away with realize property and apic base mapping could be moved into it's
own property setter in apic object and default mapping set in its initfn).
Andreas seems liked putting it there.
Then overall code would look cleaner and with less ifdefs.



Jan



--
-
 Igor



Re: [Qemu-devel] [PATCH qom-next 11/12] target-i386: initialize APIC at CPU level

2012-05-30 Thread Jan Kiszka
On 2012-05-30 16:52, Igor Mammedov wrote:
 On 05/30/2012 04:38 PM, Jan Kiszka wrote:
 On 2012-05-30 16:27, Igor Mammedov wrote:
 +
 +#ifndef CONFIG_USER_ONLY
 +#define MSI_ADDR_BASE 0xfee0
 +
 +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
 +{
 +static int apic_mapped;
 +CPUX86State *env =cpu-env;
 +
 +if (env-apic_state == NULL) {
 +return;
 +}
 +
 +if (qdev_init(env-apic_state)) {
 +error_set(errp, QERR_DEVICE_INIT_FAILED,
 +  object_get_typename(OBJECT(env-apic_state)));
 +return;
 +}
 +
 +/* XXX: mapping more APICs at the same memory location */
 +if (apic_mapped == 0) {
 +/* NOTE: the APIC is directly connected to the CPU - it is not
 +   on the global memory bus. */
 +/* XXX: what if the base changes? */
 +sysbus_mmio_map(sysbus_from_qdev(env-apic_state), 0, 
 MSI_ADDR_BASE);
 +apic_mapped = 1;
}
}
 +#endif

void x86_cpu_realize(Object *obj, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);

 +x86_cpu_apic_init(cpu, errp);
 +if (error_is_set(errp)) {
 +return;
 +}
 +

 I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the
 sake of bisectability). Or, likely better, let x86_cpu_apic_init do
 nothing for usermode emulation.
 initially x86_cpu_apic_init() was doing nothing for usermode, hence lack of
 #ifndef CONFIG_USER_ONLY, but then for patch 12/12 #ifndef CONFIG_USER_ONLY
 required here any way so I've removed usermode stub x86_cpu_apic_init() and
 squashed change in wrong patch :(.

 And since I need #ifndef in initfn anyway then probably there is no point
 in having x86_cpu_apic_init(), I'll move its body in initfn then.

 I think a function is cleaner, and we have some other examples for this
 already in this context. Its body could easily be #ifdef'ed out (the
 other reason for #ifdef in the initfn are temporary, no?).
 Yes, is temporary.
 However if Peter could be persuaded not to object for putting mapping of APIC 
 base
 into apic's initfn then x86_cpu_apic_init() would be temporary as well 
 (qdev_init part
 goes away with realize property and apic base mapping could be moved into it's
 own property setter in apic object and default mapping set in its initfn).
 Andreas seems liked putting it there.
 Then overall code would look cleaner and with less ifdefs.

Unfortunately, the mapping belongs to the CPU because it actually
performs it as we pointed out.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux



Re: [Qemu-devel] [PATCH qom-next 11/12] target-i386: initialize APIC at CPU level

2012-05-30 Thread Igor Mammedov

On 05/30/2012 04:55 PM, Jan Kiszka wrote:

On 2012-05-30 16:52, Igor Mammedov wrote:

On 05/30/2012 04:38 PM, Jan Kiszka wrote:

On 2012-05-30 16:27, Igor Mammedov wrote:

+
+#ifndef CONFIG_USER_ONLY
+#define MSI_ADDR_BASE 0xfee0
+
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+static int apic_mapped;
+CPUX86State *env =cpu-env;
+
+if (env-apic_state == NULL) {
+return;
+}
+
+if (qdev_init(env-apic_state)) {
+error_set(errp, QERR_DEVICE_INIT_FAILED,
+  object_get_typename(OBJECT(env-apic_state)));
+return;
+}
+
+/* XXX: mapping more APICs at the same memory location */
+if (apic_mapped == 0) {
+/* NOTE: the APIC is directly connected to the CPU - it is not
+   on the global memory bus. */
+/* XXX: what if the base changes? */
+sysbus_mmio_map(sysbus_from_qdev(env-apic_state), 0, MSI_ADDR_BASE);
+apic_mapped = 1;
}
}
+#endif

void x86_cpu_realize(Object *obj, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);

+x86_cpu_apic_init(cpu, errp);
+if (error_is_set(errp)) {
+return;
+}
+


I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the
sake of bisectability). Or, likely better, let x86_cpu_apic_init do
nothing for usermode emulation.

initially x86_cpu_apic_init() was doing nothing for usermode, hence lack of
#ifndef CONFIG_USER_ONLY, but then for patch 12/12 #ifndef CONFIG_USER_ONLY
required here any way so I've removed usermode stub x86_cpu_apic_init() and
squashed change in wrong patch :(.

And since I need #ifndef in initfn anyway then probably there is no point
in having x86_cpu_apic_init(), I'll move its body in initfn then.


I think a function is cleaner, and we have some other examples for this
already in this context. Its body could easily be #ifdef'ed out (the
other reason for #ifdef in the initfn are temporary, no?).

Yes, is temporary.
However if Peter could be persuaded not to object for putting mapping of APIC 
base
into apic's initfn then x86_cpu_apic_init() would be temporary as well 
(qdev_init part
goes away with realize property and apic base mapping could be moved into it's
own property setter in apic object and default mapping set in its initfn).
Andreas seems liked putting it there.
Then overall code would look cleaner and with less ifdefs.


Unfortunately, the mapping belongs to the CPU because it actually
performs it as we pointed out.

Intel SDM tells only about RE-mapping that could be done via msr, but I haven't 
found
any mention about what part of CPU does default initial mapping (it might be 
APIC itself).
Docs just state for external and internal APICs that they are mapped at default 
base
right after power on or reset. :(

Any way moving APIC mapping code in it's own setter inside apic_common object 
might
be good idea on its own even if it's called from CPU to set initial mapping.

PS:
May be Intel guys could tell us how it's done in real hardware if it's not a 
secret.




Jan



--
-
 Igor



[Qemu-devel] [PATCH qom-next 11/12] target-i386: initialize APIC at CPU level

2012-05-29 Thread Igor Mammedov
(L)APIC is a part of cpu [1] so move APIC initialization inside of
x86_cpu object. Since cpu_model and override flags currently specify
whether APIC should be created or not, APIC creation is moved into
cpu_model property setter. And APIC initialization is moved into
x86_cpu_apic_init() which is called from x86_cpu_realize().

[1] - all x86 cpus have integrated APIC if we overlook existence of i486,
and it's more convenient to model after majority of them.

v2:
 * init APIC mapping at cpu level, due to Peter's objection to putting
   it into APIC's initfn and Jan's suggestion to do it inside cpu.

Signed-off-by: Igor Mammedov imamm...@redhat.com
---
 hw/pc.c   |   44 -
 target-i386/cpu.c |   62 +
 2 files changed, 62 insertions(+), 44 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index d178801..986f119 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
@@ -70,8 +69,6 @@
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
 #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
-#define MSI_ADDR_BASE 0xfee0
-
 #define E820_NR_ENTRIES16
 
 struct e820_entry {
@@ -879,42 +876,6 @@ DeviceState *cpu_get_current_apic(void)
 }
 }
 
-static DeviceState *apic_init(void *env, uint8_t apic_id)
-{
-DeviceState *dev;
-Error *error = NULL;
-static int apic_mapped;
-
-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);
-object_property_set_link(OBJECT(dev), OBJECT(ENV_GET_CPU(env)), cpu,
- error);
-if (error_is_set(error)) {
-qerror_report_err(error);
-error_free(error);
-exit(1);
-}
-qdev_init_nofail(dev);
-
-/* XXX: mapping more APICs at the same memory location */
-if (apic_mapped == 0) {
-/* NOTE: the APIC is directly connected to the CPU - it is not
-   on the global memory bus. */
-/* XXX: what if the base changes? */
-sysbus_mmio_map(sysbus_from_qdev(dev), 0, MSI_ADDR_BASE);
-apic_mapped = 1;
-}
-
-return dev;
-}
-
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 {
 CPUX86State *s = opaque;
@@ -933,17 +894,12 @@ static void pc_cpu_reset(void *opaque)
 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;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2610d96..b649904 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,11 @@
 
 #include hyperv.h
 
+#include sysemu.h
+#ifndef CONFIG_USER_ONLY
+#include hw/sysbus.h
+#endif
+
 /* 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,13 +1755,69 @@ 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);
+object_property_set_link(OBJECT(env-apic_state), OBJECT(cpu), cpu,
+ errp);
+if (error_is_set(errp)) {
+return;
+}
+}
+#endif
+}
+
+#ifndef CONFIG_USER_ONLY
+#define MSI_ADDR_BASE 0xfee0
+
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+static int apic_mapped;
+CPUX86State *env = cpu-env;
+
+if (env-apic_state == NULL) {
+return;
+}
+
+if (qdev_init(env-apic_state)) {
+error_set(errp, QERR_DEVICE_INIT_FAILED,
+