KVM: X86: add the support of XSAVE/XRSTOR to guest

PATCH In review Dexuan Cui 2 participants 7 messages Apr 30, 2010 – May 6, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Dexuan Cui

author

2010-04-30 17:21 UTC #0 lore raw

PATCH 1/1 KVM: X86: add the support of XSAVE/XRSTOR to guest

When the host enables XSAVE/XRSTOR, the patch exposes the XSAVE/XRSTOR related CPUID leaves to guest by fixing up kvm_emulate_cpuid() and the patch allows guest to set CR4.OSXSAVE to enable XSAVE.
The patch adds per-vcpu host/guest xstate image/mask and enhances the current FXSAVE/FRSTOR with the new XSAVE/XRSTOR on the host xstate (FPU/SSE/YMM) switch.

Signed-off-by Dexuan Cui <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |   15 +--
 arch/x86/include/asm/vmx.h      |    1 +
 arch/x86/include/asm/xsave.h    |    3 +
 arch/x86/kvm/vmx.c              |   24 +++++
 arch/x86/kvm/x86.c              |  217 +++++++++++++++++++++++++++++++++++++--
 5 files changed, 242 insertions(+), 18 deletions(-)

arch/x86/include/asm/kvm_host.h +5 ~~-10~~

index 3f0007b..60be1a7 100644
@@ -303,6 +303,11 @@ struct kvm_vcpu_arch {
 	struct i387_fxsave_struct host_fx_image;
 	struct i387_fxsave_struct guest_fx_image;
 
+	struct xsave_struct *host_xstate_image;
+	struct xsave_struct *guest_xstate_image;
+	uint64_t host_xstate_mask;
+	uint64_t guest_xstate_mask;
+
 	gva_t mmio_fault_cr2;
 	struct kvm_pio_request pio;
 	void *pio_data;

@@ -718,16 +723,6 @@ static inline unsigned long read_msr(unsigned long msr)
 }
 #endif
 
-static inline void kvm_fx_save(struct i387_fxsave_struct *image)
-{
-	asm("fxsave (%0)":: "r" (image));
-}
-
-static inline void kvm_fx_restore(struct i387_fxsave_struct *image)
-{
-	asm("fxrstor (%0)":: "r" (image));
-}
-
 static inline void kvm_fx_finit(void)
 {
 	asm("finit");

arch/x86/include/asm/vmx.h +1 -0

index fb9a080..842286b 100644
@@ -260,6 +260,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

arch/x86/include/asm/xsave.h +3 -0

index ddc04cc..ada81a2 100644
@@ -13,6 +13,9 @@
 
 #define FXSAVE_SIZE	512
 
+#define XSTATE_YMM_SIZE 256
+#define XSTATE_YMM_OFFSET (512 + 64)
+
 /*
  * These are the features that the OS can handle currently.
  */

arch/x86/kvm/vmx.c +24 -0

index 875b785..a72d024 100644
@@ -35,6 +35,8 @@
 #include <asm/vmx.h>
 #include <asm/virtext.h>
 #include <asm/mce.h>
+#include <asm/i387.h>
+#include <asm/xcr.h>
 
 #include "trace.h"

@@ -2517,6 +2519,8 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
 	vmx->vcpu.arch.cr4_guest_owned_bits = KVM_CR4_GUEST_OWNED_BITS;
 	if (enable_ept)
 		vmx->vcpu.arch.cr4_guest_owned_bits |= X86_CR4_PGE;
+	if (cpu_has_xsave)
+		vmx->vcpu.arch.cr4_guest_owned_bits |= X86_CR4_OSXSAVE;
 	vmcs_writel(CR4_GUEST_HOST_MASK, ~vmx->vcpu.arch.cr4_guest_owned_bits);
 
 	tsc_base = vmx->vcpu.kvm->arch.vm_init_tsc;

@@ -3258,6 +3262,25 @@ static int handle_wbinvd(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static int handle_xsetbv(struct kvm_vcpu *vcpu)
+{
+	u64 new_bv = ((u64)kvm_register_read(vcpu, VCPU_REGS_RDX)) |
+		kvm_register_read(vcpu, VCPU_REGS_RAX);
+	u64 host_bv = vcpu->arch.host_xstate_mask;
+
+	if (((new_bv ^ host_bv) & ~host_bv) || !(new_bv & 1))
+		goto err;
+	if ((host_bv & XSTATE_YMM & new_bv) && !(new_bv & XSTATE_SSE))
+		goto err;
+	vcpu->arch.guest_xstate_mask = new_bv;
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.guest_xstate_mask);
+	skip_emulated_instruction(vcpu);
+	return 1;
+err:
+	kvm_inject_gp(vcpu, 0);
+	return 1;
+}
+
 static int handle_apic_access(struct kvm_vcpu *vcpu)
 {
 	unsigned long exit_qualification;

@@ -3556,6 +3579,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,

arch/x86/kvm/x86.c +209 -8

319 lines not on this page · raw message

Signature · 1 line

1.6.0

Avi Kivity

2010-05-02 14:13 UTC #1 lore raw

Quoted 15 lines · Dexuan Cui

When the host enables XSAVE/XRSTOR, the patch exposes the XSAVE/XRSTOR related CPUID leaves to guest by fixing up kvm_emulate_cpuid() and the patch allows guest to set CR4.OSXSAVE to enable XSAVE.
The patch adds per-vcpu host/guest xstate image/mask and enhances the current FXSAVE/FRSTOR with the new XSAVE/XRSTOR on the host xstate (FPU/SSE/YMM) switch.
  5 files changed, 242 insertions(+), 18 deletions(-)
arch/x86/include/asm/kvm_host.h +0 -0
index 3f0007b..60be1a7 100644

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 309

Quoted 7 lines

arch/x86/include/asm/kvm_host.h +3 -0

@@ -303,6 +303,11 @@ struct kvm_vcpu_arch {
  	struct i387_fxsave_struct host_fx_image;
  	struct i387_fxsave_struct guest_fx_image;
 
+	struct xsave_struct *host_xstate_image;
+	struct xsave_struct *guest_xstate_image;
+	uint64_t host_xstate_mask;

Does host_xstate_mask need to be per-vcpu, or can it be global?

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 311

Quoted 2 lines

arch/x86/include/asm/kvm_host.h +1 -0
+	uint64_t guest_xstate_mask;
 

Can be called xcr0, like other shadow registers.

Quoted 7 lines

arch/x86/include/asm/kvm_host.h +1 -0
+
  	gva_t mmio_fault_cr2;
struct kvm_pio_request pio;
void *pio_data;

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3269

Quoted 8 lines

arch/x86/include/asm/kvm_host.h +4 -0

@@ -3258,6 +3262,25 @@ static int handle_wbinvd(struct kvm_vcpu *vcpu)
  	return 1;
  }
 
+static int handle_xsetbv(struct kvm_vcpu *vcpu)
+{
+	u64 new_bv = ((u64)kvm_register_read(vcpu, VCPU_REGS_RDX)) |
+		kvm_register_read(vcpu, VCPU_REGS_RAX);

Missing shift?

Probably worthwhile to create a helper for reading/writing edx:eax into a u64.

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3271

Quoted 2 lines

arch/x86/include/asm/kvm_host.h +1 -0
+	u64 host_bv = vcpu->arch.host_xstate_mask;
 

What about ecx?

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3273

Quoted 2 lines

arch/x86/include/asm/kvm_host.h +2 -0
+
+	if (((new_bv ^ host_bv)&  ~host_bv)

Isn't (new_bv & ~host_bv) equivalent? (guest cannot exceed host...)

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3274

Quoted 2 lines

arch/x86/include/asm/kvm_host.h +0 -0
  || !(new_bv&  1))

Symbolic value or comment.

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3276

Quoted 3 lines

arch/x86/include/asm/kvm_host.h +2 -0

+		goto err;
+	if ((host_bv&  XSTATE_YMM&  new_bv)&&  !(new_bv&  XSTATE_SSE))

host_bv unneeded, I think.

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3279

Quoted 4 lines

arch/x86/include/asm/kvm_host.h +3 -0

+		goto err;
+	vcpu->arch.guest_xstate_mask = new_bv;
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.guest_xstate_mask);

Can't we run with the host xcr0? isn't it guaranteed to be a superset of the guest xcr0?

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3283

Quoted 5 lines

arch/x86/include/asm/kvm_host.h +4 -0

+	skip_emulated_instruction(vcpu);
+	return 1;
+err:
+	kvm_inject_gp(vcpu, 0);

Need to #UD in some circumstances.

Quoted 28 lines

arch/x86/include/asm/kvm_host.h +3 -0

+	return 1;
+}
+

  static int handle_apic_access(struct kvm_vcpu *vcpu)
  {
  	unsigned long exit_qualification;

arch/x86/include/asm/kvm_host.h +1 -0

@@ -3556,6 +3579,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,

arch/x86/kvm/x86.c +2 -0

index 6b2ce1d..2af3fbe 100644
@@ -52,6 +52,8 @@
  #include<asm/desc.h>
  #include<asm/mtrr.h>
  #include<asm/mce.h>
+#include<asm/i387.h>
+#include<asm/xcr.h>
 
  #define MAX_IO_MSRS 256
  #define CR0_RESERVED_BITS						\

arch/x86/kvm/x86.c line 69

Quoted 6 lines

arch/x86/kvm/x86.c +1 -0

@@ -62,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	\
+			  | (cpu_has_xsave ? X86_CR4_OSXSAVE : 0)	\
  			  | X86_CR4_OSXMMEXCPT | X86_CR4_VMXE))

