[PATCH 0/4] cr4 optimizations for vmx/ept

[PATCH 0/4] cr4 optimizations for vmx/ept

[Avi Kivity]

When ept is enabled, we aren't particularly interested in cr4.pge, so allow
the guest to own it.  This improves performance in vmap() intensive loads.

Avi Kivity (4):
  KVM: VMX: Move some cr[04] related constants to vmx.c
  KVM: Add accessor for reading cr4 (or some bits of cr4)
  KVM: VMX: Make guest cr4 mask more conservative
  KVM: VMX: When using ept, allow the guest to own cr4.pge

 arch/x86/include/asm/kvm_host.h |   14 +-------------
 arch/x86/kvm/kvm_cache_regs.h   |   12 ++++++++++++
 arch/x86/kvm/mmu.h              |    5 +++--
 arch/x86/kvm/vmx.c              |   32 ++++++++++++++++++++++++++------
 arch/x86/kvm/x86.c              |   16 ++++++----------
 5 files changed, 48 insertions(+), 31 deletions(-)

[PATCH 1/4] KVM: VMX: Move some cr[04] related constants to vmx.c

[Avi Kivity]

They have no place in common code.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |   13 -------------
 arch/x86/kvm/vmx.c              |   13 +++++++++++++
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 4f865e8..da6dee8 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -38,19 +38,6 @@
 #define CR3_L_MODE_RESERVED_BITS (CR3_NONPAE_RESERVED_BITS |	\
 				  0xFFFFFF0000000000ULL)
 
-#define KVM_GUEST_CR0_MASK_UNRESTRICTED_GUEST				\
-	(X86_CR0_WP | X86_CR0_NE | X86_CR0_NW | X86_CR0_CD)
-#define KVM_GUEST_CR0_MASK						\
-	(KVM_GUEST_CR0_MASK_UNRESTRICTED_GUEST | X86_CR0_PG | X86_CR0_PE)
-#define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST				\
-	(X86_CR0_WP | X86_CR0_NE | X86_CR0_TS | X86_CR0_MP)
-#define KVM_VM_CR0_ALWAYS_ON						\
-	(KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST | X86_CR0_PG | X86_CR0_PE)
-#define KVM_GUEST_CR4_MASK						\
-	(X86_CR4_VME | X86_CR4_PSE | X86_CR4_PAE | X86_CR4_PGE | X86_CR4_VMXE)
-#define KVM_PMODE_VM_CR4_ALWAYS_ON (X86_CR4_PAE | X86_CR4_VMXE)
-#define KVM_RMODE_VM_CR4_ALWAYS_ON (X86_CR4_VME | X86_CR4_PAE | X86_CR4_VMXE)
-
 #define INVALID_PAGE (~(hpa_t)0)
 #define UNMAPPED_GVA (~(gpa_t)0)
 
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 9a0a2cf..5ef820e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -61,6 +61,19 @@ module_param_named(unrestricted_guest,
 static int __read_mostly emulate_invalid_guest_state = 0;
 module_param(emulate_invalid_guest_state, bool, S_IRUGO);
 
+#define KVM_GUEST_CR0_MASK_UNRESTRICTED_GUEST				\
+	(X86_CR0_WP | X86_CR0_NE | X86_CR0_NW | X86_CR0_CD)
+#define KVM_GUEST_CR0_MASK						\
+	(KVM_GUEST_CR0_MASK_UNRESTRICTED_GUEST | X86_CR0_PG | X86_CR0_PE)
+#define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST				\
+	(X86_CR0_WP | X86_CR0_NE | X86_CR0_TS | X86_CR0_MP)
+#define KVM_VM_CR0_ALWAYS_ON						\
+	(KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST | X86_CR0_PG | X86_CR0_PE)
+#define KVM_GUEST_CR4_MASK						\
+	(X86_CR4_VME | X86_CR4_PSE | X86_CR4_PAE | X86_CR4_PGE | X86_CR4_VMXE)
+#define KVM_PMODE_VM_CR4_ALWAYS_ON (X86_CR4_PAE | X86_CR4_VMXE)
+#define KVM_RMODE_VM_CR4_ALWAYS_ON (X86_CR4_VME | X86_CR4_PAE | X86_CR4_VMXE)
+
 /*
  * These 2 parameters are used to config the controls for Pause-Loop Exiting:
  * ple_gap:    upper bound on the amount of time between two successive
-- 
1.6.5.3

[PATCH 2/4] KVM: Add accessor for reading cr4 (or some bits of cr4)

[Avi Kivity]

Some bits of cr4 can be owned by the guest on vmx, so when we read them,
we copy them to the vcpu structure.  In preparation for making the set of
guest-owned bits dynamic, use helpers to access these bits so we don't need
to know where the bit resides.

No changes to svm since all bits are host-owned there.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/kvm_cache_regs.h   |   12 ++++++++++++
 arch/x86/kvm/mmu.h              |    5 +++--
 arch/x86/kvm/vmx.c              |   13 ++++++++-----
 arch/x86/kvm/x86.c              |   16 ++++++----------
 5 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index da6dee8..e9f4f12 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -272,6 +272,7 @@ struct kvm_vcpu_arch {
 	unsigned long cr2;
 	unsigned long cr3;
 	unsigned long cr4;
+	unsigned long cr4_guest_owned_bits;
 	unsigned long cr8;
 	u32 hflags;
 	u64 pdptrs[4]; /* pae */
diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index 7bcc5b6..35acc36 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -38,4 +38,16 @@ static inline u64 kvm_pdptr_read(struct kvm_vcpu *vcpu, int index)
 	return vcpu->arch.pdptrs[index];
 }
 
