[PATCH] KVM: MMU: Do not unconditionally read PDPTE from guest memory

[PATCH] KVM: MMU: Do not unconditionally read PDPTE from guest memory

[Avi Kivity]

Architecturally, PDPTEs are cached in the PDPTRs when CR3 is reloaded.
On SVM, it is not possible to implement this, but on VMX this is possible
and was indeed implemented until nested SVM changed this to unconditionally
read PDPTEs dynamically.  This has noticable impact when running PAE guests.

Fix by changing the MMU to read PDPTRs from the cache, falling back to
reading from memory for the nested MMU.

Signed-off-by: Avi Kivity <avi@redhat.com>
---

I was unable to test it because i386-on-i386 nsvm appears to be broken.
The immediate cause looks like fsbase leakage from guest to host.

 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/kvm_cache_regs.h   |    7 -------
 arch/x86/kvm/mmu.c              |    5 ++++-
 arch/x86/kvm/paging_tmpl.h      |    2 +-
 arch/x86/kvm/svm.c              |   15 +++++++++++++++
 5 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 307e3cf..b31a341 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -265,6 +265,7 @@ struct kvm_mmu {
 	void (*new_cr3)(struct kvm_vcpu *vcpu);
 	void (*set_cr3)(struct kvm_vcpu *vcpu, unsigned long root);
 	unsigned long (*get_cr3)(struct kvm_vcpu *vcpu);
+	u64 (*get_pdptr)(struct kvm_vcpu *vcpu, int index);
 	int (*page_fault)(struct kvm_vcpu *vcpu, gva_t gva, u32 err,
 			  bool prefault);
 	void (*inject_page_fault)(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index 3377d53..544076c 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -45,13 +45,6 @@ static inline u64 kvm_pdptr_read(struct kvm_vcpu *vcpu, int index)
 	return vcpu->arch.walk_mmu->pdptrs[index];
 }
 
-static inline u64 kvm_pdptr_read_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, int index)
-{
-	load_pdptrs(vcpu, mmu, mmu->get_cr3(vcpu));
-
-	return mmu->pdptrs[index];
-}
-
 static inline ulong kvm_read_cr0_bits(struct kvm_vcpu *vcpu, ulong mask)
 {
 	ulong tmask = mask & KVM_POSSIBLE_CR0_GUEST_BITS;
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 9335e1b..10060ac 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2770,7 +2770,7 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
 
 		ASSERT(!VALID_PAGE(root));
 		if (vcpu->arch.mmu.root_level == PT32E_ROOT_LEVEL) {
-			pdptr = kvm_pdptr_read_mmu(vcpu, &vcpu->arch.mmu, i);
+			pdptr = vcpu->arch.mmu.get_pdptr(vcpu, i);
 			if (!is_present_gpte(pdptr)) {
 				vcpu->arch.mmu.pae_root[i] = 0;
 				continue;
@@ -3318,6 +3318,7 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
 	context->direct_map = true;
 	context->set_cr3 = kvm_x86_ops->set_tdp_cr3;
 	context->get_cr3 = get_cr3;
+	context->get_pdptr = kvm_pdptr_read;
 	context->inject_page_fault = kvm_inject_page_fault;
 	context->nx = is_nx(vcpu);
 
@@ -3376,6 +3377,7 @@ static int init_kvm_softmmu(struct kvm_vcpu *vcpu)
 
 	vcpu->arch.walk_mmu->set_cr3           = kvm_x86_ops->set_cr3;
 	vcpu->arch.walk_mmu->get_cr3           = get_cr3;
+	vcpu->arch.walk_mmu->get_pdptr         = kvm_pdptr_read;
 	vcpu->arch.walk_mmu->inject_page_fault = kvm_inject_page_fault;
 
 	return r;
@@ -3386,6 +3388,7 @@ static int init_kvm_nested_mmu(struct kvm_vcpu *vcpu)
 	struct kvm_mmu *g_context = &vcpu->arch.nested_mmu;
 
 	g_context->get_cr3           = get_cr3;
+	g_context->get_pdptr         = kvm_pdptr_read;
 	g_context->inject_page_fault = kvm_inject_page_fault;
 
 	/*
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 507e2b8..f6dd9fe 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -163,7 +163,7 @@ retry_walk:
 
 #if PTTYPE == 64
 	if (walker->level == PT32E_ROOT_LEVEL) {
-		pte = kvm_pdptr_read_mmu(vcpu, mmu, (addr >> 30) & 3);
+		pte = mmu->get_pdptr(vcpu, (addr >> 30) & 3);
 		trace_kvm_mmu_paging_element(pte, walker->level);
 		if (!is_present_gpte(pte))
 			goto error;
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2b24a88..f043168 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1844,6 +1844,20 @@ static unsigned long nested_svm_get_tdp_cr3(struct kvm_vcpu *vcpu)
 	return svm->nested.nested_cr3;
 }
 
+static u64 nested_svm_get_tdp_pdptr(struct kvm_vcpu *vcpu, int index)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+	u64 cr3 = svm->nested.nested_cr3;
+	u64 pdpte;
+	int ret;
+
+	ret = kvm_read_guest_page(vcpu->kvm, gpa_to_gfn(cr3), &pdpte,
+				  offset_in_page(cr3) + index * 8, 8);
+	if (ret)
+		return 0;
+	return pdpte;
+}
+
 static void nested_svm_set_tdp_cr3(struct kvm_vcpu *vcpu,
 				   unsigned long root)
 {
@@ -1875,6 +1889,7 @@ static int nested_svm_init_mmu_context(struct kvm_vcpu *vcpu)
 
 	vcpu->arch.mmu.set_cr3           = nested_svm_set_tdp_cr3;
 	vcpu->arch.mmu.get_cr3           = nested_svm_get_tdp_cr3;
+	vcpu->arch.mmu.get_pdptr         = nested_svm_get_tdp_pdptr;
 	vcpu->arch.mmu.inject_page_fault = nested_svm_inject_npf_exit;
 	vcpu->arch.mmu.shadow_root_level = get_npt_level();
 	vcpu->arch.walk_mmu              = &vcpu->arch.nested_mmu;
-- 
1.7.5.3

Re: [PATCH] KVM: MMU: Do not unconditionally read PDPTE from guest memory

[Roedel, Joerg]

On Thu, Jul 28, 2011 at 04:36:17AM -0400, Avi Kivity wrote:
> Architecturally, PDPTEs are cached in the PDPTRs when CR3 is reloaded.
> On SVM, it is not possible to implement this, but on VMX this is possible
> and was indeed implemented until nested SVM changed this to unconditionally
> read PDPTEs dynamically.  This has noticable impact when running PAE guests.
> 
> Fix by changing the MMU to read PDPTRs from the cache, falling back to
> reading from memory for the nested MMU.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>

Hmm, interesting. Sorry for breaking it. I tested the patch on nested
svm, it works fine.

Tested-by: Joerg Roedel <joerg.roedel@amd.com>

-- 
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] KVM: MMU: Do not unconditionally read PDPTE from guest memory

[Avi Kivity]

On 07/29/2011 02:31 PM, Roedel, Joerg wrote:
> On Thu, Jul 28, 2011 at 04:36:17AM -0400, Avi Kivity wrote:
> >  Architecturally, PDPTEs are cached in the PDPTRs when CR3 is reloaded.
> >  On SVM, it is not possible to implement this, but on VMX this is possible
> >  and was indeed implemented until nested SVM changed this to unconditionally
> >  read PDPTEs dynamically.  This has noticable impact when running PAE guests.
> >
> >  Fix by changing the MMU to read PDPTRs from the cache, falling back to
> >  reading from memory for the nested MMU.
> >
> >  Signed-off-by: Avi Kivity<avi@redhat.com>
>
> Hmm, interesting. Sorry for breaking it. I tested the patch on nested
> svm, it works fine.

Does pae-on-pae work for you?

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

Re: [PATCH] KVM: MMU: Do not unconditionally read PDPTE from guest memory

[Roedel, Joerg]

On Sun, Jul 31, 2011 at 04:08:44AM -0400, Avi Kivity wrote:
> On 07/29/2011 02:31 PM, Roedel, Joerg wrote:
> > On Thu, Jul 28, 2011 at 04:36:17AM -0400, Avi Kivity wrote:
> > >  Architecturally, PDPTEs are cached in the PDPTRs when CR3 is reloaded.
> > >  On SVM, it is not possible to implement this, but on VMX this is possible
> > >  and was indeed implemented until nested SVM changed this to unconditionally
> > >  read PDPTEs dynamically.  This has noticable impact when running PAE guests.
> > >
> > >  Fix by changing the MMU to read PDPTRs from the cache, falling back to
> > >  reading from memory for the nested MMU.
> > >
> > >  Signed-off-by: Avi Kivity<avi@redhat.com>
> >
> > Hmm, interesting. Sorry for breaking it. I tested the patch on nested
> > svm, it works fine.
> 
> Does pae-on-pae work for you?

Only tested pae-on-longmode. I'll see if I can find my 32bit
installation again and test this too.

	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] KVM: MMU: Do not unconditionally read PDPTE from guest memory

[Avi Kivity]

On 08/02/2011 12:31 PM, Roedel, Joerg wrote:
> On Sun, Jul 31, 2011 at 04:08:44AM -0400, Avi Kivity wrote:
> >  On 07/29/2011 02:31 PM, Roedel, Joerg wrote:
> >  >  On Thu, Jul 28, 2011 at 04:36:17AM -0400, Avi Kivity wrote:
> >  >  >   Architecturally, PDPTEs are cached in the PDPTRs when CR3 is reloaded.
> >  >  >   On SVM, it is not possible to implement this, but on VMX this is possible
> >  >  >   and was indeed implemented until nested SVM changed this to unconditionally
> >  >  >   read PDPTEs dynamically.  This has noticable impact when running PAE guests.
> >  >  >
> >  >  >   Fix by changing the MMU to read PDPTRs from the cache, falling back to
> >  >  >   reading from memory for the nested MMU.
> >  >  >
> >  >  >   Signed-off-by: Avi Kivity<avi@redhat.com>
> >  >
> >  >  Hmm, interesting. Sorry for breaking it. I tested the patch on nested
> >  >  svm, it works fine.
> >
> >  Does pae-on-pae work for you?
>
> Only tested pae-on-longmode. I'll see if I can find my 32bit
> installation again and test this too.

I wanted to test it since any mixup in where the PTPTRs were taken from 
would be readily apparent.  But it crashes even without the patch.

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

Re: [PATCH] KVM: MMU: Do not unconditionally read PDPTE from guest memory

[Roedel, Joerg]

On Tue, Aug 02, 2011 at 08:34:32AM -0400, Avi Kivity wrote:
> On 08/02/2011 12:31 PM, Roedel, Joerg wrote:
> > On Sun, Jul 31, 2011 at 04:08:44AM -0400, Avi Kivity wrote:
> > >  On 07/29/2011 02:31 PM, Roedel, Joerg wrote:
> > >  >  On Thu, Jul 28, 2011 at 04:36:17AM -0400, Avi Kivity wrote:
> > >  >  >   Architecturally, PDPTEs are cached in the PDPTRs when CR3 is reloaded.
> > >  >  >   On SVM, it is not possible to implement this, but on VMX this is possible
> > >  >  >   and was indeed implemented until nested SVM changed this to unconditionally
> > >  >  >   read PDPTEs dynamically.  This has noticable impact when running PAE guests.
> > >  >  >
> > >  >  >   Fix by changing the MMU to read PDPTRs from the cache, falling back to
> > >  >  >   reading from memory for the nested MMU.
> > >  >  >
> > >  >  >   Signed-off-by: Avi Kivity<avi@redhat.com>
> > >  >
> > >  >  Hmm, interesting. Sorry for breaking it. I tested the patch on nested
> > >  >  svm, it works fine.
> > >
> > >  Does pae-on-pae work for you?
> >
> > Only tested pae-on-longmode. I'll see if I can find my 32bit
> > installation again and test this too.
> 
> I wanted to test it since any mixup in where the PTPTRs were taken from 
> would be readily apparent.  But it crashes even without the patch.

Hmm, it worked when it was merged. I'll take a look at it.

	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] KVM: MMU: Do not unconditionally read PDPTE from guest memory

[Roedel, Joerg]

On Tue, Aug 02, 2011 at 08:34:32AM -0400, Avi Kivity wrote:
> On 08/02/2011 12:31 PM, Roedel, Joerg wrote:
> > On Sun, Jul 31, 2011 at 04:08:44AM -0400, Avi Kivity wrote:
> > >  On 07/29/2011 02:31 PM, Roedel, Joerg wrote:
> > >  >  On Thu, Jul 28, 2011 at 04:36:17AM -0400, Avi Kivity wrote:
> > >  >  >   Architecturally, PDPTEs are cached in the PDPTRs when CR3 is reloaded.
> > >  >  >   On SVM, it is not possible to implement this, but on VMX this is possible
> > >  >  >   and was indeed implemented until nested SVM changed this to unconditionally
> > >  >  >   read PDPTEs dynamically.  This has noticable impact when running PAE guests.
> > >  >  >
> > >  >  >   Fix by changing the MMU to read PDPTRs from the cache, falling back to
> > >  >  >   reading from memory for the nested MMU.
> > >  >  >
> > >  >  >   Signed-off-by: Avi Kivity<avi@redhat.com>
> > >  >
> > >  >  Hmm, interesting. Sorry for breaking it. I tested the patch on nested
> > >  >  svm, it works fine.
> > >
> > >  Does pae-on-pae work for you?
> >
> > Only tested pae-on-longmode. I'll see if I can find my 32bit
> > installation again and test this too.
> 
> I wanted to test it since any mixup in where the PTPTRs were taken from 
> would be readily apparent.  But it crashes even without the patch.

Hmm, it works here on a Phenom II X6 box with and without the patch.
Host was your master-branch with pae enabled. The l1-guest was a vanilla
3.0-pae kernel and the l2-guest ran with 3.0-pae and 3.0-nonpae kernels.


	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] KVM: MMU: Do not unconditionally read PDPTE from guest memory

[Marcelo Tosatti]

On Thu, Jul 28, 2011 at 11:36:17AM +0300, Avi Kivity wrote:
> Architecturally, PDPTEs are cached in the PDPTRs when CR3 is reloaded.
> On SVM, it is not possible to implement this, but on VMX this is possible
> and was indeed implemented until nested SVM changed this to unconditionally
> read PDPTEs dynamically.  This has noticable impact when running PAE guests.
> 
> Fix by changing the MMU to read PDPTRs from the cache, falling back to
> reading from memory for the nested MMU.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>

Applied, thanks.