Skip to contentSkip to conversation

KVM MMU: convert mmu tracepoints

PATCH Superseded Xiao Guangrong 2 participants 11 messages Apr 22, 2010 – Apr 23, 2010 1 message not in the archive

Subsystems (3)
x86 KVM KVM x86

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Xiao Guangrong

author
#1 lore raw

PATCH 1/10 KVM MMU: fix for calculating gpa in invlpg code

If the guest is 32-bit, we should use 'quadrant' to adjust gpa offset

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/paging_tmpl.h |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
arch/x86/kvm/paging_tmpl.h +6 -1
index d0cc07e..46d80d6 100644
@@ -478,9 +478,14 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 		    ((level == PT_DIRECTORY_LEVEL && is_large_pte(*sptep))) ||
 		    ((level == PT_PDPE_LEVEL && is_large_pte(*sptep)))) {
 			struct kvm_mmu_page *sp = page_header(__pa(sptep));
+			int offset = 0;
+
+			if (PTTYPE == 32)
+				offset = sp->role.quadrant << PT64_LEVEL_BITS;;
 
 			pte_gpa = (sp->gfn << PAGE_SHIFT);
-			pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
+			pte_gpa += (sptep - sp->spt + offset) *
+					sizeof(pt_element_t);
 
 			if (is_shadow_present_pte(*sptep)) {
 				rmap_remove(vcpu->kvm, sptep);
Signature · 1 line
1.6.1.2
#2 lore raw

Re: [PATCH 1/10] KVM MMU: fix for calculating gpa in invlpg code

Quoted 4 lines · Xiao Guangrong

If the guest is 32-bit, we should use 'quadrant' to adjust gpa offset

Good catch. Only affects kvm_mmu_pte_write(), so I don't think this had ill effects other than not prefetching the correct address?

arch/x86/kvm/paging_tmpl.h static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva) · line 485

Quoted 9 lines
arch/x86/kvm/paging_tmpl.h +4 -0
@@ -478,9 +478,14 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
  		    ((level == PT_DIRECTORY_LEVEL&&  is_large_pte(*sptep))) ||
  		    ((level == PT_PDPE_LEVEL&&  is_large_pte(*sptep)))) {
  			struct kvm_mmu_page *sp = page_header(__pa(sptep));
+			int offset = 0;
+
+			if (PTTYPE == 32)
+				offset = sp->role.quadrant<<  PT64_LEVEL_BITS;;

Wrong for PT_DIRECTORY_LEVEL (should be q << 8). Also, too many semicolons.

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
author
#3 lore raw

Re: [PATCH 1/10] KVM MMU: fix for calculating gpa in invlpg code

Quoted 9 lines · Avi Kivity
Quoted 4 lines · Xiao Guangrong

If the guest is 32-bit, we should use 'quadrant' to adjust gpa offset

Good catch. Only affects kvm_mmu_pte_write(), so I don't think this had ill effects other than not prefetching the correct address?

Yes

Quoted 5 lines
Quoted 5 lines
(quoted diff) +0 -0
@@ -478,9 +478,14 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu,
 gva_t gva)
              ((level == PT_DIRECTORY_LEVEL&&  is_large_pte(*sptep))) ||
              ((level == PT_PDPE_LEVEL&&  is_large_pte(*sptep)))) {
              struct kvm_mmu_page *sp = page_header(__pa(sptep));
Quoted 9 lines

arch/x86/kvm/paging_tmpl.h static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva) · line 485

Quoted 5 lines
arch/x86/kvm/paging_tmpl.h +4 -0
+            int offset = 0;
+
+            if (PTTYPE == 32)
+                offset = sp->role.quadrant<<  PT64_LEVEL_BITS;;

Wrong for PT_DIRECTORY_LEVEL (should be q << 8). Also, too many semicolons.

I guess you mean 'PT64_LEVEL_BITS' not 'PT_DIRECTORY_LEVEL' here :-)

It should be q << 8 here? it hardly understand, take leve = 1 for example, 32-bit guest PTE page table mapping range is 2^(10+12), PAE's PTE page table mapping range is 2^(9+12), so, i think it's quadrant << 9 here, and other function like FNAME(prefetch_page), FNAME(sync_page) also are q << 9

Sorry for the double semicolons here, will fix it

Thanks,
Xiao

#4 lore raw

Re: [PATCH 1/10] KVM MMU: fix for calculating gpa in invlpg code

Quoted 17 lines · Xiao Guangrong
Quoted 17 lines

