* KVM VMX: register state after reset violates spec
@ 2012-11-29 14:07 Julian Stecklina
2012-11-29 14:38 ` [PATCH] KVM VMX: Make register state after reset conform to specification Julian Stecklina
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Julian Stecklina @ 2012-11-29 14:07 UTC (permalink / raw)
To: kvm
Hello,
we have noticed that at least on 3.6.8 with VMX after a VCPU has been
reset via the INIT-SIPI-SIPI sequence its register state violates
Intel's specification.
Specifically for our case we see at the end of vmx_vcpu_reset the
following vcpu state:
regs_avail=ffefffff regs_dirty=00010010
EIP=00000000 EAX=000006e8 EBX=00000001 ECX=80000001 EDX=00000600
ESI=0000d238 EDI=00000000 EBP=00000000 ESP=00000000
although EAX, EBX, ECX, ESI, EDI, EBP, ESP should _all_ be zero. See
http://download.intel.com/products/processor/manual/253668.pdf section
9.1.1 (page 9-2).
Shouldn't vmx_vcpu_reset actively clear those registers? And from a
quick glance at the SVM code the problem might exist there, too.
A workaround is to use qemu-kvm with -kvm-no-irqchip.
Julian
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] KVM VMX: Make register state after reset conform to specification. 2012-11-29 14:07 KVM VMX: register state after reset violates spec Julian Stecklina @ 2012-11-29 14:38 ` Julian Stecklina 2012-11-29 14:55 ` Julian Stecklina 2012-12-02 13:38 ` KVM VMX: register state after reset violates spec Gleb Natapov 2 siblings, 0 replies; 15+ messages in thread From: Julian Stecklina @ 2012-11-29 14:38 UTC (permalink / raw) To: kvm; +Cc: Julian Stecklina Signed-off-by: Julian Stecklina <jsteckli@os.inf.tu-dresden.de> --- arch/x86/kvm/vmx.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index ff66a3b..ec5a3b3 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -3948,7 +3948,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu) vmx->soft_vnmi_blocked = 0; - vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); kvm_set_cr8(&vmx->vcpu, 0); msr = 0xfee00000 | MSR_IA32_APICBASE_ENABLE; if (kvm_vcpu_is_bsp(&vmx->vcpu)) @@ -3999,6 +3998,14 @@ 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_RAX, 0); + kvm_register_write(vcpu, VCPU_REGS_RBX, 0); + kvm_register_write(vcpu, VCPU_REGS_RCX, 0); + kvm_register_write(vcpu, VCPU_REGS_RDX, get_rdx_init_val()); + kvm_register_write(vcpu, VCPU_REGS_RSI, 0); + kvm_register_write(vcpu, VCPU_REGS_RDI, 0); + kvm_register_write(vcpu, VCPU_REGS_RBP, 0); kvm_register_write(vcpu, VCPU_REGS_RSP, 0); vmcs_writel(GUEST_DR7, 0x400); -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] KVM VMX: Make register state after reset conform to specification. 2012-11-29 14:07 KVM VMX: register state after reset violates spec Julian Stecklina 2012-11-29 14:38 ` [PATCH] KVM VMX: Make register state after reset conform to specification Julian Stecklina @ 2012-11-29 14:55 ` Julian Stecklina 2012-12-02 13:38 ` KVM VMX: register state after reset violates spec Gleb Natapov 2 siblings, 0 replies; 15+ messages in thread From: Julian Stecklina @ 2012-11-29 14:55 UTC (permalink / raw) To: kvm; +Cc: Julian Stecklina Signed-off-by: Julian Stecklina <jsteckli@os.inf.tu-dresden.de> --- arch/x86/kvm/vmx.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index ff66a3b..ec5a3b3 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -3948,7 +3948,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu) vmx->soft_vnmi_blocked = 0; - vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); kvm_set_cr8(&vmx->vcpu, 0); msr = 0xfee00000 | MSR_IA32_APICBASE_ENABLE; if (kvm_vcpu_is_bsp(&vmx->vcpu)) @@ -3999,6 +3998,14 @@ 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_RAX, 0); + kvm_register_write(vcpu, VCPU_REGS_RBX, 0); + kvm_register_write(vcpu, VCPU_REGS_RCX, 0); + kvm_register_write(vcpu, VCPU_REGS_RDX, get_rdx_init_val()); + kvm_register_write(vcpu, VCPU_REGS_RSI, 0); + kvm_register_write(vcpu, VCPU_REGS_RDI, 0); + kvm_register_write(vcpu, VCPU_REGS_RBP, 0); kvm_register_write(vcpu, VCPU_REGS_RSP, 0); vmcs_writel(GUEST_DR7, 0x400); -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: KVM VMX: register state after reset violates spec 2012-11-29 14:07 KVM VMX: register state after reset violates spec Julian Stecklina 2012-11-29 14:38 ` [PATCH] KVM VMX: Make register state after reset conform to specification Julian Stecklina 2012-11-29 14:55 ` Julian Stecklina @ 2012-12-02 13:38 ` Gleb Natapov 2012-12-03 12:05 ` Julian Stecklina ` (2 more replies) 2 siblings, 3 replies; 15+ messages in thread From: Gleb Natapov @ 2012-12-02 13:38 UTC (permalink / raw) To: Julian Stecklina; +Cc: kvm On Thu, Nov 29, 2012 at 03:07:38PM +0100, Julian Stecklina wrote: > Hello, > > we have noticed that at least on 3.6.8 with VMX after a VCPU has been > reset via the INIT-SIPI-SIPI sequence its register state violates > Intel's specification. > > Specifically for our case we see at the end of vmx_vcpu_reset the > following vcpu state: > > regs_avail=ffefffff regs_dirty=00010010 > EIP=00000000 EAX=000006e8 EBX=00000001 ECX=80000001 EDX=00000600 > ESI=0000d238 EDI=00000000 EBP=00000000 ESP=00000000 > > although EAX, EBX, ECX, ESI, EDI, EBP, ESP should _all_ be zero. See > http://download.intel.com/products/processor/manual/253668.pdf section > 9.1.1 (page 9-2). > > Shouldn't vmx_vcpu_reset actively clear those registers? And from a > quick glance at the SVM code the problem might exist there, too. > It should, so why not move the fix to kvm_vcpu_reset() so it will work for both. Also what about R8-R15? Intel SDM says nothing about them in the section you mention, but in Volume 1 section 3.4.1.1 is says: Registers only available in 64-bit mode (R8-R15 and XMM8-XMM15) are preserved across transitions from 64-bit mode into compatibility mode then back into 64-bit mode. However, values of R8-R15 and XMM8-XMM15 are undefined after transitions from 64-bit mode through compatibility mode to legacy or real mode and then back through compatibility mode to 64-bit mode. I take it that they are undefined on the first transition to 64-bit mode too. AMD spec says that they should be zeroed on reset, so lets do that. Also SVM does not set EDX to correct value on reset. It should be: Stepping ID (bits 3:0)—This field identifies the processor-revision level. Extended Model (bits 19:16) and Model (bits 7:4)—These fields combine to differentiate processor models within a instruction family. For example, two processors may share the same microarchitecture but differ in their feature set. Such processors are considered different models within the same instruction family. This is a split field, comprising an extended-model portion in bits 19:16 with a legacy portion in bits 7:4 Extended Family (bits 27:20) and Family (bits 11:8)—These fields combine to differentiate processors by their microarchitecture. > A workaround is to use qemu-kvm with -kvm-no-irqchip. > > Julian > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Gleb. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: KVM VMX: register state after reset violates spec 2012-12-02 13:38 ` KVM VMX: register state after reset violates spec Gleb Natapov @ 2012-12-03 12:05 ` Julian Stecklina 2012-12-03 13:46 ` [PATCH v2] KVM: x86: Make register state after reset conform to specification Julian Stecklina 2012-12-03 13:49 ` KVM VMX: register state after reset violates spec Julian Stecklina 2 siblings, 0 replies; 15+ messages in thread From: Julian Stecklina @ 2012-12-03 12:05 UTC (permalink / raw) To: Gleb Natapov; +Cc: kvm Thus spake Gleb Natapov <gleb@redhat.com>: > On Thu, Nov 29, 2012 at 03:07:38PM +0100, Julian Stecklina wrote: >> Hello, >> >> we have noticed that at least on 3.6.8 with VMX after a VCPU has been >> reset via the INIT-SIPI-SIPI sequence its register state violates >> Intel's specification. [...] >> Shouldn't vmx_vcpu_reset actively clear those registers? And from a >> quick glance at the SVM code the problem might exist there, too. >> > It should, so why not move the fix to kvm_vcpu_reset() so it will work > for both. Also what about R8-R15? Intel SDM says nothing about them in > the section you mention, but in Volume 1 section 3.4.1.1 is says: [...] > I take it that they are undefined on the first transition to 64-bit mode > too. AMD spec says that they should be zeroed on reset, so lets do that. > Also SVM does not set EDX to correct value on reset. I'll post a revised patch later today. Julian ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] KVM: x86: Make register state after reset conform to specification 2012-12-02 13:38 ` KVM VMX: register state after reset violates spec Gleb Natapov 2012-12-03 12:05 ` Julian Stecklina @ 2012-12-03 13:46 ` Julian Stecklina 2012-12-04 9:48 ` Gleb Natapov 2012-12-03 13:49 ` KVM VMX: register state after reset violates spec Julian Stecklina 2 siblings, 1 reply; 15+ messages in thread From: Julian Stecklina @ 2012-12-03 13:46 UTC (permalink / raw) To: kvm; +Cc: Gleb Natapov, Julian Stecklina Floating point initialization is moved to kvm_arch_vcpu_init. Added general purpose register clearing to the same function. SVM code now properly initializes EDX. Signed-off-by: Julian Stecklina <jsteckli@os.inf.tu-dresden.de> --- 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 <linux/module.h> #include <linux/mod_devicetable.h> @@ -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); + 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); 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] KVM: x86: Make register state after reset conform to specification 2012-12-03 13:46 ` [PATCH v2] KVM: x86: Make register state after reset conform to specification Julian Stecklina @ 2012-12-04 9:48 ` Gleb Natapov 2012-12-04 12:13 ` [PATCH v3] " Julian Stecklina 0 siblings, 1 reply; 15+ messages in thread From: Gleb Natapov @ 2012-12-04 9:48 UTC (permalink / raw) To: Julian Stecklina; +Cc: kvm 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 <jsteckli@os.inf.tu-dresden.de> > --- > 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 <linux/module.h> > #include <linux/mod_devicetable.h> > @@ -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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] KVM: x86: Make register state after reset conform to specification 2012-12-04 9:48 ` Gleb Natapov @ 2012-12-04 12:13 ` Julian Stecklina 2012-12-05 10:50 ` Gleb Natapov 0 siblings, 1 reply; 15+ messages in thread From: Julian Stecklina @ 2012-12-04 12:13 UTC (permalink / raw) To: kvm; +Cc: Gleb Natapov, Julian Stecklina VMX behaves now as SVM wrt to FPU initialization. Code has been moved to generic code path. General-purpose registers are now cleared on reset and INIT. SVM code properly initializes EDX. Signed-off-by: Julian Stecklina <jsteckli@os.inf.tu-dresden.de> --- arch/x86/kvm/cpuid.c | 1 + arch/x86/kvm/svm.c | 14 ++++++-------- arch/x86/kvm/vmx.c | 7 ------- arch/x86/kvm/x86.c | 8 ++++++++ 4 files changed, 15 insertions(+), 15 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..6c50121 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 <linux/module.h> #include <linux/mod_devicetable.h> @@ -1190,6 +1191,8 @@ 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; + u32 eax = 1; init_vmcb(svm); @@ -1198,8 +1201,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); + kvm_register_write(vcpu, VCPU_REGS_RDX, eax); return 0; } @@ -1257,10 +1261,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) init_vmcb(svm); kvm_write_tsc(&svm->vcpu, 0); - err = fx_init(&svm->vcpu); - if (err) - goto free_page4; - svm->vcpu.arch.apic_base = 0xfee00000 | MSR_IA32_APICBASE_ENABLE; if (kvm_vcpu_is_bsp(&svm->vcpu)) svm->vcpu.arch.apic_base |= MSR_IA32_APICBASE_BSP; @@ -1269,8 +1269,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) return &svm->vcpu; -free_page4: - __free_page(hsave_page); free_page3: __free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER); free_page2: diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index ff66a3b..85cecfe 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -3942,8 +3942,6 @@ 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)); - vmx->rmode.vm86_active = 0; vmx->soft_vnmi_blocked = 0; @@ -3955,10 +3953,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 +3993,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..2e031bf 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6066,6 +6066,10 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu) kvm_pmu_reset(vcpu); + memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs)); + vcpu->arch.regs_avail = ~0; + vcpu->arch.regs_dirty = ~0; + return kvm_x86_ops->vcpu_reset(vcpu); } @@ -6229,6 +6233,10 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) if (!zalloc_cpumask_var(&vcpu->arch.wbinvd_dirty_mask, GFP_KERNEL)) goto fail_free_mce_banks; + r = fx_init(vcpu); + if (r) + goto fail_free_mce_banks; + kvm_async_pf_hash_reset(vcpu); kvm_pmu_init(vcpu); -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3] KVM: x86: Make register state after reset conform to specification 2012-12-04 12:13 ` [PATCH v3] " Julian Stecklina @ 2012-12-05 10:50 ` Gleb Natapov 2012-12-05 12:00 ` [PATCH v4] " Julian Stecklina 0 siblings, 1 reply; 15+ messages in thread From: Gleb Natapov @ 2012-12-05 10:50 UTC (permalink / raw) To: Julian Stecklina; +Cc: kvm On Tue, Dec 04, 2012 at 01:13:30PM +0100, Julian Stecklina wrote: > VMX behaves now as SVM wrt to FPU initialization. Code has been moved to > generic code path. General-purpose registers are now cleared on reset and > INIT. SVM code properly initializes EDX. > Looks good overall, small bug bellow and regenerate the patch against git://git.kernel.org/pub/scm/virt/kvm/kvm.git queue branch please. > Signed-off-by: Julian Stecklina <jsteckli@os.inf.tu-dresden.de> > --- > arch/x86/kvm/cpuid.c | 1 + > arch/x86/kvm/svm.c | 14 ++++++-------- > arch/x86/kvm/vmx.c | 7 ------- > arch/x86/kvm/x86.c | 8 ++++++++ > 4 files changed, 15 insertions(+), 15 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..6c50121 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 <linux/module.h> > #include <linux/mod_devicetable.h> > @@ -1190,6 +1191,8 @@ 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; > + u32 eax = 1; > > init_vmcb(svm); > > @@ -1198,8 +1201,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); > + kvm_register_write(vcpu, VCPU_REGS_RDX, eax); > > return 0; > } > @@ -1257,10 +1261,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) > init_vmcb(svm); > kvm_write_tsc(&svm->vcpu, 0); > > - err = fx_init(&svm->vcpu); > - if (err) > - goto free_page4; > - > svm->vcpu.arch.apic_base = 0xfee00000 | MSR_IA32_APICBASE_ENABLE; > if (kvm_vcpu_is_bsp(&svm->vcpu)) > svm->vcpu.arch.apic_base |= MSR_IA32_APICBASE_BSP; > @@ -1269,8 +1269,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) > > return &svm->vcpu; > > -free_page4: > - __free_page(hsave_page); > free_page3: > __free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER); > free_page2: > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index ff66a3b..85cecfe 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -3942,8 +3942,6 @@ 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)); > - > vmx->rmode.vm86_active = 0; > > vmx->soft_vnmi_blocked = 0; > @@ -3955,10 +3953,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 +3993,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..2e031bf 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6066,6 +6066,10 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu) > > kvm_pmu_reset(vcpu); > > + memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs)); > + vcpu->arch.regs_avail = ~0; > + vcpu->arch.regs_dirty = ~0; > + > return kvm_x86_ops->vcpu_reset(vcpu); > } > > @@ -6229,6 +6233,10 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) > if (!zalloc_cpumask_var(&vcpu->arch.wbinvd_dirty_mask, GFP_KERNEL)) > goto fail_free_mce_banks; > > + r = fx_init(vcpu); > + if (r) > + goto fail_free_mce_banks; > + You need to free vcpu->arch.wbinvd_dirty_mask in case fx_init() fails. > kvm_async_pf_hash_reset(vcpu); > kvm_pmu_init(vcpu); > > -- > 1.7.11.7 -- Gleb. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] KVM: x86: Make register state after reset conform to specification 2012-12-05 10:50 ` Gleb Natapov @ 2012-12-05 12:00 ` Julian Stecklina 2012-12-05 13:27 ` Gleb Natapov 0 siblings, 1 reply; 15+ messages in thread From: Julian Stecklina @ 2012-12-05 12:00 UTC (permalink / raw) To: kvm; +Cc: Gleb Natapov, Julian Stecklina VMX behaves now as SVM wrt to FPU initialization. Code has been moved to generic code path. General-purpose registers are now cleared on reset and INIT. SVM code properly initializes EDX. Signed-off-by: Julian Stecklina <jsteckli@os.inf.tu-dresden.de> --- arch/x86/kvm/cpuid.c | 1 + arch/x86/kvm/svm.c | 14 ++++++-------- arch/x86/kvm/vmx.c | 7 ------- arch/x86/kvm/x86.c | 10 ++++++++++ 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 52f6166..a20ecb5 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -661,6 +661,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 dcb79527..d29d3cd 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 <linux/module.h> #include <linux/mod_devicetable.h> @@ -1193,6 +1194,8 @@ 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; + u32 eax = 1; init_vmcb(svm); @@ -1201,8 +1204,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); + kvm_register_write(vcpu, VCPU_REGS_RDX, eax); return 0; } @@ -1259,10 +1263,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) svm->asid_generation = 0; init_vmcb(svm); - err = fx_init(&svm->vcpu); - if (err) - goto free_page4; - svm->vcpu.arch.apic_base = 0xfee00000 | MSR_IA32_APICBASE_ENABLE; if (kvm_vcpu_is_bsp(&svm->vcpu)) svm->vcpu.arch.apic_base |= MSR_IA32_APICBASE_BSP; @@ -1271,8 +1271,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) return &svm->vcpu; -free_page4: - __free_page(hsave_page); free_page3: __free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER); free_page2: diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 2fd2046..4ea3cb3 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -3934,8 +3934,6 @@ 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)); - vmx->rmode.vm86_active = 0; vmx->soft_vnmi_blocked = 0; @@ -3947,10 +3945,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); @@ -3991,7 +3985,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_GDTR_BASE, 0); vmcs_write32(GUEST_GDTR_LIMIT, 0xffff); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 3bdaf29..57c76e8 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6461,6 +6461,10 @@ static int kvm_vcpu_reset(struct kvm_vcpu *vcpu) kvm_pmu_reset(vcpu); + memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs)); + vcpu->arch.regs_avail = ~0; + vcpu->arch.regs_dirty = ~0; + return kvm_x86_ops->vcpu_reset(vcpu); } @@ -6629,11 +6633,17 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) if (!zalloc_cpumask_var(&vcpu->arch.wbinvd_dirty_mask, GFP_KERNEL)) goto fail_free_mce_banks; + r = fx_init(vcpu); + if (r) + goto fail_free_wbinvd_dirty_mask; + vcpu->arch.ia32_tsc_adjust_msr = 0x0; kvm_async_pf_hash_reset(vcpu); kvm_pmu_init(vcpu); return 0; +fail_free_wbinvd_dirty_mask: + free_cpumask_var(vcpu->arch.wbinvd_dirty_mask); fail_free_mce_banks: kfree(vcpu->arch.mce_banks); fail_free_lapic: -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4] KVM: x86: Make register state after reset conform to specification 2012-12-05 12:00 ` [PATCH v4] " Julian Stecklina @ 2012-12-05 13:27 ` Gleb Natapov 2012-12-05 14:26 ` [PATCH v5] " Julian Stecklina 2012-12-05 14:27 ` [PATCH v4] " Julian Stecklina 0 siblings, 2 replies; 15+ messages in thread From: Gleb Natapov @ 2012-12-05 13:27 UTC (permalink / raw) To: Julian Stecklina; +Cc: kvm On Wed, Dec 05, 2012 at 01:00:59PM +0100, Julian Stecklina wrote: > VMX behaves now as SVM wrt to FPU initialization. Code has been moved to > generic code path. General-purpose registers are now cleared on reset and > INIT. SVM code properly initializes EDX. > > Signed-off-by: Julian Stecklina <jsteckli@os.inf.tu-dresden.de> > --- > arch/x86/kvm/cpuid.c | 1 + > arch/x86/kvm/svm.c | 14 ++++++-------- > arch/x86/kvm/vmx.c | 7 ------- > arch/x86/kvm/x86.c | 10 ++++++++++ > 4 files changed, 17 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 52f6166..a20ecb5 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -661,6 +661,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 dcb79527..d29d3cd 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 <linux/module.h> > #include <linux/mod_devicetable.h> > @@ -1193,6 +1194,8 @@ 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; > + u32 eax = 1; > > init_vmcb(svm); > > @@ -1201,8 +1204,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); > + kvm_register_write(vcpu, VCPU_REGS_RDX, eax); > > return 0; > } > @@ -1259,10 +1263,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) > svm->asid_generation = 0; > init_vmcb(svm); > > - err = fx_init(&svm->vcpu); > - if (err) > - goto free_page4; > - > svm->vcpu.arch.apic_base = 0xfee00000 | MSR_IA32_APICBASE_ENABLE; > if (kvm_vcpu_is_bsp(&svm->vcpu)) > svm->vcpu.arch.apic_base |= MSR_IA32_APICBASE_BSP; > @@ -1271,8 +1271,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) > > return &svm->vcpu; > > -free_page4: > - __free_page(hsave_page); > free_page3: > __free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER); > free_page2: > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 2fd2046..4ea3cb3 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -3934,8 +3934,6 @@ 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)); > - > vmx->rmode.vm86_active = 0; > > vmx->soft_vnmi_blocked = 0; > @@ -3947,10 +3945,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; > - Label "out" is now unused. Compiler complains. > vmx_segment_cache_clear(vmx); > > seg_setup(VCPU_SREG_CS); > @@ -3991,7 +3985,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_GDTR_BASE, 0); > vmcs_write32(GUEST_GDTR_LIMIT, 0xffff); > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3bdaf29..57c76e8 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6461,6 +6461,10 @@ static int kvm_vcpu_reset(struct kvm_vcpu *vcpu) > > kvm_pmu_reset(vcpu); > > + memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs)); > + vcpu->arch.regs_avail = ~0; > + vcpu->arch.regs_dirty = ~0; > + > return kvm_x86_ops->vcpu_reset(vcpu); > } > > @@ -6629,11 +6633,17 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) > if (!zalloc_cpumask_var(&vcpu->arch.wbinvd_dirty_mask, GFP_KERNEL)) > goto fail_free_mce_banks; > > + r = fx_init(vcpu); > + if (r) > + goto fail_free_wbinvd_dirty_mask; > + > vcpu->arch.ia32_tsc_adjust_msr = 0x0; > kvm_async_pf_hash_reset(vcpu); > kvm_pmu_init(vcpu); > > return 0; > +fail_free_wbinvd_dirty_mask: > + free_cpumask_var(vcpu->arch.wbinvd_dirty_mask); > fail_free_mce_banks: > kfree(vcpu->arch.mce_banks); > fail_free_lapic: > -- > 1.7.11.7 -- Gleb. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5] KVM: x86: Make register state after reset conform to specification 2012-12-05 13:27 ` Gleb Natapov @ 2012-12-05 14:26 ` Julian Stecklina 2012-12-05 16:02 ` Gleb Natapov 2012-12-05 14:27 ` [PATCH v4] " Julian Stecklina 1 sibling, 1 reply; 15+ messages in thread From: Julian Stecklina @ 2012-12-05 14:26 UTC (permalink / raw) To: kvm; +Cc: Gleb Natapov, Julian Stecklina VMX behaves now as SVM wrt to FPU initialization. Code has been moved to generic code path. General-purpose registers are now cleared on reset and INIT. SVM code properly initializes EDX. Signed-off-by: Julian Stecklina <jsteckli@os.inf.tu-dresden.de> --- arch/x86/kvm/cpuid.c | 1 + arch/x86/kvm/svm.c | 14 ++++++-------- arch/x86/kvm/vmx.c | 8 -------- arch/x86/kvm/x86.c | 10 ++++++++++ 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 52f6166..a20ecb5 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -661,6 +661,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 dcb79527..d29d3cd 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 <linux/module.h> #include <linux/mod_devicetable.h> @@ -1193,6 +1194,8 @@ 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; + u32 eax = 1; init_vmcb(svm); @@ -1201,8 +1204,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); + kvm_register_write(vcpu, VCPU_REGS_RDX, eax); return 0; } @@ -1259,10 +1263,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) svm->asid_generation = 0; init_vmcb(svm); - err = fx_init(&svm->vcpu); - if (err) - goto free_page4; - svm->vcpu.arch.apic_base = 0xfee00000 | MSR_IA32_APICBASE_ENABLE; if (kvm_vcpu_is_bsp(&svm->vcpu)) svm->vcpu.arch.apic_base |= MSR_IA32_APICBASE_BSP; @@ -1271,8 +1271,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) return &svm->vcpu; -free_page4: - __free_page(hsave_page); free_page3: __free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER); free_page2: diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 2fd2046..6adbad6 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -3934,8 +3934,6 @@ 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)); - vmx->rmode.vm86_active = 0; vmx->soft_vnmi_blocked = 0; @@ -3947,10 +3945,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); @@ -3991,7 +3985,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_GDTR_BASE, 0); vmcs_write32(GUEST_GDTR_LIMIT, 0xffff); @@ -4041,7 +4034,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu) /* HACK: Don't enable emulation on guest boot/reset */ vmx->emulation_required = 0; -out: return ret; } diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 3bdaf29..57c76e8 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6461,6 +6461,10 @@ static int kvm_vcpu_reset(struct kvm_vcpu *vcpu) kvm_pmu_reset(vcpu); + memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs)); + vcpu->arch.regs_avail = ~0; + vcpu->arch.regs_dirty = ~0; + return kvm_x86_ops->vcpu_reset(vcpu); } @@ -6629,11 +6633,17 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) if (!zalloc_cpumask_var(&vcpu->arch.wbinvd_dirty_mask, GFP_KERNEL)) goto fail_free_mce_banks; + r = fx_init(vcpu); + if (r) + goto fail_free_wbinvd_dirty_mask; + vcpu->arch.ia32_tsc_adjust_msr = 0x0; kvm_async_pf_hash_reset(vcpu); kvm_pmu_init(vcpu); return 0; +fail_free_wbinvd_dirty_mask: + free_cpumask_var(vcpu->arch.wbinvd_dirty_mask); fail_free_mce_banks: kfree(vcpu->arch.mce_banks); fail_free_lapic: -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5] KVM: x86: Make register state after reset conform to specification 2012-12-05 14:26 ` [PATCH v5] " Julian Stecklina @ 2012-12-05 16:02 ` Gleb Natapov 0 siblings, 0 replies; 15+ messages in thread From: Gleb Natapov @ 2012-12-05 16:02 UTC (permalink / raw) To: Julian Stecklina; +Cc: kvm On Wed, Dec 05, 2012 at 03:26:19PM +0100, Julian Stecklina wrote: > VMX behaves now as SVM wrt to FPU initialization. Code has been moved to > generic code path. General-purpose registers are now cleared on reset and > INIT. SVM code properly initializes EDX. > > Signed-off-by: Julian Stecklina <jsteckli@os.inf.tu-dresden.de> Applied to queue. Thanks. > --- > arch/x86/kvm/cpuid.c | 1 + > arch/x86/kvm/svm.c | 14 ++++++-------- > arch/x86/kvm/vmx.c | 8 -------- > arch/x86/kvm/x86.c | 10 ++++++++++ > 4 files changed, 17 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 52f6166..a20ecb5 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -661,6 +661,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 dcb79527..d29d3cd 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 <linux/module.h> > #include <linux/mod_devicetable.h> > @@ -1193,6 +1194,8 @@ 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; > + u32 eax = 1; > > init_vmcb(svm); > > @@ -1201,8 +1204,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); > + kvm_register_write(vcpu, VCPU_REGS_RDX, eax); > > return 0; > } > @@ -1259,10 +1263,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) > svm->asid_generation = 0; > init_vmcb(svm); > > - err = fx_init(&svm->vcpu); > - if (err) > - goto free_page4; > - > svm->vcpu.arch.apic_base = 0xfee00000 | MSR_IA32_APICBASE_ENABLE; > if (kvm_vcpu_is_bsp(&svm->vcpu)) > svm->vcpu.arch.apic_base |= MSR_IA32_APICBASE_BSP; > @@ -1271,8 +1271,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) > > return &svm->vcpu; > > -free_page4: > - __free_page(hsave_page); > free_page3: > __free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER); > free_page2: > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 2fd2046..6adbad6 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -3934,8 +3934,6 @@ 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)); > - > vmx->rmode.vm86_active = 0; > > vmx->soft_vnmi_blocked = 0; > @@ -3947,10 +3945,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); > @@ -3991,7 +3985,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_GDTR_BASE, 0); > vmcs_write32(GUEST_GDTR_LIMIT, 0xffff); > @@ -4041,7 +4034,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu) > /* HACK: Don't enable emulation on guest boot/reset */ > vmx->emulation_required = 0; > > -out: > return ret; > } > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3bdaf29..57c76e8 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6461,6 +6461,10 @@ static int kvm_vcpu_reset(struct kvm_vcpu *vcpu) > > kvm_pmu_reset(vcpu); > > + memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs)); > + vcpu->arch.regs_avail = ~0; > + vcpu->arch.regs_dirty = ~0; > + > return kvm_x86_ops->vcpu_reset(vcpu); > } > > @@ -6629,11 +6633,17 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) > if (!zalloc_cpumask_var(&vcpu->arch.wbinvd_dirty_mask, GFP_KERNEL)) > goto fail_free_mce_banks; > > + r = fx_init(vcpu); > + if (r) > + goto fail_free_wbinvd_dirty_mask; > + > vcpu->arch.ia32_tsc_adjust_msr = 0x0; > kvm_async_pf_hash_reset(vcpu); > kvm_pmu_init(vcpu); > > return 0; > +fail_free_wbinvd_dirty_mask: > + free_cpumask_var(vcpu->arch.wbinvd_dirty_mask); > fail_free_mce_banks: > kfree(vcpu->arch.mce_banks); > fail_free_lapic: > -- > 1.7.11.7 -- Gleb. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] KVM: x86: Make register state after reset conform to specification 2012-12-05 13:27 ` Gleb Natapov 2012-12-05 14:26 ` [PATCH v5] " Julian Stecklina @ 2012-12-05 14:27 ` Julian Stecklina 1 sibling, 0 replies; 15+ messages in thread From: Julian Stecklina @ 2012-12-05 14:27 UTC (permalink / raw) To: Gleb Natapov; +Cc: kvm Thus spake Gleb Natapov <gleb@redhat.com>: >> - ret = fx_init(&vmx->vcpu); >> - if (ret != 0) >> - goto out; >> - > Label "out" is now unused. Compiler complains. 5th time's the charm. ;) Patch is updated. Julian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: KVM VMX: register state after reset violates spec 2012-12-02 13:38 ` KVM VMX: register state after reset violates spec Gleb Natapov 2012-12-03 12:05 ` Julian Stecklina 2012-12-03 13:46 ` [PATCH v2] KVM: x86: Make register state after reset conform to specification Julian Stecklina @ 2012-12-03 13:49 ` Julian Stecklina 2 siblings, 0 replies; 15+ messages in thread From: Julian Stecklina @ 2012-12-03 13:49 UTC (permalink / raw) To: Gleb Natapov; +Cc: kvm Thus spake Gleb Natapov <gleb@redhat.com>: > It should, so why not move the fix to kvm_vcpu_reset() so it will work > for both. Also what about R8-R15? Intel SDM says nothing about them in > the section you mention, but in Volume 1 section 3.4.1.1 is says: [...] > I take it that they are undefined on the first transition to 64-bit mode > too. AMD spec says that they should be zeroed on reset, so lets do that. > Also SVM does not set EDX to correct value on reset. It should be: I have posted a new version of the patch taking your suggestions into account. The VMX version is working for me. I could not test it on AMD hardware, though. Julian ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-12-05 16:02 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-29 14:07 KVM VMX: register state after reset violates spec Julian Stecklina 2012-11-29 14:38 ` [PATCH] KVM VMX: Make register state after reset conform to specification Julian Stecklina 2012-11-29 14:55 ` Julian Stecklina 2012-12-02 13:38 ` KVM VMX: register state after reset violates spec Gleb Natapov 2012-12-03 12:05 ` Julian Stecklina 2012-12-03 13:46 ` [PATCH v2] KVM: x86: Make register state after reset conform to specification Julian Stecklina 2012-12-04 9:48 ` Gleb Natapov 2012-12-04 12:13 ` [PATCH v3] " Julian Stecklina 2012-12-05 10:50 ` Gleb Natapov 2012-12-05 12:00 ` [PATCH v4] " Julian Stecklina 2012-12-05 13:27 ` Gleb Natapov 2012-12-05 14:26 ` [PATCH v5] " Julian Stecklina 2012-12-05 16:02 ` Gleb Natapov 2012-12-05 14:27 ` [PATCH v4] " Julian Stecklina 2012-12-03 13:49 ` KVM VMX: register state after reset violates spec Julian Stecklina
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).