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

[Jan Kiszka]

On 2011-11-08 02:44, liu ping fan wrote:
> On Sun, Nov 06, 2011 at 07:06:29PM +0100, Jan Kiszka wrote:
>> On 2011-11-03 10:30, 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..09aece9 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"
>>
>> That change looks stray.
>>
> Adopted,
>>>  #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) || smp_cpus > 1) {
>>> +            DeviceState *d = qdev_create(NULL, "cpusockets");
>>> +            qdev_init_nofail(d);
>>
>> Indention is still off.
>>
> Adopted, BTW, could you tell me how do you find this(checkpatch.pl seems not 
> to work on it), so I can avoid such mistake in future :), thanks.

I guess it's a checkpatch deficit.

> 
>> While at it, why not move all this into pc_cpus_init? That way other PC
>> chipsets could once make use of it as well.
>>
> As we have discussed, ICC is a board level facility, so I think it is better 
> to put it in pc_init1 than in pc_cpus_init, where I consider it as the 
> initialization of CPUs.

The board can control the ICC creation by preventing APICs (via CPU type
and QEMUMachine.max_cpus). But the instantiation is a generic feature
again. So pc_cpus_init makes more sense.

Jan

signature.asc
Description: OpenPGP digital signature