Skip to contentSkip to conversation

KVM: MMU: mmu audit code improved

PATCH Superseded Xiao Guangrong 3 participants 20 messages Aug 28, 2010 – Sep 2, 2010

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

Xiao Guangrong

author
#0 lore raw

cover KVM: MMU: mmu audit code improved

The audit can help us to detect the mmu bugs as early as possible, it's also can help us to fix them.

It's so useful but the linux distribution is imposable to use it since:

- it need enable it by define "AUDIT" macro and compile it
- the audit code is very high overload, it lets the guest mostly hung

So, this patchset supports to enable/disable it dynamically, it's very low overhead if disable it, and it lowers the audit frequency to assure the guest running.

After this patchset, we can enable it by:
mount -t debugfs none debugfs
echo 1 > debugfs/kvm/mmu-debug

disable it by:
echo 0 > debugfs/kvm/mmu-debug

default, the audit is disabled

[PATCH 1/4] KVM: MMU: support disable/enable mmu audit dynamically [PATCH 2/4] KVM: MMU: improve active sp audit
[PATCH 3/4] KVM: MMU: improve spte audit
[PATCH 4/4] KVM: MMU: lower the aduit frequency

author
#1 lore raw

PATCH 1/4 KVM: MMU: support disable/enable mmu audit dynamicly

Add the debugfs file named 'mmu-debug', we can disable/enable mmu audit by this file:

enable:
echo 1 > debugfs/kvm/mmu-debug

disable:
echo 0 > debugfs/kvm/mmu-debug

This patch not change the logic

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/Kconfig       |    6 +
 arch/x86/kvm/mmu.c         |  250 ++--------------------------------
 arch/x86/kvm/mmu_debug.c   |  329 ++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/mmu_debug.h   |   12 ++
 arch/x86/kvm/mmutrace.h    |   19 +++
 arch/x86/kvm/paging_tmpl.h |    4 +-
 virt/kvm/kvm_main.c        |    6 +-
 7 files changed, 380 insertions(+), 246 deletions(-)
 create mode 100644 arch/x86/kvm/mmu_debug.c
 create mode 100644 arch/x86/kvm/mmu_debug.h
arch/x86/kvm/Kconfig +6 -0
index 970bbd4..67a941d 100644
@@ -64,6 +64,12 @@ config KVM_AMD
 	  To compile this as a module, choose M here: the module
 	  will be called kvm-amd.
 
+config KVM_MMU_DEBUG
+	bool "Debug KVM MMU"
+	depends on KVM && TRACEPOINTS
+	---help---
+	 This feature allows debug KVM MMU at runtime.
+
 # OK, it's a little counter-intuitive to do this, but it puts it neatly under
 # the virtualization menu.
 source drivers/vhost/Kconfig
arch/x86/kvm/mmu.c +9 -241

296 lines not on this page · raw message

arch/x86/kvm/mmu_debug.c new file +329 -0

329 lines not on this page · raw message

arch/x86/kvm/mmu_debug.h new file +12 -0
new file mode 100644
index 0000000..23f634f
@@ -0,0 +1,12 @@
+#ifndef _LINUX_MMU_DEBUG_H
+#define _LINUX_MMU_DEBUG_H
+
+#ifdef CONFIG_KVM_MMU_DEBUG
+void mmu_debug_init(void);
+void mmu_debug_cleanup(void);
+#else
+static inline void mmu_debug_init(void) {};
+static inline void mmu_debug_cleanup(void) {};
+#endif
+
+#endif
arch/x86/kvm/mmutrace.h +19 -0
index 3aab0f0..28a0e1f 100644
@@ -195,6 +195,25 @@ DEFINE_EVENT(kvm_mmu_page_class, kvm_mmu_prepare_zap_page,
 
 	TP_ARGS(sp)
 );
+
+TRACE_EVENT(
+	kvm_mmu_audit,
+	TP_PROTO(struct kvm_vcpu *vcpu, const char *msg),
+	TP_ARGS(vcpu, msg),
+
+	TP_STRUCT__entry(
+		__field(struct kvm_vcpu *, vcpu)
+		__field(const char *, msg)
+	),
+
+	TP_fast_assign(
+		__entry->vcpu = vcpu;
+		__entry->msg = msg;
+	),
+
+	TP_printk("%s", __entry->msg)
+);
+
 #endif /* _TRACE_KVMMMU_H */
 
 #undef TRACE_INCLUDE_PATH
arch/x86/kvm/paging_tmpl.h +2 -2
index a0f2feb..d6f348b 100644
@@ -542,7 +542,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr,
 	if (mmu_notifier_retry(vcpu, mmu_seq))
 		goto out_unlock;
 
-	kvm_mmu_audit(vcpu, "pre page fault");
+	trace_kvm_mmu_audit(vcpu, "pre page fault");
 	kvm_mmu_free_some_pages(vcpu);
 	sptep = FNAME(fetch)(vcpu, addr, &walker, user_fault, write_fault,
 			     level, &write_pt, pfn);

@@ -554,7 +554,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr,
 		vcpu->arch.last_pt_write_count = 0; /* reset fork detector */
 
 	++vcpu->stat.pf_fixed;
-	kvm_mmu_audit(vcpu, "post page fault (fixed)");
+	trace_kvm_mmu_audit(vcpu, "post page fault (fixed)");
 	spin_unlock(&vcpu->kvm->mmu_lock);
 
 	return write_pt;
virt/kvm/kvm_main.c +3 -3
index 9a73b98..cc7b624 100644
@@ -2262,6 +2262,8 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 	int r;
 	int cpu;
 
+	kvm_init_debug();
+
 	r = kvm_arch_init(opaque);
 	if (r)
 		goto out_fail;

@@ -2346,8 +2348,6 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 	kvm_preempt_ops.sched_in = kvm_sched_in;
 	kvm_preempt_ops.sched_out = kvm_sched_out;
 
-	kvm_init_debug();
-
 	return 0;
 
 out_free:

