On Sun, Oct 23, 2011 at 12:40:08PM +0000, Blue Swirl wrote: > On Wed, Oct 19, 2011 at 01:55, <pingf...@linux.vnet.ibm.com> wrote: > > From: Liu Ping Fan <pingf...@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 <pingf...@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 > > > > > >