It also depends on the guest cpuid value. Please add it outside the macro, it's confusing to read something that looks like a constant but isn't.

Quoted 54 lines

arch/x86/kvm/x86.c +49 -0

  int kvm_emulate_halt(struct kvm_vcpu *vcpu)

@@ -4307,6 +4346,65 @@ not_found:
  	return 36;
  }
 
+#define bitmaskof(idx)  (1U<<  ((idx)&  31))
+static void kvm_emulate_cpuid_fixup(struct kvm_vcpu *vcpu, u32 func, u32 idx)
+{
+	u32 eax, ebx, ecx, edx;
+
+	if (func != 0&&  func != 1&&  func != 0xd)
+		return;
+
+	eax = kvm_register_read(vcpu, VCPU_REGS_RAX);
+	ebx = kvm_register_read(vcpu, VCPU_REGS_RBX);
+	ecx = kvm_register_read(vcpu, VCPU_REGS_RCX);
+	edx = kvm_register_read(vcpu, VCPU_REGS_RDX);
+
+	switch (func) {
+	case 0:
+		/* fixup the Maximum Input Value */
+		if (cpu_has_xsave&&  eax<  0xd)
+			eax = 0xd;
+		break;
+	case 1:
+		ecx&= ~(bitmaskof(X86_FEATURE_XSAVE) |
+			bitmaskof(X86_FEATURE_OSXSAVE));
+		if (!cpu_has_xsave)
+			break;
+		ecx |= bitmaskof(X86_FEATURE_XSAVE);
+		if (kvm_read_cr4(vcpu)&  X86_CR4_OSXSAVE)
+			ecx |= bitmaskof(X86_FEATURE_OSXSAVE);
+		break;
+	case 0xd:
+		eax = ebx = ecx = edx = 0;
+		if (!cpu_has_xsave)
+			break;
+		switch (idx) {
+		case 0:
+			eax = vcpu->arch.host_xstate_mask&  XCNTXT_MASK;
+			/* FP/SSE + XSAVE.HEADER + YMM. */
+			ecx = 512 + 64;
+			if (eax&  XSTATE_YMM)
+				ecx += XSTATE_YMM_SIZE;
+			ebx = ecx;
+			break;
+		case 2:
+			if (!(vcpu->arch.host_xstate_mask&  XSTATE_YMM))
+				break;
+			eax = XSTATE_YMM_SIZE;
+			ebx = XSTATE_YMM_OFFSET;
+			break;
+		default:
+			break;

arch/x86/kvm/x86.c not_found: · line 4407

Quoted 10 lines

arch/x86/kvm/x86.c +9 -0

+		}
+		break;
+	}
+
+	kvm_register_write(vcpu, VCPU_REGS_RAX, eax);
+	kvm_register_write(vcpu, VCPU_REGS_RBX, ebx);
+	kvm_register_write(vcpu, VCPU_REGS_RCX, ecx);
+	kvm_register_write(vcpu, VCPU_REGS_RDX, edx);
+}

This should be part of KVM_GET_SUPPORTED_CPUID.@@ -5091,6 +5192,60 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)

Quoted 48 lines

arch/x86/kvm/x86.c +44 -0

  	return 0;
  }
 
