Patchwork [14/16] target-i386: move APIC to ICC bus

login
register
mail settings
Submitter Igor Mammedov
Date April 15, 2013, 10:12 p.m.
Message ID <1366063976-4909-15-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/236749/
State New
Headers show

Comments

Igor Mammedov - April 15, 2013, 10:12 p.m.
... to allow it to be hotplugged

 * map APIC's mmio at board level if it is present
 * do not register mmio region for each APIC, since
   only one is used/mapped

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/cpu/icc_bus.c                | 15 ++++++++++++++-
 hw/i386/kvmvapic.c              |  1 +
 hw/i386/pc.c                    | 20 +++++++++++++++++---
 hw/intc/apic_common.c           | 17 ++++++++++++-----
 include/hw/i386/apic_internal.h |  6 +++---
 include/hw/i386/icc_bus.h       |  2 ++
 target-i386/cpu.c               | 16 +++-------------
 7 files changed, 52 insertions(+), 25 deletions(-)
Andreas Färber - April 22, 2013, 3:18 p.m.
Am 16.04.2013 00:12, schrieb Igor Mammedov:
> ... to allow it to be hotplugged
> 
>  * map APIC's mmio at board level if it is present
>  * do not register mmio region for each APIC, since
>    only one is used/mapped
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/cpu/icc_bus.c                | 15 ++++++++++++++-
>  hw/i386/kvmvapic.c              |  1 +
>  hw/i386/pc.c                    | 20 +++++++++++++++++---
>  hw/intc/apic_common.c           | 17 ++++++++++++-----
>  include/hw/i386/apic_internal.h |  6 +++---
>  include/hw/i386/icc_bus.h       |  2 ++
>  target-i386/cpu.c               | 16 +++-------------
>  7 files changed, 52 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
> index 00b9be3..5078c38 100644
> --- a/hw/cpu/icc_bus.c
> +++ b/hw/cpu/icc_bus.c
> @@ -62,13 +62,26 @@ static const TypeInfo icc_device_info = {
>  
>  typedef struct ICCBridgeState {
>      SysBusDevice busdev;
> +    MemoryRegion apic_container;
>  } ICCBridgeState;
>  #define ICC_BRIGDE(obj) OBJECT_CHECK(ICCBridgeState, (obj), TYPE_ICC_BRIDGE)
>  
>  
>  static void icc_bridge_initfn(Object *obj)
>  {
> -    qbus_create(TYPE_ICC_BUS, DEVICE(obj), "icc-bus");
> +    ICCBridgeState *s = ICC_BRIGDE(obj);
> +    SysBusDevice *sb = SYS_BUS_DEVICE(obj);
> +    ICCBus *ibus;
> +
> +    ibus = ICC_BUS(qbus_create(TYPE_ICC_BUS, DEVICE(obj), "icc-bus"));

qbus_create_inplace() as suggested on ICC patch would allow to access it
through state field.

> +
> +    /* Do not change order of registering regions,
> +     * APIC must be first registered region, board maps it by 0 index
> +     */
> +    memory_region_init(&s->apic_container, "icc-apic-container",
> +                       APIC_SPACE_SIZE);
> +    sysbus_init_mmio(sb, &s->apic_container);
> +    ibus->apic_address_space = &s->apic_container;
>  }
>  
>  static const TypeInfo icc_bridge_info = {
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index 3a10c07..5b558aa 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -12,6 +12,7 @@
>  #include "sysemu/cpus.h"
>  #include "sysemu/kvm.h"
>  #include "hw/i386/apic_internal.h"
> +#include "hw/sysbus.h"
>  
>  #define VAPIC_IO_PORT           0x7e
>  

I take it, this is necessary due to dropping hw/sysbus.h include in
cpu.c? Might be split out then since patch is rather large already.

> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index cb57878..31b5294 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -52,6 +52,7 @@
>  #include "sysemu/arch_init.h"
>  #include "qemu/bitmap.h"
>  #include "qemu/config-file.h"
> +#include "hw/i386/icc_bus.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ

> @@ -889,13 +890,13 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>      }
>  }
>  
> -static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
> +static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
>  {
>      X86CPU *cpu;
>  
>      cpu = cpu_x86_create(cpu_model, errp);
>      if (!cpu) {
> -        return;
> +        return cpu;
>      }
>  
>      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp);
> @@ -904,14 +905,18 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
>      if (error_is_set(errp)) {
>          if (cpu != NULL) {
>              object_unref(OBJECT(cpu));
> +            cpu = NULL;
>          }
>      }
> +    return cpu;
>  }
>  

