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:40:13 +0100 Message-ID: <4B681D2D.3040902@siemens.com> References: <5104499c6d629296d385179f95163e7c16e1ec01.1265098707.git.jan.kiszka@siemens.com> <20100202122347.GC739@redhat.com> <4B681B36.3010300@siemens.com> <20100202123710.GD739@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 david.siemens.de ([192.35.17.14]:22362 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752471Ab0BBMkx (ORCPT ); Tue, 2 Feb 2010 07:40:53 -0500 In-Reply-To: <20100202123710.GD739@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Gleb Natapov wrote: > 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 >>>> --- >>>> 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. OK, belongs to kvm vpcu init code then - less encrypted. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux