Re: [Qemu-devel] [PATCH qom-next 07/12] target-i386: move cpu halted decision into x86_cpu_reset

2012-05-30 Thread Andreas Färber 
Am 30.05.2012 00:10, schrieb Igor Mammedov:
 From: Igor Mammedov niall...@gmail.com
 
 MP initialization protocol differs between cpu families, and for P6 and
 onward models it is up to CPU to decide if it will be BSP using this
 protocol, so try to model this. However there is no point in implementing
 MP initialization protocol in qemu. Thus first CPU is always marked as BSP.
 
 This patch:
  - moves decision to designate BSP from board into cpu, making cpu
 self-sufficient in this regard. Later it will allow to cleanup hw/pc.c
 and remove cpu_reset and wrappers from there.
  - stores flag that CPU is BSP in IA32_APIC_BASE to model behavior
 described in Inted SDM vol 3a part 1 chapter 8.4.1
  - uses MSR_IA32_APICBASE_BSP flag in apic_base for checking if cpu is BSP
 
 patch is based on Jan Kiszka's proposal:
 http://thread.gmane.org/gmane.comp.emulators.qemu/100806
 
 v2:
   - fix build for i386-linux-user
   spotted-by: Peter Maydell peter.mayd...@linaro.org
 
 Signed-off-by: Igor Mammedov imamm...@redhat.com
 ---
  hw/apic.h|2 +-
  hw/apic_common.c |   18 --
  hw/pc.c  |9 -
  target-i386/cpu.c|9 +
  target-i386/helper.c |1 -
  target-i386/kvm.c|5 +++--
  6 files changed, 25 insertions(+), 19 deletions(-)
 
 diff --git a/hw/apic.h b/hw/apic.h
 index 62179ce..d961ed4 100644
 --- a/hw/apic.h
 +++ b/hw/apic.h
 @@ -20,9 +20,9 @@ void apic_init_reset(DeviceState *s);
  void apic_sipi(DeviceState *s);
  void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
 TPRAccess access);
 +void apic_designate_bsp(DeviceState *d);
  
  /* pc.c */
 -int cpu_is_bsp(CPUX86State *env);
  DeviceState *cpu_get_current_apic(void);
  
  #endif
 diff --git a/hw/apic_common.c b/hw/apic_common.c
 index 46a9ff7..98ad6f0 100644
 --- a/hw/apic_common.c
 +++ b/hw/apic_common.c
 @@ -43,8 +43,8 @@ uint64_t cpu_get_apic_base(DeviceState *d)
  trace_cpu_get_apic_base((uint64_t)s-apicbase);
  return s-apicbase;
  } else {
 -trace_cpu_get_apic_base(0);
 -return 0;
 +trace_cpu_get_apic_base(MSR_IA32_APICBASE_BSP);
 +return MSR_IA32_APICBASE_BSP;
  }
  }
  
 @@ -201,22 +201,28 @@ void apic_init_reset(DeviceState *d)
  s-timer_expiry = -1;
  }
  
 +void apic_designate_bsp(DeviceState *d)
 +{
 +if (d) {

This check looks odd. The function call is already guarded with
cpu_index == 0. What other case would lead to d == NULL and can't we
check that at the call site?

 +APICCommonState *s = APIC_COMMON(d);

If this is a QOM cast macro, it will implicitly assert d != NULL, no?

Andreas

 +s-apicbase |= MSR_IA32_APICBASE_BSP;
 +}
 +}
 +
  static void apic_reset_common(DeviceState *d)
  {
  APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
  APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
 -bool bsp;
  
 -bsp = cpu_is_bsp(s-cpu-env);
  s-apicbase = 0xfee0 |
 -(bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
 +(s-apicbase  MSR_IA32_APICBASE_BSP) | MSR_IA32_APICBASE_ENABLE;
  
  s-vapic_paddr = 0;
  info-vapic_base_update(s);
  
  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/pc.c b/hw/pc.c
 index 6bb3d2a..2f681db 100644
 --- a/hw/pc.c
 +++ b/hw/pc.c
 @@ -870,12 +870,6 @@ void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
  nb_ne2k++;
  }
  
 -int cpu_is_bsp(CPUX86State *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) {
 @@ -942,10 +936,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
 level)
  static void pc_cpu_reset(void *opaque)
  {
  X86CPU *cpu = opaque;
 -CPUX86State *env = cpu-env;
 -
  cpu_reset(CPU(cpu));
 -env-halted = !cpu_is_bsp(env);
  }
  
  static X86CPU *pc_new_cpu(const char *cpu_model)
 diff --git a/target-i386/cpu.c b/target-i386/cpu.c
 index 41a0436..f029f2a 100644
 --- a/target-i386/cpu.c
 +++ b/target-i386/cpu.c
 @@ -1704,6 +1704,15 @@ static void x86_cpu_reset(CPUState *s)
  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)
 +/* We hard-wire the BSP to the first CPU. */
 +if (env-cpu_index == 0) {
 +apic_designate_bsp(env-apic_state);
 +}
 +
 +env-halted = !(cpu_get_apic_base(env-apic_state)  
 MSR_IA32_APICBASE_BSP);
 +#endif
  }
  
  static void mce_init(X86CPU *cpu)
 diff --git a/target-i386/helper.c b/target-i386/helper.c
 index a94be0a..fd20fd4 100644
 --- a/target-i386/helper.c
 +++ b/target-i386/helper.c
 @@ -1179,7

