Am 07.04.2015 um 15:44 schrieb Denis V. Lunev: > On 07/04/15 16:29, Paolo Bonzini wrote: >> >> >> 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 >> > > no prob, just give me 2 minutes. Side note, bsp will become uint32_t > and we will loose tracepoint inside cpu_get_apic_base() on this > path...
Yes, I intentionally left it out above as I don't think we need to trace the local usage here. Worth mentioning in the commit message though. Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)
