diff mbox

[1/2] cpu/apic: drop icc bus/bridge/

Message ID 1426583311-29455-1-git-send-email-chen.fan.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

chenfan March 17, 2015, 9:08 a.m. UTC
ICC bus was invented only to provide hotplug capability to
CPU and APIC because at the time being hotplug was available only for
BUS attached devices.

Now this patch is to drop ICC bus impl, and switch to bus-less
CPU+APIC hotplug, handling them in the same manner as pc-dimm.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c                    | 21 +++++----------------
 hw/i386/pc_piix.c               |  9 +--------
 hw/i386/pc_q35.c                |  9 +--------
 hw/intc/apic.c                  |  6 +++---
 hw/intc/apic_common.c           | 24 +++++++++++++++++++-----
 include/hw/i386/apic_internal.h |  5 ++---
 include/hw/i386/pc.h            |  2 +-
 target-i386/cpu.c               | 15 ++-------------
 target-i386/cpu.h               |  2 +-
 9 files changed, 35 insertions(+), 58 deletions(-)

Comments

Andreas Färber March 17, 2015, 10:39 a.m. UTC | #1
Am 17.03.2015 um 10:08 schrieb Chen Fan:
> ICC bus was invented only to provide hotplug capability to
> CPU and APIC because at the time being hotplug was available only for
> BUS attached devices.
> 
> Now this patch is to drop ICC bus impl, and switch to bus-less
> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c                    | 21 +++++----------------
>  hw/i386/pc_piix.c               |  9 +--------
>  hw/i386/pc_q35.c                |  9 +--------
>  hw/intc/apic.c                  |  6 +++---
>  hw/intc/apic_common.c           | 24 +++++++++++++++++++-----
>  include/hw/i386/apic_internal.h |  5 ++---
>  include/hw/i386/pc.h            |  2 +-
>  target-i386/cpu.c               | 15 ++-------------
>  target-i386/cpu.h               |  2 +-
>  9 files changed, 35 insertions(+), 58 deletions(-)

This mini-series is lacking a cover letter and a change log.
Please also find a better subject for this one: Luckily you are not just
dropping things but rather implementing the new hotplug interface.

Is this a resend from Zhu Guihua's series or did anything change?
I think for Soft Freeze and Hard Freeze coming up today it's too late.

As you may have seen, I've posted a test case to facilitate testing of
the various proposed code movements.

Regards,
Andreas
Igor Mammedov March 17, 2015, 4:29 p.m. UTC | #2
On Tue, 17 Mar 2015 17:08:30 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> ICC bus was invented only to provide hotplug capability to
> CPU and APIC because at the time being hotplug was available only for
> BUS attached devices.
> 
> Now this patch is to drop ICC bus impl, and switch to bus-less
> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---

[...]

> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 0858b45..020219e 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -27,6 +27,8 @@
>  static int apic_irq_delivered;
>  bool apic_report_tpr_access;
>  
> +static MemoryRegion *apic_container;
> +
>  void cpu_set_apic_base(DeviceState *dev, uint64_t val)
>  {
>      trace_cpu_set_apic_base(val);
> @@ -314,8 +316,11 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>      info = APIC_COMMON_GET_CLASS(s);
>      info->realize(dev, errp);
>      if (!mmio_registered) {
> -        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
> -        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
> +        memory_region_add_subregion(apic_container, 0, &s->io_memory);
> +        memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
> +                                            APIC_DEFAULT_ADDRESS,
> +                                            apic_container,
> +                                            0x1000);
>          mmio_registered = true;
>      }
Provided per CPU address spaces are used,
why do you need static apic_container and mmio_registered thing?
Shouldn't it be possible to register &s->io_memory of each APIC
in CPU's own AS and avoid using globals?


