From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:48035) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RGVfv-0005nj-6S for qemu-devel@nongnu.org; Wed, 19 Oct 2011 08:55:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RGVfo-0007W4-Qy for qemu-devel@nongnu.org; Wed, 19 Oct 2011 08:55:14 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:59902) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RGVfo-0007Q5-CU for qemu-devel@nongnu.org; Wed, 19 Oct 2011 08:55:08 -0400 Received: from /spool/local by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 19 Oct 2011 06:54:56 -0600 Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p9JCspiX102624 for ; Wed, 19 Oct 2011 06:54:52 -0600 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p9JCspd9012168 for ; Wed, 19 Oct 2011 06:54:51 -0600 Message-ID: <4E9EC898.7080200@us.ibm.com> Date: Wed, 19 Oct 2011 07:54:48 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1318989353-5576-1-git-send-email-pingfank@linux.vnet.ibm.com> <4E9EAC24.9090901@siemens.com> In-Reply-To: <4E9EAC24.9090901@siemens.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] Introduce a new bus "ICC" to connect APIC List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org 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 >> >> 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 >> --- >> 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 >> + */ >> +#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 >> + */ >> +#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 >