From patchwork Mon Dec 22 13:55:19 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Wincy Van X-Patchwork-Id: 5527431 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 41656BEEA8 for ; Mon, 22 Dec 2014 13:55:51 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2CBA22017A for ; Mon, 22 Dec 2014 13:55:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 261C920172 for ; Mon, 22 Dec 2014 13:55:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754981AbaLVNzn (ORCPT ); Mon, 22 Dec 2014 08:55:43 -0500 Received: from mail-lb0-f170.google.com ([209.85.217.170]:50121 "EHLO mail-lb0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754905AbaLVNzl (ORCPT ); Mon, 22 Dec 2014 08:55:41 -0500 Received: by mail-lb0-f170.google.com with SMTP id 10so3938909lbg.29 for ; Mon, 22 Dec 2014 05:55:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=h0wmjJ9o7jqwBXz2/lQP2GyUq9/tbMk97y53Rhj7Ohw=; b=i8ytvAaJLg3ASDyKq6qgfx07y9MmiuKMd0aszE7ga6UFCSS9mcOsbF1alAsgXd80sw eQnj1ooNzOJCClVbuXOL96tJDlBsq5eCTeWf+HdYh5TGAf1Q7s2q9MUrb2cFFZ4Gqwbm qJ0lrFR7dZhxNElPN0e5B70NTL6sIGy0y4KQZsHVkkhpP2QyT8/n9i/HdLagPh3HwxVl 8+yhPWYG9tfyeMZMsAx3x+nNiIrdGk9qXsO8rAYuDgCl6aqu7GDjJmwIHDqMrfAXMoA8 /VMnpCRz50sA6FomiJ/9MpCX0wczsWRynagZHCwy/L65j+vroHjR2sJ74MoDaeb3au6+ fmsw== X-Received: by 10.152.25.129 with SMTP id c1mr15285218lag.65.1419256540140; Mon, 22 Dec 2014 05:55:40 -0800 (PST) MIME-Version: 1.0 Received: by 10.25.161.76 with HTTP; Mon, 22 Dec 2014 05:55:19 -0800 (PST) In-Reply-To: <5493F4FA.5050605@redhat.com> References: <54915F1B.30107@redhat.com> <5493F4FA.5050605@redhat.com> From: Wincy Van Date: Mon, 22 Dec 2014 21:55:19 +0800 Message-ID: Subject: Re: [question] Why newer QEMU may lose irq when doing migration? To: Paolo Bonzini Cc: yang.z.zhang@intel.com, Bandan Das , kvm@vger.kernel.org, Wanpeng Li Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 2014-12-19 17:50 GMT+08:00 Paolo Bonzini : > > > On 19/12/2014 04:58, Wincy Van wrote: >> 2014-12-17 18:46 GMT+08:00 Paolo Bonzini : >>> >>> >>> On 17/12/2014 04:46, Wincy Van wrote: >>>> Hi, all: >>>> >>>> The patchset (https://lkml.org/lkml/2014/3/18/309) fixed migration of >>>> Windows guests, but commit 0bc830b05c667218d703f2026ec866c49df974fc >>>> (KVM: ioapic: clear IRR for edge-triggered interrupts at delivery) >>>> introduced a bug (see >>>> https://www.mail-archive.com/kvm@vger.kernel.org/msg109813.html). >>>> >>>> From the description "Unlike the old qemu-kvm, which really never did >>>> that, with new QEMU it is for some reason >>>> somewhat likely to migrate a VM with a nonzero IRR in the ioapic." >>>> >>>> Why could new QEMU do that? I can not find any codes about the "some reason".. >>>> As we know, once a irq is set in kvm's ioapic, the ioapic will send >>>> that irq to lapic, this is an "atomic" operation. >>> >>> It can happen if the IRQ is masked in the IOAPIC, for example. Until >>> commit 0bc830b, KVM could not distinguish two cases: >>> >>> 1) an edge-triggered interrupt that was raised while the IOAPIC had it >>> masked >>> >>> 2) an edge-triggered interrupt that was raised and delivered, but for >>> which userspace left the level to 1. >> >> It seems that QEMU's rtc behavior is case 2. But before this patchset, a rtc >> interrupt may be lost when doing migration, and guest will not acknowledge it, >> then the newer rtc interrupts are ignored forever. I think this is >> none of the cases above, because the interrupt was lost. It must be >> something wrong here. > > There is a third case actually. > > If the source kernel is an old one before commit 2c2bf0113697 (KVM: Use > eoi to track RTC interrupt delivery status, 2013-04-11), ioapic->irr can > also be set if the RTC interrupt was coalesced (for example because the > PPR was too high to deliver it). > Yeah, understood, thanks. > Instead, commit 2c2bf0113697 will not set ioapic->irr in this case. > Yang, was this intentional? > > The question, however, is then why my patch set worked (fixed migration) > even without moving > > ioapic->irr |= mask; > > above this: > > if (irq == RTC_GSI && line_status && > rtc_irq_check_coalesced(ioapic)) { > ret = 0; /* coalesced */ > goto out; > } > Paolo, how about this patch? It uses a new field named irr_delivered to record the status of edge-triggered interrupt, and clears the delivered irr in kvm_get_ioapic. So it have the same effect of commit 0bc830b while avoid the bug of Windows guests. Thanks, Wincy --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c index b1947e0..eda7905 100644 --- a/arch/x86/kvm/ioapic.c +++ b/arch/x86/kvm/ioapic.c @@ -206,6 +206,8 @@ static int ioapic_set_irq(struct kvm_ioapic *ioapic, unsigned int irq, old_irr = ioapic->irr; ioapic->irr |= mask; + if (edge) + ioapic->irr_delivered &= ~mask; if ((edge && old_irr == ioapic->irr) || (!edge && entry.fields.remote_irr)) { ret = 0; @@ -349,7 +351,7 @@ static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status) irqe.shorthand = 0; if (irqe.trig_mode == IOAPIC_EDGE_TRIG) - ioapic->irr &= ~(1 << irq); + ioapic->irr_delivered |= 1 << irq; if (irq == RTC_GSI && line_status) { /* @@ -597,6 +599,7 @@ static void kvm_ioapic_reset(struct kvm_ioapic *ioapic) ioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS; ioapic->ioregsel = 0; ioapic->irr = 0; + ioapic->irr_delivered = 0; ioapic->id = 0; memset(ioapic->irq_eoi, 0x00, IOAPIC_NUM_PINS); rtc_irq_eoi_tracking_reset(ioapic); @@ -654,6 +657,7 @@ int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state) spin_lock(&ioapic->lock); memcpy(state, ioapic, sizeof(struct kvm_ioapic_state)); + state->irr &= ~ioapic->irr_delivered; spin_unlock(&ioapic->lock); return 0; } diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h index 3c91955..a5cdfc0 100644 --- a/arch/x86/kvm/ioapic.h +++ b/arch/x86/kvm/ioapic.h @@ -77,6 +77,7 @@ struct kvm_ioapic { struct rtc_status rtc_status; struct delayed_work eoi_inject; u32 irq_eoi[IOAPIC_NUM_PINS]; + u32 irr_delivered; }; #ifdef DEBUG