On 06/01/2010 08:12 PM, Marcelo Tosatti wrote:
On Mon, May 31, 2010 at 07:54:11PM +0800, Sheng Yang wrote:diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 99ae513..8649627 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -36,6 +36,8 @@ #include<asm/vmx.h> #include<asm/virtext.h> #include<asm/mce.h> +#include<asm/i387.h> +#include<asm/xcr.h> #include "trace.h" @@ -3354,6 +3356,16 @@ static int handle_wbinvd(struct kvm_vcpu *vcpu) return 1; } +static int handle_xsetbv(struct kvm_vcpu *vcpu) +{ + u64 new_bv = kvm_read_edx_eax(vcpu); + u32 index = kvm_register_read(vcpu, VCPU_REGS_RCX); + + kvm_set_xcr(vcpu, index, new_bv); + skip_emulated_instruction(vcpu);Should only skip_emulated_instruction if no exception was injected.
Good catch. So, unit testing failure cases _is_ important.
+int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr) +{ + u64 xcr0; + + /* Only support XCR_XFEATURE_ENABLED_MASK(xcr0) now */ + if (index != XCR_XFEATURE_ENABLED_MASK) + return 1; + xcr0 = xcr; + if (kvm_x86_ops->get_cpl(vcpu) != 0) + return 1; + if (!(xcr0& XSTATE_FP)) + return 1; + if ((xcr0& XSTATE_YMM)&& !(xcr0& XSTATE_SSE)) + return 1; + if (xcr0& ~host_xcr0) + return 1; + vcpu->arch.xcr0 = xcr0; + xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);Won't this happen on guest entry, since vcpu->guest_xcr0_loaded == 0?
In fact we don't know what vcpu->guest_xcr0_loaded is. Best to unload it explicitly (and drop the xsetbv() call).
+ vcpu->guest_xcr0_loaded = 1; + } +} + +static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu) +{ + if (kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE)&& + vcpu->guest_xcr0_loaded) { + xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0); + vcpu->guest_xcr0_loaded = 0; + } +}What if you load guest's XCR0, then guest clears CR4.OSXSAVE? (restore of host_xcr0 should be conditional on guest_xcr0_loaded only).
Good catch again.
@@ -5140,6 +5250,8 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu) void kvm_put_guest_fpu(struct kvm_vcpu *vcpu) { + kvm_put_guest_xcr0(vcpu); +Should be in kvm_arch_vcpu_put?
That's called unconditionally from kvm_arch_vcpu_put(), so equivalent. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- 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
