[PATCH RFC] KVM: inject #UD if instruction emulation fails while vcpu is in cpl==3

[PATCH RFC] KVM: inject #UD if instruction emulation fails while vcpu is in cpl==3

[Gleb Natapov]

Do not kill VM If instruction emulation fails while vcpu is in
userspace. Inject #UD instead in a hope that guest OS will kill offending
process. Emulation failure is still traced by ftrace point to help
analyze the problem.

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

This patch goes on top of previous patchset.

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 ddfa865..4ddcb1b 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2776,11 +2776,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 889f660..b2eed27 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;
@@ -2297,16 +2297,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 875b785..d05bc46 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3003,7 +3003,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;
@@ -3260,22 +3260,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)
@@ -3487,13 +3472,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 299b602..9be8b84 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3577,24 +3577,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;
@@ -3801,6 +3783,27 @@ 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);
+	if (ctxt->cpl != 3) {
+		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
+		vcpu->run->internal.ndata = 0;
+		return EMULATE_FAIL;
+	} else {
+		/*
+		 * if emulation failed in userspace inject #UD
+		 * instead of killing VM
+		 */
+		kvm_queue_exception(vcpu, UD_VECTOR);
+		return EMULATE_DONE;
+	}
+}
+
 int emulate_instruction(struct kvm_vcpu *vcpu,
 			unsigned long cr2,
 			u16 error_code,
@@ -3869,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);
 		}
 	}
 
@@ -3898,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);
@@ -4734,7 +4735,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: [PATCH RFC] KVM: inject #UD if instruction emulation fails while vcpu is in cpl==3

[Avi Kivity]

On 04/29/2010 02:58 PM, Gleb Natapov wrote:
> Do not kill VM If instruction emulation fails while vcpu is in
> userspace. Inject #UD instead in a hope that guest OS will kill offending
> process. Emulation failure is still traced by ftrace point to help
> analyze the problem.
>    

Still there's the risk here that a critical failure goes unnoticed.  
ftrace isn't on at all times.

We can probably inject a #UD unconditionally and exit to userspace.  
Userspace would then report the problem to the user and reenter the 
guest, which would then recover or not.

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

Re: [PATCH RFC] KVM: inject #UD if instruction emulation fails while vcpu is in cpl==3

[Gleb Natapov]

On Thu, May 06, 2010 at 12:15:58PM +0300, Avi Kivity wrote:
> On 04/29/2010 02:58 PM, Gleb Natapov wrote:
> >Do not kill VM If instruction emulation fails while vcpu is in
> >userspace. Inject #UD instead in a hope that guest OS will kill offending
> >process. Emulation failure is still traced by ftrace point to help
> >analyze the problem.
> 
> Still there's the risk here that a critical failure goes unnoticed.
> ftrace isn't on at all times.
> 
Kvm_stat will still show that there was emulation failure, so if strange
application behaviour is reported kvm_stat output will have hints where
to look. Next step in analyzing the problem will be enabling emulator
tracing.

> We can probably inject a #UD unconditionally and exit to userspace.
> Userspace would then report the problem to the user and reenter the
> guest, which would then recover or not.
> 
By "unconditionally" you mean even if guest is in kernel mode? There is
no point in trying to continue after that happens. Instead of getting
paused VM at exact place where problem happened and easily analyzable we
will get misbehaved VM with undefined state.

--
			Gleb.

Re: [PATCH RFC] KVM: inject #UD if instruction emulation fails while vcpu is in cpl==3

[Avi Kivity]

On 05/06/2010 01:06 PM, Gleb Natapov wrote:
> On Thu, May 06, 2010 at 12:15:58PM +0300, Avi Kivity wrote:
>    
>> On 04/29/2010 02:58 PM, Gleb Natapov wrote:
>>      
>>> Do not kill VM If instruction emulation fails while vcpu is in
>>> userspace. Inject #UD instead in a hope that guest OS will kill offending
>>> process. Emulation failure is still traced by ftrace point to help
>>> analyze the problem.
>>>        
>> Still there's the risk here that a critical failure goes unnoticed.
>> ftrace isn't on at all times.
>>
>>      
> Kvm_stat will still show that there was emulation failure, so if strange
> application behaviour is reported kvm_stat output will have hints where
> to look. Next step in analyzing the problem will be enabling emulator
> tracing.
>    

We can expect that from a developer or a user subscribed to kvm@.  But 
what about some random user running virt-manager?

dmesg and kvm_stat will not go up the management stack.

>> We can probably inject a #UD unconditionally and exit to userspace.
>> Userspace would then report the problem to the user and reenter the
>> guest, which would then recover or not.
>>
>>      
> By "unconditionally" you mean even if guest is in kernel mode?

Yes.

> There is
> no point in trying to continue after that happens. Instead of getting
> paused VM at exact place where problem happened and easily analyzable we
> will get misbehaved VM with undefined state.
>    

True.  But the same problem exists with cpl>0 #UD.  It may be a critical 
driver in userspace (say, video driver).

Also need to think consider nested kernels (which are userspace for this 
purpose).

How about default to unconditional #UD and report, and pause if 
requested (in userspace)?  Usually emulation failures will be 100% 
reproducible, so the user can rerun their workload.

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

Re: [PATCH RFC] KVM: inject #UD if instruction emulation fails while vcpu is in cpl==3

[Gleb Natapov]

