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

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

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.

Signed-off-by: liu ping fan <pingfank@linux.vnet.ibm.com>
---
 Makefile.target |    1 +
 hw/apic.c       |   25 +++++++++++----
 hw/apic.h       |    1 +
 hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
 hw/pc.c         |   11 ++++--
 6 files changed, 174 insertions(+), 11 deletions(-)
 create mode 100644 hw/icc_bus.c
 create mode 100644 hw/icc_bus.h

On 2011-10-19 03:55, pingfank@linux.vnet.ibm.com wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> 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.
> 
> Signed-off-by: liu ping fan <pingfank@linux.vnet.ibm.com>
> ---
>  Makefile.target |    1 +
>  hw/apic.c       |   25 +++++++++++----
>  hw/apic.h       |    1 +
>  hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
>  hw/pc.c         |   11 ++++--
>  6 files changed, 174 insertions(+), 11 deletions(-)
>  create mode 100644 hw/icc_bus.c
>  create mode 100644 hw/icc_bus.h
> 
> diff --git a/Makefile.target b/Makefile.target
> index 9011f28..5607c6d 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>  obj-i386-y += testdev.o
>  obj-i386-y += acpi.o acpi_piix4.o
> +obj-i386-y += icc_bus.o
>  
>  obj-i386-y += pcspk.o i8254.o
>  obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
> diff --git a/hw/apic.c b/hw/apic.c
> index 69d6ac5..00d2297 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 "kvm.h"
> +#include "exec-memory.h"

Mmh, don't your rather want 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;
> @@ -1104,9 +1105,20 @@ static const MemoryRegionOps apic_io_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static int apic_init1(SysBusDevice *dev)
> +/**/

Empty comment.

> +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) {
> @@ -1114,7 +1126,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_region(dev, &s->io_memory);
>  
>      s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
>      s->idx = last_apic_idx++;
> @@ -1122,7 +1133,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),
> @@ -1138,7 +1149,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 c857d52..e2c0af5 100644
> --- a/hw/apic.h
> +++ b/hw/apic.h
> @@ -20,6 +20,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..61a408e
> --- /dev/null
> +++ b/hw/icc_bus.c
> @@ -0,0 +1,91 @@
> +/* icc_bus.c
> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus

Copyright?

> + *
> + * 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"
> +
> +static CPUS *dummy_cpus;

Why "dummy"? Also, no statics please. The bus owner is the chipset
(440fx), so embedded it there. That also avoid that strange "cpus"
device below.

> +
> +
> +static ICCBusInfo icc_bus_info = {
> +    .qinfo.name = "icc",
> +    .qinfo.size = sizeof(ICCBus),
> +    .qinfo.props = (Property[]) {
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
> +
> +
> +

A single line-break is sufficient, here and elsewhere.

> +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 &dummy_cpus->icc_bus->qbus;
> +}

See above: the chipset should manage this. Do we have a circular
dependency then?

> +
> +static void cpus_reset(DeviceState *d)
> +{
> +}

[ qdev.reset is an optional callback. But this is obsolete if you embed
the bus into the chipset. ]

> +
> +/*Must be called before vcpu's creation*/
> +static int cpus_init(SysBusDevice *dev)
> +{
> +    CPUS *cpus = DO_UPCAST(CPUS, sysdev, dev);
> +    BusState *b = qbus_create(&icc_bus_info.qinfo,
> +                              &dummy_cpus->sysdev.qdev,
> +                              "icc");
> +    if (b == NULL) {
> +        return -1;
> +    }
> +    b->allow_hotplug = 1; /* Yes, we can */
> +    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
> +    dummy_cpus = cpus;
> +    return 0;
> +
> +}
> +
> +
> +static SysBusDeviceInfo cpus_info = {
> +    .init = cpus_init,
> +    .qdev.name = "cpus",
> +    .qdev.size = sizeof(CPUS),
> +    /*.qdev.vmsd = &vmstate_apic,*/

[ No empty vmsd please. Either a device is unmigratable (I hope this one
is not :) ) or it is supposed to provide a stub vmsd with empty 'fields'
array. ]

