[PATCHv2] KVM: inject #UD if instruction emulation fails and exit to userspace

[PATCHv2] KVM: inject #UD if instruction emulation fails and exit to userspace

[Gleb Natapov]

Do not kill VM when instruction emulation fails. Inject #UD and report
failure to userspace instead. Userspace may choose to reenter guest if
vcpu is in userspace (cpl == 3) in which case guest OS will kill
offending process and continue running.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---

Changelog:
  v1->v2:
    - always inject #UD and report failure to userspace.

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ed48904..5aa0944 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -575,7 +575,6 @@ enum emulation_result {
 #define EMULTYPE_SKIP		    (1 << 2)
 int emulate_instruction(struct kvm_vcpu *vcpu,
 			unsigned long cr2, u16 error_code, int emulation_type);
-void kvm_report_emulation_failure(struct kvm_vcpu *cvpu, const char *context);
 void realmode_lgdt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
 void realmode_lidt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
 
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 95bee9d..41d2de8 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2788,11 +2788,8 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code)
 		return 1;
 	case EMULATE_DO_MMIO:
 		++vcpu->stat.mmio_exits;
-		return 0;
+		/* fall through */
 	case EMULATE_FAIL:
-		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
-		vcpu->run->internal.ndata = 0;
 		return 0;
 	default:
 		BUG();
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index cea916f..58c91f5 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1449,7 +1449,7 @@ static int io_interception(struct vcpu_svm *svm)
 	string = (io_info & SVM_IOIO_STR_MASK) != 0;
 	in = (io_info & SVM_IOIO_TYPE_MASK) != 0;
 	if (string || in)
-		return !(emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DO_MMIO);
+		return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
 
 	port = io_info >> 16;
 	size = (io_info & SVM_IOIO_SIZE_MASK) >> SVM_IOIO_SIZE_SHIFT;
@@ -2300,16 +2300,12 @@ static int iret_interception(struct vcpu_svm *svm)
 
 static int invlpg_interception(struct vcpu_svm *svm)
 {
-	if (emulate_instruction(&svm->vcpu, 0, 0, 0) != EMULATE_DONE)
-		pr_unimpl(&svm->vcpu, "%s: failed\n", __func__);
-	return 1;
+	return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
 }
 
 static int emulate_on_interception(struct vcpu_svm *svm)
 {
-	if (emulate_instruction(&svm->vcpu, 0, 0, 0) != EMULATE_DONE)
-		pr_unimpl(&svm->vcpu, "%s: failed\n", __func__);
-	return 1;
+	return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
 }
 
 static int cr8_write_interception(struct vcpu_svm *svm)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 777e00d..e35c479 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3074,7 +3074,7 @@ static int handle_io(struct kvm_vcpu *vcpu)
 	++vcpu->stat.io_exits;
 
 	if (string || in)
-		return !(emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DO_MMIO);
+		return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
 
 	port = exit_qualification >> 16;
 	size = (exit_qualification & 7) + 1;
@@ -3331,22 +3331,7 @@ static int handle_wbinvd(struct kvm_vcpu *vcpu)
 
 static int handle_apic_access(struct kvm_vcpu *vcpu)
 {
-	unsigned long exit_qualification;
-	enum emulation_result er;
-	unsigned long offset;
-
-	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
-	offset = exit_qualification & 0xffful;
-
-	er = emulate_instruction(vcpu, 0, 0, 0);
-
-	if (er !=  EMULATE_DONE) {
-		printk(KERN_ERR
-		       "Fail to handle apic access vmexit! Offset is 0x%lx\n",
-		       offset);
-		return -ENOEXEC;
-	}
-	return 1;
+	return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
 }
 
 static int handle_task_switch(struct kvm_vcpu *vcpu)
@@ -3558,13 +3543,8 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
 			goto out;
 		}
 
-		if (err != EMULATE_DONE) {
-			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-			vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
-			vcpu->run->internal.ndata = 0;
-			ret = 0;
-			goto out;
-		}
+		if (err != EMULATE_DONE)
+			return 0;
 
 		if (signal_pending(current))
 			goto out;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cd8a606..b5eaed4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3587,24 +3587,6 @@ int emulator_set_dr(int dr, unsigned long value, struct kvm_vcpu *vcpu)
 	return __kvm_set_dr(vcpu, dr, value);
 }
 
