From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:55151) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QF8bP-0001Ys-6g for qemu-devel@nongnu.org; Wed, 27 Apr 2011 13:32:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QF8bO-0005LL-6q for qemu-devel@nongnu.org; Wed, 27 Apr 2011 13:32:39 -0400 Received: from goliath.siemens.de ([192.35.17.28]:27624) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QF8bN-0005L5-TC for qemu-devel@nongnu.org; Wed, 27 Apr 2011 13:32:38 -0400 Message-ID: <4DB85333.8080908@siemens.com> Date: Wed, 27 Apr 2011 19:32:35 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <4DB68768.3050700@siemens.com> <4DB7151A.4000401@web.de> <4DB7C193.1070703@web.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] target-i386: Initialize CPUState::halted in cpu_reset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel On 2011-04-27 19:17, Blue Swirl wrote: > On Wed, Apr 27, 2011 at 10:11 AM, Jan Kiszka wrote: >> On 2011-04-26 21:59, Blue Swirl wrote: >>> On Tue, Apr 26, 2011 at 9:55 PM, Jan Kiszka wrote= : >>>> On 2011-04-26 20:00, Blue Swirl wrote: >>>>> On Tue, Apr 26, 2011 at 11:50 AM, Jan Kiszka wrote: >>>>>> Instead of having an extra reset function at machine level and spe= cial >>>>>> code for processing INIT, move the initialization of halted into t= he >>>>>> 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 basicall= y >>>> reporting the result of the MP init protocol in condensed from. >>> >>> Intel 64 and IA-32 Architectures Software Developer=E2=80=99s 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. >=20 > 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_*. >=20 >> 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 mov= e >> it to the CPU's vmstate. OTOH, changing vmstates only for the sake of >> minor refactorings is also not very attractive. >=20 > 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. >=20 >> >> 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; >=20 > This does not look OK. Required for APIC-less mode (otherwise there would be no BSP). Jan --=20 Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux