kvm mmu: reduce 50% memory usage

PATCH Applied Lai Jiangshan 3 participants 14 messages Apr 28, 2010 – May 26, 2010 2 messages not in the archive

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Lai Jiangshan

author

2010-04-28 11:56 UTC #0 lore raw

PATCH 1/1 kvm mmu: reduce 50% memory usage

I think users will enable tdp when their hardwares support ept or npt. This patch can reduce about 50% kvm mmu memory usage for they.

This simple patch use the fact that:

When sp->role.direct is set, sp->gfns does not contain any essential information, leaf sptes reachable from this sp are for a continuate guest physical memory range(a linear range).
So sp->gfns[i](if it was set) equals to sp->gfn + i. (PT_PAGE_TABLE_LEVEL) Obviously, it is not essential information, we can calculate it when need.

It means we don't need sp->gfns when sp->role.direct=1, Thus we can save one page usage for every kvm_mmu_page.

Note:
Access to sp->gfns must be wrapped by kvm_mmu_page_get_gfn() or kvm_mmu_page_set_gfn().
It is only exposed in FNAME(sync_page).

Signed-off-by Lai Jiangshan <[email protected]>

---
 mmu.c         |   38 +++++++++++++++++++++++++++++---------
 paging_tmpl.h |    3 +++
 2 files changed, 32 insertions(+), 9 deletions(-)

arch/x86/kvm/mmu.c +29 -9

index ddfa865..a5c6719 100644
@@ -393,6 +393,22 @@ static void mmu_free_rmap_desc(struct kvm_rmap_desc *rd)
 	kfree(rd);
 }
 
+static gfn_t kvm_mmu_page_get_gfn(struct kvm_mmu_page *sp, int index)
+{
+	if (!sp->role.direct)
+		return sp->gfns[index];
+
+	return sp->gfn + (index << ((sp->role.level - 1) * PT64_LEVEL_BITS));
+}
+
+static void kvm_mmu_page_set_gfn(struct kvm_mmu_page *sp, int index, gfn_t gfn)
+{
+	if (sp->role.direct)
+		BUG_ON(gfn != kvm_mmu_page_get_gfn(sp, index));
+	else
+		sp->gfns[index] = gfn;
+}
+
 /*
  * Return the pointer to the largepage write count for a given
  * gfn, handling slots that are not large page aligned.

@@ -543,7 +559,7 @@ static int rmap_add(struct kvm_vcpu *vcpu, u64 *spte, gfn_t gfn)
 		return count;
 	gfn = unalias_gfn(vcpu->kvm, gfn);
 	sp = page_header(__pa(spte));
-	sp->gfns[spte - sp->spt] = gfn;
+	kvm_mmu_page_set_gfn(sp, spte - sp->spt, gfn);
 	rmapp = gfn_to_rmap(vcpu->kvm, gfn, sp->role.level);
 	if (!*rmapp) {
 		rmap_printk("rmap_add: %p %llx 0->1\n", spte, *spte);

@@ -601,6 +617,7 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
 	struct kvm_rmap_desc *prev_desc;
 	struct kvm_mmu_page *sp;
 	pfn_t pfn;
+	gfn_t gfn;
 	unsigned long *rmapp;
 	int i;

@@ -612,7 +629,8 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
 		kvm_set_pfn_accessed(pfn);
 	if (is_writable_pte(*spte))
 		kvm_set_pfn_dirty(pfn);
-	rmapp = gfn_to_rmap(kvm, sp->gfns[spte - sp->spt], sp->role.level);
+	gfn = kvm_mmu_page_get_gfn(sp, spte - sp->spt);
+	rmapp = gfn_to_rmap(kvm, gfn, sp->role.level);
 	if (!*rmapp) {
 		printk(KERN_ERR "rmap_remove: %p %llx 0->BUG\n", spte, *spte);
 		BUG();

@@ -896,7 +914,8 @@ static void kvm_mmu_free_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 	ASSERT(is_empty_shadow_page(sp->spt));
 	list_del(&sp->link);
 	__free_page(virt_to_page(sp->spt));
-	__free_page(virt_to_page(sp->gfns));
+	if (!sp->role.direct)
+		__free_page(virt_to_page(sp->gfns));
 	kfree(sp);
 	++kvm->arch.n_free_mmu_pages;
 }

@@ -907,13 +926,15 @@ static unsigned kvm_page_table_hashfn(gfn_t gfn)
 }
 
 static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu,
-					       u64 *parent_pte)
+					       u64 *parent_pte, int direct)
 {
 	struct kvm_mmu_page *sp;
 
 	sp = mmu_memory_cache_alloc(&vcpu->arch.mmu_page_header_cache, sizeof *sp);
 	sp->spt = mmu_memory_cache_alloc(&vcpu->arch.mmu_page_cache, PAGE_SIZE);
-	sp->gfns = mmu_memory_cache_alloc(&vcpu->arch.mmu_page_cache, PAGE_SIZE);
+	if (!direct)
+		sp->gfns = mmu_memory_cache_alloc(&vcpu->arch.mmu_page_cache,
+						  PAGE_SIZE);
 	set_page_private(virt_to_page(sp->spt), (unsigned long)sp);
 	list_add(&sp->link, &vcpu->kvm->arch.active_mmu_pages);
 	bitmap_zero(sp->slot_bitmap, KVM_MEMORY_SLOTS + KVM_PRIVATE_MEM_SLOTS);

@@ -1347,7 +1368,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 			return sp;
 		}
 	++vcpu->kvm->stat.mmu_cache_miss;
-	sp = kvm_mmu_alloc_page(vcpu, parent_pte);
+	sp = kvm_mmu_alloc_page(vcpu, parent_pte, direct);
 	if (!sp)
 		return sp;
 	sp->gfn = gfn;

@@ -3314,7 +3335,7 @@ void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
 
 	if (*sptep & PT_WRITABLE_MASK) {
 		rev_sp = page_header(__pa(sptep));
-		gfn = rev_sp->gfns[sptep - rev_sp->spt];
+		gfn = kvm_mmu_page_get_gfn(rev_sp, sptep - rev_sp->spt);
 
 		if (!gfn_to_memslot(kvm, gfn)) {
 			if (!printk_ratelimit())

@@ -3328,8 +3349,7 @@ void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
 			return;
 		}
 
-		rmapp = gfn_to_rmap(kvm, rev_sp->gfns[sptep - rev_sp->spt],
-				    rev_sp->role.level);
+		rmapp = gfn_to_rmap(kvm, gfn, rev_sp->role.level);
 		if (!*rmapp) {
 			if (!printk_ratelimit())
 				return;

arch/x86/kvm/paging_tmpl.h +3 -0

index d0cc07e..702c016 100644
@@ -576,6 +576,9 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 
 	offset = nr_present = 0;
 
+	/* direct kvm_mmu_page can not be unsync. */
+	BUG_ON(sp->role.direct);
+
 	if (PTTYPE == 32)
 		offset = sp->role.quadrant << PT64_LEVEL_BITS;

Avi Kivity

2010-04-28 13:01 UTC #1 lore raw

Quoted 4 lines · Lai Jiangshan

I think users will enable tdp when their hardwares support ept or npt. This patch can reduce about 50% kvm mmu memory usage for they.

Good one!

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

Applied

2010-04-28 18:05 UTC #2 lore raw

In reply to Lai Jiangshan

Quoted 15 lines · Lai Jiangshan

I think users will enable tdp when their hardwares support ept or npt. This patch can reduce about 50% kvm mmu memory usage for they.
This simple patch use the fact that:
When sp->role.direct is set, sp->gfns does not contain any essential information, leaf sptes reachable from this sp are for a continuate guest physical memory range(a linear range).
So sp->gfns[i](if it was set) equals to sp->gfn + i. (PT_PAGE_TABLE_LEVEL) Obviously, it is not essential information, we can calculate it when need.
It means we don't need sp->gfns when sp->role.direct=1, Thus we can save one page usage for every kvm_mmu_page.

Quoted 6 lines

Note:
Access to sp->gfns must be wrapped by kvm_mmu_page_get_gfn() or kvm_mmu_page_set_gfn().
It is only exposed in FNAME(sync_page).
Signed-off-by Lai Jiangshan <[email protected]>

Applied, thanks.

Marcelo Tosatti

2010-04-29 18:09 UTC #3 lore raw

In reply to Lai Jiangshan

Quoted 12 lines · Lai Jiangshan

I think users will enable tdp when their hardwares support ept or npt. This patch can reduce about 50% kvm mmu memory usage for they.
This simple patch use the fact that:
When sp->role.direct is set, sp->gfns does not contain any essential information, leaf sptes reachable from this sp are for a continuate guest physical memory range(a linear range).
So sp->gfns[i](if it was set) equals to sp->gfn + i. (PT_PAGE_TABLE_LEVEL) Obviously, it is not essential information, we can calculate it when need.

Quoted 7 lines

It means we don't need sp->gfns when sp->role.direct=1, Thus we can save one page usage for every kvm_mmu_page.
Note:
Access to sp->gfns must be wrapped by kvm_mmu_page_get_gfn() or kvm_mmu_page_set_gfn().
It is only exposed in FNAME(sync_page).

Lai,

You missed quadrant on 4mb large page emulation with shadow (see updated patch below). Also for some reason i can't understand the assumption does not hold for large sptes with TDP, so reverted for now.

arch/x86/kvm/mmu.c +35 ~~-10~~

index 3266d73..a9edfdb 100644
@@ -393,6 +393,27 @@ static void mmu_free_rmap_desc(struct kvm_rmap_desc *rd)
 	kfree(rd);
 }
 
+static gfn_t kvm_mmu_page_get_gfn(struct kvm_mmu_page *sp, int index)
+{
+	gfn_t gfn;
+
+	if (!sp->role.direct)
+		return sp->gfns[index];
+
+	gfn = sp->gfn + index * (1 << (sp->role.level - 1) * PT64_LEVEL_BITS);
+	gfn += sp->role.quadrant << PT64_LEVEL_BITS;
+
+	return gfn;
+}
+
+static void kvm_mmu_page_set_gfn(struct kvm_mmu_page *sp, int index, gfn_t gfn)
+{
+	if (sp->role.direct)
+		BUG_ON(gfn != kvm_mmu_page_get_gfn(sp, index));
+	else
+		sp->gfns[index] = gfn;
+}
+
 /*
  * Return the pointer to the largepage write count for a given
  * gfn, handling slots that are not large page aligned.

@@ -543,7 +564,7 @@ static int rmap_add(struct kvm_vcpu *vcpu, u64 *spte, gfn_t gfn)
 		return count;
 	gfn = unalias_gfn(vcpu->kvm, gfn);
 	sp = page_header(__pa(spte));
-	sp->gfns[spte - sp->spt] = gfn;
+	kvm_mmu_page_set_gfn(sp, spte - sp->spt, gfn);
 	rmapp = gfn_to_rmap(vcpu->kvm, gfn, sp->role.level);
 	if (!*rmapp) {
 		rmap_printk("rmap_add: %p %llx 0->1\n", spte, *spte);

@@ -601,6 +622,7 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
 	struct kvm_rmap_desc *prev_desc;
 	struct kvm_mmu_page *sp;
 	pfn_t pfn;
+	gfn_t gfn;
 	unsigned long *rmapp;
 	int i;

@@ -612,7 +634,8 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
 		kvm_set_pfn_accessed(pfn);
 	if (is_writable_pte(*spte))
 		kvm_set_pfn_dirty(pfn);
-	rmapp = gfn_to_rmap(kvm, sp->gfns[spte - sp->spt], sp->role.level);
+	gfn = kvm_mmu_page_get_gfn(sp, spte - sp->spt);
+	rmapp = gfn_to_rmap(kvm, gfn, sp->role.level);
 	if (!*rmapp) {
 		printk(KERN_ERR "rmap_remove: %p %llx 0->BUG\n", spte, *spte);
 		BUG();

@@ -896,7 +919,8 @@ static void kvm_mmu_free_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 	ASSERT(is_empty_shadow_page(sp->spt));
 	list_del(&sp->link);
 	__free_page(virt_to_page(sp->spt));
-	__free_page(virt_to_page(sp->gfns));
+	if (!sp->role.direct)
+		__free_page(virt_to_page(sp->gfns));
 	kfree(sp);
 	++kvm->arch.n_free_mmu_pages;
 }

@@ -907,13 +931,15 @@ static unsigned kvm_page_table_hashfn(gfn_t gfn)
 }
 
 static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu,
-					       u64 *parent_pte)
+					       u64 *parent_pte, int direct)
 {
 	struct kvm_mmu_page *sp;
 
 	sp = mmu_memory_cache_alloc(&vcpu->arch.mmu_page_header_cache, sizeof *sp);
 	sp->spt = mmu_memory_cache_alloc(&vcpu->arch.mmu_page_cache, PAGE_SIZE);
-	sp->gfns = mmu_memory_cache_alloc(&vcpu->arch.mmu_page_cache, PAGE_SIZE);
+	if (!direct)
+		sp->gfns = mmu_memory_cache_alloc(&vcpu->arch.mmu_page_cache,
+						  PAGE_SIZE);
 	set_page_private(virt_to_page(sp->spt), (unsigned long)sp);
 	list_add(&sp->link, &vcpu->kvm->arch.active_mmu_pages);
 	bitmap_zero(sp->slot_bitmap, KVM_MEMORY_SLOTS + KVM_PRIVATE_MEM_SLOTS);

@@ -1352,7 +1378,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 	if (role.direct)
 		role.cr4_pae = 0;
 	role.access = access;
-	if (vcpu->arch.mmu.root_level <= PT32_ROOT_LEVEL) {
+	if (vcpu->arch.mmu.root_level == PT32_ROOT_LEVEL) {
 		quadrant = gaddr >> (PAGE_SHIFT + (PT64_PT_BITS * level));
 		quadrant &= (1 << ((PT32_PT_BITS - PT64_PT_BITS) * level)) - 1;
 		role.quadrant = quadrant;

@@ -1379,7 +1405,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 			return sp;
 		}
 	++vcpu->kvm->stat.mmu_cache_miss;
-	sp = kvm_mmu_alloc_page(vcpu, parent_pte);
+	sp = kvm_mmu_alloc_page(vcpu, parent_pte, direct);
 	if (!sp)
 		return sp;
 	sp->gfn = gfn;

@@ -3371,7 +3399,7 @@ void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
 
 	if (*sptep & PT_WRITABLE_MASK) {
 		rev_sp = page_header(__pa(sptep));
-		gfn = rev_sp->gfns[sptep - rev_sp->spt];
+		gfn = kvm_mmu_page_get_gfn(rev_sp, sptep - rev_sp->spt);
 
 		if (!gfn_to_memslot(kvm, gfn)) {
 			if (!printk_ratelimit())

@@ -3385,8 +3413,7 @@ void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
 			return;
 		}
 
-		rmapp = gfn_to_rmap(kvm, rev_sp->gfns[sptep - rev_sp->spt],
-				    rev_sp->role.level);
+		rmapp = gfn_to_rmap(kvm, gfn, rev_sp->role.level);
 		if (!*rmapp) {
 			if (!printk_ratelimit())
 				return;

arch/x86/kvm/paging_tmpl.h +3 -0

index 624b38f..2091590 100644
@@ -592,6 +592,9 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 
 	offset = nr_present = 0;
 
+	/* direct kvm_mmu_page can not be unsync. */
+	BUG_ON(sp->role.direct);
+
 	if (PTTYPE == 32)
 		offset = sp->role.quadrant << PT64_LEVEL_BITS;