-void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char *context)
-{
-	u8 opcodes[4];
-	unsigned long rip = kvm_rip_read(vcpu);
-	unsigned long rip_linear;
-
-	if (!printk_ratelimit())
-		return;
-
-	rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
-
-	kvm_read_guest_virt(rip_linear, (void *)opcodes, 4, vcpu, NULL);
-
-	printk(KERN_ERR "emulation failed (%s) rip %lx %02x %02x %02x %02x\n",
-	       context, rip, opcodes[0], opcodes[1], opcodes[2], opcodes[3]);
-}
-EXPORT_SYMBOL_GPL(kvm_report_emulation_failure);
-
 static u64 mk_cr_64(u64 curr_cr, u32 new_val)
 {
 	return (curr_cr & ~((1ULL << 32) - 1)) | new_val;
@@ -3811,6 +3793,19 @@ static void inject_emulated_exception(struct kvm_vcpu *vcpu)
 		kvm_queue_exception(vcpu, ctxt->exception);
 }
 
+static int handle_emulation_failure(struct kvm_vcpu *vcpu)
+{
+	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
+
+	++vcpu->stat.insn_emulation_fail;
+	trace_kvm_emulate_insn_failed(vcpu);
+	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
+	vcpu->run->internal.ndata = 0;
+	kvm_queue_exception(vcpu, UD_VECTOR);
+	return EMULATE_FAIL;
+}
+
 int emulate_instruction(struct kvm_vcpu *vcpu,
 			unsigned long cr2,
 			u16 error_code,
@@ -3879,11 +3874,11 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 
 		++vcpu->stat.insn_emulation;
 		if (r)  {
-			++vcpu->stat.insn_emulation_fail;
-			trace_kvm_emulate_insn_failed(vcpu);
 			if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
 				return EMULATE_DONE;
-			return EMULATE_FAIL;
+			if (emulation_type & EMULTYPE_SKIP)
+				return EMULATE_FAIL;
+			return handle_emulation_failure(vcpu);
 		}
 	}
 
@@ -3908,9 +3903,7 @@ restart:
 		if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
 			return EMULATE_DONE;
 
-		trace_kvm_emulate_insn_failed(vcpu);
-		kvm_report_emulation_failure(vcpu, "mmio");
-		return EMULATE_FAIL;
+		return handle_emulation_failure(vcpu);
 	}
 
 	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
@@ -4746,7 +4739,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 		r = emulate_instruction(vcpu, 0, 0, EMULTYPE_NO_DECODE);
 		srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
