KVM: Introduce a helper to check if gfn is in memslot

From: Takuya Yoshikawa<[email protected]>

This will be optimized later.

Signed-off-by Takuya Yoshikawa<[email protected]>

---
  arch/x86/kvm/paging_tmpl.h |   12 +++++++++---
  1 files changed, 9 insertions(+), 3 deletions(-)

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

index 74f8567..109939a 100644
@@ -109,6 +109,14 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte)
  	return access;
  }
 
+static int FNAME(read_guest_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
+				 gfn_t table_gfn, int offset, pt_element_t *ptep)
+{
+	return kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, ptep,
+				       offset, sizeof(*ptep),
+				       PFERR_USER_MASK | PFERR_WRITE_MASK);
+}
+
  /*
   * Fetch a guest pte for a guest virtual address
   */

Quoted 29 lines · Takuya Yoshikawa

From: Takuya Yoshikawa<[email protected]>

This will be optimized later.

Signed-off-by Takuya Yoshikawa<[email protected]>

---
  arch/x86/kvm/paging_tmpl.h |   12 +++++++++---
  1 files changed, 9 insertions(+), 3 deletions(-)

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

index 74f8567..109939a 100644
@@ -109,6 +109,14 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte)
  	return access;
  }
 
+static int FNAME(read_guest_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
+				 gfn_t table_gfn, int offset, pt_element_t *ptep)
+{
+	return kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, ptep,
+				       offset, sizeof(*ptep),
+				       PFERR_USER_MASK | PFERR_WRITE_MASK);
+}
+
  /*
   * Fetch a guest pte for a guest virtual address
   */

arch/x86/kvm/paging_tmpl.h walk: · line 174

Quoted 10 lines

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

@@ -160,9 +168,7 @@ walk:
  		walker->table_gfn[walker->level - 1] = table_gfn;
  		walker->pte_gpa[walker->level - 1] = pte_gpa;
 
-		if (kvm_read_guest_page_mmu(vcpu, mmu, table_gfn,&pte,
-					    offset, sizeof(pte),
-					    PFERR_USER_MASK|PFERR_WRITE_MASK)) {
+		if (FNAME(read_guest_pte)(vcpu, mmu, table_gfn, offset,&pte)) {
  			present = false;
  			break;
  		}

I think it's better to avoid a separate function for this. The reason is I'd like to use ptep_user for cmpxchg_gpte() later on in walk_addr_generic(), so we use the same calculation for both read and write. So please just inline the new code in walk_addr_generic().

In fact there's probably a bug there for nested npt - we use gfn_to_page(table_gfn), but table_gfn is actually an ngfn, not a gfn. Joerg, am I right here?

Quoted 47 lines · Avi Kivity

Quoted 32 lines · Takuya Yoshikawa

From: Takuya Yoshikawa<[email protected]>

This will be optimized later.

Signed-off-by Takuya Yoshikawa<[email protected]>

---

   arch/x86/kvm/paging_tmpl.h |   12 +++++++++---
   1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 74f8567..109939a 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -109,6 +109,14 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte)

   	return access;
   }

+static int FNAME(read_guest_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
+ gfn_t table_gfn, int offset, pt_element_t *ptep)
+{
+ return kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, ptep,
+ offset, sizeof(*ptep),
+ PFERR_USER_MASK | PFERR_WRITE_MASK);
+}
+
/*
* Fetch a guest pte for a guest virtual address
*/
@@ -160,9 +168,7 @@ walk:

   		walker->table_gfn[walker->level - 1] = table_gfn;
   		walker->pte_gpa[walker->level - 1] = pte_gpa;

Quoted 7 lines

- if (kvm_read_guest_page_mmu(vcpu, mmu, table_gfn,&pte,
- offset, sizeof(pte),
- PFERR_USER_MASK|PFERR_WRITE_MASK)) {

 +		if (FNAME(read_guest_pte)(vcpu, mmu, table_gfn, offset,&pte)) {
   			present = false;
   			break;
   		}

I think it's better to avoid a separate function for this. The reason is I'd like to use ptep_user for cmpxchg_gpte() later on in walk_addr_generic(), so we use the same calculation for both read and write. So please just inline the new code in walk_addr_generic().

Quoted 3 lines

In fact there's probably a bug there for nested npt - we use gfn_to_page(table_gfn), but table_gfn is actually an ngfn, not a gfn. Joerg, am I right here?

This patch seems only to introduce another wrapper around kvm_read_guest_page_mmu(), so I don't see a problem in this patch.

The kvm_read_guest_page_mmu takes care whether it gets a l1-gfn or l2-gfn (by calling mmu->translate_gpa).

Quoted 2 lines · Joerg

This patch seems only to introduce another wrapper around kvm_read_guest_page_mmu(), so I don't see a problem in this patch.

By patch 3, ptep_user will be computed in this function and no longer available for setting the accessed bit later on.

Quoted 2 lines

The kvm_read_guest_page_mmu takes care whether it gets a l1-gfn or l2-gfn (by calling mmu->translate_gpa).

But cmpxchg_gpte() does not.

Quoted 8 lines · Avi Kivity

Quoted 2 lines · Joerg

This patch seems only to introduce another wrapper around kvm_read_guest_page_mmu(), so I don't see a problem in this patch.

By patch 3, ptep_user will be computed in this function and no longer available for setting the accessed bit later on.

Quoted 4 lines

Quoted 2 lines

The kvm_read_guest_page_mmu takes care whether it gets a l1-gfn or l2-gfn (by calling mmu->translate_gpa).

But cmpxchg_gpte() does not.

You are right, cmpxchg_gpte needs to handle this too. But the bug is not introduced with this patch-set it was there before.

The cmpxchg_gpte function treats all table_gfns as l1-gfns. I'll send a fix soon.

Quoted 2 lines · Joerg

The cmpxchg_gpte function treats all table_gfns as l1-gfns. I'll send a fix soon.

Thanks.

From 6b1dcd9f17bbd482061180001d1f45c3adcef430 Mon Sep 17 00:00:00 2001

arch/x86/kvm/paging_tmpl.h +1 -1

@@ -245,13 +257,17 @@ walk:
 		goto error;
 
 	if (write_fault && !is_dirty_gpte(pte)) {
-		bool ret;
+		int ret;
 

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

+
 		mark_page_dirty(vcpu->kvm, table_gfn);
 		pte |= PT_DIRTY_MASK;
 		walker->ptes[walker->level - 1] = pte;

Signature · 1 line

1.7.1

Quoted 7 lines · Joerg

@@ -245,13 +257,17 @@ walk:
goto error;

if (write_fault&& !is_dirty_gpte(pte)) {
- bool ret;
+ int ret;

Quoted 10 lines

trace_kvm_mmu_set_dirty_bit(table_gfn, index, sizeof(pte));
- ret = FNAME(cmpxchg_gpte)(vcpu->kvm, table_gfn, index, pte,
+ ret = FNAME(cmpxchg_gpte)(vcpu, mmu, table_gfn, index, pte,
pte|PT_DIRTY_MASK);
- if (ret)
+ if (ret< 0) {
+ present = false;
+ goto error;
+ } if (ret)
goto walk;

Preferably else if or another line ? :)

Correct. The reason I don't want the helper, is so we can use ptep_user in both places (not for efficiency, just to make sure it's exactly the same value).

Quoted 2 lines

The cmpxchg_gpte function treats all table_gfns as l1-gfns. I'll send a fix soon.

From: Takuya Yoshikawa <[email protected]>

We optimize multi level guest page table walk as follows:

1. We cache the memslot which, probably, includes the next guest page tables to avoid searching for it many times.
2. We use get_user() instead of copy_from_user().

Note that this is kind of a restricted way of Xiao's more generic
work: "KVM: optimize memslots searching and cache GPN to GFN."

With this patch applied, paging64_walk_addr_generic() has improved as the following tracing results show.

Before:

  3.169 us   |  paging64_walk_addr_generic();
  1.880 us   |  paging64_walk_addr_generic();
  1.243 us   |  paging64_walk_addr_generic();
  1.517 us   |  paging64_walk_addr_generic();
  3.009 us   |  paging64_walk_addr_generic();
  1.814 us   |  paging64_walk_addr_generic();
  1.340 us   |  paging64_walk_addr_generic();
  1.659 us   |  paging64_walk_addr_generic();
  1.748 us   |  paging64_walk_addr_generic();
  1.488 us   |  paging64_walk_addr_generic();

After:

  1.714 us   |  paging64_walk_addr_generic();
  0.806 us   |  paging64_walk_addr_generic();
  0.664 us   |  paging64_walk_addr_generic();
  0.619 us   |  paging64_walk_addr_generic();
  0.645 us   |  paging64_walk_addr_generic();
  0.605 us   |  paging64_walk_addr_generic();
  1.388 us   |  paging64_walk_addr_generic();
  0.753 us   |  paging64_walk_addr_generic();
  0.594 us   |  paging64_walk_addr_generic();
  0.833 us   |  paging64_walk_addr_generic();

Nice optimization! What scenarios have you used to test it?

Can I test nested SVM easily, e.g.?

From: Takuya Yoshikawa <[email protected]>

We optimize multi level guest page table walk as follows:

1. We cache the memslot which, probably, includes the next guest page tables to avoid searching for it many times.

2. We use get_user() instead of copy_from_user().

Note that this is kind of a restricted way of Xiao's more generic
work: "KVM: optimize memslots searching and cache GPN to GFN."

With this patch applied, paging64_walk_addr_generic() has improved as the following tracing results show.

Before:

  3.169 us   |  paging64_walk_addr_generic();
  1.880 us   |  paging64_walk_addr_generic();
  1.243 us   |  paging64_walk_addr_generic();
  1.517 us   |  paging64_walk_addr_generic();
  3.009 us   |  paging64_walk_addr_generic();
  1.814 us   |  paging64_walk_addr_generic();
  1.340 us   |  paging64_walk_addr_generic();
  1.659 us   |  paging64_walk_addr_generic();
  1.748 us   |  paging64_walk_addr_generic();
  1.488 us   |  paging64_walk_addr_generic();

After:

  1.714 us   |  paging64_walk_addr_generic();
  0.806 us   |  paging64_walk_addr_generic();
  0.664 us   |  paging64_walk_addr_generic();
  0.619 us   |  paging64_walk_addr_generic();
  0.645 us   |  paging64_walk_addr_generic();
  0.605 us   |  paging64_walk_addr_generic();
  1.388 us   |  paging64_walk_addr_generic();
  0.753 us   |  paging64_walk_addr_generic();
  0.594 us   |  paging64_walk_addr_generic();
  0.833 us   |  paging64_walk_addr_generic();

Signed-off-by Takuya Yoshikawa <[email protected]>

---
 arch/x86/kvm/paging_tmpl.h |   37 ++++++++++++++++++++++++++++++++-----
 1 files changed, 32 insertions(+), 5 deletions(-)

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

index 109939a..614aa3f 100644
@@ -109,12 +109,37 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte)
 	return access;
 }
 
