KVM: x86: Check LMA bit before set_efer

PATCH Superseded Sheng Yang 3 participants 16 messages May 11, 2010 – May 12, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Sheng Yang

author

2010-05-11 05:30 UTC #0 lore raw

PATCH 1/4 KVM: x86: Check LMA bit before set_efer

kvm_x86_ops->set_efer() would execute vcpu->arch.efer = efer, so the checking of LMA bit didn't work.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/x86.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

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

index cd8a606..2cae460 100644
@@ -715,11 +715,11 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 			return 1;
 	}
 
-	kvm_x86_ops->set_efer(vcpu, efer);
-
 	efer &= ~EFER_LMA;
 	efer |= vcpu->arch.efer & EFER_LMA;
 
+	kvm_x86_ops->set_efer(vcpu, efer);
+
 	vcpu->arch.efer = efer;
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;

Signature · 1 line

1.7.0.1

Sheng Yang

author

2010-05-11 05:30 UTC #1 lore raw

PATCH 2/4 KVM: Clean up duplicate assignment

mmu.free() already set root_hpa to INVALID_PAGE, no need to do it again in the destory_kvm_mmu().

kvm_x86_ops->set_cr4() and set_efer() already assign cr4/efer to vcpu->arch.cr4/efer, no need to do it again later.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/mmu.c |    5 ++---
 arch/x86/kvm/x86.c |    4 +---
 2 files changed, 3 insertions(+), 6 deletions(-)

arch/x86/kvm/mmu.c +2 -3

index 95bee9d..5412185 100644
@@ -2450,10 +2450,9 @@ static int init_kvm_mmu(struct kvm_vcpu *vcpu)
 static void destroy_kvm_mmu(struct kvm_vcpu *vcpu)
 {
 	ASSERT(vcpu);
-	if (VALID_PAGE(vcpu->arch.mmu.root_hpa)) {
+	if (VALID_PAGE(vcpu->arch.mmu.root_hpa))
+		/* mmu.free() should set root_hpa = INVALID_PAGE */
 		vcpu->arch.mmu.free(vcpu);
-		vcpu->arch.mmu.root_hpa = INVALID_PAGE;
-	}
 }
 
 int kvm_mmu_reset_context(struct kvm_vcpu *vcpu)

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

index 2cae460..764f89b 100644
@@ -486,7 +486,7 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 		return 1;
 
 	kvm_x86_ops->set_cr4(vcpu, cr4);
-	vcpu->arch.cr4 = cr4;
+
 	kvm_mmu_reset_context(vcpu);
 
 	return 0;

@@ -720,8 +720,6 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	kvm_x86_ops->set_efer(vcpu, efer);
 
-	vcpu->arch.efer = efer;
-
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
 	kvm_mmu_reset_context(vcpu);

Signature · 1 line

1.7.0.1

Sheng Yang

author

2010-05-11 05:30 UTC #2 lore raw

PATCH 3/4 KVM: x86: Remove kvm_mmu_reset_context() in kvm_set_efer()

In reply to Sheng Yang

Modify EFER won't result in mode switch directly. After EFER.LME set, the following set CR0.PG would result in mode switch to IA32e. And the later action already covered by kvm_set_cr0().

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/x86.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

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

index 764f89b..b59fc67 100644
@@ -721,7 +721,6 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	kvm_x86_ops->set_efer(vcpu, efer);
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
-	kvm_mmu_reset_context(vcpu);
 
 	return 0;
 }

Signature · 1 line

1.7.0.1

Marcelo Tosatti

2010-05-11 19:36 UTC #3 lore raw

Re: [PATCH 3/4] KVM: x86: Remove kvm_mmu_reset_context() in kvm_set_efer()

Quoted 14 lines · Sheng Yang

Modify EFER won't result in mode switch directly. After EFER.LME set, the following set CR0.PG would result in mode switch to IA32e. And the later action already covered by kvm_set_cr0().
Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/kvm/x86.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +0 -0
index 764f89b..b59fc67 100644

arch/x86/kvm/x86.c static int set_efer(struct kvm_vcpu *vcpu, u64 efer) · line 724

