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

login
register
mail settings
Submitter Jan Kiszka
Date April 27, 2011, 7:11 a.m.
Message ID <4DB7C193.1070703@web.de>
Download mbox | patch
Permalink /patch/92989/
State New
Headers show

Comments

Jan Kiszka - April 27, 2011, 7:11 a.m.
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(-)
Blue Swirl - April 27, 2011, 5:17 p.m.
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.
Jan Kiszka - April 27, 2011, 5:32 p.m.
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
Blue Swirl - April 27, 2011, 6:29 p.m.
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.

Patch

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