[Qemu-devel] [PATCH 0/2 v2] target-i386: refactor reset handling and move it into cpu.c

[Qemu-devel] [PATCH 0/2 v2] target-i386: refactor reset handling and move it into cpu.c

[Igor Mammedov]

v2:
  ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
  to keep cpu_init implementation in -softmmu and -user targets the same
  in single place and maintanable.

tree for testing:
  https://github.com/imammedo/qemu/tree/x86_reset

comiple & run tested with x86_64-linux-user, x86_64-softmmu targets

Igor Mammedov (2):
  target-i386: move cpu halted decision into x86_cpu_reset
  target-i386: move cpu_reset and reset callback to cpu.c

 hw/apic.h            |    2 +-
 hw/apic_common.c     |   20 ++++++++++++++------
 hw/pc.c              |   18 +-----------------
 target-i386/cpu.c    |   25 +++++++++++++++++++++++++
 target-i386/helper.c |    1 -
 target-i386/kvm.c    |    5 +++--
 6 files changed, 44 insertions(+), 27 deletions(-)

[Qemu-devel] [PATCH 1/2] target-i386: move cpu halted decision into x86_cpu_reset

[Igor Mammedov]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 5507 bytes --]

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.maydell@linaro.org>
v3:
  - style change requested by Andreas Färber <afaerber@suse.de>

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/apic.h            |    2 +-
 hw/apic_common.c     |   20 ++++++++++++++------
 hw/pc.c              |    9 ---------
 target-i386/cpu.c    |    9 +++++++++
 target-i386/helper.c |    1 -
 target-i386/kvm.c    |    5 +++--
 6 files changed, 27 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 60b8259..095b09e 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,30 @@ void apic_init_reset(DeviceState *d)
     s->timer_expiry = -1;
 }
 
+void apic_designate_bsp(DeviceState *d)
+{
+    if (d == NULL) {
+        return;
+    }
+
+    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 = 0xfee00000 |
-        (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 c7e9ab3..50c1715 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -871,12 +871,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) {
@@ -927,10 +921,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 5521709..f9ed6d8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1686,6 +1686,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 d3af6ea..b748d90 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1191,7 +1191,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;
     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

Re: [Qemu-devel] [PATCH 1/2] target-i386: move cpu halted decision into x86_cpu_reset

[Gleb Natapov]

On Tue, Jul 10, 2012 at 03:15:51PM +0200, Igor Mammedov wrote:
>  /* pc.c */
> -int cpu_is_bsp(CPUX86State *env);
Why remove it instead of modifying it to check BSP bit in apic base?
I think it will make the patch smaller and open code the check does not
look nice.


--
			Gleb.

Re: [Qemu-devel] [PATCH 1/2] target-i386: move cpu halted decision into x86_cpu_reset

[Igor Mammedov]

On 07/12/2012 08:38 AM, Gleb Natapov wrote:
> On Tue, Jul 10, 2012 at 03:15:51PM +0200, Igor Mammedov wrote:
>>   /* pc.c */
>> -int cpu_is_bsp(CPUX86State *env);
> Why remove it instead of modifying it to check BSP bit in apic base?
> I think it will make the patch smaller and open code the check does not
> look nice.

It's smaller than if I keep cpu_is_bsp() because keeping it would require
as minimum adapting Andreas' patch [1] and without cpu_is_bsp() there won't
be need in it as well.

plain check doesn't look horrible though. It's documented in patch
description and Intel's SDM also mentions APIC base and BSP bit in it. So
may be it's better to use it this way.

Anyway,
I've made a version that keeps cpu_is_bsp() with a bits from [1].
I'll send it as followup to this email, please see if that way is any better.


1) [PATCH qom-next 06/59] pc: Pass X86CPU to cpu_is_bsp()
      http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg03185.html
-- 
-----
  Igor

[Qemu-devel] [PATCH v2] 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.

[Igor Mammedov]

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.maydell@linaro.org>
v3:
  - style change requested by Andreas Färber <afaerber@suse.de>

v4:
  - reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
      requested by  Gleb Natapov <gleb@redhat.com>
  - hijacked Andreas' patch [1] to use X86CPU instead of CPUX86State in
    cpu_is_bsp()

  1)  http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg03185.html

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/apic.h            |    5 ++++-
 hw/apic_common.c     |   16 +++++++++++++---
 hw/pc.c              |    9 ---------
 target-i386/cpu.c    |   18 ++++++++++++++++++
 target-i386/helper.c |    1 -
 target-i386/kvm.c    |    4 +++-
 6 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/hw/apic.h b/hw/apic.h