arch/x86/kvm/paging_tmpl.h static void FNAME(invlpg)(struct kvm_vcpu *vcpu, · line 487

Quoted 13 lines
arch/x86/kvm/paging_tmpl.h +4 -0
@@ -478,9 +478,14 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu,
 gva_t gva)
               ((level == PT_DIRECTORY_LEVEL&&   is_large_pte(*sptep))) ||
               ((level == PT_PDPE_LEVEL&&   is_large_pte(*sptep)))) {
               struct kvm_mmu_page *sp = page_header(__pa(sptep));
+            int offset = 0;
+
+            if (PTTYPE == 32)
+                offset = sp->role.quadrant<<   PT64_LEVEL_BITS;;

Wrong for PT_DIRECTORY_LEVEL (should be q<< 8). Also, too many semicolons.

Quoted 2 lines

I guess you mean 'PT64_LEVEL_BITS' not 'PT_DIRECTORY_LEVEL' here :-)

No, I mean if level == PT_DIRECTORY_LEVEL, then we want role.quadrant << 8, not 9.

Quoted 3 lines

It should be q<< 8 here? it hardly understand, take leve = 1 for example, 32-bit guest PTE page table mapping range is 2^(10+12), PAE's PTE page table mapping range is 2^(9+12),

For level == PT_DIRECTORY_LEVEL, quadrant is in the range 0..3. Each sp maps 1GB, while the guest page table maps 4GB. So the upper two bits become the quadrant.

Quoted 3 lines

so, i think it's quadrant<< 9 here, and other
function like FNAME(prefetch_page), FNAME(sync_page) also are q<< 9

They only work for PT_PAGE_TABLE_LEVEL, so for them 9 is correct.

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
author
#5 lore raw

PATCH 2/10 KVM MMU: convert mmu tracepoints

In reply to a message not in the archive

Convert mmu tracepoints by using DECLARE_EVENT_CLASS

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmutrace.h |   69 +++++++++++++++++-----------------------------
 1 files changed, 26 insertions(+), 43 deletions(-)
arch/x86/kvm/mmutrace.h +26 -43
index bc4f7f0..d860a03 100644
@@ -92,15 +92,15 @@ TRACE_EVENT(
 	TP_printk("pte %llx level %u", __entry->pte, __entry->level)
 );
 
