KVM: MMU: fix release pfn in mmu code

PATCH Superseded Xiao Guangrong 3 participants 17 messages Sep 14, 2012 – Sep 20, 2012

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Xiao Guangrong

author

2012-09-14 09:56 UTC #0 lore raw

v2 cover KVM: MMU: fix release pfn in mmu code

Release pfn in the mmu code is little special for we allow no-slot pfn go to spte walk on page fault path, that means, on page fault fail path, we can not directly call kvm_release_pfn_clean.

This patchset fixes the bug which release no-slot pfn on fail path and clean up all the paths where kvm_release_pfn_clean is called

Xiao Guangrong

author

2012-09-14 09:57 UTC #1 lore raw

PATCH v2 1/5 KVM: MMU: release noslot pfn on the fail path properly

We can not directly call kvm_release_pfn_clean to release the pfn since we can meet noslot pfn which is used to cache mmio info into spte

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c         |    6 ++++--
 arch/x86/kvm/paging_tmpl.h |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

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

index aa0b469..f74c63a 100644
@@ -2877,7 +2877,8 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,
 
 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);
-	kvm_release_pfn_clean(pfn);
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
 	return 0;
 }

@@ -3345,7 +3346,8 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa, u32 error_code,
 
 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);
-	kvm_release_pfn_clean(pfn);
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
 	return 0;
 }

arch/x86/kvm/paging_tmpl.h +4 -2

index bf8c42b..c004ab6 100644
@@ -544,7 +544,8 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 out_gpte_changed:
 	if (sp)
 		kvm_mmu_put_page(sp, it.sptep);
-	kvm_release_pfn_clean(pfn);
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
 	return NULL;
 }

@@ -645,7 +646,8 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 
 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);
-	kvm_release_pfn_clean(pfn);
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
 	return 0;
 }

Signature · 1 line

1.7.7.6

Marcelo Tosatti

2012-09-15 15:13 UTC #2 lore raw

Re: [PATCH v2 1/5] KVM: MMU: release noslot pfn on the fail path properly

Quoted 9 lines · Xiao Guangrong

We can not directly call kvm_release_pfn_clean to release the pfn since we can meet noslot pfn which is used to cache mmio info into spte
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c         |    6 ++++--
 arch/x86/kvm/paging_tmpl.h |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

Its clearer to the reader if is_invalid_pfn() is used instead of is_error_pfn.

BTW how about killing this unused helper

static bool mmu_invalid_pfn(pfn_t pfn)

{
        return unlikely(is_invalid_pfn(pfn));
}

This can be done inlined.

Quoted 13 lines

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

index aa0b469..f74c63a 100644
@@ -2877,7 +2877,8 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,
 
 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);
-	kvm_release_pfn_clean(pfn);
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
 	return 0;
 }

Xiao Guangrong

author

2012-09-18 07:46 UTC #3 lore raw

Re: [PATCH v2 1/5] KVM: MMU: release noslot pfn on the fail path properly

Quoted 14 lines · Marcelo Tosatti

Quoted 9 lines · Xiao Guangrong
We can not directly call kvm_release_pfn_clean to release the pfn since we can meet noslot pfn which is used to cache mmio info into spte
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c         |    6 ++++--
 arch/x86/kvm/paging_tmpl.h |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)
Its clearer to the reader if is_invalid_pfn() is used instead of is_error_pfn.

Quoted 8 lines

BTW how about killing this unused helper
static bool mmu_invalid_pfn(pfn_t pfn)
{
        return unlikely(is_invalid_pfn(pfn));
}
This can be done inlined.

Okay, will do. Thanks Marcelo!

Xiao Guangrong

author

2012-09-14 09:57 UTC #4 lore raw

PATCH v2 2/5 KVM: MMU: do not release pfn in mmu_set_spte

In reply to Xiao Guangrong

It helps us to cleanup release pfn in the later patches

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c         |   29 ++++++++++++++---------------
 arch/x86/kvm/paging_tmpl.h |   18 +++++++++++-------
 2 files changed, 25 insertions(+), 22 deletions(-)

arch/x86/kvm/mmu.c +14 ~~-15~~

index f74c63a..29ce28b 100644
@@ -2496,9 +2496,6 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 				rmap_recycle(vcpu, sptep, gfn);
 		}
 	}
-
-	if (!is_error_pfn(pfn))
-		kvm_release_pfn_clean(pfn);
 }
 
 static void nonpaging_new_cr3(struct kvm_vcpu *vcpu)

@@ -2535,12 +2532,15 @@ static int direct_pte_prefetch_many(struct kvm_vcpu *vcpu,
 	if (ret <= 0)
 		return -1;
 
-	for (i = 0; i < ret; i++, gfn++, start++)
+	for (i = 0; i < ret; i++, gfn++, start++) {
 		mmu_set_spte(vcpu, start, ACC_ALL,
 			     access, 0, 0, NULL,
 			     sp->role.level, gfn,
 			     page_to_pfn(pages[i]), true, true);
 
+		kvm_release_page_clean(pages[i]);
+	}
+
 	return 0;
 }

@@ -2863,23 +2863,22 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,
 		return r;
 
 	spin_lock(&vcpu->kvm->mmu_lock);
-	if (mmu_notifier_retry(vcpu, mmu_seq))
+	if (mmu_notifier_retry(vcpu, mmu_seq)) {
+		r = 0;
 		goto out_unlock;
+	}
+
 	kvm_mmu_free_some_pages(vcpu);
 	if (likely(!force_pt_level))
 		transparent_hugepage_adjust(vcpu, &gfn, &pfn, &level);
 	r = __direct_map(vcpu, v, write, map_writable, level, gfn, pfn,
 			 prefault);
-	spin_unlock(&vcpu->kvm->mmu_lock);
-
-
-	return r;
 
 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);
 	if (!is_error_pfn(pfn))
 		kvm_release_pfn_clean(pfn);
-	return 0;
+	return r;
 }

@@ -3333,22 +3332,22 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa, u32 error_code,
 		return r;
 
 	spin_lock(&vcpu->kvm->mmu_lock);
-	if (mmu_notifier_retry(vcpu, mmu_seq))
+	if (mmu_notifier_retry(vcpu, mmu_seq)) {
+		r = 0;
 		goto out_unlock;
+	}
+
 	kvm_mmu_free_some_pages(vcpu);
 	if (likely(!force_pt_level))
 		transparent_hugepage_adjust(vcpu, &gfn, &pfn, &level);
 	r = __direct_map(vcpu, gpa, write, map_writable,
 			 level, gfn, pfn, prefault);
-	spin_unlock(&vcpu->kvm->mmu_lock);
-
-	return r;
 
 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);
 	if (!is_error_pfn(pfn))
 		kvm_release_pfn_clean(pfn);
-	return 0;
+	return r;
 }
 
 static void nonpaging_free(struct kvm_vcpu *vcpu)

arch/x86/kvm/paging_tmpl.h +11 -7

index c004ab6..92f466c 100644
@@ -380,6 +380,9 @@ static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 	mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
 		     NULL, PT_PAGE_TABLE_LEVEL,
 		     gpte_to_gfn(gpte), pfn, true, true);
+
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
 }
 
 static bool FNAME(gpte_changed)(struct kvm_vcpu *vcpu,

@@ -452,6 +455,8 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
 		mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
 			     NULL, PT_PAGE_TABLE_LEVEL, gfn,
 			     pfn, true, true);
+		if (!is_error_pfn(pfn))
+			kvm_release_pfn_clean(pfn);
 	}
 }

@@ -544,8 +549,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 out_gpte_changed:
 	if (sp)
 		kvm_mmu_put_page(sp, it.sptep);
-	if (!is_error_pfn(pfn))
-		kvm_release_pfn_clean(pfn);
+
 	return NULL;
 }

@@ -625,8 +629,10 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 		return r;
 
 	spin_lock(&vcpu->kvm->mmu_lock);
-	if (mmu_notifier_retry(vcpu, mmu_seq))
+	if (mmu_notifier_retry(vcpu, mmu_seq)) {
+		r = 0;
 		goto out_unlock;
+	}
 
 	kvm_mmu_audit(vcpu, AUDIT_PRE_PAGE_FAULT);
 	kvm_mmu_free_some_pages(vcpu);

@@ -640,15 +646,13 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 
 	++vcpu->stat.pf_fixed;
 	kvm_mmu_audit(vcpu, AUDIT_POST_PAGE_FAULT);
-	spin_unlock(&vcpu->kvm->mmu_lock);
-
-	return emulate;
+	r = emulate;
 
 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);
 	if (!is_error_pfn(pfn))
 		kvm_release_pfn_clean(pfn);
