From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liran Alon Subject: Re: [PATCH 2/4] KVM: nVMX: Require immediate-exit when event reinjected to L2 and L1 event pending Date: Sat, 11 Nov 2017 03:44:28 +0200 Message-ID: <5A0655FC.90003@ORACLE.COM> References: <1509890866-8736-1-git-send-email-liran.alon@oracle.com> <1509890866-8736-3-git-send-email-liran.alon@oracle.com> <1585ee8f-2783-028f-caee-f8116a080fcc@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: jmattson@google.com, idan.brown@ORACLE.COM, Konrad Rzeszutek Wilk To: Paolo Bonzini , rkrcmar@redhat.com, kvm@vger.kernel.org Return-path: Received: from aserp1040.oracle.com ([141.146.126.69]:18436 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750904AbdKKBoh (ORCPT ); Fri, 10 Nov 2017 20:44:37 -0500 In-Reply-To: <1585ee8f-2783-028f-caee-f8116a080fcc@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 11/11/17 01:26, Paolo Bonzini wrote: > On 05/11/2017 15:07, Liran Alon wrote: >> + /* >> + * If we reinjected a previous event, >> + * don't consider a new pending event >> + */ >> + if (kvm_event_needs_reinjection(vcpu)) >> + return 0; >> + > > Could you end up with > > WARN_ON(vcpu->arch.exception.pending); > > in vcpu_enter_guest after returning 0 here? > > Maybe it would be safer to return a non-zero value so that the caller > sets req_immediate_exit = true. But I haven't really thought through > the consequences. The only difference before and after this patch *should* have been that now if L1 has pending event (as specified by vmx_check_nested_events()), a value of -EBUSY will be returned and an immediate-exit will be requested. Even if a re-injection had occurred. If that is not the case, previous code and this code should return 0 on the exact same cases. *However*, if exception.pending=true and nmi_injected/interrupt.pending=true, then previous code would have continued inject_pending_event() while this code would return too-soon. Indeed triggering the above mentioned warning. Therefore I think you have found here a bug that was missed in the review-chain from some reason and wasn't observed in tests... Will investigate how tests didn't caught this. Sorry for this. It seems that this patch approach would have worked on version before commit 664f8e26b00c ("KVM: X86: Fix loss of exception which has not yet been injected") but afterwards will break... Will fix and re-run all tests. Thanks, -Liran > > Thanks, > > Paolo >