diff mbox

[1/7] Introduce a new bus "ICC" to connect APIC

Message ID 1329347774-23262-2-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Feb. 15, 2012, 11:16 p.m. UTC
Introduce a new structure CPUS as the controller of ICC (INTERRUPT
CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
of sysbus. So we can support APIC hot-plug feature.

This is repost of original patch for qemu-kvm rebased on current qemu:
http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
All credits to Liu Ping Fan for writing it.

V2 changes:
  - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
    we got as input param instead for qbus_create, this makes cpus
    apics visible in "info qtree" monitor command
  - fix format error spotted by Jan and missed by checkpatch
  - cpu_has_apic_feature: return bool instead of int

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 Makefile.objs       |    2 +-
 hw/apic.c           |   24 ++++++++---
 hw/apic.h           |    1 +
 hw/icc_bus.c        |  113 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/icc_bus.h        |   41 ++++++++++++++++++
 hw/pc.c             |   14 +++++--
 target-i386/cpu.h   |    1 +
 target-i386/cpuid.c |   12 +++++
 8 files changed, 196 insertions(+), 12 deletions(-)
 create mode 100644 hw/icc_bus.c
 create mode 100644 hw/icc_bus.h

Comments

Jan Kiszka Feb. 16, 2012, 11:25 a.m. UTC | #1
On 2012-02-16 00:16, Igor Mammedov wrote:
> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
> of sysbus. So we can support APIC hot-plug feature.
> 
> This is repost of original patch for qemu-kvm rebased on current qemu:
> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
> All credits to Liu Ping Fan for writing it.
> 
> V2 changes:
>   - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>     we got as input param instead for qbus_create, this makes cpus
>     apics visible in "info qtree" monitor command
>   - fix format error spotted by Jan and missed by checkpatch
>   - cpu_has_apic_feature: return bool instead of int
> 

This patch surely no longer applies. And the ICC requires QOM conversion.

Jan
Anthony Liguori Feb. 16, 2012, 12:42 p.m. UTC | #2
On 02/16/2012 05:25 AM, Jan Kiszka wrote:
> On 2012-02-16 00:16, Igor Mammedov wrote:
>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>> of sysbus. So we can support APIC hot-plug feature.
>>
>> This is repost of original patch for qemu-kvm rebased on current qemu:
>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>> All credits to Liu Ping Fan for writing it.
>>
>> V2 changes:
>>    - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>      we got as input param instead for qbus_create, this makes cpus
>>      apics visible in "info qtree" monitor command
>>    - fix format error spotted by Jan and missed by checkpatch
>>    - cpu_has_apic_feature: return bool instead of int
>>
>
> This patch surely no longer applies. And the ICC requires QOM conversion.

Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.

The LAPIC can be made a child of the CPU device with a bidirectional link.

I would simply create a fixed set of CPU links<> hung off of /devices somewhere 
and use that as the hotplug mechanism.  This matches well the way we model this 
to the guest (we expose a fixed number of pluggable sockets).

Regards,

Anthony Liguori

>
> Jan
>
Jan Kiszka Feb. 16, 2012, 12:50 p.m. UTC | #3
On 2012-02-16 13:42, Anthony Liguori wrote:
> On 02/16/2012 05:25 AM, Jan Kiszka wrote:
>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>>> of sysbus. So we can support APIC hot-plug feature.
>>>
>>> This is repost of original patch for qemu-kvm rebased on current qemu:
>>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>>> All credits to Liu Ping Fan for writing it.
>>>
>>> V2 changes:
>>>    - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>>      we got as input param instead for qbus_create, this makes cpus
>>>      apics visible in "info qtree" monitor command
>>>    - fix format error spotted by Jan and missed by checkpatch
>>>    - cpu_has_apic_feature: return bool instead of int
>>>
>>
>> This patch surely no longer applies. And the ICC requires QOM conversion.
> 
> Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.
> 
> The LAPIC can be made a child of the CPU device with a bidirectional link.

We do have a bus here, the IO-APICs are attached to it as well. But we
can surely save that dummy cpusockets device.

Jan
Anthony Liguori Feb. 16, 2012, 12:59 p.m. UTC | #4
On 02/16/2012 06:50 AM, Jan Kiszka wrote:
> On 2012-02-16 13:42, Anthony Liguori wrote:
>> On 02/16/2012 05:25 AM, Jan Kiszka wrote:
>>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>>>> of sysbus. So we can support APIC hot-plug feature.
>>>>
>>>> This is repost of original patch for qemu-kvm rebased on current qemu:
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>>>> All credits to Liu Ping Fan for writing it.
>>>>
>>>> V2 changes:
>>>>     - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>>>       we got as input param instead for qbus_create, this makes cpus
>>>>       apics visible in "info qtree" monitor command
>>>>     - fix format error spotted by Jan and missed by checkpatch
>>>>     - cpu_has_apic_feature: return bool instead of int
>>>>
>>>
>>> This patch surely no longer applies. And the ICC requires QOM conversion.
>>
>> Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.
>>
>> The LAPIC can be made a child of the CPU device with a bidirectional link.
>
> We do have a bus here, the IO-APICs are attached to it as well.


Isn't that a bus protocol that's specific to the LAPICs/IO-APICs?  I don't think 
that you would logically model the CPU as part of it.

I wonder if modeling inter-lapic communication via a bus is a bit overkill 
compared to what we do today (just a for() loop when needed).

Regards,

Anthony Liguori

  But we
> can surely save that dummy cpusockets device.
>
> Jan
>
Jan Kiszka Feb. 16, 2012, 1:06 p.m. UTC | #5
On 2012-02-16 13:59, Anthony Liguori wrote:
> On 02/16/2012 06:50 AM, Jan Kiszka wrote:
>> On 2012-02-16 13:42, Anthony Liguori wrote:
>>> On 02/16/2012 05:25 AM, Jan Kiszka wrote:
>>>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>>>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>>>>> of sysbus. So we can support APIC hot-plug feature.
>>>>>
>>>>> This is repost of original patch for qemu-kvm rebased on current qemu:
>>>>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>>>>> All credits to Liu Ping Fan for writing it.
>>>>>
>>>>> V2 changes:
>>>>>     - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>>>>       we got as input param instead for qbus_create, this makes cpus
>>>>>       apics visible in "info qtree" monitor command
>>>>>     - fix format error spotted by Jan and missed by checkpatch
>>>>>     - cpu_has_apic_feature: return bool instead of int
>>>>>
>>>>
>>>> This patch surely no longer applies. And the ICC requires QOM conversion.
>>>
>>> Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.
>>>
>>> The LAPIC can be made a child of the CPU device with a bidirectional link.
>>
>> We do have a bus here, the IO-APICs are attached to it as well.
> 
> 
> Isn't that a bus protocol that's specific to the LAPICs/IO-APICs?  I don't think 
> that you would logically model the CPU as part of it.

Actually, MSIs go this way as well.

> 
> I wonder if modeling inter-lapic communication via a bus is a bit overkill 
> compared to what we do today (just a for() loop when needed).

For sure, we can keep such loops for all those use case. But Andreas may
have a different vision as he wanted to eliminate (public) first_cpu
references.

Jan
Igor Mammedov Feb. 17, 2012, 5:02 p.m. UTC | #6
On 02/16/2012 01:42 PM, Anthony Liguori wrote:
> On 02/16/2012 05:25 AM, Jan Kiszka wrote:
>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>>> of sysbus. So we can support APIC hot-plug feature.
>>>
>>> This is repost of original patch for qemu-kvm rebased on current qemu:
>>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>>> All credits to Liu Ping Fan for writing it.
>>>
>>> V2 changes:
>>> - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>> we got as input param instead for qbus_create, this makes cpus
>>> apics visible in "info qtree" monitor command
>>> - fix format error spotted by Jan and missed by checkpatch
>>> - cpu_has_apic_feature: return bool instead of int
>>>
>>
>> This patch surely no longer applies. And the ICC requires QOM conversion.
>
> Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.
>
> The LAPIC can be made a child of the CPU device with a bidirectional link.
>
> I would simply create a fixed set of CPU links<> hung off of /devices somewhere and use that as the hotplug mechanism. This matches well the way we
> model this to the guest (we expose a fixed number of pluggable sockets).

I've just QOM-ified it, but in light of what you just said it may be ignored.
ICC bus was used on pre Pentium 4 smp systems. And whole thing with introducing
it was to provide hot-plugable bus for cpus, since hot-plug on sysbus is disabled
and people argued that sysbus shouldn't be hot-plugable. However it depends on
what we choose to model, we can use pre P4 ICC bus for inter-apic/ioapic communications
or use P4 model allowing hot-plug on sysbus and use it for inter-apic/ioapic
communications if needed.

So I'd rather drop ICC patch and try your approach with CPU links<>, I see no
point in introducing new bus providing we have an alternative model and existing
bus for the task.

>
> Regards,
>
> Anthony Liguori
>
>>
>> Jan
>>
>
>
Igor Mammedov Feb. 24, 2012, 1:05 p.m. UTC | #7
On 02/17/2012 06:02 PM, Igor Mammedov wrote:
> On 02/16/2012 01:42 PM, Anthony Liguori wrote:
>> On 02/16/2012 05:25 AM, Jan Kiszka wrote:
>>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>>>> of sysbus. So we can support APIC hot-plug feature.
>>>>
>>>> This is repost of original patch for qemu-kvm rebased on current qemu:
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>>>> All credits to Liu Ping Fan for writing it.
>>>>
>>>> V2 changes:
>>>> - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>>> we got as input param instead for qbus_create, this makes cpus
>>>> apics visible in "info qtree" monitor command
>>>> - fix format error spotted by Jan and missed by checkpatch
>>>> - cpu_has_apic_feature: return bool instead of int
>>>>
>>>
>>> This patch surely no longer applies. And the ICC requires QOM conversion.
>>
>> Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.
>>
>> The LAPIC can be made a child of the CPU device with a bidirectional link.
>>
>> I would simply create a fixed set of CPU links<> hung off of /devices somewhere and use that as the hotplug mechanism. This matches well the way we
>> model this to the guest (we expose a fixed number of pluggable sockets).
>
> I've just QOM-ified it, but in light of what you just said it may be ignored.
> ICC bus was used on pre Pentium 4 smp systems. And whole thing with introducing
> it was to provide hot-plugable bus for cpus, since hot-plug on sysbus is disabled
> and people argued that sysbus shouldn't be hot-plugable. However it depends on
> what we choose to model, we can use pre P4 ICC bus for inter-apic/ioapic communications
> or use P4 model allowing hot-plug on sysbus and use it for inter-apic/ioapic
> communications if needed.
>
> So I'd rather drop ICC patch and try your approach with CPU links<>, I see no
> point in introducing new bus providing we have an alternative model and existing
> bus for the task.

I've looked at device_add command and qdev_device_add it uses for doing actual work
and in current state it requires (based on Andreas' qom_cpu branch):

For approach where apic and cpu hot-plugged to sysbus.
   1. created object must be descendant of TYPE_DEVICE. So QOM TYPE_CPU should be inherited
      from TYPE_DEVICE at least or TYPE_SYS_BUS_DEVICE.
   2. hot-plug on the bus should be allowed. if we ditch icc bus then we should allow
      hot-plug on sysbus. Can we do this? (i.e. it seems that for P4 and later cpus
      sysbus should be hot-plugable).
   3. should DeviceClass.init be used for cpu initialization or should .instance_init
      do all the job and make DeviceClass.init nop?

Another approach that tries to re-use device_add interface:
   1. allow run-time type detection in qdev_device_add and execute separate branch for
      TYPE_CPU. This way we could easily use links<> on sysbus
   2. device_del will require the same hacking as device_add
   3. apic now is sysbus device, question is what will be lost if it is attached to link and
      won't be sysbus_device_type anymore?
   4. will reset called on sysbus reach apic/cpu if it is on the link?

Any opinions on direction I should look more closely?
Andreas Färber Feb. 24, 2012, 1:30 p.m. UTC | #8
Am 24.02.2012 14:05, schrieb Igor Mammedov:
> On 02/17/2012 06:02 PM, Igor Mammedov wrote:
>> So I'd rather drop ICC patch and try your approach with CPU links<>, I
>> see no
>> point in introducing new bus providing we have an alternative model
>> and existing
>> bus for the task.
> 
> I've looked at device_add command and qdev_device_add it uses for doing
> actual work
> and in current state it requires (based on Andreas' qom_cpu branch):
> 
> For approach where apic and cpu hot-plugged to sysbus.
>   1. created object must be descendant of TYPE_DEVICE. So QOM TYPE_CPU
> should be inherited
>      from TYPE_DEVICE at least or TYPE_SYS_BUS_DEVICE.
>   2. hot-plug on the bus should be allowed. if we ditch icc bus then we
> should allow
>      hot-plug on sysbus. Can we do this? (i.e. it seems that for P4 and
> later cpus
>      sysbus should be hot-plugable).
>   3. should DeviceClass.init be used for cpu initialization or should
> .instance_init
>      do all the job and make DeviceClass.init nop?

SysBus is supposed to go away in Anthony's upcoming 4th QOM series, so
I'd rather not base a new series on that.

The issue with TYPE_DEVICE is that we don't want to leak that into the
user emulators (would break the build), and any infrastructure only
available to qdev should gradually be made accessible to all objects
(Paolo has done some work in that direction wrt properties). So the main
remaining difference between Object and Device is the GPIO IRQ support.
Anthony wanted to introduce Pin objects to replace qemu_irq.

As for init, the idea was to have initialization code in an initfn, the
resulting state is then supposed to be overridable by the user (e.g.,
set family, model, stepping differently on the CPU) and then do the
construction work in a realize function that corresponds more closely to
DeviceClass::init. This too, still needs to be generalized.

Andreas
Paolo Bonzini Feb. 24, 2012, 1:40 p.m. UTC | #9
On 02/24/2012 02:30 PM, Andreas Färber wrote:
> SysBus is supposed to go away in Anthony's upcoming 4th QOM series, so
> I'd rather not base a new series on that.

Not sure about that.  I haven't understood well the scope of the series,
but I think it only converted buses to QOM, it didn't kill them.
Perhaps SysBus was special, in which case it would become automatically
hotpluggable: just create a new QOM object.

> The issue with TYPE_DEVICE is that we don't want to leak that into the
> user emulators (would break the build), and any infrastructure only
> available to qdev should gradually be made accessible to all objects
> (Paolo has done some work in that direction wrt properties).

I haven't, but it would be next on the list of things to do.

> So the main remaining difference between Object and Device is the
> GPIO IRQ support. Anthony wanted to introduce Pin objects to replace
> qemu_irq.

Aiming at "replacing" is a bad idea unless you can do it fast and
painlessly.  Adding gpio_in and gpio_out property types would be more
useful and would let you expose qemu_irq as QOM.  You can then change
the existing qdev.c functions to operate on those new property types.

Paolo
Anthony Liguori Feb. 24, 2012, 1:47 p.m. UTC | #10
On 02/24/2012 07:40 AM, Paolo Bonzini wrote:
> On 02/24/2012 02:30 PM, Andreas Färber wrote:
>> SysBus is supposed to go away in Anthony's upcoming 4th QOM series, so
>> I'd rather not base a new series on that.
>
> Not sure about that.  I haven't understood well the scope of the series,
> but I think it only converted buses to QOM, it didn't kill them.
> Perhaps SysBus was special, in which case it would become automatically
> hotpluggable: just create a new QOM object.
>
>> The issue with TYPE_DEVICE is that we don't want to leak that into the
>> user emulators (would break the build), and any infrastructure only
>> available to qdev should gradually be made accessible to all objects
>> (Paolo has done some work in that direction wrt properties).
>
> I haven't, but it would be next on the list of things to do.
>
>> So the main remaining difference between Object and Device is the
>> GPIO IRQ support. Anthony wanted to introduce Pin objects to replace
>> qemu_irq.
>
> Aiming at "replacing" is a bad idea unless you can do it fast and
> painlessly.  Adding gpio_in and gpio_out property types would be more
> useful and would let you expose qemu_irq as QOM.

I agree with you in principle, but in practice, there is not obvious way to 
serialize gpio_in/gpio_out via Visitors.  Finding some way to do it as an 
integer is clearly wrong IMHO.

I think a simple Pin object with the following interfaces:

/**
  * Connect a pin to a qemu_irq such that whenever the pin is
  * raised, qemu_irq_raise() is called too on irq.
  */
void pin_connect_qemu_irq(Pin *obj, qemu_irq irq);

/**
  * Returns a qemu_irq such that whenever qemu_irq_raise() is
  * called, pin_set_level(obj, true) is called.
  */
qemu_irq pin_get_qemu_irq(Pin *obj);

Let's you incrementally refactor objects to use Pins while maintaining the 
existing qemu_irq infrastructure.

Regards,

Anthony Liguori

>  You can then change
> the existing qdev.c functions to operate on those new property types.
>
> Paolo
Paolo Bonzini Feb. 24, 2012, 1:50 p.m. UTC | #11
On 02/24/2012 02:47 PM, Anthony Liguori wrote:
>>
> 
> I agree with you in principle, but in practice, there is not obvious way
> to serialize gpio_in/gpio_out via Visitors.  Finding some way to do it
> as an integer is clearly wrong IMHO.

"%s/gpio_in[%d]" % (object_get_canonical_path(...), opaque->n) is what I
was thinking about.

> I think a simple Pin object with the following interfaces:
> 
> /**
>  * Connect a pin to a qemu_irq such that whenever the pin is
>  * raised, qemu_irq_raise() is called too on irq.
>  */
> void pin_connect_qemu_irq(Pin *obj, qemu_irq irq);
> 
> /**
>  * Returns a qemu_irq such that whenever qemu_irq_raise() is
>  * called, pin_set_level(obj, true) is called.
>  */
> qemu_irq pin_get_qemu_irq(Pin *obj);
> 
> Let's you incrementally refactor objects to use Pins while maintaining the existing qemu_irq infrastructure.

Sure, a simple bridge is a fine alternative.  What I'm not sure about is
making Pins stateful, because that means you have to serialize them.

Paolo
Anthony Liguori Feb. 24, 2012, 2:44 p.m. UTC | #12
On 02/24/2012 07:50 AM, Paolo Bonzini wrote:
> On 02/24/2012 02:47 PM, Anthony Liguori wrote:
>>>
>>
>> I agree with you in principle, but in practice, there is not obvious way
>> to serialize gpio_in/gpio_out via Visitors.  Finding some way to do it
>> as an integer is clearly wrong IMHO.
>
> "%s/gpio_in[%d]" % (object_get_canonical_path(...), opaque->n) is what I
> was thinking about.

This creates another namespace that's independent of the QOM graph.  This is 
something we should try to avoid.

>> I think a simple Pin object with the following interfaces:
>>
>> /**
>>   * Connect a pin to a qemu_irq such that whenever the pin is
>>   * raised, qemu_irq_raise() is called too on irq.
>>   */
>> void pin_connect_qemu_irq(Pin *obj, qemu_irq irq);
>>
>> /**
>>   * Returns a qemu_irq such that whenever qemu_irq_raise() is
>>   * called, pin_set_level(obj, true) is called.
>>   */
>> qemu_irq pin_get_qemu_irq(Pin *obj);
>>
>> Let's you incrementally refactor objects to use Pins while maintaining the existing qemu_irq infrastructure.
>
> Sure, a simple bridge is a fine alternative.  What I'm not sure about is
> making Pins stateful, because that means you have to serialize them.

Being stateful is a feature but the concept would work just as well if you 
didn't store the pin state.  Then it just looks like:

struct Pin
{
    Object parent;

    /*< private >*/
    NotifierLister level_change_notifiers;
};

The reason to introduce another type (instead of attempting to convert qemu_irq) 
is that the life cycle of qemu_irq is very un-QOM.  I don't think we can do it 
without incrementally refactoring the users of qemu_irq and a new type makes it 
easier to do that incrementally.

Regards,

Anthony Liguori

> Paolo
diff mbox

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 4f6d26c..45df666 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -212,7 +212,7 @@  hw-obj-$(CONFIG_USB_UHCI) += usb-uhci.o
 hw-obj-$(CONFIG_USB_OHCI) += usb-ohci.o
 hw-obj-$(CONFIG_USB_EHCI) += usb-ehci.o
 hw-obj-$(CONFIG_FDC) += fdc.o
-hw-obj-$(CONFIG_ACPI) += acpi.o acpi_piix4.o
+hw-obj-$(CONFIG_ACPI) += acpi.o acpi_piix4.o icc_bus.o
 hw-obj-$(CONFIG_APM) += pm_smbus.o apm.o
 hw-obj-$(CONFIG_DMA) += dma.o
 hw-obj-$(CONFIG_HPET) += hpet.o
diff --git a/hw/apic.c b/hw/apic.c
index 9d0f460..e666b4c 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -21,9 +21,10 @@ 
 #include "ioapic.h"
 #include "qemu-timer.h"
 #include "host-utils.h"
-#include "sysbus.h"
+#include "icc_bus.h"
 #include "trace.h"
 #include "pc.h"
+#include "exec-memory.h"
 
 /* APIC Local Vector Table */
 #define APIC_LVT_TIMER   0
@@ -80,7 +81,7 @@ 
 typedef struct APICState APICState;
 
 struct APICState {
-    SysBusDevice busdev;
+    ICCBusDevice busdev;
     MemoryRegion io_memory;
     void *cpu_env;
     uint32_t apicbase;
@@ -988,9 +989,19 @@  static const MemoryRegionOps apic_io_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int apic_init1(SysBusDevice *dev)
+int apic_mmio_map(DeviceState *dev, target_phys_addr_t base)
 {
-    APICState *s = FROM_SYSBUS(APICState, dev);
+    APICState *s = DO_UPCAST(APICState, busdev.qdev, dev);
+
+    memory_region_add_subregion(get_system_memory(),
+                                base,
+                                &s->io_memory);
+    return 0;
+}
+
+static int apic_init1(ICCBusDevice *dev)
+{
+    APICState *s = DO_UPCAST(APICState, busdev, dev);
     static int last_apic_idx;
 
     if (last_apic_idx >= MAX_APICS) {
@@ -998,7 +1009,6 @@  static int apic_init1(SysBusDevice *dev)
     }
     memory_region_init_io(&s->io_memory, &apic_io_ops, s, "apic",
                           MSI_ADDR_SIZE);
-    sysbus_init_mmio(dev, &s->io_memory);
 
     s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
     s->idx = last_apic_idx++;
@@ -1006,7 +1016,7 @@  static int apic_init1(SysBusDevice *dev)
     return 0;
 }
 
-static SysBusDeviceInfo apic_info = {
+static ICCBusDeviceInfo apic_info = {
     .init = apic_init1,
     .qdev.name = "apic",
     .qdev.size = sizeof(APICState),
@@ -1022,7 +1032,7 @@  static SysBusDeviceInfo apic_info = {
 
 static void apic_register_devices(void)
 {
-    sysbus_register_withprop(&apic_info);
+    iccbus_register_devinfo(&apic_info);
 }
 
 device_init(apic_register_devices)
diff --git a/hw/apic.h b/hw/apic.h
index a5c910f..d42081e 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -17,6 +17,7 @@  void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
 uint8_t cpu_get_apic_tpr(DeviceState *s);
 void apic_init_reset(DeviceState *s);
 void apic_sipi(DeviceState *s);
+int apic_mmio_map(DeviceState *dev, target_phys_addr_t base);
 
 /* pc.c */
 int cpu_is_bsp(CPUState *env);
diff --git a/hw/icc_bus.c b/hw/icc_bus.c
new file mode 100644
index 0000000..8a3b1ee
--- /dev/null
+++ b/hw/icc_bus.c
@@ -0,0 +1,113 @@ 
+/* icc_bus.c
+ * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+#include "icc_bus.h"
+
+typedef struct CPUSockets CPUSockets;
+typedef struct ICCBus ICCBus;
+typedef struct ICCBusInfo ICCBusInfo;
+
+struct CPUSockets {
+    SysBusDevice sysdev;
+    ICCBus *icc_bus;
+};
+
+struct CPUSInfo {
+    DeviceInfo info;
+};
+
+struct ICCBus {
+    BusState qbus;
+};
+
+struct ICCBusInfo {
+    BusInfo qinfo;
+};
+
+static CPUSockets *cpu_sockets;
+
+static ICCBusInfo icc_bus_info = {
+    .qinfo.name = "icc",
+    .qinfo.size = sizeof(ICCBus),
+    .qinfo.props = (Property[]) {
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+static int iccbus_device_init(DeviceState *dev, DeviceInfo *base)
+{
+    ICCBusDeviceInfo *info = container_of(base, ICCBusDeviceInfo, qdev);
+    ICCBusDevice *idev = DO_UPCAST(ICCBusDevice, qdev, dev);
+
+    return info->init(idev);
+}
+
+void iccbus_register_devinfo(ICCBusDeviceInfo *info)
+{
+    info->qdev.init = iccbus_device_init;
+    info->qdev.bus_info = &icc_bus_info.qinfo;
+    assert(info->qdev.size >= sizeof(ICCBusDevice));
+    qdev_register(&info->qdev);
+}
+
+BusState *get_icc_bus(void)
+{
+    return &cpu_sockets->icc_bus->qbus;
+}
+
+/*Must be called before vcpu's creation*/
+static int cpusockets_init(SysBusDevice *dev)
+{
+    CPUSockets *cpus = DO_UPCAST(CPUSockets, sysdev, dev);
+    BusState *b = qbus_create(&icc_bus_info.qinfo,
+                              &cpus->sysdev.qdev,
+                              "icc");
+    if (b == NULL) {
+        return -1;
+    }
+    b->allow_hotplug = 1; /* Yes, we can */
+    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
+    cpu_sockets = cpus;
+    return 0;
+
+}
+
+static const VMStateDescription vmstate_cpusockets = {
+    .name = "cpusockets",
+    .version_id = 1,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static SysBusDeviceInfo cpusockets_info = {
+    .init = cpusockets_init,
+    .qdev.name = "cpusockets",
+    .qdev.size = sizeof(CPUSockets),
+    .qdev.vmsd = &vmstate_cpusockets,
+    .qdev.reset = NULL,
+    .qdev.no_user = 1,
+};
+
+static void cpusockets_register_devices(void)
+{
+    sysbus_register_withprop(&cpusockets_info);
+}
+
+device_init(cpusockets_register_devices)
diff --git a/hw/icc_bus.h b/hw/icc_bus.h
new file mode 100644
index 0000000..f817873
--- /dev/null
+++ b/hw/icc_bus.h
@@ -0,0 +1,41 @@ 
+/* ICCBus.h
+ * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+#ifndef QEMU_ICC_H
+#define QEMU_ICC_H
+
+#include "qdev.h"
+#include "sysbus.h"
+
+struct ICCBusDevice {
+    DeviceState qdev;
+};
+typedef struct ICCBusDevice ICCBusDevice;
+typedef int (*iccbus_initfn)(ICCBusDevice *dev);
+
+struct ICCBusDeviceInfo {
+    DeviceInfo qdev;
+    iccbus_initfn init;
+};
+typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
+
+BusState *get_icc_bus(void);
+
+void iccbus_register_devinfo(ICCBusDeviceInfo *info);
+
+#endif
diff --git a/hw/pc.c b/hw/pc.c
index 85304cf..33d8090 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -24,6 +24,7 @@ 
 #include "hw.h"
 #include "pc.h"
 #include "apic.h"
+#include "icc_bus.h"
 #include "fdc.h"
 #include "ide.h"
 #include "pci.h"
@@ -878,21 +879,21 @@  DeviceState *cpu_get_current_apic(void)
 static DeviceState *apic_init(void *env, uint8_t apic_id)
 {
     DeviceState *dev;
-    SysBusDevice *d;
+    BusState *b;
     static int apic_mapped;
 
-    dev = qdev_create(NULL, "apic");
+    b = get_icc_bus();
+    dev = qdev_create(b, "apic");
     qdev_prop_set_uint8(dev, "id", apic_id);
     qdev_prop_set_ptr(dev, "cpu_env", env);
     qdev_init_nofail(dev);
-    d = sysbus_from_qdev(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(d, 0, MSI_ADDR_BASE);
+        apic_mmio_map(dev, MSI_ADDR_BASE);
         apic_mapped = 1;
     }
 
@@ -959,6 +960,11 @@  void pc_cpus_init(const char *cpu_model)
 #endif
     }
 
+    if (cpu_has_apic_feature(cpu_model) || smp_cpus > 1) {
+        DeviceState *d = qdev_create(NULL, "cpusockets");
+        qdev_init_nofail(d);
+    }
+
     for(i = 0; i < smp_cpus; i++) {
         pc_new_cpu(cpu_model);
     }
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 37dde79..7e66bcf 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -892,6 +892,7 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
 int cpu_x86_register (CPUX86State *env, const char *cpu_model);
+bool cpu_has_apic_feature(const char *cpu_model);
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 91a104b..1664c6c 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -850,6 +850,18 @@  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
     }
 }
 
+bool cpu_has_apic_feature(const char *cpu_model)
+{
+    x86_def_t def;
+
+    memset(&def, 0, sizeof(def));
+
+    if (cpu_x86_find_by_name(&def, cpu_model) < 0) {
+        return false;
+    }
+    return def.features & CPUID_APIC;
+}
+
 int cpu_x86_register (CPUX86State *env, const char *cpu_model)
 {
     x86_def_t def1, *def = &def1;