[PATCH v4] KVM: VMX: Enable XSAVE/XRSTORE for guest

[PATCH v4] KVM: VMX: Enable XSAVE/XRSTORE for guest

[Sheng Yang]

From: Dexuan Cui <dexuan.cui@intel.com>

Enable XSAVE/XRSTORE for guest.

Change from V3:
1. Enforced the assumption that host OS would use all available xstate bits.
2. Various fixes, addressed Avi's comments.

Change from V2:
Various fixes, addressed Avi's comments.

Change from V1:

1. Use FPU API.
2. Fix CPUID issue.
3. Save/restore all possible guest xstate fields when switching. Because we
don't know which fields guest has already touched.

Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>
Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---

I am still not clear about why we need to reload guest xcr0 when cr4.osxsave
set...

 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/include/asm/vmx.h      |    1 +
 arch/x86/kvm/kvm_cache_regs.h   |    6 +++
 arch/x86/kvm/vmx.c              |   26 +++++++++++
 arch/x86/kvm/x86.c              |   88 +++++++++++++++++++++++++++++++++++---
 5 files changed, 115 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d08bb4a..3938bd1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -302,6 +302,7 @@ struct kvm_vcpu_arch {
 	} update_pte;
 
 	struct fpu guest_fpu;
+	u64 xcr0;
 
 	gva_t mmio_fault_cr2;
 	struct kvm_pio_request pio;
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 9e6779f..346ea66 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -266,6 +266,7 @@ enum vmcs_field {
 #define EXIT_REASON_EPT_VIOLATION       48
 #define EXIT_REASON_EPT_MISCONFIG       49
 #define EXIT_REASON_WBINVD		54
+#define EXIT_REASON_XSETBV		55
 
 /*
  * Interruption-information format
diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index d2a98f8..6491ac8 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -71,4 +71,10 @@ static inline ulong kvm_read_cr4(struct kvm_vcpu *vcpu)
 	return kvm_read_cr4_bits(vcpu, ~0UL);
 }
 
+static inline u64 kvm_read_edx_eax(struct kvm_vcpu *vcpu)
+{
+	return (kvm_register_read(vcpu, VCPU_REGS_RAX) & -1u)
+		| ((u64)(kvm_register_read(vcpu, VCPU_REGS_RDX) & -1u) << 32);
+}
+
 #endif
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 99ae513..a4881ff 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,29 @@ 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);
+
+	if (kvm_register_read(vcpu, VCPU_REGS_RCX) != 0)
+		goto err;
+	if (vmx_get_cpl(vcpu) != 0)
+		goto err;
+	if (!(new_bv & XSTATE_FP))
+		goto err;
+	if ((new_bv & XSTATE_YMM) && !(new_bv & XSTATE_SSE))
+		goto err;
+	if (new_bv & ~XCNTXT_MASK)
+		goto err;
+	vcpu->arch.xcr0 = new_bv;
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);
+	skip_emulated_instruction(vcpu);
+	return 1;
+err:
+	kvm_inject_gp(vcpu, 0);
+	return 1;
+}
+
 static int handle_apic_access(struct kvm_vcpu *vcpu)
 {
 	return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
@@ -3632,6 +3657,7 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
 	[EXIT_REASON_TPR_BELOW_THRESHOLD]     = handle_tpr_below_threshold,
 	[EXIT_REASON_APIC_ACCESS]             = handle_apic_access,
 	[EXIT_REASON_WBINVD]                  = handle_wbinvd,
+	[EXIT_REASON_XSETBV]                  = handle_xsetbv,
 	[EXIT_REASON_TASK_SWITCH]             = handle_task_switch,
 	[EXIT_REASON_MCE_DURING_VMENTRY]      = handle_machine_check,
 	[EXIT_REASON_EPT_VIOLATION]	      = handle_ept_violation,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7be1d36..2214632 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -64,6 +64,7 @@
 	(~(unsigned long)(X86_CR4_VME | X86_CR4_PVI | X86_CR4_TSD | X86_CR4_DE\
 			  | X86_CR4_PSE | X86_CR4_PAE | X86_CR4_MCE	\
 			  | X86_CR4_PGE | X86_CR4_PCE | X86_CR4_OSFXSR	\
+			  | X86_CR4_OSXSAVE \
 			  | X86_CR4_OSXMMEXCPT | X86_CR4_VMXE))
 
 #define CR8_RESERVED_BITS (~(unsigned long)X86_CR8_TPR)
@@ -149,6 +150,13 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ NULL }
 };
 
+static u64 __read_mostly host_xcr0;
+
+static inline u32 bit(int bitno)
+{
+	return 1 << (bitno & 31);
+}
+
 static void kvm_on_user_return(struct user_return_notifier *urn)
 {
 	unsigned slot;
@@ -473,6 +481,30 @@ void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long msw)
 }
 EXPORT_SYMBOL_GPL(kvm_lmsw);
 
+static bool guest_cpuid_has_xsave(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 1, 0);
+	return best && (best->ecx & bit(X86_FEATURE_XSAVE));
+}
+
+static void update_cpuid(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 1, 0);
+	if (!best)
+		return;
+
+	/* Update OSXSAVE bit */
+	if (cpu_has_xsave && best->function == 0x1) {
+		best->ecx &= ~(bit(X86_FEATURE_OSXSAVE));
+		if (kvm_read_cr4(vcpu) & X86_CR4_OSXSAVE)
+			best->ecx |= bit(X86_FEATURE_OSXSAVE);
+	}
+}
+
 int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 {
 	unsigned long old_cr4 = kvm_read_cr4(vcpu);
@@ -481,6 +513,9 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 	if (cr4 & CR4_RESERVED_BITS)
 		return 1;
 
+	if (!guest_cpuid_has_xsave(vcpu) && (cr4 & X86_CR4_OSXSAVE))
+		return 1;
+
 	if (is_long_mode(vcpu)) {
 		if (!(cr4 & X86_CR4_PAE))
 			return 1;
@@ -497,6 +532,9 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 	if ((cr4 ^ old_cr4) & pdptr_bits)
 		kvm_mmu_reset_context(vcpu);
 
+	if ((cr4 ^ old_cr4) & X86_CR4_OSXSAVE)
+		update_cpuid(vcpu);
+
 	return 0;
 }
 
@@ -665,11 +703,6 @@ int kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
 }
 EXPORT_SYMBOL_GPL(kvm_get_dr);
 