index 62179ce..4da10b6 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -20,9 +20,12 @@ 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);
 
+/* cpu.c */
+bool cpu_is_bsp(X86CPU *cpu);
+
 #endif
diff --git a/hw/apic_common.c b/hw/apic_common.c
index 60b8259..58e63b0 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,13 +201,23 @@ void apic_init_reset(DeviceState *d)
     s->timer_expiry = -1;
 }
 
+void apic_designate_bsp(DeviceState *d)
+{
+    if (d == NULL) {
+        return;
+    }
+
+    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);
+    bsp = cpu_is_bsp(x86_env_get_cpu(s->cpu_env));
     s->apicbase = 0xfee00000 |
         (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
 
diff --git a/hw/pc.c b/hw/pc.c
index c7e9ab3..50c1715 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -871,12 +871,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) {
@@ -927,10 +921,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 5521709..0c38b7f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1686,6 +1686,24 @@ 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_is_bsp(cpu);
+#endif
+}
+
+#ifndef CONFIG_USER_ONLY
+bool cpu_is_bsp(X86CPU *cpu)
+{
+    return cpu_get_apic_base(cpu->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 d3af6ea..b748d90 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1191,7 +1191,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..97a2cb1 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -579,11 +579,13 @@ int kvm_arch_init_vcpu(CPUX86State *env)
 
 void kvm_arch_reset_vcpu(CPUX86State *env)
 {
+    X86CPU *cpu = x86_env_get_cpu(env);
+
     env->exception_injected = -1;
     env->interrupt_injected = -1;
     env->xcr0 = 1;
     if (kvm_irqchip_in_kernel()) {
-        env->mp_state = cpu_is_bsp(env) ? KVM_MP_STATE_RUNNABLE :
+        env->mp_state = cpu_is_bsp(cpu) ? KVM_MP_STATE_RUNNABLE :
                                           KVM_MP_STATE_UNINITIALIZED;
     } else {
         env->mp_state = KVM_MP_STATE_RUNNABLE;
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2] 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.

[Igor Mammedov]

Hello Gleb,

Is this v2 patch more acceptable then v1?

Re: [Qemu-devel] [PATCH v2] 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.

[Gleb Natapov]

On Mon, Jul 23, 2012 at 09:44:05AM +0200, Igor Mammedov wrote:
> Hello Gleb,
> 
> Is this v2 patch more acceptable then v1?
Yes. Sorry for not being explicit about it :)

--
			Gleb.

Re: [Qemu-devel] [PATCH v2] 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.

[Andreas Färber]

Am 12.07.2012 15:22, schrieb Igor Mammedov:
> 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.maydell@linaro.org>
> v3:
>   - style change requested by Andreas Färber <afaerber@suse.de>
> 
> v4:
>   - reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>       requested by  Gleb Natapov <gleb@redhat.com>
>   - hijacked Andreas' patch [1] to use X86CPU instead of CPUX86State in
>     cpu_is_bsp()
> 
>   1)  http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg03185.html
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/apic.h            |    5 ++++-
>  hw/apic_common.c     |   16 +++++++++++++---
>  hw/pc.c              |    9 ---------
>  target-i386/cpu.c    |   18 ++++++++++++++++++
>  target-i386/helper.c |    1 -
>  target-i386/kvm.c    |    4 +++-
>  6 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/apic.h b/hw/apic.h
> index 62179ce..4da10b6 100644
> --- a/hw/apic.h
> +++ b/hw/apic.h
> @@ -20,9 +20,12 @@ 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);
>  
> +/* cpu.c */
> +bool cpu_is_bsp(X86CPU *cpu);
> +
>  #endif
> diff --git a/hw/apic_common.c b/hw/apic_common.c
> index 60b8259..58e63b0 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,13 +201,23 @@ void apic_init_reset(DeviceState *d)
>      s->timer_expiry = -1;
>  }
>  
> +void apic_designate_bsp(DeviceState *d)
> +{
> +    if (d == NULL) {
> +        return;
> +    }
> +
> +    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);
> +    bsp = cpu_is_bsp(x86_env_get_cpu(s->cpu_env));
>      s->apicbase = 0xfee00000 |
>          (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
>  
> diff --git a/hw/pc.c b/hw/pc.c
> index c7e9ab3..50c1715 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -871,12 +871,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) {
> @@ -927,10 +921,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 5521709..0c38b7f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1686,6 +1686,24 @@ 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_is_bsp(cpu);
> +#endif
> +}
> +
> +#ifndef CONFIG_USER_ONLY
> +bool cpu_is_bsp(X86CPU *cpu)
> +{
> +    return cpu_get_apic_base(cpu->env.apic_state) & MSR_IA32_APICBASE_BSP;
> +}
> +#endif
> +
>  }

I'm okay with this approach too, but I think the above brace is a merge
conflict?

Did you git-grep for "cpu_is_bsp" to be sure you caught all usages?

Andreas

>  
>  static void mce_init(X86CPU *cpu)
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index d3af6ea..b748d90 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1191,7 +1191,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..97a2cb1 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -579,11 +579,13 @@ int kvm_arch_init_vcpu(CPUX86State *env)
>  
>  void kvm_arch_reset_vcpu(CPUX86State *env)
>  {
> +    X86CPU *cpu = x86_env_get_cpu(env);
> +
>      env->exception_injected = -1;
>      env->interrupt_injected = -1;
>      env->xcr0 = 1;
>      if (kvm_irqchip_in_kernel()) {
> -        env->mp_state = cpu_is_bsp(env) ? KVM_MP_STATE_RUNNABLE :
> +        env->mp_state = cpu_is_bsp(cpu) ? KVM_MP_STATE_RUNNABLE :
>                                            KVM_MP_STATE_UNINITIALIZED;
>      } else {
>          env->mp_state = KVM_MP_STATE_RUNNABLE;
> 

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

[Qemu-devel] [PATCH 2/2] target-i386: move cpu_reset and reset callback to cpu.c

[Igor Mammedov]

Moving reset callback into cpu object from board level and
resetting cpu at the end of x86_cpu_realize() will allow properly
create cpu object during run-time (hotplug) without calling reset exteraly.

When reset over QOM hierarchy is implemented, reset callback
should be removed.

v2:
  leave cpu_reset in pc_new_cpu() for now, it's to be cleaned up when APIC init is moved in cpu.c

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c           |    9 +--------
 target-i386/cpu.c |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 50c1715..d74ca6e 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -918,12 +918,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static void pc_cpu_reset(void *opaque)
-{
-    X86CPU *cpu = opaque;
-    cpu_reset(CPU(cpu));
-}
-
 static X86CPU *pc_new_cpu(const char *cpu_model)
 {
     X86CPU *cpu;
@@ -938,8 +932,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
     if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
         env->apic_state = apic_init(env, env->cpuid_apic_id);
     }
-    qemu_register_reset(pc_cpu_reset, cpu);
-    pc_cpu_reset(cpu);
+    cpu_reset(CPU(cpu));
     return cpu;
 }
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f9ed6d8..65c7446 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -31,6 +31,8 @@
 
 #include "hyperv.h"
 
+#include "hw/hw.h"
+
 /* feature flags taken from "Intel Processor Identification and the CPUID
  * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
  * between feature naming conventions, aliases may be added.
@@ -1697,6 +1699,15 @@ static void x86_cpu_reset(CPUState *s)
 #endif
 }
 
+#ifndef CONFIG_USER_ONLY
+/* TODO: remove me, when reset over QOM tree is implemented */
+static void x86_cpu_machine_reset_cb(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    cpu_reset(CPU(cpu));
+}
+#endif
+
 static void mce_init(X86CPU *cpu)
 {
     CPUX86State *cenv = &cpu->env;
@@ -1717,8 +1728,13 @@ void x86_cpu_realize(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
 
+#ifndef CONFIG_USER_ONLY
+    qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
+#endif
+
     mce_init(cpu);
     qemu_init_vcpu(&cpu->env);
+    cpu_reset(CPU(cpu));
 }
 
 static void x86_cpu_initfn(Object *obj)
-- 
1.7.1

[Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Igor Mammedov]

v2:
  ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
  to keep cpu_init implementation in -softmmu and -user targets the same
  in single place and maintanable.

v3:
  reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set

tree for testing:
  https://github.com/imammedo/qemu/tree/x86_reset_v3

comiple & run tested with x86_64-linux-user, x86_64-softmmu targets

Igor Mammedov (2):
  target-i386: move cpu halted decision into x86_cpu_reset
  target-i386: move cpu_reset and reset callback to cpu.c

 hw/apic.h            |    5 ++++-
 hw/apic_common.c     |   16 +++++++++++++---
 hw/pc.c              |   18 +-----------------
 target-i386/cpu.c    |   30 ++++++++++++++++++++++++++++++
 target-i386/helper.c |    1 -
 target-i386/kvm.c    |    4 +++-
 6 files changed, 51 insertions(+), 23 deletions(-)

[Qemu-devel] [PATCH 1/2] target-i386: move cpu halted decision into x86_cpu_reset

[Igor Mammedov]

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>

---
Changelog:
 v2:
   - fix build for i386-linux-user
      spotted-by: Peter Maydell <peter.maydell@linaro.org>
 v3:
   - style change requested by Andreas Färber <afaerber@suse.de>

 v4:
   - reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
      requested by  Gleb Natapov <gleb@redhat.com>
   - hijacked patch [1] to use X86CPU instead of CPUX86State in cpu_is_bsp()

 v5:
   - move Changelog under ---
      requested by: Peter Maydell <peter.maydell@linaro.org>

 1) [PATCH qom-next 06/59] pc: Pass X86CPU to cpu_is_bsp()
   SoB: Andreas Färber <afaerber@suse.de>
   http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg03185.html

 hw/apic.h            |    5 ++++-
 hw/apic_common.c     |   16 +++++++++++++---
 hw/pc.c              |    9 ---------
 target-i386/cpu.c    |   16 ++++++++++++++++
 target-i386/helper.c |    1 -
 target-i386/kvm.c    |    4 +++-
 6 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/hw/apic.h b/hw/apic.h
index a89542b..1d48e02 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -21,9 +21,12 @@ void apic_sipi(DeviceState *s);
 void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
                                    TPRAccess access);
 void apic_poll_irq(DeviceState *d);
+void apic_designate_bsp(DeviceState *d);
 
 /* pc.c */
-int cpu_is_bsp(CPUX86State *env);
 DeviceState *cpu_get_current_apic(void);
 
+/* cpu.c */
+bool cpu_is_bsp(X86CPU *cpu);
+
 #endif
diff --git a/hw/apic_common.c b/hw/apic_common.c
index 60b8259..58e63b0 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,13 +201,23 @@ void apic_init_reset(DeviceState *d)
     s->timer_expiry = -1;
 }
 
+void apic_designate_bsp(DeviceState *d)
+{
+    if (d == NULL) {
+        return;
+    }
+
+    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);
+    bsp = cpu_is_bsp(x86_env_get_cpu(s->cpu_env));
     s->apicbase = 0xfee00000 |
         (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
 
diff --git a/hw/pc.c b/hw/pc.c
index 598267a..a920686 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -857,12 +857,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) {
@@ -913,10 +907,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 b3bcbac..cf612a8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1686,8 +1686,24 @@ 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_is_bsp(cpu);
+#endif
 }
 
+#ifndef CONFIG_USER_ONLY
+bool cpu_is_bsp(X86CPU *cpu)
+{
+    return cpu_get_apic_base(cpu->env.apic_state) & MSR_IA32_APICBASE_BSP;
+}
+#endif
+
 static void mce_init(X86CPU *cpu)
 {
     CPUX86State *cenv = &cpu->env;
diff --git a/target-i386/helper.c b/target-i386/helper.c
index d3af6ea..b748d90 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1191,7 +1191,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 e53c2f6..4cfb3fa 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -584,11 +584,13 @@ int kvm_arch_init_vcpu(CPUX86State *env)
 
 void kvm_arch_reset_vcpu(CPUX86State *env)
 {
+    X86CPU *cpu = x86_env_get_cpu(env);
+
     env->exception_injected = -1;
     env->interrupt_injected = -1;
     env->xcr0 = 1;
     if (kvm_irqchip_in_kernel()) {
-        env->mp_state = cpu_is_bsp(env) ? KVM_MP_STATE_RUNNABLE :
+        env->mp_state = cpu_is_bsp(cpu) ? KVM_MP_STATE_RUNNABLE :
                                           KVM_MP_STATE_UNINITIALIZED;
     } else {
         env->mp_state = KVM_MP_STATE_RUNNABLE;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] target-i386: move cpu halted decision into x86_cpu_reset

[Andreas Färber]

Am 23.07.2012 15:22, schrieb Igor Mammedov:
> 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
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> ---
> Changelog:
>  v2:
>    - fix build for i386-linux-user
>       spotted-by: Peter Maydell <peter.maydell@linaro.org>
>  v3:
>    - style change requested by Andreas Färber <afaerber@suse.de>
> 
>  v4:
>    - reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>       requested by  Gleb Natapov <gleb@redhat.com>
>    - hijacked patch [1] to use X86CPU instead of CPUX86State in cpu_is_bsp()
> 
>  v5:
>    - move Changelog under ---
>       requested by: Peter Maydell <peter.maydell@linaro.org>
> 
>  1) [PATCH qom-next 06/59] pc: Pass X86CPU to cpu_is_bsp()
>    SoB: Andreas Färber <afaerber@suse.de>
>    http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg03185.html

Reviewed-by: Andreas Färber <afaerber@suse.de>

I'm fine with this patch, including the squashing of my trivial patch
without SoB, but would appreciate someone who knows the APIC (Jan?) to
add an explicit Acked-by.

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 1/2] target-i386: move cpu halted decision into x86_cpu_reset