> +    .qdev.reset = cpus_reset,
> +    .qdev.no_user = 1,
> +};
> +
> +static void cpus_register_devices(void)
> +{
> +    sysbus_register_withprop(&cpus_info);
> +}
> +
> +device_init(cpus_register_devices)
> diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> new file mode 100644
> index 0000000..64ac153
> --- /dev/null
> +++ b/hw/icc_bus.h
> @@ -0,0 +1,56 @@
> +/* ICCBus.h
> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> + *
> + * 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"
> +
> +typedef struct CPUS CPUS;
> +typedef struct ICCBus ICCBus;
> +typedef struct ICCBusInfo ICCBusInfo;
> +typedef struct ICCBusDevice ICCBusDevice;
> +typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
> +
> +struct CPUS {
> +    SysBusDevice sysdev;
> +    ICCBus *icc_bus;
> +};
> +
> +struct ICCBus {
> +    BusState qbus;
> +};
> +
> +struct ICCBusInfo {
> +    BusInfo qinfo;
> +};
> +struct ICCBusDevice {
> +    DeviceState qdev;
> +};
> +
> +typedef int (*iccbus_initfn)(ICCBusDevice *dev);
> +
> +struct ICCBusDeviceInfo {
> +    DeviceInfo qdev;
> +    iccbus_initfn init;
> +};
> +
> +
> +void iccbus_register_devinfo(ICCBusDeviceInfo *info);
> +BusState *get_icc_bus(void);
> +
> +#endif
> diff --git a/hw/pc.c b/hw/pc.c
> index 6b3662e..78b7826 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"
> @@ -875,21 +876,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;
>      }
>  
> @@ -955,6 +956,8 @@ CPUState *pc_new_cpu(const char *cpu_model)
>  void pc_cpus_init(const char *cpu_model)
>  {
>      int i;
> +    DeviceState *d = qdev_create(NULL, "cpus");
> +    qdev_init_nofail(d);
>  
>      /* init CPUs */
>      for(i = 0; i < smp_cpus; i++) {

Cool. Looking forward to seeing CPU hotplug work again, and then finally
in upstream!

Jan

On 10/19/2011 05:53 AM, Jan Kiszka wrote:
> On 2011-10-19 03:55, pingfank@linux.vnet.ibm.com wrote:
>> From: Liu Ping Fan<pingfank@linux.vnet.ibm.com>
>>
>> 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.
>>
>> Signed-off-by: liu ping fan<pingfank@linux.vnet.ibm.com>
>> ---
>>   Makefile.target |    1 +
>>   hw/apic.c       |   25 +++++++++++----
>>   hw/apic.h       |    1 +
>>   hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
>>   hw/pc.c         |   11 ++++--
>>   6 files changed, 174 insertions(+), 11 deletions(-)
>>   create mode 100644 hw/icc_bus.c
>>   create mode 100644 hw/icc_bus.h
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index 9011f28..5607c6d 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>>   obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>   obj-i386-y += testdev.o
>>   obj-i386-y += acpi.o acpi_piix4.o
>> +obj-i386-y += icc_bus.o
>>
>>   obj-i386-y += pcspk.o i8254.o
>>   obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
>> diff --git a/hw/apic.c b/hw/apic.c
>> index 69d6ac5..00d2297 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 "kvm.h"
>> +#include "exec-memory.h"
>
> Mmh, don't your rather want 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;
>> @@ -1104,9 +1105,20 @@ static const MemoryRegionOps apic_io_ops = {
>>       .endianness = DEVICE_NATIVE_ENDIAN,
>>   };
>>
>> -static int apic_init1(SysBusDevice *dev)
>> +/**/
>
> Empty comment.
>
>> +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) {
>> @@ -1114,7 +1126,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_region(dev,&s->io_memory);
>>
>>       s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
>>       s->idx = last_apic_idx++;
>> @@ -1122,7 +1133,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),
>> @@ -1138,7 +1149,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 c857d52..e2c0af5 100644
>> --- a/hw/apic.h
>> +++ b/hw/apic.h
>> @@ -20,6 +20,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..61a408e
>> --- /dev/null
>> +++ b/hw/icc_bus.c
>> @@ -0,0 +1,91 @@
>> +/* icc_bus.c
>> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
>
> Copyright?
>
>> + *
>> + * 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"
>> +
>> +static CPUS *dummy_cpus;
>
> Why "dummy"? Also, no statics please. The bus owner is the chipset
> (440fx), so embedded it there. That also avoid that strange "cpus"
> device below.

That's an odd model IMHO.  The i440fx doesn't implement the ICC bus.  The ICC 
bus is entirely independent of the northbridge.

Maybe CPUSockets would be a better device name?

Regards,

Anthony Liguori

>
>> +
>> +
>> +static ICCBusInfo icc_bus_info = {
>> +    .qinfo.name = "icc",
>> +    .qinfo.size = sizeof(ICCBus),
>> +    .qinfo.props = (Property[]) {
>> +        DEFINE_PROP_END_OF_LIST(),
>> +    }
>> +};
>> +
>> +
>> +
>
> A single line-break is sufficient, here and elsewhere.
>
>> +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&dummy_cpus->icc_bus->qbus;
>> +}
>
> See above: the chipset should manage this. Do we have a circular
> dependency then?
>
>> +
>> +static void cpus_reset(DeviceState *d)
>> +{
>> +}
>
> [ qdev.reset is an optional callback. But this is obsolete if you embed
> the bus into the chipset. ]
>
>> +
>> +/*Must be called before vcpu's creation*/
>> +static int cpus_init(SysBusDevice *dev)
>> +{
>> +    CPUS *cpus = DO_UPCAST(CPUS, sysdev, dev);
>> +    BusState *b = qbus_create(&icc_bus_info.qinfo,
>> +&dummy_cpus->sysdev.qdev,
>> +                              "icc");
>> +    if (b == NULL) {
>> +        return -1;
>> +    }
>> +    b->allow_hotplug = 1; /* Yes, we can */
>> +    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
>> +    dummy_cpus = cpus;
>> +    return 0;
>> +
>> +}
>> +
>> +
>> +static SysBusDeviceInfo cpus_info = {
>> +    .init = cpus_init,
>> +    .qdev.name = "cpus",
>> +    .qdev.size = sizeof(CPUS),
>> +    /*.qdev.vmsd =&vmstate_apic,*/
>
> [ No empty vmsd please. Either a device is unmigratable (I hope this one
> is not :) ) or it is supposed to provide a stub vmsd with empty 'fields'
> array. ]
>
>> +    .qdev.reset = cpus_reset,
>> +    .qdev.no_user = 1,
>> +};
>> +
>> +static void cpus_register_devices(void)
>> +{
>> +    sysbus_register_withprop(&cpus_info);
>> +}
>> +
>> +device_init(cpus_register_devices)
>> diff --git a/hw/icc_bus.h b/hw/icc_bus.h
>> new file mode 100644
>> index 0000000..64ac153
>> --- /dev/null
>> +++ b/hw/icc_bus.h
>> @@ -0,0 +1,56 @@
>> +/* ICCBus.h
>> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
>> + *
>> + * 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"
>> +
>> +typedef struct CPUS CPUS;
>> +typedef struct ICCBus ICCBus;
>> +typedef struct ICCBusInfo ICCBusInfo;
>> +typedef struct ICCBusDevice ICCBusDevice;
>> +typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
>> +
>> +struct CPUS {
>> +    SysBusDevice sysdev;
>> +    ICCBus *icc_bus;
>> +};
>> +
>> +struct ICCBus {
>> +    BusState qbus;
>> +};
>> +
>> +struct ICCBusInfo {
>> +    BusInfo qinfo;
>> +};
>> +struct ICCBusDevice {
>> +    DeviceState qdev;
>> +};
>> +
>> +typedef int (*iccbus_initfn)(ICCBusDevice *dev);
>> +
>> +struct ICCBusDeviceInfo {
>> +    DeviceInfo qdev;
>> +    iccbus_initfn init;
>> +};
>> +
>> +
>> +void iccbus_register_devinfo(ICCBusDeviceInfo *info);
>> +BusState *get_icc_bus(void);
>> +
>> +#endif
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 6b3662e..78b7826 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"
>> @@ -875,21 +876,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;
>>       }
>>
>> @@ -955,6 +956,8 @@ CPUState *pc_new_cpu(const char *cpu_model)
>>   void pc_cpus_init(const char *cpu_model)
>>   {
>>       int i;
>> +    DeviceState *d = qdev_create(NULL, "cpus");
>> +    qdev_init_nofail(d);
>>
>>       /* init CPUs */
>>       for(i = 0; i<  smp_cpus; i++) {
>
> Cool. Looking forward to seeing CPU hotplug work again, and then finally
> in upstream!
>
> Jan
>

On 2011-10-19 14:54, Anthony Liguori wrote:
> On 10/19/2011 05:53 AM, Jan Kiszka wrote:
>> On 2011-10-19 03:55, pingfank@linux.vnet.ibm.com wrote:
>>> From: Liu Ping Fan<pingfank@linux.vnet.ibm.com>
>>>
>>> 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.
>>>
>>> Signed-off-by: liu ping fan<pingfank@linux.vnet.ibm.com>
>>> ---
>>>   Makefile.target |    1 +
>>>   hw/apic.c       |   25 +++++++++++----
>>>   hw/apic.h       |    1 +
>>>   hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
>>>   hw/pc.c         |   11 ++++--
>>>   6 files changed, 174 insertions(+), 11 deletions(-)
>>>   create mode 100644 hw/icc_bus.c
>>>   create mode 100644 hw/icc_bus.h
>>>
>>> diff --git a/Makefile.target b/Makefile.target
>>> index 9011f28..5607c6d 100644
>>> --- a/Makefile.target
>>> +++ b/Makefile.target
>>> @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>>>   obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>>   obj-i386-y += testdev.o
>>>   obj-i386-y += acpi.o acpi_piix4.o
>>> +obj-i386-y += icc_bus.o
>>>
>>>   obj-i386-y += pcspk.o i8254.o
>>>   obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
>>> diff --git a/hw/apic.c b/hw/apic.c
>>> index 69d6ac5..00d2297 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 "kvm.h"
>>> +#include "exec-memory.h"
>>
>> Mmh, don't your rather want 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;
>>> @@ -1104,9 +1105,20 @@ static const MemoryRegionOps apic_io_ops = {
>>>       .endianness = DEVICE_NATIVE_ENDIAN,
>>>   };
>>>
>>> -static int apic_init1(SysBusDevice *dev)
>>> +/**/
>>
>> Empty comment.
>>
>>> +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) {
>>> @@ -1114,7 +1126,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_region(dev,&s->io_memory);
>>>
>>>       s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
>>>       s->idx = last_apic_idx++;
>>> @@ -1122,7 +1133,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),
>>> @@ -1138,7 +1149,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 c857d52..e2c0af5 100644
>>> --- a/hw/apic.h
>>> +++ b/hw/apic.h
>>> @@ -20,6 +20,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..61a408e
>>> --- /dev/null
>>> +++ b/hw/icc_bus.c
>>> @@ -0,0 +1,91 @@
>>> +/* icc_bus.c
>>> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
>>
>> Copyright?
>>
>>> + *
>>> + * 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"
>>> +
>>> +static CPUS *dummy_cpus;
>>
>> Why "dummy"? Also, no statics please. The bus owner is the chipset
>> (440fx), so embedded it there. That also avoid that strange "cpus"
>> device below.
> 
> That's an odd model IMHO.  The i440fx doesn't implement the ICC bus.  The ICC
> bus is entirely independent of the northbridge.
> 
> Maybe CPUSockets would be a better device name?
> 

The problem is that it is even something non-existent in the absence of
an IOAPIC - which is an optional device for the i440fx/PIIX3, though we
only model this for ISA PCs, not for PCI machines (and make a lot of
mistakes in this area as I recently noticed). Still, we will need an ICC
bus on a ISAPC from now on. Also not correct.

In an ideal world there would be no dummy device CPUSockets, just an
optional unowned ICC bus. CPUs were not ICC bus devices, but could
attach to one if its there. But I think this cannot be modeled yet. So
we likely have to live with the proposed setup for now.

Jan

On 2011-10-19 15:33, Jan Kiszka wrote:
> On 2011-10-19 14:54, Anthony Liguori wrote:
>> On 10/19/2011 05:53 AM, Jan Kiszka wrote:
>>> On 2011-10-19 03:55, pingfank@linux.vnet.ibm.com wrote:
>>>> From: Liu Ping Fan<pingfank@linux.vnet.ibm.com>
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: liu ping fan<pingfank@linux.vnet.ibm.com>
>>>> ---
>>>>   Makefile.target |    1 +
>>>>   hw/apic.c       |   25 +++++++++++----
>>>>   hw/apic.h       |    1 +
>>>>   hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
>>>>   hw/pc.c         |   11 ++++--
>>>>   6 files changed, 174 insertions(+), 11 deletions(-)
>>>>   create mode 100644 hw/icc_bus.c
>>>>   create mode 100644 hw/icc_bus.h
>>>>
>>>> diff --git a/Makefile.target b/Makefile.target
>>>> index 9011f28..5607c6d 100644
>>>> --- a/Makefile.target
>>>> +++ b/Makefile.target
>>>> @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>>>>   obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>>>   obj-i386-y += testdev.o
>>>>   obj-i386-y += acpi.o acpi_piix4.o
>>>> +obj-i386-y += icc_bus.o
>>>>
>>>>   obj-i386-y += pcspk.o i8254.o
>>>>   obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
>>>> diff --git a/hw/apic.c b/hw/apic.c
>>>> index 69d6ac5..00d2297 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 "kvm.h"
>>>> +#include "exec-memory.h"
>>>
>>> Mmh, don't your rather want 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;
>>>> @@ -1104,9 +1105,20 @@ static const MemoryRegionOps apic_io_ops = {
>>>>       .endianness = DEVICE_NATIVE_ENDIAN,
>>>>   };
>>>>
>>>> -static int apic_init1(SysBusDevice *dev)
>>>> +/**/
>>>
>>> Empty comment.
>>>
>>>> +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) {
>>>> @@ -1114,7 +1126,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_region(dev,&s->io_memory);
>>>>
>>>>       s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
>>>>       s->idx = last_apic_idx++;
>>>> @@ -1122,7 +1133,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),
>>>> @@ -1138,7 +1149,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 c857d52..e2c0af5 100644
>>>> --- a/hw/apic.h
>>>> +++ b/hw/apic.h
>>>> @@ -20,6 +20,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..61a408e
>>>> --- /dev/null
>>>> +++ b/hw/icc_bus.c
>>>> @@ -0,0 +1,91 @@
>>>> +/* icc_bus.c
>>>> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
>>>
>>> Copyright?
>>>
>>>> + *
>>>> + * 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"
>>>> +
>>>> +static CPUS *dummy_cpus;
>>>
>>> Why "dummy"? Also, no statics please. The bus owner is the chipset
>>> (440fx), so embedded it there. That also avoid that strange "cpus"
>>> device below.
>>
>> That's an odd model IMHO.  The i440fx doesn't implement the ICC bus.  The ICC
>> bus is entirely independent of the northbridge.
>>
>> Maybe CPUSockets would be a better device name?
>>
> 
> The problem is that it is even something non-existent in the absence of
> an IOAPIC - which is an optional device for the i440fx/PIIX3, though we
> only model this for ISA PCs, not for PCI machines (and make a lot of
> mistakes in this area as I recently noticed). Still, we will need an ICC
> bus on a ISAPC from now on. Also not correct.
> 
> In an ideal world there would be no dummy device CPUSockets, just an
> optional unowned ICC bus. CPUs were not ICC bus devices, but could
> attach to one if its there. But I think this cannot be modeled yet. So
> we likely have to live with the proposed setup for now.

Mmh, maybe we could at least provide two types of CPUs instead: a sysbus
version for non-ICC setups and a ICC version if there is such a bus. The
only variance should be in two qdev descriptions and two MMIO map helpers.

Still leaves us with the dummy CPUSockets device, but that could be
removed at some later point. I presume empty vmsds create no traces in
the binary migration format, correct?

Jan

On Wed, Oct 19, 2011 at 03:42:27PM +0200, Jan Kiszka wrote:
> On 2011-10-19 15:33, Jan Kiszka wrote:
> > On 2011-10-19 14:54, Anthony Liguori wrote:
> >> On 10/19/2011 05:53 AM, Jan Kiszka wrote:
> >>> On 2011-10-19 03:55, pingfank@linux.vnet.ibm.com wrote:
> >>>> From: Liu Ping Fan<pingfank@linux.vnet.ibm.com>
> >>>>
> >>>> 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.
> >>>>
> >>>> Signed-off-by: liu ping fan<pingfank@linux.vnet.ibm.com>
> >>>> ---
> >>>>   Makefile.target |    1 +
> >>>>   hw/apic.c       |   25 +++++++++++----
> >>>>   hw/apic.h       |    1 +
> >>>>   hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>   hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
> >>>>   hw/pc.c         |   11 ++++--
> >>>>   6 files changed, 174 insertions(+), 11 deletions(-)
> >>>>   create mode 100644 hw/icc_bus.c
> >>>>   create mode 100644 hw/icc_bus.h
> >>>>
> >>>> diff --git a/Makefile.target b/Makefile.target
> >>>> index 9011f28..5607c6d 100644
> >>>> --- a/Makefile.target
> >>>> +++ b/Makefile.target
> >>>> @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
> >>>>   obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >>>>   obj-i386-y += testdev.o
> >>>>   obj-i386-y += acpi.o acpi_piix4.o
> >>>> +obj-i386-y += icc_bus.o
> >>>>
> >>>>   obj-i386-y += pcspk.o i8254.o
> >>>>   obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
> >>>> diff --git a/hw/apic.c b/hw/apic.c
> >>>> index 69d6ac5..00d2297 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 "kvm.h"
> >>>> +#include "exec-memory.h"
> >>>
> >>> Mmh, don't your rather want 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;
> >>>> @@ -1104,9 +1105,20 @@ static const MemoryRegionOps apic_io_ops = {
> >>>>       .endianness = DEVICE_NATIVE_ENDIAN,
> >>>>   };
> >>>>
> >>>> -static int apic_init1(SysBusDevice *dev)
> >>>> +/**/
> >>>
> >>> Empty comment.
> >>>
> >>>> +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) {
> >>>> @@ -1114,7 +1126,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_region(dev,&s->io_memory);
> >>>>
> >>>>       s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
> >>>>       s->idx = last_apic_idx++;
> >>>> @@ -1122,7 +1133,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),
> >>>> @@ -1138,7 +1149,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 c857d52..e2c0af5 100644
> >>>> --- a/hw/apic.h
> >>>> +++ b/hw/apic.h
> >>>> @@ -20,6 +20,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..61a408e
> >>>> --- /dev/null
> >>>> +++ b/hw/icc_bus.c
> >>>> @@ -0,0 +1,91 @@
> >>>> +/* icc_bus.c
> >>>> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> >>>
> >>> Copyright?
> >>>
> >>>> + *
> >>>> + * 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"
> >>>> +
> >>>> +static CPUS *dummy_cpus;
> >>>
> >>> Why "dummy"? Also, no statics please. The bus owner is the chipset
> >>> (440fx), so embedded it there. That also avoid that strange "cpus"
> >>> device below.
> >>
> >> That's an odd model IMHO.  The i440fx doesn't implement the ICC bus.  The ICC
> >> bus is entirely independent of the northbridge.
> >>
> >> Maybe CPUSockets would be a better device name?
> >>
> > 
> > The problem is that it is even something non-existent in the absence of
> > an IOAPIC - which is an optional device for the i440fx/PIIX3, though we
> > only model this for ISA PCs, not for PCI machines (and make a lot of
> > mistakes in this area as I recently noticed). Still, we will need an ICC
> > bus on a ISAPC from now on. Also not correct.
> > 
> > In an ideal world there would be no dummy device CPUSockets, just an
> > optional unowned ICC bus. CPUs were not ICC bus devices, but could
> > attach to one if its there. But I think this cannot be modeled yet. So
> > we likely have to live with the proposed setup for now.
Yeah, what makes modeling hard is the "unowned" ICC bus in the ideal world.
But obeying to qdev model requirement, there must be a device parent for ICC
bus, so we come to the compromise --CPUSockets and maybe it could be a holder
for other orphan buses besides ICC, when we run into the similar corner.
> 
> Mmh, maybe we could at least provide two types of CPUs instead: a sysbus
> version for non-ICC setups and a ICC version if there is such a bus. The
> only variance should be in two qdev descriptions and two MMIO map helpers.
This makes the model more ideal, but at a cost of code maintenance. As you
said, we could remove the dummy CPUSockets device at some later point, what
about leaving it handled all at once at that time.
> 
> Still leaves us with the dummy CPUSockets device, but that could be
> removed at some later point. I presume empty vmsds create no traces in
> the binary migration format, correct?
Yeah, you are right. I made the mistake because not familiar with vmsd.

Thanks and regards,
ping fan
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

On Wed, Oct 19, 2011 at 01:55,  <pingfank@linux.vnet.ibm.com> wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> 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.

Is this ICC bus or APIC hot plugging documented somewhere?

> Signed-off-by: liu ping fan <pingfank@linux.vnet.ibm.com>
> ---
>  Makefile.target |    1 +
>  hw/apic.c       |   25 +++++++++++----
>  hw/apic.h       |    1 +
>  hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
>  hw/pc.c         |   11 ++++--
>  6 files changed, 174 insertions(+), 11 deletions(-)
>  create mode 100644 hw/icc_bus.c
>  create mode 100644 hw/icc_bus.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 9011f28..5607c6d 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>  obj-i386-y += testdev.o
>  obj-i386-y += acpi.o acpi_piix4.o
> +obj-i386-y += icc_bus.o
>
>  obj-i386-y += pcspk.o i8254.o
>  obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
> diff --git a/hw/apic.c b/hw/apic.c
> index 69d6ac5..00d2297 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 "kvm.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;
> @@ -1104,9 +1105,20 @@ 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) {
> @@ -1114,7 +1126,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_region(dev, &s->io_memory);
>
>     s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
>     s->idx = last_apic_idx++;
> @@ -1122,7 +1133,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),
> @@ -1138,7 +1149,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 c857d52..e2c0af5 100644
> --- a/hw/apic.h
> +++ b/hw/apic.h
> @@ -20,6 +20,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..61a408e
> --- /dev/null
> +++ b/hw/icc_bus.c
> @@ -0,0 +1,91 @@
> +/* icc_bus.c
> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> + *
> + * 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"
> +
> +static CPUS *dummy_cpus;
> +
> +
> +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 &dummy_cpus->icc_bus->qbus;
> +}
> +
> +static void cpus_reset(DeviceState *d)
> +{
> +}
> +
> +/*Must be called before vcpu's creation*/
> +static int cpus_init(SysBusDevice *dev)
> +{
> +    CPUS *cpus = DO_UPCAST(CPUS, sysdev, dev);
> +    BusState *b = qbus_create(&icc_bus_info.qinfo,
> +                              &dummy_cpus->sysdev.qdev,
> +                              "icc");
> +    if (b == NULL) {
> +        return -1;
> +    }
> +    b->allow_hotplug = 1; /* Yes, we can */
> +    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
> +    dummy_cpus = cpus;
> +    return 0;
> +
> +}
> +
> +
> +static SysBusDeviceInfo cpus_info = {
> +    .init = cpus_init,
> +    .qdev.name = "cpus",
> +    .qdev.size = sizeof(CPUS),
> +    /*.qdev.vmsd = &vmstate_apic,*/
> +    .qdev.reset = cpus_reset,
> +    .qdev.no_user = 1,
> +};
> +
> +static void cpus_register_devices(void)
> +{
> +    sysbus_register_withprop(&cpus_info);
> +}
> +
> +device_init(cpus_register_devices)
> diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> new file mode 100644
> index 0000000..64ac153
> --- /dev/null
> +++ b/hw/icc_bus.h
> @@ -0,0 +1,56 @@
> +/* ICCBus.h
> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> + *
> + * 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"
> +
> +typedef struct CPUS CPUS;
> +typedef struct ICCBus ICCBus;
> +typedef struct ICCBusInfo ICCBusInfo;
> +typedef struct ICCBusDevice ICCBusDevice;
> +typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
> +
> +struct CPUS {
> +    SysBusDevice sysdev;
> +    ICCBus *icc_bus;
> +};
> +
> +struct ICCBus {
> +    BusState qbus;
> +};
> +
> +struct ICCBusInfo {
> +    BusInfo qinfo;
> +};
> +struct ICCBusDevice {
> +    DeviceState qdev;
> +};
> +
> +typedef int (*iccbus_initfn)(ICCBusDevice *dev);
> +
> +struct ICCBusDeviceInfo {
> +    DeviceInfo qdev;
> +    iccbus_initfn init;
> +};
> +
> +
> +void iccbus_register_devinfo(ICCBusDeviceInfo *info);
> +BusState *get_icc_bus(void);
> +
> +#endif
> diff --git a/hw/pc.c b/hw/pc.c
> index 6b3662e..78b7826 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"
> @@ -875,21 +876,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;
>     }
>
> @@ -955,6 +956,8 @@ CPUState *pc_new_cpu(const char *cpu_model)
>  void pc_cpus_init(const char *cpu_model)
>  {
>     int i;
> +    DeviceState *d = qdev_create(NULL, "cpus");
> +    qdev_init_nofail(d);

This makes the 'cpus' separate device from CPUs which only makes sense
now that LAPICs are in a way shared.

I think the final model should be that PC board should create the CPUs
first, specifying also if ICC should be created (no for machines
without APIC, ISA or i386). Then each CPU should create a per-CPU ICC
bus as needed. Near APIC init, the CPUs are queried if an ICC bus
exists, if yes, an APIC will be attached to this bus (for each CPU).
If the function get_icc_bus() is still needed, it should grab the bus
from CPUState. The APIC base can be changed per CPU.

I'm not sure that a full bus is needed for now, even if it could match
real HW better, since the memory API already provides the separation
needed. Perhaps this would be needed later to make IRQs per-CPU also,
or to put IOAPIC also to the bus?

>
>     /* init CPUs */
>     for(i = 0; i < smp_cpus; i++) {
> --
> 1.7.4.4
>
>
>

On 2011-10-23 14:40, Blue Swirl wrote:
> I'm not sure that a full bus is needed for now, even if it could match
> real HW better, since the memory API already provides the separation
> needed. Perhaps this would be needed later to make IRQs per-CPU also,
> or to put IOAPIC also to the bus?

The ICC interconnects LAPICs and IOAPICs. So it should next take over
the management of the local_apics array from apic.c and the ioapics
array from ioapic.c. It could implement generic message delivery
services. Every bus participant would then have a reception handler that
first checks the type and additional fields of a generic ICC message
and, on match, forwards it to the corresponding device model functions.
That would allow for something nicer than global apic_deliver_irq or
ioapic_eoi_broadcast.

That's clearly beyond the scope of this series but a good reason to
model the ICC as accurately as qdev allows right from the start.

Jan

On Sun, Oct 23, 2011 at 15:45, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-10-23 14:40, Blue Swirl wrote:
>> I'm not sure that a full bus is needed for now, even if it could match
>> real HW better, since the memory API already provides the separation
>> needed. Perhaps this would be needed later to make IRQs per-CPU also,
>> or to put IOAPIC also to the bus?
>
> The ICC interconnects LAPICs and IOAPICs.

But not between CPU core and its LAPIC?

> So it should next take over
> the management of the local_apics array from apic.c and the ioapics
> array from ioapic.c. It could implement generic message delivery
> services. Every bus participant would then have a reception handler that
> first checks the type and additional fields of a generic ICC message
> and, on match, forwards it to the corresponding device model functions.
> That would allow for something nicer than global apic_deliver_irq or
> ioapic_eoi_broadcast.
>
> That's clearly beyond the scope of this series but a good reason to
> model the ICC as accurately as qdev allows right from the start.

OK then, ICC could be a major cleanup.

On 2011-10-23 17:54, Blue Swirl wrote:
> On Sun, Oct 23, 2011 at 15:45, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-10-23 14:40, Blue Swirl wrote:
>>> I'm not sure that a full bus is needed for now, even if it could match
>>> real HW better, since the memory API already provides the separation
>>> needed. Perhaps this would be needed later to make IRQs per-CPU also,
>>> or to put IOAPIC also to the bus?
>>
>> The ICC interconnects LAPICs and IOAPICs.
> 
> But not between CPU core and its LAPIC?

Nope, that link is core-internal and has nothing to do with the ICC. See
e.g. Figure 2-2 of Intel's "MultiProcessor Specification".

Jan

On Sun, Oct 23, 2011 at 12:40:08PM +0000, Blue Swirl wrote:
> On Wed, Oct 19, 2011 at 01:55,  <pingfank@linux.vnet.ibm.com> wrote:
> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >
> > 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.
> 
> Is this ICC bus or APIC hot plugging documented somewhere?
> 
> > Signed-off-by: liu ping fan <pingfank@linux.vnet.ibm.com>
> > ---
> >  Makefile.target |    1 +
> >  hw/apic.c       |   25 +++++++++++----
> >  hw/apic.h       |    1 +
> >  hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
> >  hw/pc.c         |   11 ++++--
> >  6 files changed, 174 insertions(+), 11 deletions(-)
> >  create mode 100644 hw/icc_bus.c
> >  create mode 100644 hw/icc_bus.h
> >
> > diff --git a/Makefile.target b/Makefile.target
> > index 9011f28..5607c6d 100644
> > --- a/Makefile.target
> > +++ b/Makefile.target
> > @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
> >  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >  obj-i386-y += testdev.o
> >  obj-i386-y += acpi.o acpi_piix4.o
> > +obj-i386-y += icc_bus.o
> >
> >  obj-i386-y += pcspk.o i8254.o
> >  obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 69d6ac5..00d2297 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 "kvm.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;
> > @@ -1104,9 +1105,20 @@ 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) {
> > @@ -1114,7 +1126,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_region(dev, &s->io_memory);
> >
> >     s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
> >     s->idx = last_apic_idx++;
> > @@ -1122,7 +1133,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),
> > @@ -1138,7 +1149,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 c857d52..e2c0af5 100644
> > --- a/hw/apic.h
> > +++ b/hw/apic.h
> > @@ -20,6 +20,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..61a408e
> > --- /dev/null
> > +++ b/hw/icc_bus.c
> > @@ -0,0 +1,91 @@
> > +/* icc_bus.c
> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> > + *
> > + * 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"
> > +
> > +static CPUS *dummy_cpus;
> > +
> > +
> > +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 &dummy_cpus->icc_bus->qbus;
> > +}
> > +
> > +static void cpus_reset(DeviceState *d)
> > +{
> > +}
> > +
> > +/*Must be called before vcpu's creation*/
> > +static int cpus_init(SysBusDevice *dev)
> > +{
> > +    CPUS *cpus = DO_UPCAST(CPUS, sysdev, dev);
> > +    BusState *b = qbus_create(&icc_bus_info.qinfo,
> > +                              &dummy_cpus->sysdev.qdev,
> > +                              "icc");
> > +    if (b == NULL) {
> > +        return -1;
> > +    }
> > +    b->allow_hotplug = 1; /* Yes, we can */
> > +    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
> > +    dummy_cpus = cpus;
> > +    return 0;
> > +
> > +}
> > +
> > +
> > +static SysBusDeviceInfo cpus_info = {
> > +    .init = cpus_init,
> > +    .qdev.name = "cpus",
> > +    .qdev.size = sizeof(CPUS),
> > +    /*.qdev.vmsd = &vmstate_apic,*/
> > +    .qdev.reset = cpus_reset,
> > +    .qdev.no_user = 1,
> > +};
> > +
> > +static void cpus_register_devices(void)
> > +{
> > +    sysbus_register_withprop(&cpus_info);
> > +}
> > +
> > +device_init(cpus_register_devices)
> > diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> > new file mode 100644
> > index 0000000..64ac153
> > --- /dev/null
> > +++ b/hw/icc_bus.h
> > @@ -0,0 +1,56 @@
> > +/* ICCBus.h
> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> > + *
> > + * 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"
> > +
> > +typedef struct CPUS CPUS;
> > +typedef struct ICCBus ICCBus;
> > +typedef struct ICCBusInfo ICCBusInfo;
> > +typedef struct ICCBusDevice ICCBusDevice;
> > +typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
> > +
> > +struct CPUS {
> > +    SysBusDevice sysdev;
> > +    ICCBus *icc_bus;
> > +};
> > +
> > +struct ICCBus {
> > +    BusState qbus;
> > +};
> > +
> > +struct ICCBusInfo {
> > +    BusInfo qinfo;
> > +};
> > +struct ICCBusDevice {
> > +    DeviceState qdev;
> > +};
> > +
> > +typedef int (*iccbus_initfn)(ICCBusDevice *dev);
> > +
> > +struct ICCBusDeviceInfo {
> > +    DeviceInfo qdev;
> > +    iccbus_initfn init;
> > +};
> > +
> > +
> > +void iccbus_register_devinfo(ICCBusDeviceInfo *info);
> > +BusState *get_icc_bus(void);
> > +
> > +#endif
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 6b3662e..78b7826 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"
> > @@ -875,21 +876,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;
> >     }
> >
> > @@ -955,6 +956,8 @@ CPUState *pc_new_cpu(const char *cpu_model)
> >  void pc_cpus_init(const char *cpu_model)
> >  {
> >     int i;
> > +    DeviceState *d = qdev_create(NULL, "cpus");
> > +    qdev_init_nofail(d);
> 
> This makes the 'cpus' separate device from CPUs which only makes sense
> now that LAPICs are in a way shared.
> 
> I think the final model should be that PC board should create the CPUs
> first, specifying also if ICC should be created (no for machines
> without APIC, ISA or i386). Then each CPU should create a per-CPU ICC
> bus as needed. Near APIC init, the CPUs are queried if an ICC bus
> exists, if yes, an APIC will be attached to this bus (for each CPU).
> If the function get_icc_bus() is still needed, it should grab the bus
> from CPUState. The APIC base can be changed per CPU.
> 
So I will do like this in pc_init1(),
    if (need_icc_bus(global_cpu_model))
        create_icc_bus();

    pc_cpus_init(cpu_model);
Right?

But what about the parent bus of APICState? Do we need two models for APICState
static SysBusDeviceInfo apic_info and static ICCBusDeviceInfo apic_icc_info?
And what about APICState,just insert another bus stub, or create a new one?
    struct APICState {
        SysBusDevice busdev;
       ------------------> ICCBusDevice iccdev;
        MemoryRegion io_memory;
        .......
    }

Thanks,
ping fan

> I'm not sure that a full bus is needed for now, even if it could match
> real HW better, since the memory API already provides the separation
> needed. Perhaps this would be needed later to make IRQs per-CPU also,
> or to put IOAPIC also to the bus?
> 
> >
> >     /* init CPUs */
> >     for(i = 0; i < smp_cpus; i++) {
> > --
> > 1.7.4.4
> >
> >
> >

On Tue, Oct 25, 2011 at 08:55, liu ping fan <qemulist@gmail.com> wrote:
> On Sun, Oct 23, 2011 at 12:40:08PM +0000, Blue Swirl wrote:
>> On Wed, Oct 19, 2011 at 01:55,  <pingfank@linux.vnet.ibm.com> wrote:
>> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> >
>> > 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.
>>
>> Is this ICC bus or APIC hot plugging documented somewhere?
>>
>> > Signed-off-by: liu ping fan <pingfank@linux.vnet.ibm.com>
>> > ---
>> >  Makefile.target |    1 +
>> >  hw/apic.c       |   25 +++++++++++----
>> >  hw/apic.h       |    1 +
>> >  hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
>> >  hw/pc.c         |   11 ++++--
>> >  6 files changed, 174 insertions(+), 11 deletions(-)
>> >  create mode 100644 hw/icc_bus.c
>> >  create mode 100644 hw/icc_bus.h
>> >
>> > diff --git a/Makefile.target b/Makefile.target
>> > index 9011f28..5607c6d 100644
>> > --- a/Makefile.target
>> > +++ b/Makefile.target
>> > @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>> >  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>> >  obj-i386-y += testdev.o
>> >  obj-i386-y += acpi.o acpi_piix4.o
>> > +obj-i386-y += icc_bus.o
>> >
>> >  obj-i386-y += pcspk.o i8254.o
>> >  obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
>> > diff --git a/hw/apic.c b/hw/apic.c
>> > index 69d6ac5..00d2297 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 "kvm.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;
>> > @@ -1104,9 +1105,20 @@ 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) {
>> > @@ -1114,7 +1126,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_region(dev, &s->io_memory);
>> >
>> >     s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
>> >     s->idx = last_apic_idx++;
>> > @@ -1122,7 +1133,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),
>> > @@ -1138,7 +1149,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 c857d52..e2c0af5 100644
>> > --- a/hw/apic.h
>> > +++ b/hw/apic.h
>> > @@ -20,6 +20,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..61a408e
>> > --- /dev/null
>> > +++ b/hw/icc_bus.c
>> > @@ -0,0 +1,91 @@
>> > +/* icc_bus.c
>> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
>> > + *
>> > + * 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"
>> > +
>> > +static CPUS *dummy_cpus;
>> > +
>> > +
>> > +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 &dummy_cpus->icc_bus->qbus;
>> > +}
>> > +
>> > +static void cpus_reset(DeviceState *d)
>> > +{
>> > +}
>> > +
>> > +/*Must be called before vcpu's creation*/
>> > +static int cpus_init(SysBusDevice *dev)
>> > +{
>> > +    CPUS *cpus = DO_UPCAST(CPUS, sysdev, dev);
>> > +    BusState *b = qbus_create(&icc_bus_info.qinfo,
>> > +                              &dummy_cpus->sysdev.qdev,
>> > +                              "icc");
>> > +    if (b == NULL) {
>> > +        return -1;
>> > +    }
>> > +    b->allow_hotplug = 1; /* Yes, we can */
>> > +    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
>> > +    dummy_cpus = cpus;
>> > +    return 0;
>> > +
>> > +}
>> > +
>> > +
>> > +static SysBusDeviceInfo cpus_info = {
>> > +    .init = cpus_init,
>> > +    .qdev.name = "cpus",
>> > +    .qdev.size = sizeof(CPUS),
>> > +    /*.qdev.vmsd = &vmstate_apic,*/
>> > +    .qdev.reset = cpus_reset,
>> > +    .qdev.no_user = 1,
>> > +};
>> > +
>> > +static void cpus_register_devices(void)
>> > +{
>> > +    sysbus_register_withprop(&cpus_info);
>> > +}
>> > +
>> > +device_init(cpus_register_devices)
>> > diff --git a/hw/icc_bus.h b/hw/icc_bus.h
>> > new file mode 100644
>> > index 0000000..64ac153
>> > --- /dev/null
>> > +++ b/hw/icc_bus.h
>> > @@ -0,0 +1,56 @@
>> > +/* ICCBus.h
>> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
>> > + *
>> > + * 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"
>> > +
>> > +typedef struct CPUS CPUS;
>> > +typedef struct ICCBus ICCBus;
>> > +typedef struct ICCBusInfo ICCBusInfo;
>> > +typedef struct ICCBusDevice ICCBusDevice;
>> > +typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
>> > +
>> > +struct CPUS {
>> > +    SysBusDevice sysdev;
>> > +    ICCBus *icc_bus;
>> > +};
>> > +
>> > +struct ICCBus {
>> > +    BusState qbus;
>> > +};
>> > +
>> > +struct ICCBusInfo {
>> > +    BusInfo qinfo;
>> > +};
>> > +struct ICCBusDevice {
>> > +    DeviceState qdev;
>> > +};
>> > +
>> > +typedef int (*iccbus_initfn)(ICCBusDevice *dev);
>> > +
>> > +struct ICCBusDeviceInfo {
>> > +    DeviceInfo qdev;
>> > +    iccbus_initfn init;
>> > +};
>> > +
>> > +
>> > +void iccbus_register_devinfo(ICCBusDeviceInfo *info);
>> > +BusState *get_icc_bus(void);
>> > +
>> > +#endif
>> > diff --git a/hw/pc.c b/hw/pc.c
>> > index 6b3662e..78b7826 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"
>> > @@ -875,21 +876,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;
>> >     }
>> >
>> > @@ -955,6 +956,8 @@ CPUState *pc_new_cpu(const char *cpu_model)
>> >  void pc_cpus_init(const char *cpu_model)
>> >  {
>> >     int i;
>> > +    DeviceState *d = qdev_create(NULL, "cpus");
>> > +    qdev_init_nofail(d);
>>
>> This makes the 'cpus' separate device from CPUs which only makes sense
>> now that LAPICs are in a way shared.
>>
>> I think the final model should be that PC board should create the CPUs
>> first, specifying also if ICC should be created (no for machines
>> without APIC, ISA or i386). Then each CPU should create a per-CPU ICC
>> bus as needed. Near APIC init, the CPUs are queried if an ICC bus
>> exists, if yes, an APIC will be attached to this bus (for each CPU).
>> If the function get_icc_bus() is still needed, it should grab the bus
>> from CPUState. The APIC base can be changed per CPU.
>>
> So I will do like this in pc_init1(),
>    if (need_icc_bus(global_cpu_model))
>        create_icc_bus();
>
>    pc_cpus_init(cpu_model);
> Right?

Sorry, my model was not correct. So one ICC should be created at board
level if any APICs exist, each LAPIC should connect to it and later
also IOAPIC.

> But what about the parent bus of APICState? Do we need two models for APICState
> static SysBusDeviceInfo apic_info and static ICCBusDeviceInfo apic_icc_info?
> And what about APICState,just insert another bus stub, or create a new one?
>    struct APICState {
>        SysBusDevice busdev;
>       ------------------> ICCBusDevice iccdev;
>        MemoryRegion io_memory;
>        .......
>    }

Looks OK.

> Thanks,
> ping fan
>
>> I'm not sure that a full bus is needed for now, even if it could match
>> real HW better, since the memory API already provides the separation
>> needed. Perhaps this would be needed later to make IRQs per-CPU also,
>> or to put IOAPIC also to the bus?
>>
>> >
>> >     /* init CPUs */
>> >     for(i = 0; i < smp_cpus; i++) {
>> > --
>> > 1.7.4.4
>> >
>> >
>> >
>

Hi,

I want to rework on it according to your comments. Before that, just want to make clear that I understand your meanings exactly :)

According to the previous discussion, I will model the system according the rule -- if there is APIC in the system (including UP and MP), ICC bus will be created, otherwise no.

But there is a special case in UP scene,that is, if we make 8259a connect directly to APIC without using IOAPIC, as showed by Figure 3-3 in intel's "MultiProcessor Specification", I think the rule can also be suitable.

So in board level initialization--pc1_init(), I will check _cpuid_features_ & CPUID_APIC to judge whether to create ICC or not.

Any objection?

Thanks and regards,
ping fan


On Tue, Oct 25, 2011 at 08:24:21PM +0000, Blue Swirl wrote:
> On Tue, Oct 25, 2011 at 08:55, liu ping fan <qemulist@gmail.com> wrote:
> > On Sun, Oct 23, 2011 at 12:40:08PM +0000, Blue Swirl wrote:
> >> On Wed, Oct 19, 2011 at 01:55,  <pingfank@linux.vnet.ibm.com> wrote:
> >> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> >
> >> > 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.
> >>
> >> Is this ICC bus or APIC hot plugging documented somewhere?
> >>
> >> > Signed-off-by: liu ping fan <pingfank@linux.vnet.ibm.com>
> >> > ---
> >> >  Makefile.target |    1 +
> >> >  hw/apic.c       |   25 +++++++++++----
> >> >  hw/apic.h       |    1 +
> >> >  hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
> >> >  hw/pc.c         |   11 ++++--
> >> >  6 files changed, 174 insertions(+), 11 deletions(-)
> >> >  create mode 100644 hw/icc_bus.c
> >> >  create mode 100644 hw/icc_bus.h
> >> >
> >> > diff --git a/Makefile.target b/Makefile.target
> >> > index 9011f28..5607c6d 100644
> >> > --- a/Makefile.target
> >> > +++ b/Makefile.target
> >> > @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
> >> >  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >> >  obj-i386-y += testdev.o
> >> >  obj-i386-y += acpi.o acpi_piix4.o
> >> > +obj-i386-y += icc_bus.o
> >> >
> >> >  obj-i386-y += pcspk.o i8254.o
> >> >  obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
> >> > diff --git a/hw/apic.c b/hw/apic.c
> >> > index 69d6ac5..00d2297 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 "kvm.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;
> >> > @@ -1104,9 +1105,20 @@ 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) {
> >> > @@ -1114,7 +1126,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_region(dev, &s->io_memory);
> >> >
> >> >     s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
> >> >     s->idx = last_apic_idx++;
> >> > @@ -1122,7 +1133,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),
> >> > @@ -1138,7 +1149,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 c857d52..e2c0af5 100644
> >> > --- a/hw/apic.h
> >> > +++ b/hw/apic.h
> >> > @@ -20,6 +20,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..61a408e
> >> > --- /dev/null
> >> > +++ b/hw/icc_bus.c
> >> > @@ -0,0 +1,91 @@
> >> > +/* icc_bus.c
> >> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> >> > + *
> >> > + * 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"
> >> > +
> >> > +static CPUS *dummy_cpus;
> >> > +
> >> > +
> >> > +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 &dummy_cpus->icc_bus->qbus;
> >> > +}
> >> > +
> >> > +static void cpus_reset(DeviceState *d)
> >> > +{
> >> > +}
> >> > +
> >> > +/*Must be called before vcpu's creation*/
> >> > +static int cpus_init(SysBusDevice *dev)
> >> > +{
> >> > +    CPUS *cpus = DO_UPCAST(CPUS, sysdev, dev);
> >> > +    BusState *b = qbus_create(&icc_bus_info.qinfo,
> >> > +                              &dummy_cpus->sysdev.qdev,
> >> > +                              "icc");
> >> > +    if (b == NULL) {
> >> > +        return -1;
> >> > +    }
> >> > +    b->allow_hotplug = 1; /* Yes, we can */
> >> > +    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
> >> > +    dummy_cpus = cpus;
> >> > +    return 0;
> >> > +
> >> > +}
> >> > +
> >> > +
> >> > +static SysBusDeviceInfo cpus_info = {
> >> > +    .init = cpus_init,
> >> > +    .qdev.name = "cpus",
> >> > +    .qdev.size = sizeof(CPUS),
> >> > +    /*.qdev.vmsd = &vmstate_apic,*/
> >> > +    .qdev.reset = cpus_reset,
> >> > +    .qdev.no_user = 1,
> >> > +};
> >> > +
> >> > +static void cpus_register_devices(void)
> >> > +{
> >> > +    sysbus_register_withprop(&cpus_info);
> >> > +}
> >> > +
> >> > +device_init(cpus_register_devices)
> >> > diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> >> > new file mode 100644
> >> > index 0000000..64ac153
> >> > --- /dev/null
> >> > +++ b/hw/icc_bus.h
> >> > @@ -0,0 +1,56 @@
> >> > +/* ICCBus.h
> >> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> >> > + *
> >> > + * 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"
> >> > +
> >> > +typedef struct CPUS CPUS;
> >> > +typedef struct ICCBus ICCBus;
> >> > +typedef struct ICCBusInfo ICCBusInfo;
> >> > +typedef struct ICCBusDevice ICCBusDevice;
> >> > +typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
> >> > +
> >> > +struct CPUS {
> >> > +    SysBusDevice sysdev;
> >> > +    ICCBus *icc_bus;
> >> > +};
> >> > +
> >> > +struct ICCBus {
> >> > +    BusState qbus;
> >> > +};
> >> > +
> >> > +struct ICCBusInfo {
> >> > +    BusInfo qinfo;
> >> > +};
> >> > +struct ICCBusDevice {
> >> > +    DeviceState qdev;
> >> > +};
> >> > +
> >> > +typedef int (*iccbus_initfn)(ICCBusDevice *dev);
> >> > +
> >> > +struct ICCBusDeviceInfo {
> >> > +    DeviceInfo qdev;
> >> > +    iccbus_initfn init;
> >> > +};
> >> > +
> >> > +
> >> > +void iccbus_register_devinfo(ICCBusDeviceInfo *info);
> >> > +BusState *get_icc_bus(void);
> >> > +
> >> > +#endif
> >> > diff --git a/hw/pc.c b/hw/pc.c
> >> > index 6b3662e..78b7826 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"
> >> > @@ -875,21 +876,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;
> >> >     }
> >> >
> >> > @@ -955,6 +956,8 @@ CPUState *pc_new_cpu(const char *cpu_model)
> >> >  void pc_cpus_init(const char *cpu_model)
> >> >  {
> >> >     int i;
> >> > +    DeviceState *d = qdev_create(NULL, "cpus");
> >> > +    qdev_init_nofail(d);
> >>
> >> This makes the 'cpus' separate device from CPUs which only makes sense
> >> now that LAPICs are in a way shared.
> >>
> >> I think the final model should be that PC board should create the CPUs
> >> first, specifying also if ICC should be created (no for machines
> >> without APIC, ISA or i386). Then each CPU should create a per-CPU ICC
> >> bus as needed. Near APIC init, the CPUs are queried if an ICC bus
> >> exists, if yes, an APIC will be attached to this bus (for each CPU).
> >> If the function get_icc_bus() is still needed, it should grab the bus
> >> from CPUState. The APIC base can be changed per CPU.
> >>
> > So I will do like this in pc_init1(),
> >    if (need_icc_bus(global_cpu_model))
> >        create_icc_bus();
> >
> >    pc_cpus_init(cpu_model);
> > Right?
> 
> Sorry, my model was not correct. So one ICC should be created at board
> level if any APICs exist, each LAPIC should connect to it and later
> also IOAPIC.
> 
> > But what about the parent bus of APICState? Do we need two models for APICState
> > static SysBusDeviceInfo apic_info and static ICCBusDeviceInfo apic_icc_info?
> > And what about APICState,just insert another bus stub, or create a new one?
> >    struct APICState {
> >        SysBusDevice busdev;
> >       ------------------> ICCBusDevice iccdev;
> >        MemoryRegion io_memory;
> >        .......
> >    }
> 
> Looks OK.
> 
> > Thanks,
> > ping fan
> >
> >> I'm not sure that a full bus is needed for now, even if it could match
> >> real HW better, since the memory API already provides the separation
> >> needed. Perhaps this would be needed later to make IRQs per-CPU also,
> >> or to put IOAPIC also to the bus?
> >>
> >> >
> >> >     /* init CPUs */
> >> >     for(i = 0; i < smp_cpus; i++) {
> >> > --
> >> > 1.7.4.4
> >> >
> >> >
> >> >
> >

On Thu, Oct 27, 2011 at 08:41, liu ping fan <qemulist@gmail.com> wrote:
> Hi,
>
> I want to rework on it according to your comments. Before that, just want to make clear that I understand your meanings exactly :)
>
> According to the previous discussion, I will model the system according the rule -- if there is APIC in the system (including UP and MP), ICC bus will be created, otherwise no.
>
> But there is a special case in UP scene,that is, if we make 8259a connect directly to APIC without using IOAPIC, as showed by Figure 3-3 in intel's "MultiProcessor Specification", I think the rule can also be suitable.

