Message ID | 555EDE75.4010300@cn.fujitsu.com |
---|---|
State | New |
Headers | show |
On Fri, May 22, 2015 at 03:44:53PM +0800, Chen Fan wrote: > 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. > [...] > 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(); > } What about all the other architectures and machines that may expect different reset ordering, and that already register their own CPU reset handlers? If x86 has specific CPU reset ordering requirements, we should be able to ensure the expected ordering in x86-specific code (in pc.c?), not hardcode reset ordering for all machines. (BTW, what was the motivation to move qemu_register_reset() from pc.c to target-i386/cpu.c? The only architectures that register reset handlers inside the CPU code are x86 and s390x, all others register reset handlers inside machine code.)
Am 22.05.2015 um 18:56 schrieb Eduardo Habkost: > On Fri, May 22, 2015 at 03:44:53PM +0800, Chen Fan wrote: >> 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(); >> } > > What about all the other architectures and machines that may expect > different reset ordering, and that already register their own CPU reset > handlers? > > If x86 has specific CPU reset ordering requirements, we should be able > to ensure the expected ordering in x86-specific code (in pc.c?), not > hardcode reset ordering for all machines. +1 In particular pseries has special ordering requirements. > (BTW, what was the motivation to move qemu_register_reset() from pc.c to > target-i386/cpu.c? The only architectures that register reset handlers > inside the CPU code are x86 and s390x, all others register reset > handlers inside machine code.) I don't remember the motivation, it was Anthony overturning my objection against that exception from the rule though. If we can bring x86 or the other targets back in line, feel free to make an RFC. I still have an old branch with initial reset support for alpha and rth had a patch to that effect, too. Regards, Andreas
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)