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

["Andreas Färber" <afaerber@suse.de>]

Am 07.04.2015 um 15:14 schrieb Igor Mammedov:
> On Tue, 07 Apr 2015 13:57:34 +0200
> Andreas Färber <afaerber@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);
>     }

I know, that's what this patch is changing, and I am saying that by the
same logic the CPU has no business fiddling with the APIC's apicbase
field when the APIC's reset is touching that very same field.

And I do remember that we arrived at this solution to get to a halfway
simple generic reset semantics, moving an external BSP designation into
the reset handling. However, this patch is making the twisted logic
worse IMO.

Andreas

> 
> 
>>     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
>>
> 


-- 
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)