Squash this into the pc_new_cpu() introduction?

>  void pc_cpus_init(const char *cpu_model)
>  {
>      int i;
> +    X86CPU *cpu = NULL;
>      Error *error = NULL;
> +    SysBusDevice *ib;

icc maybe? Anything that more's comprehensible. ;)

>  
>      /* init CPUs */
>      if (cpu_model == NULL) {
> @@ -922,14 +927,23 @@ void pc_cpus_init(const char *cpu_model)
>  #endif
>      }
>  
> +    ib = SYS_BUS_DEVICE(object_resolve_path_type("icc-bridge",
> +                                                 TYPE_ICC_BRIDGE, NULL));
> +
>      for (i = 0; i < smp_cpus; i++) {
> -        pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
> +        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);

(Here it would've been nice too but would lead to assigned-but-not-used
warnings.)

>          if (error) {
>              fprintf(stderr, "%s\n", error_get_pretty(error));
>              error_free(error);
>              exit(1);
>          }
>      }
> +
> +    /* map APIC MMIO area if CPU has APIC */
> +    if (cpu && cpu->env.apic_state) {
> +        /* XXX: what if the base changes? */
> +        sysbus_mmio_map_overlap(ib, 0, APIC_DEFAULT_ADDRESS, 0x1000);
> +    }
>  }
>  
>  void pc_acpi_init(const char *default_dsdt)
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index e0ae07a..39396f1 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -21,6 +21,8 @@
>  #include "hw/i386/apic_internal.h"
>  #include "trace.h"
>  #include "sysemu/kvm.h"
> +#include "hw/qdev.h"
> +#include "hw/sysbus.h"
>  
>  static int apic_irq_delivered;
>  bool apic_report_tpr_access;
> @@ -282,12 +284,14 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> -static int apic_init_common(SysBusDevice *dev)
> +static int apic_init_common(ICCDevice *dev)

Instead of mechanically replacing SysBusDevice with ICCDevice in qdev
init functions, please change to realize. Could be its own prepended
patch if you prefer.

