From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46761) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLV5H-0000h4-Kk for qemu-devel@nongnu.org; Wed, 02 Nov 2011 03:18:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RLV5F-00025d-N9 for qemu-devel@nongnu.org; Wed, 02 Nov 2011 03:18:03 -0400 Received: from fmmailgate02.web.de ([217.72.192.227]:38654) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLV5E-00025R-Vi for qemu-devel@nongnu.org; Wed, 02 Nov 2011 03:18:01 -0400 Received: from moweb002.kundenserver.de (moweb002.kundenserver.de [172.19.20.108]) by fmmailgate02.web.de (Postfix) with ESMTP id 4F5A91B9DA35D for ; Wed, 2 Nov 2011 08:17:59 +0100 (CET) Message-ID: <4EB0EE9E.70802@web.de> Date: Wed, 02 Nov 2011 08:17:50 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <1320133290-5547-1-git-send-email-pingfank@linux.vnet.ibm.com> <4EAFF88E.1050406@web.de> <20111102014051.GA5474@oc8440477808.ibm.com> In-Reply-To: <20111102014051.GA5474@oc8440477808.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig7A190593A8CB9BC063B1823A" Subject: Re: [Qemu-devel] [PATCH V2] Introduce a new bus "ICC" to connect APIC List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liu ping fan Cc: blauwirbel@gmail.com, aliguori@us.ibm.com, pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org, ryanh@us.ibm.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig7A190593A8CB9BC063B1823A Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 2011-11-02 02:40, liu ping fan wrote: > On Tue, Nov 01, 2011 at 02:47:58PM +0100, Jan Kiszka wrote: >> On 2011-11-01 08:41, 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 | 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) +=3D kvmclock.o >>> obj-i386-$(CONFIG_SPICE) +=3D qxl.o qxl-logger.o qxl-render.o >>> obj-i386-y +=3D testdev.o >>> obj-i386-y +=3D acpi.o acpi_piix4.o >>> +obj-i386-y +=3D icc_bus.o >>> =20 >>> obj-i386-y +=3D pcspk.o i8254.o >>> obj-i386-$(CONFIG_KVM_PIT) +=3D 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" >>> =20 >>> /* APIC Local Vector Table */ >>> #define APIC_LVT_TIMER 0 >>> @@ -80,7 +81,7 @@ >>> typedef struct APICState APICState; >>> =20 >>> 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 =3D {= >>> .endianness =3D DEVICE_NATIVE_ENDIAN, >>> }; >>> =20 >>> -static int apic_init1(SysBusDevice *dev) >>> +int apic_mmio_map(DeviceState *dev, target_phys_addr_t base) >>> { >>> - APICState *s =3D FROM_SYSBUS(APICState, dev); >>> + APICState *s =3D 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 =3D DO_UPCAST(APICState, busdev, dev); >>> static int last_apic_idx; >>> =20 >>> if (last_apic_idx >=3D 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); >>> =20 >>> s->timer =3D qemu_new_timer_ns(vm_clock, apic_timer, s); >>> s->idx =3D last_apic_idx++; >>> @@ -1122,7 +1132,7 @@ static int apic_init1(SysBusDevice *dev) >>> return 0; >>> } >>> =20 >>> -static SysBusDeviceInfo apic_info =3D { >>> +static ICCBusDeviceInfo apic_info =3D { >>> .init =3D apic_init1, >>> .qdev.name =3D "apic", >>> .qdev.size =3D sizeof(APICState), >>> @@ -1138,7 +1148,7 @@ static SysBusDeviceInfo apic_info =3D { >>> =20 >>> static void apic_register_devices(void) >>> { >>> - sysbus_register_withprop(&apic_info); >>> + iccbus_register_devinfo(&apic_info); >>> } >>> =20 >>> 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); >>> =20 >>> /* 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 >>> + */ >>> +#include "icc_bus.h" >>> + >>> +static CPUSockets *cpu_sockets; >>> + >>> +static ICCBusInfo icc_bus_info =3D { >>> + .qinfo.name =3D "icc", >>> + .qinfo.size =3D sizeof(ICCBus), >>> + .qinfo.props =3D (Property[]) { >>> + DEFINE_PROP_END_OF_LIST(), >>> + } >>> +}; >>> + >>> +static int iccbus_device_init(DeviceState *dev, DeviceInfo *base) >>> +{ >>> + ICCBusDeviceInfo *info =3D container_of(base, ICCBusDeviceInfo, = qdev); >>> + ICCBusDevice *idev =3D DO_UPCAST(ICCBusDevice, qdev, dev); >>> + >>> + return info->init(idev); >>> +} >>> + >>> +void iccbus_register_devinfo(ICCBusDeviceInfo *info) >>> +{ >>> + info->qdev.init =3D iccbus_device_init; >>> + info->qdev.bus_info =3D &icc_bus_info.qinfo; >>> + assert(info->qdev.size >=3D 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 =3D DO_UPCAST(CPUSockets, sysdev, dev); >>> + BusState *b =3D qbus_create(&icc_bus_info.qinfo, >>> + &cpu_sockets->sysdev.qdev, >>> + "icc"); >>> + if (b =3D=3D NULL) { >>> + return -1; >>> + } >>> + b->allow_hotplug =3D 1; /* Yes, we can */ >>> + cpus->icc_bus =3D DO_UPCAST(ICCBus, qbus, b); >>> + cpu_sockets =3D cpus; >>> + return 0; >>> + >>> +} >>> + >>> +static const VMStateDescription vmstate_cpusockets =3D { >>> + .name =3D "cpusockets", >>> + .version_id =3D 1, >>> + .minimum_version_id =3D 0, >>> + .fields =3D (VMStateField[]) { >>> + VMSTATE_END_OF_LIST() >>> + } >>> +}; >>> + >>> +static SysBusDeviceInfo cpusockets_info =3D { >>> + .init =3D cpusockets_init, >>> + .qdev.name =3D "cpusockets", >>> + .qdev.size =3D sizeof(CPUSockets), >>> + .qdev.vmsd =3D &vmstate_cpusockets, >>> + .qdev.reset =3D NULL, >>> + .qdev.no_user =3D 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 >>> + */ >>> +#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; >>> =20 >>> - dev =3D qdev_create(NULL, "apic"); >>> + b =3D get_icc_bus(); >>> + dev =3D 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 =3D sysbus_from_qdev(dev); >>> =20 >>> /* XXX: mapping more APICs at the same memory location */ >>> if (apic_mapped =3D=3D 0) { >>> /* NOTE: the APIC is directly connected to the CPU - it is n= ot >>> 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 =3D 1; >>> } >>> =20 >>> 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 >>> @@ -102,8 +101,21 @@ static void pc_init1(MemoryRegion *system_memory= , >>> MemoryRegion *ram_memory; >>> MemoryRegion *pci_memory; >>> =20 >>> + if (cpu_model =3D=3D NULL) { >>> +#ifdef TARGET_X86_64 >>> + cpu_model =3D "qemu64"; >>> +#else >>> + cpu_model =3D "qemu32"; >>> +#endif >>> + } >>> + >>> global_cpu_model =3D cpu_model; >>> =20 >>> + if (cpu_has_apic_feature(cpu_model)) { >>> + DeviceState *d =3D qdev_create(NULL, "cpusockets"); >>> + qdev_init_nofail(d); >>> + } >>> + >>> pc_cpus_init(cpu_model); >>> =20 >>> 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 ind= ex, 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) >>> } >>> } >>> =20 >>> +int cpu_has_apic_feature(const char *cpu_model) >>> +{ >>> + x86_def_t def1, *def =3D &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 =3D &def1; >> >> Have you tested -smp + -cpu 486? I suspect it's broken after this patc= h. >> > Yeah, you are right! So I will use the condition "if (cpu_has_apic_feat= ure(cpu_model) || smps > 1)" to decide whether ICC bus is needed or not? Yep. >> 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". Then sorry. I was assuming it would detect the broken indention of that "if cpu_has_apic_feature" block. Jan --------------enig7A190593A8CB9BC063B1823A Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk6w7p4ACgkQitSsb3rl5xQAowCg3TjdEb1cvwuIY2vWevXUqobt KEoAoLlYggzkxLxBRmI1QqMi0156iOIi =s9VI -----END PGP SIGNATURE----- --------------enig7A190593A8CB9BC063B1823A--