Avi Kivity

2010-04-30 17:22 UTC #4 lore raw

Quoted 3 lines · Marcelo Tosatti

You missed quadrant on 4mb large page emulation with shadow (see updated patch below).

Good catch.

Quoted 3 lines

Also for some reason i can't understand the assumption does not hold for large sptes with TDP, so reverted for now.

It's unrelated to TDP, same issue with shadow. I think the calculation is correct. For example the 4th spte for a level=2 page will yield gfn=4*512.

Quoted 4 lines

arch/x86/kvm/mmu.c +0 -0

@@ -393,6 +393,27 @@ static void mmu_free_rmap_desc(struct kvm_rmap_desc *rd)
  	kfree(rd);
  }

arch/x86/kvm/mmu.c static void mmu_free_rmap_desc(struct kvm_rmap_desc *rd) · line 405

Quoted 10 lines

arch/x86/kvm/mmu.c +9 -0

+static gfn_t kvm_mmu_page_get_gfn(struct kvm_mmu_page *sp, int index)
+{
+	gfn_t gfn;
+
+	if (!sp->role.direct)
+		return sp->gfns[index];
+
+	gfn = sp->gfn + index * (1<<  (sp->role.level - 1) * PT64_LEVEL_BITS);
+	gfn += sp->role.quadrant<<  PT64_LEVEL_BITS;

PT64_LEVEL_BITS * level

Quoted 6 lines

arch/x86/kvm/mmu.c +4 -0
+
+	return gfn;
+}
+
 
 

Signature · 2 lines

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

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

Avi Kivity

2010-04-30 17:21 UTC #6 lore raw

Quoted 10 lines · Lai Jiangshan

Quoted 5 lines
It's unrelated to TDP, same issue with shadow. I think the calculation is correct. For example the 4th spte for a level=2 page will yield gfn=4*512.
Avi, Marcelo
Thank you very much.
The calculation I used is correct.

Yes. btw, can you update the patch to also correct mmu.txt?

Quoted 7 lines

The fault is comes from the FNAME(fetch) when using shadow page.
I have fix it in the patch
"[RFC PATCH] kvm: calculate correct gfn for small host pages which emulates large guest pages"
(It seems that the mail lists is unreachable, did you receive it?)

The lists are down at the moment.

Signature · 2 lines

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

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

Avi Kivity

2010-04-30 10:00 UTC #8 lore raw

Quoted 29 lines · Lai Jiangshan

Quoted 8 lines · Avi Kivity
Quoted 6 lines · Lai Jiangshan
Quoted 6 lines
It's unrelated to TDP, same issue with shadow. I think the calculation is correct. For example the 4th spte for a level=2 page will yield gfn=4*512.
Quoted 8 lines
Quoted 6 lines
Avi, Marcelo
Thank you very much.
The calculation I used is correct.
Yes. btw, can you update the patch to also correct mmu.txt?
The corresponding content in mmu.txt are:
role.direct:
If set, leaf sptes reachable from this page are for a linear range. Examples include real mode translation, large guest pages backed by small host pages, and gpa->hpa translations when NPT or EPT is active. The linear range starts at (gfn<< PAGE_SHIFT) and its size is determined by role.level (2MB for first level, 1GB for second level, 0.5TB for third level, 256TB for fourth level)
If clear, this page corresponds to a guest page table denoted by the gfn field.

Quoted 10 lines

gfn:
Either the guest page table containing the translations shadowed by this page, or the base page frame for linear translations. See role.direct.
These are correct. My patch is fully base on this document. I think it is not need to be fixed.
Did I miss something?

