Re: [Qemu-devel] [PATCH v6 3/4] cpu/apic: drop icc bus/bridge

[Chen Fan <chen.fan.fnst@cn.fujitsu.com>]

On 05/20/2015 10:53 PM, Igor Mammedov wrote:
> On Wed, 20 May 2015 10:40:48 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> From: Chen Fan <chen.fan.fnst@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.fnst@cn.fujitsu.com>
>> Signed-off-by: Zhu Guihua <zhugh.fnst@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.
Hi Igor, Thanks for you pointing it out.

I had found that the problem appeared after we moved the apic reset into 
cpu reset.

the original operation is that there are devices (such as hpet, rtc) 
reset before apic reset,
when these devices reset, it would send irq to apic, before the change, 
the apic reset
is behind these devices reset. so the apic register is set to default 
values.

but after the change, thanks to the cpu reset is before the qemu system 
reset which causes
that the apic reset ahead the other devices reset. but before guest boot 
up, the irq request
should be rejected.  so when linux enable local apic, it would found 
there were irr requests.
then cause warn_on.

so I make a enforce cpu reset after qemu system reset. and I also change 
the apicbase value
by default, because cpu_set_apic_base()/apic_set_base() can not enable 
the APICBASE_ENABLE
bit in apic model. and I have test it, it seems work fine. the hasty 
change is that:

diff --git a/cpus.c b/cpus.c
index de6469f..b99e6ec 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1196,6 +1196,15 @@ void resume_all_vcpus(void)
      }
  }

+void reset_all_vcpus(void)
+{
+    CPUState *cpu;
+
+    CPU_FOREACH(cpu) {
+        cpu_reset(cpu);
+    }
+}
+
  /* For temporary buffers for forming a name */
  #define VCPU_THREAD_NAME_SIZE 16

diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index 3f162a9..5c1e9f2 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -5,6 +5,7 @@
  void qemu_init_cpu_loop(void);
  void resume_all_vcpus(void);
  void pause_all_vcpus(void);
+void reset_all_vcpus(void);
  void cpu_stop_current(void);

  void cpu_synchronize_all_states(void);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4080909..18bbe35 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2694,13 +2694,6 @@ bool cpu_is_bsp(X86CPU *cpu)
  {
      return cpu_get_apic_base(cpu->apic_state) & MSR_IA32_APICBASE_BSP;
  }
-
-/* TODO: remove me, when reset over QOM tree is implemented */
-static void x86_cpu_machine_reset_cb(void *opaque)
-{
-    X86CPU *cpu = opaque;
-    cpu_reset(CPU(cpu));
-}
  #endif

  static void mce_init(X86CPU *cpu)
@@ -2739,8 +2732,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error 
**errp)
      /* TODO: convert to link<> */
      apic = APIC_COMMON(cpu->apic_state);
      apic->cpu = cpu;
-    cpu_set_apic_base(cpu->apic_state,
-                      APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE);
+    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
  }

  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
@@ -2801,8 +2793,6 @@ static void x86_cpu_realizefn(DeviceState *dev, 
Error **errp)
      }

  #ifndef CONFIG_USER_ONLY
-    qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
-
      if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
          x86_cpu_apic_create(cpu, &local_err);
          if (local_err != NULL) {
diff --git a/vl.c b/vl.c
index 15bccc4..0c53053 100644
--- a/vl.c
+++ b/vl.c
@@ -1618,6 +1618,7 @@ void qemu_devices_reset(void)
      QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
          re->func(re->opaque);
      }
+    reset_all_vcpus();
  }

  void qemu_system_reset(bool report)

>> ---
>>   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;
> .
>