| Message ID | 1509890866-8736-3-git-send-email-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Thanks, Paolo
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 >
On 11/11/2017 02:44, Liran Alon wrote: > > 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. Probably because the exception.pending case is very rare right now. It will become more common however if I can get rid of kvm_inject_page_fault_nested. This more or less also answers your question as to why this patch is related to the removal of kvm_inject_page_fault_nested. Thanks, Paolo
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 03869eb7fcd6..41f199287edf 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6379,33 +6379,38 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win) int r; /* try to reinject previous events if any */ - if (vcpu->arch.exception.injected) { + if (vcpu->arch.exception.injected) kvm_x86_ops->queue_exception(vcpu); - return 0; - } - /* * Exceptions must be injected immediately, or the exception * frame will have the address of the NMI or interrupt handler. */ - if (!vcpu->arch.exception.pending) { - if (vcpu->arch.nmi_injected) { + else if (!vcpu->arch.exception.pending) { + if (vcpu->arch.nmi_injected) kvm_x86_ops->set_nmi(vcpu); - return 0; - } - - if (vcpu->arch.interrupt.pending) { + else if (vcpu->arch.interrupt.pending) kvm_x86_ops->set_irq(vcpu); - return 0; - } } + /* + * Call check_nested_events() even if we reinjected a previous event + * in order for caller to determine if it should require immediate-exit + * from L2 to L1 due to pending L1 events which require exit + * from L2 to L1. + */ if (is_guest_mode(vcpu) && kvm_x86_ops->check_nested_events) { r = kvm_x86_ops->check_nested_events(vcpu, req_int_win); if (r != 0) return r; } + /* + * If we reinjected a previous event, + * don't consider a new pending event + */ + if (kvm_event_needs_reinjection(vcpu)) + return 0; + /* try to inject new event if pending */ if (vcpu->arch.exception.pending) { trace_kvm_inj_exception(vcpu->arch.exception.nr,