+/*
+ * Read the guest PTE refered to by table_gfn and offset and put it into ptep.
+ *
+ * *slot_hint, if not NULL, should point to a memslot which probably includes
+ * the guest PTE.  The actual memslot will be put back into this so that
+ * callers can cache it.
+ */
 static int FNAME(read_guest_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
-				 gfn_t table_gfn, int offset, pt_element_t *ptep)
+				 gfn_t table_gfn, int offset, pt_element_t *ptep,
+				 struct kvm_memory_slot **slot_hint)
 {
-	return kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, ptep,
-				       offset, sizeof(*ptep),
-				       PFERR_USER_MASK | PFERR_WRITE_MASK);
+	unsigned long addr;
+	pt_element_t __user *ptep_user;
+	gfn_t real_gfn;
+

Quoted 4 lines

We optimize multi level guest page table walk as follows:

1. We cache the memslot which, probably, includes the next guest page tables to avoid searching for it many times.

Yeah, the hit is very high, after optimizing the algorithm of memslots
(http://lwn.net/Articles/429308/), maybe the advantage is not so significant, could you apply this patchset and test again please?

Quoted 9 lines

2. We use get_user() instead of copy_from_user().

Note that this is kind of a restricted way of Xiao's more generic
work: "KVM: optimize memslots searching and cache GPN to GFN."

With this patch applied, paging64_walk_addr_generic() has improved as the following tracing results show.

arch/x86/kvm/paging_tmpl.h static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte) · line 136

Quoted 4 lines

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

+
+	if (!(*slot_hint) || !gfn_in_memslot(*slot_hint, real_gfn))
+		*slot_hint = gfn_to_memslot(vcpu->kvm, real_gfn);
+

You forgot to check the result. (if *slot_hint == NULL)? ...　;-)

So if certain algorithm seems to be addapted, yes, I will test based on that. IIRC, any practically good algorithm has not been found yet, right?

From: Takuya Yoshikawa<[email protected]>

We optimize multi level guest page table walk as follows:

1. We cache the memslot which, probably, includes the next guest page tables to avoid searching for it many times.
2. We use get_user() instead of copy_from_user().

Note that this is kind of a restricted way of Xiao's more generic
work: "KVM: optimize memslots searching and cache GPN to GFN."

+/*
+ * Read the guest PTE refered to by table_gfn and offset and put it into ptep.
+ *
+ * *slot_hint, if not NULL, should point to a memslot which probably includes
+ * the guest PTE. The actual memslot will be put back into this so that
+ * callers can cache it.
+ */

