target-i386: Initialize CPUState::halted in cpu_reset
PATCH 1/1 target-i386: Initialize CPUState::halted in cpu_reset
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.
--- hw/pc.c | 12 ++---------- target-i386/helper.c | 5 ++++- 2 files changed, 6 insertions(+), 11 deletions(-)
hw/pc.c +2 -10
@@ -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; }
target-i386/helper.c +4 -1
@@ -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)
Quoted 3 lines · 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.
Nack. A CPU is designated as a BSP at board level. CPUs do not need to know about this at all.
Quoted 7 lines · Blue Swirl
Quoted 3 lines · 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.
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
Quoted 9 lines · Jan Kiszka
Quoted 7 lines · Blue Swirl
Quoted 3 lines · 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.
Nack. A CPU is designated as a BSP at board level. CPUs do not need to know about this at all.
Quoted 6 lines
That's why we have
cpu_is_bsp()inpc.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.
Quoted 17 lines · Blue Swirl
Quoted 9 lines · Jan Kiszka
Quoted 7 lines · Blue Swirl
Quoted 3 lines · 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.
Nack. A CPU is designated as a BSP at board level. CPUs do not need to know about this at all.
Quoted 6 lines
That's why we have
cpu_is_bsp()inpc.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.
Quoted 6 lines
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. Thencpu_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(-)
hw/apic.c +13 -5
@@ -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
hw/apic.h +1 -1
@@ -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
hw/pc.c +7 -7
@@ -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);
target-i386/helper.c +2 -1
@@ -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)
target-i386/kvm.c +3 -2
@@ -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 ; }
Signature · 1 line
1.7.1
Quoted 18 lines · Jan Kiszka
Quoted 17 lines · Blue Swirl
Quoted 15 lines · Jan Kiszka
Quoted 7 lines · Blue Swirl
Quoted 3 lines · 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.
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()inpc.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.
Quoted 8 lines
Quoted 6 lines
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. Thencpu_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.
Quoted 4 lines
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.
Quoted 16 lines
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(-)hw/apic.c +0
-0
hw/apic.c uint64_t cpu_get_apic_base(DeviceState *d) · line 321
Quoted 5 lines
hw/apic.c +1
-1@@ -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.
Quoted 26 lines · Blue Swirl
Quoted 25 lines · Jan Kiszka
Quoted 23 lines · Blue Swirl
Quoted 9 lines · Jan Kiszka
Quoted 7 lines · Blue Swirl
Quoted 3 lines · 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.
Nack. A CPU is designated as a BSP at board level. CPUs do not need to know about this at all.
Quoted 6 lines
That's why we have
cpu_is_bsp()inpc.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. Thencpu_is_bsp()would only check the MSR, which naturally belongs to the CPU/APIC domain.
Quoted 4 lines
Quoted 1 line
Something like this? The original patch has to be rebased on top.
How about not deleting
cpu_is_bsp()but moving it toapic.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_*.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 18 lines
Quoted 18 lines
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(-)hw/apic.c +0
-0@@ -318,7 +318,7 @@ uint64_t cpu_get_apic_base(DeviceState *d)
Quoted 6 lines
hw/apic.c uint64_t cpu_get_apic_base(DeviceState *d) · line 321
Quoted 4 lines
hw/apic.c +1
-1trace_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
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 29 lines · Jan Kiszka
Quoted 28 lines · Blue Swirl
Quoted 26 lines · Jan Kiszka
Quoted 17 lines · Blue Swirl
Quoted 9 lines · Jan Kiszka
Quoted 7 lines · Blue Swirl
Quoted 3 lines · 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.
Nack. A CPU is designated as a BSP at board level. CPUs do not need to know about this at all.
Quoted 6 lines
That's why we have
cpu_is_bsp()inpc.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.Quoted 6 lines
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. Thencpu_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.
Quoted 7 lines
Quoted 2 lines
How about not deleting
cpu_is_bsp()but moving it toapic.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.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
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(-)hw/apic.c +0
-0@@ -318,7 +318,7 @@ uint64_t cpu_get_apic_base(DeviceState *d)
Quoted 8 lines
Quoted 6 lines
hw/apic.c uint64_t cpu_get_apic_base(DeviceState *d) · line 321
Quoted 4 lines
hw/apic.c +1
-1trace_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.