-		if (r == EMULATE_DO_MMIO) {
+		if (r != EMULATE_DONE) {
 			r = 0;
 			goto out;
 		}
--
			Gleb.

Re: [PATCHv2] KVM: inject #UD if instruction emulation fails and exit to userspace

[Gleb Natapov]

On Mon, May 10, 2010 at 11:16:56AM +0300, Gleb Natapov wrote:
> Do not kill VM when instruction emulation fails. Inject #UD and report
> failure to userspace instead. Userspace may choose to reenter guest if
> vcpu is in userspace (cpl == 3) in which case guest OS will kill
> offending process and continue running.
> 
Please use this one instead. Compilation warning fixed.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ed48904..5aa0944 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -575,7 +575,6 @@ enum emulation_result {
 #define EMULTYPE_SKIP		    (1 << 2)
 int emulate_instruction(struct kvm_vcpu *vcpu,
 			unsigned long cr2, u16 error_code, int emulation_type);
-void kvm_report_emulation_failure(struct kvm_vcpu *cvpu, const char *context);
 void realmode_lgdt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
 void realmode_lidt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
 
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 95bee9d..41d2de8 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2788,11 +2788,8 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code)
 		return 1;
 	case EMULATE_DO_MMIO:
 		++vcpu->stat.mmio_exits;
-		return 0;
+		/* fall through */
 	case EMULATE_FAIL:
-		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
-		vcpu->run->internal.ndata = 0;
 		return 0;
 	default:
 		BUG();
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index cea916f..58c91f5 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1449,7 +1449,7 @@ static int io_interception(struct vcpu_svm *svm)
 	string = (io_info & SVM_IOIO_STR_MASK) != 0;
 	in = (io_info & SVM_IOIO_TYPE_MASK) != 0;
 	if (string || in)
-		return !(emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DO_MMIO);
+		return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
 
 	port = io_info >> 16;
 	size = (io_info & SVM_IOIO_SIZE_MASK) >> SVM_IOIO_SIZE_SHIFT;
@@ -2300,16 +2300,12 @@ static int iret_interception(struct vcpu_svm *svm)
 
 static int invlpg_interception(struct vcpu_svm *svm)
 {
-	if (emulate_instruction(&svm->vcpu, 0, 0, 0) != EMULATE_DONE)
-		pr_unimpl(&svm->vcpu, "%s: failed\n", __func__);
-	return 1;
+	return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
 }
 
 static int emulate_on_interception(struct vcpu_svm *svm)
 {
-	if (emulate_instruction(&svm->vcpu, 0, 0, 0) != EMULATE_DONE)
-		pr_unimpl(&svm->vcpu, "%s: failed\n", __func__);
-	return 1;
+	return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
 }
 
 static int cr8_write_interception(struct vcpu_svm *svm)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 777e00d..e35c479 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3074,7 +3074,7 @@ static int handle_io(struct kvm_vcpu *vcpu)
 	++vcpu->stat.io_exits;
 
 	if (string || in)
-		return !(emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DO_MMIO);
+		return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
 
 	port = exit_qualification >> 16;
 	size = (exit_qualification & 7) + 1;
@@ -3331,22 +3331,7 @@ static int handle_wbinvd(struct kvm_vcpu *vcpu)
 
 static int handle_apic_access(struct kvm_vcpu *vcpu)
 {
-	unsigned long exit_qualification;
-	enum emulation_result er;
-	unsigned long offset;
-
-	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
-	offset = exit_qualification & 0xffful;
-
-	er = emulate_instruction(vcpu, 0, 0, 0);
-
-	if (er !=  EMULATE_DONE) {
-		printk(KERN_ERR
-		       "Fail to handle apic access vmexit! Offset is 0x%lx\n",
-		       offset);
-		return -ENOEXEC;
-	}
-	return 1;
+	return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
 }
 
 static int handle_task_switch(struct kvm_vcpu *vcpu)
@@ -3558,13 +3543,8 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
 			goto out;
 		}
 
-		if (err != EMULATE_DONE) {
-			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-			vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
-			vcpu->run->internal.ndata = 0;
-			ret = 0;
-			goto out;
-		}
+		if (err != EMULATE_DONE)
+			return 0;
 
 		if (signal_pending(current))
 			goto out;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cd8a606..eb845cf 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3587,24 +3587,6 @@ int emulator_set_dr(int dr, unsigned long value, struct kvm_vcpu *vcpu)
 	return __kvm_set_dr(vcpu, dr, value);
 }
 
-void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char *context)
-{
-	u8 opcodes[4];
-	unsigned long rip = kvm_rip_read(vcpu);
-	unsigned long rip_linear;
-
-	if (!printk_ratelimit())
-		return;
-
-	rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
-
-	kvm_read_guest_virt(rip_linear, (void *)opcodes, 4, vcpu, NULL);
-
-	printk(KERN_ERR "emulation failed (%s) rip %lx %02x %02x %02x %02x\n",
-	       context, rip, opcodes[0], opcodes[1], opcodes[2], opcodes[3]);
-}
-EXPORT_SYMBOL_GPL(kvm_report_emulation_failure);
-
 static u64 mk_cr_64(u64 curr_cr, u32 new_val)
 {
 	return (curr_cr & ~((1ULL << 32) - 1)) | new_val;
@@ -3811,6 +3793,17 @@ static void inject_emulated_exception(struct kvm_vcpu *vcpu)
 		kvm_queue_exception(vcpu, ctxt->exception);
 }
 