Re: [Qemu-devel] [PATCH qom-next 07/12] target-i386: move cpu halted decision into x86_cpu_reset

2012-05-30 Thread Andreas Färber 
Am 30.05.2012 16:13, schrieb Igor Mammedov:
 On 05/30/2012 02:11 PM, Andreas Färber wrote:
 Am 30.05.2012 00:10, schrieb Igor Mammedov:
 From: Igor Mammedovniall...@gmail.com

 MP initialization protocol differs between cpu families, and for P6 and
 onward models it is up to CPU to decide if it will be BSP using this
 protocol, so try to model this. However there is no point in
 implementing
 MP initialization protocol in qemu. Thus first CPU is always marked
 as BSP.

 This patch:
   - moves decision to designate BSP from board into cpu, making cpu
 self-sufficient in this regard. Later it will allow to cleanup hw/pc.c
 and remove cpu_reset and wrappers from there.
   - stores flag that CPU is BSP in IA32_APIC_BASE to model behavior
 described in Inted SDM vol 3a part 1 chapter 8.4.1
   - uses MSR_IA32_APICBASE_BSP flag in apic_base for checking if cpu
 is BSP

 patch is based on Jan Kiszka's proposal:
  http://thread.gmane.org/gmane.comp.emulators.qemu/100806

 v2:
