From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 15/21] qemu-kvm: Clean up mpstate synchronization Date: Tue, 02 Feb 2010 13:31:50 +0100 Message-ID: <4B681B36.3010300@siemens.com> References: <5104499c6d629296d385179f95163e7c16e1ec01.1265098707.git.jan.kiszka@siemens.com> <20100202122347.GC739@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Avi Kivity , Marcelo Tosatti , "kvm@vger.kernel.org" , Anthony Liguori , Alexander Graf , Glauber Costa , "qemu-devel@nongnu.org" To: Gleb Natapov Return-path: Received: from goliath.siemens.de ([192.35.17.28]:21498 "EHLO goliath.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751907Ab0BBMca (ORCPT ); Tue, 2 Feb 2010 07:32:30 -0500 In-Reply-To: <20100202122347.GC739@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 >> --- >> 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. Thanks, Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux