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

[Andreas Färber]

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.

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;

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.

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)