-	return 0;
+	return r;
 }
 
 static gpa_t FNAME(get_level1_sp_gpa)(struct kvm_mmu_page *sp)

Signature · 1 line

1.7.7.6

Xiao Guangrong

author

2012-09-14 09:58 UTC #5 lore raw

PATCH v2 3/5 KVM: MMU: cleanup FNAME(page_fault)

In reply to Xiao Guangrong

Let it return emulate state instead of spte like __direct_map

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/paging_tmpl.h |   28 ++++++++++------------------
 1 files changed, 10 insertions(+), 18 deletions(-)

arch/x86/kvm/paging_tmpl.h +10 ~~-18~~

index 92f466c..0adf376 100644
@@ -463,20 +463,18 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
 /*
  * Fetch a shadow pte for a specific level in the paging hierarchy.
  */
-static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
+static int FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 			 struct guest_walker *gw,
 			 int user_fault, int write_fault, int hlevel,
-			 int *emulate, pfn_t pfn, bool map_writable,
-			 bool prefault)
+			 pfn_t pfn, bool map_writable, bool prefault)
 {
-	unsigned access = gw->pt_access;
 	struct kvm_mmu_page *sp = NULL;
-	int top_level;
-	unsigned direct_access;
 	struct kvm_shadow_walk_iterator it;
+	unsigned direct_access, access = gw->pt_access;
+	int top_level, emulate = 0;
 
 	if (!is_present_gpte(gw->ptes[gw->level - 1]))
-		return NULL;
+		return 0;
 
 	direct_access = gw->pte_access;

@@ -540,17 +538,17 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 
 	clear_sp_write_flooding_count(it.sptep);
 	mmu_set_spte(vcpu, it.sptep, access, gw->pte_access,
-		     user_fault, write_fault, emulate, it.level,
+		     user_fault, write_fault, &emulate, it.level,
 		     gw->gfn, pfn, prefault, map_writable);
 	FNAME(pte_prefetch)(vcpu, gw, it.sptep);
 
-	return it.sptep;
+	return emulate;
 
 out_gpte_changed:
 	if (sp)
 		kvm_mmu_put_page(sp, it.sptep);
 
-	return NULL;
+	return 0;
 }
 
 /*

@@ -573,8 +571,6 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 	int write_fault = error_code & PFERR_WRITE_MASK;
 	int user_fault = error_code & PFERR_USER_MASK;
 	struct guest_walker walker;
-	u64 *sptep;
-	int emulate = 0;
 	int r;
 	pfn_t pfn;
 	int level = PT_PAGE_TABLE_LEVEL;

@@ -638,15 +634,11 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 	kvm_mmu_free_some_pages(vcpu);
 	if (!force_pt_level)
 		transparent_hugepage_adjust(vcpu, &walker.gfn, &pfn, &level);
-	sptep = FNAME(fetch)(vcpu, addr, &walker, user_fault, write_fault,
-			     level, &emulate, pfn, map_writable, prefault);
-	(void)sptep;
-	pgprintk("%s: shadow pte %p %llx emulate %d\n", __func__,
-		 sptep, *sptep, emulate);
+	r = FNAME(fetch)(vcpu, addr, &walker, user_fault, write_fault,
+			 level, pfn, map_writable, prefault);
 
 	++vcpu->stat.pf_fixed;
 	kvm_mmu_audit(vcpu, AUDIT_POST_PAGE_FAULT);
-	r = emulate;
 
 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);

Signature · 1 line

1.7.7.6

Avi Kivity

2012-09-20 10:54 UTC #6 lore raw

Re: [PATCH v2 3/5] KVM: MMU: cleanup FNAME(page_fault)

Quoted 13 lines · Xiao Guangrong

Let it return emulate state instead of spte like __direct_map
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/paging_tmpl.h |   28 ++++++++++------------------
 1 files changed, 10 insertions(+), 18 deletions(-)
arch/x86/kvm/paging_tmpl.h +0 -0
index 92f466c..0adf376 100644
@@ -463,20 +463,18 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
 /*

arch/x86/kvm/paging_tmpl.h static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw, · line 470

Quoted 10 lines

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

  * Fetch a shadow pte for a specific level in the paging hierarchy.
  */
-static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
+static int FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 			 struct guest_walker *gw,
 			 int user_fault, int write_fault, int hlevel,