- fix build for i386-linux-user
spotted-by: Peter Maydellpeter.mayd...@linaro.org

 Signed-off-by: Igor Mammedovimamm...@redhat.com
 ---
   hw/apic.h|2 +-
   hw/apic_common.c |   18 --
   hw/pc.c  |9 -
   target-i386/cpu.c|9 +
   target-i386/helper.c |1 -
   target-i386/kvm.c|5 +++--
   6 files changed, 25 insertions(+), 19 deletions(-)

 diff --git a/hw/apic.h b/hw/apic.h
 index 62179ce..d961ed4 100644
 --- a/hw/apic.h
 +++ b/hw/apic.h
 @@ -20,9 +20,9 @@ void apic_init_reset(DeviceState *s);
   void apic_sipi(DeviceState *s);
   void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
  TPRAccess access);
 +void apic_designate_bsp(DeviceState *d);

   /* pc.c */
 -int cpu_is_bsp(CPUX86State *env);
   DeviceState *cpu_get_current_apic(void);

   #endif
 diff --git a/hw/apic_common.c b/hw/apic_common.c
 index 46a9ff7..98ad6f0 100644
 --- a/hw/apic_common.c
 +++ b/hw/apic_common.c
 @@ -43,8 +43,8 @@ uint64_t cpu_get_apic_base(DeviceState *d)
   trace_cpu_get_apic_base((uint64_t)s-apicbase);
   return s-apicbase;
   } else {
 -trace_cpu_get_apic_base(0);
 -return 0;
 +trace_cpu_get_apic_base(MSR_IA32_APICBASE_BSP);
 +return MSR_IA32_APICBASE_BSP;
   }
   }

 @@ -201,22 +201,28 @@ void apic_init_reset(DeviceState *d)
   s-timer_expiry = -1;
   }

 +void apic_designate_bsp(DeviceState *d)
 +{
 +if (d) {

 This check looks odd. The function call is already guarded with
 cpu_index == 0. What other case would lead to d == NULL and can't we
 check that at the call site?
 
 cpu_index == 0 doesn't imply that cpu has apic hence the check. And we
 for sure
 can check it call site, would you like to do it there?
 PS:
  there are many checks for this condition in APIC code, so may be it should
  be there style-wise at least?

What I referred to as odd was the indentation of the actual implementation.

So I'd be fine with either checking at the call site or

if (d == NULL) {
return;
}

...

 +APICCommonState *s = APIC_COMMON(d);

 If this is a QOM cast macro, it will implicitly assert d != NULL, no?
 
 it actually will lead to null pointer dereference, like this:
   OBJECT_CHECK(type, obj, name) \
 ((type *)object_dynamic_cast_assert(OBJECT(obj), (name)))
   =  object_dynamic_cast(obj, typename)
 =  object_is_type(obj, target_type)
   =  type_is_ancestor(obj-class-type, target_type);
   ^^^

Mind to send a patch fixing that? :-)
I'd suppose the _is_... functions should probably return false and the
cast should assert. Anthony?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH qom-next 07/12] target-i386: move cpu halted decision into x86_cpu_reset

2012-05-30 Thread Igor Mammedov 

On 05/30/2012 04:54 PM, Andreas Färber wrote:

Am 30.05.2012 16:13, schrieb Igor Mammedov:

On 05/30/2012 02:11 PM, Andreas Färber wrote:

Am 30.05.2012 00:10, schrieb Igor Mammedov:

From: Igor Mammedovniall...@gmail.com

MP initialization protocol differs between cpu families, and for P6 and
onward models it is up to CPU to decide if it will be BSP using this
protocol, so try to model this. However there is no point in
implementing
MP initialization protocol in qemu. Thus first CPU is always marked
as BSP.

This patch:
   - moves decision to designate BSP from board into cpu, making cpu
self-sufficient in this regard. Later it will allow to cleanup hw/pc.c
and remove cpu_reset and wrappers from there.
   - stores flag that CPU is BSP in IA32_APIC_BASE to model behavior
described in Inted SDM vol 3a part 1 chapter 8.4.1
   - uses MSR_IA32_APICBASE_BSP flag in apic_base for checking if cpu
is BSP

patch is based on Jan Kiszka's proposal:
  http://thread.gmane.org/gmane.comp.emulators.qemu/100806

v2:
- fix build for i386-linux-user
spotted-by: Peter Maydellpeter.mayd...@linaro.org

Signed-off-by: Igor Mammedovimamm...@redhat.com
---
   hw/apic.h|2 +-
   hw/apic_common.c |   18 --
   hw/pc.c  |9 -
   target-i386/cpu.c|9 +
   target-i386/helper.c |1 -
   target-i386/kvm.c|5 +++--
   6 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/hw/apic.h b/hw/apic.h
index 62179ce..d961ed4 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -20,9 +20,9 @@ void apic_init_reset(DeviceState *s);
   void apic_sipi(DeviceState *s);
   void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
  TPRAccess access);
+void apic_designate_bsp(DeviceState *d);

   /* pc.c */
-int cpu_is_bsp(CPUX86State *env);
   DeviceState *cpu_get_current_apic(void);

   #endif