On Thu, May 06, 2010 at 01:13:30PM +0300, Avi Kivity wrote:
> On 05/06/2010 01:06 PM, Gleb Natapov wrote:
> >On Thu, May 06, 2010 at 12:15:58PM +0300, Avi Kivity wrote:
> >>On 04/29/2010 02:58 PM, Gleb Natapov wrote:
> >>>Do not kill VM If instruction emulation fails while vcpu is in
> >>>userspace. Inject #UD instead in a hope that guest OS will kill offending
> >>>process. Emulation failure is still traced by ftrace point to help
> >>>analyze the problem.
> >>Still there's the risk here that a critical failure goes unnoticed.
> >>ftrace isn't on at all times.
> >>
> >Kvm_stat will still show that there was emulation failure, so if strange
> >application behaviour is reported kvm_stat output will have hints where
> >to look. Next step in analyzing the problem will be enabling emulator
> >tracing.
> 
> We can expect that from a developer or a user subscribed to kvm@.
> But what about some random user running virt-manager?
> 
What virt-manager would do about such error?

> dmesg and kvm_stat will not go up the management stack.
> 
> >>We can probably inject a #UD unconditionally and exit to userspace.
> >>Userspace would then report the problem to the user and reenter the
> >>guest, which would then recover or not.
> >>
> >By "unconditionally" you mean even if guest is in kernel mode?
> 
> Yes.
> 
> >There is
> >no point in trying to continue after that happens. Instead of getting
> >paused VM at exact place where problem happened and easily analyzable we
> >will get misbehaved VM with undefined state.
> 
> True.  But the same problem exists with cpl>0 #UD.  It may be a
> critical driver in userspace (say, video driver).
> 
> Also need to think consider nested kernels (which are userspace for
> this purpose).
Ugh, we can check if vcpu is in nested mode.

> 
> How about default to unconditional #UD and report, and pause if
> requested (in userspace)?  Usually emulation failures will be 100%
> reproducible, so the user can rerun their workload.
> 
Didn't what to involve userspace in this, but it can be done of course.
 
--
			Gleb.

Re: [PATCH RFC] KVM: inject #UD if instruction emulation fails while vcpu is in cpl==3

[Avi Kivity]

On 05/06/2010 03:25 PM, Gleb Natapov wrote:
>
>> We can expect that from a developer or a user subscribed to kvm@.
>> But what about some random user running virt-manager?
>>
>>      
> What virt-manager would do about such error?
>    

Call up abrt.

>> True.  But the same problem exists with cpl>0 #UD.  It may be a
>> critical driver in userspace (say, video driver).
>>
>> Also need to think consider nested kernels (which are userspace for
>> this purpose).
>>      
> Ugh, we can check if vcpu is in nested mode.
>    

And do what?  Inject #UD to the guest?  Or force some vmexit?

>> How about default to unconditional #UD and report, and pause if
>> requested (in userspace)?  Usually emulation failures will be 100%
>> reproducible, so the user can rerun their workload.
>>
>>      
> Didn't what to involve userspace in this, but it can be done of course.
>    

Whenever we have to make a decision, we involve userspace.

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

Re: [PATCH RFC] KVM: inject #UD if instruction emulation fails while vcpu is in cpl==3

[Gleb Natapov]

On Thu, May 06, 2010 at 03:33:12PM +0300, Avi Kivity wrote:
> On 05/06/2010 03:25 PM, Gleb Natapov wrote:
> >
> >>We can expect that from a developer or a user subscribed to kvm@.
> >>But what about some random user running virt-manager?
> >>
> >What virt-manager would do about such error?
> 
> Call up abrt.
> 
The idea is not to let userspace process running in a VM kill the VM.

> >>True.  But the same problem exists with cpl>0 #UD.  It may be a
> >>critical driver in userspace (say, video driver).
> >>
> >>Also need to think consider nested kernels (which are userspace for
> >>this purpose).
> >Ugh, we can check if vcpu is in nested mode.
> 
> And do what?  Inject #UD to the guest?  Or force some vmexit?
> 
Does host emulator will ever run on behalf of nested guest? We have
emulator inside nested guest for this.

> >>How about default to unconditional #UD and report, and pause if
> >>requested (in userspace)?  Usually emulation failures will be 100%
> >>reproducible, so the user can rerun their workload.
> >>
> >Didn't what to involve userspace in this, but it can be done of course.
> 
> Whenever we have to make a decision, we involve userspace.
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.

Re: [PATCH RFC] KVM: inject #UD if instruction emulation fails while vcpu is in cpl==3

[Avi Kivity]

On 05/06/2010 03:41 PM, Gleb Natapov wrote:
> On Thu, May 06, 2010 at 03:33:12PM +0300, Avi Kivity wrote:
>    
>> On 05/06/2010 03:25 PM, Gleb Natapov wrote:
>>      
>>>        
>>>> We can expect that from a developer or a user subscribed to kvm@.
>>>> But what about some random user running virt-manager?
>>>>
>>>>          
>>> What virt-manager would do about such error?
>>>        
>> Call up abrt.
>>
>>      
> The idea is not to let userspace process running in a VM kill the VM.
>    

Well, log the problem (including registers and instruction code), and 
continue with the #UD.

>>>> True.  But the same problem exists with cpl>0 #UD.  It may be a
>>>> critical driver in userspace (say, video driver).
>>>>
>>>> Also need to think consider nested kernels (which are userspace for
>>>> this purpose).
>>>>          
>>> Ugh, we can check if vcpu is in nested mode.
>>>        
>> And do what?  Inject #UD to the guest?  Or force some vmexit?
>>
>>      
> Does host emulator will ever run on behalf of nested guest? We have
> emulator inside nested guest for this.
>    

If the guest doesn't map the page, it will emulate.  If it does map the 
page, and the host doesn't, the host emulates.

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