-			 int *emulate, pfn_t pfn, bool map_writable,
-			 bool prefault)
+			 pfn_t pfn, bool map_writable, bool prefault)
 {

Please document the return value in the comment.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2012-09-14 09:59 UTC #7 lore raw

PATCH v2 4/5 KVM: MMU: introduce page_fault_start and page_fault_end

In reply to Xiao Guangrong

Wrap the common operations into these two functions

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c         |   53 +++++++++++++++++++++++++++----------------
 arch/x86/kvm/paging_tmpl.h |   16 +++++--------
 2 files changed, 39 insertions(+), 30 deletions(-)

arch/x86/kvm/mmu.c +33 ~~-20~~

index 29ce28b..7e7b8cd 100644
@@ -2825,6 +2825,29 @@ exit:
 static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 			 gva_t gva, pfn_t *pfn, bool write, bool *writable);
 
+static bool
+page_fault_start(struct kvm_vcpu *vcpu, gfn_t *gfnp, pfn_t *pfnp, int *levelp,
+		 bool force_pt_level, unsigned long mmu_seq)
+{
+	spin_lock(&vcpu->kvm->mmu_lock);
+	if (mmu_notifier_retry(vcpu, mmu_seq))
+		return false;
+
+	kvm_mmu_free_some_pages(vcpu);
+	if (likely(!force_pt_level))
+		transparent_hugepage_adjust(vcpu, gfnp, pfnp, levelp);
+
+	return true;
+}
+
+static void page_fault_end(struct kvm_vcpu *vcpu, pfn_t pfn)
+{
+	spin_unlock(&vcpu->kvm->mmu_lock);
+
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
+}
+
 static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,
 			 gfn_t gfn, bool prefault)
 {

@@ -2862,22 +2885,17 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,
 	if (handle_abnormal_pfn(vcpu, v, gfn, pfn, ACC_ALL, &r))
 		return r;
 
-	spin_lock(&vcpu->kvm->mmu_lock);
-	if (mmu_notifier_retry(vcpu, mmu_seq)) {
+	if (!page_fault_start(vcpu, &gfn, &pfn, &level, force_pt_level,
+	      mmu_seq)) {
 		r = 0;
-		goto out_unlock;
+		goto exit;
 	}
 
-	kvm_mmu_free_some_pages(vcpu);
-	if (likely(!force_pt_level))
-		transparent_hugepage_adjust(vcpu, &gfn, &pfn, &level);
 	r = __direct_map(vcpu, v, write, map_writable, level, gfn, pfn,
 			 prefault);
 
-out_unlock:
-	spin_unlock(&vcpu->kvm->mmu_lock);
-	if (!is_error_pfn(pfn))
-		kvm_release_pfn_clean(pfn);
+exit:
+	page_fault_end(vcpu, pfn);
 	return r;
 }

@@ -3331,22 +3349,17 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa, u32 error_code,
 	if (handle_abnormal_pfn(vcpu, 0, gfn, pfn, ACC_ALL, &r))
 		return r;
 
-	spin_lock(&vcpu->kvm->mmu_lock);
-	if (mmu_notifier_retry(vcpu, mmu_seq)) {
+	if (!page_fault_start(vcpu, &gfn, &pfn, &level, force_pt_level,
+	      mmu_seq)) {
 		r = 0;
-		goto out_unlock;
+		goto exit;
 	}
 
-	kvm_mmu_free_some_pages(vcpu);
-	if (likely(!force_pt_level))
-		transparent_hugepage_adjust(vcpu, &gfn, &pfn, &level);
 	r = __direct_map(vcpu, gpa, write, map_writable,
 			 level, gfn, pfn, prefault);
 
-out_unlock:
-	spin_unlock(&vcpu->kvm->mmu_lock);
-	if (!is_error_pfn(pfn))
-		kvm_release_pfn_clean(pfn);
+exit:
+	page_fault_end(vcpu, pfn);
 	return r;
 }

arch/x86/kvm/paging_tmpl.h +6 ~~-10~~

index 0adf376..1a738c5 100644
@@ -624,26 +624,22 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 				walker.gfn, pfn, walker.pte_access, &r))
 		return r;
 