-static inline u32 bit(int bitno)
-{
-	return 1 << (bitno & 31);
-}
-
 /*
  * List of msr numbers which we expose to userspace through KVM_GET_MSRS
  * and KVM_SET_MSRS, and KVM_GET_MSR_INDEX_LIST.
@@ -1742,6 +1775,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		per_cpu(cpu_tsc_khz, cpu) = khz;
 	}
 	kvm_request_guest_time_update(vcpu);
+
+	if (kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE))
+		xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
@@ -1813,6 +1849,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	r = 0;
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops->cpuid_update(vcpu);
+	update_cpuid(vcpu);
 
 out_free:
 	vfree(cpuid_entries);
@@ -1836,6 +1873,7 @@ static int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 	vcpu->arch.cpuid_nent = cpuid->nent;
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops->cpuid_update(vcpu);
+	update_cpuid(vcpu);
 	return 0;
 
 out:
@@ -1916,7 +1954,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		0 /* Reserved */ | F(CX16) | 0 /* xTPR Update, PDCM */ |
 		0 /* Reserved, DCA */ | F(XMM4_1) |
 		F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) |
-		0 /* Reserved, XSAVE, OSXSAVE */;
+		0 /* Reserved, AES */ | F(XSAVE) | 0 /* OSXSAVE */;
 	/* cpuid 0x80000001.ecx */
 	const u32 kvm_supported_word6_x86_features =
 		F(LAHF_LM) | F(CMP_LEGACY) | F(SVM) | 0 /* ExtApicSpace */ |
@@ -1931,7 +1969,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 
 	switch (function) {
 	case 0:
-		entry->eax = min(entry->eax, (u32)0xb);
+		entry->eax = min(entry->eax, (u32)0xd);
 		break;
 	case 1:
 		entry->edx &= kvm_supported_word0_x86_features;
@@ -1989,6 +2027,20 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		}
 		break;
 	}