diff --git a/hw/apic_common.c b/hw/apic_common.c
index 46a9ff7..98ad6f0 100644
--- a/hw/apic_common.c
+++ b/hw/apic_common.c
@@ -43,8 +43,8 @@ uint64_t cpu_get_apic_base(DeviceState *d)
   trace_cpu_get_apic_base((uint64_t)s-apicbase);
   return s-apicbase;
   } else {
-trace_cpu_get_apic_base(0);
-return 0;
+trace_cpu_get_apic_base(MSR_IA32_APICBASE_BSP);
+return MSR_IA32_APICBASE_BSP;
   }
   }

@@ -201,22 +201,28 @@ void apic_init_reset(DeviceState *d)
   s-timer_expiry = -1;
   }

+void apic_designate_bsp(DeviceState *d)
+{
+if (d) {


This check looks odd. The function call is already guarded with
cpu_index == 0. What other case would lead to d == NULL and can't we
check that at the call site?


cpu_index == 0 doesn't imply that cpu has apic hence the check. And we
for sure
can check it call site, would you like to do it there?
PS:
  there are many checks for this condition in APIC code, so may be it should
  be there style-wise at least?


What I referred to as odd was the indentation of the actual implementation.

So I'd be fine with either checking at the call site or

 if (d == NULL) {
 return;
 }

 ...


+APICCommonState *s = APIC_COMMON(d);


If this is a QOM cast macro, it will implicitly assert d != NULL, no?


it actually will lead to null pointer dereference, like this:
   OBJECT_CHECK(type, obj, name) \
 ((type *)object_dynamic_cast_assert(OBJECT(obj), (name)))
   =   object_dynamic_cast(obj, typename)
 =   object_is_type(obj, target_type)
   =   type_is_ancestor(obj-class-type, target_type);
   ^^^


Mind to send a patch fixing that? :-)
I'd suppose the _is_... functions should probably return false and the
cast should assert. Anthony?


Maybe asserting right at the beginning of  object_dynamic_cast_assert would be 
better
than hiding it somewhere deep in call chain, because later someone could add 
access to obj
before *_is_* functions and with explicit assert in the beginnig of 
object_dynamic_cast_assert()
it would be hard to mess things up.



Andreas



--
-
 Igor

Re: [Qemu-devel] [PATCH qom-next 07/12] target-i386: move cpu halted decision into x86_cpu_reset

2012-05-30 Thread Igor Mammedov 

On 05/30/2012 02:11 PM, Andreas Färber wrote:

Am 30.05.2012 00:10, schrieb Igor Mammedov:

From: Igor Mammedovniall...@gmail.com

MP initialization protocol differs between cpu families, and for P6 and
onward models it is up to CPU to decide if it will be BSP using this
protocol, so try to model this. However there is no point in implementing
MP initialization protocol in qemu. Thus first CPU is always marked as BSP.

This patch:
  - moves decision to designate BSP from board into cpu, making cpu
self-sufficient in this regard. Later it will allow to cleanup hw/pc.c
and remove cpu_reset and wrappers from there.
  - stores flag that CPU is BSP in IA32_APIC_BASE to model behavior
described in Inted SDM vol 3a part 1 chapter 8.4.1
  - uses MSR_IA32_APICBASE_BSP flag in apic_base for checking if cpu is BSP

patch is based on Jan Kiszka's proposal:
 http://thread.gmane.org/gmane.comp.emulators.qemu/100806

v2:
   - fix build for i386-linux-user
   spotted-by: Peter Maydellpeter.mayd...@linaro.org

Signed-off-by: Igor Mammedovimamm...@redhat.com
---
  hw/apic.h|2 +-
  hw/apic_common.c |   18 --
  hw/pc.c  |9 -
  target-i386/cpu.c|9 +
  target-i386/helper.c |1 -
  target-i386/kvm.c|5 +++--
  6 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/hw/apic.h b/hw/apic.h
index 62179ce..d961ed4 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -20,9 +20,9 @@ void apic_init_reset(DeviceState *s);
  void apic_sipi(DeviceState *s);
  void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
 TPRAccess access);
