On Tue, 07 Apr 2015 13:57:34 +0200 Andreas Färber <afaer...@suse.de> wrote:
> Am 07.04.2015 um 13:09 schrieb Andreas Färber: > > Am 07.04.2015 um 12:54 schrieb Paolo Bonzini: > >> On 07/04/2015 12:44, Andreas Färber wrote: > >>>>> It can change at runtime, though, if you're using the KVM > >>>>> in-kernel LAPIC. > >>> Got a pointer? A quick git-grep doesn't show anything in hw/ or > >>> kvm-all.c or target-i386/ assigning cpu_index, so it'll always > >>> have the initial value. > >> > >> Not cpu_index, s->apicbase's MSR_IA32_APICBASE_BSP bit can change > >> with KVM in-kernel LAPIC. It cannot change with QEMU's userspace > >> LAPIC. > >> > >> Because it can change, you have to call apic_designate_bsp for all > >> CPUs and not only on CPU 0. > > > > Now I'm even more confused. Surely CPUState is initially > > zero-initialized. Then we designate one as BSP on reset. That > > should be the same result as designating all non-BSP CPUs, no? The > > only way that would change is then cpu_index == 0 goes away > > (hot-unplug, not implemented yet), and in that case it would be > > about designating a different CPU, not about un-designating one. > > > > If this is some issue with sync'ing state back and forth before > > QEMU and KVM then the real issue has not been explained. guest can set BSP flag in apicbase of non the first CPU and then o reset on KVM exit it will be propagated into QEMU's state kvm_arch_post_run() -> cpu_set_apic_base() as result with current code after reset we will have the first CPU with BSP bit and another one since nobody bothered to clear it. That's what this patch does. [...] > Unless I'm missing something, since we are in the APIC device's reset > function, this is effectively a twisted way of writing: > > bsp = s->apicbase & MSR_IA32_APICBASE_BSP; > s->apicbase = APIC_DEFAULT_ADDRESS | > (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE; > > In which case we already relied on s->cpu and could thus simply change > this to something like: > > bsp = CPU(s->cpu)->cpu_index == 0; ^^^ shouldn't be part of APIC code, APIC shouldn't have anything to do with cpu_index. we do above in CPU code currently /* We hard-wire the BSP to the first CPU. */ if (s->cpu_index == 0) { apic_designate_bsp(cpu->apic_state); } > s->apicbase = APIC_DEFAULT_ADDRESS | > (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE; > > Then the apicbase manipulation would be nicely encapsulated in the > APIC rather than the APIC reset retaining it and the CPU reset > meddling with its state. > > Andreas >