@@ -2379,7 +2379,6 @@ EXPORT_SYMBOL_GPL(kvm_init);
 
 void kvm_exit(void)
 {
-	kvm_exit_debug();
 	misc_deregister(&kvm_dev);
 	kmem_cache_destroy(kvm_vcpu_cache);
 	sysdev_unregister(&kvm_sysdev);

@@ -2389,6 +2388,7 @@ void kvm_exit(void)
 	on_each_cpu(hardware_disable, NULL, 1);
 	kvm_arch_hardware_unsetup();
 	kvm_arch_exit();
+	kvm_exit_debug();
 	free_cpumask_var(cpus_hardware_enabled);
 	__free_page(hwpoison_page);
 	__free_page(bad_page);
Signature · 1 line
1.7.0.4
#2 lore raw

Re: [PATCH 1/4] KVM: MMU: support disable/enable mmu audit dynamicly

Quoted 8 lines · Xiao Guangrong

Add the debugfs file named 'mmu-debug', we can disable/enable mmu audit by this file:

enable:
echo 1> debugfs/kvm/mmu-debug

disable:
echo 0> debugfs/kvm/mmu-debug

Better as a runtime rw module parameter perhaps? At least it avoids the large debugfs callbacks.

Also, call it audit to preserve the name.

Quoted 7 lines

This patch not change the logic

Signed-off-by Xiao Guangrong<[email protected]>
---
  arch/x86/kvm/Kconfig       |    6 +
  arch/x86/kvm/mmu.c         |  250 ++--------------------------------
  arch/x86/kvm/mmu_debug.c   |  329 ++++++++++++++++++++++++++++++++++++++++++++

Please put the move to mmu_debug in a separate patch.

arch/x86/kvm/mmu_debug.c line 260

Quoted 13 lines
arch/x86/kvm/mmu_debug.c new file +13 -0
+
+static void mmu_debug_enable(void)
+{
+	int ret;
+
+	if (mmu_debug)
+		return;
+
+	ret = register_trace_kvm_mmu_audit(kvm_mmu_audit, NULL);
+	WARN_ON(ret);
+
+	mmu_debug = true;
+}

Really neat use of tracepoints.

Quoted 8 lines
arch/x86/kvm/mmutrace.h +0 -0
index 3aab0f0..28a0e1f 100644
@@ -195,6 +195,25 @@ DEFINE_EVENT(kvm_mmu_page_class, kvm_mmu_prepare_zap_page,
 
  	TP_ARGS(sp)
  );

arch/x86/kvm/mmutrace.h DEFINE_EVENT(kvm_mmu_page_class, kvm_mmu_prepare_zap_page, · line 207

Quoted 10 lines
arch/x86/kvm/mmutrace.h +10 -0
+
+TRACE_EVENT(
+	kvm_mmu_audit,
+	TP_PROTO(struct kvm_vcpu *vcpu, const char *msg),
+	TP_ARGS(vcpu, msg),
+
+	TP_STRUCT__entry(
+		__field(struct kvm_vcpu *, vcpu)
+		__field(const char *, msg)
+	),

enum instead of char *, maybe something in userspace can make use of this.

arch/x86/kvm/mmutrace.h DEFINE_EVENT(kvm_mmu_page_class, kvm_mmu_prepare_zap_page, · line 214

Quoted 7 lines
arch/x86/kvm/mmutrace.h +7 -0
+
+	TP_fast_assign(
+		__entry->vcpu = vcpu;
+		__entry->msg = msg;
+	),
+
+	TP_printk("%s", __entry->msg)

Here, of course, you can use print_symbolic() to preserve readability.

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

Re: [PATCH 1/4] KVM: MMU: support disable/enable mmu audit dynamicly

Quoted 8 lines · Avi Kivity
Quoted 7 lines · Xiao Guangrong

Add the debugfs file named 'mmu-debug', we can disable/enable mmu audit by
this file:

enable:
echo 1> debugfs/kvm/mmu-debug

Quoted 7 lines
Quoted 2 lines

disable:
echo 0> debugfs/kvm/mmu-debug

Better as a runtime rw module parameter perhaps? At least it avoids the large debugfs callbacks.

Yeah, it's a good idea.

Quoted 1 line

Also, call it audit to preserve the name.

OK

Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/mmutrace.h +4 -0
+
+TRACE_EVENT(
+    kvm_mmu_audit,
+    TP_PROTO(struct kvm_vcpu *vcpu, const char *msg),
Quoted 9 lines

arch/x86/kvm/mmutrace.h DEFINE_EVENT(kvm_mmu_page_class, kvm_mmu_prepare_zap_page, · line 207

Quoted 6 lines
arch/x86/kvm/mmutrace.h +6 -0
+    TP_ARGS(vcpu, msg),
+
+    TP_STRUCT__entry(
+        __field(struct kvm_vcpu *, vcpu)
+        __field(const char *, msg)
+    ),

enum instead of char *, maybe something in userspace can make use of this.

OK

Quoted 9 lines

arch/x86/kvm/mmutrace.h DEFINE_EVENT(kvm_mmu_page_class, kvm_mmu_prepare_zap_page, · line 214

Quoted 7 lines
arch/x86/kvm/mmutrace.h +7 -0
+
+    TP_fast_assign(
+        __entry->vcpu = vcpu;
+        __entry->msg = msg;
+    ),
+
+    TP_printk("%s", __entry->msg)

Here, of course, you can use print_symbolic() to preserve readability.

OK

Will fix them in the next version.

author
#4 lore raw

PATCH 2/4 KVM: MMU: improve active sp audit

In reply to Xiao Guangrong

Both audit_rmap() and audit_write_protection() need to walk all active sp, so we can do these checking in a sp walking

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu_debug.c |   80 +++++++++++++++++++++++----------------------
 1 files changed, 41 insertions(+), 39 deletions(-)
arch/x86/kvm/mmu_debug.c +41 -39
index d2c0048..812d6dc 100644
@@ -70,6 +70,16 @@ static void mmu_spte_walk(struct kvm_vcpu *vcpu, inspect_spte_fn fn)
 	return;
 }
 
+typedef void (*sp_handler) (struct kvm *kvm, struct kvm_mmu_page *sp);
+
+static void walk_all_active_sps(struct kvm *kvm, sp_handler fn)
+{
+	struct kvm_mmu_page *sp;
+
+	list_for_each_entry(sp, &kvm->arch.active_mmu_pages, link)
+		fn(kvm, sp);
+}
+
 static void audit_mappings_page(struct kvm_vcpu *vcpu, u64 page_pte,
 				gva_t va, int level)
 {

@@ -180,67 +190,59 @@ void audit_sptes_have_rmaps(struct kvm_vcpu *vcpu)
 	mmu_spte_walk(vcpu, inspect_spte_has_rmap);
 }
 
-static void check_mappings_rmap(struct kvm_vcpu *vcpu)
+static void check_mappings_rmap(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
-	struct kvm_mmu_page *sp;
 	int i;
 
-	list_for_each_entry(sp, &vcpu->kvm->arch.active_mmu_pages, link) {
-		u64 *pt = sp->spt;
-
-		if (sp->role.level != PT_PAGE_TABLE_LEVEL)
-			continue;
+	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
+		return;
 
-		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
-			if (!is_rmap_spte(pt[i]))
-				continue;
+	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
+		if (!is_rmap_spte(sp->spt[i]))
+			return;
 
-			inspect_spte_has_rmap(vcpu->kvm, &pt[i]);
-		}
+		inspect_spte_has_rmap(kvm, sp->spt + i);
 	}
-	return;
 }
 
-static void audit_rmap(struct kvm_vcpu *vcpu)
+static void audit_write_protection(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
-	check_mappings_rmap(vcpu);
-}
-
-static void audit_write_protection(struct kvm_vcpu *vcpu)
-{
-	struct kvm_mmu_page *sp;
 	struct kvm_memory_slot *slot;
 	unsigned long *rmapp;
 	u64 *spte;
 
-	list_for_each_entry(sp, &vcpu->kvm->arch.active_mmu_pages, link) {
-		if (sp->role.direct)
-			continue;
-		if (sp->unsync)
-			continue;
-		if (sp->role.invalid)
-			continue;
-
-		slot = gfn_to_memslot(vcpu->kvm, sp->gfn);
-		rmapp = &slot->rmap[sp->gfn - slot->base_gfn];
-
-		spte = rmap_next(vcpu->kvm, rmapp, NULL);
-		while (spte) {
-			if (is_writable_pte(*spte))
-				printk(KERN_ERR "%s: (%s) shadow page has "
+	if (sp->role.direct || sp->unsync || sp->role.invalid)
+		return;
+
+	slot = gfn_to_memslot(kvm, sp->gfn);
+	rmapp = &slot->rmap[sp->gfn - slot->base_gfn];
+
+	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);
-			spte = rmap_next(vcpu->kvm, rmapp, spte);
-		}
+		spte = rmap_next(kvm, rmapp, spte);
 	}
 }
 
+static void audit_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
+{
+	check_mappings_rmap(kvm, sp);
+	audit_write_protection(kvm, sp);
+}
+
+static void audit_all_active_sps(struct kvm *kvm)
+{
+	walk_all_active_sps(kvm, audit_sp);
+}
+
 static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, const char *msg)
 {
 	audit_msg = msg;
-	audit_rmap(vcpu);
-	audit_write_protection(vcpu);
+	audit_all_active_sps(vcpu->kvm);
 	if (strcmp("pre pte write", audit_msg) != 0)
 		audit_mappings(vcpu);
 	audit_sptes_have_rmaps(vcpu);
Signature · 1 line
1.7.0.4
author
#5 lore raw

PATCH 3/4 KVM: MMU: improve spte audit

In reply to Xiao Guangrong

Both audit_mappings() and audit_sptes_have_rmaps() need to walk vcpu's page table, so we can do these checking in a spte walking

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu_debug.c |  148 +++++++++++++++++++++------------------------
 1 files changed, 69 insertions(+), 79 deletions(-)
arch/x86/kvm/mmu_debug.c +69 -79
index 812d6dc..c4ebe6a 100644
@@ -24,23 +24,24 @@ static bool mmu_debug;
 
 static const char *audit_msg;
 
-typedef void (*inspect_spte_fn) (struct kvm *kvm, u64 *sptep);
+typedef void (*inspect_spte_fn) (struct kvm_vcpu *vcpu, u64 *sptep, int level);
 
-static void __mmu_spte_walk(struct kvm *kvm, struct kvm_mmu_page *sp,
-			    inspect_spte_fn fn)
+static void __mmu_spte_walk(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+			    inspect_spte_fn fn, int level)
 {
 	int i;
 
 	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
-		u64 ent = sp->spt[i];
-
-		if (is_shadow_present_pte(ent)) {
-			if (!is_last_spte(ent, sp->role.level)) {
-				struct kvm_mmu_page *child;
-				child = page_header(ent & PT64_BASE_ADDR_MASK);
-				__mmu_spte_walk(kvm, child, fn);
-			} else
-				fn(kvm, &sp->spt[i]);
+		u64 *ent = sp->spt;
+
+		fn(vcpu, ent + i, level);
+
+		if (is_shadow_present_pte(ent[i]) &&
+		      !is_last_spte(ent[i], level)) {
+			struct kvm_mmu_page *child;
+
+			child = page_header(ent[i] & PT64_BASE_ADDR_MASK);
+			__mmu_spte_walk(vcpu, child, fn, level - 1);
 		}
 	}
 }

@@ -52,19 +53,21 @@ static void mmu_spte_walk(struct kvm_vcpu *vcpu, inspect_spte_fn fn)
 
 	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
 		return;
+
 	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_LEVEL) {
 		hpa_t root = vcpu->arch.mmu.root_hpa;
 		sp = page_header(root);
-		__mmu_spte_walk(vcpu->kvm, sp, fn);
+		__mmu_spte_walk(vcpu, sp, fn, PT64_ROOT_LEVEL);
 		return;
 	}
+
 	for (i = 0; i < 4; ++i) {
 		hpa_t root = vcpu->arch.mmu.pae_root[i];
 
 		if (root && VALID_PAGE(root)) {
 			root &= PT64_BASE_ADDR_MASK;
 			sp = page_header(root);
-			__mmu_spte_walk(vcpu->kvm, sp, fn);
+			__mmu_spte_walk(vcpu, sp, fn, 2);
 		}
 	}
 	return;

@@ -80,80 +83,56 @@ static void walk_all_active_sps(struct kvm *kvm, sp_handler fn)
 		fn(kvm, sp);
 }
 
-static void audit_mappings_page(struct kvm_vcpu *vcpu, u64 page_pte,
-				gva_t va, int level)
+static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 {
-	u64 *pt = __va(page_pte & PT64_BASE_ADDR_MASK);
-	int i;
-	gva_t va_delta = 1ul << (PAGE_SHIFT + 9 * (level - 1));
-
-	for (i = 0; i < PT64_ENT_PER_PAGE; ++i, va += va_delta) {
-		u64 *sptep = pt + i;
-		struct kvm_mmu_page *sp;
-		gfn_t gfn;
-		pfn_t pfn;
-		hpa_t hpa;
-
-		sp = page_header(__pa(sptep));
-
-		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);
-				return;
-			}
-
-			if (*sptep == shadow_notrap_nonpresent_pte) {
-				printk(KERN_ERR "audit: (%s) error: notrap spte in unsync sp: %p\n",
-						audit_msg, sp);
-				return;
-			}
-		}
+	struct kvm_mmu_page *sp;
+	gfn_t gfn;
+	pfn_t pfn;
+	hpa_t hpa;
 
-		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);
+	sp = page_header(__pa(sptep));
+
+	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);
 			return;
 		}
 
-		if (!is_shadow_present_pte(*sptep) ||
-		      !is_last_spte(*sptep, level))
+		if (*sptep == shadow_notrap_nonpresent_pte) {
+			printk(KERN_ERR "audit: (%s) error: notrap spte in unsync sp: %p\n",
+					audit_msg, sp);
 			return;
+		}
+	}
 
-		gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
-		pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+	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);
+		return;
+	}
 
-		if (is_error_pfn(pfn)) {
-			kvm_release_pfn_clean(pfn);
-			return;
-		}
+	if (!is_shadow_present_pte(*sptep) || !is_last_spte(*sptep, level))
+		return;
 
-		hpa =  pfn << PAGE_SHIFT;
+	gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+	pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
 
-		if ((*sptep & PT64_BASE_ADDR_MASK) != hpa)
-			printk(KERN_ERR "xx audit error: (%s) levels %d"
-					   " gva %lx pfn %llx hpa %llx ent %llxn",
-					   audit_msg, vcpu->arch.mmu.root_level,
-					   va, pfn, hpa, *sptep);
+	if (is_error_pfn(pfn)) {
+		kvm_release_pfn_clean(pfn);
+		return;
 	}
-}
 
-static void audit_mappings(struct kvm_vcpu *vcpu)
-{
-	unsigned i;
-
-	if (vcpu->arch.mmu.root_level == 4)
-		audit_mappings_page(vcpu, vcpu->arch.mmu.root_hpa, 0, 4);
-	else
-		for (i = 0; i < 4; ++i)
-			if (vcpu->arch.mmu.pae_root[i] & PT_PRESENT_MASK)
-				audit_mappings_page(vcpu,
-						    vcpu->arch.mmu.pae_root[i],
-						    i << 30,
-						    2);
+	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);
 }
 
-void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
+static void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
 {
 	unsigned long *rmapp;
 	struct kvm_mmu_page *rev_sp;

@@ -185,9 +164,10 @@ void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
 	}
 }
 
-void audit_sptes_have_rmaps(struct kvm_vcpu *vcpu)
+static void audit_sptes_have_rmaps(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 {
-	mmu_spte_walk(vcpu, inspect_spte_has_rmap);
+	if (is_shadow_present_pte(*sptep) && is_last_spte(*sptep, level))
+		inspect_spte_has_rmap(vcpu->kvm, sptep);
 }
 
 static void check_mappings_rmap(struct kvm *kvm, struct kvm_mmu_page *sp)

@@ -239,13 +219,23 @@ static void audit_all_active_sps(struct kvm *kvm)
 	walk_all_active_sps(kvm, audit_sp);
 }
 
+static void audit_spte(struct kvm_vcpu *vcpu, u64 *sptep, int level)
+{
+	audit_sptes_have_rmaps(vcpu, sptep, level);
+	audit_mappings(vcpu, sptep, level);
+}
+
+
+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, const char *msg)
 {
 	audit_msg = msg;
 	audit_all_active_sps(vcpu->kvm);
-	if (strcmp("pre pte write", audit_msg) != 0)
-		audit_mappings(vcpu);
-	audit_sptes_have_rmaps(vcpu);
+	audit_vcpu_spte(vcpu);
 }
 
 static void mmu_debug_enable(void)
Signature · 1 line
1.7.0.4
author
#6 lore raw

PATCH 4/4 KVM: MMU: lower the aduit frequency

In reply to Xiao Guangrong

The audit is very high overhead, so we need lower the frequency to assure the guest running

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu_debug.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
arch/x86/kvm/mmu_debug.c +6 -0
index c4ebe6a..bc61b3d 100644
@@ -18,6 +18,7 @@
  */
 
 #include <linux/debugfs.h>
+#include <linux/ratelimit.h>
 
 static struct dentry *debugfs_file;
 static bool mmu_debug;

@@ -233,6 +234,11 @@ static void audit_vcpu_spte(struct kvm_vcpu *vcpu)
 
 static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, const char *msg)
 {
+	static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 10);
+
+	if (!__ratelimit(&ratelimit_state))
+		return;
+
 	audit_msg = msg;
 	audit_all_active_sps(vcpu->kvm);
 	audit_vcpu_spte(vcpu);
Signature · 1 line
1.7.0.4
#7 lore raw

Re: [PATCH 4/4] KVM: MMU: lower the aduit frequency

Quoted 12 lines · Xiao Guangrong

The audit is very high overhead, so we need lower the frequency to assure the guest running

   */
  #include<linux/debugfs.h>
arch/x86/kvm/mmu_debug.c +1 -0
+#include<linux/ratelimit.h>
 
  static struct dentry *debugfs_file;
  static bool mmu_debug;

@@ -233,6 +234,11 @@ static void audit_vcpu_spte(struct kvm_vcpu *vcpu)

arch/x86/kvm/mmu_debug.c static void audit_vcpu_spte(struct kvm_vcpu *vcpu) · line 244

Quoted 10 lines
arch/x86/kvm/mmu_debug.c +5 -0
  static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, const char *msg)
  {
+	static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 10);
+
+	if (!__ratelimit(&ratelimit_state))
+		return;
+
  	audit_msg = msg;
  	audit_all_active_sps(vcpu->kvm);
  	audit_vcpu_spte(vcpu);

This means we see a bug long after it happened, so we can't correlate it to the cause.

It's fine as an option (even the default) but I'd like to be able to audit after every operation. Perhaps a partial audit that only looks at the gfns and vaddrs that were affected in the last operation?

I have to admit, it's been a very long time since I last used audit.

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

Re: [PATCH 4/4] KVM: MMU: lower the aduit frequency

Quoted 29 lines · Avi Kivity
Quoted 23 lines · Xiao Guangrong

The audit is very high overhead, so we need lower the frequency to assure the guest running

   */
  #include<linux/debugfs.h>
arch/x86/kvm/mmu_debug.c +6 -0
+#include<linux/ratelimit.h>
 
  static struct dentry *debugfs_file;
  static bool mmu_debug;

@@ -233,6 +234,11 @@ static void audit_vcpu_spte(struct kvm_vcpu *vcpu)
 
  static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, const
 char *msg)
  {
+    static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 10);
+
+    if (!__ratelimit(&ratelimit_state))
+        return;
+
      audit_msg = msg;
      audit_all_active_sps(vcpu->kvm);
Quoted 1 line
      audit_vcpu_spte(vcpu);

This means we see a bug long after it happened, so we can't correlate it to the cause.

Quoted 4 lines

It's fine as an option (even the default) but I'd like to be able to audit after every operation. Perhaps a partial audit that only looks at the gfns and vaddrs that were affected in the last operation?

Audit checks all the active shadow pages and all vcpu's page table, so the overload is very high :-)

During my test, if enable the aduit, the guest mostly hung, it means the guest not do anything.
(Host: Intel(R) Xeon(R) X3430 @ 2.40GHz * 4 + 4G memory
GUest: x2VCPU + 1G memory
)

I'll set the 'ratelimit' as a module parameter, then if the user's machine is fast enough, the ratelimit can be disabled.

#9 lore raw

Re: [PATCH 4/4] KVM: MMU: lower the aduit frequency

Quoted 33 lines · Xiao Guangrong
Quoted 28 lines · Avi Kivity
Quoted 23 lines · Xiao Guangrong

The audit is very high overhead, so we need lower the frequency to assure the guest running

    */
   #include<linux/debugfs.h>
arch/x86/kvm/mmu_debug.c +6 -0
+#include<linux/ratelimit.h>
 
   static struct dentry *debugfs_file;
   static bool mmu_debug;

@@ -233,6 +234,11 @@ static void audit_vcpu_spte(struct kvm_vcpu *vcpu)
 
   static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, const
 char *msg)
   {
+    static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 10);
+
+    if (!__ratelimit(&ratelimit_state))
+        return;
+
       audit_msg = msg;
       audit_all_active_sps(vcpu->kvm);
Quoted 1 line
       audit_vcpu_spte(vcpu);

This means we see a bug long after it happened, so we can't correlate it to the cause.

Quoted 4 lines

It's fine as an option (even the default) but I'd like to be able to audit after every operation. Perhaps a partial audit that only looks at the gfns and vaddrs that were affected in the last operation?

Quoted 8 lines

Audit checks all the active shadow pages and all vcpu's page table, so the overload is very high :-)

During my test, if enable the aduit, the guest mostly hung, it means the guest not do anything.
(Host: Intel(R) Xeon(R) X3430 @ 2.40GHz * 4 + 4G memory
GUest: x2VCPU + 1G memory
)

You're right, I remember that from the last time I used audit many years ago.

Quoted 2 lines

I'll set the 'ratelimit' as a module parameter, then if the user's machine is fast enough, the ratelimit can be disabled.

It's only useful in very special cases - low memory and a very fast reproducer. I think we can live without the parameter, if someone has this special case they can hack the code.

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

PATCH v2 1/5 KVM: MMU: support disable/enable mmu audit dynamicly

In reply to Xiao Guangrong

Add a r/w module parameter named 'mmu_audit', it can control audit enable/disable:

enable:
echo 1 > /sys/module/kvm/parameters/mmu_audit

disable:
echo 0 > /sys/module/kvm/parameters/mmu_audit

This patch not change the logic

V2:
Using r/w module parameter instead of debugfs file

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/Kconfig       |    7 +++
 arch/x86/kvm/mmu.c         |   91 +++++++++++++++++++++++++++++++++++---------
 arch/x86/kvm/mmutrace.h    |   19 +++++++++
 arch/x86/kvm/paging_tmpl.h |    4 +-
 4 files changed, 101 insertions(+), 20 deletions(-)
arch/x86/kvm/Kconfig +7 -0
index 970bbd4..ddc131f 100644
@@ -64,6 +64,13 @@ config KVM_AMD
 	  To compile this as a module, choose M here: the module
 	  will be called kvm-amd.
 
+config KVM_MMU_AUDIT
+	bool "Audit KVM MMU"
+	depends on KVM && TRACEPOINTS
+	---help---
+	 This option adds a R/W kVM module parameter 'mmu_audit', which allows
+	 audit  KVM MMU at runtime.
+
 # OK, it's a little counter-intuitive to do this, but it puts it neatly under
 # the virtualization menu.
 source drivers/vhost/Kconfig
arch/x86/kvm/mmu.c +73 -18

135 lines not on this page · raw message

arch/x86/kvm/mmutrace.h +19 -0
index 3aab0f0..b60b4fd 100644
@@ -195,6 +195,25 @@ DEFINE_EVENT(kvm_mmu_page_class, kvm_mmu_prepare_zap_page,
 
 	TP_ARGS(sp)
 );
+
+TRACE_EVENT(
+	kvm_mmu_audit,
+	TP_PROTO(struct kvm_vcpu *vcpu, int audit_point),
+	TP_ARGS(vcpu, audit_point),
+
+	TP_STRUCT__entry(
+		__field(struct kvm_vcpu *, vcpu)
+		__field(int, audit_point)
+	),
+
+	TP_fast_assign(
+		__entry->vcpu = vcpu;
+		__entry->audit_point = audit_point;
+	),
+
+	TP_printk("vcpu:%d %s", __entry->vcpu->cpu,
+		  audit_point_name[__entry->audit_point])
+);
 #endif /* _TRACE_KVMMMU_H */
 
 #undef TRACE_INCLUDE_PATH
arch/x86/kvm/paging_tmpl.h +2 -2
index a0f2feb..debe770 100644
@@ -542,7 +542,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr,
 	if (mmu_notifier_retry(vcpu, mmu_seq))
 		goto out_unlock;
 
-	kvm_mmu_audit(vcpu, "pre page fault");
+	trace_kvm_mmu_audit(vcpu, AUDIT_PRE_PAGE_FAULT);
 	kvm_mmu_free_some_pages(vcpu);
 	sptep = FNAME(fetch)(vcpu, addr, &walker, user_fault, write_fault,
 			     level, &write_pt, pfn);

@@ -554,7 +554,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr,
 		vcpu->arch.last_pt_write_count = 0; /* reset fork detector */
 
 	++vcpu->stat.pf_fixed;
-	kvm_mmu_audit(vcpu, "post page fault (fixed)");
+	trace_kvm_mmu_audit(vcpu, AUDIT_POST_PAGE_FAULT);
 	spin_unlock(&vcpu->kvm->mmu_lock);
 
 	return write_pt;
Signature · 1 line
1.7.0.4
author
#11 lore raw

PATCH v2 2/5 KVM: MMU: move audit to a separate file

Move the audit code from arch/x86/kvm/mmu.c to arch/x86/kvm/mmu_audit.c

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c       |  279 +-------------------------------------------
 arch/x86/kvm/mmu_audit.c |  297 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 298 insertions(+), 278 deletions(-)
 create mode 100644 arch/x86/kvm/mmu_audit.c
arch/x86/kvm/mmu.c +0 -178
index 8b750ff..d2dad65 100644
@@ -3490,282 +3490,5 @@ int kvm_mmu_get_spte_hierarchy(struct kvm_vcpu *vcpu, u64 addr, u64 sptes[4])
 EXPORT_SYMBOL_GPL(kvm_mmu_get_spte_hierarchy);
 
 #ifdef CONFIG_KVM_MMU_AUDIT
-static const char *audit_msg;
-
-typedef void (*inspect_spte_fn) (struct kvm *kvm, u64 *sptep);
-
-static void __mmu_spte_walk(struct kvm *kvm, struct kvm_mmu_page *sp,
-			    inspect_spte_fn fn)
-{
-	int i;
-
-	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
-		u64 ent = sp->spt[i];
-
-		if (is_shadow_present_pte(ent)) {
-			if (!is_last_spte(ent, sp->role.level)) {
-				struct kvm_mmu_page *child;
-				child = page_header(ent & PT64_BASE_ADDR_MASK);
-				__mmu_spte_walk(kvm, child, fn);
-			} else
-				fn(kvm, &sp->spt[i]);
-		}
-	}
-}
-
-static void mmu_spte_walk(struct kvm_vcpu *vcpu, inspect_spte_fn fn)
-{
-	int i;
-	struct kvm_mmu_page *sp;
-
-	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
-		return;
-	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_LEVEL) {
-		hpa_t root = vcpu->arch.mmu.root_hpa;
-		sp = page_header(root);
-		__mmu_spte_walk(vcpu->kvm, sp, fn);
-		return;
-	}
-	for (i = 0; i < 4; ++i) {
-		hpa_t root = vcpu->arch.mmu.pae_root[i];
-
-		if (root && VALID_PAGE(root)) {
-			root &= PT64_BASE_ADDR_MASK;
-			sp = page_header(root);
-			__mmu_spte_walk(vcpu->kvm, sp, fn);
-		}
-	}
-	return;
-}
-
-static void audit_mappings_page(struct kvm_vcpu *vcpu, u64 page_pte,
-				gva_t va, int level)
-{
-	u64 *pt = __va(page_pte & PT64_BASE_ADDR_MASK);
-	int i;
-	gva_t va_delta = 1ul << (PAGE_SHIFT + 9 * (level - 1));
-
-	for (i = 0; i < PT64_ENT_PER_PAGE; ++i, va += va_delta) {
-		u64 *sptep = pt + i;
-		struct kvm_mmu_page *sp;
-		gfn_t gfn;
-		pfn_t pfn;
-		hpa_t hpa;
-
-		sp = page_header(__pa(sptep));
-
-		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);
-				return;
-			}
-
-			if (*sptep == shadow_notrap_nonpresent_pte) {
-				printk(KERN_ERR "audit: (%s) error: notrap spte in unsync sp: %p\n",
-						audit_msg, 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);
-			return;
-		}
-
-		if (!is_shadow_present_pte(*sptep) ||
-		      !is_last_spte(*sptep, level))
-			return;
-
-		gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
-		pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
-
-		if (is_error_pfn(pfn)) {
-			kvm_release_pfn_clean(pfn);
-			return;
-		}
-
-		hpa =  pfn << PAGE_SHIFT;
-
-		if ((*sptep & PT64_BASE_ADDR_MASK) != hpa)
-			printk(KERN_ERR "xx audit error: (%s) levels %d"
-					   " gva %lx pfn %llx hpa %llx ent %llxn",
-					   audit_msg, vcpu->arch.mmu.root_level,
-					   va, pfn, hpa, *sptep);
-	}
-}
-
-static void audit_mappings(struct kvm_vcpu *vcpu)
-{
-	unsigned i;
-
-	if (vcpu->arch.mmu.root_level == 4)
-		audit_mappings_page(vcpu, vcpu->arch.mmu.root_hpa, 0, 4);
-	else
-		for (i = 0; i < 4; ++i)
-			if (vcpu->arch.mmu.pae_root[i] & PT_PRESENT_MASK)
-				audit_mappings_page(vcpu,
-						    vcpu->arch.mmu.pae_root[i],
-						    i << 30,
-						    2);
-}
-
-void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
-{
-	unsigned long *rmapp;
-	struct kvm_mmu_page *rev_sp;
-	gfn_t gfn;
-
-
-	rev_sp = page_header(__pa(sptep));
-	gfn = kvm_mmu_page_get_gfn(rev_sp, sptep - rev_sp->spt);
-
-	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);
-		dump_stack();
-		return;
-	}
-
-	rmapp = gfn_to_rmap(kvm, gfn, rev_sp->role.level);
-	if (!*rmapp) {
-		if (!printk_ratelimit())
-			return;
-		printk(KERN_ERR "%s: no rmap for writable spte %llx\n",
-				 audit_msg, *sptep);
-		dump_stack();
-	}
-}
-
-void audit_sptes_have_rmaps(struct kvm_vcpu *vcpu)
-{
-	mmu_spte_walk(vcpu, inspect_spte_has_rmap);
-}
-
-static void check_mappings_rmap(struct kvm_vcpu *vcpu)
-{
-	struct kvm_mmu_page *sp;
-	int i;
-
-	list_for_each_entry(sp, &vcpu->kvm->arch.active_mmu_pages, link) {
-		u64 *pt = sp->spt;
-
-		if (sp->role.level != PT_PAGE_TABLE_LEVEL)
-			continue;
-
-		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
-			if (!is_rmap_spte(pt[i]))
-				continue;
-
-			inspect_spte_has_rmap(vcpu->kvm, &pt[i]);
-		}
-	}
-	return;
-}---
 arch/x86/kvm/mmu.c       |  279 +-------------------------------------------
 arch/x86/kvm/mmu_audit.c |  297 ++++++++++++++++++++++++++++++++++++++++++++++

 2 files changed, 298 insertions(+), 278 deletions(-)
 create mode 100644 arch/x86/kvm/mmu_audit.c
