qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Igor Mammedov
Subject: Re: [Qemu-devel] target-i386: clear bsp bit when designating bsp
Date: Tue, 7 Apr 2015 15:14:48 +0200

On Tue, 07 Apr 2015 13:57:34 +0200
Andreas Färber <address@hidden> 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);
    }


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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]