+#ifdef CONFIG_X86_64
+#define REX_PREFIX  "0x48, "
+#else
+#define REX_PREFIX
+#endif
+
+static inline void kvm_fx_save_host(struct kvm_vcpu *vcpu)
+{
+	if (cpu_has_xsave) {
+		asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x27"
+		: : "a" (-1), "d" (-1), "D"(vcpu->arch.host_xstate_image)
+		: "memory");
+		vcpu->arch.host_xstate_mask =
+			xgetbv(XCR_XFEATURE_ENABLED_MASK);
+	} else {
+		asm("fxsave (%0)" : : "r" (&vcpu->arch.host_fx_image));
+	}
+}
+
+static inline void kvm_fx_save_guest(struct kvm_vcpu *vcpu)
+{
+	if (cpu_has_xsave) {
+		asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x27"
+		: : "a" (-1), "d" (-1), "D"(vcpu->arch.guest_xstate_image)
+		: "memory");
+		vcpu->arch.guest_xstate_mask =
+			xgetbv(XCR_XFEATURE_ENABLED_MASK);
+	} else {
+		asm("fxsave (%0)" : : "r" (&vcpu->arch.guest_fx_image));
+	}
+}
+
+static inline void kvm_fx_restore_host(struct kvm_vcpu *vcpu)
+{
+	if (cpu_has_xsave) {
+		xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.host_xstate_mask);
+		asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x2f"
+		: : "a" (-1), "d" (-1), "D"(vcpu->arch.host_xstate_image));
+	} else {
+		asm("fxrstor (%0)" : : "r" (&vcpu->arch.host_fx_image));
+	}
+}
+
+static inline void kvm_fx_restore_guest(struct kvm_vcpu *vcpu)

arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) · line 5248

Quoted 10 lines

arch/x86/kvm/x86.c +10 -0

+{
+	if (cpu_has_xsave) {
+		xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.guest_xstate_mask);
+		asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x2f"
+		: : "a" (-1), "d" (-1), "D"(vcpu->arch.guest_xstate_image));
+	} else {
+		asm("fxrstor (%0)" : : "r" (&vcpu->arch.guest_fx_image));
+	}
+}
+

This mostly duplicates the standard x86 fpu code. I have a patch somewhere that abstracts it out, I'll dig it up and send it out.

Signature · 1 line

error compiling committee.c: too many arguments to function

Cui, Dexuan

author

2010-05-06 04:23 UTC #2 lore raw

Quoted 22 lines · Avi Kivity

Quoted 21 lines · Dexuan Cui
When the host enables XSAVE/XRSTOR, the patch exposes the XSAVE/XRSTOR
related CPUID leaves to guest by fixing up kvm_emulate_cpuid() and the
patch allows guest to set CR4.OSXSAVE to enable XSAVE.
The patch adds per-vcpu host/guest xstate image/mask and enhances the current FXSAVE/FRSTOR with the new XSAVE/XRSTOR on the host xstate (FPU/SSE/YMM) switch.
  5 files changed, 242 insertions(+), 18 deletions(-)
