| Message ID | 4DB7C193.1070703@web.de |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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; }