The current code depends on the fact that fault_page is the normal page, however, we will use the error code instead of these dummy pages in the later patch, so we use kvm_release_pfn_clean to release pfn which will release the error code properly

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

---
 arch/x86/kvm/mmu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

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

index 2419934..a9a2052 100644
@@ -3275,7 +3275,7 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 	if (!async)
 		return false; /* *pfn has correct page already */
 
-	put_page(pfn_to_page(*pfn));
+	kvm_release_pfn_clean(*pfn);
 
 	if (!prefault && can_do_async_pf(vcpu)) {
 		trace_kvm_try_async_get_page(gva, gfn);

Signature · 1 line

1.7.7.6

Xiao Guangrong

author

2012-07-26 03:58 UTC #2 lore raw

PATCH v2 2/3 KVM: use kvm_release_page_clean to release the page

In reply to Xiao Guangrong

In kvm_async_pf_wakeup_all, it uses bad_page to generate broadcast wakeup, and uses put_page to release bad_page, the work depends on the fact that bad_page is the normal page. But we will use the error code instead of bad_page, so use kvm_release_page_clean to release the page which will release the error code properly

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

---
 virt/kvm/async_pf.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

virt/kvm/async_pf.c +2 -2

index 74268b4..ebae24b 100644
@@ -112,7 +112,7 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
 				   typeof(*work), link);
 		list_del(&work->link);
 		if (work->page)
-			put_page(work->page);
+			kvm_release_page_clean(work->page);
 		kmem_cache_free(async_pf_cache, work);
 	}
 	spin_unlock(&vcpu->async_pf.lock);

@@ -139,7 +139,7 @@ void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
 		list_del(&work->queue);
 		vcpu->async_pf.queued--;
 		if (work->page)
-			put_page(work->page);
+			kvm_release_page_clean(work->page);
 		kmem_cache_free(async_pf_cache, work);
 	}
 }

Signature · 1 line

1.7.7.6

Xiao Guangrong

author

2012-07-26 03:59 UTC #3 lore raw

PATCH v2 3/3 KVM: remove dummy pages

In reply to Xiao Guangrong

Currently, kvm allocates some pages and use them as error indicators, it wastes memory and is not good for scalability

Base on Avi's suggestion, we use the error codes instead of these pages to indicate the error conditions

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

---
 include/linux/kvm_host.h |    3 +-
 virt/kvm/async_pf.c      |    3 +-
 virt/kvm/kvm_main.c      |  121 +++++++++++++++++++---------------------------
 3 files changed, 52 insertions(+), 75 deletions(-)

include/linux/kvm_host.h +2 -1

index 6d1a51e..f4e132c 100644
@@ -411,6 +411,7 @@ void kvm_arch_flush_shadow(struct kvm *kvm);
 int gfn_to_page_many_atomic(struct kvm *kvm, gfn_t gfn, struct page **pages,
 			    int nr_pages);
 
+struct page *get_bad_page(void);
 struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn);
 unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn);
 void kvm_release_page_clean(struct page *page);

@@ -564,7 +565,7 @@ void kvm_arch_sync_events(struct kvm *kvm);
 int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu);
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
 
-int kvm_is_mmio_pfn(pfn_t pfn);
+bool kvm_is_mmio_pfn(pfn_t pfn);
 
 struct kvm_irq_ack_notifier {
 	struct hlist_node link;

virt/kvm/async_pf.c +1 -2

index ebae24b..7972278 100644
@@ -203,8 +203,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
 	if (!work)
 		return -ENOMEM;
 
-	work->page = bad_page;
-	get_page(bad_page);
+	work->page = get_bad_page();
 	INIT_LIST_HEAD(&work->queue); /* for list_del to work */
 
 	spin_lock(&vcpu->async_pf.lock);

virt/kvm/kvm_main.c +49 ~~-72~~

221 lines not on this page · raw message

Signature · 1 line

1.7.7.6

Avi Kivity

2012-07-26 08:56 UTC #4 lore raw

Quoted 13 lines · Xiao Guangrong

Currently, kvm allocates some pages and use them as error indicators, it wastes memory and is not good for scalability
Base on Avi's suggestion, we use the error codes instead of these pages to indicate the error conditions
virt/kvm/kvm_main.c +6 -0
+static pfn_t get_bad_pfn(void)
+{
+	return -ENOENT;
+}
+
+pfn_t get_fault_pfn(void)

virt/kvm/kvm_main.c EXPORT_SYMBOL_GPL(kvm_disable_largepages); · line 958

Quoted 10 lines

virt/kvm/kvm_main.c +10 -0

+{
+	return -EFAULT;
+}
+EXPORT_SYMBOL_GPL(get_fault_pfn);
+
+static pfn_t get_hwpoison_pfn(void)
+{
+	return -EHWPOISON;
+}
+

Would be better as #defines

Quoted 11 lines

int is_hwpoison_pfn(pfn_t pfn)
{

virt/kvm/kvm_main.c +1 -2

-	return pfn == hwpoison_pfn;
+	return pfn == -EHWPOISON;
 }
 EXPORT_SYMBOL_GPL(is_hwpoison_pfn);
 
 int is_noslot_pfn(pfn_t pfn)
 {
-	return pfn == bad_pfn;

virt/kvm/kvm_main.c EXPORT_SYMBOL_GPL(kvm_disable_largepages); · line 975

Quoted 10 lines

virt/kvm/kvm_main.c +2 -1

+	return pfn == -ENOENT;
 }
 EXPORT_SYMBOL_GPL(is_noslot_pfn);
 
 int is_invalid_pfn(pfn_t pfn)
 {
-	return pfn == hwpoison_pfn || pfn == fault_pfn;
+	return !is_noslot_pfn(pfn) && is_error_pfn(pfn);
 }
 EXPORT_SYMBOL_GPL(is_invalid_pfn);

So is_*_pfn() could go away and be replaced by ==.

virt/kvm/kvm_main.c int gfn_to_page_many_atomic(struct kvm *kvm, gfn_t gfn, struct page **pages, · line 1249

Quoted 10 lines

EXPORT_SYMBOL_GPL(gfn_to_page);
void kvm_release_page_clean(struct page *page)
{
virt/kvm/kvm_main.c +2 -1
-	kvm_release_pfn_clean(page_to_pfn(page));
+	if (!is_error_page(page))
+		kvm_release_pfn_clean(page_to_pfn(page));
 }
 EXPORT_SYMBOL_GPL(kvm_release_page_clean);

Note, we can remove calls to kvm_release_page_clean() from error paths now, so in the future we can drop the test.

Since my comments are better done as a separate patch, I applied all three patches. Thanks!

Signature · 1 line

error compiling committee.c: too many arguments to function

Takuya Yoshikawa

2012-07-26 09:20 UTC #5 lore raw

Quoted 2 lines · On Thu, 26 Jul 2012 11:56:15 +0300 Avi Kivity <[email protected]> wrote:

Since my comments are better done as a separate patch, I applied all three patches. Thanks!

Is this patch really safe for all architectures?

IS_ERR_VALUE() casts -MAX_ERRNO to unsigned long and then does comparison. Isn't it possible to conflict with valid pfns?

What are the underlying assumptions?

Takuya

Xiao Guangrong

author

2012-07-26 09:35 UTC #6 lore raw

Quoted 11 lines · Takuya Yoshikawa

Quoted 2 lines · On Thu, 26 Jul 2012 11:56:15 +0300 Avi Kivity <[email protected]> wrote:
Since my comments are better done as a separate patch, I applied all three patches. Thanks!
Is this patch really safe for all architectures?
IS_ERR_VALUE() casts -MAX_ERRNO to unsigned long and then does comparison. Isn't it possible to conflict with valid pfns?

See IS_ERR_VALUE():

#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)

The minimal value of the error code is:
0xffff f001 on 32-bit and 0x ffff ffff ffff f001 on 64-bit, it is fair larger that a valid pfn (for the pfn, the most top of 12 bits are always 0).

Note, PAE is a special case, but only 64G physical memory is valid, 0xffff f001 is also suitable for that.

Takuya Yoshikawa

2012-07-26 09:52 UTC #7 lore raw

Quoted 10 lines · On Thu, 26 Jul 2012 17:35:13 +0800 Xiao Guangrong…

Quoted 5 lines
Is this patch really safe for all architectures?
IS_ERR_VALUE() casts -MAX_ERRNO to unsigned long and then does comparison. Isn't it possible to conflict with valid pfns?
See IS_ERR_VALUE():
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)

Quoted 7 lines

The minimal value of the error code is:
0xffff f001 on 32-bit and 0x ffff ffff ffff f001 on 64-bit, it is fair larger that a valid pfn (for the pfn, the most top of 12 bits are always 0).
Note, PAE is a special case, but only 64G physical memory is valid, 0xffff f001 is also suitable for that.

Ah, I see. I misread the type pfn_t and was confused. Thank you!

Takuya

Xiao Guangrong

author

2012-07-26 09:25 UTC #8 lore raw

In reply to Avi Kivity

Quoted 18 lines · Avi Kivity

Quoted 17 lines · Xiao Guangrong
Currently, kvm allocates some pages and use them as error indicators, it wastes memory and is not good for scalability
Base on Avi's suggestion, we use the error codes instead of these pages to indicate the error conditions
virt/kvm/kvm_main.c +10 -0
+static pfn_t get_bad_pfn(void)
+{
+	return -ENOENT;
+}
+
+pfn_t get_fault_pfn(void)
+{
+	return -EFAULT;
+}
+EXPORT_SYMBOL_GPL(get_fault_pfn);

Quoted 8 lines

virt/kvm/kvm_main.c EXPORT_SYMBOL_GPL(kvm_disable_largepages); · line 958
Quoted 6 lines
virt/kvm/kvm_main.c +6 -0
+
+static pfn_t get_hwpoison_pfn(void)
+{
+	return -EHWPOISON;
+}
+
Would be better as #defines

Okay.

Quoted 15 lines

int is_hwpoison_pfn(pfn_t pfn)
{

virt/kvm/kvm_main.c +2 -2

-	return pfn == hwpoison_pfn;
+	return pfn == -EHWPOISON;
 }
 EXPORT_SYMBOL_GPL(is_hwpoison_pfn);
 
 int is_noslot_pfn(pfn_t pfn)
 {
-	return pfn == bad_pfn;
+	return pfn == -ENOENT;
 }
 EXPORT_SYMBOL_GPL(is_noslot_pfn);

Quoted 10 lines

virt/kvm/kvm_main.c EXPORT_SYMBOL_GPL(kvm_disable_largepages); · line 975
Quoted 7 lines
int is_invalid_pfn(pfn_t pfn)
{
virt/kvm/kvm_main.c +1 -1
-	return pfn == hwpoison_pfn || pfn == fault_pfn;
+	return !is_noslot_pfn(pfn) && is_error_pfn(pfn);
 }
 EXPORT_SYMBOL_GPL(is_invalid_pfn);
So is_*_pfn() could go away and be replaced by ==.

Okay.

Quoted 14 lines

virt/kvm/kvm_main.c int gfn_to_page_many_atomic(struct kvm *kvm, gfn_t gfn, struct page **pages, · line 1249
Quoted 10 lines
EXPORT_SYMBOL_GPL(gfn_to_page);
void kvm_release_page_clean(struct page *page)
{
virt/kvm/kvm_main.c +2 -1
-	kvm_release_pfn_clean(page_to_pfn(page));
+	if (!is_error_page(page))
+		kvm_release_pfn_clean(page_to_pfn(page));
 }
 EXPORT_SYMBOL_GPL(kvm_release_page_clean);
Note, we can remove calls to kvm_release_page_clean() from error paths now, so in the future we can drop the test.

Right, since the release path (kvm_release_page_clean) is used in many place and on many architectures, i did the change as small as possible for good review.

Quoted 1 line

Since my comments are better done as a separate patch,

Yes, i will make a patch to apply all your comments. Thank you, Avi!