>  
> @@ -431,15 +436,23 @@ static Property apic_properties_common[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static void apic_common_initfn(Object *obj)
> +{
> +    if (!apic_container) {
> +        apic_container = g_malloc(sizeof(*apic_container));
> +        memory_region_init(apic_container, obj, "apic-container",
> +                           APIC_SPACE_SIZE);
> +    }
> +}
> +
chenfan March 18, 2015, 1:16 a.m. UTC | #3
On 03/17/2015 06:39 PM, Andreas Färber wrote:
> Am 17.03.2015 um 10:08 schrieb Chen Fan:
>> ICC bus was invented only to provide hotplug capability to
>> CPU and APIC because at the time being hotplug was available only for
>> BUS attached devices.
>>
>> Now this patch is to drop ICC bus impl, and switch to bus-less
>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/i386/pc.c                    | 21 +++++----------------
>>   hw/i386/pc_piix.c               |  9 +--------
>>   hw/i386/pc_q35.c                |  9 +--------
>>   hw/intc/apic.c                  |  6 +++---
>>   hw/intc/apic_common.c           | 24 +++++++++++++++++++-----
>>   include/hw/i386/apic_internal.h |  5 ++---
>>   include/hw/i386/pc.h            |  2 +-
>>   target-i386/cpu.c               | 15 ++-------------
>>   target-i386/cpu.h               |  2 +-
>>   9 files changed, 35 insertions(+), 58 deletions(-)
> This mini-series is lacking a cover letter and a change log.
> Please also find a better subject for this one: Luckily you are not just
> dropping things but rather implementing the new hotplug interface.
>
> Is this a resend from Zhu Guihua's series or did anything change?
> I think for Soft Freeze and Hard Freeze coming up today it's too late.
I picked up this two patches from Zhu Guihua's series separately and made
some changed to better adapt new interface.

>
> As you may have seen, I've posted a test case to facilitate testing of
> the various proposed code movements.
yes, I would use it to test my later version.

Thanks,
Chen

>
> Regards,
> Andreas
>
chenfan March 19, 2015, 9:07 a.m. UTC | #4
On 03/17/2015 06:39 PM, Andreas Färber wrote:
> Am 17.03.2015 um 10:08 schrieb Chen Fan:
>> ICC bus was invented only to provide hotplug capability to
>> CPU and APIC because at the time being hotplug was available only for
>> BUS attached devices.
>>
>> Now this patch is to drop ICC bus impl, and switch to bus-less
>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/i386/pc.c                    | 21 +++++----------------
>>   hw/i386/pc_piix.c               |  9 +--------
>>   hw/i386/pc_q35.c                |  9 +--------
>>   hw/intc/apic.c                  |  6 +++---
>>   hw/intc/apic_common.c           | 24 +++++++++++++++++++-----
>>   include/hw/i386/apic_internal.h |  5 ++---
>>   include/hw/i386/pc.h            |  2 +-
>>   target-i386/cpu.c               | 15 ++-------------
>>   target-i386/cpu.h               |  2 +-
>>   9 files changed, 35 insertions(+), 58 deletions(-)
> This mini-series is lacking a cover letter and a change log.
> Please also find a better subject for this one: Luckily you are not just
> dropping things but rather implementing the new hotplug interface.
>
> Is this a resend from Zhu Guihua's series or did anything change?
> I think for Soft Freeze and Hard Freeze coming up today it's too late.
>
> As you may have seen, I've posted a test case to facilitate testing of
> the various proposed code movements.
when I use 'make check' to run the test case. seems failure.

GTESTER check-qtest-x86_64
blkdebug: Suspended request 'A'
blkdebug: Resuming request 'A'
main-loop: WARNING: I/O thread spun for 1000 iterations
main-loop: WARNING: I/O thread spun for 1000 iterations
main-loop: WARNING: I/O thread spun for 1000 iterations
main-loop: WARNING: I/O thread spun for 1000 iterations
[vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio extension. 
Task offloads will be emulated.

Thanks,
Chen


>
> Regards,
> Andreas
>
chenfan March 19, 2015, 10:08 a.m. UTC | #5
On 03/18/2015 12:29 AM, Igor Mammedov wrote:
> On Tue, 17 Mar 2015 17:08:30 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>
>> ICC bus was invented only to provide hotplug capability to
>> CPU and APIC because at the time being hotplug was available only for
>> BUS attached devices.
>>
>> Now this patch is to drop ICC bus impl, and switch to bus-less
>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
> [...]
>
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>> index 0858b45..020219e 100644
>> --- a/hw/intc/apic_common.c
>> +++ b/hw/intc/apic_common.c
>> @@ -27,6 +27,8 @@
>>   static int apic_irq_delivered;
>>   bool apic_report_tpr_access;
>>   
>> +static MemoryRegion *apic_container;
>> +
>>   void cpu_set_apic_base(DeviceState *dev, uint64_t val)
>>   {
>>       trace_cpu_set_apic_base(val);
>> @@ -314,8 +316,11 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>>       info = APIC_COMMON_GET_CLASS(s);
>>       info->realize(dev, errp);
>>       if (!mmio_registered) {
>> -        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
>> -        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
>> +        memory_region_add_subregion(apic_container, 0, &s->io_memory);
>> +        memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
>> +                                            APIC_DEFAULT_ADDRESS,
>> +                                            apic_container,
>> +                                            0x1000);
>>           mmio_registered = true;
>>       }
> Provided per CPU address spaces are used,
> why do you need static apic_container and mmio_registered thing?
> Shouldn't it be possible to register &s->io_memory of each APIC
> in CPU's own AS and avoid using globals?
it seems only need to do
memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
                                             APIC_DEFAULT_ADDRESS,
                                             &io_memory,
                                             0x1000);

at here, right?

Thanks,
Chen
>
>>   
>> @@ -431,15 +436,23 @@ static Property apic_properties_common[] = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> +static void apic_common_initfn(Object *obj)
>> +{
>> +    if (!apic_container) {
>> +        apic_container = g_malloc(sizeof(*apic_container));
>> +        memory_region_init(apic_container, obj, "apic-container",
>> +                           APIC_SPACE_SIZE);
>> +    }
>> +}
>> +
>
>
> .
>
Igor Mammedov March 19, 2015, 10:28 a.m. UTC | #6
On Thu, 19 Mar 2015 18:08:32 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> 
> On 03/18/2015 12:29 AM, Igor Mammedov wrote:
> > On Tue, 17 Mar 2015 17:08:30 +0800
> > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> >
> >> ICC bus was invented only to provide hotplug capability to
> >> CPU and APIC because at the time being hotplug was available only for
> >> BUS attached devices.
> >>
> >> Now this patch is to drop ICC bus impl, and switch to bus-less
> >> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
> >>
> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> ---
> > [...]
> >
> >> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> >> index 0858b45..020219e 100644
> >> --- a/hw/intc/apic_common.c
> >> +++ b/hw/intc/apic_common.c
> >> @@ -27,6 +27,8 @@
> >>   static int apic_irq_delivered;
> >>   bool apic_report_tpr_access;
> >>   
> >> +static MemoryRegion *apic_container;
> >> +
> >>   void cpu_set_apic_base(DeviceState *dev, uint64_t val)
> >>   {
> >>       trace_cpu_set_apic_base(val);
> >> @@ -314,8 +316,11 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
> >>       info = APIC_COMMON_GET_CLASS(s);
> >>       info->realize(dev, errp);
> >>       if (!mmio_registered) {
> >> -        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
> >> -        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
> >> +        memory_region_add_subregion(apic_container, 0, &s->io_memory);
> >> +        memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
> >> +                                            APIC_DEFAULT_ADDRESS,
> >> +                                            apic_container,
> >> +                                            0x1000);
> >>           mmio_registered = true;
> >>       }
> > Provided per CPU address spaces are used,
> > why do you need static apic_container and mmio_registered thing?
> > Shouldn't it be possible to register &s->io_memory of each APIC
> > in CPU's own AS and avoid using globals?
> it seems only need to do
> memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
>                                              APIC_DEFAULT_ADDRESS,
>                                              &io_memory,
>                                              0x1000);
> 
> at here, right?
yep, something along theses lines.