-/* We set a pte accessed bit */
-TRACE_EVENT(
-	kvm_mmu_set_accessed_bit,
+DECLARE_EVENT_CLASS(kvm_mmu_set_bit_class,
+
 	TP_PROTO(unsigned long table_gfn, unsigned index, unsigned size),
+
 	TP_ARGS(table_gfn, index, size),
 
 	TP_STRUCT__entry(
 		__field(__u64, gpa)
-		),
+	),
 
 	TP_fast_assign(
 		__entry->gpa = ((u64)table_gfn << PAGE_SHIFT)

@@ -110,22 +110,20 @@ TRACE_EVENT(
 	TP_printk("gpa %llx", __entry->gpa)
 );
 
-/* We set a pte dirty bit */
-TRACE_EVENT(
-	kvm_mmu_set_dirty_bit,
+/* We set a pte accessed bit */
+DEFINE_EVENT(kvm_mmu_set_bit_class, kvm_mmu_set_accessed_bit,
+
 	TP_PROTO(unsigned long table_gfn, unsigned index, unsigned size),
-	TP_ARGS(table_gfn, index, size),
 
-	TP_STRUCT__entry(
-		__field(__u64, gpa)
-		),
+	TP_ARGS(table_gfn, index, size)
+);
 
-	TP_fast_assign(
-		__entry->gpa = ((u64)table_gfn << PAGE_SHIFT)
-				+ index * size;
-		),
+/* We set a pte dirty bit */
+DEFINE_EVENT(kvm_mmu_set_bit_class, kvm_mmu_set_dirty_bit,
 
-	TP_printk("gpa %llx", __entry->gpa)
+	TP_PROTO(unsigned long table_gfn, unsigned index, unsigned size),
+
+	TP_ARGS(table_gfn, index, size)
 );
 
 TRACE_EVENT(

@@ -164,54 +162,39 @@ TRACE_EVENT(
 		  __entry->created ? "new" : "existing")
 );
 
-TRACE_EVENT(
-	kvm_mmu_sync_page,
+DECLARE_EVENT_CLASS(kvm_mmu_page_class,
+
 	TP_PROTO(struct kvm_mmu_page *sp),
 	TP_ARGS(sp),
 
 	TP_STRUCT__entry(
 		KVM_MMU_PAGE_FIELDS
-		),
+	),
 
 	TP_fast_assign(
 		KVM_MMU_PAGE_ASSIGN(sp)
-		),
+	),
 
 	TP_printk("%s", KVM_MMU_PAGE_PRINTK())
 );
 
-TRACE_EVENT(
-	kvm_mmu_unsync_page,
+DEFINE_EVENT(kvm_mmu_page_class, kvm_mmu_sync_page,
 	TP_PROTO(struct kvm_mmu_page *sp),
-	TP_ARGS(sp),
-
-	TP_STRUCT__entry(
-		KVM_MMU_PAGE_FIELDS
-		),
-
-	TP_fast_assign(
-		KVM_MMU_PAGE_ASSIGN(sp)
-		),
 
-	TP_printk("%s", KVM_MMU_PAGE_PRINTK())
+	TP_ARGS(sp)
 );
 
-TRACE_EVENT(
-	kvm_mmu_zap_page,
+DEFINE_EVENT(kvm_mmu_page_class, kvm_mmu_unsync_page,
 	TP_PROTO(struct kvm_mmu_page *sp),
-	TP_ARGS(sp),
 
-	TP_STRUCT__entry(
-		KVM_MMU_PAGE_FIELDS
-		),
+	TP_ARGS(sp)
+);
 
-	TP_fast_assign(
-		KVM_MMU_PAGE_ASSIGN(sp)
-		),
+DEFINE_EVENT(kvm_mmu_page_class, kvm_mmu_zap_page,
+	TP_PROTO(struct kvm_mmu_page *sp),
 
-	TP_printk("%s", KVM_MMU_PAGE_PRINTK())
+	TP_ARGS(sp)
 );
-
 #endif /* _TRACE_KVMMMU_H */
 
 #undef TRACE_INCLUDE_PATH
Signature · 1 line
1.6.1.2
author
#6 lore raw

PATCH 3/10 KVM MMU: move unsync/sync tracpoints to proper place

In reply to a message not in the archive

Move unsync/sync tracepoints to the proper place, it's good for us to obtain unsync page live time

Signed-off-by 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 ddfa865..abf8bd4 100644
@@ -1189,6 +1189,7 @@ static struct kvm_mmu_page *kvm_mmu_lookup_page(struct kvm *kvm, gfn_t gfn)
 static void kvm_unlink_unsync_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
 	WARN_ON(!sp->unsync);
+	trace_kvm_mmu_sync_page(sp);
 	sp->unsync = 0;
 	--kvm->stat.mmu_unsync;
 }

@@ -1202,7 +1203,6 @@ static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 		return 1;
 	}
 
-	trace_kvm_mmu_sync_page(sp);
 	if (rmap_write_protect(vcpu->kvm, sp->gfn))
 		kvm_flush_remote_tlbs(vcpu->kvm);
 	kvm_unlink_unsync_page(vcpu->kvm, sp);

@@ -1730,7 +1730,6 @@ static int kvm_unsync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 	struct kvm_mmu_page *s;
 	struct hlist_node *node, *n;
 
-	trace_kvm_mmu_unsync_page(sp);
 	index = kvm_page_table_hashfn(sp->gfn);
 	bucket = &vcpu->kvm->arch.mmu_page_hash[index];
 	/* don't unsync if pagetable is shadowed with multiple roles */

@@ -1740,6 +1739,7 @@ static int kvm_unsync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 		if (s->role.word != sp->role.word)
 			return 1;
 	}
+	trace_kvm_mmu_unsync_page(sp);
 	++vcpu->kvm->stat.mmu_unsync;
 	sp->unsync = 1;
Signature · 1 line
1.6.1.2
author
#7 lore raw

PATCH 4/10 KVM MMU: Move invlpg code out of paging_tmpl.h

In reply to a message not in the archive

Using '!sp->role.cr4_pae' replaces 'PTTYPE == 32' and using 'pte_size = sp->role.cr4_pae ? 8 : 4' replaces sizeof(pt_element_t)

Then no need compile twice for this code

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c         |   60 ++++++++++++++++++++++++++++++++++++++++++-
 arch/x86/kvm/paging_tmpl.h |   56 -----------------------------------------
 2 files changed, 58 insertions(+), 58 deletions(-)
arch/x86/kvm/mmu.c +58 -2
index abf8bd4..fac7c09 100644
@@ -2256,6 +2256,62 @@ static bool is_rsvd_bits_set(struct kvm_vcpu *vcpu, u64 gpte, int level)
 	return (gpte & vcpu->arch.mmu.rsvd_bits_mask[bit7][level-1]) != 0;
 }
 