>  {
>      APICCommonState *s = APIC_COMMON(dev);
> +    DeviceState *d = DEVICE(dev);
>      APICCommonClass *info;
>      static DeviceState *vapic;
>      static int apic_no;
> +    static bool mmio_registered;
>  
>      if (apic_no >= MAX_APICS) {
>          return -1;
> @@ -296,8 +300,11 @@ static int apic_init_common(SysBusDevice *dev)
>  
>      info = APIC_COMMON_GET_CLASS(s);
>      info->init(s);
> -
> -    sysbus_init_mmio(dev, &s->io_memory);
> +    if (!mmio_registered) {
> +        MemoryRegion *as = ICC_BUS(d->parent_bus)->apic_address_space;
> +        memory_region_add_subregion(as, 0, &s->io_memory);
> +        mmio_registered = true;
> +    }
>  
>      /* Note: We need at least 1M to map the VAPIC option ROM */
>      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
> @@ -375,7 +382,7 @@ static Property apic_properties_common[] = {
>  
>  static void apic_common_class_init(ObjectClass *klass, void *data)
>  {
> -    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
> +    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);

idc please. :)

>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->vmsd = &vmstate_apic_common;
> @@ -387,7 +394,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
>  
>  static const TypeInfo apic_common_type = {
>      .name = TYPE_APIC_COMMON,
> -    .parent = TYPE_SYS_BUS_DEVICE,
> +    .parent = TYPE_ICC_DEVICE,
>      .instance_size = sizeof(APICCommonState),
>      .class_size = sizeof(APICCommonClass),
>      .class_init = apic_common_class_init,
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index aac6290..05acf4b 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -21,7 +21,7 @@
>  #define QEMU_APIC_INTERNAL_H
>  
>  #include "exec/memory.h"
> -#include "hw/sysbus.h"
> +#include "hw/i386/icc_bus.h"
>  #include "qemu/timer.h"
>  
>  /* APIC Local Vector Table */
> @@ -78,7 +78,7 @@ typedef struct APICCommonState APICCommonState;
>  
>  typedef struct APICCommonClass
>  {
> -    SysBusDeviceClass parent_class;
> +    ICCDeviceClass parent_class;
>  
>      void (*init)(APICCommonState *s);
>      void (*set_base)(APICCommonState *s, uint64_t val);
> @@ -92,7 +92,7 @@ typedef struct APICCommonClass
>  } APICCommonClass;
>  
>  struct APICCommonState {
> -    SysBusDevice busdev;
> +    ICCDevice busdev;

parent_obj please, while at it.

>  
>      MemoryRegion io_memory;
>      X86CPU *cpu;
> diff --git a/include/hw/i386/icc_bus.h b/include/hw/i386/icc_bus.h
> index aa30e0c..1b07619 100644
> --- a/include/hw/i386/icc_bus.h
> +++ b/include/hw/i386/icc_bus.h
> @@ -22,6 +22,7 @@
>  #ifndef ICC_BUS_H
>  #define ICC_BUS_H
>  
> +#include "exec/memory.h"
>  #include "hw/qdev-core.h"
>  
>  #define TYPE_ICC_BUS "icc-bus"
> @@ -29,6 +30,7 @@
>  #ifndef CONFIG_USER_ONLY
>  typedef struct ICCBus {
>      BusState qbus;
> +    MemoryRegion *apic_address_space;
>  } ICCBus;
>  #define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
>  
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3b5f90b..bdac6ea 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -41,10 +41,10 @@
>  #endif
>  
>  #include "sysemu/sysemu.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/i386/icc_bus.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "hw/xen/xen.h"
> -#include "hw/sysbus.h"
>  #include "hw/i386/apic_internal.h"
>  #endif
>  
> @@ -2111,6 +2111,7 @@ static void mce_init(X86CPU *cpu)
>  static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>  {
>      CPUX86State *env = &cpu->env;
> +    DeviceState *dev = DEVICE(cpu);
>      APICCommonState *apic;
>      const char *apic_type = "apic";
>  
> @@ -2120,7 +2121,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>          apic_type = "xen-apic";
>      }
>  
> -    env->apic_state = qdev_try_create(NULL, apic_type);
> +    env->apic_state = qdev_try_create(dev->parent_bus, apic_type);

qdev_get_parent_bus() according to header comment.

>      if (env->apic_state == NULL) {
>          error_setg(errp, "APIC device '%s' could not be created", apic_type);
>          return;
> @@ -2137,7 +2138,6 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>  {
>      CPUX86State *env = &cpu->env;
> -    static int apic_mapped;
>  
>      if (env->apic_state == NULL) {
>          return;
> @@ -2148,16 +2148,6 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>                     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_overlap(SYS_BUS_DEVICE(env->apic_state), 0,
> -                                APIC_DEFAULT_ADDRESS, 0x1000);
> -        apic_mapped = 1;
> -    }
>  }
>  #else
>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
> 

Andreas
Igor Mammedov - April 22, 2013, 3:59 p.m.
On Mon, 22 Apr 2013 17:18:58 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 16.04.2013 00:12, schrieb Igor Mammedov:
[...]

> >  static void icc_bridge_initfn(Object *obj)
> >  {
> > -    qbus_create(TYPE_ICC_BUS, DEVICE(obj), "icc-bus");
> > +    ICCBridgeState *s = ICC_BRIGDE(obj);
> > +    SysBusDevice *sb = SYS_BUS_DEVICE(obj);
> > +    ICCBus *ibus;
> > +
> > +    ibus = ICC_BUS(qbus_create(TYPE_ICC_BUS, DEVICE(obj), "icc-bus"));
> 
> qbus_create_inplace() as suggested on ICC patch would allow to access it
> through state field.
done

> 
> > +
> > +    /* Do not change order of registering regions,
> > +     * APIC must be first registered region, board maps it by 0 index
> > +     */
> > +    memory_region_init(&s->apic_container, "icc-apic-container",
> > +                       APIC_SPACE_SIZE);
> > +    sysbus_init_mmio(sb, &s->apic_container);
> > +    ibus->apic_address_space = &s->apic_container;
> >  }
> >  
> >  static const TypeInfo icc_bridge_info = {
> > diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> > index 3a10c07..5b558aa 100644
> > --- a/hw/i386/kvmvapic.c
> > +++ b/hw/i386/kvmvapic.c
> > @@ -12,6 +12,7 @@
> >  #include "sysemu/cpus.h"
> >  #include "sysemu/kvm.h"
> >  #include "hw/i386/apic_internal.h"
> > +#include "hw/sysbus.h"
> >  
> >  #define VAPIC_IO_PORT           0x7e
> >  
> 
> I take it, this is necessary due to dropping hw/sysbus.h include in
> cpu.c? Might be split out then since patch is rather large already.
It's due to dropping  hw/sysbus.h from include/hw/i386/apic_internal.h
It's hard to split in meaningful separate patch.

> 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index cb57878..31b5294 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -52,6 +52,7 @@
> >  #include "sysemu/arch_init.h"
> >  #include "qemu/bitmap.h"
> >  #include "qemu/config-file.h"
> > +#include "hw/i386/icc_bus.h"
> >  
> >  /* debug PC/ISA interrupts */
> >  //#define DEBUG_IRQ
> 
> > @@ -889,13 +890,13 @@ void pc_acpi_smi_interrupt(void *opaque, int irq,
> > int level) }
> >  }
> >  
> > -static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error
> > **errp) +static X86CPU *pc_new_cpu(const char *cpu_model, int64_t
> > apic_id, Error **errp) {
> >      X86CPU *cpu;
> >  
> >      cpu = cpu_x86_create(cpu_model, errp);
> >      if (!cpu) {
> > -        return;
> > +        return cpu;
> >      }
> >  
> >      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp);
> > @@ -904,14 +905,18 @@ static void pc_new_cpu(const char *cpu_model,
> > int64_t apic_id, Error **errp) if (error_is_set(errp)) {
> >          if (cpu != NULL) {
> >              object_unref(OBJECT(cpu));
> > +            cpu = NULL;
> >          }
> >      }
> > +    return cpu;
> >  }
> >  
> 
> Squash this into the pc_new_cpu() introduction?
sure, it's less obvious why it would be needed but makes this patch smaller.

> 
> >  void pc_cpus_init(const char *cpu_model)
> >  {
> >      int i;
> > +    X86CPU *cpu = NULL;
> >      Error *error = NULL;
> > +    SysBusDevice *ib;
> 
> icc maybe? Anything that more's comprehensible. ;)
icc_bridge then.

[...]
> > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> > index e0ae07a..39396f1 100644
> > --- a/hw/intc/apic_common.c
> > +++ b/hw/intc/apic_common.c
> > @@ -21,6 +21,8 @@
> >  #include "hw/i386/apic_internal.h"
> >  #include "trace.h"
> >  #include "sysemu/kvm.h"
> > +#include "hw/qdev.h"
> > +#include "hw/sysbus.h"
> >  
> >  static int apic_irq_delivered;
> >  bool apic_report_tpr_access;
> > @@ -282,12 +284,14 @@ static int apic_load_old(QEMUFile *f, void *opaque,
> > int version_id) return 0;
> >  }
> >  
> > -static int apic_init_common(SysBusDevice *dev)
> > +static int apic_init_common(ICCDevice *dev)
> 
> Instead of mechanically replacing SysBusDevice with ICCDevice in qdev
> init functions, please change to realize. Could be its own prepended
> patch if you prefer.

IRC with Andreas, agreed to do it only for ICCDevice. And do re-factoring
of init() in APICs/IOAPIC later.

> 
> >  {
> >      APICCommonState *s = APIC_COMMON(dev);
> > +    DeviceState *d = DEVICE(dev);
> >      APICCommonClass *info;
> >      static DeviceState *vapic;
> >      static int apic_no;
> > +    static bool mmio_registered;
> >  
> >      if (apic_no >= MAX_APICS) {
> >          return -1;
[...]

> > @@ -375,7 +382,7 @@ static Property apic_properties_common[] = {
> >  
> >  static void apic_common_class_init(ObjectClass *klass, void *data)
> >  {
> > -    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
> > +    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
> 
> idc please. :)
done

> 
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> >      dc->vmsd = &vmstate_apic_common;
> > @@ -387,7 +394,7 @@ static void apic_common_class_init(ObjectClass
> > *klass, void *data) 
> >  static const TypeInfo apic_common_type = {
[...]

> > @@ -2120,7 +2121,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error
> > **errp) apic_type = "xen-apic";
> >      }
> >  
> > -    env->apic_state = qdev_try_create(NULL, apic_type);
> > +    env->apic_state = qdev_try_create(dev->parent_bus, apic_type);
> 
> qdev_get_parent_bus() according to header comment.
done

> 
> >      if (env->apic_state == NULL) {
[...]
> Andreas
>

Patch

diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index 00b9be3..5078c38 100644
--- a/hw/cpu/icc_bus.c
+++ b/hw/cpu/icc_bus.c
@@ -62,13 +62,26 @@  static const TypeInfo icc_device_info = {
 
 typedef struct ICCBridgeState {
     SysBusDevice busdev;
+    MemoryRegion apic_container;
 } ICCBridgeState;
 #define ICC_BRIGDE(obj) OBJECT_CHECK(ICCBridgeState, (obj), TYPE_ICC_BRIDGE)
 
 
 static void icc_bridge_initfn(Object *obj)
 {
-    qbus_create(TYPE_ICC_BUS, DEVICE(obj), "icc-bus");
+    ICCBridgeState *s = ICC_BRIGDE(obj);
+    SysBusDevice *sb = SYS_BUS_DEVICE(obj);
+    ICCBus *ibus;
+
+    ibus = ICC_BUS(qbus_create(TYPE_ICC_BUS, DEVICE(obj), "icc-bus"));
+
+    /* Do not change order of registering regions,
+     * APIC must be first registered region, board maps it by 0 index
+     */
+    memory_region_init(&s->apic_container, "icc-apic-container",
+                       APIC_SPACE_SIZE);
+    sysbus_init_mmio(sb, &s->apic_container);
+    ibus->apic_address_space = &s->apic_container;
 }
 
 static const TypeInfo icc_bridge_info = {
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 3a10c07..5b558aa 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -12,6 +12,7 @@ 
 #include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
 #include "hw/i386/apic_internal.h"
+#include "hw/sysbus.h"
 
 #define VAPIC_IO_PORT           0x7e
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index cb57878..31b5294 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -52,6 +52,7 @@ 
 #include "sysemu/arch_init.h"
 #include "qemu/bitmap.h"
 #include "qemu/config-file.h"
+#include "hw/i386/icc_bus.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -889,13 +890,13 @@  void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
+static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
 {
     X86CPU *cpu;
 
     cpu = cpu_x86_create(cpu_model, errp);
     if (!cpu) {
-        return;
+        return cpu;
     }
 
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp);
@@ -904,14 +905,18 @@  static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
     if (error_is_set(errp)) {
         if (cpu != NULL) {
             object_unref(OBJECT(cpu));
+            cpu = NULL;
         }
     }
+    return cpu;
 }
 
 void pc_cpus_init(const char *cpu_model)
 {
     int i;
+    X86CPU *cpu = NULL;
     Error *error = NULL;
+    SysBusDevice *ib;
 
     /* init CPUs */
     if (cpu_model == NULL) {
@@ -922,14 +927,23 @@  void pc_cpus_init(const char *cpu_model)
 #endif
     }
 
+    ib = SYS_BUS_DEVICE(object_resolve_path_type("icc-bridge",
+                                                 TYPE_ICC_BRIDGE, NULL));
+
     for (i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
+        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
         if (error) {
             fprintf(stderr, "%s\n", error_get_pretty(error));
             error_free(error);
             exit(1);
         }
     }
+
+    /* map APIC MMIO area if CPU has APIC */
+    if (cpu && cpu->env.apic_state) {
+        /* XXX: what if the base changes? */
+        sysbus_mmio_map_overlap(ib, 0, APIC_DEFAULT_ADDRESS, 0x1000);
+    }
 }
 
 void pc_acpi_init(const char *default_dsdt)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index e0ae07a..39396f1 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -21,6 +21,8 @@ 
 #include "hw/i386/apic_internal.h"
 #include "trace.h"
 #include "sysemu/kvm.h"
+#include "hw/qdev.h"
+#include "hw/sysbus.h"
 
 static int apic_irq_delivered;
 bool apic_report_tpr_access;
@@ -282,12 +284,14 @@  static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static int apic_init_common(SysBusDevice *dev)
+static int apic_init_common(ICCDevice *dev)
 {
     APICCommonState *s = APIC_COMMON(dev);
+    DeviceState *d = DEVICE(dev);
     APICCommonClass *info;
     static DeviceState *vapic;
     static int apic_no;
+    static bool mmio_registered;
 
     if (apic_no >= MAX_APICS) {
         return -1;
@@ -296,8 +300,11 @@  static int apic_init_common(SysBusDevice *dev)
 
     info = APIC_COMMON_GET_CLASS(s);
     info->init(s);
-
-    sysbus_init_mmio(dev, &s->io_memory);
+    if (!mmio_registered) {
+        MemoryRegion *as = ICC_BUS(d->parent_bus)->apic_address_space;
+        memory_region_add_subregion(as, 0, &s->io_memory);
+        mmio_registered = true;
+    }
 
     /* Note: We need at least 1M to map the VAPIC option ROM */
     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
@@ -375,7 +382,7 @@  static Property apic_properties_common[] = {
 
 static void apic_common_class_init(ObjectClass *klass, void *data)
 {
-    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
+    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_apic_common;
@@ -387,7 +394,7 @@  static void apic_common_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo apic_common_type = {
     .name = TYPE_APIC_COMMON,
-    .parent = TYPE_SYS_BUS_DEVICE,
+    .parent = TYPE_ICC_DEVICE,
     .instance_size = sizeof(APICCommonState),
     .class_size = sizeof(APICCommonClass),
     .class_init = apic_common_class_init,
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index aac6290..05acf4b 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -21,7 +21,7 @@ 
 #define QEMU_APIC_INTERNAL_H
 
 #include "exec/memory.h"
-#include "hw/sysbus.h"
+#include "hw/i386/icc_bus.h"
 #include "qemu/timer.h"
 
 /* APIC Local Vector Table */
@@ -78,7 +78,7 @@  typedef struct APICCommonState APICCommonState;
 
 typedef struct APICCommonClass
 {
-    SysBusDeviceClass parent_class;
+    ICCDeviceClass parent_class;
 
     void (*init)(APICCommonState *s);
     void (*set_base)(APICCommonState *s, uint64_t val);
@@ -92,7 +92,7 @@  typedef struct APICCommonClass
 } APICCommonClass;
 
 struct APICCommonState {
-    SysBusDevice busdev;
+    ICCDevice busdev;
 
     MemoryRegion io_memory;
     X86CPU *cpu;
diff --git a/include/hw/i386/icc_bus.h b/include/hw/i386/icc_bus.h
index aa30e0c..1b07619 100644
--- a/include/hw/i386/icc_bus.h
+++ b/include/hw/i386/icc_bus.h
@@ -22,6 +22,7 @@ 
 #ifndef ICC_BUS_H
 #define ICC_BUS_H
 
+#include "exec/memory.h"
 #include "hw/qdev-core.h"
 
 #define TYPE_ICC_BUS "icc-bus"
@@ -29,6 +30,7 @@ 
 #ifndef CONFIG_USER_ONLY
 typedef struct ICCBus {
     BusState qbus;
+    MemoryRegion *apic_address_space;
 } ICCBus;
 #define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3b5f90b..bdac6ea 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -41,10 +41,10 @@ 
 #endif
 
 #include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
 #include "hw/i386/icc_bus.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/xen/xen.h"
-#include "hw/sysbus.h"
 #include "hw/i386/apic_internal.h"
 #endif
 
@@ -2111,6 +2111,7 @@  static void mce_init(X86CPU *cpu)
 static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
     CPUX86State *env = &cpu->env;
+    DeviceState *dev = DEVICE(cpu);
     APICCommonState *apic;
     const char *apic_type = "apic";
 
@@ -2120,7 +2121,7 @@  static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
         apic_type = "xen-apic";
     }
 
-    env->apic_state = qdev_try_create(NULL, apic_type);
+    env->apic_state = qdev_try_create(dev->parent_bus, apic_type);
     if (env->apic_state == NULL) {
         error_setg(errp, "APIC device '%s' could not be created", apic_type);
         return;
@@ -2137,7 +2138,6 @@  static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 {
     CPUX86State *env = &cpu->env;
-    static int apic_mapped;
 
     if (env->apic_state == NULL) {
         return;
@@ -2148,16 +2148,6 @@  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
                    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_overlap(SYS_BUS_DEVICE(env->apic_state), 0,
-                                APIC_DEFAULT_ADDRESS, 0x1000);
-        apic_mapped = 1;
-    }
 }
 #else
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)