arch/x86/include/asm/kvm_host.h +0 -0
index 3f0007b..60be1a7 100644
@@ -303,6 +303,11 @@ struct kvm_vcpu_arch {
  	struct i387_fxsave_struct host_fx_image;
  	struct i387_fxsave_struct guest_fx_image;
 

Quoted 6 lines

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch {
Quoted 4 lines
arch/x86/include/asm/kvm_host.h +3 -0
+	struct xsave_struct *host_xstate_image;
+	struct xsave_struct *guest_xstate_image;
+	uint64_t host_xstate_mask;
 
Does host_xstate_mask need to be per-vcpu, or can it be global?

(I'm sorry for my late reply as I was on a long leave...) Yes, I think host_xstate_mask can be global because in Linux xsave_cntxt_init() and xsave_init() always enables all the XSTATEs on all CPUs (currently XCNTXT_MASK has 3 bits). In host, only the kernel can execute xsetbv() and applications can't change the XCR.I think this usage of Linux won't change in future.

Quoted 5 lines

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 311
Quoted 3 lines
arch/x86/include/asm/kvm_host.h +1 -0
+	uint64_t guest_xstate_mask;
 
Can be called xcr0, like other shadow registers.

OK.

Quoted 9 lines

Quoted 9 lines
arch/x86/include/asm/kvm_host.h +1 -0
+
  	gva_t mmio_fault_cr2;
struct kvm_pio_request pio;
void *pio_data;
arch/x86/include/asm/kvm_host.h +0 -0
@@ -3258,6 +3262,25 @@ static int handle_wbinvd(struct kvm_vcpu
  *vcpu)   	return 1; }
 

Quoted 7 lines

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu · line 3268
Quoted 5 lines
arch/x86/include/asm/kvm_host.h +4 -0
+static int handle_xsetbv(struct kvm_vcpu *vcpu)
+{
+	u64 new_bv = ((u64)kvm_register_read(vcpu, VCPU_REGS_RDX)) |
+		kvm_register_read(vcpu, VCPU_REGS_RAX);
 
Missing shift?

Oh, my carelessness! Thanks for pointing it out.

Quoted 3 lines

Probably worthwhile to create a helper for reading/writing edx:eax into
a u64.

OK.

Quoted 5 lines

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu · line 3270
Quoted 3 lines
arch/x86/include/asm/kvm_host.h +1 -0
+	u64 host_bv = vcpu->arch.host_xstate_mask;
 
What about ecx?

Sorry, I missed ecx.
Currently only ecx==0 is defined. When a guest tries a non-zero value of ecx, a #GP should be injected into the guest.

Quoted 5 lines

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu · line 3272
Quoted 3 lines
arch/x86/include/asm/kvm_host.h +2 -0
+
+	if (((new_bv ^ host_bv)&  ~host_bv)
Isn't (new_bv & ~host_bv) equivalent? (guest cannot exceed host...)

OK. Will use the simple one. :-)

Quoted 5 lines

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu · line 3274
Quoted 3 lines
arch/x86/include/asm/kvm_host.h +0 -0
  || !(new_bv&  1))
 
Symbolic value or comment.

OK. the "1" here should be "XSTATE_FP".

Quoted 6 lines

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu · line 3276
Quoted 4 lines
arch/x86/include/asm/kvm_host.h +2 -0
+		goto err;
+	if ((host_bv&  XSTATE_YMM&  new_bv)&&  !(new_bv&  XSTATE_SSE))
host_bv unneeded, I think.

Agree.

Quoted 8 lines

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu · line 3279
Quoted 5 lines
arch/x86/include/asm/kvm_host.h +3 -0
+		goto err;
+	vcpu->arch.guest_xstate_mask = new_bv;
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.guest_xstate_mask);
Can't we run with the host xcr0? isn't it guaranteed to be a superset of the guest xcr0?

I agree host xcr0 is a superset of guest xcr0.
In the case guest xcr0 has less bits set than host xcr0, I suppose running with guest xcr0 would be a bit faster. If you think simplying the code by removing the field guest_xstate_mask is more important, we can do it.

Quoted 7 lines

arch/x86/include/asm/kvm_host.h static int handle_wbinvd(struct kvm_vcpu · line 3283
Quoted 5 lines
arch/x86/include/asm/kvm_host.h +4 -0
+	skip_emulated_instruction(vcpu);
+	return 1;
+err:
+	kvm_inject_gp(vcpu, 0);
Need to #UD in some circumstances.

I don't think so. When the CPU doesn't suport XSAVE, or guest doesn't set guest CR4.OSXSAVE, guest's attempt of exectuing xsetbv would get a #UD immediately and no VMexit would occur.
BTW: I missed injecting #GP in the cases ECX !=0 or CPL !=0. Will add them.

Quoted 26 lines

Quoted 26 lines
arch/x86/include/asm/kvm_host.h +3 -0
+	return 1;
+}
+
  static int handle_apic_access(struct kvm_vcpu *vcpu)   {
  	unsigned long exit_qualification;
arch/x86/include/asm/kvm_host.h +0 -0
@@ -3556,6 +3579,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,
arch/x86/kvm/x86.c +2 -0
index 6b2ce1d..2af3fbe 100644
@@ -52,6 +52,8 @@
  #include<asm/desc.h>
  #include<asm/mtrr.h>
  #include<asm/mce.h>
+#include<asm/i387.h>
+#include<asm/xcr.h>
 

Quoted 11 lines

arch/x86/kvm/x86.c line 69

Quoted 9 lines

arch/x86/kvm/x86.c +1 -0

  #define MAX_IO_MSRS 256
  #define CR0_RESERVED_BITS						\

@@ -62,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	\
+			  | (cpu_has_xsave ? X86_CR4_OSXSAVE : 0)	\
  			  | X86_CR4_OSXMMEXCPT | X86_CR4_VMXE))

It also depends on the guest cpuid value. Please add it outside the

If cpu_has_xsave is true, we always present xsave to guest via cpuid, so I think the code is correct here.

Quoted 2 lines

macro, it's confusing to read something that looks like a constant but isn't.

Ok. Will try to make it outside the macro.

Quoted 56 lines

arch/x86/kvm/x86.c +50 -0

  int kvm_emulate_halt(struct kvm_vcpu *vcpu)

@@ -4307,6 +4346,65 @@ not_found:
  	return 36;
  }
 