sp->gfns can now be NULL, so the documentation of this field needs to be updated.

Signature · 1 line

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

Marcelo Tosatti

2010-04-30 17:21 UTC #9 lore raw

In reply to Avi Kivity

Quoted 5 lines · Avi Kivity

Quoted 3 lines · Marcelo Tosatti
You missed quadrant on 4mb large page emulation with shadow (see updated patch below).

Quoted 8 lines

Good catch.
Quoted 2 lines
Also for some reason i can't understand the assumption does not hold for large sptes with TDP, so reverted for now.
It's unrelated to TDP, same issue with shadow. I think the calculation is correct. For example the 4th spte for a level=2 page will yield gfn=4*512.

Under testing i see sp at level 2, with sp->gfn == 4096, mmu_set_spte setting index 8 to gfn 4096 (whereas kvm_mmu_page_get_gfn returns 4096 + 8*512).

Lai, can you please take a look at it? You should see the kvm_mmu_page_set_gfn BUG_ON by using -mem-path on hugetlbfs.

Quoted 26 lines

arch/x86/kvm/mmu.c static void mmu_free_rmap_desc(struct kvm_rmap_desc *rd) · line 404

Quoted 13 lines

arch/x86/kvm/mmu.c +9 -0

@@ -393,6 +393,27 @@ static void mmu_free_rmap_desc(struct kvm_rmap_desc *rd)
 	kfree(rd);
 }
 
+static gfn_t kvm_mmu_page_get_gfn(struct kvm_mmu_page *sp, int index)
+{
+	gfn_t gfn;
+
+	if (!sp->role.direct)
+		return sp->gfns[index];
+
+	gfn = sp->gfn + index * (1<<  (sp->role.level - 1) * PT64_LEVEL_BITS);
+	gfn += sp->role.quadrant<<  PT64_LEVEL_BITS;

PT64_LEVEL_BITS * level

Quoted 5 lines

arch/x86/kvm/mmu.c +4 -0
+
+	return gfn;
+}
+
 

Signature · 2 lines

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

Lai Jiangshan

author

2010-05-06 07:03 UTC #10 lore raw

Quoted 12 lines · Marcelo Tosatti

Quoted 10 lines · Avi Kivity
Quoted 2 lines · Marcelo Tosatti
You missed quadrant on 4mb large page emulation with shadow (see updated patch below).
Good catch.
Quoted 2 lines
Also for some reason i can't understand the assumption does not hold for large sptes with TDP, so reverted for now.
It's unrelated to TDP, same issue with shadow. I think the calculation is correct. For example the 4th spte for a level=2 page will yield gfn=4*512.

Quoted 7 lines

Under testing i see sp at level 2, with sp->gfn == 4096, mmu_set_spte setting index 8 to gfn 4096 (whereas kvm_mmu_page_get_gfn returns 4096 + 8*512).
Lai, can you please take a look at it? You should see the kvm_mmu_page_set_gfn BUG_ON by using -mem-path on hugetlbfs.

Could you tell me how you test it? It will be better if I follow your test steps.

I also hit the kvm_mmu_page_set_gfn BUG_ON, It is because
FNAME(fetch)() set sp->gfn wrong. The patch:
[PATCH] kvm: calculate correct gfn for small host pages which emulates large guest pages fix it.

I can not hit kvm_mmu_page_set_gfn BUG_ON after this patch also applied.

So could you tell me your test steps:
The host: ept/npt enabled? 64bit? testing codes in host? The guest: OS? PAE? 32bit? 64bit? testing codes in guest?

Lai

Marcelo Tosatti

2010-05-06 19:04 UTC #11 lore raw

Quoted 17 lines · Lai Jiangshan