+static inline ulong kvm_read_cr4_bits(struct kvm_vcpu *vcpu, ulong mask)
+{
+	if (mask & vcpu->arch.cr4_guest_owned_bits)
+		kvm_x86_ops->decache_cr4_guest_bits(vcpu);
+	return vcpu->arch.cr4 & mask;
+}
+
+static inline ulong kvm_read_cr4(struct kvm_vcpu *vcpu)
+{
+	return kvm_read_cr4_bits(vcpu, ~0UL);
+}
+
 #endif
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 61a1b38..4567d80 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -2,6 +2,7 @@
 #define __KVM_X86_MMU_H
 
 #include <linux/kvm_host.h>
+#include "kvm_cache_regs.h"
 
 #define PT64_PT_BITS 9
 #define PT64_ENT_PER_PAGE (1 << PT64_PT_BITS)
@@ -64,12 +65,12 @@ static inline int is_long_mode(struct kvm_vcpu *vcpu)
 
 static inline int is_pae(struct kvm_vcpu *vcpu)
 {
-	return vcpu->arch.cr4 & X86_CR4_PAE;
+	return kvm_read_cr4_bits(vcpu, X86_CR4_PAE);
 }
 
 static inline int is_pse(struct kvm_vcpu *vcpu)
 {
-	return vcpu->arch.cr4 & X86_CR4_PSE;
+	return kvm_read_cr4_bits(vcpu, X86_CR4_PSE);
 }
 
 static inline int is_paging(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 5ef820e..ae95a0c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1612,8 +1612,10 @@ static void vmx_flush_tlb(struct kvm_vcpu *vcpu)
 
 static void vmx_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.cr4 &= KVM_GUEST_CR4_MASK;
-	vcpu->arch.cr4 |= vmcs_readl(GUEST_CR4) & ~KVM_GUEST_CR4_MASK;
+	ulong cr4_guest_owned_bits = vcpu->arch.cr4_guest_owned_bits;
+
+	vcpu->arch.cr4 &= ~cr4_guest_owned_bits;
+	vcpu->arch.cr4 |= vmcs_readl(GUEST_CR4) & cr4_guest_owned_bits;
 }
 
 static void ept_load_pdptrs(struct kvm_vcpu *vcpu)
@@ -1658,7 +1660,7 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
 			     (CPU_BASED_CR3_LOAD_EXITING |
 			      CPU_BASED_CR3_STORE_EXITING));
 		vcpu->arch.cr0 = cr0;
