Skip to contentSkip to conversation

KVM: iommu: fix releasing unmapped page

PATCH In review Xiao Guangrong 3 participants 14 messages Aug 3, 2012 – Aug 6, 2012

Subsystems (5)
x86 KVM PowerPC KVM x86 +1 more subsystems
lore
Share & tools
lore

Xiao Guangrong

author
#0 lore raw

PATCH v2 1/10 KVM: iommu: fix releasing unmapped page

There are two bugs:
- the 'error page' is forgot to be released
[ it is unneeded after commit a2766325cf9f9, for backport, we
still do kvm_release_pfn_clean for the error pfn ]

- guest pages are always released regardless of the unmapped page (e,g, caused by hwpoison)

Signed-off-by Xiao Guangrong <[email protected]>
---
 virt/kvm/iommu.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
virt/kvm/iommu.c +7 -0
index c03f1fb..6a67bea 100644
@@ -107,6 +107,7 @@ int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
 		 */
 		pfn = kvm_pin_pages(slot, gfn, page_size);
 		if (is_error_pfn(pfn)) {
+			kvm_release_pfn_clean(pfn);
 			gfn += 1;
 			continue;
 		}

@@ -300,6 +301,12 @@ static void kvm_iommu_put_pages(struct kvm *kvm,
 
 		/* Get physical address */
 		phys = iommu_iova_to_phys(domain, gfn_to_gpa(gfn));
+
+		if (!phys) {
+			gfn++;
+			continue;
+		}
+
 		pfn  = phys >> PAGE_SHIFT;
 
 		/* Unmap address from IO address space */
Signature · 1 line
1.7.7.6
author
#1 lore raw

PATCH v2 2/10 KVM: introduce KVM_PFN_ERR_FAULT

After that, the exported and un-inline function, get_fault_pfn, can be removed

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c       |    2 +-
 include/linux/kvm_host.h |    3 ++-
 virt/kvm/kvm_main.c      |   12 +++---------
 3 files changed, 6 insertions(+), 11 deletions(-)
arch/x86/kvm/mmu.c +1 -1
index a9a2052..39ed315 100644
@@ -2514,7 +2514,7 @@ static pfn_t pte_prefetch_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn,
 
 	slot = gfn_to_memslot_dirty_bitmap(vcpu, gfn, no_dirty_log);
 	if (!slot)
-		return get_fault_pfn();
+		return KVM_PFN_ERR_FAULT;
 
 	hva = gfn_to_hva_memslot(slot, gfn);
include/linux/kvm_host.h +2 -1
index dbc65f9..4c39543 100644
@@ -48,6 +48,8 @@
 #define KVM_MAX_MMIO_FRAGMENTS \
 	(KVM_MMIO_SIZE / KVM_USER_MMIO_SIZE + KVM_EXTRA_MMIO_FRAGMENTS)
 
+#define KVM_PFN_ERR_FAULT	(-EFAULT)
+
 /*
  * vcpu->requests bit members
  */

@@ -444,7 +446,6 @@ void kvm_release_pfn_clean(pfn_t pfn);
 void kvm_set_pfn_dirty(pfn_t pfn);
 void kvm_set_pfn_accessed(pfn_t pfn);
 void kvm_get_pfn(pfn_t pfn);
-pfn_t get_fault_pfn(void);
 
 int kvm_read_guest_page(struct kvm *kvm, gfn_t gfn, void *data, int offset,
 			int len);
virt/kvm/kvm_main.c +3 -9
index bcf973e..9c084f8 100644
@@ -948,12 +948,6 @@ static pfn_t get_bad_pfn(void)
 	return -ENOENT;
 }
 
-pfn_t get_fault_pfn(void)
-{
-	return -EFAULT;
-}
-EXPORT_SYMBOL_GPL(get_fault_pfn);
-
 static pfn_t get_hwpoison_pfn(void)
 {
 	return -EHWPOISON;

@@ -1124,7 +1118,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
 		struct vm_area_struct *vma;
 
 		if (atomic)
-			return get_fault_pfn();
+			return KVM_PFN_ERR_FAULT;
 
 		down_read(&current->mm->mmap_sem);
 		if (npages == -EHWPOISON ||

@@ -1136,7 +1130,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
 		vma = find_vma_intersection(current->mm, addr, addr+1);
 
 		if (vma == NULL)
-			pfn = get_fault_pfn();
+			pfn = KVM_PFN_ERR_FAULT;
 		else if ((vma->vm_flags & VM_PFNMAP)) {
 			pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) +
 				vma->vm_pgoff;

@@ -1144,7 +1138,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
 		} else {
 			if (async && (vma->vm_flags & VM_WRITE))
 				*async = true;
-			pfn = get_fault_pfn();
+			pfn = KVM_PFN_ERR_FAULT;
 		}
 		up_read(&current->mm->mmap_sem);
 	} else
Signature · 1 line
1.7.7.6
#2 lore raw

Re: [PATCH v2 02/10] KVM: introduce KVM_PFN_ERR_FAULT

include/linux/kvm_host.h line 52

Quoted 6 lines · Xiao Guangrong

After that, the exported and un-inline function, get_fault_pfn, can be removed

include/linux/kvm_host.h +2 -0
+#define KVM_PFN_ERR_FAULT	(-EFAULT)
+

IMO this symbol isn't needed, just use -EFAULT (and -EHWPOISON etc.) directly. Just document it in hva_to_pfn(), since this isn't an ordinary -EFAULT (and -ENOENT certainly needs describing).

Signature · 1 line
error compiling committee.c: too many arguments to function
#3 lore raw

Re: [PATCH v2 02/10] KVM: introduce KVM_PFN_ERR_FAULT

Quoted 11 lines · Avi Kivity

include/linux/kvm_host.h line 52

Quoted 6 lines · Xiao Guangrong

After that, the exported and un-inline function, get_fault_pfn, can be removed

include/linux/kvm_host.h +2 -0
+#define KVM_PFN_ERR_FAULT	(-EFAULT)
+

IMO this symbol isn't needed, just use -EFAULT (and -EHWPOISON etc.) directly. Just document it in hva_to_pfn(), since this isn't an ordinary -EFAULT (and -ENOENT certainly needs describing).

Okay, I see why you did that, in patch 10.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#4 lore raw

PATCH v2 3/10 KVM: introduce KVM_PFN_ERR_HWPOISON

In reply to Xiao Guangrong

Then, get_hwpoison_pfn and is_hwpoison_pfn can be removed

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c       |    2 +-
 include/linux/kvm_host.h |    2 +-
 virt/kvm/kvm_main.c      |   13 +------------
 3 files changed, 3 insertions(+), 14 deletions(-)
arch/x86/kvm/mmu.c +1 -1
index 39ed315..924b4e8 100644
@@ -2651,7 +2651,7 @@ static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct *
 static int kvm_handle_bad_page(struct kvm_vcpu *vcpu, gfn_t gfn, pfn_t pfn)
 {
 	kvm_release_pfn_clean(pfn);
-	if (is_hwpoison_pfn(pfn)) {
+	if (pfn == KVM_PFN_ERR_HWPOISON) {
 		kvm_send_hwpoison_signal(gfn_to_hva(vcpu->kvm, gfn), current);
 		return 0;
 	}
include/linux/kvm_host.h +1 -1
index 4c39543..cbd5af8 100644
@@ -49,6 +49,7 @@
 	(KVM_MMIO_SIZE / KVM_USER_MMIO_SIZE + KVM_EXTRA_MMIO_FRAGMENTS)
 
 #define KVM_PFN_ERR_FAULT	(-EFAULT)
+#define KVM_PFN_ERR_HWPOISON	(-EHWPOISON)
 
 /*
  * vcpu->requests bit members

@@ -396,7 +397,6 @@ extern struct page *bad_page;
 
 int is_error_page(struct page *page);
 int is_error_pfn(pfn_t pfn);
-int is_hwpoison_pfn(pfn_t pfn);
 int is_noslot_pfn(pfn_t pfn);
 int is_invalid_pfn(pfn_t pfn);
 int kvm_is_error_hva(unsigned long addr);
virt/kvm/kvm_main.c +1 -12
index 9c084f8..f17ce44 100644
@@ -948,17 +948,6 @@ static pfn_t get_bad_pfn(void)
 	return -ENOENT;
 }
 
-static pfn_t get_hwpoison_pfn(void)
-{
-	return -EHWPOISON;
-}
-
-int is_hwpoison_pfn(pfn_t pfn)
-{
-	return pfn == -EHWPOISON;
-}
-EXPORT_SYMBOL_GPL(is_hwpoison_pfn);
-
 int is_noslot_pfn(pfn_t pfn)
 {
 	return pfn == -ENOENT;

@@ -1124,7 +1113,7 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
 		if (npages == -EHWPOISON ||
 			(!async && check_user_page_hwpoison(addr))) {
 			up_read(&current->mm->mmap_sem);
-			return get_hwpoison_pfn();
+			return KVM_PFN_ERR_HWPOISON;
 		}
 
 		vma = find_vma_intersection(current->mm, addr, addr+1);
Signature · 1 line
1.7.7.6
author
#5 lore raw

PATCH v2 4/10 KVM: introduce KVM_PFN_ERR_BAD

In reply to Xiao Guangrong

Then, remove get_bad_pfn

Signed-off-by Xiao Guangrong <[email protected]>
---
 include/linux/kvm_host.h |    1 +
 virt/kvm/kvm_main.c      |    7 +------
 2 files changed, 2 insertions(+), 6 deletions(-)
include/linux/kvm_host.h +1 -0
index cbd5af8..ba8b222 100644
@@ -50,6 +50,7 @@
 
 #define KVM_PFN_ERR_FAULT	(-EFAULT)
 #define KVM_PFN_ERR_HWPOISON	(-EHWPOISON)
+#define KVM_PFN_ERR_BAD		(-ENOENT)
 
 /*
  * vcpu->requests bit members
virt/kvm/kvm_main.c +1 -6
index f17ce44..75d3c76 100644
@@ -943,11 +943,6 @@ int is_error_pfn(pfn_t pfn)
 }
 EXPORT_SYMBOL_GPL(is_error_pfn);
 
-static pfn_t get_bad_pfn(void)
-{
-	return -ENOENT;
-}
-
 int is_noslot_pfn(pfn_t pfn)
 {
 	return pfn == -ENOENT;

@@ -1152,7 +1147,7 @@ static pfn_t __gfn_to_pfn(struct kvm *kvm, gfn_t gfn, bool atomic, bool *async,
 
 	addr = gfn_to_hva(kvm, gfn);
 	if (kvm_is_error_hva(addr))
-		return get_bad_pfn();
+		return KVM_PFN_ERR_BAD;
 
 	return hva_to_pfn(addr, atomic, async, write_fault, writable);
 }
Signature · 1 line
1.7.7.6
author
#6 lore raw

PATCH v2 5/10 KVM: inline is_*_pfn functions

In reply to Xiao Guangrong

These functions are exported and can not inline, move them to kvm_host.h to eliminate the overload of function call

Signed-off-by Xiao Guangrong <[email protected]>
---
 include/linux/kvm_host.h |   19 ++++++++++++++++---
 virt/kvm/kvm_main.c      |   18 ------------------
 2 files changed, 16 insertions(+), 21 deletions(-)
include/linux/kvm_host.h +16 -3
index ba8b222..f604d1d 100644
@@ -21,6 +21,7 @@
 #include <linux/slab.h>
 #include <linux/rcupdate.h>
 #include <linux/ratelimit.h>
+#include <linux/err.h>
 #include <asm/signal.h>
 
 #include <linux/kvm.h>

@@ -52,6 +53,21 @@
 #define KVM_PFN_ERR_HWPOISON	(-EHWPOISON)
 #define KVM_PFN_ERR_BAD		(-ENOENT)
 
+static inline int is_error_pfn(pfn_t pfn)
+{
+	return IS_ERR_VALUE(pfn);
+}
+
+static inline int is_noslot_pfn(pfn_t pfn)
+{
+	return pfn == -ENOENT;
+}
+
+static inline int is_invalid_pfn(pfn_t pfn)
+{
+	return !is_noslot_pfn(pfn) && is_error_pfn(pfn);
+}
+
 /*
  * vcpu->requests bit members
  */

@@ -397,9 +413,6 @@ id_to_memslot(struct kvm_memslots *slots, int id)
 extern struct page *bad_page;
 
 int is_error_page(struct page *page);
-int is_error_pfn(pfn_t pfn);
-int is_noslot_pfn(pfn_t pfn);
-int is_invalid_pfn(pfn_t pfn);
 int kvm_is_error_hva(unsigned long addr);
 int kvm_set_memory_region(struct kvm *kvm,
 			  struct kvm_userspace_memory_region *mem,
virt/kvm/kvm_main.c +0 -18
index 75d3c76..08b600b 100644
@@ -937,24 +937,6 @@ int is_error_page(struct page *page)
 }
 EXPORT_SYMBOL_GPL(is_error_page);
 
-int is_error_pfn(pfn_t pfn)
-{
-	return IS_ERR_VALUE(pfn);
-}
-EXPORT_SYMBOL_GPL(is_error_pfn);
-
-int is_noslot_pfn(pfn_t pfn)
-{
-	return pfn == -ENOENT;
-}
-EXPORT_SYMBOL_GPL(is_noslot_pfn);
-
-int is_invalid_pfn(pfn_t pfn)
-{
-	return !is_noslot_pfn(pfn) && is_error_pfn(pfn);
-}
-EXPORT_SYMBOL_GPL(is_invalid_pfn);
-
 struct page *get_bad_page(void)
 {
 	return ERR_PTR(-ENOENT);
Signature · 1 line
1.7.7.6
author
#7 lore raw

PATCH v2 6/10 KVM: remove the unused declare

In reply to Xiao Guangrong

Remove it since it is not used anymore

Signed-off-by Xiao Guangrong <[email protected]>
---
 include/linux/kvm_host.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
include/linux/kvm_host.h +0 -2
index f604d1d..bdf2182 100644
@@ -410,8 +410,6 @@ id_to_memslot(struct kvm_memslots *slots, int id)
 	return slot;
 }
 
-extern struct page *bad_page;
-
 int is_error_page(struct page *page);
 int kvm_is_error_hva(unsigned long addr);
 int kvm_set_memory_region(struct kvm *kvm,
Signature · 1 line
1.7.7.6
author
#8 lore raw

PATCH v2 7/10 KVM: introduce KVM_ERR_PTR_BAD_PAGE

In reply to Xiao Guangrong

It is used to eliminate the overload of function call and cleanup the code

Signed-off-by Xiao Guangrong <[email protected]>
---
 include/linux/kvm_host.h |    9 +++++++--
 virt/kvm/async_pf.c      |    2 +-
 virt/kvm/kvm_main.c      |   13 +------------
 3 files changed, 9 insertions(+), 15 deletions(-)
include/linux/kvm_host.h +7 -2
index bdf2182..0aebe7a 100644
@@ -68,6 +68,13 @@ static inline int is_invalid_pfn(pfn_t pfn)
 	return !is_noslot_pfn(pfn) && is_error_pfn(pfn);
 }
 
+#define KVM_ERR_PTR_BAD_PAGE	(ERR_PTR(-ENOENT))
+
+static inline int is_error_page(struct page *page)
+{
+	return IS_ERR(page);
+}
+
 /*
  * vcpu->requests bit members
  */

@@ -410,7 +417,6 @@ id_to_memslot(struct kvm_memslots *slots, int id)
 	return slot;
 }
 
-int is_error_page(struct page *page);
 int kvm_is_error_hva(unsigned long addr);
 int kvm_set_memory_region(struct kvm *kvm,
 			  struct kvm_userspace_memory_region *mem,

@@ -437,7 +443,6 @@ 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);
virt/kvm/async_pf.c +1 -1
index 7972278..56f5533 100644
@@ -203,7 +203,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
 	if (!work)
 		return -ENOMEM;
 
-	work->page = get_bad_page();
+	work->page = KVM_ERR_PTR_BAD_PAGE;
 	INIT_LIST_HEAD(&work->queue); /* for list_del to work */
 
 	spin_lock(&vcpu->async_pf.lock);
virt/kvm/kvm_main.c +1 -12
index 08b600b..5873031 100644
@@ -931,17 +931,6 @@ void kvm_disable_largepages(void)
 }
 EXPORT_SYMBOL_GPL(kvm_disable_largepages);
 