Quoted 4 lines

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

@@ -721,7 +721,6 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	kvm_x86_ops->set_efer(vcpu, efer);
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
-	kvm_mmu_reset_context(vcpu);

But there are different sets of shadow pagetables for NXE on/off. See commit 9645bb56b31a1b.

Without the reset, after NXE 1->0 transition, a spte retains the NXE validity check, and subsequent use of such gpte with bit 63 set does not cause a fault.

Sheng Yang

author

2010-05-11 05:30 UTC #4 lore raw

PATCH 4/4 VMX: x86: Only reset MMU when necessary

In reply to Sheng Yang

Only modifying some bits of CR0/CR4 needs paging mode switch.

Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/mmu.c              |   17 ++++++++++++++---
 arch/x86/kvm/x86.c              |   14 ++++++++++++--
 3 files changed, 27 insertions(+), 5 deletions(-)

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

index ed48904..c8c8a03 100644
@@ -553,6 +553,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot);
 void kvm_mmu_zap_all(struct kvm *kvm);
 unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
 void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu);
 
 int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);

arch/x86/kvm/mmu.c +14 -3

index 5412185..98abdcf 100644
@@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
 	}
 }
 
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
+{
+	if (!is_paging(vcpu))
+		return;
+	if (is_long_mode(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
+	else if (is_pae(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
+	else
+		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
+}
+EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
+
 static int paging64_init_context_common(struct kvm_vcpu *vcpu, int level)
 {
 	struct kvm_mmu *context = &vcpu->arch.mmu;

@@ -2400,18 +2413,16 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
 		context->gva_to_gpa = nonpaging_gva_to_gpa;
 		context->root_level = 0;
 	} else if (is_long_mode(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT64_ROOT_LEVEL;
 	} else if (is_pae(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT32E_ROOT_LEVEL;
 	} else {
-		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
 		context->gva_to_gpa = paging32_gva_to_gpa;
 		context->root_level = PT32_ROOT_LEVEL;
 	}
+	update_rsvd_bits_mask(vcpu);
 
 	return 0;
 }

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

index b59fc67..1c76e08 100644
@@ -416,6 +416,9 @@ out:
 
 static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 {
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE;
+
 	cr0 |= X86_CR0_ET;
 
 #ifdef CONFIG_X86_64

@@ -449,7 +452,8 @@ static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 
 	kvm_x86_ops->set_cr0(vcpu, cr0);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr0 ^ old_cr0) & update_bits)
+		kvm_mmu_reset_context(vcpu);
 	return 0;
 }

@@ -487,7 +491,8 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 
 	kvm_x86_ops->set_cr4(vcpu, cr4);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr4 ^ old_cr4) & pdptr_bits)
+		kvm_mmu_reset_context(vcpu);
 
 	return 0;
 }

@@ -692,6 +697,8 @@ static u32 emulated_msrs[] = {
 
 static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
+	u64 old_efer = vcpu->arch.efer;
+
 	if (efer & efer_reserved_bits)
 		return 1;

@@ -722,6 +729,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
 
+	if ((efer ^ old_efer) & EFER_NX)
+		update_rsvd_bits_mask(vcpu);
+
 	return 0;
 }

Signature · 1 line

1.7.0.1

Marcelo Tosatti

2010-05-11 19:36 UTC #5 lore raw

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

Quoted 72 lines · Sheng Yang

Only modifying some bits of CR0/CR4 needs paging mode switch.

Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/mmu.c              |   17 ++++++++++++++---
 arch/x86/kvm/x86.c              |   14 ++++++++++++--
 3 files changed, 27 insertions(+), 5 deletions(-)

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

index ed48904..c8c8a03 100644
@@ -553,6 +553,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot);
 void kvm_mmu_zap_all(struct kvm *kvm);
 unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
 void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu);
 
 int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);

arch/x86/kvm/mmu.c +14 -3

index 5412185..98abdcf 100644
@@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
 	}
 }
 
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
+{
+	if (!is_paging(vcpu))
+		return;
+	if (is_long_mode(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
+	else if (is_pae(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
+	else
+		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
+}
+EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
+
 static int paging64_init_context_common(struct kvm_vcpu *vcpu, int level)
 {
 	struct kvm_mmu *context = &vcpu->arch.mmu;

@@ -2400,18 +2413,16 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
 		context->gva_to_gpa = nonpaging_gva_to_gpa;
 		context->root_level = 0;
 	} else if (is_long_mode(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT64_ROOT_LEVEL;
 	} else if (is_pae(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT32E_ROOT_LEVEL;
 	} else {
-		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
 		context->gva_to_gpa = paging32_gva_to_gpa;
 		context->root_level = PT32_ROOT_LEVEL;
 	}
+	update_rsvd_bits_mask(vcpu);
 
 	return 0;
 }

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

index b59fc67..1c76e08 100644

arch/x86/kvm/x86.c out: · line 420

Quoted 5 lines

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

@@ -416,6 +416,9 @@ out:
 
 static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 {
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE;

If PAE paging would be in use following an execution of MOV to CR0 or MOV to CR4 (see Section 4.1.1) and the instruction is modifying any of CR0.CD, CR0.NW, CR0.PG, CR4.PAE, CR4.PGE, or CR4.PSE; then the PDPTEs are loaded from the address in CR3.

If the PDPTRS changed, the mmu must be reloaded.

Sheng Yang

author

2010-05-12 01:54 UTC #6 lore raw

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

Quoted 112 lines · On Wednesday 12 May 2010 03:36:26 Marcelo Tosatti wrote:

Quoted 100 lines · Sheng Yang

Only modifying some bits of CR0/CR4 needs paging mode switch.

Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.

Signed-off-by Sheng Yang <[email protected]>

---

 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/mmu.c              |   17 ++++++++++++++---
 arch/x86/kvm/x86.c              |   14 ++++++++++++--
 3 files changed, 27 insertions(+), 5 deletions(-)

(quoted diff) +0 -0

b/arch/x86/include/asm/kvm_host.h index ed48904..c8c8a03 100644

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

@@ -553,6 +553,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm
 *kvm, int slot);
 
 void kvm_mmu_zap_all(struct kvm *kvm);
 unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
 void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int
 kvm_nr_mmu_pages);
 
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu);
 
 int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);

arch/x86/kvm/mmu.c +13 -0

index 5412185..98abdcf 100644
@@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu
 *vcpu, int level)
 
 	}
 
 }
 
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
+{
+	if (!is_paging(vcpu))
+		return;
+	if (is_long_mode(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
+	else if (is_pae(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
+	else
+		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
+}
+EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
+

static int paging64_init_context_common(struct kvm_vcpu *vcpu, int level) {

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

 	struct kvm_mmu *context = &vcpu->arch.mmu;

@@ -2400,18 +2413,16 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu
 *vcpu)
 
 		context->gva_to_gpa = nonpaging_gva_to_gpa;
 		context->root_level = 0;
 
 	} else if (is_long_mode(vcpu)) {
 
-		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
 
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT64_ROOT_LEVEL;
 
 	} else if (is_pae(vcpu)) {
 
-		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
 
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT32E_ROOT_LEVEL;

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

 	} else {
 
-		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
 
 		context->gva_to_gpa = paging32_gva_to_gpa;
 		context->root_level = PT32_ROOT_LEVEL;
 
 	}
 
+	update_rsvd_bits_mask(vcpu);
 
 	return 0;
 
 }

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

index b59fc67..1c76e08 100644

arch/x86/kvm/x86.c out: · line 420

Quoted 5 lines

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

@@ -416,6 +416,9 @@ out:
 static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 {
 
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE;

Quoted 1 line

If the PDPTRS changed, the mmu must be reloaded.

Yes... Would add CR0.CD and CR0.NW here.

--
regards
Yang, Sheng

Sheng Yang

author

2010-05-12 02:10 UTC #7 lore raw

PATCH 4/4 VMX: x86: Only reset MMU when necessary

Only modifying some bits of CR0/CR4 needs paging mode switch.

Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/mmu.c              |   17 ++++++++++++++---
 arch/x86/kvm/x86.c              |   15 +++++++++++++--
 3 files changed, 28 insertions(+), 5 deletions(-)

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

index ed48904..c8c8a03 100644
@@ -553,6 +553,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot);
 void kvm_mmu_zap_all(struct kvm *kvm);
 unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
 void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu);
 
 int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);

arch/x86/kvm/mmu.c +14 -3

index 5412185..98abdcf 100644
@@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
 	}
 }
 
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
+{
+	if (!is_paging(vcpu))
+		return;
+	if (is_long_mode(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
+	else if (is_pae(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
+	else
+		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
+}
+EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
+
 static int paging64_init_context_common(struct kvm_vcpu *vcpu, int level)
 {
 	struct kvm_mmu *context = &vcpu->arch.mmu;

@@ -2400,18 +2413,16 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
 		context->gva_to_gpa = nonpaging_gva_to_gpa;
 		context->root_level = 0;
 	} else if (is_long_mode(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT64_ROOT_LEVEL;
 	} else if (is_pae(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT32E_ROOT_LEVEL;
 	} else {
-		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
 		context->gva_to_gpa = paging32_gva_to_gpa;
 		context->root_level = PT32_ROOT_LEVEL;
 	}
+	update_rsvd_bits_mask(vcpu);
 
 	return 0;
 }

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

index b59fc67..971a295 100644
@@ -416,6 +416,10 @@ out:
 
 static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 {
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE |
+				    X86_CR0_CD | X86_CR0_NW;
+
 	cr0 |= X86_CR0_ET;
 
 #ifdef CONFIG_X86_64

@@ -449,7 +453,8 @@ static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 
 	kvm_x86_ops->set_cr0(vcpu, cr0);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr0 ^ old_cr0) & update_bits)
+		kvm_mmu_reset_context(vcpu);
 	return 0;
 }

@@ -487,7 +492,8 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 
 	kvm_x86_ops->set_cr4(vcpu, cr4);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr4 ^ old_cr4) & pdptr_bits)
+		kvm_mmu_reset_context(vcpu);
 
 	return 0;
 }

@@ -692,6 +698,8 @@ static u32 emulated_msrs[] = {
 
 static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
+	u64 old_efer = vcpu->arch.efer;
+
 	if (efer & efer_reserved_bits)
 		return 1;

@@ -722,6 +730,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
 
+	if ((efer ^ old_efer) & EFER_NX)
+		update_rsvd_bits_mask(vcpu);
+
 	return 0;
 }

Signature · 1 line

1.7.0.1

Avi Kivity

2010-05-12 06:31 UTC #8 lore raw

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

Quoted 4 lines · Sheng Yang

Only modifying some bits of CR0/CR4 needs paging mode switch.
Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.

Can you please repost the whole series? Due to a problem with my mailbox I don't have the patches either in my inbox or on kvm@.

Signature · 1 line

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

Sheng Yang

author

2010-05-12 06:33 UTC #9 lore raw

PATCH 1/4 KVM: x86: Check LMA bit before set_efer

kvm_x86_ops->set_efer() would execute vcpu->arch.efer = efer, so the checking of LMA bit didn't work.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/x86.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

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

index cd8a606..2cae460 100644
@@ -715,11 +715,11 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 			return 1;
 	}
 
-	kvm_x86_ops->set_efer(vcpu, efer);
-
 	efer &= ~EFER_LMA;
 	efer |= vcpu->arch.efer & EFER_LMA;
 
+	kvm_x86_ops->set_efer(vcpu, efer);
+
 	vcpu->arch.efer = efer;
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;

Signature · 1 line

1.7.0.1

Sheng Yang

author

2010-05-12 06:33 UTC #10 lore raw

PATCH 2/4 KVM: Clean up duplicate assignment

In reply to Avi Kivity

mmu.free() already set root_hpa to INVALID_PAGE, no need to do it again in the destory_kvm_mmu().

kvm_x86_ops->set_cr4() and set_efer() already assign cr4/efer to vcpu->arch.cr4/efer, no need to do it again later.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/mmu.c |    5 ++---
 arch/x86/kvm/x86.c |    4 +---
 2 files changed, 3 insertions(+), 6 deletions(-)

arch/x86/kvm/mmu.c +2 -3

index 95bee9d..5412185 100644
@@ -2450,10 +2450,9 @@ static int init_kvm_mmu(struct kvm_vcpu *vcpu)
 static void destroy_kvm_mmu(struct kvm_vcpu *vcpu)
 {
 	ASSERT(vcpu);
-	if (VALID_PAGE(vcpu->arch.mmu.root_hpa)) {
+	if (VALID_PAGE(vcpu->arch.mmu.root_hpa))
+		/* mmu.free() should set root_hpa = INVALID_PAGE */
 		vcpu->arch.mmu.free(vcpu);
-		vcpu->arch.mmu.root_hpa = INVALID_PAGE;
-	}
 }
 
 int kvm_mmu_reset_context(struct kvm_vcpu *vcpu)

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

index 2cae460..764f89b 100644
@@ -486,7 +486,7 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 		return 1;
 
 	kvm_x86_ops->set_cr4(vcpu, cr4);
-	vcpu->arch.cr4 = cr4;
+
 	kvm_mmu_reset_context(vcpu);
 
 	return 0;

@@ -720,8 +720,6 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	kvm_x86_ops->set_efer(vcpu, efer);
 
-	vcpu->arch.efer = efer;
-
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
 	kvm_mmu_reset_context(vcpu);

Signature · 1 line

1.7.0.1

Sheng Yang

author

2010-05-12 06:33 UTC #11 lore raw

PATCH 3/4 KVM: x86: Remove kvm_mmu_reset_context() in kvm_set_efer()

In reply to Avi Kivity

Modify EFER won't result in mode switch directly. After EFER.LME set, the following set CR0.PG would result in mode switch to IA32e. And the later action already covered by kvm_set_cr0().

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/x86.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

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

index 764f89b..b59fc67 100644
@@ -721,7 +721,6 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	kvm_x86_ops->set_efer(vcpu, efer);
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
-	kvm_mmu_reset_context(vcpu);
 
 	return 0;
 }

Signature · 1 line

1.7.0.1

Sheng Yang

author

2010-05-12 06:33 UTC #12 lore raw

PATCH 4/4 VMX: x86: Only reset MMU when necessary

In reply to Avi Kivity

Only modifying some bits of CR0/CR4 needs paging mode switch.

Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/mmu.c              |   17 ++++++++++++++---
 arch/x86/kvm/x86.c              |   15 +++++++++++++--
 3 files changed, 28 insertions(+), 5 deletions(-)

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

index ed48904..c8c8a03 100644
@@ -553,6 +553,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot);
 void kvm_mmu_zap_all(struct kvm *kvm);
 unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
 void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu);
 
 int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);

arch/x86/kvm/mmu.c +14 -3

index 5412185..98abdcf 100644
@@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
 	}
 }
 
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
+{
+	if (!is_paging(vcpu))
+		return;
+	if (is_long_mode(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
+	else if (is_pae(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
+	else
+		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
+}
+EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
+
 static int paging64_init_context_common(struct kvm_vcpu *vcpu, int level)
 {
 	struct kvm_mmu *context = &vcpu->arch.mmu;

@@ -2400,18 +2413,16 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
 		context->gva_to_gpa = nonpaging_gva_to_gpa;
 		context->root_level = 0;
 	} else if (is_long_mode(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT64_ROOT_LEVEL;
 	} else if (is_pae(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT32E_ROOT_LEVEL;
 	} else {
-		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
 		context->gva_to_gpa = paging32_gva_to_gpa;
 		context->root_level = PT32_ROOT_LEVEL;
 	}
+	update_rsvd_bits_mask(vcpu);
 
 	return 0;
 }

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

index b59fc67..971a295 100644
@@ -416,6 +416,10 @@ out:
 
 static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 {
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE |
+				    X86_CR0_CD | X86_CR0_NW;
+
 	cr0 |= X86_CR0_ET;
 
 #ifdef CONFIG_X86_64

@@ -449,7 +453,8 @@ static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 
 	kvm_x86_ops->set_cr0(vcpu, cr0);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr0 ^ old_cr0) & update_bits)
+		kvm_mmu_reset_context(vcpu);
 	return 0;
 }

@@ -487,7 +492,8 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 
 	kvm_x86_ops->set_cr4(vcpu, cr4);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr4 ^ old_cr4) & pdptr_bits)
+		kvm_mmu_reset_context(vcpu);
 
 	return 0;
 }