-	spin_lock(&vcpu->kvm->mmu_lock);
-	if (mmu_notifier_retry(vcpu, mmu_seq)) {
+	if (!page_fault_start(vcpu, &walker.gfn, &pfn, &level,
+	      force_pt_level, mmu_seq)) {
 		r = 0;
-		goto out_unlock;
+		goto exit;
 	}
 
 	kvm_mmu_audit(vcpu, AUDIT_PRE_PAGE_FAULT);
-	kvm_mmu_free_some_pages(vcpu);
-	if (!force_pt_level)
-		transparent_hugepage_adjust(vcpu, &walker.gfn, &pfn, &level);
+
 	r = FNAME(fetch)(vcpu, addr, &walker, user_fault, write_fault,
 			 level, pfn, map_writable, prefault);
 
 	++vcpu->stat.pf_fixed;
 	kvm_mmu_audit(vcpu, AUDIT_POST_PAGE_FAULT);
 
-out_unlock:
-	spin_unlock(&vcpu->kvm->mmu_lock);
-	if (!is_error_pfn(pfn))
-		kvm_release_pfn_clean(pfn);
+exit:
+	page_fault_end(vcpu, pfn);
 	return r;
 }

Signature · 1 line

1.7.7.6

Marcelo Tosatti

2012-09-15 15:25 UTC #8 lore raw

Re: [PATCH v2 4/5] KVM: MMU: introduce page_fault_start and page_fault_end

Quoted 3 lines · Xiao Guangrong

Wrap the common operations into these two functions
Signed-off-by Xiao Guangrong <[email protected]>

Why? I think people are used to

spin_lock(lock)
sequence
spin_unlock(lock)

So its easy to verify whether access to data structures are protected.

Unrelated to this patch, one opportunity i see to simplify this code is:

- error pfn / mmio pfn / invalid pfn relation

Have the meaning of this bits unified in a single function/helper, see comment to patch 1 (perhaps you can further improve).

Xiao Guangrong

author

2012-09-18 08:15 UTC #9 lore raw

Re: [PATCH v2 4/5] KVM: MMU: introduce page_fault_start and page_fault_end

Quoted 10 lines · Marcelo Tosatti

Quoted 3 lines · Xiao Guangrong
Wrap the common operations into these two functions
Signed-off-by Xiao Guangrong <[email protected]>
Why? I think people are used to
spin_lock(lock)
sequence
spin_unlock(lock)

Marcelo,

There are many functions use this style that wrap the lock into the _start and _end functions in kernel (eg.: cgroup_pidlist_start and cgroup_pidlist_stop in kernel/cgroup.c).

Actually, i just wanted to remove below duplicate ugly code:

	if (!is_error_pfn(pfn))
		kvm_release_pfn_clean(pfn);

Quoted 10 lines

So its easy to verify whether access to data structures are protected.
Unrelated to this patch, one opportunity i see to simplify this code is:
- error pfn / mmio pfn / invalid pfn relation
Have the meaning of this bits unified in a single function/helper, see comment to patch 1 (perhaps you can further improve).

Sorry, more detail?

Marcelo Tosatti

2012-09-18 23:43 UTC #10 lore raw

Re: [PATCH v2 4/5] KVM: MMU: introduce page_fault_start and page_fault_end

Quoted 29 lines · Xiao Guangrong

Quoted 10 lines · Marcelo Tosatti
Quoted 3 lines · Xiao Guangrong
Wrap the common operations into these two functions
Signed-off-by Xiao Guangrong <[email protected]>
Why? I think people are used to
spin_lock(lock)
sequence
spin_unlock(lock)
Marcelo,
There are many functions use this style that wrap the lock into the _start and _end functions in kernel (eg.: cgroup_pidlist_start and cgroup_pidlist_stop in kernel/cgroup.c).
Actually, i just wanted to remove below duplicate ugly code:
	if (!is_error_pfn(pfn))
		kvm_release_pfn_clean(pfn);
Quoted 6 lines
So its easy to verify whether access to data structures are protected.
Unrelated to this patch, one opportunity i see to simplify this code is:

Quoted 6 lines

Quoted 4 lines
- error pfn / mmio pfn / invalid pfn relation
Have the meaning of this bits unified in a single function/helper, see comment to patch 1 (perhaps you can further improve).
Sorry, more detail?

Should force the reader of the code to understand error pfn / mmio pfn / invalid pfn in a single helper. That is, avoid using error pfn at all.

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Xiao Guangrong

author

2012-09-20 02:59 UTC #11 lore raw

