Re: [Qemu-devel] [PATCH] target-i386: Initialize CPUState::halted in cpu_reset

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 5865 bytes --]

On 2011-04-26 21:59, Blue Swirl wrote:
> On Tue, Apr 26, 2011 at 9:55 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-04-26 20:00, Blue Swirl wrote:
>>> On Tue, Apr 26, 2011 at 11:50 AM, Jan Kiszka <jan.kiszka@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.

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;
     }
-- 
1.7.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]