I also do not remember if we really need it for every APIC device
that inherits from APIC_COMMON.It might be that kvm_iqrchip don't need it at all.
You  could experiment with -machine kernel_irqchip=on/off options
to switch between QEMU emulated APIC and kernel_irqchip.

Also this change is independent of dropping icc_bridge,
so please split it out into a separate patch. 


> 
> Thanks,
> Chen
> >
> >>   
> >> @@ -431,15 +436,23 @@ static Property apic_properties_common[] = {
> >>       DEFINE_PROP_END_OF_LIST(),
> >>   };
> >>   
> >> +static void apic_common_initfn(Object *obj)
> >> +{
> >> +    if (!apic_container) {
> >> +        apic_container = g_malloc(sizeof(*apic_container));
> >> +        memory_region_init(apic_container, obj, "apic-container",
> >> +                           APIC_SPACE_SIZE);
> >> +    }
> >> +}
> >> +
> >
> >
> > .
> >
>
Andreas Färber March 19, 2015, 11:49 a.m. UTC | #7
Am 19.03.2015 um 10:07 schrieb Chen Fan:
> when I use 'make check' to run the test case. seems failure.
> 
> GTESTER check-qtest-x86_64
> blkdebug: Suspended request 'A'
> blkdebug: Resuming request 'A'
> main-loop: WARNING: I/O thread spun for 1000 iterations
> main-loop: WARNING: I/O thread spun for 1000 iterations
> main-loop: WARNING: I/O thread spun for 1000 iterations
> main-loop: WARNING: I/O thread spun for 1000 iterations
> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio extension.
> Task offloads will be emulated.