-int is_error_page(struct page *page)
-{
-	return IS_ERR(page);
-}
-EXPORT_SYMBOL_GPL(is_error_page);
-
-struct page *get_bad_page(void)
-{
-	return ERR_PTR(-ENOENT);
-}
-
 static inline unsigned long bad_hva(void)
 {
 	return PAGE_OFFSET;

@@ -1188,7 +1177,7 @@ static struct page *kvm_pfn_to_page(pfn_t pfn)
 	WARN_ON(kvm_is_mmio_pfn(pfn));
 
 	if (is_error_pfn(pfn) || kvm_is_mmio_pfn(pfn))
-		return get_bad_page();
+		return KVM_ERR_PTR_BAD_PAGE;
 
 	return pfn_to_page(pfn);
 }
Signature · 1 line
1.7.7.6
author
#9 lore raw

PATCH v2 8/10 KVM: do not release the error pfn

In reply to Xiao Guangrong

After commit a2766325cf9f9, the error pfn is replaced by the error code, it need not be released anymore

[ The patch has been compiling tested for powerpc ]

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/powerpc/kvm/e500_tlb.c |    1 -
 arch/x86/kvm/mmu.c          |    7 +++----
 arch/x86/kvm/mmu_audit.c    |    4 +---
 arch/x86/kvm/paging_tmpl.h  |    8 ++------
 virt/kvm/iommu.c            |    1 -
 virt/kvm/kvm_main.c         |   14 ++++++++------
 6 files changed, 14 insertions(+), 21 deletions(-)
