[PATCH 0/3] KVM fixes and cleanups

[PATCH 0/3] KVM fixes and cleanups

[Joerg Roedel]

Hi Avi, Marcelo,

here are 3 patches which came up during the final testing of the
nested-npt patch set. This patch set fixes two issues I found and the
last patch contains a minor cleanup which does not fix any real bug.
Please have a look at them and feel free to apply them (only if no
objections, of course ;-) )
For the bug that patch 2 fixes I will write a unit-test and submit it
separatly.

Thanks,
	Joerg

Shortlog:

Joerg Roedel (3):
      KVM: MMU: Fix 32 bit legacy paging with NPT
      KVM: SVM: Restore correct registers after sel_cr0 intercept emulation
      KVM: SVM: Clean up rip handling in vmrun emulation

Diffstat:

 arch/x86/kvm/mmu.c |    8 ++++++--
 arch/x86/kvm/svm.c |   39 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 40 insertions(+), 7 deletions(-)

[PATCH 1/3] KVM: MMU: Fix 32 bit legacy paging with NPT

[Joerg Roedel]

This patch fixes 32 bit legacy paging with NPT enabled. The
mmu_check_root call on the top-level of the loop causes
root_gfn to take values (in the tdp_enabled path) which are
outside of guest memory. So the mmu_check_root call fails at
some point in the loop interation causing the guest to
tiple-fault.
This patch changes the mmu_check_root calls to the places
where they are really necessary. As a side-effect it
introduces a check for the root of a pae page table too.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kvm/mmu.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index d2dad65..b2136f9 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2387,6 +2387,10 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu)
 		return 0;
 	}
 	direct = !is_paging(vcpu);
+
+	if (mmu_check_root(vcpu, root_gfn))
+		return 1;
+
 	for (i = 0; i < 4; ++i) {
 		hpa_t root = vcpu->arch.mmu.pae_root[i];
 
@@ -2398,10 +2402,10 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu)
 				continue;
 			}
 			root_gfn = pdptr >> PAGE_SHIFT;
+			if (mmu_check_root(vcpu, root_gfn))
+				return 1;
 		} else if (vcpu->arch.mmu.root_level == 0)
 			root_gfn = 0;
-		if (mmu_check_root(vcpu, root_gfn))
-			return 1;
 		if (tdp_enabled) {
 			direct = 1;
 			root_gfn = i << 30;
-- 
1.7.0.4

Re: [PATCH 1/3] KVM: MMU: Fix 32 bit legacy paging with NPT

[Avi Kivity]

  On 09/02/2010 06:29 PM, Joerg Roedel wrote:
> This patch fixes 32 bit legacy paging with NPT enabled. The
> mmu_check_root call on the top-level of the loop causes
> root_gfn to take values (in the tdp_enabled path) which are
> outside of guest memory. So the mmu_check_root call fails at
> some point in the loop interation causing the guest to
> tiple-fault.
> This patch changes the mmu_check_root calls to the places
> where they are really necessary. As a side-effect it
> introduces a check for the root of a pae page table too.
>
>
> @@ -2387,6 +2387,10 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu)
>   		return 0;
>   	}
>   	direct = !is_paging(vcpu);
> +
> +	if (mmu_check_root(vcpu, root_gfn))
> +		return 1;
> +
>   	for (i = 0; i<  4; ++i) {
>   		hpa_t root = vcpu->arch.mmu.pae_root[i];
>
> @@ -2398,10 +2402,10 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu)
>   				continue;
>   			}
>   			root_gfn = pdptr>>  PAGE_SHIFT;
> +			if (mmu_check_root(vcpu, root_gfn))
> +				return 1;
>   		} else if (vcpu->arch.mmu.root_level == 0)
>   			root_gfn = 0;
> -		if (mmu_check_root(vcpu, root_gfn))
> -			return 1;
>   		if (tdp_enabled) {
>   			direct = 1;
>   			root_gfn = i<<  30;

The overloading of root_gfn is pretty bad.  Also, we don't really need 
to check root_gfn for the direct case (the guest can easily switch cr3 
later to one that would fail the check).

However, I'll apply the patch since it fixes the direct problem.  More 
involved fixes can come later (esp. after the nnpt patches land).


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

Re: [PATCH 1/3] KVM: MMU: Fix 32 bit legacy paging with NPT

[Roedel, Joerg]

On Thu, Sep 02, 2010 at 11:56:10AM -0400, Avi Kivity wrote:
>   On 09/02/2010 06:29 PM, Joerg Roedel wrote:
> > This patch fixes 32 bit legacy paging with NPT enabled. The
> > mmu_check_root call on the top-level of the loop causes
> > root_gfn to take values (in the tdp_enabled path) which are
> > outside of guest memory. So the mmu_check_root call fails at
> > some point in the loop interation causing the guest to
> > tiple-fault.
> > This patch changes the mmu_check_root calls to the places
> > where they are really necessary. As a side-effect it
> > introduces a check for the root of a pae page table too.
> >
> >
> > @@ -2387,6 +2387,10 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu)
> >   		return 0;
> >   	}
> >   	direct = !is_paging(vcpu);
> > +
> > +	if (mmu_check_root(vcpu, root_gfn))
> > +		return 1;
> > +
> >   	for (i = 0; i<  4; ++i) {
> >   		hpa_t root = vcpu->arch.mmu.pae_root[i];
> >
> > @@ -2398,10 +2402,10 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu)
> >   				continue;
> >   			}
> >   			root_gfn = pdptr>>  PAGE_SHIFT;
> > +			if (mmu_check_root(vcpu, root_gfn))
> > +				return 1;
> >   		} else if (vcpu->arch.mmu.root_level == 0)
> >   			root_gfn = 0;
> > -		if (mmu_check_root(vcpu, root_gfn))
> > -			return 1;
> >   		if (tdp_enabled) {
> >   			direct = 1;
> >   			root_gfn = i<<  30;
> 
> The overloading of root_gfn is pretty bad.  Also, we don't really need 
> to check root_gfn for the direct case (the guest can easily switch cr3 
> later to one that would fail the check).
> 
> However, I'll apply the patch since it fixes the direct problem.  More 
> involved fixes can come later (esp. after the nnpt patches land).

Ok, great. I will rebase my nnpt patches on-top of this and do some more
final testing before I post them.

Thanks,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 2/3] KVM: SVM: Restore correct registers after sel_cr0 intercept emulation

[Joerg Roedel]

This patch implements restoring of the correct rip, rsp, and
rax after the svm emulation in KVM injected a selective_cr0
write intercept into the guest hypervisor. The problem was
that the vmexit is emulated in the instruction emulation
which later commits the registers right after the write-cr0
instruction. So the l1 guest will continue to run with the
l2 rip, rsp and rax resulting in unpredictable behavior.

This patch is not the final word, it is just an easy patch
to fix the issue. The real fix will be done when the
instruction emulator is made aware of nested virtualization.
Until this is done this patch fixes the issue and provides
an easy way to fix this in -stable too.

Cc: stable@kernel.org
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kvm/svm.c |   33 +++++++++++++++++++++++++++++++--
 1 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 55743ab..ecd4e58 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -88,6 +88,14 @@ struct nested_state {
 	/* A VMEXIT is required but not yet emulated */
 	bool exit_required;
 
+	/*
+	 * If we vmexit during an instruction emulation we need this to restore
+	 * the l1 guest rip after the emulation
+	 */
+	unsigned long vmexit_rip;
+	unsigned long vmexit_rsp;
+	unsigned long vmexit_rax;
+
 	/* cache for intercepts of the guest */
 	u16 intercept_cr_read;
 	u16 intercept_cr_write;
@@ -1213,8 +1221,12 @@ static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 		if (old == new) {
 			/* cr0 write with ts and mp unchanged */
 			svm->vmcb->control.exit_code = SVM_EXIT_CR0_SEL_WRITE;
-			if (nested_svm_exit_handled(svm) == NESTED_EXIT_DONE)
+			if (nested_svm_exit_handled(svm) == NESTED_EXIT_DONE) {
+				svm->nested.vmexit_rip = kvm_rip_read(vcpu);
+				svm->nested.vmexit_rsp = kvm_register_read(vcpu, VCPU_REGS_RSP);
+				svm->nested.vmexit_rax = kvm_register_read(vcpu, VCPU_REGS_RAX);
 				return;
+			}
 		}
 	}
 
@@ -2430,6 +2442,23 @@ static int emulate_on_interception(struct vcpu_svm *svm)
 	return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
 }
 
+static int cr0_write_interception(struct vcpu_svm *svm)
+{
+	struct kvm_vcpu *vcpu = &svm->vcpu;
+	int r;
+
+	r = emulate_instruction(&svm->vcpu, 0, 0, 0);
+
+	if (svm->nested.vmexit_rip) {
+		kvm_register_write(vcpu, VCPU_REGS_RIP, svm->nested.vmexit_rip);
+		kvm_register_write(vcpu, VCPU_REGS_RSP, svm->nested.vmexit_rsp);
+		kvm_register_write(vcpu, VCPU_REGS_RAX, svm->nested.vmexit_rax);
+		svm->nested.vmexit_rip = 0;
+	}
+
+	return r == EMULATE_DONE;
+}
+
 static int cr8_write_interception(struct vcpu_svm *svm)
 {
 	struct kvm_run *kvm_run = svm->vcpu.run;
@@ -2692,7 +2721,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm) = {
 	[SVM_EXIT_READ_CR4]			= emulate_on_interception,
 	[SVM_EXIT_READ_CR8]			= emulate_on_interception,
 	[SVM_EXIT_CR0_SEL_WRITE]		= emulate_on_interception,
-	[SVM_EXIT_WRITE_CR0]			= emulate_on_interception,
+	[SVM_EXIT_WRITE_CR0]			= cr0_write_interception,
 	[SVM_EXIT_WRITE_CR3]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_CR4]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_CR8]			= cr8_write_interception,
-- 
1.7.0.4

Re: [PATCH 2/3] KVM: SVM: Restore correct registers after sel_cr0 intercept emulation

[Avi Kivity]

  On 09/02/2010 06:29 PM, Joerg Roedel wrote:
> This patch implements restoring of the correct rip, rsp, and
> rax after the svm emulation in KVM injected a selective_cr0
> write intercept into the guest hypervisor. The problem was
> that the vmexit is emulated in the instruction emulation
> which later commits the registers right after the write-cr0
> instruction. So the l1 guest will continue to run with the
> l2 rip, rsp and rax resulting in unpredictable behavior.
>

Please post a unit test for this.

> This patch is not the final word, it is just an easy patch
> to fix the issue. The real fix will be done when the
> instruction emulator is made aware of nested virtualization.
> Until this is done this patch fixes the issue and provides
> an easy way to fix this in -stable too.

I agree.  We can probably use X86EMUL_PROPAGATE_FAULT to abort 
emulation, but looking at the code, it will take some refactoring.

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

Re: [PATCH 2/3] KVM: SVM: Restore correct registers after sel_cr0 intercept emulation

[Roedel, Joerg]

On Thu, Sep 02, 2010 at 12:02:02PM -0400, Avi Kivity wrote:
>   On 09/02/2010 06:29 PM, Joerg Roedel wrote:
> > This patch implements restoring of the correct rip, rsp, and
> > rax after the svm emulation in KVM injected a selective_cr0
> > write intercept into the guest hypervisor. The problem was
> > that the vmexit is emulated in the instruction emulation
> > which later commits the registers right after the write-cr0
> > instruction. So the l1 guest will continue to run with the
> > l2 rip, rsp and rax resulting in unpredictable behavior.
> 
> Please post a unit test for this.

Will do. Should be an easy test.

> > This patch is not the final word, it is just an easy patch
> > to fix the issue. The real fix will be done when the
> > instruction emulator is made aware of nested virtualization.
> > Until this is done this patch fixes the issue and provides
> > an easy way to fix this in -stable too.
> 
> I agree.  We can probably use X86EMUL_PROPAGATE_FAULT to abort 
> emulation, but looking at the code, it will take some refactoring.

I thought of an X86EMUL_INTERCEPTED. An architecture specific function
is called after instruction decoding which checks if an intercept is
necessary. If it returns X86EMUL_INTERCEPTED then the instruction
emulation is discarded and kvm goes straight back into the guest.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 2/3] KVM: SVM: Restore correct registers after sel_cr0 intercept emulation

[Avi Kivity]

  On 09/02/2010 07:29 PM, Roedel, Joerg wrote:
>
>> I agree.  We can probably use X86EMUL_PROPAGATE_FAULT to abort
>> emulation, but looking at the code, it will take some refactoring.
> I thought of an X86EMUL_INTERCEPTED. An architecture specific function
> is called after instruction decoding which checks if an intercept is
> necessary. If it returns X86EMUL_INTERCEPTED then the instruction
> emulation is discarded and kvm goes straight back into the guest.

Yes, this sounds just right.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[PATCH 3/3] KVM: SVM: Clean up rip handling in vmrun emulation

[Joerg Roedel]

This patch changes the rip handling in the vmrun emulation
path from using next_rip to the generic kvm register access
functions.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kvm/svm.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ecd4e58..1643f30 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2069,7 +2069,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 		return false;
 	}
 