That's not an error. Use make check-qtest-x86_64 V=1 to see details.

Regards,
Andreas
diff mbox

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b5b2aad..660e244 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -57,7 +57,6 @@ 
 #include "qemu/config-file.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/cpu_hotplug.h"
-#include "hw/cpu/icc_bus.h"
 #include "hw/boards.h"
 #include "hw/pci/pci_host.h"
 #include "acpi-build.h"
@@ -990,12 +989,12 @@  void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 }
 
 static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
-                          DeviceState *icc_bridge, Error **errp)
+                          Error **errp)
 {
     X86CPU *cpu;
     Error *local_err = NULL;
 
-    cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
+    cpu = cpu_x86_create(cpu_model, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         return NULL;
@@ -1016,7 +1015,6 @@  static const char *current_cpu_model;
 
 void pc_hot_add_cpu(const int64_t id, Error **errp)
 {
-    DeviceState *icc_bridge;
     int64_t apic_id = x86_cpu_apic_id_from_index(id);
 
     if (id < 0) {
@@ -1043,12 +1041,10 @@  void pc_hot_add_cpu(const int64_t id, Error **errp)
         return;
     }
 
-    icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
-                                                 TYPE_ICC_BRIDGE, NULL));
-    pc_new_cpu(current_cpu_model, apic_id, icc_bridge, errp);
+    pc_new_cpu(current_cpu_model, apic_id, errp);
 }
 
-void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
+void pc_cpus_init(const char *cpu_model)
 {
     int i;
     X86CPU *cpu = NULL;
@@ -1074,20 +1070,13 @@  void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
 
     for (i = 0; i < smp_cpus; i++) {
         cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
-                         icc_bridge, &error);
+                         &error);
         if (error) {
             error_report_err(error);
             exit(1);
         }
     }
 
-    /* map APIC MMIO area if CPU has APIC */
-    if (cpu && cpu->apic_state) {
-        /* XXX: what if the base changes? */
-        sysbus_mmio_map_overlap(SYS_BUS_DEVICE(icc_bridge), 0,
-                                APIC_DEFAULT_ADDRESS, 0x1000);
-    }
-
     /* tell smbios about cpuid version and features */
     smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
 }
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8eab4ba..cc9adee 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -39,7 +39,6 @@ 
 #include "hw/kvm/clock.h"
 #include "sysemu/sysemu.h"
 #include "hw/sysbus.h"
-#include "hw/cpu/icc_bus.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/block-backend.h"
 #include "hw/i2c/smbus.h"
