On Tue, Feb 02, 2010 at 01:31:50PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Tue, Feb 02, 2010 at 09:19:01AM +0100, Jan Kiszka wrote:
> >> Push mpstate reading/writing into kvm_arch_load/save_regs and, on x86,
> >> properly synchronize with halted in the accessor functions.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> >> ---
> >> hw/apic.c | 7 ----
> >> qemu-kvm-ia64.c | 4 ++-
> >> qemu-kvm-x86.c | 88 +++++++++++++++++++++++++++---------------------
> >> qemu-kvm.c | 30 -----------------
> >> qemu-kvm.h | 15 --------
> >> target-i386/machine.c | 6 ---
> >> target-ia64/machine.c | 3 ++
> >> 7 files changed, 55 insertions(+), 98 deletions(-)
> >>
> >> diff --git a/hw/apic.c b/hw/apic.c
> >> index 3e03e10..092c61e 100644
> >> --- a/hw/apic.c
> >> +++ b/hw/apic.c
> >> @@ -507,13 +507,6 @@ void apic_init_reset(CPUState *env)
> >> s->wait_for_sipi = 1;
> >>
> >> env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
> >> -#ifdef KVM_CAP_MP_STATE
> >> - if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> >> - env->mp_state
> >> - = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
> >> - kvm_load_mpstate(env);
> >> - }
> >> -#endif
> >> }
> >>
> >> static void apic_startup(APICState *s, int vector_num)
> >> diff --git a/qemu-kvm-ia64.c b/qemu-kvm-ia64.c
> >> index fc8110e..39bcbeb 100644
> >> --- a/qemu-kvm-ia64.c
> >> +++ b/qemu-kvm-ia64.c
> >> @@ -124,7 +124,9 @@ void kvm_arch_cpu_reset(CPUState *env)
> >> {
> >> if (kvm_irqchip_in_kernel(kvm_context)) {
> >> #ifdef KVM_CAP_MP_STATE
> >> - kvm_reset_mpstate(env->kvm_cpu_state.vcpu_ctx);
> >> + struct kvm_mp_state mp_state = {.mp_state = KVM_MP_STATE_UNINITIALIZED
> >> + };
> >> + kvm_set_mpstate(env, &mp_state);
> >> #endif
> >> } else {
> >> env->interrupt_request &= ~CPU_INTERRUPT_HARD;
> >> diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
> >> index 63cd095..6b5895f 100644
> >> --- a/qemu-kvm-x86.c
> >> +++ b/qemu-kvm-x86.c
> >> @@ -754,6 +754,48 @@ static int get_msr_entry(struct kvm_msr_entry *entry, CPUState *env)
> >> return 0;
> >> }
> >>
> >> +static void kvm_arch_save_mpstate(CPUState *env)
> >> +{
> >> +#ifdef KVM_CAP_MP_STATE
> >> + int r;
> >> + struct kvm_mp_state mp_state;
> >> +
> >> + r = kvm_get_mpstate(env, &mp_state);
> >> + if (r < 0) {
> >> + env->mp_state = -1;
> >> + } else {
> >> + env->mp_state = mp_state.mp_state;
> >> + if (kvm_irqchip_in_kernel()) {
> >> + env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
> >> + }
> >> + }
> >> +#else
> >> + env->mp_state = -1;
> >> +#endif
> >> +}
> >> +
> >> +static void kvm_arch_load_mpstate(CPUState *env)
> >> +{
> >> +#ifdef KVM_CAP_MP_STATE
> >> + struct kvm_mp_state mp_state;
> >> +
> >> + /*
> >> + * -1 indicates that the host did not support GET_MP_STATE ioctl,
> >> + * so don't touch it.
> >> + */
> >> + if (env->mp_state != -1) {
> >> + if (kvm_irqchip_in_kernel()) {
> >> + env->mp_state = env->halted ? KVM_MP_STATE_UNINITIALIZED :
> >> + KVM_MP_STATE_RUNNABLE;
> > When irqchip is in kernel env->halted doesn't contain any relevant
> > information, so this is incorrect. Actually env->halted is updated only
> > to show correct cpu state during "info cpus".
>
> OK, copied from apic_init_reset, see above. So that hunk was probably at
> least useless, and now it's harmfull. Will drop this and only sync from
> mp_state -> halted.
>
It was not useless in apic_init_reset it was a shortcut for:
env->mp_state = !(s->apicbase & MSR_IA32_APICBASE_BSP) ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
On reset BSP VCPU should set env->mp_state to KVM_MP_STATE_RUNNABLE and
all others to KVM_MP_STATE_UNINITIALIZED.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