arch/powerpc/kvm/e500_tlb.c +0 -1
index c8f6c58..09ce5ac 100644
@@ -524,7 +524,6 @@ static inline void kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500,
 		if (is_error_pfn(pfn)) {
 			printk(KERN_ERR "Couldn't get real page for gfn %lx!\n",
 					(long)gfn);
-			kvm_release_pfn_clean(pfn);
 			return;
 		}
arch/x86/kvm/mmu.c +3 -4
index 924b4e8..d68223f 100644
@@ -2498,7 +2498,9 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 				rmap_recycle(vcpu, sptep, gfn);
 		}
 	}
-	kvm_release_pfn_clean(pfn);
+
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
 }
 
 static void nonpaging_new_cr3(struct kvm_vcpu *vcpu)

@@ -2650,7 +2652,6 @@ static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct *
 
 static int kvm_handle_bad_page(struct kvm_vcpu *vcpu, gfn_t gfn, pfn_t pfn)
 {
-	kvm_release_pfn_clean(pfn);
 	if (pfn == KVM_PFN_ERR_HWPOISON) {
 		kvm_send_hwpoison_signal(gfn_to_hva(vcpu->kvm, gfn), current);
 		return 0;

@@ -3275,8 +3276,6 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 	if (!async)
 		return false; /* *pfn has correct page already */
 
-	kvm_release_pfn_clean(*pfn);
-
 	if (!prefault && can_do_async_pf(vcpu)) {
 		trace_kvm_try_async_get_page(gva, gfn);
 		if (kvm_find_async_pf_gfn(vcpu, gfn)) {
arch/x86/kvm/mmu_audit.c +1 -3
index 7d7d0b9..bac5fa4 100644
@@ -116,10 +116,8 @@ static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 	gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
 	pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
 
-	if (is_error_pfn(pfn)) {
-		kvm_release_pfn_clean(pfn);
+	if (is_error_pfn(pfn))
 		return;
-	}
 
 	hpa =  pfn << PAGE_SHIFT;
 	if ((*sptep & PT64_BASE_ADDR_MASK) != hpa)
arch/x86/kvm/paging_tmpl.h +2 -6
index bb7cf01..bf8c42b 100644
@@ -370,10 +370,8 @@ static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 	pgprintk("%s: gpte %llx spte %p\n", __func__, (u64)gpte, spte);
 	pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte, true);
 	pfn = gfn_to_pfn_atomic(vcpu->kvm, gpte_to_gfn(gpte));
-	if (mmu_invalid_pfn(pfn)) {
-		kvm_release_pfn_clean(pfn);
+	if (mmu_invalid_pfn(pfn))
 		return;
-	}
 
 	/*
 	 * we call mmu_set_spte() with host_writable = true because that

@@ -448,10 +446,8 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
 		gfn = gpte_to_gfn(gpte);
 		pfn = pte_prefetch_gfn_to_pfn(vcpu, gfn,
 				      pte_access & ACC_WRITE_MASK);
-		if (mmu_invalid_pfn(pfn)) {
-			kvm_release_pfn_clean(pfn);
+		if (mmu_invalid_pfn(pfn))
 			break;
-		}
 
 		mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
 			     NULL, PT_PAGE_TABLE_LEVEL, gfn,
virt/kvm/iommu.c +0 -1
index 6a67bea..037cb67 100644
@@ -107,7 +107,6 @@ int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
 		 */
 		pfn = kvm_pin_pages(slot, gfn, page_size);
 		if (is_error_pfn(pfn)) {
-			kvm_release_pfn_clean(pfn);
 			gfn += 1;
 			continue;
 		}
virt/kvm/kvm_main.c +8 -6
index 5873031..bd175f7 100644
@@ -102,9 +102,6 @@ static bool largepages_enabled = true;
 
 bool kvm_is_mmio_pfn(pfn_t pfn)
 {
-	if (is_error_pfn(pfn))
-		return false;
-
 	if (pfn_valid(pfn)) {
 		int reserved;
 		struct page *tail = pfn_to_page(pfn);

@@ -1174,10 +1171,13 @@ EXPORT_SYMBOL_GPL(gfn_to_page_many_atomic);
 
 static struct page *kvm_pfn_to_page(pfn_t pfn)
 {
-	WARN_ON(kvm_is_mmio_pfn(pfn));
+	if (is_error_pfn(pfn))
+		return KVM_ERR_PTR_BAD_PAGE;
 
-	if (is_error_pfn(pfn) || kvm_is_mmio_pfn(pfn))
+	if (kvm_is_mmio_pfn(pfn)) {
+		WARN_ON(1);
 		return KVM_ERR_PTR_BAD_PAGE;
+	}
 
 	return pfn_to_page(pfn);
 }

@@ -1202,7 +1202,9 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean);
 
 void kvm_release_pfn_clean(pfn_t pfn)
 {
-	if (!is_error_pfn(pfn) && !kvm_is_mmio_pfn(pfn))
+	WARN_ON(is_error_pfn(pfn));
+
+	if (!kvm_is_mmio_pfn(pfn))
 		put_page(pfn_to_page(pfn));
 }
 EXPORT_SYMBOL_GPL(kvm_release_pfn_clean);
Signature · 1 line
1.7.7.6
author
#10 lore raw

PATCH v2 9/10 KVM: do not release the error page

In reply to Xiao Guangrong

After commit a2766325cf9f9, the error page is replaced by the error code, it need not be released anymore

[ The patch has been compiling tested for powerpc ]

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/powerpc/kvm/44x_tlb.c   |    1 -
 arch/powerpc/kvm/book3s_pr.c |    4 +---
 arch/x86/kvm/svm.c           |    1 -
 arch/x86/kvm/vmx.c           |    5 ++---
 arch/x86/kvm/x86.c           |    9 +++------
 include/linux/kvm_host.h     |    2 +-
 virt/kvm/async_pf.c          |    4 ++--
 virt/kvm/kvm_main.c          |    5 +++--
 8 files changed, 12 insertions(+), 19 deletions(-)
arch/powerpc/kvm/44x_tlb.c +0 -1
index 33aa715..5dd3ab4 100644
@@ -319,7 +319,6 @@ void kvmppc_mmu_map(struct kvm_vcpu *vcpu, u64 gvaddr, gpa_t gpaddr,
 	if (is_error_page(new_page)) {
 		printk(KERN_ERR "Couldn't get guest page for gfn %llx!\n",
 			(unsigned long long)gfn);
-		kvm_release_page_clean(new_page);
 		return;
 	}
 	hpaddr = page_to_phys(new_page);
arch/powerpc/kvm/book3s_pr.c +1 -3
index a1baec3..05c28f5 100644
@@ -242,10 +242,8 @@ static void kvmppc_patch_dcbz(struct kvm_vcpu *vcpu, struct kvmppc_pte *pte)
 	int i;
 
 	hpage = gfn_to_page(vcpu->kvm, pte->raddr >> PAGE_SHIFT);
-	if (is_error_page(hpage)) {
-		kvm_release_page_clean(hpage);
+	if (is_error_page(hpage))
 		return;
-	}
 
 	hpage_offset = pte->raddr & ~PAGE_MASK;
 	hpage_offset &= ~0xFFFULL;
arch/x86/kvm/svm.c +0 -1
index 687d0c3..31be4a5 100644
@@ -2105,7 +2105,6 @@ static void *nested_svm_map(struct vcpu_svm *svm, u64 gpa, struct page **_page)
 	return kmap(page);
 
 error:
-	kvm_release_page_clean(page);
 	kvm_inject_gp(&svm->vcpu, 0);
 
 	return NULL;
arch/x86/kvm/vmx.c +2 -3
index 2300e53..4b6e809 100644
@@ -596,10 +596,9 @@ static inline struct vmcs12 *get_vmcs12(struct kvm_vcpu *vcpu)
 static struct page *nested_get_page(struct kvm_vcpu *vcpu, gpa_t addr)
 {
 	struct page *page = gfn_to_page(vcpu->kvm, addr >> PAGE_SHIFT);
-	if (is_error_page(page)) {
-		kvm_release_page_clean(page);
+	if (is_error_page(page))
 		return NULL;
-	}
+
 	return page;
 }
arch/x86/kvm/x86.c +3 -6
index b6379e5..3c94d80 100644
@@ -1635,10 +1635,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 		vcpu->arch.time_page =
 				gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);
 
-		if (is_error_page(vcpu->arch.time_page)) {
-			kvm_release_page_clean(vcpu->arch.time_page);
+		if (is_error_page(vcpu->arch.time_page))
 			vcpu->arch.time_page = NULL;
-		}
+
 		break;
 	}
 	case MSR_KVM_ASYNC_PF_EN:

@@ -3941,10 +3940,8 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 		goto emul_write;
 
 	page = gfn_to_page(vcpu->kvm, gpa >> PAGE_SHIFT);
-	if (is_error_page(page)) {
-		kvm_release_page_clean(page);
+	if (is_error_page(page))
 		goto emul_write;
-	}
 
 	kaddr = kmap_atomic(page);
 	kaddr += offset_in_page(gpa);
include/linux/kvm_host.h +1 -1
index 0aebe7a..a8989fc 100644
@@ -458,7 +458,7 @@ pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn);
 pfn_t gfn_to_pfn_prot(struct kvm *kvm, gfn_t gfn, bool write_fault,
 		      bool *writable);
 pfn_t gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn);
-void kvm_release_pfn_dirty(pfn_t);
+void kvm_release_pfn_dirty(pfn_t pfn);
 void kvm_release_pfn_clean(pfn_t pfn);
 void kvm_set_pfn_dirty(pfn_t pfn);
 void kvm_set_pfn_accessed(pfn_t pfn);
virt/kvm/async_pf.c +2 -2
index 56f5533..ea475cd 100644
@@ -111,7 +111,7 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
 			list_entry(vcpu->async_pf.done.next,
 				   typeof(*work), link);
 		list_del(&work->link);
-		if (work->page)
+		if (!is_error_page(work->page))
 			kvm_release_page_clean(work->page);
 		kmem_cache_free(async_pf_cache, work);
 	}

