On Wed, 20 May 2015 10:40:48 +0800
Zhu Guihua <zhugh.f...@cn.fujitsu.com> wrote:
> From: Chen Fan <chen.fan.f...@cn.fujitsu.com>
>
> After CPU hotplug has been converted to BUS-less hot-plug infrastructure,
> the only function ICC bus performs is to propagate reset to LAPICs. However
> LAPIC could be reset by its parent (CPU) directly when CPU is being reset.
> Do so and drop ~200LOC of not needed anymore ICCBus related code.
>
> Signed-off-by: Chen Fan <chen.fan.f...@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.f...@cn.fujitsu.com>
This patch regresses emulated APIC,
during RHEL7 boot:
[ 1.073487] ------------[ cut here ]------------
[ 1.074019] WARNING: at arch/x86/kernel/apic/apic.c:1401
setup_local_APIC+0x268/0x320()
[ 1.075011] Modules linked in:
[ 1.076474] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.10.0.sort+ #100
[ 1.077012] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.8.1-0-g4adadbd-20150316_085822-nilsson.home.kraxel.org 04/01/2014
[ 1.078011] 0000000000000000 00000000d1b49dbb ffff88007c787da8
ffffffff81649983
[ 1.082011] ffff88007c787de0 ffffffff810b3241 0000000000000001
0000000000000000
[ 1.085012] 00000000000000f0 0000000000000000 00000000ffffffff
ffff88007c787df0
[ 1.088012] Call Trace:
[ 1.089019] [<ffffffff81649983>] dump_stack+0x19/0x1b
[ 1.090017] [<ffffffff810b3241>] warn_slowpath_common+0x61/0x80
[ 1.091015] [<ffffffff810b336a>] warn_slowpath_null+0x1a/0x20
[ 1.092016] [<ffffffff81089ae8>] setup_local_APIC+0x268/0x320
[ 1.093019] [<ffffffff81ad4f02>] native_smp_prepare_cpus+0x294/0x35b
[ 1.094018] [<ffffffff81ac1133>] kernel_init_freeable+0xbb/0x217
[ 1.095017] [<ffffffff81636fe0>] ? rest_init+0x80/0x80
[ 1.096015] [<ffffffff81636fee>] kernel_init+0xe/0x180
[ 1.097016] [<ffffffff816598fc>] ret_from_fork+0x7c/0xb0
[ 1.098016] [<ffffffff81636fe0>] ? rest_init+0x80/0x80
[ 1.099017] ---[ end trace d99eba50bffa17c5 ]---
void setup_local_APIC(void)
...
} while (queued && max_loops > 0);
WARN_ON(max_loops <= 0); <=== here
...
reproducer:
qemu-system-x86_64 -enable-kvm -m 2048 -smp 4 -machine kernel_irqchip=off
rhel7.img
or just slower plain TCG
qemu-system-x86_64 -m 2048 -smp 4 rhel7.img
it happens only on VM startup, there isn't any warning when booting after reset.
> ---
> hw/i386/pc.c | 19 ++++---------------
> hw/i386/pc_piix.c | 9 +--------
> hw/i386/pc_q35.c | 9 +--------
> hw/intc/apic_common.c | 5 ++---
> include/hw/i386/apic_internal.h | 7 ++++---
> include/hw/i386/pc.h | 2 +-
> target-i386/cpu.c | 14 ++++++--------
> 7 files changed, 19 insertions(+), 46 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index bc06bb4..4458dbb 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -58,7 +58,6 @@
> #include "qemu/config-file.h"
> #include "hw/acpi/acpi.h"
> #include "hw/acpi/cpu_hotplug.h"
> -#include "hw/cpu/icc_bus.h"
> #include "hw/boards.h"
> #include "hw/pci/pci_host.h"
> #include "acpi-build.h"
> @@ -990,23 +989,16 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> level)
> }
>
> static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> - DeviceState *icc_bridge, Error **errp)
> + Error **errp)
> {
> X86CPU *cpu = NULL;
> Error *local_err = NULL;
>
> - if (icc_bridge == NULL) {
> - error_setg(&local_err, "Invalid icc-bridge value");
> - goto out;
> - }
> -
> cpu = cpu_x86_create(cpu_model, &local_err);
> if (local_err != NULL) {
> goto out;
> }
>
> - qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
> -
> object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>
> @@ -1023,7 +1015,6 @@ static const char *current_cpu_model;
>
> void pc_hot_add_cpu(const int64_t id, Error **errp)
> {
> - DeviceState *icc_bridge;
> X86CPU *cpu;
> int64_t apic_id = x86_cpu_apic_id_from_index(id);
> Error *local_err = NULL;
> @@ -1052,9 +1043,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
> return;
> }
>
> - icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
> - TYPE_ICC_BRIDGE, NULL));
> - cpu = pc_new_cpu(current_cpu_model, apic_id, icc_bridge, &local_err);
> + cpu = pc_new_cpu(current_cpu_model, apic_id, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> @@ -1062,7 +1051,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
> object_unref(OBJECT(cpu));
> }
>
> -void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
> +void pc_cpus_init(const char *cpu_model)
> {
> int i;
> X86CPU *cpu = NULL;
> @@ -1088,7 +1077,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState
> *icc_bridge)
>
> for (i = 0; i < smp_cpus; i++) {
> cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
> - icc_bridge, &error);
> + &error);
> if (error) {
> error_report_err(error);
> exit(1);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1fe7bfb..6004580 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -39,7 +39,6 @@
> #include "hw/kvm/clock.h"
> #include "sysemu/sysemu.h"
> #include "hw/sysbus.h"
> -#include "hw/cpu/icc_bus.h"
> #include "sysemu/arch_init.h"
> #include "sysemu/block-backend.h"
> #include "hw/i2c/smbus.h"
> @@ -98,7 +97,6 @@ static void pc_init1(MachineState *machine,
> MemoryRegion *ram_memory;
> MemoryRegion *pci_memory;
> MemoryRegion *rom_memory;
> - DeviceState *icc_bridge;
> FWCfgState *fw_cfg = NULL;
> PcGuestInfo *guest_info;
> ram_addr_t lowmem;
> @@ -143,11 +141,7 @@ static void pc_init1(MachineState *machine,
> exit(1);
> }
>
> - icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> - object_property_add_child(qdev_get_machine(), "icc-bridge",
> - OBJECT(icc_bridge), NULL);
> -
> - pc_cpus_init(machine->cpu_model, icc_bridge);
> + pc_cpus_init(machine->cpu_model);
>
> if (kvm_enabled() && kvmclock_enabled) {
> kvmclock_create();
> @@ -230,7 +224,6 @@ static void pc_init1(MachineState *machine,
> if (pci_enabled) {
> ioapic_init_gsi(gsi_state, "i440fx");
> }
> - qdev_init_nofail(icc_bridge);
>
> pc_register_ferr_irq(gsi[13]);
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index dcc17c0..3b89e6a 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -43,7 +43,6 @@
> #include "hw/ide/pci.h"
> #include "hw/ide/ahci.h"
> #include "hw/usb.h"
> -#include "hw/cpu/icc_bus.h"
> #include "qemu/error-report.h"
>
> /* ICH9 AHCI has 6 ports */
> @@ -85,7 +84,6 @@ static void pc_q35_init(MachineState *machine)
> int i;
> ICH9LPCState *ich9_lpc;
> PCIDevice *ahci;
> - DeviceState *icc_bridge;
> PcGuestInfo *guest_info;
> ram_addr_t lowmem;
> DriveInfo *hd[MAX_SATA_PORTS];
> @@ -132,11 +130,7 @@ static void pc_q35_init(MachineState *machine)
> exit(1);
> }
>
> - icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> - object_property_add_child(qdev_get_machine(), "icc-bridge",
> - OBJECT(icc_bridge), NULL);
> -
> - pc_cpus_init(machine->cpu_model, icc_bridge);
> + pc_cpus_init(machine->cpu_model);
> pc_acpi_init("q35-acpi-dsdt.aml");
>
> kvmclock_create();
> @@ -240,7 +234,6 @@ static void pc_q35_init(MachineState *machine)
> if (pci_enabled) {
> ioapic_init_gsi(gsi_state, "q35");
> }
> - qdev_init_nofail(icc_bridge);
>
> pc_register_ferr_irq(gsi[13]);
>
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 1c83753..edb4e42 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -430,13 +430,12 @@ static Property apic_properties_common[] = {
>
> static void apic_common_class_init(ObjectClass *klass, void *data)
> {
> - ICCDeviceClass *idc = ICC_DEVICE_CLASS(klass);
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> dc->vmsd = &vmstate_apic_common;
> dc->reset = apic_reset_common;
> dc->props = apic_properties_common;
> - idc->realize = apic_common_realize;
> + dc->realize = apic_common_realize;
> /*
> * Reason: APIC and CPU need to be wired up by
> * x86_cpu_apic_create()
> @@ -446,7 +445,7 @@ static void apic_common_class_init(ObjectClass *klass,
> void *data)
>
> static const TypeInfo apic_common_type = {
> .name = TYPE_APIC_COMMON,
> - .parent = TYPE_ICC_DEVICE,
> + .parent = TYPE_DEVICE,
> .instance_size = sizeof(APICCommonState),
> .class_size = sizeof(APICCommonClass),
> .class_init = apic_common_class_init,
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index dc7a89d..08d6f9b 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -21,7 +21,6 @@
> #define QEMU_APIC_INTERNAL_H
>
> #include "exec/memory.h"
> -#include "hw/cpu/icc_bus.h"
> #include "qemu/timer.h"
>
> /* APIC Local Vector Table */
> @@ -78,7 +77,7 @@ typedef struct APICCommonState APICCommonState;
>
> typedef struct APICCommonClass
> {
> - ICCDeviceClass parent_class;
> + DeviceClass parent_class;
>
> DeviceRealize realize;
> void (*set_base)(APICCommonState *s, uint64_t val);
> @@ -93,7 +92,9 @@ typedef struct APICCommonClass
> } APICCommonClass;
>
> struct APICCommonState {
> - ICCDevice busdev;
> + /*< private >*/
> + DeviceState parent_obj;
> + /*< public >*/
>
> MemoryRegion io_memory;
> X86CPU *cpu;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1b35168..7c9d044 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -163,7 +163,7 @@ extern int fd_bootchk;
> void pc_register_ferr_irq(qemu_irq irq);
> void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>
> -void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge);
> +void pc_cpus_init(const char *cpu_model);
> void pc_hot_add_cpu(const int64_t id, Error **errp);
> void pc_acpi_init(const char *default_dsdt);
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index f83e526..4080909 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -42,7 +42,6 @@
>
> #include "sysemu/sysemu.h"
> #include "hw/qdev-properties.h"
> -#include "hw/cpu/icc_bus.h"
> #ifndef CONFIG_USER_ONLY
> #include "hw/xen/xen.h"
> #include "hw/i386/apic_internal.h"
> @@ -2595,6 +2594,11 @@ static void x86_cpu_reset(CPUState *s)
>
> xcc->parent_reset(s);
>
> + /* since APIC is a bus-less device, propagate reset to it manually */
> + if (cpu->apic_state) {
> + device_reset(DEVICE(cpu->apic_state));
> + }
> +
> memset(env, 0, offsetof(CPUX86State, cpuid_level));
>
> tlb_flush(s, 1);
> @@ -2718,7 +2722,6 @@ static void mce_init(X86CPU *cpu)
> #ifndef CONFIG_USER_ONLY
> static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> {
> - DeviceState *dev = DEVICE(cpu);
> APICCommonState *apic;
> const char *apic_type = "apic";
>
> @@ -2728,11 +2731,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error
> **errp)
> apic_type = "xen-apic";
> }
>
> - cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
> - if (cpu->apic_state == NULL) {
> - error_setg(errp, "APIC device '%s' could not be created", apic_type);
> - return;
> - }
> + cpu->apic_state = DEVICE(object_new(apic_type));
>
> object_property_add_child(OBJECT(cpu), "apic",
> OBJECT(cpu->apic_state), NULL);
> @@ -2969,7 +2968,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc,
> void *data)
>
> xcc->parent_realize = dc->realize;
> dc->realize = x86_cpu_realizefn;
> - dc->bus_type = TYPE_ICC_BUS;
> dc->props = x86_cpu_properties;
>
> xcc->parent_reset = cc->reset;