+static void paging_invlpg(struct kvm_vcpu *vcpu, gva_t gva)
+{
+	struct kvm_shadow_walk_iterator iterator;
+	gpa_t pte_gpa = -1;
+	int level;
+	u64 *sptep;
+	int need_flush = 0;
+	unsigned pte_size = 0;
+
+	spin_lock(&vcpu->kvm->mmu_lock);
+
+	for_each_shadow_entry(vcpu, gva, iterator) {
+		level = iterator.level;
+		sptep = iterator.sptep;
+
+		if (level == PT_PAGE_TABLE_LEVEL  ||
+		    ((level == PT_DIRECTORY_LEVEL && is_large_pte(*sptep))) ||
+		    ((level == PT_PDPE_LEVEL && is_large_pte(*sptep)))) {
+			struct kvm_mmu_page *sp = page_header(__pa(sptep));
+			int offset = 0;
+
+			if (!sp->role.cr4_pae)
+				offset = sp->role.quadrant << PT64_LEVEL_BITS;;
+			pte_size = sp->role.cr4_pae ? 8 : 4;
+			pte_gpa = (sp->gfn << PAGE_SHIFT);
+			pte_gpa += (sptep - sp->spt + offset) * pte_size;
+
+			if (is_shadow_present_pte(*sptep)) {
+				rmap_remove(vcpu->kvm, sptep);
+				if (is_large_pte(*sptep))
+					--vcpu->kvm->stat.lpages;
+				need_flush = 1;
+			}
+			__set_spte(sptep, shadow_trap_nonpresent_pte);
+			break;
+		}
+
+		if (!is_shadow_present_pte(*sptep))
+			break;
+	}
+
+	if (need_flush)
+		kvm_flush_remote_tlbs(vcpu->kvm);
+
+	atomic_inc(&vcpu->kvm->arch.invlpg_counter);
+
+	spin_unlock(&vcpu->kvm->mmu_lock);
+
+	if (pte_gpa == -1)
+		return;
+
+	if (mmu_topup_memory_caches(vcpu))
+		return;
+	kvm_mmu_pte_write(vcpu, pte_gpa, NULL, pte_size, 0);
+}
+
 #define PTTYPE 64
 #include "paging_tmpl.h"
 #undef PTTYPE

@@ -2335,7 +2391,7 @@ static int paging64_init_context_common(struct kvm_vcpu *vcpu, int level)
 	context->gva_to_gpa = paging64_gva_to_gpa;
 	context->prefetch_page = paging64_prefetch_page;
 	context->sync_page = paging64_sync_page;
-	context->invlpg = paging64_invlpg;
+	context->invlpg = paging_invlpg;
 	context->free = paging_free;
 	context->root_level = level;
 	context->shadow_root_level = level;

@@ -2360,7 +2416,7 @@ static int paging32_init_context(struct kvm_vcpu *vcpu)
 	context->free = paging_free;
 	context->prefetch_page = paging32_prefetch_page;
 	context->sync_page = paging32_sync_page;
-	context->invlpg = paging32_invlpg;
+	context->invlpg = paging_invlpg;
 	context->root_level = PT32_ROOT_LEVEL;
 	context->shadow_root_level = PT32E_ROOT_LEVEL;
 	context->root_hpa = INVALID_PAGE;
arch/x86/kvm/paging_tmpl.h +0 -56

62 lines not on this page · raw message

Signature · 1 line
1.6.1.2
#8 lore raw

Re: [PATCH 4/10] KVM MMU: Move invlpg code out of paging_tmpl.h

Quoted 66 lines · Xiao Guangrong

Using '!sp->role.cr4_pae' replaces 'PTTYPE == 32' and using 'pte_size = sp->role.cr4_pae ? 8 : 4' replaces sizeof(pt_element_t)

Then no need compile twice for this code

Signed-off-by Xiao Guangrong<[email protected]>
---
  arch/x86/kvm/mmu.c         |   60 ++++++++++++++++++++++++++++++++++++++++++-
  arch/x86/kvm/paging_tmpl.h |   56 -----------------------------------------
  2 files changed, 58 insertions(+), 58 deletions(-)
arch/x86/kvm/mmu.c +47 -0
index abf8bd4..fac7c09 100644
@@ -2256,6 +2256,62 @@ static bool is_rsvd_bits_set(struct kvm_vcpu *vcpu, u64 gpte, int level)
  	return (gpte&  vcpu->arch.mmu.rsvd_bits_mask[bit7][level-1]) != 0;
  }
 