-	trace_kvm_nested_vmrun(svm->vmcb->save.rip - 3, vmcb_gpa,
+	trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa,
 			       nested_vmcb->save.rip,
 			       nested_vmcb->control.int_ctl,
 			       nested_vmcb->control.event_inj,
@@ -2270,8 +2270,8 @@ static int vmrun_interception(struct vcpu_svm *svm)
 	if (nested_svm_check_permissions(svm))
 		return 1;
 
-	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
-	skip_emulated_instruction(&svm->vcpu);
+	/* Save rip after vmrun instruction */
+	kvm_rip_write(&svm->vcpu, kvm_rip_read(&svm->vcpu) + 3);
 
 	if (!nested_svm_vmrun(svm))
 		return 1;
-- 
1.7.0.4

Re: [PATCH 3/3] KVM: SVM: Clean up rip handling in vmrun emulation

[Roedel, Joerg]

On Thu, Sep 02, 2010 at 11:29:47AM -0400, Joerg Roedel wrote:
> This patch changes the rip handling in the vmrun emulation
> path from using next_rip to the generic kvm register access
> functions.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  arch/x86/kvm/svm.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ecd4e58..1643f30 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2069,7 +2069,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
>  		return false;
>  	}
>  
> -	trace_kvm_nested_vmrun(svm->vmcb->save.rip - 3, vmcb_gpa,
> +	trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa,
>  			       nested_vmcb->save.rip,
>  			       nested_vmcb->control.int_ctl,
>  			       nested_vmcb->control.event_inj,
> @@ -2270,8 +2270,8 @@ static int vmrun_interception(struct vcpu_svm *svm)
>  	if (nested_svm_check_permissions(svm))
>  		return 1;
>  
> -	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> -	skip_emulated_instruction(&svm->vcpu);
> +	/* Save rip after vmrun instruction */
> +	kvm_rip_write(&svm->vcpu, kvm_rip_read(&svm->vcpu) + 3);
>  
>  	if (!nested_svm_vmrun(svm))
>  		return 1;

Argh, in my interactive commit I forgot one part of this patch. Please
apply the attached one instead.


>From 42450df2b72c23538d61616834dbdf1b53deafd7 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel@amd.com>
Date: Thu, 2 Sep 2010 17:12:18 +0200
Subject: [PATCH 3/3] KVM: SVM: Clean up rip handling in vmrun emulation

This patch changes the rip handling in the vmrun emulation
path from using next_rip to the generic kvm register access
functions.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kvm/svm.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ecd4e58..6808f64 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2069,7 +2069,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 		return false;
 	}
 
-	trace_kvm_nested_vmrun(svm->vmcb->save.rip - 3, vmcb_gpa,
+	trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa,
 			       nested_vmcb->save.rip,
 			       nested_vmcb->control.int_ctl,
 			       nested_vmcb->control.event_inj,
@@ -2098,7 +2098,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 	hsave->save.cr0    = kvm_read_cr0(&svm->vcpu);
 	hsave->save.cr4    = svm->vcpu.arch.cr4;
 	hsave->save.rflags = vmcb->save.rflags;
-	hsave->save.rip    = svm->next_rip;
+	hsave->save.rip    = kvm_rip_read(&svm->vcpu);
 	hsave->save.rsp    = vmcb->save.rsp;
 	hsave->save.rax    = vmcb->save.rax;
 	if (npt_enabled)
@@ -2270,8 +2270,8 @@ static int vmrun_interception(struct vcpu_svm *svm)
 	if (nested_svm_check_permissions(svm))
 		return 1;
 
-	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
-	skip_emulated_instruction(&svm->vcpu);
+	/* Save rip after vmrun instruction */
+	kvm_rip_write(&svm->vcpu, kvm_rip_read(&svm->vcpu) + 3);
 
 	if (!nested_svm_vmrun(svm))
 		return 1;
-- 
1.7.0.4


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 3/3] KVM: SVM: Clean up rip handling in vmrun emulation

[Alexander Graf]

On 03.09.2010, at 14:21, Roedel, Joerg wrote:

