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

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

Den