Skip to contentSkip to conversation

KVM: MMU: update 'root_hpa' out of loop in PAE shadow path

PATCH In review Xiao Guangrong 2 participants 14 messages Sep 27, 2010 – Sep 28, 2010

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Xiao Guangrong

author
#0 lore raw

PATCH 1/7 KVM: MMU: update 'root_hpa' out of loop in PAE shadow path

The value of 'vcpu->arch.mmu.pae_root' is not modified, so we can update 'root_hpa' out of the loop

Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/mmu.c +2 -2
index c94c432..3630046 100644
@@ -2393,8 +2393,8 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
 			++sp->root_count;
 			spin_unlock(&vcpu->kvm->mmu_lock);
 			vcpu->arch.mmu.pae_root[i] = root | PT_PRESENT_MASK;
-			vcpu->arch.mmu.root_hpa = __pa(vcpu->arch.mmu.pae_root);
 		}
+		vcpu->arch.mmu.root_hpa = __pa(vcpu->arch.mmu.pae_root);
 	} else
 		BUG();
 

@@ -2466,8 +2466,8 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
 		spin_unlock(&vcpu->kvm->mmu_lock);
 
 		vcpu->arch.mmu.pae_root[i] = root | pm_mask;
-		vcpu->arch.mmu.root_hpa = __pa(vcpu->arch.mmu.pae_root);
 	}
+	vcpu->arch.mmu.root_hpa = __pa(vcpu->arch.mmu.pae_root);
 
 	/*
 	 * If we shadow a 32 bit page table with a long mode page
Signature · 1 line
1.7.0.4
author
#1 lore raw

PATCH 2/7 KVM: MMU: cleanup for error mask set while walk guest page table

Small cleanup for set page fault error code

Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/paging_tmpl.h |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)
arch/x86/kvm/paging_tmpl.h +7 -10
index 2bdd843..a83ff37 100644
@@ -224,9 +224,7 @@ walk:
 			    is_cpuid_PSE36())
 				gfn += pse36_gfn_delta(pte);
 
-			access |= write_fault ? PFERR_WRITE_MASK : 0;
-			access |= fetch_fault ? PFERR_FETCH_MASK : 0;
-			access |= user_fault  ? PFERR_USER_MASK  : 0;
+			access |= write_fault | fetch_fault | user_fault;
 
 			real_gpa = mmu->translate_gpa(vcpu, gfn_to_gpa(gfn),
 						      access);

@@ -268,10 +266,9 @@ error:
 	walker->error_code = 0;
 	if (present)
 		walker->error_code |= PFERR_PRESENT_MASK;
-	if (write_fault)
-		walker->error_code |= PFERR_WRITE_MASK;
-	if (user_fault)
-		walker->error_code |= PFERR_USER_MASK;
+
+	walker->error_code |= write_fault | user_fault;
+
 	if (fetch_fault && mmu->nx)
 		walker->error_code |= PFERR_FETCH_MASK;
 	if (rsvd_fault)

@@ -673,9 +670,9 @@ static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, gva_t vaddr, u32 access,
 	int r;
 
 	r = FNAME(walk_addr)(&walker, vcpu, vaddr,
-			     !!(access & PFERR_WRITE_MASK),
-			     !!(access & PFERR_USER_MASK),
-			     !!(access & PFERR_FETCH_MASK));
+			     access & PFERR_WRITE_MASK,
+			     access & PFERR_USER_MASK,
+			     access & PFERR_FETCH_MASK);
 
 	if (r) {
 		gpa = gfn_to_gpa(walker.gfn);
Signature · 1 line
1.7.0.4
#2 lore raw

Re: [PATCH 2/7] KVM: MMU: cleanup for error mask set while walk guest page table

Quoted 39 lines · Xiao Guangrong

Small cleanup for set page fault error code

Xiao Guangrong<[email protected]>

---
  arch/x86/kvm/paging_tmpl.h |   17 +++++++----------
  1 files changed, 7 insertions(+), 10 deletions(-)
arch/x86/kvm/paging_tmpl.h +4 -7
index 2bdd843..a83ff37 100644
@@ -224,9 +224,7 @@ walk:
  			    is_cpuid_PSE36())
  				gfn += pse36_gfn_delta(pte);
 
-			access |= write_fault ? PFERR_WRITE_MASK : 0;
-			access |= fetch_fault ? PFERR_FETCH_MASK : 0;
-			access |= user_fault  ? PFERR_USER_MASK  : 0;
+			access |= write_fault | fetch_fault | user_fault;
 
  			real_gpa = mmu->translate_gpa(vcpu, gfn_to_gpa(gfn),
  						      access);

@@ -268,10 +266,9 @@ error:
  	walker->error_code = 0;
  	if (present)
  		walker->error_code |= PFERR_PRESENT_MASK;
-	if (write_fault)
-		walker->error_code |= PFERR_WRITE_MASK;
-	if (user_fault)
-		walker->error_code |= PFERR_USER_MASK;
+
+	walker->error_code |= write_fault | user_fault;
+
  	if (fetch_fault&&  mmu->nx)
  		walker->error_code |= PFERR_FETCH_MASK;
  	if (rsvd_fault)

@@ -673,9 +670,9 @@ static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, gva_t vaddr, u32 access,
  	int r;

arch/x86/kvm/paging_tmpl.h static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, gva_t vaddr, u32 access, · line 678

Quoted 10 lines
arch/x86/kvm/paging_tmpl.h +3 -3
  	r = FNAME(walk_addr)(&walker, vcpu, vaddr,
-			     !!(access&  PFERR_WRITE_MASK),
-			     !!(access&  PFERR_USER_MASK),
-			     !!(access&  PFERR_FETCH_MASK));
+			     access&  PFERR_WRITE_MASK,
+			     access&  PFERR_USER_MASK,
+			     access&  PFERR_FETCH_MASK);
 
  	if (r) {
  		gpa = gfn_to_gpa(walker.gfn);

Interesting. Maybe a next step is to pass the page-fault error code instead of the various bits? Not sure how that interacts with nested ept (which has a different permission model).

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#3 lore raw

Re: [PATCH 2/7] KVM: MMU: cleanup for error mask set while walk guest page table

Quoted 8 lines · Avi Kivity
Quoted 8 lines

r = FNAME(walk_addr)(&walker, vcpu, vaddr,

(quoted diff) +3 -3
-                 !!(access&  PFERR_WRITE_MASK),
-                 !!(access&  PFERR_USER_MASK),
-                 !!(access&  PFERR_FETCH_MASK));
+                 access&  PFERR_WRITE_MASK,
+                 access&  PFERR_USER_MASK,
+                 access&  PFERR_FETCH_MASK);
Quoted 5 lines
Quoted 2 lines
      if (r) {
          gpa = gfn_to_gpa(walker.gfn);

Interesting. Maybe a next step is to pass the page-fault error code instead of the various bits?

Yeah, it's a good idea, i'll post a patch to do it.

Quoted 3 lines

Not sure how that interacts with nested
ept (which has a different permission model).

Umm, we just move the error code parsing from the caller site to FNAME(walk_addr) function, i think it not make trouble for implement nested ept.

#4 lore raw

Re: [PATCH 2/7] KVM: MMU: cleanup for error mask set while walk guest page table

Quoted 8 lines · Xiao Guangrong

Yeah, it's a good idea, i'll post a patch to do it.

Quoted 3 lines

Not sure how that interacts with nested
ept (which has a different permission model).

Umm, we just move the error code parsing from the caller site to FNAME(walk_addr) function, i think it not make trouble for implement nested ept.

It means the nept code will have to take the ept error code and convert it to the standard error code. Not too difficult.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#5 lore raw

PATCH 3/7 KVM: MMU: set access bit for direct mapping

In reply to Xiao Guangrong

Set access bit while setup up direct page table if it's nonpaing or npt enabled, it's good for CPU's speculate access

Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
arch/x86/kvm/mmu.c +2 -1
index 3630046..88203fa 100644
@@ -2240,7 +2240,8 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			__set_spte(iterator.sptep,
 				   __pa(sp->spt)
 				   | PT_PRESENT_MASK | PT_WRITABLE_MASK
-				   | shadow_user_mask | shadow_x_mask);
+				   | shadow_user_mask | shadow_x_mask
+				   | shadow_accessed_mask);
 		}
 	}
 	return pt_write;
Signature · 1 line
1.7.0.4
author
#6 lore raw

PATCH 5/7 KVM: MMU: audit: unregister audit tracepoints before module unloaded

In reply to Xiao Guangrong

fix:

Call Trace:
 [<ffffffffa01e46ba>] ? kvm_mmu_pte_write+0x229/0x911 [kvm]
 [<ffffffffa01c6ba9>] ? gfn_to_memslot+0x39/0xa0 [kvm]
 [<ffffffffa01c6c26>] ? mark_page_dirty+0x16/0x2e [kvm]
 [<ffffffffa01c6d6f>] ? kvm_write_guest_page+0x67/0x7f [kvm]
 [<ffffffff81066fbd>] ? local_clock+0x2a/0x3b
 [<ffffffffa01d52ce>] emulator_write_phys+0x46/0x54 [kvm]
 ......
Code:  Bad RIP value.

RIP [<ffffffffa0172056>] 0xffffffffa0172056
RSP <ffff880134f69a70>
CR2: ffffffffa0172056

Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)
arch/x86/kvm/mmu.c +12 -9
index 88203fa..afde64b 100644
@@ -3355,15 +3355,6 @@ int kvm_mmu_setup(struct kvm_vcpu *vcpu)
 	return init_kvm_mmu(vcpu);
 }
 
-void kvm_mmu_destroy(struct kvm_vcpu *vcpu)
-{
-	ASSERT(vcpu);
-
-	destroy_kvm_mmu(vcpu);
-	free_mmu_pages(vcpu);
-	mmu_free_memory_caches(vcpu);
-}
-
 void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
 {
 	struct kvm_mmu_page *sp;

@@ -3662,4 +3653,16 @@ EXPORT_SYMBOL_GPL(kvm_mmu_get_spte_hierarchy);
 
 #ifdef CONFIG_KVM_MMU_AUDIT
 #include "mmu_audit.c"
+#else
+static void mmu_audit_disable(void) { }
 #endif
+
+void kvm_mmu_destroy(struct kvm_vcpu *vcpu)
+{
+	ASSERT(vcpu);
+
+	destroy_kvm_mmu(vcpu);
+	free_mmu_pages(vcpu);
+	mmu_free_memory_caches(vcpu);
+	mmu_audit_disable();
+}
Signature · 1 line
1.7.0.4
author
#7 lore raw

PATCH 6/7 KVM: MMU: audit: introduce audit_printk to cleanup audit code

In reply to Xiao Guangrong

Introduce audit_printk, and record audit point instead audit name

Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu_audit.c |   42 ++++++++++++++++++------------------------
 1 files changed, 18 insertions(+), 24 deletions(-)
arch/x86/kvm/mmu_audit.c +18 -24
index dcca3e7..66219af 100644
@@ -19,7 +19,11 @@
 
 #include <linux/ratelimit.h>
 
-static const char *audit_msg;
+static int audit_point;
+
+#define audit_printk(fmt, args...)		\
+	printk(KERN_ERR "audit: (%s) error: "	\
+		fmt, audit_point_name[audit_point], ##args)
 
 typedef void (*inspect_spte_fn) (struct kvm_vcpu *vcpu, u64 *sptep, int level);
 

@@ -93,21 +97,18 @@ static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 
 	if (sp->unsync) {
 		if (level != PT_PAGE_TABLE_LEVEL) {
-			printk(KERN_ERR "audit: (%s) error: unsync sp: %p level = %d\n",
-				audit_msg, sp, level);
+			audit_printk("unsync sp: %p level = %d\n", sp, level);
 			return;
 		}
 
 		if (*sptep == shadow_notrap_nonpresent_pte) {
-			printk(KERN_ERR "audit: (%s) error: notrap spte in unsync sp: %p\n",
-				audit_msg, sp);
+			audit_printk("notrap spte in unsync sp: %p\n", sp);
 			return;
 		}
 	}
 
 	if (sp->role.direct && *sptep == shadow_notrap_nonpresent_pte) {
-		printk(KERN_ERR "audit: (%s) error: notrap spte in direct sp: %p\n",
-			audit_msg, sp);
+		audit_printk("notrap spte in direct sp: %p\n", sp);
 		return;
 	}
 

@@ -124,10 +125,8 @@ static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 
 	hpa =  pfn << PAGE_SHIFT;
 	if ((*sptep & PT64_BASE_ADDR_MASK) != hpa)
-		printk(KERN_ERR "xx audit error: (%s) levels %d"
-				   "pfn %llx hpa %llx ent %llxn",
-				   audit_msg, vcpu->arch.mmu.root_level,
-				   pfn, hpa, *sptep);
+		audit_printk("levels %d pfn %llx hpa %llx ent %llxn",
+				   vcpu->arch.mmu.root_level, pfn, hpa, *sptep);
 }
 
 static void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)

@@ -143,11 +142,9 @@ static void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
 	if (!gfn_to_memslot(kvm, gfn)) {
 		if (!printk_ratelimit())
 			return;
-		printk(KERN_ERR "%s: no memslot for gfn %llx\n",
-				 audit_msg, gfn);
-		printk(KERN_ERR "%s: index %ld of sp (gfn=%llx)\n",
-		       audit_msg, (long int)(sptep - rev_sp->spt),
-				rev_sp->gfn);
+		audit_printk("no memslot for gfn %llx\n", gfn);
+		audit_printk("index %ld of sp (gfn=%llx)\n",
+		       (long int)(sptep - rev_sp->spt), rev_sp->gfn);
 		dump_stack();
 		return;
 	}

@@ -156,8 +153,7 @@ static void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
 	if (!*rmapp) {
 		if (!printk_ratelimit())
 			return;
-		printk(KERN_ERR "%s: no rmap for writable spte %llx\n",
-				 audit_msg, *sptep);
+		audit_printk("no rmap for writable spte %llx\n", *sptep);
 		dump_stack();
 	}
 }

@@ -198,10 +194,8 @@ void audit_write_protection(struct kvm *kvm, struct kvm_mmu_page *sp)
 	spte = rmap_next(kvm, rmapp, NULL);
 	while (spte) {
 		if (is_writable_pte(*spte))
-			printk(KERN_ERR "%s: (%s) shadow page has "
-				"writable mappings: gfn %llx role %x\n",
-			       __func__, audit_msg, sp->gfn,
-			       sp->role.word);
+			audit_printk("shadow page has writable mappings: gfn "
+				     "%llx role %x\n", sp->gfn, sp->role.word);
 		spte = rmap_next(kvm, rmapp, spte);
 	}
 }

@@ -228,14 +222,14 @@ static void audit_vcpu_spte(struct kvm_vcpu *vcpu)
 	mmu_spte_walk(vcpu, audit_spte);
 }
 
-static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, int audit_point)
+static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, int point)
 {
 	static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 10);
 
 	if (!__ratelimit(&ratelimit_state))
 		return;
 
-	audit_msg = audit_point_name[audit_point];
+	audit_point = point;
 	audit_all_active_sps(vcpu->kvm);
 	audit_vcpu_spte(vcpu);
 }
Signature · 1 line
1.7.0.4
author
#8 lore raw

PATCH 7/7 KVM: MMU: audit: check whether have unsync sps after root sync

In reply to Xiao Guangrong

After root synced, all unsync sps are synced, this patch add a check to make sure it's no unsync sps in VCPU's page table

Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c       |   11 +++++++++--
 arch/x86/kvm/mmu_audit.c |   11 ++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)
arch/x86/kvm/mmu.c +9 -2
index afde64b..ba7e764 100644
@@ -53,14 +53,18 @@ enum {
 	AUDIT_PRE_PAGE_FAULT,
 	AUDIT_POST_PAGE_FAULT,
 	AUDIT_PRE_PTE_WRITE,
-	AUDIT_POST_PTE_WRITE
+	AUDIT_POST_PTE_WRITE,
+	AUDIT_PRE_SYNC,
+	AUDIT_POST_SYNC
 };
 
 char *audit_point_name[] = {
 	"pre page fault",
 	"post page fault",
 	"pre pte write",
-	"post pte write"
+	"post pte write",
+	"pre sync",
+	"post sync"
 };
 
 #undef MMU_DEBUG

@@ -2516,6 +2520,8 @@ static void mmu_sync_roots(struct kvm_vcpu *vcpu)
 
 	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
 		return;
+
+	trace_kvm_mmu_audit(vcpu, AUDIT_PRE_SYNC);
 	if (vcpu->arch.mmu.root_level == PT64_ROOT_LEVEL) {
 		hpa_t root = vcpu->arch.mmu.root_hpa;
 		sp = page_header(root);

@@ -2531,6 +2537,7 @@ static void mmu_sync_roots(struct kvm_vcpu *vcpu)
 			mmu_sync_children(vcpu, sp);
 		}
 	}
+	trace_kvm_mmu_audit(vcpu, AUDIT_POST_SYNC);
 }
 
 void kvm_mmu_sync_roots(struct kvm_vcpu *vcpu)
arch/x86/kvm/mmu_audit.c +10 -1
index 66219af..4aee32c 100644
@@ -164,6 +164,14 @@ static void audit_sptes_have_rmaps(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 		inspect_spte_has_rmap(vcpu->kvm, sptep);
 }
 
+static void audit_spte_after_sync(struct kvm_vcpu *vcpu, u64 *sptep, int level)
+{
+	struct kvm_mmu_page *sp = page_header(__pa(sptep));
+
+	if (audit_point == AUDIT_POST_SYNC && sp->unsync)
+		audit_printk("meet unsync sp(%p) after sync root.\n", sp);
+}
+
 static void check_mappings_rmap(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
 	int i;

@@ -179,7 +187,7 @@ static void check_mappings_rmap(struct kvm *kvm, struct kvm_mmu_page *sp)
 	}
 }
 
-void audit_write_protection(struct kvm *kvm, struct kvm_mmu_page *sp)
+static void audit_write_protection(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
 	struct kvm_memory_slot *slot;
 	unsigned long *rmapp;

@@ -215,6 +223,7 @@ static void audit_spte(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 {
 	audit_sptes_have_rmaps(vcpu, sptep, level);
 	audit_mappings(vcpu, sptep, level);
+	audit_spte_after_sync(vcpu, sptep, level);
 }
 
 static void audit_vcpu_spte(struct kvm_vcpu *vcpu)
Signature · 1 line
1.7.0.4
author
#9 lore raw
In reply to Xiao Guangrong
Quoted 4 lines · Xiao Guangrong

The value of 'vcpu->arch.mmu.pae_root' is not modified, so we can update 'root_hpa' out of the loop

Xiao Guangrong <[email protected]>

Hi Avi, Marcelo,

I'm so sorry that missed "Signed-off-by: " string in all patches, i'll repost this patchset after your review. :-(

#10 lore raw
Quoted 10 lines · Xiao Guangrong
Quoted 4 lines · Xiao Guangrong

The value of 'vcpu->arch.mmu.pae_root' is not modified, so we can update 'root_hpa' out of the loop

Xiao Guangrong<[email protected]>

Hi Avi, Marcelo,

I'm so sorry that missed "Signed-off-by: " string in all patches, i'll repost this patchset after your review. :-(

I use 'git commit -s' to simplify adding signoffs.

Signature · 1 line
error compiling committee.c: too many arguments to function
#11 lore raw
In reply to Xiao Guangrong
Quoted 10 lines · Xiao Guangrong
Quoted 4 lines · Xiao Guangrong

The value of 'vcpu->arch.mmu.pae_root' is not modified, so we can update 'root_hpa' out of the loop

Xiao Guangrong<[email protected]>

Hi Avi, Marcelo,

I'm so sorry that missed "Signed-off-by: " string in all patches, i'll repost this patchset after your review. :-(

I fixed up the signoffs and applied. Thanks.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#12 lore raw
Quoted 8 lines · Avi Kivity
Quoted 7 lines · Xiao Guangrong
Quoted 1 line · Xiao Guangrong

The value of 'vcpu->arch.mmu.pae_root' is not modified, so we can

update 'root_hpa'

Quoted 3 lines

out of the loop

Xiao Guangrong<[email protected]>

Quoted 7 lines
Quoted 5 lines

Hi Avi, Marcelo,

I'm so sorry that missed "Signed-off-by: " string in all patches, i'll repost this
patchset after your review. :-(

I fixed up the signoffs and applied.

Thanks Avi.

I noticed you applied "KVM: MMU: rename 'sp->root_count' to 'sp->active_count'" (commit 55d80c448e7e3417) instead of this patch :-)

#13 lore raw
Quoted 15 lines · Xiao Guangrong
Quoted 14 lines · Avi Kivity
Quoted 12 lines · Xiao Guangrong
Quoted 1 line · Xiao Guangrong

The value of 'vcpu->arch.mmu.pae_root' is not modified, so we can

update 'root_hpa'

Quoted 3 lines

out of the loop

Xiao Guangrong<[email protected]>

Hi Avi, Marcelo,

I'm so sorry that missed "Signed-off-by: " string in all patches, i'll repost this
patchset after your review. :-(

Quoted 6 lines
Quoted 1 line

I fixed up the signoffs and applied.

Thanks Avi.

I noticed you applied "KVM: MMU: rename 'sp->root_count' to 'sp->active_count'" (commit 55d80c448e7e3417) instead of this patch :-)

Thanks for noticing! Fixed.

No idea how it happened.

Signature · 1 line
error compiling committee.c: too many arguments to function