kvm, x86: use ro page and don't copy shared page

When page fault, we always call get_user_pages(write=1).

Actually, we don't need to do this when it is not write fault. get_user_pages(write=1) will cause shared page(ksm) copied. If this page is not modified in future, this copying and the copied page are just wasted. Ksm may scan and merge them and may cause thrash.

In this patch, if the page is RO for host VMM and it not write fault for guest, we will use RO page, otherwise we use a writable page.

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

---

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

index 8ba9b0d..6382140 100644
@@ -1832,6 +1832,45 @@ static void kvm_unsync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
 	}
 }
 
+/* get a current mapped page fast, and test whether the page is writable. */
+static struct page *get_user_page_and_protection(unsigned long addr,
+	int *writable)
+{
+	struct page *page[1];
+
+	if (__get_user_pages_fast(addr, 1, 1, page) == 1) {
+		*writable = 1;
+		return page[0];
+	}
+	if (__get_user_pages_fast(addr, 1, 0, page) == 1) {
+		*writable = 0;
+		return page[0];
+	}
+	return NULL;
+}
+
+static pfn_t kvm_get_pfn_for_page_fault(struct kvm *kvm, gfn_t gfn,
+		int write_fault, int *host_writable)
+{
+	unsigned long addr;
+	struct page *page;
+
+	if (!write_fault) {
+		addr = gfn_to_hva(kvm, gfn);
+		if (kvm_is_error_hva(addr)) {
+			get_page(bad_page);
+			return page_to_pfn(bad_page);
+		}

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

 static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
 				  bool can_unsync)
 {

@@ -2085,6 +2124,7 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, int write, gfn_t gfn)
 	int level;
 	pfn_t pfn;
 	unsigned long mmu_seq;
+	int host_writable;
 
 	level = mapping_level(vcpu, gfn);
 

@@ -2099,7 +2139,7 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, int write, gfn_t gfn)
 
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
 	smp_rmb();
-	pfn = kvm_get_pfn_for_gfn(vcpu->kvm, gfn);
+	pfn = kvm_get_pfn_for_page_fault(vcpu->kvm, gfn, write, &host_writable);
 
 	/* mmio */
 	if (is_error_pfn(pfn))

@@ -2109,7 +2149,7 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, int write, gfn_t gfn)
 	if (mmu_notifier_retry(vcpu, mmu_seq))
 		goto out_unlock;
 	kvm_mmu_free_some_pages(vcpu);
-	r = __direct_map(vcpu, v, write, level, gfn, pfn, true);
+	r = __direct_map(vcpu, v, write, level, gfn, pfn, host_writable);
 	spin_unlock(&vcpu->kvm->mmu_lock);
 
 

@@ -2307,6 +2347,8 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa,
 	int level;
 	gfn_t gfn = gpa >> PAGE_SHIFT;
 	unsigned long mmu_seq;
+	int write_fault = error_code & PFERR_WRITE_MASK;
+	int host_writable;
 
 	ASSERT(vcpu);
 	ASSERT(VALID_PAGE(vcpu->arch.mmu.root_hpa));

@@ -2321,15 +2363,16 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa,
 
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
 	smp_rmb();
-	pfn = kvm_get_pfn_for_gfn(vcpu->kvm, gfn);
+	pfn = kvm_get_pfn_for_page_fault(vcpu->kvm, gfn, write_fault,
+			&host_writable);
 	if (is_error_pfn(pfn))
 		return kvm_handle_bad_page(vcpu->kvm, gfn, pfn);
 	spin_lock(&vcpu->kvm->mmu_lock);
 	if (mmu_notifier_retry(vcpu, mmu_seq))
 		goto out_unlock;
 	kvm_mmu_free_some_pages(vcpu);
-	r = __direct_map(vcpu, gpa, error_code & PFERR_WRITE_MASK,
-			 level, gfn, pfn, true);
+	r = __direct_map(vcpu, gpa, write_fault,
+			 level, gfn, pfn, host_writable);
 	spin_unlock(&vcpu->kvm->mmu_lock);
 
 	return r;

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

index a9dbaa0..1874f51 100644
@@ -430,6 +430,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr,
 	pfn_t pfn;
 	int level = PT_PAGE_TABLE_LEVEL;
 	unsigned long mmu_seq;
+	int host_writable;
 
 	pgprintk("%s: addr %lx err %x\n", __func__, addr, error_code);
 	kvm_mmu_audit(vcpu, "pre page fault");

@@ -461,7 +462,8 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr,
 
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
 	smp_rmb();
-	pfn = kvm_get_pfn_for_gfn(vcpu->kvm, walker.gfn);
+	pfn = kvm_get_pfn_for_page_fault(vcpu->kvm, walker.gfn, write_fault,
+			&host_writable);
 
 	/* mmio */
 	if (is_error_pfn(pfn))

@@ -472,7 +474,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr,
 		goto out_unlock;
 	kvm_mmu_free_some_pages(vcpu);
 	sptep = FNAME(fetch)(vcpu, addr, &walker, user_fault, write_fault,
-			     level, &write_pt, pfn, true);
+			     level, &write_pt, pfn, host_writable);
 	(void)sptep;
 	pgprintk("%s: shadow pte %p %llx ptwrite %d\n", __func__,
 		 sptep, *sptep, write_pt);

arch/x86/mm/gup.c +2 -1

index 738e659..a4ce19f 100644
@@ -8,6 +8,7 @@
 #include <linux/mm.h>
 #include <linux/vmstat.h>
 #include <linux/highmem.h>
+#include <linux/module.h>
 
 #include <asm/pgtable.h>
 

@@ -274,6 +275,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 
 	return nr;
 }
+EXPORT_SYMBOL_GPL(__get_user_pages_fast);
 
 /**
  * get_user_pages_fast() - pin user pages in memory
--

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

Quoted 7 lines · Lai Jiangshan

When page fault, we always call get_user_pages(write=1).

Actually, we don't need to do this when it is not write fault. get_user_pages(write=1) will cause shared page(ksm) copied. If this page is not modified in future, this copying and the copied page are just wasted. Ksm may scan and merge them and may cause thrash.

But is page is written into afterwords we will get another page fault.

Quoted 3 lines

In this patch, if the page is RO for host VMM and it not write fault for guest, we will use RO page, otherwise we use a writable page.

Currently pages allocated for guest memory are required to be RW, so after your series behaviour will remain exactly the same as before.

Quoted 46 lines

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

---

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

index 8ba9b0d..6382140 100644
@@ -1832,6 +1832,45 @@ static void kvm_unsync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
 	}
 }
 
+/* get a current mapped page fast, and test whether the page is writable. */
+static struct page *get_user_page_and_protection(unsigned long addr,
+	int *writable)
+{
+	struct page *page[1];
+
+	if (__get_user_pages_fast(addr, 1, 1, page) == 1) {
+		*writable = 1;
+		return page[0];
+	}
+	if (__get_user_pages_fast(addr, 1, 0, page) == 1) {
+		*writable = 0;
+		return page[0];
+	}
+	return NULL;
+}
+
+static pfn_t kvm_get_pfn_for_page_fault(struct kvm *kvm, gfn_t gfn,
+		int write_fault, int *host_writable)
+{
+	unsigned long addr;
+	struct page *page;
+
+	if (!write_fault) {
+		addr = gfn_to_hva(kvm, gfn);
+		if (kvm_is_error_hva(addr)) {
+			get_page(bad_page);
+			return page_to_pfn(bad_page);
+		}
+
+		page = get_user_page_and_protection(addr, host_writable);
+		if (page)
+			return page_to_pfn(page);
+	}
+
+	*host_writable = 1;

arch/x86/kvm/mmu.c static void kvm_unsync_pages(struct kvm_vcpu *vcpu, gfn_t gfn) · line 1873

Quoted 3 lines

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

+	return kvm_get_pfn_for_gfn(kvm, gfn);
+}
+

kvm_get_pfn_for_gfn() returns fault_page if page is mapped RO, so caller of kvm_get_pfn_for_page_fault() and kvm_get_pfn_for_gfn() will get different results when called on the same page. Not good. kvm_get_pfn_for_page_fault() logic should be folded into kvm_get_pfn_for_gfn().

Quoted 10 lines · Gleb Natapov

Quoted 7 lines · Lai Jiangshan

When page fault, we always call get_user_pages(write=1).

Actually, we don't need to do this when it is not write fault. get_user_pages(write=1) will cause shared page(ksm) copied. If this page is not modified in future, this copying and the copied page are just wasted. Ksm may scan and merge them and may cause thrash.

But is page is written into afterwords we will get another page fault.

Quoted 5 lines

Quoted 3 lines

In this patch, if the page is RO for host VMM and it not write fault for guest, we will use RO page, otherwise we use a writable page.

Currently pages allocated for guest memory are required to be RW, so after your series behaviour will remain exactly the same as before.

Except KSM pages.

Quoted 46 lines

Quoted 46 lines

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

---

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

index 8ba9b0d..6382140 100644
@@ -1832,6 +1832,45 @@ static void kvm_unsync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
 	}
 }
 
+/* get a current mapped page fast, and test whether the page is writable. */
+static struct page *get_user_page_and_protection(unsigned long addr,
+	int *writable)
+{
+	struct page *page[1];
+
+	if (__get_user_pages_fast(addr, 1, 1, page) == 1) {
+		*writable = 1;
+		return page[0];
+	}
+	if (__get_user_pages_fast(addr, 1, 0, page) == 1) {
+		*writable = 0;
+		return page[0];
+	}
+	return NULL;
+}
+
+static pfn_t kvm_get_pfn_for_page_fault(struct kvm *kvm, gfn_t gfn,
+		int write_fault, int *host_writable)
+{
+	unsigned long addr;
+	struct page *page;
+
+	if (!write_fault) {
+		addr = gfn_to_hva(kvm, gfn);
+		if (kvm_is_error_hva(addr)) {
+			get_page(bad_page);
+			return page_to_pfn(bad_page);
+		}
+
+		page = get_user_page_and_protection(addr, host_writable);
+		if (page)
+			return page_to_pfn(page);
+	}
+
+	*host_writable = 1;

Quoted 8 lines

arch/x86/kvm/mmu.c static void kvm_unsync_pages(struct kvm_vcpu *vcpu, gfn_t gfn) · line 1873

Quoted 3 lines

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

+	return kvm_get_pfn_for_gfn(kvm, gfn);
+}
+

kvm_get_pfn_for_gfn() returns fault_page if page is mapped RO, so caller of kvm_get_pfn_for_page_fault() and kvm_get_pfn_for_gfn() will get different results when called on the same page. Not good. kvm_get_pfn_for_page_fault() logic should be folded into kvm_get_pfn_for_gfn().

Agreed. Please keep gfn_to_pfn related code in virt/kvm/kvm_main.c.

Quoted 7 lines

Quoted 4 lines

Currently pages allocated for guest memory are required to be RW, so after your series behaviour will remain exactly the same as before.

Except KSM pages.

KSM page will be COWed by __get_user_pages_fast(addr, 1, 1, page) in get_user_page_and_protection() just like it COWed now, no?

Quoted 5 lines · Gleb Natapov

Quoted 5 lines

Quoted 3 lines

Currently pages allocated for guest memory are required to be RW, so after your series behaviour will remain exactly the same as before.

Except KSM pages.

Quoted 2 lines

KSM page will be COWed by __get_user_pages_fast(addr, 1, 1, page) in get_user_page_and_protection() just like it COWed now, no?

Well, we don't want to COW it on write faults.

The optimal behaviour is:

- write faults: COW and instantiate a writeable spte

Quoted 5 lines · Avi Kivity

Quoted 3 lines · Gleb Natapov

Quoted 3 lines

Quoted 3 lines

Currently pages allocated for guest memory are required to be RW, so after your series behaviour will remain exactly the same as before.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Except KSM pages.

KSM page will be COWed by __get_user_pages_fast(addr, 1, 1, page) in get_user_page_and_protection() just like it COWed now, no?

Well, we don't want to COW it on write faults.

Quoted 4 lines

The optimal behaviour is:

- write faults: COW and instantiate a writeable spte

So do we or don't we want to COW on write faults?

Quoted 8 lines · Gleb Natapov

Quoted 7 lines · Lai Jiangshan

When page fault, we always call get_user_pages(write=1).

Actually, we don't need to do this when it is not write fault. get_user_pages(write=1) will cause shared page(ksm) copied. If this page is not modified in future, this copying and the copied page are just wasted. Ksm may scan and merge them and may cause thrash.

Quoted 7 lines

But is page is written into afterwords we will get another page fault.

Quoted 3 lines

In this patch, if the page is RO for host VMM and it not write fault for guest, we will use RO page, otherwise we use a writable page.

Currently pages allocated for guest memory are required to be RW, so after your series behaviour will remain exactly the same as before.

Except KSM pages.

Quoted 46 lines

Quoted 46 lines

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

---

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

index 8ba9b0d..6382140 100644
@@ -1832,6 +1832,45 @@ static void kvm_unsync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
 	}
 }
 
+/* get a current mapped page fast, and test whether the page is writable. */
+static struct page *get_user_page_and_protection(unsigned long addr,
+	int *writable)
+{
+	struct page *page[1];
+
+	if (__get_user_pages_fast(addr, 1, 1, page) == 1) {
+		*writable = 1;
+		return page[0];
+	}
+	if (__get_user_pages_fast(addr, 1, 0, page) == 1) {
+		*writable = 0;
+		return page[0];
+	}
+	return NULL;
+}
+
+static pfn_t kvm_get_pfn_for_page_fault(struct kvm *kvm, gfn_t gfn,
+		int write_fault, int *host_writable)
+{
+	unsigned long addr;
+	struct page *page;
+
+	if (!write_fault) {
+		addr = gfn_to_hva(kvm, gfn);
+		if (kvm_is_error_hva(addr)) {
+			get_page(bad_page);
+			return page_to_pfn(bad_page);
+		}
+
+		page = get_user_page_and_protection(addr, host_writable);
+		if (page)
+			return page_to_pfn(page);
+	}
+
+	*host_writable = 1;

Quoted 8 lines

arch/x86/kvm/mmu.c static void kvm_unsync_pages(struct kvm_vcpu *vcpu, gfn_t gfn) · line 1873

Quoted 3 lines

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

+	return kvm_get_pfn_for_gfn(kvm, gfn);
+}
+

kvm_get_pfn_for_gfn() returns fault_page if page is mapped RO, so caller of kvm_get_pfn_for_page_fault() and kvm_get_pfn_for_gfn() will get different results when called on the same page. Not good. kvm_get_pfn_for_page_fault() logic should be folded into kvm_get_pfn_for_gfn().

Agreed. Please keep gfn_to_pfn related code in virt/kvm/kvm_main.c.

Quoted 36 lines

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

+/* get a current mapped page fast, and test whether the page is writable. */
+static struct page *get_user_page_and_protection(unsigned long addr,
+	int *writable)
+{
+	struct page *page[1];
+
+	if (__get_user_pages_fast(addr, 1, 1, page) == 1) {
+		*writable = 1;
+		return page[0];
+	}
+	if (__get_user_pages_fast(addr, 1, 0, page) == 1) {
+		*writable = 0;
+		return page[0];
+	}
+	return NULL;
+}
+
+static pfn_t kvm_get_pfn_for_page_fault(struct kvm *kvm, gfn_t gfn,
+		int write_fault, int *host_writable)
+{
+	unsigned long addr;
+	struct page *page;
+
+	if (!write_fault) {
+		addr = gfn_to_hva(kvm, gfn);
+		if (kvm_is_error_hva(addr)) {
+			get_page(bad_page);
+			return page_to_pfn(bad_page);
+		}
+
+		page = get_user_page_and_protection(addr, host_writable);
+		if (page)
+			return page_to_pfn(page);
+	}
+
+	*host_writable = 1;

arch/x86/kvm/mmu.c static void kvm_unsync_pages(struct kvm_vcpu *vcpu, gfn_t gfn) · line 1873

Quoted 3 lines

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

+	return kvm_get_pfn_for_gfn(kvm, gfn);
+}
+

kvm_get_pfn_for_gfn() returns fault_page if page is mapped RO, so caller of kvm_get_pfn_for_page_fault() and kvm_get_pfn_for_gfn() will get different results when called on the same page. Not good. kvm_get_pfn_for_page_fault() logic should be folded into kvm_get_pfn_for_gfn().

Quoted 36 lines · Gleb Natapov

Quoted 36 lines

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

+/* get a current mapped page fast, and test whether the page is writable. */
+static struct page *get_user_page_and_protection(unsigned long addr,
+	int *writable)
+{
+	struct page *page[1];
+
+	if (__get_user_pages_fast(addr, 1, 1, page) == 1) {
+		*writable = 1;
+		return page[0];
+	}
+	if (__get_user_pages_fast(addr, 1, 0, page) == 1) {
+		*writable = 0;
+		return page[0];
+	}
+	return NULL;
+}
+
+static pfn_t kvm_get_pfn_for_page_fault(struct kvm *kvm, gfn_t gfn,
+		int write_fault, int *host_writable)
+{
+	unsigned long addr;
+	struct page *page;
+
+	if (!write_fault) {
+		addr = gfn_to_hva(kvm, gfn);
+		if (kvm_is_error_hva(addr)) {
+			get_page(bad_page);
+			return page_to_pfn(bad_page);
+		}
+
+		page = get_user_page_and_protection(addr, host_writable);
+		if (page)
+			return page_to_pfn(page);
+	}
+
+	*host_writable = 1;

Quoted 9 lines

arch/x86/kvm/mmu.c static void kvm_unsync_pages(struct kvm_vcpu *vcpu, gfn_t gfn) · line 1873

Quoted 3 lines

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

+	return kvm_get_pfn_for_gfn(kvm, gfn);
+}
+

kvm_get_pfn_for_gfn() returns fault_page if page is mapped RO, so caller of kvm_get_pfn_for_page_fault() and kvm_get_pfn_for_gfn() will get different results when called on the same page. Not good. kvm_get_pfn_for_page_fault() logic should be folded into kvm_get_pfn_for_gfn().

The different results are the things we just need.

We don't want to copy and write a page which is mapped RO when only read fault.