define hwpoison variables static.

PATCH In review Gleb Natapov 2 participants 9 messages Jul 7, 2010 – Jul 8, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Gleb Natapov

author

2010-07-07 17:16 UTC #0 lore raw

PATCH 1/3 define hwpoison variables static.

They are not used outside of the file.

Signed-off-by Gleb Natapov <[email protected]>

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

virt/kvm/kvm_main.c +2 -2

index a60b6b0..630d122 100644
@@ -93,8 +93,8 @@ static bool kvm_rebooting;
 
 static bool largepages_enabled = true;
 
-struct page *hwpoison_page;
-pfn_t hwpoison_pfn;
+static struct page *hwpoison_page;
+static pfn_t hwpoison_pfn;
 
 inline int kvm_is_mmio_pfn(pfn_t pfn)
 {

Signature · 1 line

1.7.1

Gleb Natapov

author

2010-07-07 17:16 UTC #1 lore raw

PATCH 2/3 Return EFAULT from kvm ioctl when guest access bad area.

Currently if guest access address that belongs to memory slot but is not backed up by page or page is read only KVM treats it like MMIO access. Remove that capability. It was never part of the interface and should not be relied upon.

Signed-off-by Gleb Natapov <[email protected]>

---
 arch/x86/kvm/mmu.c       |    6 ++++--
 include/linux/kvm_host.h |    1 +
 virt/kvm/kvm_main.c      |   28 ++++++++++++++++++++++++----
 3 files changed, 29 insertions(+), 6 deletions(-)

arch/x86/kvm/mmu.c +4 -2

index c515753..a893eb2 100644
@@ -2040,8 +2040,10 @@ static int kvm_handle_bad_page(struct kvm *kvm, gfn_t gfn, pfn_t pfn)
 	if (is_hwpoison_pfn(pfn)) {
 		kvm_send_hwpoison_signal(kvm, gfn);
 		return 0;
-	}
-	return 1;
+	} else if (is_fault_pfn(pfn))
+		return -EFAULT;
+
+	return 1; 
 }
 
 static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, int write, gfn_t gfn)

include/linux/kvm_host.h +1 -0

index e796326..8055067 100644
@@ -269,6 +269,7 @@ extern pfn_t bad_pfn;
 int is_error_page(struct page *page);
 int is_error_pfn(pfn_t pfn);
 int is_hwpoison_pfn(pfn_t pfn);
+int is_fault_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 +24 -4

index 630d122..bb65d31 100644
@@ -96,6 +96,9 @@ static bool largepages_enabled = true;
 static struct page *hwpoison_page;
 static pfn_t hwpoison_pfn;
 
+static struct page *fault_page;
+static pfn_t fault_pfn;
+
 inline int kvm_is_mmio_pfn(pfn_t pfn)
 {
 	if (pfn_valid(pfn)) {

@@ -815,13 +818,13 @@ EXPORT_SYMBOL_GPL(kvm_disable_largepages);
 
 int is_error_page(struct page *page)
 {
-	return page == bad_page || page == hwpoison_page;
+	return page == bad_page || page == hwpoison_page || page == fault_page;
 }
 EXPORT_SYMBOL_GPL(is_error_page);
 
 int is_error_pfn(pfn_t pfn)
 {
-	return pfn == bad_pfn || pfn == hwpoison_pfn;
+	return pfn == bad_pfn || pfn == hwpoison_pfn || pfn == fault_pfn;
 }
 EXPORT_SYMBOL_GPL(is_error_pfn);

@@ -831,6 +834,12 @@ int is_hwpoison_pfn(pfn_t pfn)
 }
 EXPORT_SYMBOL_GPL(is_hwpoison_pfn);
 
+int is_fault_pfn(pfn_t pfn)
+{
+	return pfn == fault_pfn;
+}
+EXPORT_SYMBOL_GPL(is_fault_pfn);
+
 static inline unsigned long bad_hva(void)
 {
 	return PAGE_OFFSET;

@@ -959,8 +968,8 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr)
 		if (vma == NULL || addr < vma->vm_start ||
 		    !(vma->vm_flags & VM_PFNMAP)) {
 			up_read(&current->mm->mmap_sem);
-			get_page(bad_page);
-			return page_to_pfn(bad_page);
+			get_page(fault_page);
+			return page_to_pfn(fault_page);
 		}
 
 		pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;

@@ -2226,6 +2235,15 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 
 	hwpoison_pfn = page_to_pfn(hwpoison_page);
 
+	fault_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+	
+	if (fault_page == NULL) {
+		r = -ENOMEM;
+		goto out_free_0;
+	}
+
+	fault_pfn = page_to_pfn(fault_page);
+
 	if (!zalloc_cpumask_var(&cpus_hardware_enabled, GFP_KERNEL)) {
 		r = -ENOMEM;
 		goto out_free_0;

@@ -2298,6 +2316,8 @@ out_free_1:
 out_free_0a:
 	free_cpumask_var(cpus_hardware_enabled);
 out_free_0:
+	if (fault_page)
+		__free_page(fault_page);
 	if (hwpoison_page)
 		__free_page(hwpoison_page);
 	__free_page(bad_page);

Signature · 1 line

1.7.1

Gleb Natapov

author

2010-07-07 17:16 UTC #2 lore raw

PATCH 3/3 Reenter guest after instruction emulation failure if emulation was due to access to non-mmio address.

In reply to Gleb Natapov

When shadow pages are in use sometimes KVM try to emulate an instruction when it accesses a shadowed page. If emulation fails KVM un-shadows the page and reenter guest to allow vcpu to execute the instruction. If page is not in shadow page hash KVM assumes that this was attempt to do MMIO and reports emulation failure to userspace since there is no way to fix the situation. This logic has a race though. If two vcpus tries to write to the same shadowed page simultaneously both will enter emulator, but only one of them will find the page in shadow page hash since the one who founds it also removes it from there, so another cpu will report failure to userspace and will abort the guest.

Fix this by checking (in addition to checking shadowed page hash) that page that caused the emulation belongs to valid memory slot. If it is then reenter the guest to allow vcpu to reexecute the instruction.

Signed-off-by Gleb Natapov <[email protected]>

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

arch/x86/kvm/x86.c +4 -0

index 7070b41..dd7b241 100644
@@ -4000,6 +4000,8 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 		if (r)  {
 			if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
 				return EMULATE_DONE;
+			if (!kvm_is_error_hva(gfn_to_hva(vcpu->kvm, cr2)))
+				return EMULATE_DONE;
 			if (emulation_type & EMULTYPE_SKIP)
 				return EMULATE_FAIL;
 			return handle_emulation_failure(vcpu);

@@ -4026,6 +4028,8 @@ restart:
 		 */
 		if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
 			return EMULATE_DONE;
+		if (!kvm_is_error_hva(gfn_to_hva(vcpu->kvm, cr2)))
+			return EMULATE_DONE;
 
 		return handle_emulation_failure(vcpu);
 	}

Signature · 1 line

1.7.1

Avi Kivity

2010-07-08 09:06 UTC #3 lore raw

Re: [PATCH 3/3] Reenter guest after instruction emulation failure if emulation was due to access to non-mmio address.

Quoted 21 lines · Gleb Natapov

When shadow pages are in use sometimes KVM try to emulate an instruction when it accesses a shadowed page. If emulation fails KVM un-shadows the page and reenter guest to allow vcpu to execute the instruction. If page is not in shadow page hash KVM assumes that this was attempt to do MMIO and reports emulation failure to userspace since there is no way to fix the situation. This logic has a race though. If two vcpus tries to write to the same shadowed page simultaneously both will enter emulator, but only one of them will find the page in shadow page hash since the one who founds it also removes it from there, so another cpu will report failure to userspace and will abort the guest.
Fix this by checking (in addition to checking shadowed page hash) that page that caused the emulation belongs to valid memory slot. If it is then reenter the guest to allow vcpu to reexecute the instruction.
arch/x86/kvm/x86.c +0 -0
index 7070b41..dd7b241 100644

arch/x86/kvm/x86.c int emulate_instruction(struct kvm_vcpu *vcpu, · line 4005

Quoted 6 lines

arch/x86/kvm/x86.c +2 -0

@@ -4000,6 +4000,8 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
  		if (r)  {
  			if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
  				return EMULATE_DONE;
+			if (!kvm_is_error_hva(gfn_to_hva(vcpu->kvm, cr2)))
+				return EMULATE_DONE;

cr2 is a gva, not a gfn.

cr2 is not always valid, but I guess that's for a later patch.

arch/x86/kvm/x86.c restart: · line 4034

Quoted 11 lines

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

  			if (emulation_type&  EMULTYPE_SKIP)
  				return EMULATE_FAIL;

  			return handle_emulation_failure(vcpu);

arch/x86/kvm/x86.c +2 -0

@@ -4026,6 +4028,8 @@ restart:
  		 */
  		if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
  			return EMULATE_DONE;
+		if (!kvm_is_error_hva(gfn_to_hva(vcpu->kvm, cr2)))
+			return EMULATE_DONE;

Code is duplicated. Helper?

Quoted 3 lines

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

  		return handle_emulation_failure(vcpu);

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2010-07-08 09:15 UTC #4 lore raw

Re: [PATCH 3/3] Reenter guest after instruction emulation failure if emulation was due to access to non-mmio address.

Quoted 17 lines · Avi Kivity

Quoted 16 lines · Gleb Natapov
When shadow pages are in use sometimes KVM try to emulate an instruction when it accesses a shadowed page. If emulation fails KVM un-shadows the page and reenter guest to allow vcpu to execute the instruction. If page is not in shadow page hash KVM assumes that this was attempt to do MMIO and reports emulation failure to userspace since there is no way to fix the situation. This logic has a race though. If two vcpus tries to write to the same shadowed page simultaneously both will enter emulator, but only one of them will find the page in shadow page hash since the one who founds it also removes it from there, so another cpu will report failure to userspace and will abort the guest.
Fix this by checking (in addition to checking shadowed page hash) that page that caused the emulation belongs to valid memory slot. If it is then reenter the guest to allow vcpu to reexecute the instruction.

Quoted 12 lines

arch/x86/kvm/x86.c int emulate_instruction(struct kvm_vcpu *vcpu, · line 4004

Quoted 10 lines

arch/x86/kvm/x86.c +2 -0

index 7070b41..dd7b241 100644
@@ -4000,6 +4000,8 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
          if (r)  {
              if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
                  return EMULATE_DONE;
+            if (!kvm_is_error_hva(gfn_to_hva(vcpu->kvm, cr2)))
+                return EMULATE_DONE;

cr2 is a gva, not a gfn.

btw, that will mean another page walk, so better fold into kvm_mmu_unprotect_page_virt() (which needs a new name, since it does more than unprotect a page now).

Say, kvm_make_guest_writeable().

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

author

2010-07-08 09:17 UTC #5 lore raw

Re: [PATCH 3/3] Reenter guest after instruction emulation failure if emulation was due to access to non-mmio address.

Quoted 32 lines · Avi Kivity

Quoted 18 lines · Avi Kivity
Quoted 17 lines · Gleb Natapov
When shadow pages are in use sometimes KVM try to emulate an instruction when it accesses a shadowed page. If emulation fails KVM un-shadows the page and reenter guest to allow vcpu to execute the instruction. If page is not in shadow page hash KVM assumes that this was attempt to do MMIO and reports emulation failure to userspace since there is no way to fix the situation. This logic has a race though. If two vcpus tries to write to the same shadowed page simultaneously both will enter emulator, but only one of them will find the page in shadow page hash since the one who
founds it also removes it from there, so another cpu will report failure to userspace and will abort the guest.
Fix this by checking (in addition to checking shadowed page hash) that page that caused the emulation belongs to valid memory slot. If it is then reenter the guest to allow vcpu to reexecute the instruction.
Quoted 12 lines
arch/x86/kvm/x86.c int emulate_instruction(struct kvm_vcpu *vcpu, · line 4004
Quoted 10 lines
arch/x86/kvm/x86.c +2 -0
index 7070b41..dd7b241 100644
@@ -4000,6 +4000,8 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
         if (r)  {
             if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
                 return EMULATE_DONE;
+            if (!kvm_is_error_hva(gfn_to_hva(vcpu->kvm, cr2)))
+                return EMULATE_DONE;
cr2 is a gva, not a gfn.

Quoted 4 lines

btw, that will mean another page walk, so better fold into kvm_mmu_unprotect_page_virt() (which needs a new name, since it does more than unprotect a page now).

But this code will be taken very rarely and usually on the way to failure anyway, do you think additional page walk is a problem?

Quoted 4 lines

Say, kvm_make_guest_writeable().
Signature · 1 line
error compiling committee.c: too many arguments to function

--
Gleb.

Avi Kivity

2010-07-08 09:18 UTC #6 lore raw

Re: [PATCH 3/3] Reenter guest after instruction emulation failure if emulation was due to access to non-mmio address.

Quoted 9 lines · Gleb Natapov

Quoted 6 lines
btw, that will mean another page walk, so better fold into kvm_mmu_unprotect_page_virt() (which needs a new name, since it does more than unprotect a page now).
But this code will be taken very rarely and usually on the way to failure anyway, do you think additional page walk is a problem?

No, you're right.

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

author

2010-07-08 09:41 UTC #7 lore raw

PATCH v2 3/3 Reenter guest after instruction emulation failure if emulation was due to access to non-mmio address.

In reply to Gleb Natapov

Signed-off-by Gleb Natapov <[email protected]>

arch/x86/kvm/x86.c +25 -7

index 7070b41..6ed3176 100644
@@ -3930,6 +3930,29 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)
 	return EMULATE_FAIL;
 }
 
+static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t gva)
+{
+	gpa_t gpa;
+
+	/*
+	 * if emulation was due to access to shadowed page table
+	 * and it failed try to unshadow page and re-entetr the
+	 * guest to let CPU execute the instruction.
+	 */
+	if (kvm_mmu_unprotect_page_virt(vcpu, gva))
+		return true;
+
+	gpa = kvm_mmu_gva_to_gpa_system(vcpu, gva, NULL);
+
+	if (gpa == UNMAPPED_GVA)
+		return true; /* let cpu generate fault */
+
+	if (!kvm_is_error_hva(gfn_to_hva(vcpu->kvm, gpa >> PAGE_SHIFT)))
+		return true;
+
+	return false;
+}
+
 int emulate_instruction(struct kvm_vcpu *vcpu,
 			unsigned long cr2,
 			u16 error_code,

@@ -3998,7 +4021,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 
 		++vcpu->stat.insn_emulation;
 		if (r)  {
-			if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
+			if (reexecute_instruction(vcpu, cr2))
 				return EMULATE_DONE;
 			if (emulation_type & EMULTYPE_SKIP)
 				return EMULATE_FAIL;

@@ -4019,12 +4042,7 @@ restart:
 	r = x86_emulate_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
 
 	if (r) { /* emulation failed */
-		/*
-		 * if emulation was due to access to shadowed page table
-		 * and it failed try to unshadow page and re-entetr the
-		 * guest to let CPU execute the instruction.
-		 */
-		if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
+		if (reexecute_instruction(vcpu, cr2))
 			return EMULATE_DONE;
 
 		return handle_emulation_failure(vcpu);

--
Gleb.

Avi Kivity

Applied

2010-07-08 09:46 UTC #8 lore raw

Re: [PATCH v2 3/3] Reenter guest after instruction emulation failure if emulation was due to access to non-mmio address.

Quoted 11 lines · Gleb Natapov

When shadow pages are in use sometimes KVM try to emulate an instruction when it accesses a shadowed page. If emulation fails KVM un-shadows the page and reenter guest to allow vcpu to execute the instruction. If page is not in shadow page hash KVM assumes that this was attempt to do MMIO and reports emulation failure to userspace since there is no way to fix the situation. This logic has a race though. If two vcpus tries to write to the same shadowed page simultaneously both will enter emulator, but only one of them will find the page in shadow page hash since the one who founds it also removes it from there, so another cpu will report failure to userspace and will abort the guest.

Quoted 5 lines

Fix this by checking (in addition to checking shadowed page hash) that page that caused the emulation belongs to valid memory slot. If it is then reenter the guest to allow vcpu to reexecute the instruction.

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function