Re: [PATCH v2 4/5] KVM: MMU: introduce page_fault_start and page_fault_end

Quoted 9 lines · Marcelo Tosatti

Quoted 6 lines
Quoted 4 lines
- error pfn / mmio pfn / invalid pfn relation
Have the meaning of this bits unified in a single function/helper, see comment to patch 1 (perhaps you can further improve).
Sorry, more detail?
Should force the reader of the code to understand error pfn / mmio pfn / invalid pfn in a single helper. That is, avoid using error pfn at all.

Reasonable suggestion, i will think about it more. Thank you, Marcelo!

Avi Kivity

2012-09-20 10:57 UTC #12 lore raw

Re: [PATCH v2 4/5] KVM: MMU: introduce page_fault_start and page_fault_end

In reply to Xiao Guangrong

Quoted 28 lines · Xiao Guangrong

Wrap the common operations into these two functions

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c         |   53 +++++++++++++++++++++++++++----------------
 arch/x86/kvm/paging_tmpl.h |   16 +++++--------
 2 files changed, 39 insertions(+), 30 deletions(-)

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

index 29ce28b..7e7b8cd 100644
@@ -2825,6 +2825,29 @@ exit:
 static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 			 gva_t gva, pfn_t *pfn, bool write, bool *writable);
 
+static bool
+page_fault_start(struct kvm_vcpu *vcpu, gfn_t *gfnp, pfn_t *pfnp, int *levelp,
+		 bool force_pt_level, unsigned long mmu_seq)
+{
+	spin_lock(&vcpu->kvm->mmu_lock);
+	if (mmu_notifier_retry(vcpu, mmu_seq))
+		return false;
+
+	kvm_mmu_free_some_pages(vcpu);
+	if (likely(!force_pt_level))
+		transparent_hugepage_adjust(vcpu, gfnp, pfnp, levelp);
+

arch/x86/kvm/mmu.c exit: · line 2849

Quoted 10 lines

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

+	return true;
+}
+
+static void page_fault_end(struct kvm_vcpu *vcpu, pfn_t pfn)
+{
+	spin_unlock(&vcpu->kvm->mmu_lock);
+
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
+}

Needs sparse annotations (__acquires, __releases).

These code blocks have nothing in common except for being shared. Often that's not good for maintainability because it means that further changes can affect one path but not the other. But we can try it out and see.

Signature · 1 line

error compiling committee.c: too many arguments to function

Xiao Guangrong

author

2012-09-14 09:59 UTC #13 lore raw

PATCH v2 5/5 KVM: MMU: introduce FNAME(prefetch_gpte)

In reply to Xiao Guangrong

The only different thing between FNAME(update_pte) and FNAME(pte_prefetch) is that the former is allowed to prefetch gfn from dirty logged slot, so introduce a common function to prefetch spte

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/paging_tmpl.h |   50 ++++++++++++++++---------------------------
 1 files changed, 19 insertions(+), 31 deletions(-)

arch/x86/kvm/paging_tmpl.h +19 ~~-31~~

index 1a738c5..9742eab 100644
@@ -356,35 +356,45 @@ no_present:
 	return true;
 }
 
-static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
-			      u64 *spte, const void *pte)
+static void
+FNAME(prefetch_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+		     u64 *spte, pt_element_t gpte, bool no_dirty_log)
 {
-	pt_element_t gpte;
 	unsigned pte_access;
+	gfn_t gfn;
 	pfn_t pfn;
 
-	gpte = *(const pt_element_t *)pte;
 	if (FNAME(prefetch_invalid_gpte)(vcpu, sp, spte, gpte))
 		return;
 
 	pgprintk("%s: gpte %llx spte %p\n", __func__, (u64)gpte, spte);
+
+	gfn = gpte_to_gfn(gpte);
 	pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte, true);
-	pfn = gfn_to_pfn_atomic(vcpu->kvm, gpte_to_gfn(gpte));
+	pfn = pte_prefetch_gfn_to_pfn(vcpu, gfn,
+			no_dirty_log && (pte_access & ACC_WRITE_MASK));
 	if (mmu_invalid_pfn(pfn))
 		return;
 
 	/*
 	 * we call mmu_set_spte() with host_writable = true because that
-	 * vcpu->arch.update_pte.pfn was fetched from get_user_pages(write = 1).
+	 * pte_prefetch_gfn_to_pfn always gets a writable pfn.
 	 */
 	mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
