[PATCH 1/2 rebased] KVM: MMU: Avoid dropping accessed bit while removing write access

[PATCH 1/2 rebased] KVM: MMU: Avoid dropping accessed bit while removing write access

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

One more "KVM: MMU: Don't drop accessed bit while updating an spte."

Sptes are accessed by both kvm and hardware.
This patch uses update_spte() to fix the way of removing write access.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/mmu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 04c49b9..d75ba1e 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3446,7 +3446,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
 		for (i = 0; i < PT64_ENT_PER_PAGE; ++i)
 			/* avoid RMW */
 			if (is_writable_pte(pt[i]))
-				pt[i] &= ~PT_WRITABLE_MASK;
+				update_spte(&pt[i], pt[i] & ~PT_WRITABLE_MASK);
 	}
 	kvm_flush_remote_tlbs(kvm);
 }
-- 
1.7.1

[PATCH 2/2 rebased] KVM: MMU: Introduce a helper to access lpage_info

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Index calculation to access lpage_info appears three times.
A helper is worthwhile.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/mmu.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index d75ba1e..e434503 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -476,6 +476,12 @@ static void kvm_mmu_page_set_gfn(struct kvm_mmu_page *sp, int index, gfn_t gfn)
 		sp->gfns[index] = gfn;
 }
 
+static unsigned long lpage_idx(gfn_t gfn, gfn_t base_gfn, int level)
+{
+	return (gfn >> KVM_HPAGE_GFN_SHIFT(level)) -
+	       (base_gfn >> KVM_HPAGE_GFN_SHIFT(level));
+}
+
 /*
  * Return the pointer to the largepage write count for a given
  * gfn, handling slots that are not large page aligned.
@@ -484,10 +490,8 @@ static int *slot_largepage_idx(gfn_t gfn,
 			       struct kvm_memory_slot *slot,
 			       int level)
 {
-	unsigned long idx;
+	unsigned long idx = lpage_idx(gfn, slot->base_gfn, level);
 
-	idx = (gfn >> KVM_HPAGE_GFN_SHIFT(level)) -
-	      (slot->base_gfn >> KVM_HPAGE_GFN_SHIFT(level));
 	return &slot->lpage_info[level - 2][idx].write_count;
 }
 
@@ -591,8 +595,7 @@ static unsigned long *gfn_to_rmap(struct kvm *kvm, gfn_t gfn, int level)
 	if (likely(level == PT_PAGE_TABLE_LEVEL))
 		return &slot->rmap[gfn - slot->base_gfn];
 
-	idx = (gfn >> KVM_HPAGE_GFN_SHIFT(level)) -
-		(slot->base_gfn >> KVM_HPAGE_GFN_SHIFT(level));
+	idx = lpage_idx(gfn, slot->base_gfn, level);
 
 	return &slot->lpage_info[level - 2][idx].rmap_pde;
 }
@@ -887,11 +890,10 @@ static int kvm_handle_hva(struct kvm *kvm, unsigned long hva,
 
 			for (j = 0; j < KVM_NR_PAGE_SIZES - 1; ++j) {
 				unsigned long idx;
-				int sh;
 
-				sh = KVM_HPAGE_GFN_SHIFT(PT_DIRECTORY_LEVEL+j);
-				idx = ((memslot->base_gfn+gfn_offset) >> sh) -
-					(memslot->base_gfn >> sh);
+				idx = lpage_idx(memslot->base_gfn + gfn_offset,
+						memslot->base_gfn,
+						PT_DIRECTORY_LEVEL + j);
 				ret |= handler(kvm,
 					&memslot->lpage_info[j][idx].rmap_pde,
 					data);
-- 
1.7.1

Re: [PATCH 2/2 rebased] KVM: MMU: Introduce a helper to access lpage_info

[Xiao Guangrong]

On 12/06/2010 12:13 AM, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> 
> Index calculation to access lpage_info appears three times.
> A helper is worthwhile.
> 

Why not get lpage_info instead of index in the helper?

Re: [PATCH 2/2 rebased] KVM: MMU: Introduce a helper to access lpage_info

[Takuya Yoshikawa]

(2010/12/06 15:37), Xiao Guangrong wrote:
> On 12/06/2010 12:13 AM, Takuya Yoshikawa wrote:
>> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>>
>> Index calculation to access lpage_info appears three times.
>> A helper is worthwhile.
>>
>
> Why not get lpage_info instead of index in the helper?

Seems a better way! I didn't think about it.

I'll check and resend [2/2] if it's OK.

Thanks,
   Takuya

Re: [PATCH 2/2 rebased] KVM: MMU: Introduce a helper to access lpage_info

[Takuya Yoshikawa]

(2010/12/06 16:55), Takuya Yoshikawa wrote:
> (2010/12/06 15:37), Xiao Guangrong wrote:
>> On 12/06/2010 12:13 AM, Takuya Yoshikawa wrote:
>>> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>>>
>>> Index calculation to access lpage_info appears three times.
>>> A helper is worthwhile.
>>>
>>
>> Why not get lpage_info instead of index in the helper?
>
> Seems a better way! I didn't think about it.
>
> I'll check and resend [2/2] if it's OK.
>

Sadly, the structure of lpage_info has no name.

Returning rmap_pde may be another possible cleanup.

But I don't know it's worthwhile.


Thanks,
   Takuya

Re: [PATCH 2/2 rebased] KVM: MMU: Introduce a helper to access lpage_info

[Avi Kivity]

On 12/06/2010 10:57 AM, Takuya Yoshikawa wrote:
> (2010/12/06 16:55), Takuya Yoshikawa wrote:
>> (2010/12/06 15:37), Xiao Guangrong wrote:
>>> On 12/06/2010 12:13 AM, Takuya Yoshikawa wrote:
>>>> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>>>>
>>>> Index calculation to access lpage_info appears three times.
>>>> A helper is worthwhile.
>>>>
>>>
>>> Why not get lpage_info instead of index in the helper?
>>
>> Seems a better way! I didn't think about it.
>>
>> I'll check and resend [2/2] if it's OK.
>>
>
> Sadly, the structure of lpage_info has no name.
>

Well, you can give it a name.  Meanwhile I've applied patch 1, thanks.

-- 
error compiling committee.c: too many arguments to function

[PATCH 2/2 v2] KVM: MMU: Make the way of accessing lpage_info more generic

[Takuya Yoshikawa]

> 
> Well, you can give it a name.  Meanwhile I've applied patch 1, thanks.
> 

How about this?

===
Subject: [PATCH] KVM: MMU: Make the way of accessing lpage_info more generic

Large page information has two elements but one of them, write_count, alone
is accessed by a helper function.

This patch replaces this helper function with more generic one which returns
newly named kvm_lpage_info structure and use it to access the other element
rmap_pde.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 Change log
   v1->v2: adapted Xiao and Avi's advice to return lpage_info but idx.

 arch/x86/kvm/mmu.c       |   54 +++++++++++++++++++++------------------------
 include/linux/kvm_host.h |   10 +++++---
 2 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index d75ba1e..102d2a7 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -477,46 +477,46 @@ static void kvm_mmu_page_set_gfn(struct kvm_mmu_page *sp, int index, gfn_t gfn)
 }
 
 /*
- * Return the pointer to the largepage write count for a given
- * gfn, handling slots that are not large page aligned.
+ * Return the pointer to the large page information for a given gfn,
+ * handling slots that are not large page aligned.
  */
