[PATCH 1/3] define hwpoison variables static.

[PATCH 1/3] define hwpoison variables static.

[Gleb Natapov]

They are not used outside of the file.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 virt/kvm/kvm_main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a60b6b0..630d122 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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)
 {
-- 
1.7.1

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

[Gleb Natapov]

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 <gleb@redhat.com>
---
 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(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index c515753..a893eb2 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -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)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index e796326..8055067 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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,
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 630d122..bb65d31 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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);
-- 
1.7.1

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

[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 <gleb@redhat.com>
---
 arch/x86/kvm/x86.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7070b41..dd7b241 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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);
 	}
-- 
1.7.1

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

[Avi Kivity]

On 07/07/2010 08:16 PM, Gleb Natapov wrote:
> 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.
>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7070b41..dd7b241 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -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.

>   			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;
>
>    

Code is duplicated.  Helper?

>   		return handle_emulation_failure(vcpu);
>   	}
>    


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

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

[Avi Kivity]

On 07/08/2010 12:06 PM, Avi Kivity wrote:
> On 07/07/2010 08:16 PM, Gleb Natapov wrote:
>> 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.
>>
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 7070b41..dd7b241 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -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().

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

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

[Gleb Natapov]

On Thu, Jul 08, 2010 at 12:15:18PM +0300, Avi Kivity wrote:
> On 07/08/2010 12:06 PM, Avi Kivity wrote:
> >On 07/07/2010 08:16 PM, Gleb Natapov wrote:
> >>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.
> >>
> >>
> >>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >>index 7070b41..dd7b241 100644
> >>--- a/arch/x86/kvm/x86.c
> >>+++ b/arch/x86/kvm/x86.c
> >>@@ -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).
> 
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?

> Say, kvm_make_guest_writeable().
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.

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

[Avi Kivity]

On 07/08/2010 12:17 PM, Gleb Natapov wrote:
>
>> 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.

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

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

[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 <gleb@redhat.com>
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7070b41..6ed3176 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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.

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

[Avi Kivity]

On 07/08/2010 12:41 PM, Gleb Natapov wrote:
> 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.
>
>    

Applied, thanks.

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