On 07/04/2015 13:57, Andreas Färber wrote: >> > If this is some issue with sync'ing state back and forth before QEMU and >> > KVM then the real issue has not been explained. > Hm, hw/intc/apic_common.c:apic_reset_common() has: > > bsp = cpu_is_bsp(s->cpu); > s->apicbase = APIC_DEFAULT_ADDRESS | > (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE; > > What this is doing is really: > > bsp = cpu_get_apic_base(s->cpu->apic_state) & MSR_IA32_APICBASE_BSP; > s->apicbase = APIC_DEFAULT_ADDRESS | > (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE; > > 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;
Yes, this is more readable. > 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; > 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. ... but I find this worse. apic_designate_bsp corresponds to hardware executing the MP initialization protocol. Paolo
