KVM: VMX: Fix 32bit Windows blue screen with EPT

PATCH Superseded Sheng Yang 2 participants 9 messages Dec 30, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Sheng Yang

author

2010-12-30 08:34 UTC #0 lore raw

PATCH 1/1 KVM: VMX: Fix 32bit Windows blue screen with EPT

After CR0 is changed during VMExit, the result of kvm_read_cr3() may be different. Commit d95bfcdd7cda4dfdac9588e684bc7c75794a075e "KVM: Fetch guest cr3 from hardware on demand" caused 32bit Windows guest blue screen when using with EPT. This patch fixes it by decache CR3 before CR0 change, for both paging to nonpaging, and nonpaging to paging switch.

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

---

But I haven't found the exactly point affected by this, any clue?

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

arch/x86/kvm/vmx.c +1 -5

index f107315..0b8cfc1 100644
@@ -1921,8 +1921,7 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
 					unsigned long cr0,
 					struct kvm_vcpu *vcpu)
 {
-	ulong cr3;
-
+	kvm_read_cr3(vcpu);
 	if (!(cr0 & X86_CR0_PG)) {
 		/* From paging/starting to nonpaging */
 		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,

@@ -1937,11 +1936,8 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
 			     vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) &
 			     ~(CPU_BASED_CR3_LOAD_EXITING |
 			       CPU_BASED_CR3_STORE_EXITING));
-		/* Must fetch cr3 before updating cr0 */
-		cr3 = kvm_read_cr3(vcpu);
 		vcpu->arch.cr0 = cr0;
 		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
-		vmx_set_cr3(vcpu, cr3);
 	}
 
 	if (!(cr0 & X86_CR0_WP))

Signature · 1 line

1.7.0.1

Avi Kivity

2010-12-30 08:57 UTC #1 lore raw

Quoted 11 lines · Sheng Yang

After CR0 is changed during VMExit, the result of kvm_read_cr3() may be different. Commit d95bfcdd7cda4dfdac9588e684bc7c75794a075e "KVM: Fetch guest cr3 from hardware on demand" caused 32bit Windows guest blue screen when using with EPT. This patch fixes it by decache CR3 before CR0 change, for both paging to nonpaging, and nonpaging to paging switch.
Signed-off-by Sheng Yang<[email protected]>
---
But I haven't found the exactly point affected by this, any clue?

Can't see it either.