arch/x86/kvm/mmu.c +1 -278

283 lines not on this page · raw message

arch/x86/kvm/mmu_audit.c new file +297 -0

297 lines not on this page · raw message

Signature · 104 lines
1.7.0.4

-
-static void audit_rmap(struct kvm_vcpu *vcpu)
-{
-	check_mappings_rmap(vcpu);
-}
-
-static void audit_write_protection(struct kvm_vcpu *vcpu)
-{
-	struct kvm_mmu_page *sp;
-	struct kvm_memory_slot *slot;
-	unsigned long *rmapp;
-	u64 *spte;
-
-	list_for_each_entry(sp, &vcpu->kvm->arch.active_mmu_pages, link) {
-		if (sp->role.direct)
-			continue;
-		if (sp->unsync)
-			continue;
-		if (sp->role.invalid)
-			continue;
-
-		slot = gfn_to_memslot(vcpu->kvm, sp->gfn);
-		rmapp = &slot->rmap[sp->gfn - slot->base_gfn];
-
-		spte = rmap_next(vcpu->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);
-			spte = rmap_next(vcpu->kvm, rmapp, spte);
-		}
-	}
-}
-
-static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, int audit_point)
-{
-	audit_msg = audit_point_name[audit_point];
-	audit_rmap(vcpu);
-	audit_write_protection(vcpu);
-	if (strcmp("pre pte write", audit_msg) != 0)
-		audit_mappings(vcpu);
-	audit_sptes_have_rmaps(vcpu);
-}
-
-static bool mmu_audit;
-
-static void mmu_audit_enable(void)
-{
-	int ret;
-
-	if (mmu_audit)
-		return;
-
-	ret = register_trace_kvm_mmu_audit(kvm_mmu_audit, NULL);
-	WARN_ON(ret);
-
-	mmu_audit = true;
-}
-
-static void mmu_audit_disable(void)
-{
-	if (!mmu_audit)
-		return;
-
-	unregister_trace_kvm_mmu_audit(kvm_mmu_audit, NULL);
-	tracepoint_synchronize_unregister();
-	mmu_audit = false;
-}
-
-static int mmu_audit_set(const char *val, const struct kernel_param *kp)
-{
-	int ret;
-	unsigned long enable;
-
-	ret = strict_strtoul(val, 10, &enable);
-	if (ret < 0)
-		return -EINVAL;
-
-	switch (enable) {
-	case 0:
-		mmu_audit_disable();
-		break;
-	case 1:
-		mmu_audit_enable();
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static struct kernel_param_ops audit_param_ops = {
-	.set = mmu_audit_set,
-	.get = param_get_bool,
-};
-
-module_param_cb(mmu_audit, &audit_param_ops, &mmu_audit, 0644);
+#include "mmu_audit.c"
 #endif
arch/x86/kvm/mmu_audit.c new file +297 -0

297 lines not on this page · raw message

Signature · 1 line
1.7.0.4
author
#12 lore raw

PATCH v2 3/5 KVM: MMU: improve active sp audit

In reply to Xiao Guangrong

Both audit_rmap() and audit_write_protection() need to walk all active sp, so we can do these checking in a sp walking

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu_audit.c |   74 +++++++++++++++++++++++----------------------
 1 files changed, 38 insertions(+), 36 deletions(-)
arch/x86/kvm/mmu_audit.c +38 -36
index fb8a461..8becb86 100644
@@ -65,6 +65,16 @@ static void mmu_spte_walk(struct kvm_vcpu *vcpu, inspect_spte_fn fn)
 	return;
 }
 
+typedef void (*sp_handler) (struct kvm *kvm, struct kvm_mmu_page *sp);
+
+static void walk_all_active_sps(struct kvm *kvm, sp_handler fn)
+{
+	struct kvm_mmu_page *sp;
+
+	list_for_each_entry(sp, &kvm->arch.active_mmu_pages, link)
+		fn(kvm, sp);
+}
+
 static void audit_mappings_page(struct kvm_vcpu *vcpu, u64 page_pte,
 				gva_t va, int level)
 {

@@ -175,67 +185,59 @@ void audit_sptes_have_rmaps(struct kvm_vcpu *vcpu)
 	mmu_spte_walk(vcpu, inspect_spte_has_rmap);
 }
 
-static void check_mappings_rmap(struct kvm_vcpu *vcpu)
+static void check_mappings_rmap(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
-	struct kvm_mmu_page *sp;
 	int i;
 
-	list_for_each_entry(sp, &vcpu->kvm->arch.active_mmu_pages, link) {
-		u64 *pt = sp->spt;
+	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
+		return;
 
-		if (sp->role.level != PT_PAGE_TABLE_LEVEL)
+	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
+		if (!is_rmap_spte(sp->spt[i]))
 			continue;
 
-		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
-			if (!is_rmap_spte(pt[i]))
-				continue;
-
-			inspect_spte_has_rmap(vcpu->kvm, &pt[i]);
-		}
+		inspect_spte_has_rmap(kvm, sp->spt + i);
 	}
-	return;
 }
 
-static void audit_rmap(struct kvm_vcpu *vcpu)
+void audit_write_protection(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
-	check_mappings_rmap(vcpu);
-}
-
-static void audit_write_protection(struct kvm_vcpu *vcpu)
-{
-	struct kvm_mmu_page *sp;
 	struct kvm_memory_slot *slot;
 	unsigned long *rmapp;
 	u64 *spte;
 
-	list_for_each_entry(sp, &vcpu->kvm->arch.active_mmu_pages, link) {
-		if (sp->role.direct)
-			continue;
-		if (sp->unsync)
-			continue;
-		if (sp->role.invalid)
-			continue;
+	if (sp->role.direct || sp->unsync || sp->role.invalid)
+		return;
 
-		slot = gfn_to_memslot(vcpu->kvm, sp->gfn);
-		rmapp = &slot->rmap[sp->gfn - slot->base_gfn];
+	slot = gfn_to_memslot(kvm, sp->gfn);
+	rmapp = &slot->rmap[sp->gfn - slot->base_gfn];
 
-		spte = rmap_next(vcpu->kvm, rmapp, NULL);
-		while (spte) {
-			if (is_writable_pte(*spte))
-				printk(KERN_ERR "%s: (%s) shadow page has "
+	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);
-			spte = rmap_next(vcpu->kvm, rmapp, spte);
-		}
+		spte = rmap_next(kvm, rmapp, spte);
 	}
 }
 