+void apic_designate_bsp(DeviceState *d);

  /* pc.c */
-int cpu_is_bsp(CPUX86State *env);
  DeviceState *cpu_get_current_apic(void);

  #endif
diff --git a/hw/apic_common.c b/hw/apic_common.c
index 46a9ff7..98ad6f0 100644
--- a/hw/apic_common.c
+++ b/hw/apic_common.c
@@ -43,8 +43,8 @@ uint64_t cpu_get_apic_base(DeviceState *d)
  trace_cpu_get_apic_base((uint64_t)s-apicbase);
  return s-apicbase;
  } else {
-trace_cpu_get_apic_base(0);
-return 0;
+trace_cpu_get_apic_base(MSR_IA32_APICBASE_BSP);
+return MSR_IA32_APICBASE_BSP;
  }
  }

@@ -201,22 +201,28 @@ void apic_init_reset(DeviceState *d)
  s-timer_expiry = -1;
  }

+void apic_designate_bsp(DeviceState *d)
+{
+if (d) {


This check looks odd. The function call is already guarded with
cpu_index == 0. What other case would lead to d == NULL and can't we
check that at the call site?


cpu_index == 0 doesn't imply that cpu has apic hence the check. And we for sure
can check it call site, would you like to do it there?
PS:
 there are many checks for this condition in APIC code, so may be it should
 be there style-wise at least?




+APICCommonState *s = APIC_COMMON(d);


If this is a QOM cast macro, it will implicitly assert d != NULL, no?


it actually will lead to null pointer dereference, like this:
  OBJECT_CHECK(type, obj, name) \
((type *)object_dynamic_cast_assert(OBJECT(obj), (name)))
  =  object_dynamic_cast(obj, typename)
=  object_is_type(obj, target_type)
  =  type_is_ancestor(obj-class-type, target_type);
  ^^^



Andreas


+s-apicbase |= MSR_IA32_APICBASE_BSP;
+}
+}
+
  static void apic_reset_common(DeviceState *d)
  {
  APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
  APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
-bool bsp;

-bsp = cpu_is_bsp(s-cpu-env);
  s-apicbase = 0xfee0 |
-(bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
+(s-apicbase  MSR_IA32_APICBASE_BSP) | MSR_IA32_APICBASE_ENABLE;

  s-vapic_paddr = 0;
  info-vapic_base_update(s);

  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/pc.c b/hw/pc.c
index 6bb3d2a..2f681db 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -870,12 +870,6 @@ void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
  nb_ne2k++;
  }

-int cpu_is_bsp(CPUX86State *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) {
@@ -942,10 +936,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
level)
  static void pc_cpu_reset(void *opaque)
  {
  X86CPU *cpu = opaque;
-CPUX86State *env =cpu-env;
-
  cpu_reset(CPU(cpu));
-env-halted = !cpu_is_bsp(env);
  }

  static X86CPU *pc_new_cpu(const char *cpu_model)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 41a0436..f029f2a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1704,6 +1704,15 @@ static void x86_cpu_reset(CPUState *s)
  env-dr[7] = DR7_FIXED_1;

[Qemu-devel] [PATCH qom-next 07/12] target-i386: move cpu halted decision into x86_cpu_reset

2012-05-29 Thread Igor Mammedov 
From: Igor Mammedov niall...@gmail.com

MP initialization protocol differs between cpu families, and for P6 and
onward models it is up to CPU to decide if it will be BSP using this
protocol, so try to model this. However there is no point in implementing
MP initialization protocol in qemu. Thus first CPU is always marked as BSP.

This patch:
 - moves decision to designate BSP from board into cpu, making cpu
self-sufficient in this regard. Later it will allow to cleanup hw/pc.c
and remove cpu_reset and wrappers from there.
 - stores flag that CPU is BSP in IA32_APIC_BASE to model behavior
described in Inted SDM vol 3a part 1 chapter 8.4.1
 - uses MSR_IA32_APICBASE_BSP flag in apic_base for checking if cpu is BSP

patch is based on Jan Kiszka's proposal:
http://thread.gmane.org/gmane.comp.emulators.qemu/100806

v2:
  - fix build for i386-linux-user
  spotted-by: Peter Maydell peter.mayd...@linaro.org

Signed-off-by: Igor Mammedov imamm...@redhat.com
---
 hw/apic.h|2 +-
 hw/apic_common.c |   18 --
 hw/pc.c  |9 -
 target-i386/cpu.c|9 +
 target-i386/helper.c |1 -
 target-i386/kvm.c|5 +++--
 6 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/hw/apic.h b/hw/apic.h
index 62179ce..d961ed4 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -20,9 +20,9 @@ void apic_init_reset(DeviceState *s);
 void apic_sipi(DeviceState *s);
 void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
TPRAccess access);
+void apic_designate_bsp(DeviceState *d);
 
 /* pc.c */