+	case 0xd: {
+		int i;
+
+		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+		for (i = 1; *nent < maxnent; ++i) {
+			if (entry[i - 1].eax == 0 && i != 2)
+				break;
+			do_cpuid_1_ent(&entry[i], function, i);
+			entry[i].flags |=
+			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+			++*nent;
+		}
+		break;
+	}
 	case KVM_CPUID_SIGNATURE: {
 		char signature[12] = "KVMKVMKVM\0\0";
 		u32 *sigptr = (u32 *)signature;
@@ -4124,6 +4176,8 @@ int kvm_arch_init(void *opaque)
 
 	perf_register_guest_info_callbacks(&kvm_guest_cbs);
 
+	host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
+
 	return 0;
 
 out:
@@ -5118,6 +5172,13 @@ void fx_init(struct kvm_vcpu *vcpu)
 	fpu_alloc(&vcpu->arch.guest_fpu);
 	fpu_finit(&vcpu->arch.guest_fpu);
 
+	/*
+	 * Ensure guest xcr0 is valid for loading, also using as
+	 * the indicator of if guest cpuid has XSAVE
+	 */
+	if (guest_cpuid_has_xsave(vcpu))
+		vcpu->arch.xcr0 = XSTATE_FP;
+
 	vcpu->arch.cr0 |= X86_CR0_ET;
 }
 EXPORT_SYMBOL_GPL(fx_init);
@@ -5134,6 +5195,12 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu)
 
 	vcpu->guest_fpu_loaded = 1;
 	unlazy_fpu(current);
+	/*
+	 * Restore all possible states in the guest,
+	 * and assume host would use all available bits.
+	 */
+	if (cpu_has_xsave && vcpu->arch.xcr0)
+		xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
 	fpu_restore_checking(&vcpu->arch.guest_fpu);
 	trace_kvm_fpu(1);
 }
@@ -5144,6 +5211,13 @@ void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
 		return;
 
 	vcpu->guest_fpu_loaded = 0;
+	/*
+	 * Save all possible states in the guest,
+	 * and assume host would use all available bits.
+	 * Also load host_xcr0 for host usage.
+	 */
+	if (cpu_has_xsave && vcpu->arch.xcr0)
+		xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
 	fpu_save_init(&vcpu->arch.guest_fpu);
 	++vcpu->stat.fpu_reload;
 	set_bit(KVM_REQ_DEACTIVATE_FPU, &vcpu->requests);
-- 
1.7.0.1

Re: [PATCH v4] KVM: VMX: Enable XSAVE/XRSTORE for guest

[Avi Kivity]

On 05/24/2010 01:03 PM, Sheng Yang wrote:
> From: Dexuan Cui<dexuan.cui@intel.com>
>
> Enable XSAVE/XRSTORE for guest.
>
> Change from V3:
> 1. Enforced the assumption that host OS would use all available xstate bits.
> 2. Various fixes, addressed Avi's comments.
>
> I am still not clear about why we need to reload guest xcr0 when cr4.osxsave
> set...
>    

When cr4.osxsave=0, then the guest executes with the host xcr0 (since 
xgetbv will trap; this is similar to the guest running with the host fpu 
if cr0.ts=0).  So if cr4.osxsave transtions, we need to transition xcr0 
as well.

> @@ -3354,6 +3356,29 @@ 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);
> +
> +	if (kvm_register_read(vcpu, VCPU_REGS_RCX) != 0)
> +		goto err;
> +	if (vmx_get_cpl(vcpu) != 0)
> +		goto err;
> +	if (!(new_bv&  XSTATE_FP))
> +		goto err;
> +	if ((new_bv&  XSTATE_YMM)&&  !(new_bv&  XSTATE_SSE))
> +		goto err;
> +	if (new_bv&  ~XCNTXT_MASK)
> +		goto err;
>    

Ok.  This means we must update kvm immediately when XCNTXT_MASK changes.

(Otherwise we would use KVM_XCNTXT_MASK which is always smaller than 
than XCNTXT_MASK).

