From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mohammed Gamal <m.gamal005@gmail.com>
Subject: Re: [PATCHv2] KVM: inject #UD if instruction emulation fails and exit
	to userspace
Date: Mon, 10 May 2010 19:06:05 +0300
Message-ID: <y2j52d4a3891005100906g1353b394h76ce675fe98f73bc@mail.gmail.com>
References: <20100510081656.GJ24787@redhat.com>
	 <20100510102525.GO24787@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: avi@redhat.com, mtosatti@redhat.com, kvm@vger.kernel.org
To: Gleb Natapov <gleb@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-fx0-f46.google.com ([209.85.161.46]:46534 "EHLO
	mail-fx0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751233Ab0EJQGH convert rfc822-to-8bit (ORCPT
	<rfc822;kvm@vger.kernel.org>); Mon, 10 May 2010 12:06:07 -0400
Received: by fxm7 with SMTP id 7so244566fxm.19
        for <kvm@vger.kernel.org>; Mon, 10 May 2010 09:06:05 -0700 (PDT)
In-Reply-To: <20100510102525.GO24787@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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 repo=
rt
>> failure to userspace instead. Userspace may choose to reenter guest =
if
>> vcpu is in userspace (cpl =3D=3D 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 =3D=3D 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/k=
vm_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 {
> =A0#define EMULTYPE_SKIP =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 2)
> =A0int emulate_instruction(struct kvm_vcpu *vcpu,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned long cr2, u16=
 error_code, int emulation_type);
> -void kvm_report_emulation_failure(struct kvm_vcpu *cvpu, const char =
*context);
> =A0void realmode_lgdt(struct kvm_vcpu *vcpu, u16 size, unsigned long =
address);
> =A0void 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)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 1;
> =A0 =A0 =A0 =A0case EMULATE_DO_MMIO:
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0++vcpu->stat.mmio_exits;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* fall through */
> =A0 =A0 =A0 =A0case EMULATE_FAIL:
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 vcpu->run->exit_reason =3D KVM_EXIT_INT=
ERNAL_ERROR;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 vcpu->run->internal.suberror =3D KVM_IN=
TERNAL_ERROR_EMULATION;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 vcpu->run->internal.ndata =3D 0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 0;
> =A0 =A0 =A0 =A0default:
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BUG();
> 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=
)
> =A0 =A0 =A0 =A0string =3D (io_info & SVM_IOIO_STR_MASK) !=3D 0;
> =A0 =A0 =A0 =A0in =3D (io_info & SVM_IOIO_TYPE_MASK) !=3D 0;
> =A0 =A0 =A0 =A0if (string || in)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return !(emulate_instruction(vcpu, 0, 0=
, 0) =3D=3D EMULATE_DO_MMIO);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return emulate_instruction(vcpu, 0, 0, =
0) =3D=3D EMULATE_DONE;
>
> =A0 =A0 =A0 =A0port =3D io_info >> 16;
> =A0 =A0 =A0 =A0size =3D (io_info & SVM_IOIO_SIZE_MASK) >> SVM_IOIO_SI=
ZE_SHIFT;
> @@ -2300,16 +2300,12 @@ static int iret_interception(struct vcpu_svm =
*svm)
>
> =A0static int invlpg_interception(struct vcpu_svm *svm)
> =A0{
> - =A0 =A0 =A0 if (emulate_instruction(&svm->vcpu, 0, 0, 0) !=3D EMULA=
TE_DONE)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pr_unimpl(&svm->vcpu, "%s: failed\n", _=
_func__);
> - =A0 =A0 =A0 return 1;
> + =A0 =A0 =A0 return emulate_instruction(&svm->vcpu, 0, 0, 0) =3D=3D =
EMULATE_DONE;
> =A0}
>
> =A0static int emulate_on_interception(struct vcpu_svm *svm)
> =A0{
> - =A0 =A0 =A0 if (emulate_instruction(&svm->vcpu, 0, 0, 0) !=3D EMULA=
TE_DONE)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pr_unimpl(&svm->vcpu, "%s: failed\n", _=
_func__);
> - =A0 =A0 =A0 return 1;
> + =A0 =A0 =A0 return emulate_instruction(&svm->vcpu, 0, 0, 0) =3D=3D =
EMULATE_DONE;
> =A0}
>
> =A0static 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)
> =A0 =A0 =A0 =A0++vcpu->stat.io_exits;
>
> =A0 =A0 =A0 =A0if (string || in)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return !(emulate_instruction(vcpu, 0, 0=
, 0) =3D=3D EMULATE_DO_MMIO);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return emulate_instruction(vcpu, 0, 0, =
0) =3D=3D EMULATE_DONE;
>
> =A0 =A0 =A0 =A0port =3D exit_qualification >> 16;
> =A0 =A0 =A0 =A0size =3D (exit_qualification & 7) + 1;
> @@ -3331,22 +3331,7 @@ static int handle_wbinvd(struct kvm_vcpu *vcpu=
)
>
> =A0static int handle_apic_access(struct kvm_vcpu *vcpu)
> =A0{
> - =A0 =A0 =A0 unsigned long exit_qualification;
> - =A0 =A0 =A0 enum emulation_result er;
> - =A0 =A0 =A0 unsigned long offset;
> -
> - =A0 =A0 =A0 exit_qualification =3D vmcs_readl(EXIT_QUALIFICATION);
> - =A0 =A0 =A0 offset =3D exit_qualification & 0xffful;
> -
> - =A0 =A0 =A0 er =3D emulate_instruction(vcpu, 0, 0, 0);
> -
> - =A0 =A0 =A0 if (er !=3D =A0EMULATE_DONE) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_ERR
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"Fail to handle apic acc=
ess vmexit! Offset is 0x%lx\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0offset);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOEXEC;
> - =A0 =A0 =A0 }
> - =A0 =A0 =A0 return 1;
> + =A0 =A0 =A0 return emulate_instruction(vcpu, 0, 0, 0) =3D=3D EMULAT=
E_DONE;
> =A0}
>
> =A0static int handle_task_switch(struct kvm_vcpu *vcpu)
> @@ -3558,13 +3543,8 @@ static int handle_invalid_guest_state(struct k=
vm_vcpu *vcpu)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto out;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (err !=3D EMULATE_DONE) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 vcpu->run->exit_reason =
=3D KVM_EXIT_INTERNAL_ERROR;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 vcpu->run->internal.sub=
error =3D KVM_INTERNAL_ERROR_EMULATION;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 vcpu->run->internal.nda=
ta =3D 0;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D 0;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (err !=3D EMULATE_DONE)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (signal_pending(current))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto 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 valu=
e, struct kvm_vcpu *vcpu)
> =A0 =A0 =A0 =A0return __kvm_set_dr(vcpu, dr, value);
> =A0}
>
> -void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char =
*context)
> -{
> - =A0 =A0 =A0 u8 opcodes[4];
> - =A0 =A0 =A0 unsigned long rip =3D kvm_rip_read(vcpu);
> - =A0 =A0 =A0 unsigned long rip_linear;
> -
> - =A0 =A0 =A0 if (!printk_ratelimit())
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
> -
> - =A0 =A0 =A0 rip_linear =3D rip + get_segment_base(vcpu, VCPU_SREG_C=
S);
> -
> - =A0 =A0 =A0 kvm_read_guest_virt(rip_linear, (void *)opcodes, 4, vcp=
u, NULL);
> -
> - =A0 =A0 =A0 printk(KERN_ERR "emulation failed (%s) rip %lx %02x %02=
x %02x %02x\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0context, rip, opcodes[0], opcodes[1], op=
codes[2], opcodes[3]);
> -}
> -EXPORT_SYMBOL_GPL(kvm_report_emulation_failure);
> -
> =A0static u64 mk_cr_64(u64 curr_cr, u32 new_val)
> =A0{
> =A0 =A0 =A0 =A0return (curr_cr & ~((1ULL << 32) - 1)) | new_val;
> @@ -3811,6 +3793,17 @@ static void inject_emulated_exception(struct k=
vm_vcpu *vcpu)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0kvm_queue_exception(vcpu, ctxt->except=
ion);
> =A0}
>
> +static int handle_emulation_failure(struct kvm_vcpu *vcpu)
> +{
> + =A0 =A0 =A0 ++vcpu->stat.insn_emulation_fail;
> + =A0 =A0 =A0 trace_kvm_emulate_insn_failed(vcpu);
> + =A0 =A0 =A0 vcpu->run->exit_reason =3D KVM_EXIT_INTERNAL_ERROR;
> + =A0 =A0 =A0 vcpu->run->internal.suberror =3D KVM_INTERNAL_ERROR_EMU=
LATION;
> + =A0 =A0 =A0 vcpu->run->internal.ndata =3D 0;
> + =A0 =A0 =A0 kvm_queue_exception(vcpu, UD_VECTOR);
> + =A0 =A0 =A0 return EMULATE_FAIL;
> +}
> +
> =A0int emulate_instruction(struct kvm_vcpu *vcpu,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned long cr2,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u16 error_code,
> @@ -3879,11 +3872,11 @@ int emulate_instruction(struct kvm_vcpu *vcpu=
,
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0++vcpu->stat.insn_emulation;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (r) =A0{
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ++vcpu->stat.insn_emula=
tion_fail;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 trace_kvm_emulate_insn_=
failed(vcpu);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (kvm_mmu_unprotect_=
page_virt(vcpu, cr2))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return=
 EMULATE_DONE;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return EMULATE_FAIL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (emulation_type & EM=
ULTYPE_SKIP)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return =
EMULATE_FAIL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return handle_emulation=
_failure(vcpu);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0}
>
> @@ -3908,9 +3901,7 @@ restart:
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (kvm_mmu_unprotect_page_virt(vcpu, =
cr2))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return EMULATE_DONE;
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 trace_kvm_emulate_insn_failed(vcpu);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 kvm_report_emulation_failure(vcpu, "mmi=
o");
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return EMULATE_FAIL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return handle_emulation_failure(vcpu);
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.=
interruptibility);
> @@ -4746,7 +4737,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vc=
pu, struct kvm_run *kvm_run)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vcpu->srcu_idx =3D srcu_read_lock(&vcp=
u->kvm->srcu);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0r =3D emulate_instruction(vcpu, 0, 0, =
EMULTYPE_NO_DECODE);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0srcu_read_unlock(&vcpu->kvm->srcu, vcp=
u->srcu_idx);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (r =3D=3D EMULATE_DO_MMIO) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (r !=3D EMULATE_DONE) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0r =3D 0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto out;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> --
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Gleb.
> --
> 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 =A0http://vger.kernel.org/majordomo-info.html
>