+static int handle_emulation_failure(struct kvm_vcpu *vcpu)
+{
+	++vcpu->stat.insn_emulation_fail;
+	trace_kvm_emulate_insn_failed(vcpu);
+	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
+	vcpu->run->internal.ndata = 0;
+	kvm_queue_exception(vcpu, UD_VECTOR);
+	return EMULATE_FAIL;
+}
+
 int emulate_instruction(struct kvm_vcpu *vcpu,
 			unsigned long cr2,
 			u16 error_code,
@@ -3879,11 +3872,11 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 
 		++vcpu->stat.insn_emulation;
 		if (r)  {
-			++vcpu->stat.insn_emulation_fail;
-			trace_kvm_emulate_insn_failed(vcpu);
 			if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
 				return EMULATE_DONE;
-			return EMULATE_FAIL;
+			if (emulation_type & EMULTYPE_SKIP)
+				return EMULATE_FAIL;
+			return handle_emulation_failure(vcpu);
 		}
 	}
 
@@ -3908,9 +3901,7 @@ restart:
 		if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
 			return EMULATE_DONE;
 
-		trace_kvm_emulate_insn_failed(vcpu);
-		kvm_report_emulation_failure(vcpu, "mmio");
-		return EMULATE_FAIL;
+		return handle_emulation_failure(vcpu);
 	}
 
 	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
@@ -4746,7 +4737,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 		r = emulate_instruction(vcpu, 0, 0, EMULTYPE_NO_DECODE);
 		srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
