From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v2] KVM: x86: Make register state after reset conform to specification Date: Tue, 4 Dec 2012 11:48:27 +0200 Message-ID: <20121204094827.GD19514@redhat.com> References: <20121202133802.GA8731@redhat.com> <1354542415-13706-1-git-send-email-jsteckli@os.inf.tu-dresden.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org To: Julian Stecklina Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39187 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752018Ab2LDJsc (ORCPT ); Tue, 4 Dec 2012 04:48:32 -0500 Content-Disposition: inline In-Reply-To: <1354542415-13706-1-git-send-email-jsteckli@os.inf.tu-dresden.de> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Dec 03, 2012 at 02:46:55PM +0100, Julian Stecklina wrote: > Floating point initialization is moved to kvm_arch_vcpu_init. Added general purpose Actually Intel SDM Table 9-1 footnote 5 says: "The state of the x87 FPU and MMX registers is not changed by the execution of an INIT.". So lets drop this part of the patch for now since vcpu_reset() does not distinguish between reset and INIT currently. > register clearing to the same function. SVM code now properly initializes > EDX. > > Signed-off-by: Julian Stecklina > --- > arch/x86/kvm/cpuid.c | 1 + > arch/x86/kvm/svm.c | 7 +++++-- > arch/x86/kvm/vmx.c | 7 +------ > arch/x86/kvm/x86.c | 12 +++++++++++- > 4 files changed, 18 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 0595f13..aa468c2 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -659,6 +659,7 @@ void kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx, u32 *ecx, u32 *edx) > } else > *eax = *ebx = *ecx = *edx = 0; > } > +EXPORT_SYMBOL_GPL(kvm_cpuid); > > void kvm_emulate_cpuid(struct kvm_vcpu *vcpu) > { > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index baead95..642213a 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -20,6 +20,7 @@ > #include "mmu.h" > #include "kvm_cache_regs.h" > #include "x86.h" > +#include "cpuid.h" > > #include > #include > @@ -1190,6 +1191,7 @@ static void init_vmcb(struct vcpu_svm *svm) > static int svm_vcpu_reset(struct kvm_vcpu *vcpu) > { > struct vcpu_svm *svm = to_svm(vcpu); > + u32 dummy, eax; > > init_vmcb(svm); > > @@ -1198,8 +1200,9 @@ static int svm_vcpu_reset(struct kvm_vcpu *vcpu) > svm->vmcb->save.cs.base = svm->vcpu.arch.sipi_vector << 12; > svm->vmcb->save.cs.selector = svm->vcpu.arch.sipi_vector << 8; > } > - vcpu->arch.regs_avail = ~0; > - vcpu->arch.regs_dirty = ~0; > + > + kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy); You are passing uninitialized eax here. It is used by kvm_cpuid(). Should set it to 1. > + kvm_register_write(vcpu, VCPU_REGS_RDX, eax); > > return 0; > } > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index ff66a3b..363fb03 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -3942,7 +3942,7 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu) > u64 msr; > int ret; > > - vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 << VCPU_REGS_RSP)); > + vcpu->arch.regs_avail = ~(1 << VCPU_REGS_RIP); VCPU_REGS_RIP is part of registers array too, why not dropping the line? > > vmx->rmode.vm86_active = 0; > > @@ -3955,10 +3955,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu) > msr |= MSR_IA32_APICBASE_BSP; > kvm_set_apic_base(&vmx->vcpu, msr); > > - ret = fx_init(&vmx->vcpu); > - if (ret != 0) > - goto out; > - > vmx_segment_cache_clear(vmx); > > seg_setup(VCPU_SREG_CS); > @@ -3999,7 +3995,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu) > kvm_rip_write(vcpu, 0xfff0); > else > kvm_rip_write(vcpu, 0); > - kvm_register_write(vcpu, VCPU_REGS_RSP, 0); > > vmcs_writel(GUEST_DR7, 0x400); > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 2966c84..9640ea5 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6045,6 +6045,8 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu) > > int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu) > { > + int ret; > + > atomic_set(&vcpu->arch.nmi_queued, 0); > vcpu->arch.nmi_pending = 0; > vcpu->arch.nmi_injected = false; > @@ -6066,7 +6068,15 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu) > > kvm_pmu_reset(vcpu); > > - return kvm_x86_ops->vcpu_reset(vcpu); > + memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs)); > + vcpu->arch.regs_avail = ~0; > + vcpu->arch.regs_dirty = ~0; > + > + if ((ret = fx_init(vcpu)) != 0) goto out; > + > + ret = kvm_x86_ops->vcpu_reset(vcpu); > +out: > + return ret; > } > > int kvm_arch_hardware_enable(void *garbage) > -- > 1.7.11.7 -- Gleb.