[Igor Mammedov]

On 08/01/2012 04:00 PM, Andreas Färber wrote:
> Am 23.07.2012 15:22, schrieb Igor Mammedov:
>> 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
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>
>> ---
>> Changelog:
>>   v2:
>>     - fix build for i386-linux-user
>>        spotted-by: Peter Maydell <peter.maydell@linaro.org>
>>   v3:
>>     - style change requested by Andreas Färber <afaerber@suse.de>
>>
>>   v4:
>>     - reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>>        requested by  Gleb Natapov <gleb@redhat.com>
>>     - hijacked patch [1] to use X86CPU instead of CPUX86State in cpu_is_bsp()
>>
>>   v5:
>>     - move Changelog under ---
>>        requested by: Peter Maydell <peter.maydell@linaro.org>
>>
>>   1) [PATCH qom-next 06/59] pc: Pass X86CPU to cpu_is_bsp()
>>     SoB: Andreas Färber <afaerber@suse.de>
>>     http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg03185.html
>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> I'm fine with this patch, including the squashing of my trivial patch
> without SoB, but would appreciate someone who knows the APIC (Jan?) to
> add an explicit Acked-by.
Andreas,

I apologise for not including explicitly your SoB here. Next time I'll 
ask if SoB should be included.

> Andreas
>