> On Thu, Sep 02, 2010 at 11:29:47AM -0400, Joerg Roedel wrote:
>> This patch changes the rip handling in the vmrun emulation
>> path from using next_rip to the generic kvm register access
>> functions.
>> 
>> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>> ---
>> arch/x86/kvm/svm.c |    6 +++---
>> 1 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index ecd4e58..1643f30 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2069,7 +2069,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
>> 		return false;
>> 	}
>> 
>> -	trace_kvm_nested_vmrun(svm->vmcb->save.rip - 3, vmcb_gpa,
>> +	trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa,
>> 			       nested_vmcb->save.rip,
>> 			       nested_vmcb->control.int_ctl,
>> 			       nested_vmcb->control.event_inj,
>> @@ -2270,8 +2270,8 @@ static int vmrun_interception(struct vcpu_svm *svm)
>> 	if (nested_svm_check_permissions(svm))
>> 		return 1;
>> 
>> -	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
>> -	skip_emulated_instruction(&svm->vcpu);
>> +	/* Save rip after vmrun instruction */
>> +	kvm_rip_write(&svm->vcpu, kvm_rip_read(&svm->vcpu) + 3);
>> 
>> 	if (!nested_svm_vmrun(svm))
>> 		return 1;
> 
> Argh, in my interactive commit I forgot one part of this patch. Please
> apply the attached one instead.
> 
> 
> From 42450df2b72c23538d61616834dbdf1b53deafd7 Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <joerg.roedel@amd.com>
> Date: Thu, 2 Sep 2010 17:12:18 +0200
> Subject: [PATCH 3/3] KVM: SVM: Clean up rip handling in vmrun emulation
> 
> This patch changes the rip handling in the vmrun emulation
> path from using next_rip to the generic kvm register access
> functions.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
> arch/x86/kvm/svm.c |    8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ecd4e58..6808f64 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2069,7 +2069,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
> 		return false;
> 	}
> 
> -	trace_kvm_nested_vmrun(svm->vmcb->save.rip - 3, vmcb_gpa,
> +	trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa,
> 			       nested_vmcb->save.rip,
> 			       nested_vmcb->control.int_ctl,
> 			       nested_vmcb->control.event_inj,
> @@ -2098,7 +2098,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
> 	hsave->save.cr0    = kvm_read_cr0(&svm->vcpu);
> 	hsave->save.cr4    = svm->vcpu.arch.cr4;
> 	hsave->save.rflags = vmcb->save.rflags;
> -	hsave->save.rip    = svm->next_rip;
> +	hsave->save.rip    = kvm_rip_read(&svm->vcpu);
> 	hsave->save.rsp    = vmcb->save.rsp;
> 	hsave->save.rax    = vmcb->save.rax;
> 	if (npt_enabled)
> @@ -2270,8 +2270,8 @@ static int vmrun_interception(struct vcpu_svm *svm)
> 	if (nested_svm_check_permissions(svm))
> 		return 1;
> 
> -	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> -	skip_emulated_instruction(&svm->vcpu);
> +	/* Save rip after vmrun instruction */
> +	kvm_rip_write(&svm->vcpu, kvm_rip_read(&svm->vcpu) + 3);


Any reason we can't use the next_rip information here? A hypervisor could potentially do badness and put a prefix here, thus break all the logic, right?

(yes, I know, I wrote that code, but still ...)


Alex

Re: [PATCH 3/3] KVM: SVM: Clean up rip handling in vmrun emulation

[Joerg Roedel]

On Fri, Sep 03, 2010 at 11:29:11PM +0200, Alexander Graf wrote:
> Any reason we can't use the next_rip information here? A hypervisor
> could potentially do badness and put a prefix here, thus break all the
> logic, right?

Next_rip is not available on older hardware. Yes, this problem exists in
theory (as it does with rdmsr/wrmsr, cpuid, ... too). But a fix for this
on non-next-rip capable hardware would involve the instruction emulator
and kills all performance there. So we use this stupid and fast solution
here which works for all hypervisors I tested :-)

> (yes, I know, I wrote that code, but still ...)

When you wrote that code next_rip capable hardware was not available, so
don't worry :-)


	Joerg

Re: [PATCH 0/3] KVM fixes and cleanups

[Marcelo Tosatti]

On Thu, Sep 02, 2010 at 05:29:44PM +0200, Joerg Roedel wrote:
> Hi Avi, Marcelo,
> 
> here are 3 patches which came up during the final testing of the
> nested-npt patch set. This patch set fixes two issues I found and the
> last patch contains a minor cleanup which does not fix any real bug.
> Please have a look at them and feel free to apply them (only if no
> objections, of course ;-) )
> For the bug that patch 2 fixes I will write a unit-test and submit it
> separatly.
> 
> Thanks,
> 	Joerg
> 
> Shortlog:
> 
> Joerg Roedel (3):
>       KVM: MMU: Fix 32 bit legacy paging with NPT
>       KVM: SVM: Restore correct registers after sel_cr0 intercept emulation
>       KVM: SVM: Clean up rip handling in vmrun emulation
> 
> Diffstat:
> 
>  arch/x86/kvm/mmu.c |    8 ++++++--
>  arch/x86/kvm/svm.c |   39 ++++++++++++++++++++++++++++++++++-----
>  2 files changed, 40 insertions(+), 7 deletions(-)

Applied, thanks.