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

[liu ping fan]

On Tue, Nov 01, 2011 at 02:47:58PM +0100, Jan Kiszka wrote:
> On 2011-11-01 08:41, address@hidden wrote:
> > From: Liu Ping Fan <address@hidden>
> > 
> > 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 <address@hidden>
> > ---
> >  Makefile.target     |    1 +
> >  hw/apic.c           |   24 +++++++++----
> >  hw/apic.h           |    1 +
> >  hw/icc_bus.c        |   92 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/icc_bus.h        |   61 +++++++++++++++++++++++++++++++++
> >  hw/pc.c             |    9 +++--
> >  hw/pc_piix.c        |   14 +++++++-
> >  target-i386/cpu.h   |    1 +
> >  target-i386/cpuid.c |   16 +++++++++
> >  9 files changed, 207 insertions(+), 12 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..34fa1dd 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,19 @@ static const MemoryRegionOps apic_io_ops = {
> >      .endianness = DEVICE_NATIVE_ENDIAN,
> >  };
> >  
> > -static int apic_init1(SysBusDevice *dev)
> > +int apic_mmio_map(DeviceState *dev, target_phys_addr_t base)
> >  {
> > -    APICState *s = FROM_SYSBUS(APICState, dev);
> > +    APICState *s = DO_UPCAST(APICState, busdev.qdev, dev);
> > +
> > +    memory_region_add_subregion(get_system_memory(),
> > +                                base,
> > +                                &s->io_memory);
> > +    return 0;
> > +}
> > +
> > +static int apic_init1(ICCBusDevice *dev)
> > +{
> > +    APICState *s = DO_UPCAST(APICState, busdev, dev);
> >      static int last_apic_idx;
> >  
> >      if (last_apic_idx >= MAX_APICS) {
> > @@ -1114,7 +1125,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 +1132,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 +1148,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..ac88f2e
> > --- /dev/null
> > +++ b/hw/icc_bus.c
> > @@ -0,0 +1,92 @@
> > +/* icc_bus.c
> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> > + *
> > + * Copyright IBM, Corp. 2011
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see 
> > <http://www.gnu.org/licenses/>
> > + */
> > +#include "icc_bus.h"
> > +
> > +static CPUSockets *cpu_sockets;
> > +
> > +static ICCBusInfo icc_bus_info = {
> > +    .qinfo.name = "icc",
> > +    .qinfo.size = sizeof(ICCBus),
> > +    .qinfo.props = (Property[]) {
> > +        DEFINE_PROP_END_OF_LIST(),
> > +    }
> > +};
> > +
> > +static int iccbus_device_init(DeviceState *dev, DeviceInfo *base)
> > +{
> > +    ICCBusDeviceInfo *info = container_of(base, ICCBusDeviceInfo, qdev);
> > +    ICCBusDevice *idev = DO_UPCAST(ICCBusDevice, qdev, dev);
> > +
> > +    return info->init(idev);
> > +}
> > +
> > +void iccbus_register_devinfo(ICCBusDeviceInfo *info)
> > +{
> > +    info->qdev.init = iccbus_device_init;
> > +    info->qdev.bus_info = &icc_bus_info.qinfo;
> > +    assert(info->qdev.size >= sizeof(ICCBusDevice));
> > +    qdev_register(&info->qdev);
> > +}
> > +
> > +BusState *get_icc_bus(void)
> > +{
> > +    return &cpu_sockets->icc_bus->qbus;
> > +}
> > +
> > +/*Must be called before vcpu's creation*/
> > +static int cpusockets_init(SysBusDevice *dev)
> > +{
> > +    CPUSockets *cpus = DO_UPCAST(CPUSockets, sysdev, dev);
> > +    BusState *b = qbus_create(&icc_bus_info.qinfo,
> > +                              &cpu_sockets->sysdev.qdev,
> > +                              "icc");
> > +    if (b == NULL) {
> > +        return -1;
> > +    }
> > +    b->allow_hotplug = 1; /* Yes, we can */
> > +    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
> > +    cpu_sockets = cpus;
> > +    return 0;
> > +
> > +}
> > +
> > +static const VMStateDescription vmstate_cpusockets = {
> > +    .name = "cpusockets",
> > +    .version_id = 1,
> > +    .minimum_version_id = 0,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static SysBusDeviceInfo cpusockets_info = {
> > +    .init = cpusockets_init,
> > +    .qdev.name = "cpusockets",
> > +    .qdev.size = sizeof(CPUSockets),
> > +    .qdev.vmsd = &vmstate_cpusockets,
> > +    .qdev.reset = NULL,
> > +    .qdev.no_user = 1,
> > +};
> > +
> > +static void cpusockets_register_devices(void)
> > +{
> > +    sysbus_register_withprop(&cpusockets_info);
> > +}
> > +
> > +device_init(cpusockets_register_devices)
> > diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> > new file mode 100644
> > index 0000000..9f10e1e
> > --- /dev/null
> > +++ b/hw/icc_bus.h
> > @@ -0,0 +1,61 @@
> > +/* ICCBus.h
> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> > + *
> > + * Copyright IBM, Corp. 2011
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see 
> > <http://www.gnu.org/licenses/>
> > + */
> > +#ifndef QEMU_ICC_H
> > +#define QEMU_ICC_H
> > +
> > +#include "qdev.h"
> > +#include "sysbus.h"
> > +
> > +typedef struct CPUSockets CPUSockets;
> > +typedef struct ICCBus ICCBus;
> > +typedef struct ICCBusInfo ICCBusInfo;
> > +typedef struct ICCBusDevice ICCBusDevice;
> > +typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
> > +
> > +struct CPUSockets {
> > +    SysBusDevice sysdev;
> > +    ICCBus *icc_bus;
> > +};
> > +
> > +struct CPUSInfo {
> > +    DeviceInfo info;
> > +};
> > +
> > +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..ffdca64 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;
> >      }
> >  
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 7055591..7c6f42d 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -41,7 +41,6 @@
> >  #include "blockdev.h"
> >  #include "smbus.h"
> >  #include "xen.h"
> > -#include "memory.h"
> >  #include "exec-memory.h"
> >  #ifdef CONFIG_XEN
> >  #  include <xen/hvm/hvm_info_table.h>
> > @@ -102,8 +101,21 @@ static void pc_init1(MemoryRegion *system_memory,
> >      MemoryRegion *ram_memory;
> >      MemoryRegion *pci_memory;
> >  
> > +    if (cpu_model == NULL) {
> > +#ifdef TARGET_X86_64
> > +        cpu_model = "qemu64";
> > +#else
> > +        cpu_model = "qemu32";
> > +#endif
> > +    }
> > +
> >      global_cpu_model = cpu_model;
> >  
> > +    if (cpu_has_apic_feature(cpu_model)) {
> > +            DeviceState *d = qdev_create(NULL, "cpusockets");
> > +            qdev_init_nofail(d);
> > +    }
> > +
> >      pc_cpus_init(cpu_model);
> >  
> >      if (kvmclock_enabled) {
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 2a071f2..abdeb40 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -884,6 +884,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> > uint32_t count,
> >                     uint32_t *eax, uint32_t *ebx,
> >                     uint32_t *ecx, uint32_t *edx);
> >  int cpu_x86_register (CPUX86State *env, const char *cpu_model);
> > +int cpu_has_apic_feature(const char *cpu_model);
> >  void cpu_clear_apic_feature(CPUX86State *env);
> >  void host_cpuid(uint32_t function, uint32_t count,
> >                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t 
> > *edx);
> > diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
> > index f179999..beaef5b 100644
> > --- a/target-i386/cpuid.c
> > +++ b/target-i386/cpuid.c
> > @@ -850,6 +850,22 @@ void x86_cpu_list(FILE *f, fprintf_function 
> > cpu_fprintf, const char *optarg)
> >      }
> >  }
> >  
> > +int cpu_has_apic_feature(const char *cpu_model)
> > +{
> > +    x86_def_t def1, *def = &def1;
> > +
> > +    memset(def, 0, sizeof(*def));
> > +
> > +    if (cpu_x86_find_by_name(def, cpu_model) < 0) {
> > +        return 0;
> > +    }
> > +    if (def->features & CPUID_APIC) {
> > +        return 1;
> > +    } else {
> > +        return 0;
> > +    }
> > +}
> > +
> >  int cpu_x86_register (CPUX86State *env, const char *cpu_model)
> >  {
> >      x86_def_t def1, *def = &def1;
> 
> Have you tested -smp + -cpu 486? I suspect it's broken after this patch.
> 
Yeah, you are right! So I will use the condition "if 
(cpu_has_apic_feature(cpu_model) || smps > 1)" to decide whether ICC bus is 
needed or not?
> And please use checkpatch before posting.
> 
Do you mean scripts/checkpatch.pl? I has used it, and "total: 0 errors, 0 
warnings, 319 lines checked".

Thanks and regards,
ping fan

> Jan
>