-		if (r == EMULATE_DO_MMIO) {
+		if (r != EMULATE_DONE) {
 			r = 0;
 			goto out;
 		}
--
			Gleb.

Re: [PATCHv2] KVM: inject #UD if instruction emulation fails and exit to userspace

[Mohammed Gamal]

On Mon, May 10, 2010 at 1:25 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Mon, May 10, 2010 at 11:16:56AM +0300, Gleb Natapov wrote:
>> Do not kill VM when instruction emulation fails. Inject #UD and report
>> failure to userspace instead. Userspace may choose to reenter guest if
>> vcpu is in userspace (cpl == 3) in which case guest OS will kill
>> offending process and continue running.
>>

I am curious to know what'd happen in case the vcpu is in kernel space
(cpl == 0). Is that case handled?

> Please use this one instead. Compilation warning fixed.
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index ed48904..5aa0944 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -575,7 +575,6 @@ enum emulation_result {
>  #define EMULTYPE_SKIP              (1 << 2)
>  int emulate_instruction(struct kvm_vcpu *vcpu,
>                        unsigned long cr2, u16 error_code, int emulation_type);
> -void kvm_report_emulation_failure(struct kvm_vcpu *cvpu, const char *context);
>  void realmode_lgdt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
>  void realmode_lidt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 95bee9d..41d2de8 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2788,11 +2788,8 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code)
>                return 1;
>        case EMULATE_DO_MMIO:
>                ++vcpu->stat.mmio_exits;
> -               return 0;
> +               /* fall through */
>        case EMULATE_FAIL:
> -               vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -               vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> -               vcpu->run->internal.ndata = 0;
>                return 0;
>        default:
>                BUG();
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index cea916f..58c91f5 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1449,7 +1449,7 @@ static int io_interception(struct vcpu_svm *svm)
>        string = (io_info & SVM_IOIO_STR_MASK) != 0;
>        in = (io_info & SVM_IOIO_TYPE_MASK) != 0;
>        if (string || in)
> -               return !(emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DO_MMIO);
> +               return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
>
>        port = io_info >> 16;
>        size = (io_info & SVM_IOIO_SIZE_MASK) >> SVM_IOIO_SIZE_SHIFT;
> @@ -2300,16 +2300,12 @@ static int iret_interception(struct vcpu_svm *svm)
>
>  static int invlpg_interception(struct vcpu_svm *svm)
>  {
> -       if (emulate_instruction(&svm->vcpu, 0, 0, 0) != EMULATE_DONE)
> -               pr_unimpl(&svm->vcpu, "%s: failed\n", __func__);
> -       return 1;
> +       return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
>  }
>
>  static int emulate_on_interception(struct vcpu_svm *svm)
>  {
> -       if (emulate_instruction(&svm->vcpu, 0, 0, 0) != EMULATE_DONE)
> -               pr_unimpl(&svm->vcpu, "%s: failed\n", __func__);
> -       return 1;
> +       return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
>  }
>
>  static int cr8_write_interception(struct vcpu_svm *svm)
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 777e00d..e35c479 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3074,7 +3074,7 @@ static int handle_io(struct kvm_vcpu *vcpu)
>        ++vcpu->stat.io_exits;
>
>        if (string || in)
> -               return !(emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DO_MMIO);
> +               return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
>
>        port = exit_qualification >> 16;
>        size = (exit_qualification & 7) + 1;
> @@ -3331,22 +3331,7 @@ static int handle_wbinvd(struct kvm_vcpu *vcpu)
>
>  static int handle_apic_access(struct kvm_vcpu *vcpu)
>  {
> -       unsigned long exit_qualification;
> -       enum emulation_result er;
> -       unsigned long offset;
> -
> -       exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> -       offset = exit_qualification & 0xffful;
> -
> -       er = emulate_instruction(vcpu, 0, 0, 0);
> -
> -       if (er !=  EMULATE_DONE) {
> -               printk(KERN_ERR
> -                      "Fail to handle apic access vmexit! Offset is 0x%lx\n",
> -                      offset);
> -               return -ENOEXEC;
> -       }
> -       return 1;
> +       return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
>  }
>
>  static int handle_task_switch(struct kvm_vcpu *vcpu)
> @@ -3558,13 +3543,8 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
>                        goto out;
>                }
>
> -               if (err != EMULATE_DONE) {
> -                       vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -                       vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> -                       vcpu->run->internal.ndata = 0;
> -                       ret = 0;
> -                       goto out;
> -               }
> +               if (err != EMULATE_DONE)
> +                       return 0;
>
>                if (signal_pending(current))
>                        goto out;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cd8a606..eb845cf 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3587,24 +3587,6 @@ int emulator_set_dr(int dr, unsigned long value, struct kvm_vcpu *vcpu)
>        return __kvm_set_dr(vcpu, dr, value);
>  }
>
> -void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char *context)
> -{
> -       u8 opcodes[4];
> -       unsigned long rip = kvm_rip_read(vcpu);
> -       unsigned long rip_linear;
> -
> -       if (!printk_ratelimit())
> -               return;
> -
> -       rip_linear = rip + get_segment_base(vcpu, VCPU_SREG_CS);
> -
> -       kvm_read_guest_virt(rip_linear, (void *)opcodes, 4, vcpu, NULL);
> -
> -       printk(KERN_ERR "emulation failed (%s) rip %lx %02x %02x %02x %02x\n",
> -              context, rip, opcodes[0], opcodes[1], opcodes[2], opcodes[3]);
> -}
> -EXPORT_SYMBOL_GPL(kvm_report_emulation_failure);
> -
>  static u64 mk_cr_64(u64 curr_cr, u32 new_val)
>  {
>        return (curr_cr & ~((1ULL << 32) - 1)) | new_val;
> @@ -3811,6 +3793,17 @@ static void inject_emulated_exception(struct kvm_vcpu *vcpu)
>                kvm_queue_exception(vcpu, ctxt->exception);
>  }
>
> +static int handle_emulation_failure(struct kvm_vcpu *vcpu)
> +{
> +       ++vcpu->stat.insn_emulation_fail;
> +       trace_kvm_emulate_insn_failed(vcpu);
> +       vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +       vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> +       vcpu->run->internal.ndata = 0;
> +       kvm_queue_exception(vcpu, UD_VECTOR);
> +       return EMULATE_FAIL;
> +}
> +
>  int emulate_instruction(struct kvm_vcpu *vcpu,
>                        unsigned long cr2,
>                        u16 error_code,
> @@ -3879,11 +3872,11 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>
>                ++vcpu->stat.insn_emulation;
>                if (r)  {
> -                       ++vcpu->stat.insn_emulation_fail;
> -                       trace_kvm_emulate_insn_failed(vcpu);
>                        if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
>                                return EMULATE_DONE;
> -                       return EMULATE_FAIL;
> +                       if (emulation_type & EMULTYPE_SKIP)
> +                               return EMULATE_FAIL;
> +                       return handle_emulation_failure(vcpu);
>                }
>        }
>
> @@ -3908,9 +3901,7 @@ restart:
>                if (kvm_mmu_unprotect_page_virt(vcpu, cr2))
>                        return EMULATE_DONE;
>
> -               trace_kvm_emulate_insn_failed(vcpu);
> -               kvm_report_emulation_failure(vcpu, "mmio");
> -               return EMULATE_FAIL;
> +               return handle_emulation_failure(vcpu);
>        }
>
>        toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
> @@ -4746,7 +4737,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>                vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>                r = emulate_instruction(vcpu, 0, 0, EMULTYPE_NO_DECODE);
>                srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> -               if (r == EMULATE_DO_MMIO) {
> +               if (r != EMULATE_DONE) {
>                        r = 0;
>                        goto out;
>                }
> --
>                        Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCHv2] KVM: inject #UD if instruction emulation fails and exit to userspace

