On Wed, Apr 27, 2011 at 8:32 PM, Jan Kiszka <jan.kis...@siemens.com> wrote: > On 2011-04-27 19:17, Blue Swirl wrote: >> 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. > > Maybe as an inline helper. > > But all this apic cpu_* helpers are not really beautiful. Logically, > they should take a CPUState, not an APIC. Or they should be called apic_*.
Well, cpu_{s,g}et_apic_base() are in the wrong place. TPR is shared between CR8 and APIC, currently it is handled by APIC so the functions could be renamed. >> >>> 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. > > Changing is not a big issue. But we will only add code this way, > unfortunately not just move it around: we will still have to load and > sync the apicbase for older versions. Perhaps we could start deprecating old versions. For example, v0.16 could deprecate v0.14 or older and v0.17 drop support for them and deprecate v0.15. > >> >>> >>> 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. > > Required for APIC-less mode (otherwise there would be no BSP). Perhaps the check should be moved to CPU level then.