static int FNAME(read_guest_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,

arch/x86/kvm/paging_tmpl.h +25 -5

-				 gfn_t table_gfn, int offset, pt_element_t *ptep)
+				 gfn_t table_gfn, int offset, pt_element_t *ptep,
+				 struct kvm_memory_slot **slot_hint)
  {
-	return kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, ptep,
-				       offset, sizeof(*ptep),
-				       PFERR_USER_MASK | PFERR_WRITE_MASK);
+	unsigned long addr;
+	pt_element_t __user *ptep_user;
+	gfn_t real_gfn;
+
+	real_gfn = mmu->translate_gpa(vcpu, gfn_to_gpa(table_gfn),
+				      PFERR_USER_MASK | PFERR_WRITE_MASK);
+	if (real_gfn == UNMAPPED_GVA)
+		return -EFAULT;
+
+	real_gfn = gpa_to_gfn(real_gfn);
+
+	if (!(*slot_hint) || !gfn_in_memslot(*slot_hint, real_gfn))
+		*slot_hint = gfn_to_memslot(vcpu->kvm, real_gfn);
+
+	addr = gfn_to_hva_memslot(*slot_hint, real_gfn);
+	if (kvm_is_error_hva(addr))
+		return -EFAULT;
+
+	ptep_user = (pt_element_t __user *)((void *)addr + offset);
+	return get_user(*ptep, ptep_user);
  }
 
  /*

@@ -130,6 +155,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
  	gpa_t pte_gpa;
  	bool eperm, present, rsvd_fault;
  	int offset, write_fault, user_fault, fetch_fault;
+	struct kvm_memory_slot *slot_cache = NULL;
 
  	write_fault = access&  PFERR_WRITE_MASK;
  	user_fault = access&  PFERR_USER_MASK;

@@ -168,7 +194,8 @@ walk:
  		walker->table_gfn[walker->level - 1] = table_gfn;
  		walker->pte_gpa[walker->level - 1] = pte_gpa;
 
-		if (FNAME(read_guest_pte)(vcpu, mmu, table_gfn, offset,&pte)) {
+		if (FNAME(read_guest_pte)(vcpu, mmu, table_gfn,
+					  offset,&pte,&slot_cache)) {
  			present = false;
  			break;
  		}

Good optimization. copy_from_user() really isn't optimized for short buffers, I expect much of the improvement comes from that.

Quoted 3 lines · Avi Kivity

Good optimization. copy_from_user() really isn't optimized for short buffers, I expect much of the improvement comes from that.

Actually it is equivalent to get_user for the lenghts supported by get_user, assuming you pass in a constant length. You probably do not.

-Andi

Quoted 8 lines · On Thu, 28 Apr 2011 19:46:00 -0700 Andi Kleen <[email protected]> wrote:

Quoted 3 lines · Avi Kivity

Good optimization. copy_from_user() really isn't optimized for short buffers, I expect much of the improvement comes from that.

Actually it is equivalent to get_user for the lenghts supported by get_user, assuming you pass in a constant length. You probably do not.

Quoted 1 line

-Andi

Weird, I actually measured some changes even after dropping other parts than get_user() usage.

Only I can guess for that reason is the reduction of some function calls by inlining some functions.

Only I can guess for that reason is the reduction of some function calls by inlining some functions.

Quoted 2 lines

Only I can guess for that reason is the reduction of some function calls by inlining some functions.

Yes once at a time cfu was inline too and just checked for the right
sizes and the used g*u, but it got lost in the "icache over everything else" mania which is unfortunately en vogue for quite some time in kernel land (aka measuring patches only based on their impact on the .text
size, not the actual performance)

But you might getter better gains by fixing this general c*u() regression.

Do you think the following case would not differ so much from (1' 2') ?

walk_addr_generic() ---1''
copy_from_user() ---2''

Quoted 5 lines

Do you think the following case would not differ so much from (1' 2') ?

walk_addr_generic() ---1''
copy_from_user() ---2''

Yes it should be the same and is cleaner.

If you do a make .../foo.i and look at the code coming out of the preprocessor you'll see it expands to a

	if (!__builtin_constant_p(size))
                 return copy_user_generic(dst, (__force void *)src, size);
         switch (size) {
         case 1:__get_user_asm(*(u8 *)dst, (u8 __user *)src,
                               ret, "b", "b", "=q", 1);
                 return ret;
	case 2: ..
	case 4: ..
	case 8: ..
	case 10: ..
	case 16: ..
	}

Ok it looks like the 32bit kernel only handles 1/2/4. Maybe that was the problem if you ran on 32bit.

Quoted 3 lines

Ok it looks like the 32bit kernel only handles 1/2/4. Maybe that was the problem if you ran on 32bit.

I'm happy with a slower copy_from_user() for that particular case.