@@ -98,7 +97,6 @@  static void pc_init1(MachineState *machine,
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
-    DeviceState *icc_bridge;
     FWCfgState *fw_cfg = NULL;
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
@@ -143,11 +141,7 @@  static void pc_init1(MachineState *machine,
         exit(1);
     }
 
-    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
-    object_property_add_child(qdev_get_machine(), "icc-bridge",
-                              OBJECT(icc_bridge), NULL);
-
-    pc_cpus_init(machine->cpu_model, icc_bridge);
+    pc_cpus_init(machine->cpu_model);
 
     if (kvm_enabled() && kvmclock_enabled) {
         kvmclock_create();
@@ -230,7 +224,6 @@  static void pc_init1(MachineState *machine,
     if (pci_enabled) {
         ioapic_init_gsi(gsi_state, "i440fx");
     }
-    qdev_init_nofail(icc_bridge);
 
     pc_register_ferr_irq(gsi[13]);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c0f21fe..d018e05 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -43,7 +43,6 @@ 
 #include "hw/ide/pci.h"
 #include "hw/ide/ahci.h"
 #include "hw/usb.h"
-#include "hw/cpu/icc_bus.h"
 #include "qemu/error-report.h"
 
 /* ICH9 AHCI has 6 ports */
@@ -85,7 +84,6 @@  static void pc_q35_init(MachineState *machine)
     int i;
     ICH9LPCState *ich9_lpc;
     PCIDevice *ahci;
-    DeviceState *icc_bridge;
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
     DriveInfo *hd[MAX_SATA_PORTS];
@@ -132,11 +130,7 @@  static void pc_q35_init(MachineState *machine)
         exit(1);
     }
 
-    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
-    object_property_add_child(qdev_get_machine(), "icc-bridge",
-                              OBJECT(icc_bridge), NULL);
-
-    pc_cpus_init(machine->cpu_model, icc_bridge);
+    pc_cpus_init(machine->cpu_model);
     pc_acpi_init("q35-acpi-dsdt.aml");
 
     kvmclock_create();
@@ -240,7 +234,6 @@  static void pc_q35_init(MachineState *machine)
     if (pci_enabled) {
         ioapic_init_gsi(gsi_state, "q35");
     }
-    qdev_init_nofail(icc_bridge);
 
     pc_register_ferr_irq(gsi[13]);
 
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 0f97b47..00ae0ec 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -376,7 +376,7 @@  static void apic_update_irq(APICCommonState *s)
         cpu_interrupt(cpu, CPU_INTERRUPT_POLL);
     } else if (apic_irq_pending(s) > 0) {
         cpu_interrupt(cpu, CPU_INTERRUPT_HARD);
-    } else if (!apic_accept_pic_intr(&s->busdev.qdev) || !pic_get_output(isa_pic)) {
+    } else if (!apic_accept_pic_intr(DEVICE(s)) || !pic_get_output(isa_pic)) {
         cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
     }
 }