+#define bitmaskof(idx)  (1U<<  ((idx)&  31))
+static void kvm_emulate_cpuid_fixup(struct kvm_vcpu *vcpu, u32
 func, u32 idx) +{ +	u32 eax, ebx, ecx, edx;
+
+	if (func != 0&&  func != 1&&  func != 0xd)
+		return;
+
+	eax = kvm_register_read(vcpu, VCPU_REGS_RAX);
+	ebx = kvm_register_read(vcpu, VCPU_REGS_RBX);
+	ecx = kvm_register_read(vcpu, VCPU_REGS_RCX);
+	edx = kvm_register_read(vcpu, VCPU_REGS_RDX);
+
+	switch (func) {
+	case 0:
+		/* fixup the Maximum Input Value */
+		if (cpu_has_xsave&&  eax<  0xd)
+			eax = 0xd;
+		break;
+	case 1:
+		ecx&= ~(bitmaskof(X86_FEATURE_XSAVE) |
+			bitmaskof(X86_FEATURE_OSXSAVE));
+		if (!cpu_has_xsave)
+			break;
+		ecx |= bitmaskof(X86_FEATURE_XSAVE);
+		if (kvm_read_cr4(vcpu)&  X86_CR4_OSXSAVE)
+			ecx |= bitmaskof(X86_FEATURE_OSXSAVE);
+		break;
+	case 0xd:
+		eax = ebx = ecx = edx = 0;
+		if (!cpu_has_xsave)
+			break;
+		switch (idx) {
+		case 0:
+			eax = vcpu->arch.host_xstate_mask&  XCNTXT_MASK;
+			/* FP/SSE + XSAVE.HEADER + YMM. */
+			ecx = 512 + 64;
+			if (eax&  XSTATE_YMM)
+				ecx += XSTATE_YMM_SIZE;
+			ebx = ecx;
+			break;
+		case 2:
+			if (!(vcpu->arch.host_xstate_mask&  XSTATE_YMM)) +				break;
+			eax = XSTATE_YMM_SIZE;
+			ebx = XSTATE_YMM_OFFSET;
+			break;
+		default:
+			break;
+		}
+		break;
+	}
+

Quoted 8 lines

arch/x86/kvm/x86.c not_found:
Quoted 6 lines
arch/x86/kvm/x86.c +5 -0
+	kvm_register_write(vcpu, VCPU_REGS_RAX, eax);
+	kvm_register_write(vcpu, VCPU_REGS_RBX, ebx);
+	kvm_register_write(vcpu, VCPU_REGS_RCX, ecx);
+	kvm_register_write(vcpu, VCPU_REGS_RDX, edx);
+}
 
This should be part of KVM_GET_SUPPORTED_CPUID.@@ -5091,6 +5192,60 @@

Ok. Will add this.

Quoted 44 lines

int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)

Quoted 42 lines

arch/x86/kvm/x86.c +10 -0

  	return 0;
  }
 
+#ifdef CONFIG_X86_64
+#define REX_PREFIX  "0x48, "
+#else
+#define REX_PREFIX
+#endif
+
+static inline void kvm_fx_save_host(struct kvm_vcpu *vcpu) +{
+	if (cpu_has_xsave) {
+		asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x27"
+		: : "a" (-1), "d" (-1), "D"(vcpu->arch.host_xstate_image) +		:

"memory"); + vcpu->arch.host_xstate_mask =

arch/x86/kvm/x86.c +9 -0

+			xgetbv(XCR_XFEATURE_ENABLED_MASK);
+	} else {
+		asm("fxsave (%0)" : : "r" (&vcpu->arch.host_fx_image)); +	}
+}
+
+static inline void kvm_fx_save_guest(struct kvm_vcpu *vcpu) +{
+	if (cpu_has_xsave) {
+		asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x27"
+		: : "a" (-1), "d" (-1), "D"(vcpu->arch.guest_xstate_image) +		:

"memory"); + vcpu->arch.guest_xstate_mask =

arch/x86/kvm/x86.c +10 -0

+			xgetbv(XCR_XFEATURE_ENABLED_MASK);
+	} else {
+		asm("fxsave (%0)" : : "r" (&vcpu->arch.guest_fx_image)); +	}
+}
+
+static inline void kvm_fx_restore_host(struct kvm_vcpu *vcpu) +{
+	if (cpu_has_xsave) {
+		xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.host_xstate_mask);
+		asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x2f"
+		: : "a" (-1), "d" (-1), "D"(vcpu->arch.host_xstate_image)); +	}

else { + asm("fxrstor (%0)" : : "r" (&vcpu->arch.host_fx_image));

arch/x86/kvm/x86.c +7 -0

+	} +}
+
+static inline void kvm_fx_restore_guest(struct kvm_vcpu *vcpu) +{
+	if (cpu_has_xsave) {
+		xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.guest_xstate_mask);
+		asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x2f"
+		: : "a" (-1), "d" (-1), "D"(vcpu->arch.guest_xstate_image)); +	}

Quoted 8 lines

