Patchwork target-i386: Initialize CPUState::halted in cpu_reset

login
register
mail settings
Submitter Jan Kiszka
Date April 26, 2011, 8:50 a.m.
Message ID <4DB68768.3050700@siemens.com>
Download mbox | patch
Permalink /patch/92893/
State New
Headers show

Comments

Jan Kiszka - April 26, 2011, 8:50 a.m.
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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pc.c              |   12 ++----------
 target-i386/helper.c |    5 ++++-
 2 files changed, 6 insertions(+), 11 deletions(-)
Blue Swirl - April 26, 2011, 6 p.m.
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.
Jan Kiszka - April 26, 2011, 6:55 p.m.
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.

Jan
Blue Swirl - April 26, 2011, 7:59 p.m.
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.

Patch

diff --git a/hw/pc.c b/hw/pc.c
index 6939c04..8ef86db 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -913,14 +913,6 @@  void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static void pc_cpu_reset(void *opaque)
-{
-    CPUState *env = opaque;
-
-    cpu_reset(env);
-    env->halted = !cpu_is_bsp(env);
-}
-
 static CPUState *pc_new_cpu(const char *cpu_model)
 {
     CPUState *env;
@@ -934,8 +926,8 @@  static CPUState *pc_new_cpu(const char *cpu_model)
         env->cpuid_apic_id = env->cpu_index;
         env->apic_state = apic_init(env, env->cpuid_apic_id);
     }
-    qemu_register_reset(pc_cpu_reset, env);
-    pc_cpu_reset(env);
+    qemu_register_reset((QEMUResetHandler *)cpu_reset, env);
+    cpu_reset(env);
     return env;
 }
 
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 89df997..56cca96 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -106,6 +106,10 @@  void cpu_reset(CPUX86State *env)
     env->dr[7] = DR7_FIXED_1;
     cpu_breakpoint_remove_all(env, BP_CPU);
     cpu_watchpoint_remove_all(env, BP_CPU);
+
+#if !defined(CONFIG_USER_ONLY)
+    env->halted = !cpu_is_bsp(env);
+#endif
 }
 
 void cpu_x86_close(CPUX86State *env)
@@ -1282,7 +1286,6 @@  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);
 }
 
 void do_cpu_sipi(CPUState *env)