Quoted 16 lines · Marcelo Tosatti
Quoted 10 lines · Avi Kivity
Quoted 2 lines · Marcelo Tosatti
You missed quadrant on 4mb large page emulation with shadow (see updated patch below).
Good catch.
Quoted 2 lines
Also for some reason i can't understand the assumption does not hold for large sptes with TDP, so reverted for now.
It's unrelated to TDP, same issue with shadow. I think the calculation is correct. For example the 4th spte for a level=2 page will yield gfn=4*512.
Under testing i see sp at level 2, with sp->gfn == 4096, mmu_set_spte setting index 8 to gfn 4096 (whereas kvm_mmu_page_get_gfn returns 4096 + 8*512).

Quoted 6 lines

Quoted 3 lines
Lai, can you please take a look at it? You should see the kvm_mmu_page_set_gfn BUG_ON by using -mem-path on hugetlbfs.
Could you tell me how you test it? It will be better if I follow your test steps.

mount -t hugetlbfs none /mnt/
echo xyz > /proc/sys/vm/nr_hugepages
qemu-kvm parameters -mem-path /mnt/

Quoted 10 lines

I also hit the kvm_mmu_page_set_gfn BUG_ON, It is because
FNAME(fetch)() set sp->gfn wrong. The patch:
[PATCH] kvm: calculate correct gfn for small host pages which emulates large guest pages fix it.
I can not hit kvm_mmu_page_set_gfn BUG_ON after this patch also applied.
So could you tell me your test steps:
The host: ept/npt enabled? 64bit? testing codes in host?

Intel EPT enabled.

Quoted 1 line

The guest: OS? PAE? 32bit? 64bit? testing codes in guest?

FC12 guest.

Lai Jiangshan

author

2010-05-07 08:25 UTC #12 lore raw

Quoted 35 lines · Marcelo Tosatti

Quoted 16 lines · Lai Jiangshan
Quoted 15 lines · Marcelo Tosatti
Quoted 10 lines · Avi Kivity
Quoted 2 lines · Marcelo Tosatti
You missed quadrant on 4mb large page emulation with shadow (see updated patch below).
Good catch.
Quoted 2 lines
Also for some reason i can't understand the assumption does not hold for large sptes with TDP, so reverted for now.
It's unrelated to TDP, same issue with shadow. I think the calculation is correct. For example the 4th spte for a level=2 page will yield gfn=4*512.
Under testing i see sp at level 2, with sp->gfn == 4096, mmu_set_spte setting index 8 to gfn 4096 (whereas kvm_mmu_page_get_gfn returns 4096 + 8*512).
Quoted 5 lines
Quoted 3 lines
Lai, can you please take a look at it? You should see the kvm_mmu_page_set_gfn BUG_ON by using -mem-path on hugetlbfs.
Could you tell me how you test it? It will be better if I follow your test steps.
mount -t hugetlbfs none /mnt/
echo xyz > /proc/sys/vm/nr_hugepages
qemu-kvm parameters -mem-path /mnt/
Quoted 8 lines
I also hit the kvm_mmu_page_set_gfn BUG_ON, It is because
FNAME(fetch)() set sp->gfn wrong. The patch:
[PATCH] kvm: calculate correct gfn for small host pages which emulates large guest pages fix it.
I can not hit kvm_mmu_page_set_gfn BUG_ON after this patch also applied.

Quoted 9 lines

Quoted 2 lines
So could you tell me your test steps:
The host: ept/npt enabled? 64bit? testing codes in host?
Intel EPT enabled.
Quoted 1 line
The guest: OS? PAE? 32bit? 64bit? testing codes in guest?
FC12 guest.

I forgot to check if the calculation of base gfn is correct in __direct_map().

Subject: [PATCH] kvm, tdp: calculate correct base gfn for non-DIR level

the base gfn calculation is incorrect in __direct_map(), it does not calculate correctly when level=3 or 4.

Signed-off-by Lai Jiangshan <[email protected]>

Reported-by Marcelo Tosatti <[email protected]>

---

arch/x86/kvm/mmu.c +4 -1

index a5c6719..6986a6f 100644
@@ -1955,7 +1956,10 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 		}
 
 		if (*iterator.sptep == shadow_trap_nonpresent_pte) {
-			pseudo_gfn = (iterator.addr & PT64_DIR_BASE_ADDR_MASK) >> PAGE_SHIFT;
+			u64 base_addr = iterator.addr;
+
+			base_addr &= PT64_LVL_ADDR_MASK(iterator.level);
+			pseudo_gfn = base_addr >> PAGE_SHIFT;
 			sp = kvm_mmu_get_page(vcpu, pseudo_gfn, iterator.addr,
 					      iterator.level - 1,
 					      1, ACC_ALL, iterator.sptep);

Marcelo Tosatti

2010-05-07 20:56 UTC #13 lore raw

Quoted 14 lines · Lai Jiangshan

Subject: [PATCH] kvm, tdp: calculate correct base gfn for non-DIR level
the base gfn calculation is incorrect in __direct_map(), it does not calculate correctly when level=3 or 4.
Signed-off-by Lai Jiangshan <[email protected]>
Reported-by Marcelo Tosatti <[email protected]>
---
arch/x86/kvm/mmu.c +0 -0
index a5c6719..6986a6f 100644
@@ -1955,7 +1956,10 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 		}

arch/x86/kvm/mmu.c static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write, · line 1965

Quoted 10 lines

arch/x86/kvm/mmu.c +4 -1

 
 		if (*iterator.sptep == shadow_trap_nonpresent_pte) {
-			pseudo_gfn = (iterator.addr & PT64_DIR_BASE_ADDR_MASK) >> PAGE_SHIFT;
+			u64 base_addr = iterator.addr;
+
+			base_addr &= PT64_LVL_ADDR_MASK(iterator.level);
+			pseudo_gfn = base_addr >> PAGE_SHIFT;
 			sp = kvm_mmu_get_page(vcpu, pseudo_gfn, iterator.addr,
 					      iterator.level - 1,
 					      1, ACC_ALL, iterator.sptep);

Looks good, please resend the whole patch.

Lai Jiangshan

author

2010-05-26 08:48 UTC #14 lore raw

PATCH RESEND 2/3 kvm, tdp: calculate correct base gfn for non-DIR level

In reply to Marcelo Tosatti

In Document/kvm/mmu.txt:
gfn:
Either the guest page table containing the translations shadowed by this page, or the base page frame for linear translations. See role.direct.

But in __direct_map(), the base gfn calculation is incorrect, it does not calculate correctly when level=3 or 4.

Reported-by Marcelo Tosatti <[email protected]>

Signed-off-by Lai Jiangshan <[email protected]>

---

arch/x86/kvm/mmu.c +4 -1

index a5c6719..6986a6f 100644
@@ -1955,7 +1956,10 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 		}
 
 		if (*iterator.sptep == shadow_trap_nonpresent_pte) {
-			pseudo_gfn = (iterator.addr & PT64_DIR_BASE_ADDR_MASK) >> PAGE_SHIFT;
+			u64 base_addr = iterator.addr;
+
+			base_addr &= PT64_LVL_ADDR_MASK(iterator.level);
+			pseudo_gfn = base_addr >> PAGE_SHIFT;
 			sp = kvm_mmu_get_page(vcpu, pseudo_gfn, iterator.addr,
 					      iterator.level - 1,
 					      1, ACC_ALL, iterator.sptep);

Avi Kivity

Applied

2010-05-26 10:15 UTC #15 lore raw

Re: [RESEND PATCH 2/3] kvm, tdp: calculate correct base gfn for non-DIR level

Quoted 8 lines · Lai Jiangshan

In Document/kvm/mmu.txt:
gfn:
Either the guest page table containing the translations shadowed by this page, or the base page frame for linear translations. See role.direct.
But in __direct_map(), the base gfn calculation is incorrect, it does not calculate correctly when level=3 or 4.

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function