@@ -692,6 +698,8 @@ static u32 emulated_msrs[] = {
 
 static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
+	u64 old_efer = vcpu->arch.efer;
+
 	if (efer & efer_reserved_bits)
 		return 1;

@@ -722,6 +730,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
 
+	if ((efer ^ old_efer) & EFER_NX)
+		update_rsvd_bits_mask(vcpu);
+
 	return 0;
 }

Signature · 1 line

1.7.0.1

Avi Kivity

2010-05-12 06:59 UTC #13 lore raw

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

Quoted 12 lines · Sheng Yang

Only modifying some bits of CR0/CR4 needs paging mode switch.
Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.
arch/x86/kvm/mmu.c +3 -0
@@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
  	}
  }
 
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
+{
+	if (!is_paging(vcpu))

arch/x86/kvm/mmu.c static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level) · line 2350

Quoted 10 lines

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

+		return;
+	if (is_long_mode(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
+	else if (is_pae(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
+	else
+		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
+}
+EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);

Needs a kvm_ prefix if made a global symbol. But isn't nx switching rare enough so we can reload the mmu completely?

arch/x86/kvm/x86.c out: · line 422

Quoted 12 lines

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

index b59fc67..971a295 100644
@@ -416,6 +416,10 @@ out:
 
  static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
  {
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE |
+				    X86_CR0_CD | X86_CR0_NW;

PE doesn't affect paging, CD, NW don't either?

What about WP?

Quoted 25 lines

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

+
  	cr0 |= X86_CR0_ET;

  #ifdef CONFIG_X86_64

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

@@ -449,7 +453,8 @@ static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 
  	kvm_x86_ops->set_cr0(vcpu, cr0);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr0 ^ old_cr0)&  update_bits)
+		kvm_mmu_reset_context(vcpu);
  	return 0;
  }

@@ -692,6 +698,8 @@ static u32 emulated_msrs[] = {
 
  static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
  {
+	u64 old_efer = vcpu->arch.efer;
+
  	if (efer&  efer_reserved_bits)
  		return 1;

arch/x86/kvm/x86.c static int set_efer(struct kvm_vcpu *vcpu, u64 efer) · line 738

Quoted 9 lines

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

@@ -722,6 +730,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
  	vcpu->arch.mmu.base_role.nxe = (efer&  EFER_NX)&&  !tdp_enabled;
 
+	if ((efer ^ old_efer)&  EFER_NX)
+		update_rsvd_bits_mask(vcpu);
+
  	return 0;
  }

I think it's fine to reset the entire mmu context here, most guests won't toggle nx all the time. But it needs to be in patch 3, otherwise we have a regression between 3 and 4.

Signature · 1 line

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

Sheng Yang

author

2010-05-12 07:32 UTC #14 lore raw

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

Quoted 14 lines · On Wednesday 12 May 2010 14:59:14 Avi Kivity wrote:

Quoted 13 lines · Sheng Yang
Only modifying some bits of CR0/CR4 needs paging mode switch.
Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.
(quoted diff) +0 -0
@@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu
 *vcpu, int level)
 
  	}
 
  }
 

Quoted 15 lines