@@ -138,7 +138,7 @@ void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
 
 		list_del(&work->queue);
 		vcpu->async_pf.queued--;
-		if (work->page)
+		if (!is_error_page(work->page))
 			kvm_release_page_clean(work->page);
 		kmem_cache_free(async_pf_cache, work);
 	}
virt/kvm/kvm_main.c +3 -2
index bd175f7..798ba5a 100644
@@ -1195,8 +1195,9 @@ EXPORT_SYMBOL_GPL(gfn_to_page);
 
 void kvm_release_page_clean(struct page *page)
 {
-	if (!is_error_page(page))
-		kvm_release_pfn_clean(page_to_pfn(page));
+	WARN_ON(is_error_page(page));
+
+	kvm_release_pfn_clean(page_to_pfn(page));
 }
 EXPORT_SYMBOL_GPL(kvm_release_page_clean);
Signature · 1 line
1.7.7.6
author
#11 lore raw

PATCH v2 10/10 KVM: let the error pfn not depend on error code

In reply to Xiao Guangrong

Currently, we use the error code as error pfn to indicat the error condition, it is not straightforward and it will not work on PAE 32-bit cpu with huge memory, since the valid physical address can be at most 52 bits

For the normal pfn, the highest 12 bits should be zero, so we can mask these bits to indicate the error.

Signed-off-by Xiao Guangrong <[email protected]>
---
 include/linux/kvm_host.h |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)
include/linux/kvm_host.h +15 -9
index a8989fc..2aaff6e 100644
@@ -49,28 +49,34 @@
 #define KVM_MAX_MMIO_FRAGMENTS \
 	(KVM_MMIO_SIZE / KVM_USER_MMIO_SIZE + KVM_EXTRA_MMIO_FRAGMENTS)
 
-#define KVM_PFN_ERR_FAULT	(-EFAULT)
-#define KVM_PFN_ERR_HWPOISON	(-EHWPOISON)
-#define KVM_PFN_ERR_BAD		(-ENOENT)
+/*
+ * For the normal pfn, the highest 12 bits should be zero,
+ * so we can mask these bits to indicate the error.
+ */
+#define KVM_PFN_ERR_MASK	(0xfffULL << 52)
+
+#define KVM_PFN_ERR_FAULT	(KVM_PFN_ERR_MASK)
+#define KVM_PFN_ERR_HWPOISON	(KVM_PFN_ERR_MASK + 1)
+#define KVM_PFN_ERR_BAD		(KVM_PFN_ERR_MASK + 2)
 
-static inline int is_error_pfn(pfn_t pfn)
+static inline bool is_error_pfn(pfn_t pfn)
 {
-	return IS_ERR_VALUE(pfn);
+	return !!(pfn & KVM_PFN_ERR_MASK);
 }
 
