[PATCH] Fix to kvm_arch_vcpu_ioctl_set_sregs so that set_cr0 works properly

[PATCH] Fix to kvm_arch_vcpu_ioctl_set_sregs so that set_cr0 works properly

[Paul Knowles]

Whilst working on getting a VM to initialize in to IA32e mode I found  
this issue. set_cr0 relies on comparing the old cr0 to the new one to  
work correctly.

Whilst this fix gets me a little closer I still fail to run due to an  
invalid VMX state. Has anybody else ever tried to set up a VM to jump  
straight in to IA32e mode?

 From b5cbbfabb49eb6e2da6fef0a3d48620103040e4f Mon Sep 17 00:00:00 2001
From: Paul Knowles <paul-1LojSYAwM1QXQ3Lr6voeyA@public.gmane.org>
Date: Wed, 6 Feb 2008 10:30:29 +0000
Subject: [PATCH] Fix to kvm_arch_vcpu_ioctl_set_sregs so that set_cr0  
works properly


Signed-off-by: Paul Knowles <paul-1LojSYAwM1QXQ3Lr6voeyA@public.gmane.org>
---
  arch/x86/kvm/x86.c |    1 -
  1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0987191..64d541d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2866,7 +2866,6 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct  
kvm_vcpu *vcpu,
  	kvm_x86_ops->decache_cr4_guest_bits(vcpu);

  	mmu_reset_needed |= vcpu->arch.cr0 != sregs->cr0;
-	vcpu->arch.cr0 = sregs->cr0;
  	kvm_x86_ops->set_cr0(vcpu, sregs->cr0);

  	mmu_reset_needed |= vcpu->arch.cr4 != sregs->cr4;
-- 
1.5.3.6


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Fix to kvm_arch_vcpu_ioctl_set_sregs so that set_cr0 works properly

[Joerg Roedel]

On Wed, Feb 06, 2008 at 11:02:35AM +0000, Paul Knowles wrote:
>   	mmu_reset_needed |= vcpu->arch.cr0 != sregs->cr0;
> -	vcpu->arch.cr0 = sregs->cr0;
>   	kvm_x86_ops->set_cr0(vcpu, sregs->cr0);

Why should this fix anything? The vcpu->arch.cr0 is also set in
kvm_x86_ops->set_cr0() aka vmx_set_cr0() and its not touched on the path
to it?
Btw, I think its very likely that you set an invalid guest state with
your set_sregs ioctl (e.g. an impossible hardware register state).
Starting a guest in PAE mode is basically working because otherwise live
migration of PAE guests won't work.

Joerg

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Fix to kvm_arch_vcpu_ioctl_set_sregs so that set_cr0 works properly

[Paul Knowles]

Hi Joerg

The problem is this block of code in set_cr0

#ifdef CONFIG_X86_64
   if (vcpu->arch.shadow_efer & EFER_LME) {
     if (!is_paging(vcpu) && (cr0 & X86_CR0_PG))
       enter_lmode(vcpu);
     if (is_paging(vcpu) && !(cr0 & X86_CR0_PG))
       exit_lmode(vcpu);
   }
#endif

and

static inline int is_paging(struct kvm_vcpu *vcpu)
{
   return vcpu->arch.cr0 & X86_CR0_PG;
}

If we set vcpu->arch.cr0 = cr0 before calling it then the 2 if  
statements can never be true and we will never call enter_lmode or  
exit_lmode.

Paul

On 6 Feb 2008, at 20:21, Joerg Roedel wrote:

> On Wed, Feb 06, 2008 at 11:02:35AM +0000, Paul Knowles wrote:
>>  	mmu_reset_needed |= vcpu->arch.cr0 != sregs->cr0;
>> -	vcpu->arch.cr0 = sregs->cr0;
>>  	kvm_x86_ops->set_cr0(vcpu, sregs->cr0);
>
> Why should this fix anything? The vcpu->arch.cr0 is also set in
> kvm_x86_ops->set_cr0() aka vmx_set_cr0() and its not touched on the  
> path
> to it?
> Btw, I think its very likely that you set an invalid guest state with
> your set_sregs ioctl (e.g. an impossible hardware register state).
> Starting a guest in PAE mode is basically working because otherwise  
> live
> migration of PAE guests won't work.
>
> Joerg
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2008.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Fix to kvm_arch_vcpu_ioctl_set_sregs so that set_cr0 works properly

[Joerg Roedel]

On Thu, Feb 07, 2008 at 08:55:59AM +0000, Paul Knowles wrote:
> Hi Joerg
> 
> The problem is this block of code in set_cr0
> 
> #ifdef CONFIG_X86_64
>   if (vcpu->arch.shadow_efer & EFER_LME) {
>     if (!is_paging(vcpu) && (cr0 & X86_CR0_PG))
>       enter_lmode(vcpu);
>     if (is_paging(vcpu) && !(cr0 & X86_CR0_PG))
>       exit_lmode(vcpu);
>   }
> #endif

Hmm, the stuff that is done in enter_lmode() is also done in
vmx_set_efer() (except for the TSS fixup). I am still pretty sure it
should work without your fix (also because it will likely break SVM with
Nested Paging). Have you set EFER.LME *and* EFER.LMA when you called
set_sregs?

Joerg


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Fix to kvm_arch_vcpu_ioctl_set_sregs so that set_cr0 works properly

[Avi Kivity]

Paul Knowles wrote:
> Whilst working on getting a VM to initialize in to IA32e mode I found  
> this issue. set_cr0 relies on comparing the old cr0 to the new one to  
> work correctly.
>
>   

Applied, thanks.  I moved the assignment below rather than deleting it 
to be a little more conservative (svm_set_cr0 doesn't always actually 
set cr0 these days).

> Whilst this fix gets me a little closer I still fail to run due to an  
> invalid VMX state. Has anybody else ever tried to set up a VM to jump  
> straight in to IA32e mode?
>   

Sure.  Initially kvm only did long mode, and these days when you live 
migrate (or save/restore) a long mode guest you are effectively starting 
the new instance in long mode.

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/