> +	vcpu->arch.xcr0 = new_bv;
> +	xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);
> +	skip_emulated_instruction(vcpu);
> +	return 1;
> +err:
> +	kvm_inject_gp(vcpu, 0);
> +	return 1;
> +}
> +
>
>
> @@ -4124,6 +4176,8 @@ int kvm_arch_init(void *opaque)
>
>   	perf_register_guest_info_callbacks(&kvm_guest_cbs);
>
> +	host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
> +
>   	return 0;
>    

Will fault on old cpu.

>   EXPORT_SYMBOL_GPL(fx_init);
> @@ -5134,6 +5195,12 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu)
>
>   	vcpu->guest_fpu_loaded = 1;
>   	unlazy_fpu(current);
> +	/*
> +	 * Restore all possible states in the guest,
> +	 * and assume host would use all available bits.
> +	 */
> +	if (cpu_has_xsave&&  vcpu->arch.xcr0)
> +		xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
>   	fpu_restore_checking(&vcpu->arch.guest_fpu);
>    

I think we need to reload xcr0 now to the guest's value.

>   	trace_kvm_fpu(1);
>   }
> @@ -5144,6 +5211,13 @@ void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
>   		return;
>
>   	vcpu->guest_fpu_loaded = 0;
> +	/*
> +	 * Save all possible states in the guest,
> +	 * and assume host would use all available bits.
> +	 * Also load host_xcr0 for host usage.
> +	 */
> +	if (cpu_has_xsave&&  vcpu->arch.xcr0)
> +		xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
>   	fpu_save_init(&vcpu->arch.guest_fpu);
>   	++vcpu->stat.fpu_reload;
>   	set_bit(KVM_REQ_DEACTIVATE_FPU,&vcpu->requests);
>    

This might be unnecessary.

So far xcr0 life cycle is almost that of 
save_host_state()/load_host_state(), but not exactly.  When loading the 
guest fpu we switch temporarily to host xcr0, then we have to switch 
back, but only if gcr4.osxsave.  When saving the guest fpu, we're 
already using the host xcr0:

> void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> {
>     kvm_x86_ops->vcpu_put(vcpu);
>     kvm_put_guest_fpu(vcpu);
> }

One way to simplify this is to have a vcpu->guest_xcr0_loaded flag and 
check it when needed.  So the transition matrix is:

   save_host_state: if gcr4.osxsave, set guest_xcr0_loaded, load it
   set gcr4.osxsave: ditto
   clear gcr4.osxsave: do nothing
   load_host_state: if guest_xcr0_loaded, clear it, reload host xcr0
   fpu switching: if (switched) switch; reload fpu; if (switched) switch

may be simplified if we move xcr0 reload back to guest entry (... :)  
but make it lazy:

   save_host_state: nothing
   set cr4.osxsave: nothing
   clear cr4.osxsave: nothing
   guest entry: if (gcr4.osxsave && !guest_xcr0_loaded) { 
guest_xcr0_loaded = true, load gxcr0 }
   load_host_state: if (guest_xcr0_loaded) { guest_xcr0_loaded = false; 
load host xcr0 }
   fpu switching:  if (guest_xcr0_loaded) { guest_xcr0_loaded = false; 
load host xcr0 }, do fpu stuff

So we delay xcr0 reload as late as possible for both entry and exit.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH v4] KVM: VMX: Enable XSAVE/XRSTORE for guest

[Avi Kivity]

On 05/24/2010 04:36 PM, Avi Kivity wrote:
>
> may be simplified if we move xcr0 reload back to guest entry (... :)  
> but make it lazy:
>
>   save_host_state: nothing
>   set cr4.osxsave: nothing
>   clear cr4.osxsave: nothing
>   guest entry: if (gcr4.osxsave && !guest_xcr0_loaded) { 
> guest_xcr0_loaded = true, load gxcr0 }
>   load_host_state: if (guest_xcr0_loaded) { guest_xcr0_loaded = false; 
> load host xcr0 }
>   fpu switching:  if (guest_xcr0_loaded) { guest_xcr0_loaded = false; 
> load host xcr0 }, do fpu stuff
>
> So we delay xcr0 reload as late as possible for both entry and exit.
>