[Gleb Natapov]

On Mon, May 10, 2010 at 07:06:05PM +0300, Mohammed Gamal wrote:
> On Mon, May 10, 2010 at 1:25 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Mon, May 10, 2010 at 11:16:56AM +0300, Gleb Natapov wrote:
> >> Do not kill VM when instruction emulation fails. Inject #UD and report
> >> failure to userspace instead. Userspace may choose to reenter guest if
> >> vcpu is in userspace (cpl == 3) in which case guest OS will kill
> >> offending process and continue running.
> >>
> 
> I am curious to know what'd happen in case the vcpu is in kernel space
> (cpl == 0). Is that case handled?
> 
Currently no matter where emulation fails VM is stopped and cpu state is
printed on stderr. After that patch userspace may choose to continue VM
execution after emulation error (#UD will be injected into VM though). The
policy is in userspace, but I don't see the point to continue execution
after emulation failed in kernel. How kernel can recover from the #UD?

--
			Gleb.

Re: [PATCHv2] KVM: inject #UD if instruction emulation fails and exit to userspace

[Takuya Yoshikawa]

(2010/05/11 2:33), Gleb Natapov wrote:
> On Mon, May 10, 2010 at 07:06:05PM +0300, Mohammed Gamal wrote:
>> On Mon, May 10, 2010 at 1:25 PM, Gleb Natapov<gleb@redhat.com>  wrote:
>>> On Mon, May 10, 2010 at 11:16:56AM +0300, Gleb Natapov wrote:
>>>> Do not kill VM when instruction emulation fails. Inject #UD and report
>>>> failure to userspace instead. Userspace may choose to reenter guest if
>>>> vcpu is in userspace (cpl == 3) in which case guest OS will kill
>>>> offending process and continue running.
>>>>
>>
>> I am curious to know what'd happen in case the vcpu is in kernel space
>> (cpl == 0). Is that case handled?
>>
> Currently no matter where emulation fails VM is stopped and cpu state is
> printed on stderr. After that patch userspace may choose to continue VM
> execution after emulation error (#UD will be injected into VM though). The
> policy is in userspace, but I don't see the point to continue execution
> after emulation failed in kernel. How kernel can recover from the #UD?

I don't see what is the recommended(possible) way of trouble shooting yet.

If the user is managing both the guest and the host, it's simple: no worth
reentering the guest. The user will just see the stderr.

But what about if the user can only see the guest?

In the case of non-virt, usually the user sees oops log or something and calls
to a support staff. Compared to that, if VM is silently stopped, what information
can the user see? In such a case, catching up the trouble and calling to a support
staff is host side management staff's job?

If you give us preferred way of trouble shooting of KVM, from the developer's point
of view, it will really help us to prepare for the future use of KVM.

   What we can do will depend on your development!
   And this is one of the reasons why I'm interested in the x86's emulation
   development. :-)

Thanks,
   Takuya


>
> --
> 			Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] KVM: inject #UD if instruction emulation fails and exit to userspace

[Marcelo Tosatti]

On Mon, May 10, 2010 at 11:16:56AM +0300, Gleb Natapov wrote:
> Do not kill VM when instruction emulation fails. Inject #UD and report
> failure to userspace instead. Userspace may choose to reenter guest if
> vcpu is in userspace (cpl == 3) in which case guest OS will kill
> offending process and continue running.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
> 
> Changelog:
>   v1->v2:
>     - always inject #UD and report failure to userspace.

Applied, thanks.