-		     NULL, PT_PAGE_TABLE_LEVEL,
-		     gpte_to_gfn(gpte), pfn, true, true);
+		     NULL, PT_PAGE_TABLE_LEVEL, gfn, pfn, true, true);
 
 	if (!is_error_pfn(pfn))
 		kvm_release_pfn_clean(pfn);
 }
 
+static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+			      u64 *spte, const void *pte)
+{
+	pt_element_t gpte = *(const pt_element_t *)pte;
+
+	return FNAME(prefetch_gpte)(vcpu, sp, spte, gpte, false);
+}
+
 static bool FNAME(gpte_changed)(struct kvm_vcpu *vcpu,
 				struct guest_walker *gw, int level)
 {

@@ -428,35 +438,13 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
 	spte = sp->spt + i;
 
 	for (i = 0; i < PTE_PREFETCH_NUM; i++, spte++) {
-		pt_element_t gpte;
-		unsigned pte_access;
-		gfn_t gfn;
-		pfn_t pfn;
-
 		if (spte == sptep)
 			continue;
 
 		if (is_shadow_present_pte(*spte))
 			continue;
 
-		gpte = gptep[i];
-
-		if (FNAME(prefetch_invalid_gpte)(vcpu, sp, spte, gpte))
-			continue;
-
-		pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte,
-								  true);
-		gfn = gpte_to_gfn(gpte);
-		pfn = pte_prefetch_gfn_to_pfn(vcpu, gfn,
-				      pte_access & ACC_WRITE_MASK);
-		if (mmu_invalid_pfn(pfn))
-			break;
-
-		mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
-			     NULL, PT_PAGE_TABLE_LEVEL, gfn,
-			     pfn, true, true);
-		if (!is_error_pfn(pfn))
-			kvm_release_pfn_clean(pfn);
+		return FNAME(prefetch_gpte)(vcpu, sp, spte, gptep[i], true);
 	}
 }

Signature · 1 line

1.7.7.6

Xiao Guangrong

author

2012-09-14 10:13 UTC #14 lore raw

Re: [PATCH v2 5/5] KVM: MMU: introduce FNAME(prefetch_gpte)

Review

Quoted 1 line · Xiao Guangrong

(quoted diff) +1 -0

+		return FNAME(prefetch_gpte)(vcpu, sp, spte, gptep[i], true);

Sorry, this was wrong. Update this patch.

[PATCH v2 5/5] KVM: MMU: introduce FNAME(prefetch_gpte)

The only different thing between FNAME(update_pte) and FNAME(pte_prefetch) is that the former is allowed to prefetch gfn from dirty logged slot, so introduce a common function to prefetch spte

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/paging_tmpl.h |   50 ++++++++++++++++---------------------------
 1 files changed, 19 insertions(+), 31 deletions(-)

arch/x86/kvm/paging_tmpl.h +19 ~~-31~~

index 1a738c5..32facf7 100644
@@ -356,35 +356,45 @@ no_present:
 	return true;
 }
 
-static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
-			      u64 *spte, const void *pte)
+static void
+FNAME(prefetch_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+		     u64 *spte, pt_element_t gpte, bool no_dirty_log)
 {
-	pt_element_t gpte;
 	unsigned pte_access;
+	gfn_t gfn;
 	pfn_t pfn;
 
-	gpte = *(const pt_element_t *)pte;
 	if (FNAME(prefetch_invalid_gpte)(vcpu, sp, spte, gpte))
 		return;
 
 	pgprintk("%s: gpte %llx spte %p\n", __func__, (u64)gpte, spte);
+
+	gfn = gpte_to_gfn(gpte);
 	pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte, true);
-	pfn = gfn_to_pfn_atomic(vcpu->kvm, gpte_to_gfn(gpte));
+	pfn = pte_prefetch_gfn_to_pfn(vcpu, gfn,
+			no_dirty_log && (pte_access & ACC_WRITE_MASK));
 	if (mmu_invalid_pfn(pfn))
 		return;
 
 	/*
 	 * we call mmu_set_spte() with host_writable = true because that
-	 * vcpu->arch.update_pte.pfn was fetched from get_user_pages(write = 1).
+	 * pte_prefetch_gfn_to_pfn always gets a writable pfn.
 	 */
 	mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
