On Wed, Apr 27, 2011 at 10:11 AM, Jan Kiszka <jan.kis...@web.de> wrote: > On 2011-04-26 21:59, Blue Swirl wrote: >> On Tue, Apr 26, 2011 at 9:55 PM, Jan Kiszka <jan.kis...@web.de> wrote: >>> On 2011-04-26 20:00, Blue Swirl wrote: >>>> On Tue, Apr 26, 2011 at 11:50 AM, Jan Kiszka <jan.kis...@siemens.com> >>>> wrote: >>>>> Instead of having an extra reset function at machine level and special >>>>> code for processing INIT, move the initialization of halted into the >>>>> cpu reset handler. >>>> >>>> Nack. A CPU is designated as a BSP at board level. CPUs do not need to >>>> know about this at all. >>> >>> That's why we have cpu_is_bsp() in pc.c. >>> >>> Obviously, every CPU (which includes the APIC) must know if it is >>> supposed to be BP or AP. It would be unable to enter the right state >>> after reset otherwise (e.g. Intel SDM 9.1). cpu_is_bsp() is basically >>> reporting the result of the MP init protocol in condensed from. >> >> Intel 64 and IA-32 Architectures Software Developer’s Manual vol 3A, >> 7.5.1 says that the protocol result is stored in APIC MSR. I think we >> should be using that instead. For example, the board could call >> cpu_designate_bsp() to set the BSP flag in MSR. Then cpu_is_bsp() >> would only check the MSR, which naturally belongs to the CPU/APIC >> domain. > > Something like this? The original patch has to be rebased on top.
How about not deleting cpu_is_bsp() but moving it to apic.c, as a check for the BSP flag? That would simplify the patches a bit. > I'm still struggling how to deal with apicbase long-term. I doesn't > belong to the APIC, but it's saved/restored there. Guess we should move > it to the CPU's vmstate. OTOH, changing vmstates only for the sake of > minor refactorings is also not very attractive. CPU should be the correct place. You could wait until the vmstate is changed anyway, or be the first. > > Jan > > --- > hw/apic.c | 18 +++++++++++++----- > hw/apic.h | 2 +- > hw/pc.c | 14 +++++++------- > target-i386/helper.c | 3 ++- > target-i386/kvm.c | 5 +++-- > 5 files changed, 26 insertions(+), 16 deletions(-) > > diff --git a/hw/apic.c b/hw/apic.c > index 9febf40..31ac6cd 100644 > --- a/hw/apic.c > +++ b/hw/apic.c > @@ -318,7 +318,7 @@ uint64_t cpu_get_apic_base(DeviceState *d) > > trace_cpu_get_apic_base(s ? (uint64_t)s->apicbase: 0); > > - return s ? s->apicbase : 0; > + return s ? s->apicbase : MSR_IA32_APICBASE_BSP; This does not look OK.