arch/x86/kvm/x86.c
Quoted 4 lines
else { + asm("fxrstor (%0)" : : "r" (&vcpu->arch.guest_fx_image));
arch/x86/kvm/x86.c +2 -0
+	} +}
+
This mostly duplicates the standard x86 fpu code. I have a patch somewhere that abstracts it out, I'll dig it up and send it out.

I tried to avoid touching the host kernel itself. :-)
I saw the 2 patches you sent. They (like the new APIs fpu_alloc/fpu_free) will simplify the implementation of kvm xsave support. Thanks a lot!
BTW: Sheng Yang(in the Cc) will help me to follow up all the left issues.

Thanks,
-- Dexuan

Avi Kivity

2010-05-06 08:14 UTC #3 lore raw

Quoted 9 lines · Dexuan

Quoted 9 lines
arch/x86/kvm/vmx.c static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3276
Quoted 6 lines
arch/x86/kvm/vmx.c +3 -0
+		goto err;
+	vcpu->arch.guest_xstate_mask = new_bv;
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.guest_xstate_mask);
Can't we run with the host xcr0? isn't it guaranteed to be a superset of the guest xcr0?

Quoted 3 lines

I agree host xcr0 is a superset of guest xcr0.
In the case guest xcr0 has less bits set than host xcr0, I suppose running with guest xcr0 would be a bit faster.

However, switching xcr0 may be slow. That's our experience with msrs. Can you measure its latency?

Can also be avoided if guest and host xcr0 match.

Quoted 3 lines

If you think simplying the code by removing the field guest_xstate_mask is more important, we can do it.

Well we need guest_xstate_mask, it's the guest's xcr0, no?

btw, it needs save/restore for live migration, as well as save/restore for the new fpu state.

Quoted 8 lines

Quoted 8 lines
arch/x86/kvm/vmx.c static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3280
Quoted 6 lines
arch/x86/kvm/vmx.c +4 -0
+	skip_emulated_instruction(vcpu);
+	return 1;
+err:
+	kvm_inject_gp(vcpu, 0);
Need to #UD in some circumstances.

Quoted 4 lines

I don't think so. When the CPU doesn't suport XSAVE, or guest doesn't set guest CR4.OSXSAVE, guest's attempt of exectuing xsetbv would get a #UD immediately and no VMexit would occur.

Ok.

Quoted 10 lines

arch/x86/kvm/x86.c line 70

Quoted 8 lines

arch/x86/kvm/x86.c +1 -0

@@ -62,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	\
+			  | (cpu_has_xsave ? X86_CR4_OSXSAVE : 0)	\
   			  | X86_CR4_OSXMMEXCPT | X86_CR4_VMXE))

It also depends on the guest cpuid value. Please add it outside the

Quoted 3 lines

If cpu_has_xsave is true, we always present xsave to guest via cpuid, so I think the code is correct here.

We don't pass all host cpuid values to the guest. We expose them to userspace via KVM_GET_SUPPORTED_CPUID, and then userspace decides what cpuid to present to the guest via KVM_SET_CPUID2. So the guest may run with fewer features than the host.

Quoted 3 lines

I saw the 2 patches you sent. They (like the new APIs fpu_alloc/fpu_free) will simplify the implementation of kvm xsave support. Thanks a lot!

Thanks. To simplify things, please separate host xsave support (switching to the fpu api) and guest xsave support (cpuid, xsetbv, new ioctls) into separate patches. In fact, guest xsave support is probably best split into patches as well.

Signature · 1 line

error compiling committee.c: too many arguments to function

Cui, Dexuan

author

2010-05-06 14:20 UTC #4 lore raw

Quoted 10 lines · Avi Kivity

Quoted 9 lines · Dexuan
Quoted 9 lines
arch/x86/kvm/vmx.c static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3276
Quoted 6 lines
arch/x86/kvm/vmx.c +3 -0
+		goto err;
+	vcpu->arch.guest_xstate_mask = new_bv;
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.guest_xstate_mask);
Can't we run with the host xcr0? isn't it guaranteed to be a superset of the guest xcr0?

Quoted 6 lines

Quoted 3 lines
I agree host xcr0 is a superset of guest xcr0.
In the case guest xcr0 has less bits set than host xcr0, I suppose running with guest xcr0 would be a bit faster.
However, switching xcr0 may be slow. That's our experience with msrs. Can you measure its latency?

We can measure that.
However, I think the changing xcr0 to guest xcr0 in handle_xsetbv() is necessary -- or else, inside guest xgetbv() would return host xcr0 rather than guest xcr0 -- this is obviously incorrect. Once handle_xsetbv() changes the xcr0 to guest's value, the xsetbv() in kvm_fx_restore_host() is also necessary, and the xsetbv() in kvm_fx_restore_guest() is also necessary. So looks guest can't run with the host xcr0.

