From patchwork Wed Apr 27 07:11:15 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jan Kiszka X-Patchwork-Id: 92989 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id CCD9DB6EEE for ; Wed, 27 Apr 2011 17:11:36 +1000 (EST) Received: from localhost ([::1]:40895 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QEyuL-0006iC-7o for incoming@patchwork.ozlabs.org; Wed, 27 Apr 2011 03:11:33 -0400 Received: from eggs.gnu.org ([140.186.70.92]:35875) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QEyu9-0006hv-Od for qemu-devel@nongnu.org; Wed, 27 Apr 2011 03:11:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QEyu5-0005Wa-P9 for qemu-devel@nongnu.org; Wed, 27 Apr 2011 03:11:21 -0400 Received: from fmmailgate01.web.de ([217.72.192.221]:36168) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QEyu5-0005WO-7p for qemu-devel@nongnu.org; Wed, 27 Apr 2011 03:11:17 -0400 Received: from smtp04.web.de ( [172.20.0.225]) by fmmailgate01.web.de (Postfix) with ESMTP id EA49718D3016E; Wed, 27 Apr 2011 09:11:15 +0200 (CEST) Received: from [88.64.17.9] (helo=mchn199C.mchp.siemens.de) by smtp04.web.de with asmtp (TLSv1:AES256-SHA:256) (WEB.DE 4.110 #2) id 1QEyu3-0007uV-00; Wed, 27 Apr 2011 09:11:15 +0200 Message-ID: <4DB7C193.1070703@web.de> Date: Wed, 27 Apr 2011 09:11:15 +0200 From: Jan Kiszka User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 MIME-Version: 1.0 To: Blue Swirl References: <4DB68768.3050700@siemens.com> <4DB7151A.4000401@web.de> In-Reply-To: X-Enigmail-Version: 1.1.2 X-Sender: jan.kiszka@web.de X-Provags-ID: V01U2FsdGVkX1+GMjMq25Y6I2HfHCpJEnWD6RQ3RGYU93bBT0J0 6S956BkjBhRScPMdWiuj3AlPSfisl9ByFr5tipPJqFYhJMJqJf VTFAnq7vg= X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4-2.6 X-Received-From: 217.72.192.221 Cc: qemu-devel Subject: Re: [Qemu-devel] [PATCH] target-i386: Initialize CPUState::halted in cpu_reset X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 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. 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. 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; } void cpu_set_apic_tpr(DeviceState *d, uint8_t val) @@ -958,18 +958,26 @@ static const VMStateDescription vmstate_apic = { } }; +void apic_designate_bsp(DeviceState *d) +{ + APICState *s = DO_UPCAST(APICState, busdev.qdev, d); + + if (!d) { + return; + } + s->apicbase |= MSR_IA32_APICBASE_BSP; +} + static void apic_reset(DeviceState *d) { APICState *s = DO_UPCAST(APICState, busdev.qdev, d); - int bsp; - bsp = cpu_is_bsp(s->cpu_env); s->apicbase = 0xfee00000 | - (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE; + (s->apicbase & MSR_IA32_APICBASE_BSP) | MSR_IA32_APICBASE_ENABLE; apic_init_reset(d); - if (bsp) { + if (s->apicbase & MSR_IA32_APICBASE_BSP) { /* * LINT0 delivery mode on CPU #0 is set to ExtInt at initialization * time typically by BIOS, so PIC interrupt can be delivered to the diff --git a/hw/apic.h b/hw/apic.h index 8a0c9d0..935144b 100644 --- a/hw/apic.h +++ b/hw/apic.h @@ -19,9 +19,9 @@ void cpu_set_apic_tpr(DeviceState *s, uint8_t val); uint8_t cpu_get_apic_tpr(DeviceState *s); void apic_init_reset(DeviceState *s); void apic_sipi(DeviceState *s); +void apic_designate_bsp(DeviceState *d); /* pc.c */ -int cpu_is_bsp(CPUState *env); DeviceState *cpu_get_current_apic(void); #endif diff --git a/hw/pc.c b/hw/pc.c index 6939c04..36ca238 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -852,12 +852,6 @@ void pc_init_ne2k_isa(NICInfo *nd) nb_ne2k++; } -int cpu_is_bsp(CPUState *env) -{ - /* We hard-wire the BSP to the first CPU. */ - return env->cpu_index == 0; -} - DeviceState *cpu_get_current_apic(void) { if (cpu_single_env) { @@ -918,7 +912,8 @@ static void pc_cpu_reset(void *opaque) CPUState *env = opaque; cpu_reset(env); - env->halted = !cpu_is_bsp(env); + env->halted = + !(cpu_get_apic_base(env->apic_state) & MSR_IA32_APICBASE_BSP); } static CPUState *pc_new_cpu(const char *cpu_model) @@ -933,6 +928,11 @@ static CPUState *pc_new_cpu(const char *cpu_model) if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { env->cpuid_apic_id = env->cpu_index; env->apic_state = apic_init(env, env->cpuid_apic_id); + + /* We hard-wire the BSP to the first CPU. */ + if (env->cpu_index == 0) { + apic_designate_bsp(env->apic_state); + } } qemu_register_reset(pc_cpu_reset, env); pc_cpu_reset(env); diff --git a/target-i386/helper.c b/target-i386/helper.c index 89df997..52b3a44 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -1282,7 +1282,8 @@ void do_cpu_init(CPUState *env) env->interrupt_request = sipi; env->pat = pat; apic_init_reset(env->apic_state); - env->halted = !cpu_is_bsp(env); + env->halted = + !(cpu_get_apic_base(env->apic_state) & MSR_IA32_APICBASE_BSP); } void do_cpu_sipi(CPUState *env) diff --git a/target-i386/kvm.c b/target-i386/kvm.c index a13599d..fef78c2 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -504,8 +504,9 @@ void kvm_arch_reset_vcpu(CPUState *env) env->interrupt_injected = -1; env->xcr0 = 1; if (kvm_irqchip_in_kernel()) { - env->mp_state = cpu_is_bsp(env) ? KVM_MP_STATE_RUNNABLE : - KVM_MP_STATE_UNINITIALIZED; + env->mp_state = + cpu_get_apic_base(env->apic_state) & MSR_IA32_APICBASE_BSP ? + KVM_MP_STATE_RUNNABLE : KVM_MP_STATE_UNINITIALIZED; } else { env->mp_state = KVM_MP_STATE_RUNNABLE; }