From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3FDABC433B4 for ; Mon, 12 Apr 2021 21:43:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 096236135D for ; Mon, 12 Apr 2021 21:43:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240671AbhDLVoE (ORCPT ); Mon, 12 Apr 2021 17:44:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37500 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239771AbhDLVoA (ORCPT ); Mon, 12 Apr 2021 17:44:00 -0400 Received: from mail-pf1-x42b.google.com (mail-pf1-x42b.google.com [IPv6:2607:f8b0:4864:20::42b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6E008C061756 for ; Mon, 12 Apr 2021 14:43:41 -0700 (PDT) Received: by mail-pf1-x42b.google.com with SMTP id m11so10048272pfc.11 for ; Mon, 12 Apr 2021 14:43:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=LTAUlRV+90mXaog4LE7DEsSgTnXw1ApT5g7eDZ+3hQQ=; b=EOQdsGxc+trZ9fBNqii1aYmL+oejnNTIpAmeJxEDRtdv4sfpgl7CUHgZDeu0bzilfo Ad1P6/PTP7qzpUVzhfxZkSse+TSPAK5HIfUeanowvW60r32lgP66yn+90YMmPzDHeS+Q gN0qv9EKIq2UrdKOuGWLEZInZ9IbXf3jXjeaq+okQNM70WNHO2oyNe40kGKx6q3fp3hj 3nEDYeAVKe2W5jj8S48nyesHC432eKHVfm72/FI3OZh8IblL7iMO1ZqcghmMvMrLxV+M EfxoVNNr9U1B4spOlvqUCL9Pjj0WFo3+f2J+r4kojtqyQNEJbPGP7ywIk1aRVsG5zHyT i2yA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=LTAUlRV+90mXaog4LE7DEsSgTnXw1ApT5g7eDZ+3hQQ=; b=pnKIqWwmT2g2fQF/wSp3rYfFshGYaq/RFZ3rBgExYh71d8QUsevplpMEzCKGx1EVsw jx42A3twHiGgHn7+gSlRnJ9l1ewSheT9zwf54I9Rsm9a2BzmYS3iGaR73hdZvFA5KN5A pmgATBOgTsAx1zGiT/div8m0JuhJSutkhapGcS7GHWZwf/0AxQe2ipmD9+1BI7gfIZQT +VABJarWJ7Uj2AbToP6GeWQND6DPXd5zUSxqo06K4lLCxL/+sJbyyMy2JedNiHoe0+te yVlOj9CfLEX42BmWO78uxtSIoR3K8gT88eL1cPUDWz1blRnLMx0DqwRqxHK2AxpbpM7e fweg== X-Gm-Message-State: AOAM530rFCohJZbprwHkffcthm7xYpp8eXO8JC+TX3z2dOD5cLoD1C7w yFC+3lFWZT3jS2dFt4EvBiwSEA== X-Google-Smtp-Source: ABdhPJzHRXu/pBnp+xlAC/tzDGNIdRDWyySF7tk5goKiPbjZXsGgZ8G7eIRRFRHas8vMljy5GHXXZQ== X-Received: by 2002:a63:d50c:: with SMTP id c12mr11168928pgg.145.1618263820594; Mon, 12 Apr 2021 14:43:40 -0700 (PDT) Received: from google.com (240.111.247.35.bc.googleusercontent.com. [35.247.111.240]) by smtp.gmail.com with ESMTPSA id 77sm12720008pgf.55.2021.04.12.14.43.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 Apr 2021 14:43:39 -0700 (PDT) Date: Mon, 12 Apr 2021 21:43:35 +0000 From: Sean Christopherson To: Lai Jiangshan Cc: Paolo Bonzini , LKML , kvm@vger.kernel.org, Filippo Sironi , David Woodhouse , "v4.7+" , Wanpeng Li Subject: Re: [PATCH 2/2] KVM: x86: Fix split-irqchip vs interrupt injection window request Message-ID: References: <20201127112114.3219360-1-pbonzini@redhat.com> <20201127112114.3219360-3-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Fri, Apr 09, 2021, Lai Jiangshan wrote: > On Fri, Nov 27, 2020 at 7:26 PM Paolo Bonzini wrote: > > > > kvm_cpu_accept_dm_intr and kvm_vcpu_ready_for_interrupt_injection are > > a hodge-podge of conditions, hacked together to get something that > > more or less works. But what is actually needed is much simpler; > > in both cases the fundamental question is, do we have a place to stash > > an interrupt if userspace does KVM_INTERRUPT? > > > > In userspace irqchip mode, that is !vcpu->arch.interrupt.injected. > > Currently kvm_event_needs_reinjection(vcpu) covers it, but it is > > unnecessarily restrictive. > > > > In split irqchip mode it's a bit more complicated, we need to check > > kvm_apic_accept_pic_intr(vcpu) (the IRQ window exit is basically an INTACK > > cycle and thus requires ExtINTs not to be masked) as well as > > !pending_userspace_extint(vcpu). However, there is no need to > > check kvm_event_needs_reinjection(vcpu), since split irqchip keeps > > pending ExtINT state separate from event injection state, and checking > > kvm_cpu_has_interrupt(vcpu) is wrong too since ExtINT has higher > > priority than APIC interrupts. In fact the latter fixes a bug: > > when userspace requests an IRQ window vmexit, an interrupt in the > > local APIC can cause kvm_cpu_has_interrupt() to be true and thus > > kvm_vcpu_ready_for_interrupt_injection() to return false. When this > > happens, vcpu_run does not exit to userspace but the interrupt window > > vmexits keep occurring. The VM loops without any hope of making progress. > > > > Once we try to fix these with something like > > > > return kvm_arch_interrupt_allowed(vcpu) && > > - !kvm_cpu_has_interrupt(vcpu) && > > - !kvm_event_needs_reinjection(vcpu) && > > - kvm_cpu_accept_dm_intr(vcpu); > > + (!lapic_in_kernel(vcpu) > > + ? !vcpu->arch.interrupt.injected > > + : (kvm_apic_accept_pic_intr(vcpu) > > + && !pending_userspace_extint(v))); > > > > we realize two things. First, thanks to the previous patch the complex > > conditional can reuse !kvm_cpu_has_extint(vcpu). Second, the interrupt > > window request in vcpu_enter_guest() > > > > bool req_int_win = > > dm_request_for_irq_injection(vcpu) && > > kvm_cpu_accept_dm_intr(vcpu); > > > > should be kept in sync with kvm_vcpu_ready_for_interrupt_injection(): > > it is unnecessary to ask the processor for an interrupt window > > if we would not be able to return to userspace. Therefore, the > > complex conditional is really the correct implementation of > > kvm_cpu_accept_dm_intr(vcpu). It all makes sense: > > > > - we can accept an interrupt from userspace if there is a place > > to stash it (and, for irqchip split, ExtINTs are not masked). > > Interrupts from userspace _can_ be accepted even if right now > > EFLAGS.IF=0. > > Hello, Paolo > > If userspace does KVM_INTERRUPT, vcpu->arch.interrupt.injected is > set immediately, and in inject_pending_event(), we have > > else if (!vcpu->arch.exception.pending) { > if (vcpu->arch.nmi_injected) { > kvm_x86_ops.set_nmi(vcpu); > can_inject = false; > } else if (vcpu->arch.interrupt.injected) { > kvm_x86_ops.set_irq(vcpu); > can_inject = false; > } > } > > I'm curious about that can the kvm_x86_ops.set_irq() here be possible > to queue the irq with EFLAGS.IF=0? If not, which code prevents it? The interrupt is only directly injected if the local APIC is _not_ in-kernel. If userspace is managing the local APIC, my understanding is that userspace is also responsible for honoring EFLAGS.IF, though KVM aids userspace by updating vcpu->run->ready_for_interrupt_injection when exiting to userspace. When userspace is modeling the local APIC, that resolves to kvm_vcpu_ready_for_interrupt_injection(): return kvm_arch_interrupt_allowed(vcpu) && kvm_cpu_accept_dm_intr(vcpu); where kvm_arch_interrupt_allowed() checks EFLAGS.IF (and an edge case related to nested virtualization). KVM also captures EFLAGS.IF in vcpu->run->if_flag. For whatever reason, QEMU checks both vcpu->run flags before injecting an IRQ, maybe to handle a case where QEMU itself clears EFLAGS.IF? > I'm asking about this because I just noticed that interrupt can > be queued when exception pending, and this patch relaxed it even > more. > > Note: interrupt can NOT be queued when exception pending > until 664f8e26b00c7 ("KVM: X86: Fix loss of exception which > has not yet been injected") which I think is dangerous.