-static int *slot_largepage_idx(gfn_t gfn,
-			       struct kvm_memory_slot *slot,
-			       int level)
+static struct kvm_lpage_info *lpage_info_slot(gfn_t gfn,
+					      struct kvm_memory_slot *slot,
+					      int level)
 {
 	unsigned long idx;
 
 	idx = (gfn >> KVM_HPAGE_GFN_SHIFT(level)) -
 	      (slot->base_gfn >> KVM_HPAGE_GFN_SHIFT(level));
-	return &slot->lpage_info[level - 2][idx].write_count;
+	return &slot->lpage_info[level - 2][idx];
 }
 
 static void account_shadowed(struct kvm *kvm, gfn_t gfn)
 {
 	struct kvm_memory_slot *slot;
-	int *write_count;
+	struct kvm_lpage_info *linfo;
 	int i;
 
 	slot = gfn_to_memslot(kvm, gfn);
 	for (i = PT_DIRECTORY_LEVEL;
 	     i < PT_PAGE_TABLE_LEVEL + KVM_NR_PAGE_SIZES; ++i) {
-		write_count   = slot_largepage_idx(gfn, slot, i);
-		*write_count += 1;
+		linfo = lpage_info_slot(gfn, slot, i);
+		linfo->write_count += 1;
 	}
 }
 
 static void unaccount_shadowed(struct kvm *kvm, gfn_t gfn)
 {
 	struct kvm_memory_slot *slot;
-	int *write_count;
+	struct kvm_lpage_info *linfo;
 	int i;
 
 	slot = gfn_to_memslot(kvm, gfn);
 	for (i = PT_DIRECTORY_LEVEL;
 	     i < PT_PAGE_TABLE_LEVEL + KVM_NR_PAGE_SIZES; ++i) {
-		write_count   = slot_largepage_idx(gfn, slot, i);
-		*write_count -= 1;
-		WARN_ON(*write_count < 0);
+		linfo = lpage_info_slot(gfn, slot, i);
+		linfo->write_count -= 1;
+		WARN_ON(linfo->write_count < 0);
 	}
 }
 
@@ -525,12 +525,12 @@ static int has_wrprotected_page(struct kvm *kvm,
 				int level)
 {
 	struct kvm_memory_slot *slot;
-	int *largepage_idx;
+	struct kvm_lpage_info *linfo;
 
 	slot = gfn_to_memslot(kvm, gfn);
 	if (slot) {
-		largepage_idx = slot_largepage_idx(gfn, slot, level);
-		return *largepage_idx;
+		linfo = lpage_info_slot(gfn, slot, level);
+		return linfo->write_count;
 	}
 
 	return 1;
@@ -585,16 +585,15 @@ static int mapping_level(struct kvm_vcpu *vcpu, gfn_t large_gfn)
 static unsigned long *gfn_to_rmap(struct kvm *kvm, gfn_t gfn, int level)
 {
 	struct kvm_memory_slot *slot;
-	unsigned long idx;
+	struct kvm_lpage_info *linfo;
 
 	slot = gfn_to_memslot(kvm, gfn);
 	if (likely(level == PT_PAGE_TABLE_LEVEL))
 		return &slot->rmap[gfn - slot->base_gfn];
 
-	idx = (gfn >> KVM_HPAGE_GFN_SHIFT(level)) -
-		(slot->base_gfn >> KVM_HPAGE_GFN_SHIFT(level));
+	linfo = lpage_info_slot(gfn, slot, level);
 
-	return &slot->lpage_info[level - 2][idx].rmap_pde;
+	return &linfo->rmap_pde;
 }
 
 /*
@@ -882,19 +881,16 @@ static int kvm_handle_hva(struct kvm *kvm, unsigned long hva,
 		end = start + (memslot->npages << PAGE_SHIFT);
 		if (hva >= start && hva < end) {
 			gfn_t gfn_offset = (hva - start) >> PAGE_SHIFT;
+			gfn_t gfn = memslot->base_gfn + gfn_offset;
 
 			ret = handler(kvm, &memslot->rmap[gfn_offset], data);
 
 			for (j = 0; j < KVM_NR_PAGE_SIZES - 1; ++j) {
-				unsigned long idx;
-				int sh;
-
-				sh = KVM_HPAGE_GFN_SHIFT(PT_DIRECTORY_LEVEL+j);
-				idx = ((memslot->base_gfn+gfn_offset) >> sh) -
-					(memslot->base_gfn >> sh);
-				ret |= handler(kvm,
-					&memslot->lpage_info[j][idx].rmap_pde,
-					data);
+				struct kvm_lpage_info *linfo;
+
+				linfo = lpage_info_slot(gfn, memslot,
+							PT_DIRECTORY_LEVEL + j);
+				ret |= handler(kvm, &linfo->rmap_pde, data);
 			}
 			trace_kvm_age_page(hva, memslot, ret);
 			retval |= ret;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ac4e83a..bd0da8f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -146,6 +146,11 @@ struct kvm_vcpu {
  */
 #define KVM_MEM_MAX_NR_PAGES ((1UL << 31) - 1)
 
+struct kvm_lpage_info {
+	unsigned long rmap_pde;
+	int write_count;
+};
+
 struct kvm_memory_slot {
 	gfn_t base_gfn;
 	unsigned long npages;
@@ -153,10 +158,7 @@ struct kvm_memory_slot {
 	unsigned long *rmap;
 	unsigned long *dirty_bitmap;
 	unsigned long *dirty_bitmap_head;
-	struct {
-		unsigned long rmap_pde;
-		int write_count;
-	} *lpage_info[KVM_NR_PAGE_SIZES - 1];
+	struct kvm_lpage_info *lpage_info[KVM_NR_PAGE_SIZES - 1];
 	unsigned long userspace_addr;
 	int user_alloc;
 	int id;
-- 
1.7.1

Re: [PATCH 2/2 v2] KVM: MMU: Make the way of accessing lpage_info more generic

[Avi Kivity]

On 12/07/2010 05:59 AM, Takuya Yoshikawa wrote:
> >
> >  Well, you can give it a name.  Meanwhile I've applied patch 1, thanks.
> >
>
> How about this?

Looks good, applied, thanks.

-- 
error compiling committee.c: too many arguments to function