@@ -549,10 +549,10 @@  static void apic_deliver(DeviceState *dev, uint8_t dest, uint8_t dest_mode,
 
 static bool apic_check_pic(APICCommonState *s)
 {
-    if (!apic_accept_pic_intr(&s->busdev.qdev) || !pic_get_output(isa_pic)) {
+    if (!apic_accept_pic_intr(DEVICE(s)) || !pic_get_output(isa_pic)) {
         return false;
     }
-    apic_deliver_pic_intr(&s->busdev.qdev, 1);
+    apic_deliver_pic_intr(DEVICE(s), 1);
     return true;
 }
 
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 0858b45..020219e 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -27,6 +27,8 @@ 
 static int apic_irq_delivered;
 bool apic_report_tpr_access;
 
+static MemoryRegion *apic_container;
+
 void cpu_set_apic_base(DeviceState *dev, uint64_t val)
 {
     trace_cpu_set_apic_base(val);
@@ -314,8 +316,11 @@  static void apic_common_realize(DeviceState *dev, Error **errp)
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
     if (!mmio_registered) {
-        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
-        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
+        memory_region_add_subregion(apic_container, 0, &s->io_memory);
+        memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
+                                            APIC_DEFAULT_ADDRESS,
+                                            apic_container,
+                                            0x1000);
         mmio_registered = true;
     }
 
@@ -431,15 +436,23 @@  static Property apic_properties_common[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void apic_common_initfn(Object *obj)
+{
+    if (!apic_container) {
+        apic_container = g_malloc(sizeof(*apic_container));
+        memory_region_init(apic_container, obj, "apic-container",
+                           APIC_SPACE_SIZE);
+    }
+}
+
 static void apic_common_class_init(ObjectClass *klass, void *data)
 {
-    ICCDeviceClass *idc = ICC_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_apic_common;
     dc->reset = apic_reset_common;
     dc->props = apic_properties_common;
-    idc->realize = apic_common_realize;
+    dc->realize = apic_common_realize;
     /*
      * Reason: APIC and CPU need to be wired up by
      * x86_cpu_apic_create()
@@ -449,8 +462,9 @@  static void apic_common_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo apic_common_type = {
     .name = TYPE_APIC_COMMON,
-    .parent = TYPE_ICC_DEVICE,
+    .parent = TYPE_DEVICE,
     .instance_size = sizeof(APICCommonState),
+    .instance_init = apic_common_initfn,
     .class_size = sizeof(APICCommonClass),
     .class_init = apic_common_class_init,
     .abstract = true,
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index dc7a89d..6eeceff 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -21,7 +21,6 @@ 
 #define QEMU_APIC_INTERNAL_H
 
 #include "exec/memory.h"
-#include "hw/cpu/icc_bus.h"
 #include "qemu/timer.h"
 
 /* APIC Local Vector Table */
@@ -78,7 +77,7 @@  typedef struct APICCommonState APICCommonState;
 
 typedef struct APICCommonClass
 {
-    ICCDeviceClass parent_class;
+    DeviceClass parent_class;
 
     DeviceRealize realize;
     void (*set_base)(APICCommonState *s, uint64_t val);
@@ -93,7 +92,7 @@  typedef struct APICCommonClass
 } APICCommonClass;
 
 struct APICCommonState {
-    ICCDevice busdev;
+    DeviceState dev;
 
     MemoryRegion io_memory;
     X86CPU *cpu;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 1b35168..7c9d044 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -163,7 +163,7 @@  extern int fd_bootchk;
 void pc_register_ferr_irq(qemu_irq irq);
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
-void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge);
+void pc_cpus_init(const char *cpu_model);
 void pc_hot_add_cpu(const int64_t id, Error **errp);
 void pc_acpi_init(const char *default_dsdt);
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ed7e5d5..4993ab4 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -42,7 +42,6 @@ 
 
 #include "sysemu/sysemu.h"
 #include "hw/qdev-properties.h"
-#include "hw/cpu/icc_bus.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/xen/xen.h"
 #include "hw/i386/apic_internal.h"
@@ -2076,7 +2075,7 @@  static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 
 }
 
-X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
+X86CPU *cpu_x86_create(const char *cpu_model,
                        Error **errp)
 {
     X86CPU *cpu = NULL;
@@ -2108,15 +2107,6 @@  X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
 
     cpu = X86_CPU(object_new(object_class_get_name(oc)));
 
-#ifndef CONFIG_USER_ONLY
-    if (icc_bridge == NULL) {
-        error_setg(&error, "Invalid icc-bridge value");
-        goto out;
-    }
-    qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
-    object_unref(OBJECT(cpu));
-#endif
-
     x86_cpu_parse_featurestr(CPU(cpu), features, &error);
     if (error) {
         goto out;
@@ -2139,7 +2129,7 @@  X86CPU *cpu_x86_init(const char *cpu_model)
     Error *error = NULL;
     X86CPU *cpu;
 
-    cpu = cpu_x86_create(cpu_model, NULL, &error);
+    cpu = cpu_x86_create(cpu_model, &error);
     if (error) {
         goto out;
     }
@@ -2951,7 +2941,6 @@  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     xcc->parent_realize = dc->realize;
     dc->realize = x86_cpu_realizefn;
-    dc->bus_type = TYPE_ICC_BUS;
     dc->props = x86_cpu_properties;
 
     xcc->parent_reset = cc->reset;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index e4c27b1..52c30f9 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -982,7 +982,7 @@  typedef struct CPUX86State {
 #include "cpu-qom.h"
 
 X86CPU *cpu_x86_init(const char *cpu_model);
-X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
+X86CPU *cpu_x86_create(const char *cpu_model,
                        Error **errp);
 int cpu_x86_exec(CPUX86State *s);
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);