Re: [Qemu-devel] target-i386: clear bsp bit when designating bsp

[Paolo Bonzini <pbonzini@redhat.com>]

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