arch/x86/kvm/vmx.c static void ept_update_paging_mode_cr0(unsigned long *hw_cr0, · line 1924

Quoted 7 lines

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

@@ -1921,8 +1921,7 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
  					unsigned long cr0,
  					struct kvm_vcpu *vcpu)
  {
-	ulong cr3;
-
+	kvm_read_cr3(vcpu);

Without this line, it fails?

I think it's better to call vmx_decache_cr3() explicitly, since it explains what we're doing. vmx_decache_cr3 depends on arch.cr0, and we're changing that here.

arch/x86/kvm/vmx.c static void ept_update_paging_mode_cr0(unsigned long *hw_cr0, · line 1944

Quoted 12 lines

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

  	if (!(cr0&  X86_CR0_PG)) {
  		/* From paging/starting to nonpaging */
  		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,

@@ -1937,11 +1936,8 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
  			     vmcs_read32(CPU_BASED_VM_EXEC_CONTROL)&
  			~(CPU_BASED_CR3_LOAD_EXITING |
  			       CPU_BASED_CR3_STORE_EXITING));
-		/* Must fetch cr3 before updating cr0 */
-		cr3 = kvm_read_cr3(vcpu);
  		vcpu->arch.cr0 = cr0;
  		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
-		vmx_set_cr3(vcpu, cr3);

This is indeed bogus. But what ensures that we'll have the correct GUEST_CR3 after enabling paging?

Quoted 3 lines

(quoted diff) +0 -0

  	}
 
  	if (!(cr0&  X86_CR0_WP))

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2010-12-30 09:04 UTC #2 lore raw

Quoted 21 lines · On Thursday 30 December 2010 16:57:20 Avi Kivity wrote:

Quoted 11 lines · Sheng Yang
After CR0 is changed during VMExit, the result of kvm_read_cr3() may be different. Commit d95bfcdd7cda4dfdac9588e684bc7c75794a075e "KVM: Fetch guest cr3 from hardware on demand" caused 32bit Windows guest blue screen when using with EPT. This patch fixes it by decache CR3 before CR0 change, for both paging to nonpaging, and nonpaging to paging switch.
Signed-off-by Sheng Yang<[email protected]>
---
But I haven't found the exactly point affected by this, any clue?
Can't see it either.
Quoted 6 lines
(quoted diff) +0 -0
@@ -1921,8 +1921,7 @@ static void ept_update_paging_mode_cr0(unsigned
 long *hw_cr0,
 
  					unsigned long cr0,
  					struct kvm_vcpu *vcpu)
 

Quoted 7 lines

arch/x86/kvm/vmx.c static void ept_update_paging_mode_cr0(unsigned long *hw_cr0, · line 1924
Quoted 5 lines
arch/x86/kvm/vmx.c +1 -2
  {
 
-	ulong cr3;
-
+	kvm_read_cr3(vcpu);
Without this line, it fails?

Yes, seems something happened on paging to nonpaging switch process.

Quoted 4 lines

I think it's better to call vmx_decache_cr3() explicitly, since it explains what we're doing. vmx_decache_cr3 depends on arch.cr0, and we're changing that here.

OK.

Quoted 18 lines

  	if (!(cr0&  X86_CR0_PG)) {

/* From paging/starting to nonpaging */ vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,

(quoted diff) +0 -2

@@ -1937,11 +1936,8 @@ static void ept_update_paging_mode_cr0(unsigned
 long *hw_cr0,
 
  			     vmcs_read32(CPU_BASED_VM_EXEC_CONTROL)&
 
  			~(CPU_BASED_CR3_LOAD_EXITING |
 
  			       CPU_BASED_CR3_STORE_EXITING));
 
-		/* Must fetch cr3 before updating cr0 */
-		cr3 = kvm_read_cr3(vcpu);

Quoted 7 lines

arch/x86/kvm/vmx.c static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
Quoted 4 lines
arch/x86/kvm/vmx.c +0 -1
  		vcpu->arch.cr0 = cr0;
  		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
 
-		vmx_set_cr3(vcpu, cr3);
This is indeed bogus. But what ensures that we'll have the correct GUEST_CR3 after enabling paging?

In fact I don't understand why we need this line. All modification is for CR3 reading, why we need to set hardware CR3 again? It should be the same as when we don't have CR3 accessor I think.

--
regards
Yang, Sheng

Quoted 4 lines

Quoted 4 lines
  	}
if (!(cr0& X86_CR0_WP))

Avi Kivity

2010-12-30 09:14 UTC #3 lore raw

Quoted 21 lines · Sheng Yang

Quoted 21 lines
Quoted 21 lines
   	if (!(cr0&   X86_CR0_PG)) {
   		/* From paging/starting to nonpaging */
   		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
@@ -1937,11 +1936,8 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
   			vmcs_read32(CPU_BASED_VM_EXEC_CONTROL)&
   			
   			~(CPU_BASED_CR3_LOAD_EXITING |
   			
   			CPU_BASED_CR3_STORE_EXITING));
- /* Must fetch cr3 before updating cr0 */
- cr3 = kvm_read_cr3(vcpu);
   		vcpu->arch.cr0 = cr0;
   		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
- vmx_set_cr3(vcpu, cr3);
This is indeed bogus. But what ensures that we'll have the correct GUEST_CR3 after enabling paging?
In fact I don't understand why we need this line. All modification is for CR3 reading, why we need to set hardware CR3 again? It should be the same as when we don't have CR3 accessor I think.

when cr0.pg=0 then we set GUEST_CR3=identity_pagetable. We don't want that when we we switch to paging mode.

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2010-12-30 09:18 UTC #4 lore raw

Quoted 26 lines · On Thursday 30 December 2010 17:14:23 Avi Kivity wrote:

Quoted 25 lines · Sheng Yang
Quoted 24 lines
Quoted 14 lines
   	if (!(cr0&   X86_CR0_PG)) {
   		/* From paging/starting to nonpaging */
   		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
@@ -1937,11 +1936,8 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
   			vmcs_read32(CPU_BASED_VM_EXEC_CONTROL)&
   			
   			~(CPU_BASED_CR3_LOAD_EXITING |
   			
   			CPU_BASED_CR3_STORE_EXITING));
Quoted 7 lines
- /* Must fetch cr3 before updating cr0 */
- cr3 = kvm_read_cr3(vcpu);
   		vcpu->arch.cr0 = cr0;
   		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
- vmx_set_cr3(vcpu, cr3);
This is indeed bogus. But what ensures that we'll have the correct GUEST_CR3 after enabling paging?

Quoted 6 lines

Quoted 3 lines
In fact I don't understand why we need this line. All modification is for CR3 reading, why we need to set hardware CR3 again? It should be the same as when we don't have CR3 accessor I think.
when cr0.pg=0 then we set GUEST_CR3=identity_pagetable. We don't want that when we we switch to paging mode.

I think kvm_mmu_reset_context() in kvm_set_cr0() should cover this?

--
regards
Yang, Sheng

Avi Kivity

2010-12-30 09:23 UTC #5 lore raw

Quoted 27 lines · Sheng Yang

Quoted 26 lines · On Thursday 30 December 2010 17:14:23 Avi Kivity wrote:
Quoted 25 lines · Sheng Yang
Quoted 24 lines
Quoted 14 lines
    	if (!(cr0&    X86_CR0_PG)) {
    	
    		/* From paging/starting to nonpaging */
    		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
@@ -1937,11 +1936,8 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
    			vmcs_read32(CPU_BASED_VM_EXEC_CONTROL)&
    			
    			~(CPU_BASED_CR3_LOAD_EXITING |
    			
    			CPU_BASED_CR3_STORE_EXITING));
Quoted 7 lines
  -		/* Must fetch cr3 before updating cr0 */
  -		cr3 = kvm_read_cr3(vcpu);
    		vcpu->arch.cr0 = cr0;
    		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
  -		vmx_set_cr3(vcpu, cr3);
This is indeed bogus. But what ensures that we'll have the correct GUEST_CR3 after enabling paging?

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
In fact I don't understand why we need this line. All modification is for CR3 reading, why we need to set hardware CR3 again? It should be the same as when we don't have CR3 accessor I think.
when cr0.pg=0 then we set GUEST_CR3=identity_pagetable. We don't want that when we we switch to paging mode.
I think kvm_mmu_reset_context() in kvm_set_cr0() should cover this?

I looked at it but missed it. kvm_mmu_reset_context() doesn't actually do this, but it does schedule a kvm_mmu_load() which will call vmx_set_cr3().

Thanks, I'll apply your v2 now.

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2010-12-30 09:08 UTC #6 lore raw

In reply to Avi Kivity

Quoted 50 lines · On Thursday 30 December 2010 16:57:20 Avi Kivity wrote:

Quoted 11 lines · Sheng Yang
After CR0 is changed during VMExit, the result of kvm_read_cr3() may be different. Commit d95bfcdd7cda4dfdac9588e684bc7c75794a075e "KVM: Fetch guest cr3 from hardware on demand" caused 32bit Windows guest blue screen when using with EPT. This patch fixes it by decache CR3 before CR0 change, for both paging to nonpaging, and nonpaging to paging switch.
Signed-off-by Sheng Yang<[email protected]>
---
But I haven't found the exactly point affected by this, any clue?
Can't see it either.
arch/x86/kvm/vmx.c static void ept_update_paging_mode_cr0(unsigned · line 1924
Quoted 11 lines
arch/x86/kvm/vmx.c +1 -2
@@ -1921,8 +1921,7 @@ static void ept_update_paging_mode_cr0(unsigned
 long *hw_cr0,
 
  					unsigned long cr0,
  					struct kvm_vcpu *vcpu)
 
  {
 
-	ulong cr3;
-
+	kvm_read_cr3(vcpu);
Without this line, it fails?
I think it's better to call vmx_decache_cr3() explicitly, since it explains what we're doing. vmx_decache_cr3 depends on arch.cr0, and we're changing that here.
Quoted 17 lines
  	if (!(cr0&  X86_CR0_PG)) {
/* From paging/starting to nonpaging */ vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
(quoted diff) +0 -2
@@ -1937,11 +1936,8 @@ static void ept_update_paging_mode_cr0(unsigned
 long *hw_cr0,
 
  			     vmcs_read32(CPU_BASED_VM_EXEC_CONTROL)&
 
  			~(CPU_BASED_CR3_LOAD_EXITING |
 
  			       CPU_BASED_CR3_STORE_EXITING));
 
-		/* Must fetch cr3 before updating cr0 */
-		cr3 = kvm_read_cr3(vcpu);

Quoted 7 lines

arch/x86/kvm/vmx.c static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
Quoted 4 lines
arch/x86/kvm/vmx.c +0 -1
  		vcpu->arch.cr0 = cr0;
  		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
 
-		vmx_set_cr3(vcpu, cr3);
This is indeed bogus. But what ensures that we'll have the correct GUEST_CR3 after enabling paging?

BTW: What did you find when you added this two lines?

--
regards
Yang, Sheng

Quoted 4 lines

Quoted 4 lines
  	}
if (!(cr0& X86_CR0_WP))

Avi Kivity

2010-12-30 09:16 UTC #7 lore raw

Quoted 21 lines · Sheng Yang

Quoted 21 lines
Quoted 21 lines
   	if (!(cr0&   X86_CR0_PG)) {
   		/* From paging/starting to nonpaging */
   		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
@@ -1937,11 +1936,8 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
   			vmcs_read32(CPU_BASED_VM_EXEC_CONTROL)&
   			
   			~(CPU_BASED_CR3_LOAD_EXITING |
   			
   			CPU_BASED_CR3_STORE_EXITING));
- /* Must fetch cr3 before updating cr0 */
- cr3 = kvm_read_cr3(vcpu);
   		vcpu->arch.cr0 = cr0;
   		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
- vmx_set_cr3(vcpu, cr3);
This is indeed bogus. But what ensures that we'll have the correct GUEST_CR3 after enabling paging?
BTW: What did you find when you added this two lines?

Don't remember... but I can't see how it works without them (though they're wrong - they set the wrong cr3 when !enable_ept).

I'll do some work to disentangle this mess. ->set_cr3() means two different things, sometimes it's the guest visible cr3, sometimes it's the mmu cr3.

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2010-12-30 09:19 UTC #8 lore raw

PATCH v2 1/1 KVM: VMX: Fix 32bit Windows blue screen with EPT

In reply to Avi Kivity

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

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

arch/x86/kvm/vmx.c +1 -5

index f107315..bf89ec2 100644
@@ -1921,8 +1921,7 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
 					unsigned long cr0,
 					struct kvm_vcpu *vcpu)
 {
-	ulong cr3;
-
+	vmx_decache_cr3(vcpu);
 	if (!(cr0 & X86_CR0_PG)) {
 		/* From paging/starting to nonpaging */
 		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,

@@ -1937,11 +1936,8 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
 			     vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) &
 			     ~(CPU_BASED_CR3_LOAD_EXITING |
 			       CPU_BASED_CR3_STORE_EXITING));
-		/* Must fetch cr3 before updating cr0 */
-		cr3 = kvm_read_cr3(vcpu);
 		vcpu->arch.cr0 = cr0;
 		vmx_set_cr4(vcpu, kvm_read_cr4(vcpu));
-		vmx_set_cr3(vcpu, cr3);
 	}
 
 	if (!(cr0 & X86_CR0_WP))

Signature · 1 line

1.7.0.1