-		vmx_set_cr4(vcpu, vcpu->arch.cr4);
+		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
 	} else if (!is_paging(vcpu)) {
 		/* From nonpaging to paging */
 		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
@@ -1666,7 +1668,7 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
 			     ~(CPU_BASED_CR3_LOAD_EXITING |
 			       CPU_BASED_CR3_STORE_EXITING));
 		vcpu->arch.cr0 = cr0;
-		vmx_set_cr4(vcpu, vcpu->arch.cr4);
+		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
 	}
 
 	if (!(cr0 & X86_CR0_WP))
@@ -2417,6 +2419,7 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
 
 	vmcs_writel(CR0_GUEST_HOST_MASK, ~0UL);
 	vmcs_writel(CR4_GUEST_HOST_MASK, KVM_GUEST_CR4_MASK);
+	vmx->vcpu.arch.cr4_guest_owned_bits = KVM_GUEST_CR4_MASK;
 
 	tsc_base = vmx->vcpu.kvm->arch.vm_init_tsc;
 	rdtscll(tsc_this);
@@ -3047,7 +3050,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 				vcpu->arch.eff_db[dr] = val;
 			break;
 		case 4 ... 5:
-			if (vcpu->arch.cr4 & X86_CR4_DE)
+			if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
 				kvm_queue_exception(vcpu, UD_VECTOR);
 			break;
 		case 6:
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index dd15d7a..4a16337 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -481,7 +481,7 @@ EXPORT_SYMBOL_GPL(kvm_lmsw);
 
 void kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 {
-	unsigned long old_cr4 = vcpu->arch.cr4;
+	unsigned long old_cr4 = kvm_read_cr4(vcpu);
 	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE | X86_CR4_PAE;
 
 	if (cr4 & CR4_RESERVED_BITS) {
@@ -1899,7 +1899,7 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
 		return 0;
 	if (mce->status & MCI_STATUS_UC) {
 		if ((vcpu->arch.mcg_status & MCG_STATUS_MCIP) ||
-		    !(vcpu->arch.cr4 & X86_CR4_MCE)) {
+		    !kvm_read_cr4_bits(vcpu, X86_CR4_MCE)) {
 			printk(KERN_DEBUG "kvm: set_mce: "
 			       "injects mce exception while "
 			       "previous one is in progress!\n");
@@ -3612,7 +3612,6 @@ unsigned long realmode_get_cr(struct kvm_vcpu *vcpu, int cr)
 {
 	unsigned long value;
 
-	kvm_x86_ops->decache_cr4_guest_bits(vcpu);
 	switch (cr) {
 	case 0:
 		value = vcpu->arch.cr0;
@@ -3624,7 +3623,7 @@ unsigned long realmode_get_cr(struct kvm_vcpu *vcpu, int cr)
 		value = vcpu->arch.cr3;
 		break;
 	case 4:
-		value = vcpu->arch.cr4;
+		value = kvm_read_cr4(vcpu);
 		break;
 	case 8:
 		value = kvm_get_cr8(vcpu);
@@ -3652,7 +3651,7 @@ void realmode_set_cr(struct kvm_vcpu *vcpu, int cr, unsigned long val,
 		kvm_set_cr3(vcpu, val);
 		break;
 	case 4:
-		kvm_set_cr4(vcpu, mk_cr_64(vcpu->arch.cr4, val));
+		kvm_set_cr4(vcpu, mk_cr_64(kvm_read_cr4(vcpu), val));
 		break;
 	case 8:
 		kvm_set_cr8(vcpu, val & 0xfUL);
@@ -4232,11 +4231,10 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 	sregs->gdt.limit = dt.limit;
 	sregs->gdt.base = dt.base;
 
-	kvm_x86_ops->decache_cr4_guest_bits(vcpu);
 	sregs->cr0 = vcpu->arch.cr0;
 	sregs->cr2 = vcpu->arch.cr2;
 	sregs->cr3 = vcpu->arch.cr3;
-	sregs->cr4 = vcpu->arch.cr4;
+	sregs->cr4 = kvm_read_cr4(vcpu);
 	sregs->cr8 = kvm_get_cr8(vcpu);
 	sregs->efer = vcpu->arch.shadow_efer;
 	sregs->apic_base = kvm_get_apic_base(vcpu);
@@ -4732,13 +4730,11 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	kvm_x86_ops->set_efer(vcpu, sregs->efer);
 	kvm_set_apic_base(vcpu, sregs->apic_base);
 
-	kvm_x86_ops->decache_cr4_guest_bits(vcpu);
-
 	mmu_reset_needed |= vcpu->arch.cr0 != sregs->cr0;
 	kvm_x86_ops->set_cr0(vcpu, sregs->cr0);
 	vcpu->arch.cr0 = sregs->cr0;
 
-	mmu_reset_needed |= vcpu->arch.cr4 != sregs->cr4;
+	mmu_reset_needed |= kvm_read_cr4(vcpu) != sregs->cr4;
 	kvm_x86_ops->set_cr4(vcpu, sregs->cr4);
 	if (!is_long_mode(vcpu) && is_pae(vcpu)) {
 		load_pdptrs(vcpu, vcpu->arch.cr3);
-- 
1.6.5.3

[PATCH 3/4] KVM: VMX: Make guest cr4 mask more conservative

[Avi Kivity]

Instead of specifying the bits which we want to trap on, specify the bits
which we allow the guest to change transparently.  This is safer wrt future
changes to cr4.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/vmx.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ae95a0c..d34fdd3 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -69,8 +69,10 @@ module_param(emulate_invalid_guest_state, bool, S_IRUGO);
 	(X86_CR0_WP | X86_CR0_NE | X86_CR0_TS | X86_CR0_MP)
 #define KVM_VM_CR0_ALWAYS_ON						\
 	(KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST | X86_CR0_PG | X86_CR0_PE)
-#define KVM_GUEST_CR4_MASK						\
-	(X86_CR4_VME | X86_CR4_PSE | X86_CR4_PAE | X86_CR4_PGE | X86_CR4_VMXE)
+#define KVM_CR4_GUEST_OWNED_BITS				      \
+	(X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR      \
+	 | X86_CR4_OSXMMEXCPT)
+
 #define KVM_PMODE_VM_CR4_ALWAYS_ON (X86_CR4_PAE | X86_CR4_VMXE)
 #define KVM_RMODE_VM_CR4_ALWAYS_ON (X86_CR4_VME | X86_CR4_PAE | X86_CR4_VMXE)
 
@@ -2418,8 +2420,8 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
 	vmcs_write32(VM_ENTRY_CONTROLS, vmcs_config.vmentry_ctrl);
 
 	vmcs_writel(CR0_GUEST_HOST_MASK, ~0UL);
-	vmcs_writel(CR4_GUEST_HOST_MASK, KVM_GUEST_CR4_MASK);
-	vmx->vcpu.arch.cr4_guest_owned_bits = KVM_GUEST_CR4_MASK;
+	vmx->vcpu.arch.cr4_guest_owned_bits = KVM_CR4_GUEST_OWNED_BITS;
+	vmcs_writel(CR4_GUEST_HOST_MASK, ~vmx->vcpu.arch.cr4_guest_owned_bits);
 
 	tsc_base = vmx->vcpu.kvm->arch.vm_init_tsc;
 	rdtscll(tsc_this);
-- 
1.6.5.3

[PATCH 4/4] KVM: VMX: When using ept, allow the guest to own cr4.pge

[Avi Kivity]

We make no use of cr4.pge if ept is enabled, but the guest does (to flush
global mappings, as with vmap()), so give the guest ownership of this bit.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/vmx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index d34fdd3..2e47e65 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2421,6 +2421,8 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
 
 	vmcs_writel(CR0_GUEST_HOST_MASK, ~0UL);
 	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;
 	vmcs_writel(CR4_GUEST_HOST_MASK, ~vmx->vcpu.arch.cr4_guest_owned_bits);
 
 	tsc_base = vmx->vcpu.kvm->arch.vm_init_tsc;
-- 
1.6.5.3

Re: [PATCH 2/4] KVM: Add accessor for reading cr4 (or some bits of cr4)

[Sheng Yang]

On Monday 07 December 2009 18:47:10 Avi Kivity wrote:
> Some bits of cr4 can be owned by the guest on vmx, so when we read them,
> we copy them to the vcpu structure.  In preparation for making the set of
> guest-owned bits dynamic, use helpers to access these bits so we don't need
> to know where the bit resides.
> 
> No changes to svm since all bits are host-owned there.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |    1 +
>  arch/x86/kvm/kvm_cache_regs.h   |   12 ++++++++++++
>  arch/x86/kvm/mmu.h              |    5 +++--
>  arch/x86/kvm/vmx.c              |   13 ++++++++-----
>  arch/x86/kvm/x86.c              |   16 ++++++----------
>  5 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h
>  b/arch/x86/include/asm/kvm_host.h index da6dee8..e9f4f12 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -272,6 +272,7 @@ struct kvm_vcpu_arch {
>  	unsigned long cr2;
>  	unsigned long cr3;
>  	unsigned long cr4;
> +	unsigned long cr4_guest_owned_bits;
>  	unsigned long cr8;
>  	u32 hflags;
>  	u64 pdptrs[4]; /* pae */
> diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
> index 7bcc5b6..35acc36 100644
> --- a/arch/x86/kvm/kvm_cache_regs.h
> +++ b/arch/x86/kvm/kvm_cache_regs.h
> @@ -38,4 +38,16 @@ static inline u64 kvm_pdptr_read(struct kvm_vcpu *vcpu,
>  int index) return vcpu->arch.pdptrs[index];
>  }
> 
> +static inline ulong kvm_read_cr4_bits(struct kvm_vcpu *vcpu, ulong mask)
> +{
> +	if (mask & vcpu->arch.cr4_guest_owned_bits)
> +		kvm_x86_ops->decache_cr4_guest_bits(vcpu);
> +	return vcpu->arch.cr4 & mask;
> +}
> +
> +static inline ulong kvm_read_cr4(struct kvm_vcpu *vcpu)
> +{
> +	return kvm_read_cr4_bits(vcpu, ~0UL);
> +}
> +
>  #endif
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 61a1b38..4567d80 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -2,6 +2,7 @@
>  #define __KVM_X86_MMU_H
> 
>  #include <linux/kvm_host.h>
> +#include "kvm_cache_regs.h"
> 
>  #define PT64_PT_BITS 9
>  #define PT64_ENT_PER_PAGE (1 << PT64_PT_BITS)
> @@ -64,12 +65,12 @@ static inline int is_long_mode(struct kvm_vcpu *vcpu)
> 
>  static inline int is_pae(struct kvm_vcpu *vcpu)
>  {
> -	return vcpu->arch.cr4 & X86_CR4_PAE;
> +	return kvm_read_cr4_bits(vcpu, X86_CR4_PAE);
>  }
> 
>  static inline int is_pse(struct kvm_vcpu *vcpu)
>  {
> -	return vcpu->arch.cr4 & X86_CR4_PSE;
> +	return kvm_read_cr4_bits(vcpu, X86_CR4_PSE);
>  }
> 
>  static inline int is_paging(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 5ef820e..ae95a0c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1612,8 +1612,10 @@ static void vmx_flush_tlb(struct kvm_vcpu *vcpu)
> 
>  static void vmx_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
>  {
> -	vcpu->arch.cr4 &= KVM_GUEST_CR4_MASK;
> -	vcpu->arch.cr4 |= vmcs_readl(GUEST_CR4) & ~KVM_GUEST_CR4_MASK;
> +	ulong cr4_guest_owned_bits = vcpu->arch.cr4_guest_owned_bits;
> +
> +	vcpu->arch.cr4 &= ~cr4_guest_owned_bits;
> +	vcpu->arch.cr4 |= vmcs_readl(GUEST_CR4) & cr4_guest_owned_bits;
>  }
> 
>  static void ept_load_pdptrs(struct kvm_vcpu *vcpu)
> @@ -1658,7 +1660,7 @@ static void ept_update_paging_mode_cr0(unsigned long
>  *hw_cr0, (CPU_BASED_CR3_LOAD_EXITING |
>  			      CPU_BASED_CR3_STORE_EXITING));
>  		vcpu->arch.cr0 = cr0;
> -		vmx_set_cr4(vcpu, vcpu->arch.cr4);
> +		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
>  	} else if (!is_paging(vcpu)) {
>  		/* From nonpaging to paging */
>  		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
> @@ -1666,7 +1668,7 @@ static void ept_update_paging_mode_cr0(unsigned long
>  *hw_cr0, ~(CPU_BASED_CR3_LOAD_EXITING |
>  			       CPU_BASED_CR3_STORE_EXITING));
>  		vcpu->arch.cr0 = cr0;
> -		vmx_set_cr4(vcpu, vcpu->arch.cr4);
> +		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
>  	}

Another place accessed cr4 directly, in ept_update_paging_mode_cr4()

>        } else if (!(vcpu->arch.cr4 & X86_CR4_PAE))                    
>                *hw_cr4 &= ~X86_CR4_PAE;                               

Others looks fine to me.

-- 
regards
Yang, Sheng

Re: [PATCH 2/4] KVM: Add accessor for reading cr4 (or some bits of cr4)

[Avi Kivity]

On 12/08/2009 09:57 AM, Sheng Yang wrote:
> vcpu->arch.cr0 = cr0;
>> -		vmx_set_cr4(vcpu, vcpu->arch.cr4);
>> +		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
>>   	}
>>      
> Another place accessed cr4 directly, in ept_update_paging_mode_cr4()
>
>    

That one is called from vmx_set_cr4(); at that time CR4_READ_SHADOW is 
not up-to-date and vmx_decache_cr4_guest_bits() will actually corrupt 
vcpu->arch.cr4 (except it won't be called, since cr4.pae is never guest 
owned).

But you are right, I should have placed a comment.  I'll add a patch 
that inlines ept_update_paging_mode_cr4 into its caller so it can access 
the cr4 parameter directly instead of vcpu->arch.cr4.

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

Re: [PATCH 2/4] KVM: Add accessor for reading cr4 (or some bits of cr4)

[Sheng Yang]

On Tue, Dec 08, 2009 at 11:36:59AM +0200, Avi Kivity wrote:
> On 12/08/2009 09:57 AM, Sheng Yang wrote:
> >vcpu->arch.cr0 = cr0;
> >>-		vmx_set_cr4(vcpu, vcpu->arch.cr4);
> >>+		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
> >>  	}
> >Another place accessed cr4 directly, in ept_update_paging_mode_cr4()
> >
> 
> That one is called from vmx_set_cr4(); at that time CR4_READ_SHADOW
> is not up-to-date and vmx_decache_cr4_guest_bits() will actually
> corrupt vcpu->arch.cr4 (except it won't be called, since cr4.pae is
> never guest owned).

Yes, you are right...
> 
> But you are right, I should have placed a comment.  I'll add a patch
> that inlines ept_update_paging_mode_cr4 into its caller so it can
> access the cr4 parameter directly instead of vcpu->arch.cr4.

Just notice another thing, seems the cr4_guest_owned_bits' initial value is
wrong. It should be ~KVM_GUEST_CR4_MASK rather than KVM_GUEST_CR4_MASK in this
patch IIUC.

-- 
regards
Yang, Sheng

Re: [PATCH 2/4] KVM: Add accessor for reading cr4 (or some bits of cr4)

[Avi Kivity]

On 12/08/2009 04:40 PM, Sheng Yang wrote:
>
>> But you are right, I should have placed a comment.  I'll add a patch
>> that inlines ept_update_paging_mode_cr4 into its caller so it can
>> access the cr4 parameter directly instead of vcpu->arch.cr4.
>>      
> Just notice another thing, seems the cr4_guest_owned_bits' initial value is
> wrong. It should be ~KVM_GUEST_CR4_MASK rather than KVM_GUEST_CR4_MASK in this
> patch IIUC.
>    

Yes.  Will fix.

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

Re: [PATCH 0/4] cr4 optimizations for vmx/ept

[Avi Kivity]

On 12/07/2009 12:47 PM, Avi Kivity wrote:
> When ept is enabled, we aren't particularly interested in cr4.pge, so allow
> the guest to own it.  This improves performance in vmap() intensive loads.
>    

This is now applied, after modifying according to review comments.

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