Quoted 7 lines

Can also be avoided if guest and host xcr0 match.
Quoted 3 lines
If you think simplying the code by removing the field guest_xstate_mask is more important, we can do it.
Well we need guest_xstate_mask, it's the guest's xcr0, no?

I misread it. Yes, we need geust_xsave_mask.

Quoted 3 lines

btw, it needs save/restore for live migration, as well as save/restore for the new fpu state.

Yes. This part is missing. Sheng and I is also doing this -- it may be a bittle troublesome as the number of XSTATEs can grown as time goes on. We'll have to handle the compatibility issue.

Quoted 32 lines

Quoted 9 lines
Quoted 9 lines
arch/x86/kvm/vmx.c static int handle_wbinvd(struct kvm_vcpu *vcpu) · line 3280
Quoted 7 lines
arch/x86/kvm/vmx.c +4 -0
+	skip_emulated_instruction(vcpu);
+	return 1;
+err:
+	kvm_inject_gp(vcpu, 0);
Need to #UD in some circumstances.
Quoted 5 lines
I don't think so. When the CPU doesn't suport XSAVE, or guest doesn't set guest CR4.OSXSAVE, guest's attempt of exectuing xsetbv would get a #UD immediately
and no VMexit would occur.
Ok.
Quoted 10 lines
Quoted 10 lines
arch/x86/kvm/x86.c line 70
Quoted 8 lines
arch/x86/kvm/x86.c +1 -0
@@ -62,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	\
+			  | (cpu_has_xsave ? X86_CR4_OSXSAVE : 0)	\
   			  | X86_CR4_OSXMMEXCPT | X86_CR4_VMXE))
 
 
It also depends on the guest cpuid value. Please add it outside the
Quoted 4 lines
If cpu_has_xsave is true, we always present xsave to guest via cpuid, so I
think the code is correct here.

Quoted 4 lines

We don't pass all host cpuid values to the guest. We expose them to userspace via KVM_GET_SUPPORTED_CPUID, and then userspace decides what cpuid to present to the guest via KVM_SET_CPUID2. So the guest may run with fewer features than the host.

Sorry, I didn't notice this. Will look into this.

Quoted 10 lines

Quoted 5 lines
I saw the 2 patches you sent. They (like the new APIs fpu_alloc/fpu_free) will simplify the implementation of kvm xsave support. Thanks a lot!
Thanks. To simplify things, please separate host xsave support (switching to the fpu api) and guest xsave support (cpuid, xsetbv, new ioctls) into separate patches. In fact, guest xsave support is probably best split into patches as well.

Ok. Will try to do these.

Thanks,
-- Dexuan

Avi Kivity

2010-05-06 19:45 UTC #5 lore raw

Quoted 4 lines · Dexuan

Quoted 4 lines
However, switching xcr0 may be slow. That's our experience with msrs. Can you measure its latency?

Quoted 8 lines

We can measure that.
However, I think the changing xcr0 to guest xcr0 in handle_xsetbv() is necessary -- or else, inside guest xgetbv() would return host xcr0 rather than guest xcr0 -- this is obviously incorrect. Once handle_xsetbv() changes the xcr0 to guest's value, the xsetbv() in kvm_fx_restore_host() is also necessary, and the xsetbv() in kvm_fx_restore_guest() is also necessary. So looks guest can't run with the host xcr0.

Right. Moreover, xsave would write into memory the guest doesn't expect.

Quoted 7 lines

Quoted 3 lines
btw, it needs save/restore for live migration, as well as save/restore for the new fpu state.
Yes. This part is missing. Sheng and I is also doing this -- it may be a bittle troublesome as the number of XSTATEs can grown as time goes on. We'll have to handle the compatibility issue.

Reserve tons of space in the ioctl - and we can use the same format as xsave.

All those control registers are annoying, we have cr1 and cr5-cr7 free, cr9-cr15 on x86_64, infinite msr space, and now we have XCRs. Great.

Looking forward to YCR0.

Signature · 1 line

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

Avi Kivity

2010-05-06 19:49 UTC #6 lore raw

Quoted 5 lines · Avi Kivity

All those control registers are annoying, we have cr1 and cr5-cr7 free, cr9-cr15 on x86_64, infinite msr space, and now we have XCRs. Great.
Looking forward to YCR0.

I think I see the reason - xgetbv is unprivileged, so applications can see what the OS supports instead of looking at cpuid and hoping.

Signature · 1 line

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