-int cpu_is_bsp(CPUX86State *env);
 DeviceState *cpu_get_current_apic(void);
 
 #endif
diff --git a/hw/apic_common.c b/hw/apic_common.c
index 46a9ff7..98ad6f0 100644
--- a/hw/apic_common.c
+++ b/hw/apic_common.c
@@ -43,8 +43,8 @@ uint64_t cpu_get_apic_base(DeviceState *d)
 trace_cpu_get_apic_base((uint64_t)s-apicbase);
 return s-apicbase;
 } else {
-trace_cpu_get_apic_base(0);
-return 0;
+trace_cpu_get_apic_base(MSR_IA32_APICBASE_BSP);
+return MSR_IA32_APICBASE_BSP;
 }
 }
 
@@ -201,22 +201,28 @@ void apic_init_reset(DeviceState *d)
 s-timer_expiry = -1;
 }
 
+void apic_designate_bsp(DeviceState *d)
+{
+if (d) {
+APICCommonState *s = APIC_COMMON(d);
+s-apicbase |= MSR_IA32_APICBASE_BSP;
+}
+}
+
 static void apic_reset_common(DeviceState *d)
 {
 APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
 APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
-bool bsp;
 
-bsp = cpu_is_bsp(s-cpu-env);
 s-apicbase = 0xfee0 |
-(bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
+(s-apicbase  MSR_IA32_APICBASE_BSP) | MSR_IA32_APICBASE_ENABLE;
 
 s-vapic_paddr = 0;
 info-vapic_base_update(s);
 
 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/pc.c b/hw/pc.c
index 6bb3d2a..2f681db 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -870,12 +870,6 @@ void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
 nb_ne2k++;
 }
 
-int cpu_is_bsp(CPUX86State *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) {
@@ -942,10 +936,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
level)
 static void pc_cpu_reset(void *opaque)
 {
 X86CPU *cpu = opaque;
-CPUX86State *env = cpu-env;
-
 cpu_reset(CPU(cpu));
-env-halted = !cpu_is_bsp(env);
 }
 
 static X86CPU *pc_new_cpu(const char *cpu_model)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 41a0436..f029f2a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1704,6 +1704,15 @@ static void x86_cpu_reset(CPUState *s)
 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)
+/* We hard-wire the BSP to the first CPU. */
+if (env-cpu_index == 0) {
+apic_designate_bsp(env-apic_state);
+}
+
+env-halted = !(cpu_get_apic_base(env-apic_state)  
MSR_IA32_APICBASE_BSP);
+#endif
 }
 
 static void mce_init(X86CPU *cpu)
diff --git a/target-i386/helper.c b/target-i386/helper.c
index a94be0a..fd20fd4 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1179,7 +1179,6 @@ void do_cpu_init(X86CPU *cpu)
 env-interrupt_request = sipi;
 env-pat = pat;
 apic_init_reset(env-apic_state);
-env-halted = !cpu_is_bsp(env);
 }
 
 void do_cpu_sipi(X86CPU *cpu)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0d0d8f6..09621e5 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -583,8 +583,9 @@ void kvm_arch_reset_vcpu(CPUX86State *env)
 env-interrupt_injected = -1;