-static inline int is_noslot_pfn(pfn_t pfn)
+static inline bool is_noslot_pfn(pfn_t pfn)
 {
-	return pfn == -ENOENT;
+	return pfn == KVM_PFN_ERR_BAD;
 }
 
-static inline int is_invalid_pfn(pfn_t pfn)
+static inline bool is_invalid_pfn(pfn_t pfn)
 {
 	return !is_noslot_pfn(pfn) && is_error_pfn(pfn);
 }
 
 #define KVM_ERR_PTR_BAD_PAGE	(ERR_PTR(-ENOENT))
 
-static inline int is_error_page(struct page *page)
+static inline bool is_error_page(struct page *page)
 {
 	return IS_ERR(page);
 }
Signature · 1 line
1.7.7.6
#12 lore raw
In reply to Xiao Guangrong
Quoted 9 lines · Xiao Guangrong

There are two bugs:
- the 'error page' is forgot to be released
[ it is unneeded after commit a2766325cf9f9, for backport, we
still do kvm_release_pfn_clean for the error pfn ]

- guest pages are always released regardless of the unmapped page (e,g, caused by hwpoison)

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

Looks good.

#13 lore raw
In reply to Xiao Guangrong
Quoted 8 lines · Xiao Guangrong

There are two bugs:
- the 'error page' is forgot to be released
[ it is unneeded after commit a2766325cf9f9, for backport, we
still do kvm_release_pfn_clean for the error pfn ]

- guest pages are always released regardless of the unmapped page (e,g, caused by hwpoison)

All applied, thanks.

Signature · 1 line
error compiling committee.c: too many arguments to function