I think this is handled for some part by pic_irq_request() in pc.c.

> So in board level initialization--pc1_init(), I will check _cpuid_features_ & CPUID_APIC to judge whether to create ICC or not.
>
> Any objection?

OK for me.

> Thanks and regards,
> ping fan
>
>
> On Tue, Oct 25, 2011 at 08:24:21PM +0000, Blue Swirl wrote:
>> On Tue, Oct 25, 2011 at 08:55, liu ping fan <qemulist@gmail.com> wrote:
>> > On Sun, Oct 23, 2011 at 12:40:08PM +0000, Blue Swirl wrote:
>> >> On Wed, Oct 19, 2011 at 01:55,  <pingfank@linux.vnet.ibm.com> wrote:
>> >> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> >> >
>> >> > 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.
>> >>
>> >> Is this ICC bus or APIC hot plugging documented somewhere?
>> >>
>> >> > Signed-off-by: liu ping fan <pingfank@linux.vnet.ibm.com>
>> >> > ---
>> >> >  Makefile.target |    1 +
>> >> >  hw/apic.c       |   25 +++++++++++----
>> >> >  hw/apic.h       |    1 +
>> >> >  hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >  hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
>> >> >  hw/pc.c         |   11 ++++--
>> >> >  6 files changed, 174 insertions(+), 11 deletions(-)
>> >> >  create mode 100644 hw/icc_bus.c
>> >> >  create mode 100644 hw/icc_bus.h
>> >> >
>> >> > diff --git a/Makefile.target b/Makefile.target
>> >> > index 9011f28..5607c6d 100644
>> >> > --- a/Makefile.target
>> >> > +++ b/Makefile.target
>> >> > @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>> >> >  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>> >> >  obj-i386-y += testdev.o
>> >> >  obj-i386-y += acpi.o acpi_piix4.o
>> >> > +obj-i386-y += icc_bus.o
>> >> >
>> >> >  obj-i386-y += pcspk.o i8254.o
>> >> >  obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
>> >> > diff --git a/hw/apic.c b/hw/apic.c
>> >> > index 69d6ac5..00d2297 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 "kvm.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;
>> >> > @@ -1104,9 +1105,20 @@ 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) {
>> >> > @@ -1114,7 +1126,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_region(dev, &s->io_memory);
>> >> >
>> >> >     s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
>> >> >     s->idx = last_apic_idx++;
>> >> > @@ -1122,7 +1133,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),
>> >> > @@ -1138,7 +1149,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 c857d52..e2c0af5 100644
>> >> > --- a/hw/apic.h
>> >> > +++ b/hw/apic.h
>> >> > @@ -20,6 +20,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..61a408e
>> >> > --- /dev/null
>> >> > +++ b/hw/icc_bus.c
>> >> > @@ -0,0 +1,91 @@
>> >> > +/* icc_bus.c
>> >> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
>> >> > + *
>> >> > + * 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"
>> >> > +
>> >> > +static CPUS *dummy_cpus;
>> >> > +
>> >> > +
>> >> > +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 &dummy_cpus->icc_bus->qbus;
>> >> > +}
>> >> > +
>> >> > +static void cpus_reset(DeviceState *d)
>> >> > +{
>> >> > +}
>> >> > +
>> >> > +/*Must be called before vcpu's creation*/
>> >> > +static int cpus_init(SysBusDevice *dev)
>> >> > +{
>> >> > +    CPUS *cpus = DO_UPCAST(CPUS, sysdev, dev);
>> >> > +    BusState *b = qbus_create(&icc_bus_info.qinfo,
>> >> > +                              &dummy_cpus->sysdev.qdev,
>> >> > +                              "icc");
>> >> > +    if (b == NULL) {
>> >> > +        return -1;
>> >> > +    }
>> >> > +    b->allow_hotplug = 1; /* Yes, we can */
>> >> > +    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
>> >> > +    dummy_cpus = cpus;
>> >> > +    return 0;
>> >> > +
>> >> > +}
>> >> > +
>> >> > +
>> >> > +static SysBusDeviceInfo cpus_info = {
>> >> > +    .init = cpus_init,
>> >> > +    .qdev.name = "cpus",
>> >> > +    .qdev.size = sizeof(CPUS),
>> >> > +    /*.qdev.vmsd = &vmstate_apic,*/
>> >> > +    .qdev.reset = cpus_reset,
>> >> > +    .qdev.no_user = 1,
>> >> > +};
>> >> > +
>> >> > +static void cpus_register_devices(void)
>> >> > +{
>> >> > +    sysbus_register_withprop(&cpus_info);
>> >> > +}
>> >> > +
>> >> > +device_init(cpus_register_devices)
>> >> > diff --git a/hw/icc_bus.h b/hw/icc_bus.h
>> >> > new file mode 100644
>> >> > index 0000000..64ac153
>> >> > --- /dev/null
>> >> > +++ b/hw/icc_bus.h
>> >> > @@ -0,0 +1,56 @@
>> >> > +/* ICCBus.h
>> >> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
>> >> > + *
>> >> > + * 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"
>> >> > +
>> >> > +typedef struct CPUS CPUS;
>> >> > +typedef struct ICCBus ICCBus;
>> >> > +typedef struct ICCBusInfo ICCBusInfo;
>> >> > +typedef struct ICCBusDevice ICCBusDevice;
>> >> > +typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
>> >> > +
>> >> > +struct CPUS {
>> >> > +    SysBusDevice sysdev;
>> >> > +    ICCBus *icc_bus;
>> >> > +};
>> >> > +
>> >> > +struct ICCBus {
>> >> > +    BusState qbus;
>> >> > +};
>> >> > +
>> >> > +struct ICCBusInfo {
>> >> > +    BusInfo qinfo;
>> >> > +};
>> >> > +struct ICCBusDevice {
>> >> > +    DeviceState qdev;
>> >> > +};
>> >> > +
>> >> > +typedef int (*iccbus_initfn)(ICCBusDevice *dev);
>> >> > +
>> >> > +struct ICCBusDeviceInfo {
>> >> > +    DeviceInfo qdev;
>> >> > +    iccbus_initfn init;
>> >> > +};
>> >> > +
>> >> > +
>> >> > +void iccbus_register_devinfo(ICCBusDeviceInfo *info);
>> >> > +BusState *get_icc_bus(void);
>> >> > +
>> >> > +#endif
>> >> > diff --git a/hw/pc.c b/hw/pc.c
>> >> > index 6b3662e..78b7826 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"
>> >> > @@ -875,21 +876,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;
>> >> >     }
>> >> >
>> >> > @@ -955,6 +956,8 @@ CPUState *pc_new_cpu(const char *cpu_model)
>> >> >  void pc_cpus_init(const char *cpu_model)
>> >> >  {
>> >> >     int i;
>> >> > +    DeviceState *d = qdev_create(NULL, "cpus");
>> >> > +    qdev_init_nofail(d);
>> >>
>> >> This makes the 'cpus' separate device from CPUs which only makes sense
>> >> now that LAPICs are in a way shared.
>> >>
>> >> I think the final model should be that PC board should create the CPUs
>> >> first, specifying also if ICC should be created (no for machines
>> >> without APIC, ISA or i386). Then each CPU should create a per-CPU ICC
>> >> bus as needed. Near APIC init, the CPUs are queried if an ICC bus
>> >> exists, if yes, an APIC will be attached to this bus (for each CPU).
>> >> If the function get_icc_bus() is still needed, it should grab the bus
>> >> from CPUState. The APIC base can be changed per CPU.
>> >>
>> > So I will do like this in pc_init1(),
>> >    if (need_icc_bus(global_cpu_model))
>> >        create_icc_bus();
>> >
>> >    pc_cpus_init(cpu_model);
>> > Right?
>>
>> Sorry, my model was not correct. So one ICC should be created at board
>> level if any APICs exist, each LAPIC should connect to it and later
>> also IOAPIC.
>>
>> > But what about the parent bus of APICState? Do we need two models for APICState
>> > static SysBusDeviceInfo apic_info and static ICCBusDeviceInfo apic_icc_info?
>> > And what about APICState,just insert another bus stub, or create a new one?
>> >    struct APICState {
>> >        SysBusDevice busdev;
>> >       ------------------> ICCBusDevice iccdev;
>> >        MemoryRegion io_memory;
>> >        .......
>> >    }
>>
>> Looks OK.
>>
>> > Thanks,
>> > ping fan
>> >
>> >> I'm not sure that a full bus is needed for now, even if it could match
>> >> real HW better, since the memory API already provides the separation
>> >> needed. Perhaps this would be needed later to make IRQs per-CPU also,
>> >> or to put IOAPIC also to the bus?
>> >>
>> >> >
>> >> >     /* init CPUs */
>> >> >     for(i = 0; i < smp_cpus; i++) {
>> >> > --
>> >> > 1.7.4.4
>> >> >
>> >> >
>> >> >
>> >
>