btw, this is similar to how we switch the fpu itself: 
if-needed-and-not-already-loaded on guest entry, if-loaded on vcpu_put.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH v4] KVM: VMX: Enable XSAVE/XRSTORE for guest

[Sheng Yang]

On Monday 24 May 2010 21:36:12 Avi Kivity wrote:
> On 05/24/2010 01:03 PM, Sheng Yang wrote:
> > From: Dexuan Cui<dexuan.cui@intel.com>
> > 
> > Enable XSAVE/XRSTORE for guest.
> > 
> > Change from V3:
> > 1. Enforced the assumption that host OS would use all available xstate
> > bits. 2. Various fixes, addressed Avi's comments.
> > 
> > I am still not clear about why we need to reload guest xcr0 when
> > cr4.osxsave set...
> 
> When cr4.osxsave=0, then the guest executes with the host xcr0 (since
> xgetbv will trap; this is similar to the guest running with the host fpu
> if cr0.ts=0).  So if cr4.osxsave transtions, we need to transition xcr0
> as well.

Yes...
> 
> > @@ -3354,6 +3356,29 @@ 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);
> > +
> > +	if (kvm_register_read(vcpu, VCPU_REGS_RCX) != 0)
> > +		goto err;
> > +	if (vmx_get_cpl(vcpu) != 0)
> > +		goto err;
> > +	if (!(new_bv&  XSTATE_FP))
> > +		goto err;
> > +	if ((new_bv&  XSTATE_YMM)&&  !(new_bv&  XSTATE_SSE))
> > +		goto err;
> > +	if (new_bv&  ~XCNTXT_MASK)
> > +		goto err;
> 
> Ok.  This means we must update kvm immediately when XCNTXT_MASK changes.
> 
> (Otherwise we would use KVM_XCNTXT_MASK which is always smaller than
> than XCNTXT_MASK).

I guess use host_xcr0 here is better?
> 
> > +	vcpu->arch.xcr0 = new_bv;
> > +	xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);
> > +	skip_emulated_instruction(vcpu);
> > +	return 1;
> > +err:
> > +	kvm_inject_gp(vcpu, 0);
> > +	return 1;
> > +}
> > +
> > 
> > 
> > @@ -4124,6 +4176,8 @@ int kvm_arch_init(void *opaque)
> > 
> >   	perf_register_guest_info_callbacks(&kvm_guest_cbs);
> > 
> > +	host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
> > +
> > 
> >   	return 0;
> 
> Will fault on old cpu.

...
> 
> >   EXPORT_SYMBOL_GPL(fx_init);
> > 
> > @@ -5134,6 +5195,12 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu)
> > 
> >   	vcpu->guest_fpu_loaded = 1;
> >   	unlazy_fpu(current);
> > 
> > +	/*
> > +	 * Restore all possible states in the guest,
> > +	 * and assume host would use all available bits.
> > +	 */
> > +	if (cpu_has_xsave&&  vcpu->arch.xcr0)
> > +		xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
> > 
> >   	fpu_restore_checking(&vcpu->arch.guest_fpu);
> 
> I think we need to reload xcr0 now to the guest's value.
> 
> >   	trace_kvm_fpu(1);
> >   
> >   }
> > 
> > @@ -5144,6 +5211,13 @@ void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
> > 
> >   		return;
> >   	
> >   	vcpu->guest_fpu_loaded = 0;
> > 
> > +	/*
> > +	 * Save all possible states in the guest,
> > +	 * and assume host would use all available bits.
> > +	 * Also load host_xcr0 for host usage.
> > +	 */
> > +	if (cpu_has_xsave&&  vcpu->arch.xcr0)
> > +		xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
> > 
> >   	fpu_save_init(&vcpu->arch.guest_fpu);
> >   	++vcpu->stat.fpu_reload;
> >   	set_bit(KVM_REQ_DEACTIVATE_FPU,&vcpu->requests);
> 
> This might be unnecessary.
> 
> So far xcr0 life cycle is almost that of
> save_host_state()/load_host_state(), but not exactly.  When loading the
> guest fpu we switch temporarily to host xcr0, then we have to switch
> back, but only if gcr4.osxsave.  When saving the guest fpu, we're
> 
> already using the host xcr0:
> > void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> > {
> > 
> >     kvm_x86_ops->vcpu_put(vcpu);
> >     kvm_put_guest_fpu(vcpu);
> > 
> > }
> 
> One way to simplify this is to have a vcpu->guest_xcr0_loaded flag and
> check it when needed.  So the transition matrix is:
> 
>    save_host_state: if gcr4.osxsave, set guest_xcr0_loaded, load it
>    set gcr4.osxsave: ditto
>    clear gcr4.osxsave: do nothing
>    load_host_state: if guest_xcr0_loaded, clear it, reload host xcr0
>    fpu switching: if (switched) switch; reload fpu; if (switched) switch
> 
> may be simplified if we move xcr0 reload back to guest entry (... :)
> but make it lazy:
> 
>    save_host_state: nothing
>    set cr4.osxsave: nothing
>    clear cr4.osxsave: nothing
>    guest entry: if (gcr4.osxsave && !guest_xcr0_loaded) {
> guest_xcr0_loaded = true, load gxcr0 }
>    load_host_state: if (guest_xcr0_loaded) { guest_xcr0_loaded = false;
> load host xcr0 }
>    fpu switching:  if (guest_xcr0_loaded) { guest_xcr0_loaded = false;
> load host xcr0 }, do fpu stuff
> 
> So we delay xcr0 reload as late as possible for both entry and exit.

