From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liran Alon Subject: Re: [PATCH 2/3] KVM: x86: emulator: Return to user-mode on L1 CPL=0 emulation failure Date: Mon, 06 Nov 2017 15:25:48 +0200 Message-ID: <5A0062DC.4030806@ORACLE.COM> References: <1509891703-9735-1-git-send-email-liran.alon@oracle.com> <1509891703-9735-3-git-send-email-liran.alon@oracle.com> <61331a1d-3f34-9328-ec28-c223d67fe1e9@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: idan.brown@ORACLE.COM, Liran Alon , Konrad Rzeszutek Wilk To: Paolo Bonzini , rkrcmar@redhat.com, kvm@vger.kernel.org Return-path: Received: from aserp1040.oracle.com ([141.146.126.69]:18424 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752793AbdKFNZ5 (ORCPT ); Mon, 6 Nov 2017 08:25:57 -0500 In-Reply-To: <61331a1d-3f34-9328-ec28-c223d67fe1e9@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 06/11/17 11:19, Paolo Bonzini wrote: > On 05/11/2017 15:21, Liran Alon wrote: >> From: Liran Alon >> >> On this case, handle_emulation_failure() fills kvm_run with >> internal-error information which it expects to be delivered >> to user-mode for further processing. >> However, the code reports a wrong return-value which makes KVM to never >> return to user-mode on this scenario. >> >> Fixes: 6d77dbfc88e3 ("KVM: inject #UD if instruction emulation fails and exit to >> userspace") >> >> Signed-off-by: Liran Alon >> Reviewed-by: Nikita Leshenko >> Reviewed-by: Konrad Rzeszutek Wilk >> Signed-off-by: Konrad Rzeszutek Wilk >> --- >> arch/x86/kvm/x86.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 03869eb7fcd6..f4edb4baf441 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -5413,7 +5413,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu) >> vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; >> vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; >> vcpu->run->internal.ndata = 0; >> - r = EMULATE_FAIL; >> + r = EMULATE_USER_EXIT; >> } >> kvm_queue_exception(vcpu, UD_VECTOR); >> >> > > You should still get an emulation failure when the emulator is invoked > by mmu.c: > > case EMULATE_USER_EXIT: > ++vcpu->stat.mmio_exits; > /* fall through */ > case EMULATE_FAIL: > return 0; > > What is the caller of x86_emulate_instruction that you are interested > in? #UD is not affected, because emulation_type has EMULTYPE_TRAP_UD > set and therefore x86_emulate_instruction exits before invoking > handle_emulation_failure. I think this patch is still correct from a number of reasons: 1) If I understand the code correctly, semantically it doesn't make sense to fill kvm_run struct without exiting to user-mode. Therefore, if emulator filled kvm_run, it makes sense that it needs to return EMULATE_USER_EXIT. 2) EMULTYPE_TRAP_UD only causes the emulator to return EMULATE_FAIL in case emulation fails on instruction decoding. However, consider a case where #UD intercept happens on a valid instruction (such as VMMCALL AMD opcode on physical Intel CPU). In that case, instruction decoding doesn't fail but we could still fail on instruction emulation at x86_emulate_insn(). In this case, EMULTYPE_TRAP_UD flag is not considered anymore and failure will reach handle_emulation_failure(). 3) We have another KVM commits series (not upstream yet) that adds intercept on #GP which calls the x86 emulator. This is done to allow access to I/O ports even though they aren't allowed via guest's TSS I/O permissions bitmap. > > Thanks, > > Paolo >