+static void paging_invlpg(struct kvm_vcpu *vcpu, gva_t gva)
+{
+	struct kvm_shadow_walk_iterator iterator;
+	gpa_t pte_gpa = -1;
+	int level;
+	u64 *sptep;
+	int need_flush = 0;
+	unsigned pte_size = 0;
+
+	spin_lock(&vcpu->kvm->mmu_lock);
+
+	for_each_shadow_entry(vcpu, gva, iterator) {
+		level = iterator.level;
+		sptep = iterator.sptep;
+
+		if (level == PT_PAGE_TABLE_LEVEL  ||
+		    ((level == PT_DIRECTORY_LEVEL&&  is_large_pte(*sptep))) ||
+		    ((level == PT_PDPE_LEVEL&&  is_large_pte(*sptep)))) {
+			struct kvm_mmu_page *sp = page_header(__pa(sptep));
+			int offset = 0;
+
+			if (!sp->role.cr4_pae)
+				offset = sp->role.quadrant<<  PT64_LEVEL_BITS;;
+			pte_size = sp->role.cr4_pae ? 8 : 4;
+			pte_gpa = (sp->gfn<<  PAGE_SHIFT);
+			pte_gpa += (sptep - sp->spt + offset) * pte_size;
+
+			if (is_shadow_present_pte(*sptep)) {
+				rmap_remove(vcpu->kvm, sptep);
+				if (is_large_pte(*sptep))
+					--vcpu->kvm->stat.lpages;
+				need_flush = 1;
+			}
+			__set_spte(sptep, shadow_trap_nonpresent_pte);
+			break;
+		}
+
+		if (!is_shadow_present_pte(*sptep))
+			break;
+	}
+
+	if (need_flush)
+		kvm_flush_remote_tlbs(vcpu->kvm);
+
+	atomic_inc(&vcpu->kvm->arch.invlpg_counter);
+
+	spin_unlock(&vcpu->kvm->mmu_lock);

arch/x86/kvm/mmu.c static bool is_rsvd_bits_set(struct kvm_vcpu *vcpu, u64 gpte, int level) · line 2315

Quoted 10 lines
arch/x86/kvm/mmu.c +9 -0
+
+	if (pte_gpa == -1)
+		return;
+
+	if (mmu_topup_memory_caches(vcpu))
+		return;
+	kvm_mmu_pte_write(vcpu, pte_gpa, NULL, pte_size, 0);
+}
+

I think we should keep it in - kvm_mmu_pte_write() calls back to FNAME(update_pte), we could make the call directly from here speed things up, since we already have the spte and don't need to look it up.

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
#9 lore raw

Re: [PATCH 4/10] KVM MMU: Move invlpg code out of paging_tmpl.h

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

Using '!sp->role.cr4_pae' replaces 'PTTYPE == 32' and using 'pte_size = sp->role.cr4_pae ? 8 : 4' replaces sizeof(pt_element_t)

Then no need compile twice for this code

I think we should keep it in - kvm_mmu_pte_write() calls back to FNAME(update_pte), we could make the call directly from here speed things up, since we already have the spte and don't need to look it up.

I see you do this in patches 9, 10 - but is it possible to use update_pte directly? I think we'll need to make guess_page_from_pte_write() part of paging_tmpl.h (in general anything that depends on pte size is better off in paging_tmpl.h).

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
author
#10 lore raw

Re: [PATCH 4/10] KVM MMU: Move invlpg code out of paging_tmpl.h

Quoted 12 lines · Avi Kivity
Quoted 10 lines · Avi Kivity
Quoted 5 lines · Xiao Guangrong

Using '!sp->role.cr4_pae' replaces 'PTTYPE == 32' and using 'pte_size = sp->role.cr4_pae ? 8 : 4' replaces sizeof(pt_element_t)

Then no need compile twice for this code

I think we should keep it in - kvm_mmu_pte_write() calls back to FNAME(update_pte), we could make the call directly from here speed things up, since we already have the spte and don't need to look it up.

Quoted 5 lines

I see you do this in patches 9, 10 - but is it possible to use update_pte directly? I think we'll need to make guess_page_from_pte_write() part of paging_tmpl.h (in general anything that depends on pte size is better off in paging_tmpl.h).

OK, i'll keep invlpg code in paging_tmpl.h and directly call FNAME(update_pte).

But, i don't see mmu_guess_page_from_pte_write() code depends on pte size. :-(

Xiao

#11 lore raw

Re: [PATCH 4/10] KVM MMU: Move invlpg code out of paging_tmpl.h

Quoted 5 lines · Xiao Guangrong

OK, i'll keep invlpg code in paging_tmpl.h and directly call FNAME(update_pte).

But, i don't see mmu_guess_page_from_pte_write() code depends on pte size. :-(

It doesn't indeed, I misremembered. It's mmu_pte_write_new_pte() (which is no longer needed).

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.