I think I got it. But why we need do it at "load_host_state()"? I guess just put 
code before fpu testing in kvm_put_guest_fpu() is fine?

--
regards
Yang, Sheng

Re: [PATCH v4] KVM: VMX: Enable XSAVE/XRSTORE for guest

[Avi Kivity]

On 05/25/2010 09:28 AM, Sheng Yang wrote:
>>
>>> @@ -3354,6 +3356,29 @@ 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);
>>> +
>>> +	if (kvm_register_read(vcpu, VCPU_REGS_RCX) != 0)
>>> +		goto err;
>>> +	if (vmx_get_cpl(vcpu) != 0)
>>> +		goto err;
>>> +	if (!(new_bv&   XSTATE_FP))
>>> +		goto err;
>>> +	if ((new_bv&   XSTATE_YMM)&&   !(new_bv&   XSTATE_SSE))
>>> +		goto err;
>>> +	if (new_bv&   ~XCNTXT_MASK)
>>> +		goto err;
>>>        
>> Ok.  This means we must update kvm immediately when XCNTXT_MASK changes.
>>
>> (Otherwise we would use KVM_XCNTXT_MASK which is always smaller than
>> than XCNTXT_MASK).
>>      
> I guess use host_xcr0 here is better?
>    

Yes - it might be smaller than XCNTXT_MASK

>> may be simplified if we move xcr0 reload back to guest entry (... :)
>> but make it lazy:
>>
>>     save_host_state: nothing
>>     set cr4.osxsave: nothing
>>     clear cr4.osxsave: nothing
>>     guest entry: if (gcr4.osxsave&&  !guest_xcr0_loaded) {
>> guest_xcr0_loaded = true, load gxcr0 }
>>     load_host_state: if (guest_xcr0_loaded) { guest_xcr0_loaded = false;
>> load host xcr0 }
>>     fpu switching:  if (guest_xcr0_loaded) { guest_xcr0_loaded = false;
>> load host xcr0 }, do fpu stuff
>>
>> So we delay xcr0 reload as late as possible for both entry and exit.
>>      
> I think I got it. But why we need do it at "load_host_state()"? I guess just put
> code before fpu testing in kvm_put_guest_fpu() is fine?
>    

Right, load_host_state() is bad because it is vmx specific.  
kvm_put_guest_fpu() (or perhaps kvm_arch_vcpu_put()) is better.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v4] KVM: VMX: Enable XSAVE/XRSTORE for guest

[Avi Kivity]

On 05/24/2010 01:03 PM, Sheng Yang wrote:
> From: Dexuan Cui<dexuan.cui@intel.com>
>
> Enable XSAVE/XRSTORE for guest.
>
>    

btw2, still missing save/restore support for xcr and xsave-capable fpu.  
Please make that a separate patch, this one is complicated enough already.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.