-		     NULL, PT_PAGE_TABLE_LEVEL,
-		     gpte_to_gfn(gpte), pfn, true, true);
+		     NULL, PT_PAGE_TABLE_LEVEL, gfn, pfn, true, true);
 
 	if (!is_error_pfn(pfn))
 		kvm_release_pfn_clean(pfn);
 }
 
+static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+			      u64 *spte, const void *pte)
+{
+	pt_element_t gpte = *(const pt_element_t *)pte;
+
+	return FNAME(prefetch_gpte)(vcpu, sp, spte, gpte, false);
+}
+
 static bool FNAME(gpte_changed)(struct kvm_vcpu *vcpu,
 				struct guest_walker *gw, int level)
 {

@@ -428,35 +438,13 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
 	spte = sp->spt + i;
 
 	for (i = 0; i < PTE_PREFETCH_NUM; i++, spte++) {
-		pt_element_t gpte;
-		unsigned pte_access;
-		gfn_t gfn;
-		pfn_t pfn;
-
 		if (spte == sptep)
 			continue;
 
 		if (is_shadow_present_pte(*spte))
 			continue;
 
-		gpte = gptep[i];
-
-		if (FNAME(prefetch_invalid_gpte)(vcpu, sp, spte, gpte))
-			continue;
-
-		pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte,
-								  true);
-		gfn = gpte_to_gfn(gpte);
-		pfn = pte_prefetch_gfn_to_pfn(vcpu, gfn,
-				      pte_access & ACC_WRITE_MASK);
-		if (mmu_invalid_pfn(pfn))
-			break;
-
-		mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
-			     NULL, PT_PAGE_TABLE_LEVEL, gfn,
-			     pfn, true, true);
-		if (!is_error_pfn(pfn))
-			kvm_release_pfn_clean(pfn);
+		FNAME(prefetch_gpte)(vcpu, sp, spte, gptep[i], true);
 	}
 }

Signature · 1 line

1.7.7.6

Marcelo Tosatti

2012-09-15 15:31 UTC #15 lore raw

Re: [PATCH v2 5/5] KVM: MMU: introduce FNAME(prefetch_gpte)

Quoted 6 lines · Xiao Guangrong

Review
Quoted 1 line · Xiao Guangrong
(quoted diff) +1 -0
+		return FNAME(prefetch_gpte)(vcpu, sp, spte, gptep[i], true);
Sorry, this was wrong. Update this patch.

Quoted 7 lines

[PATCH v2 5/5] KVM: MMU: introduce FNAME(prefetch_gpte)
The only different thing between FNAME(update_pte) and FNAME(pte_prefetch) is that the former is allowed to prefetch gfn from dirty logged slot, so introduce a common function to prefetch spte
Signed-off-by Xiao Guangrong <[email protected]>

IMHO, for the human reader, the meaning of the two functions is different and therefore separation is justified. Moreover they already share common code via FNAME(prefetch_invalid_gpte) (which BTW, is a confusing name because the function does not prefetch gpte, it validates gpte).

Xiao Guangrong

author

2012-09-18 08:26 UTC #16 lore raw

Re: [PATCH v2 5/5] KVM: MMU: introduce FNAME(prefetch_gpte)

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong
Review
Quoted 1 line · Xiao Guangrong
(quoted diff) +1 -0
+		return FNAME(prefetch_gpte)(vcpu, sp, spte, gptep[i], true);
Sorry, this was wrong. Update this patch.
[PATCH v2 5/5] KVM: MMU: introduce FNAME(prefetch_gpte)

Quoted 8 lines

Quoted 5 lines
The only different thing between FNAME(update_pte) and FNAME(pte_prefetch) is that the former is allowed to prefetch gfn from dirty logged slot, so introduce a common function to prefetch spte
Signed-off-by Xiao Guangrong <[email protected]>
IMHO, for the human reader, the meaning of the two functions is different and therefore separation is justified. Moreover they already

Actually, these two functions do the same things, both of them prefetch spte based on gpte. The only different is, in the case of update_pte, we have high opportunity that the spte can be accessed soon (that why it is allowed to prefetch dirty logged gfn).

Quoted 3 lines

share common code via FNAME(prefetch_invalid_gpte) (which BTW, is a confusing name because the function does not prefetch gpte, it validates gpte).

I think it is not too bad for it not just validates gpte, it also can drop spte if the gpte is invalid, it is also the behaviour that modify spte based on gpte. ;)