On 07/04/2015 15:26, Denis V. Lunev wrote: > On 07/04/15 16:18, Paolo Bonzini wrote: >> >> >> 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. > > just $0.02 :) > > why don't > bsp = s->apicbase & MSR_IA32_APICBASE_BSP; > s->apicbase = > APIC_DEFAULT_ADDRESS | bsp | MSR_IA32_APICBASE_ENABLE; > in this case. This looks the same from the textual point of view.
Yes. Would you like to send a patch? Paolo