+static void audit_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
+{
+	check_mappings_rmap(kvm, sp);
+	audit_write_protection(kvm, sp);
+}
+
+static void audit_all_active_sps(struct kvm *kvm)
+{
+	walk_all_active_sps(kvm, audit_sp);
+}
+
 static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, int audit_point)
 {
 	audit_msg = audit_point_name[audit_point];
-	audit_rmap(vcpu);
-	audit_write_protection(vcpu);
+	audit_all_active_sps(vcpu->kvm);
 	if (strcmp("pre pte write", audit_msg) != 0)
 		audit_mappings(vcpu);
 	audit_sptes_have_rmaps(vcpu);
Signature · 1 line
1.7.0.4
author
#13 lore raw

PATCH v2 4/5 KVM: MMU: improve spte audit

In reply to Xiao Guangrong

Both audit_mappings() and audit_sptes_have_rmaps() need to walk vcpu's page table, so we can do these checking in a spte walking

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu_audit.c |  148 +++++++++++++++++++++------------------------
 1 files changed, 69 insertions(+), 79 deletions(-)
arch/x86/kvm/mmu_audit.c +69 -79
index 8becb86..3bde186 100644
@@ -19,23 +19,24 @@
 
 static const char *audit_msg;
 
-typedef void (*inspect_spte_fn) (struct kvm *kvm, u64 *sptep);
+typedef void (*inspect_spte_fn) (struct kvm_vcpu *vcpu, u64 *sptep, int level);
 
-static void __mmu_spte_walk(struct kvm *kvm, struct kvm_mmu_page *sp,
-			    inspect_spte_fn fn)
+static void __mmu_spte_walk(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+			    inspect_spte_fn fn, int level)
 {
 	int i;
 
 	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
-		u64 ent = sp->spt[i];
-
-		if (is_shadow_present_pte(ent)) {
-			if (!is_last_spte(ent, sp->role.level)) {
-				struct kvm_mmu_page *child;
-				child = page_header(ent & PT64_BASE_ADDR_MASK);
-				__mmu_spte_walk(kvm, child, fn);
-			} else
-				fn(kvm, &sp->spt[i]);
+		u64 *ent = sp->spt;
+
+		fn(vcpu, ent + i, level);
+
+		if (is_shadow_present_pte(ent[i]) &&
+		      !is_last_spte(ent[i], level)) {
+			struct kvm_mmu_page *child;
+
+			child = page_header(ent[i] & PT64_BASE_ADDR_MASK);
+			__mmu_spte_walk(vcpu, child, fn, level - 1);
 		}
 	}
 }

@@ -47,21 +48,25 @@ static void mmu_spte_walk(struct kvm_vcpu *vcpu, inspect_spte_fn fn)
 
 	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
 		return;
+
 	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_LEVEL) {
 		hpa_t root = vcpu->arch.mmu.root_hpa;
+
 		sp = page_header(root);
-		__mmu_spte_walk(vcpu->kvm, sp, fn);
+		__mmu_spte_walk(vcpu, sp, fn, PT64_ROOT_LEVEL);
 		return;
 	}
+
 	for (i = 0; i < 4; ++i) {
 		hpa_t root = vcpu->arch.mmu.pae_root[i];
 
 		if (root && VALID_PAGE(root)) {
 			root &= PT64_BASE_ADDR_MASK;
 			sp = page_header(root);
-			__mmu_spte_walk(vcpu->kvm, sp, fn);
+			__mmu_spte_walk(vcpu, sp, fn, 2);
 		}
 	}
+
 	return;
 }
 

@@ -75,80 +80,55 @@ static void walk_all_active_sps(struct kvm *kvm, sp_handler fn)
 		fn(kvm, sp);
 }
 
-static void audit_mappings_page(struct kvm_vcpu *vcpu, u64 page_pte,
-				gva_t va, int level)
+static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 {
-	u64 *pt = __va(page_pte & PT64_BASE_ADDR_MASK);
-	int i;
-	gva_t va_delta = 1ul << (PAGE_SHIFT + 9 * (level - 1));
-
-	for (i = 0; i < PT64_ENT_PER_PAGE; ++i, va += va_delta) {
-		u64 *sptep = pt + i;
-		struct kvm_mmu_page *sp;
-		gfn_t gfn;
-		pfn_t pfn;
-		hpa_t hpa;
-
-		sp = page_header(__pa(sptep));
-
-		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);
-				return;
-			}
-
-			if (*sptep == shadow_notrap_nonpresent_pte) {
-				printk(KERN_ERR "audit: (%s) error: notrap spte in unsync sp: %p\n",
-						audit_msg, sp);
-				return;
-			}
-		}
+	struct kvm_mmu_page *sp;
+	gfn_t gfn;
+	pfn_t pfn;
+	hpa_t hpa;
 
-		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);
+	sp = page_header(__pa(sptep));
+
+	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);
 			return;
 		}
 
-		if (!is_shadow_present_pte(*sptep) ||
-		      !is_last_spte(*sptep, level))
+		if (*sptep == shadow_notrap_nonpresent_pte) {
+			printk(KERN_ERR "audit: (%s) error: notrap spte in unsync sp: %p\n",
+				audit_msg, sp);
 			return;
+		}
+	}
 
-		gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
-		pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+	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);
+		return;
+	}
 
-		if (is_error_pfn(pfn)) {
-			kvm_release_pfn_clean(pfn);
-			return;
-		}
+	if (!is_shadow_present_pte(*sptep) || !is_last_spte(*sptep, level))
+		return;
 
-		hpa =  pfn << PAGE_SHIFT;
+	gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+	pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
 
-		if ((*sptep & PT64_BASE_ADDR_MASK) != hpa)
-			printk(KERN_ERR "xx audit error: (%s) levels %d"
-					   " gva %lx pfn %llx hpa %llx ent %llxn",
-					   audit_msg, vcpu->arch.mmu.root_level,
-					   va, pfn, hpa, *sptep);
+	if (is_error_pfn(pfn)) {
+		kvm_release_pfn_clean(pfn);
+		return;
 	}
-}
 
-static void audit_mappings(struct kvm_vcpu *vcpu)
-{
-	unsigned i;
-
-	if (vcpu->arch.mmu.root_level == 4)
-		audit_mappings_page(vcpu, vcpu->arch.mmu.root_hpa, 0, 4);
-	else
-		for (i = 0; i < 4; ++i)
-			if (vcpu->arch.mmu.pae_root[i] & PT_PRESENT_MASK)
-				audit_mappings_page(vcpu,
-						    vcpu->arch.mmu.pae_root[i],
-						    i << 30,
-						    2);
+	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);
 }
 
-void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
+static void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
 {
 	unsigned long *rmapp;
 	struct kvm_mmu_page *rev_sp;

@@ -180,9 +160,10 @@ void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
 	}
 }
 
-void audit_sptes_have_rmaps(struct kvm_vcpu *vcpu)
+static void audit_sptes_have_rmaps(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 {
-	mmu_spte_walk(vcpu, inspect_spte_has_rmap);
+	if (is_shadow_present_pte(*sptep) && is_last_spte(*sptep, level))
+		inspect_spte_has_rmap(vcpu->kvm, sptep);
 }
 
 static void check_mappings_rmap(struct kvm *kvm, struct kvm_mmu_page *sp)

@@ -234,13 +215,22 @@ static void audit_all_active_sps(struct kvm *kvm)
 	walk_all_active_sps(kvm, audit_sp);
 }
 
