Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752766AbdHXJrM (ORCPT ); Thu, 24 Aug 2017 05:47:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55756 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752435AbdHXJrJ (ORCPT ); Thu, 24 Aug 2017 05:47:09 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6B1207E42F Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=pbonzini@redhat.com Subject: Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected To: Wanpeng Li Cc: "linux-kernel@vger.kernel.org" , kvm , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Wanpeng Li References: <1503548506-4457-1-git-send-email-wanpeng.li@hotmail.com> <1503548506-4457-2-git-send-email-wanpeng.li@hotmail.com> From: Paolo Bonzini Message-ID: <5424f512-3612-4f5a-2a10-36f4182f931d@redhat.com> Date: Thu, 24 Aug 2017 11:47:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 24 Aug 2017 09:47:09 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3826 Lines: 103 On 24/08/2017 11:34, Wanpeng Li wrote: > 2017-08-24 17:13 GMT+08:00 Wanpeng Li : >> 2017-08-24 16:57 GMT+08:00 Paolo Bonzini : >>> On 24/08/2017 08:52, Wanpeng Li wrote: >>>>> @@ -6862,6 +6876,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) >>>>> kvm_x86_ops->enable_nmi_window(vcpu); >>>>> if (kvm_cpu_has_injectable_intr(vcpu) || req_int_win) >>>>> kvm_x86_ops->enable_irq_window(vcpu); >>>>> + WARN_ON(vcpu->arch.exception.pending); >>>> >>>> This WARN_ON() is suggested during the review of last version, >>>> however, there are many cases in inject_pending_event() can result in >>>> return directly w/ vcpu->arch.exception.pending is true. Actually I >>>> have already catched the warning several times during the testing. I >>>> think we should remove it when committing. >>> >>> No, it's a good thing that it's failing, because it's finding a bug. >>> There's no such thing as an "exception window", so at the very least it >> >> Good point, the code looks good, I will fold it in next version. >> However, I still can observe the warning. >> >> Regards, >> Wanpeng Li > > I observed sometimes both vcpu->arch.exception.pending and > vcpu->arch.expception.injected are true before executing below codes: > > if (vcpu->arch.exception.injected) { > kvm_x86_ops->queue_exception(vcpu); > return 0; > } More missing pieces: 1) kvm_x86_ops->queue_exception must be called in the vcpu->arch.exception.pending case. Compare it with the others, which are calling enter_smm, kvm_x86_ops->set_irq, kvm_x86_ops->set_nmi. 2) here: if (!vcpu->arch.exception.pending || !vcpu->arch.exception.injected) { queue: if (has_error && !is_protmode(vcpu)) has_error = false; if (reinject) vcpu->arch.exception.injected = true; else vcpu->arch.exception.pending = true; you need to reset the other field, because you can get here from the double-fault case. Likewise below: if ((class1 == EXCPT_CONTRIBUTORY && class2 == EXCPT_CONTRIBUTORY) || (class1 == EXCPT_PF && class2 != EXCPT_BENIGN)) { /* generate double fault per SDM Table 5-5 */ vcpu->arch.exception.pending = true; injected must be cleared. > Regards, > Wanpeng Li > >> >>> should set req_immediate_exit to true. >>> >>> Does this help? >>> >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index b698b2f135a2..76d5a192be6c 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -6365,14 +6365,20 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win) >>> return 0; >>> } >>> >>> - if (vcpu->arch.nmi_injected) { >>> - kvm_x86_ops->set_nmi(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) { >>> + kvm_x86_ops->set_nmi(vcpu); >>> + return 0; >>> + } >>> >>> - if (vcpu->arch.interrupt.pending) { >>> - kvm_x86_ops->set_irq(vcpu); >>> - return 0; >>> + if (vcpu->arch.interrupt.pending) { >>> + kvm_x86_ops->set_irq(vcpu); >>> + return 0; >>> + } >>> } >>> >>> if (is_guest_mode(vcpu) && kvm_x86_ops->check_nested_events) { >>> >>> Paolo