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

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

[Jan Kiszka]

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(-)

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)

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

[Blue Swirl]

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.

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

[Jan Kiszka]

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

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


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

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

[Blue Swirl]

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.

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

[Jan Kiszka]

[-- 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 --]

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

[Blue Swirl]

On Wed, Apr 27, 2011 at 10:11 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> 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.

How about not deleting cpu_is_bsp() but moving it to apic.c, as a
check for the BSP flag? That would simplify the patches a bit.

> 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.

CPU should be the correct place. You could wait until the vmstate is
changed anyway, or be the first.

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

This does not look OK.

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

[Jan Kiszka]

On 2011-04-27 19:17, Blue Swirl wrote:
> On Wed, Apr 27, 2011 at 10:11 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> 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.
> 
> How about not deleting cpu_is_bsp() but moving it to apic.c, as a
> check for the BSP flag? That would simplify the patches a bit.

Maybe as an inline helper.

But all this apic cpu_* helpers are not really beautiful. Logically,
they should take a CPUState, not an APIC. Or they should be called apic_*.

> 
>> 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.
> 
> CPU should be the correct place. You could wait until the vmstate is
> changed anyway, or be the first.

Changing is not a big issue. But we will only add code this way,
unfortunately not just move it around: we will still have to load and
sync the apicbase for older versions.

> 
>>
>> 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;
> 
> This does not look OK.

Required for APIC-less mode (otherwise there would be no BSP).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Blue Swirl]

On Wed, Apr 27, 2011 at 8:32 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-04-27 19:17, Blue Swirl wrote:
>> On Wed, Apr 27, 2011 at 10:11 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> 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.
>>
>> How about not deleting cpu_is_bsp() but moving it to apic.c, as a
>> check for the BSP flag? That would simplify the patches a bit.
>
> Maybe as an inline helper.
>
> But all this apic cpu_* helpers are not really beautiful. Logically,
> they should take a CPUState, not an APIC. Or they should be called apic_*.

Well, cpu_{s,g}et_apic_base() are in the wrong place.

TPR is shared between CR8 and APIC, currently it is handled by APIC so
the functions could be renamed.

>>
>>> 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.
>>
>> CPU should be the correct place. You could wait until the vmstate is
>> changed anyway, or be the first.
>
> Changing is not a big issue. But we will only add code this way,
> unfortunately not just move it around: we will still have to load and
> sync the apicbase for older versions.

Perhaps we could start deprecating old versions. For example, v0.16
could deprecate v0.14 or older and v0.17 drop support for them and
deprecate v0.15.

>
>>
>>>
>>> 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;
>>
>> This does not look OK.
>
> Required for APIC-less mode (otherwise there would be no BSP).

Perhaps the check should be moved to CPU level then.