+static void audit_spte(struct kvm_vcpu *vcpu, u64 *sptep, int level)
+{
+	audit_sptes_have_rmaps(vcpu, sptep, level);
+	audit_mappings(vcpu, sptep, level);
+}
+
+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)
 {
 	audit_msg = audit_point_name[audit_point];
 	audit_all_active_sps(vcpu->kvm);
-	if (strcmp("pre pte write", audit_msg) != 0)
-		audit_mappings(vcpu);
-	audit_sptes_have_rmaps(vcpu);
+	audit_vcpu_spte(vcpu);
 }
 
 static bool mmu_audit;
Signature · 1 line
1.7.0.4
author
#14 lore raw

PATCH v2 5/5 KVM: MMU: lower the aduit frequency

In reply to Xiao Guangrong

The audit is very high overhead, so we need lower the frequency to assure the guest running

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu_audit.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
arch/x86/kvm/mmu_audit.c +7 -0
index 3bde186..bd2b1be 100644
@@ -17,6 +17,8 @@
  *
  */
 
+#include <linux/ratelimit.h>
+
 static const char *audit_msg;
 
 typedef void (*inspect_spte_fn) (struct kvm_vcpu *vcpu, u64 *sptep, int level);

@@ -228,6 +230,11 @@ static void audit_vcpu_spte(struct kvm_vcpu *vcpu)
 
 static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, int audit_point)
 {
+	static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 10);
+
+	if (!__ratelimit(&ratelimit_state))
+		return;
+
 	audit_msg = audit_point_name[audit_point];
 	audit_all_active_sps(vcpu->kvm);
 	audit_vcpu_spte(vcpu);
Signature · 1 line
1.7.0.4
#15 lore raw

Re: [PATCH v2 5/5] KVM: MMU: lower the aduit frequency

Quoted 34 lines · Xiao Guangrong

The audit is very high overhead, so we need lower the frequency to assure the guest running

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu_audit.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
arch/x86/kvm/mmu_audit.c +7 -0
index 3bde186..bd2b1be 100644
@@ -17,6 +17,8 @@
  *
  */
 
+#include <linux/ratelimit.h>
+
 static const char *audit_msg;
 
 typedef void (*inspect_spte_fn) (struct kvm_vcpu *vcpu, u64 *sptep, int level);

@@ -228,6 +230,11 @@ static void audit_vcpu_spte(struct kvm_vcpu *vcpu)
 
 static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, int audit_point)
 {
+	static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 10);
+
+	if (!__ratelimit(&ratelimit_state))
+		return;
+
 	audit_msg = audit_point_name[audit_point];
 	audit_all_active_sps(vcpu->kvm);
 	audit_vcpu_spte(vcpu);
Signature · 1 line
1.7.0.4

Well, as Avi said this makes it difficult to trace back to offender (the audit points are placed around modifications to the shadow page tree for that reason).

I've always seen progress from the guest while running with audit enabled (its slow, but its not supposed to be fast anyway).

Did you experience a freeze?

author
#16 lore raw

Re: [PATCH v2 5/5] KVM: MMU: lower the aduit frequency

Quoted 23 lines · Marcelo Tosatti
Quoted 22 lines · Xiao Guangrong

The audit is very high overhead, so we need lower the frequency to assure the guest running

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu_audit.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
arch/x86/kvm/mmu_audit.c +2 -0
index 3bde186..bd2b1be 100644
@@ -17,6 +17,8 @@
  *
  */
 
+#include <linux/ratelimit.h>
+
 static const char *audit_msg;
 
 typedef void (*inspect_spte_fn) (struct kvm_vcpu *vcpu, u64 *sptep, int level);

@@ -228,6 +230,11 @@ static void audit_vcpu_spte(struct kvm_vcpu *vcpu)
Quoted 17 lines
Quoted 12 lines
arch/x86/kvm/mmu_audit.c +5 -0
 static void kvm_mmu_audit(void *ignore, struct kvm_vcpu *vcpu, int audit_point)
 {
+	static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 10);
+
+	if (!__ratelimit(&ratelimit_state))
+		return;
+
 	audit_msg = audit_point_name[audit_point];
 	audit_all_active_sps(vcpu->kvm);
 	audit_vcpu_spte(vcpu);
Signature · 1 line
1.7.0.4

Well, as Avi said this makes it difficult to trace back to offender (the audit points are placed around modifications to the shadow page tree for that reason).

Yeah. it's the best way that not rate limit it, but...

Quoted 5 lines

I've always seen progress from the guest while running with audit enabled (its slow, but its not supposed to be fast anyway).

Did you experience a freeze?

There is a simply test in the guest if it's not rate limit:

# time ls
anaconda-ks.cfg  Documents  install.log         Music     Public     Videos
Desktop          Downloads  install.log.syslog  Pictures  Templates
real    1m26.053s
user    0m0.311s
sys     0m1.813s

'ls' command cost about 1.5 minute, if we run the memory test program, i think the time/delay is unacceptable...... :-(

#17 lore raw

Re: [PATCH v2 5/5] KVM: MMU: lower the aduit frequency

Quoted 12 lines · Xiao Guangrong
Quoted 6 lines

I've always seen progress from the guest while running with audit enabled (its slow, but its not supposed to be fast anyway).

Did you experience a freeze?

There is a simply test in the guest if it's not rate limit:

# time ls
anaconda-ks.cfg  Documents  install.log         Music     Public     Videos
Desktop          Downloads  install.log.syslog  Pictures  Templates
Quoted 6 lines
real    1m26.053s
user    0m0.311s
sys     0m1.813s

'ls' command cost about 1.5 minute, if we run the memory test program, i think the time/delay is unacceptable...... :-(

Marcelo, would making the ratelimit optional help? personally I think without ratelimit audit is useless, and with ratelimit it is a lot less useful but can still point out problems. But I haven't used it in a while.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#18 lore raw

Re: [PATCH v2 5/5] KVM: MMU: lower the aduit frequency

Quoted 17 lines · Avi Kivity
Quoted 16 lines · Xiao Guangrong
Quoted 6 lines

I've always seen progress from the guest while running with audit enabled (its slow, but its not supposed to be fast anyway).

Did you experience a freeze?

There is a simply test in the guest if it's not rate limit:

# time ls
anaconda-ks.cfg  Documents  install.log         Music     Public     Videos
Desktop          Downloads  install.log.syslog  Pictures  Templates
real    1m26.053s
user    0m0.311s
sys     0m1.813s
Quoted 7 lines
Quoted 2 lines

'ls' command cost about 1.5 minute, if we run the memory test program, i think the time/delay is unacceptable...... :-(

Marcelo, would making the ratelimit optional help? personally I think without ratelimit audit is useless, and with ratelimit it is a lot less useful but can still point out problems. But I haven't used it in a while.

Was just wondering whether there was a freeze. Patchset looks good (ratelimit can be disabled manually).

#19 lore raw

Re: [PATCH v2 5/5] KVM: MMU: lower the aduit frequency

Quoted 4 lines · Marcelo Tosatti

Was just wondering whether there was a freeze. Patchset looks good (ratelimit can be disabled manually).

Ok, applied now.

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