arch/x86/kvm/mmu.c static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level) · line 2349
Quoted 12 lines
arch/x86/kvm/mmu.c +12 -0
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
+{
+	if (!is_paging(vcpu))
+		return;
+	if (is_long_mode(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
+	else if (is_pae(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
+	else
+		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
+}
+EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
Needs a kvm_ prefix if made a global symbol. But isn't nx switching rare enough so we can reload the mmu completely?

Yes, it should be rare enough. I am OK with keeping it, though "reload MMU" seems a little misleading meaning here.

Quoted 9 lines

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

index b59fc67..971a295 100644
@@ -416,6 +416,10 @@ out:
  static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
  {

Quoted 5 lines

arch/x86/kvm/x86.c out: · line 421
Quoted 3 lines
arch/x86/kvm/x86.c +3 -0
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE |
+				    X86_CR0_CD | X86_CR0_NW;
PE doesn't affect paging, CD, NW don't either?

Yes, PE can't affect alone.

Marcelo has commented on CD/NW, because we need to reload pdptrs if they changed, then we need to reload MMU.

Quoted 2 lines

What about WP?

How WP would affect?

Quoted 39 lines

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

+
 
  	cr0 |= X86_CR0_ET;

  #ifdef CONFIG_X86_64

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

@@ -449,7 +453,8 @@ static int __kvm_set_cr0(struct kvm_vcpu *vcpu,
 unsigned long cr0)
 
  	kvm_x86_ops->set_cr0(vcpu, cr0);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr0 ^ old_cr0)&  update_bits)
+		kvm_mmu_reset_context(vcpu);
 
  	return 0;

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

@@ -692,6 +698,8 @@ static u32 emulated_msrs[] = {
 
  static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
  {
 
+	u64 old_efer = vcpu->arch.efer;
+
 
  	if (efer&  efer_reserved_bits)

return 1;

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

@@ -722,6 +730,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
  	vcpu->arch.mmu.base_role.nxe = (efer&  EFER_NX)&&  !tdp_enabled;
 
+	if ((efer ^ old_efer)&  EFER_NX)
+		update_rsvd_bits_mask(vcpu);
+

Quoted 7 lines

Quoted 3 lines
arch/x86/kvm/x86.c +0 -0
  	return 0;
 
  }
I think it's fine to reset the entire mmu context here, most guests won't toggle nx all the time. But it needs to be in patch 3, otherwise we have a regression between 3 and 4.

OK. Would drop patch 3 and keep mmu reset if you like... --
regards
Yang, Sheng

Avi Kivity

2010-05-12 08:11 UTC #15 lore raw

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

Quoted 16 lines · Sheng Yang

Quoted 10 lines
Quoted 10 lines
arch/x86/kvm/x86.c +0 -0
index b59fc67..971a295 100644
@@ -416,6 +416,10 @@ out:
   static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
   {
 
Quoted 6 lines
arch/x86/kvm/x86.c out: · line 424
Quoted 4 lines
arch/x86/kvm/x86.c +3 -0
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE |
+				    X86_CR0_CD | X86_CR0_NW;
 
PE doesn't affect paging, CD, NW don't either?

Quoted 5 lines

Yes, PE can't affect alone.
Marcelo has commented on CD/NW, because we need to reload pdptrs if they changed, then we need to reload MMU.

Ah, correct.

Quoted 4 lines

Quoted 2 lines
What about WP?
How WP would affect?

If cr0.wp=0 then we can have a pte with gpte.rw=0 but spte.rw=1 (since the guest always runs with cr0.wp=1). So we need to reload the mmu to switch page tables.

This won't work now, I'll post a patch adding cr0.wp to sp->role. But please add cr0.wp to the set of bits requiring reload so we won't have a regression.

Quoted 10 lines

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

@@ -722,6 +730,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
   	vcpu->arch.mmu.base_role.nxe = (efer&   EFER_NX)&&   !tdp_enabled;
 
+	if ((efer ^ old_efer)&   EFER_NX)
+		update_rsvd_bits_mask(vcpu);
+
 
   	return 0;

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
   }
I think it's fine to reset the entire mmu context here, most guests won't toggle nx all the time. But it needs to be in patch 3, otherwise we have a regression between 3 and 4.
OK. Would drop patch 3 and keep mmu reset if you like...

Yes please.

Signature · 1 line

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