* Re: [PATCH] KVM: VMX: Fix 32bit Windows blue screen with EPT
2010-12-30 8:57 ` Avi Kivity
@ 2010-12-30 9:05 ` Sheng Yang
2010-12-30 9:14 ` Avi Kivity
2010-12-30 9:09 ` Sheng Yang
2010-12-30 9:21 ` [PATCH v2] " Sheng Yang
2 siblings, 1 reply; 9+ messages in thread
From: Sheng Yang @ 2010-12-30 9:05 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, kvm
On Thursday 30 December 2010 16:57:20 Avi Kivity wrote:
> On 12/30/2010 10:35 AM, Sheng Yang wrote:
> > 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<sheng@linux.intel.com>
> > ---
> >
> > But I haven't found the exactly point affected by this, any clue?
>
> Can't see it either.
>
> > @@ -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?
Yes, seems something happened on paging to nonpaging switch process.
>
> 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.
>
> > 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?
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
>
> > }
> >
> > if (!(cr0& X86_CR0_WP))
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: VMX: Fix 32bit Windows blue screen with EPT
2010-12-30 9:05 ` Sheng Yang
@ 2010-12-30 9:14 ` Avi Kivity
2010-12-30 9:20 ` Sheng Yang
0 siblings, 1 reply; 9+ messages in thread
From: Avi Kivity @ 2010-12-30 9:14 UTC (permalink / raw)
To: Sheng Yang; +Cc: Marcelo Tosatti, kvm
On 12/30/2010 11:05 AM, Sheng Yang wrote:
> >
> > > 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?
>
> 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.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: VMX: Fix 32bit Windows blue screen with EPT
2010-12-30 9:14 ` Avi Kivity
@ 2010-12-30 9:20 ` Sheng Yang
2010-12-30 9:23 ` Avi Kivity
0 siblings, 1 reply; 9+ messages in thread
From: Sheng Yang @ 2010-12-30 9:20 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, kvm
On Thursday 30 December 2010 17:14:23 Avi Kivity wrote:
> On 12/30/2010 11:05 AM, Sheng Yang wrote:
> > > > 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?
> >
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: VMX: Fix 32bit Windows blue screen with EPT
2010-12-30 9:20 ` Sheng Yang
@ 2010-12-30 9:23 ` Avi Kivity
0 siblings, 0 replies; 9+ messages in thread
From: Avi Kivity @ 2010-12-30 9:23 UTC (permalink / raw)
To: Sheng Yang; +Cc: Marcelo Tosatti, kvm
On 12/30/2010 11:20 AM, Sheng Yang wrote:
> On Thursday 30 December 2010 17:14:23 Avi Kivity wrote:
> > On 12/30/2010 11:05 AM, Sheng Yang wrote:
> > > > > 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?
> > >
> > > 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.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: VMX: Fix 32bit Windows blue screen with EPT
2010-12-30 8:57 ` Avi Kivity
2010-12-30 9:05 ` Sheng Yang
@ 2010-12-30 9:09 ` Sheng Yang
2010-12-30 9:16 ` Avi Kivity
2010-12-30 9:21 ` [PATCH v2] " Sheng Yang
2 siblings, 1 reply; 9+ messages in thread
From: Sheng Yang @ 2010-12-30 9:09 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, kvm
On Thursday 30 December 2010 16:57:20 Avi Kivity wrote:
> On 12/30/2010 10:35 AM, Sheng Yang wrote:
> > 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<sheng@linux.intel.com>
> > ---
> >
> > But I haven't found the exactly point affected by this, any clue?
>
> Can't see it either.
>
> > @@ -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.
>
> > 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?
BTW: What did you find when you added this two lines?
--
regards
Yang, Sheng
>
> > }
> >
> > if (!(cr0& X86_CR0_WP))
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: VMX: Fix 32bit Windows blue screen with EPT
2010-12-30 9:09 ` Sheng Yang
@ 2010-12-30 9:16 ` Avi Kivity
0 siblings, 0 replies; 9+ messages in thread
From: Avi Kivity @ 2010-12-30 9:16 UTC (permalink / raw)
To: Sheng Yang; +Cc: Marcelo Tosatti, kvm
On 12/30/2010 11:09 AM, Sheng Yang wrote:
> >
> > > 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?
>
> 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.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] KVM: VMX: Fix 32bit Windows blue screen with EPT
2010-12-30 8:57 ` Avi Kivity
2010-12-30 9:05 ` Sheng Yang
2010-12-30 9:09 ` Sheng Yang
@ 2010-12-30 9:21 ` Sheng Yang
2 siblings, 0 replies; 9+ messages in thread
From: Sheng Yang @ 2010-12-30 9:21 UTC (permalink / raw)
To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, 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 <sheng@linux.intel.com>
---
arch/x86/kvm/vmx.c | 6 +-----
1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index f107315..bf89ec2 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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))
--
1.7.0.1
^ permalink raw reply related [flat|nested] 9+ messages in thread