Re: [Qemu-devel] [PATCH 1/1] Introduce a new bus "ICC" to connect APIC

[Jan Kiszka <jan.kiszka@siemens.com>]

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

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