From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: Regression after "Remove support for reporting coalesced APIC IRQs" Date: Sun, 23 Jun 2013 12:13:10 +0300 Message-ID: <20130623091310.GR5832@redhat.com> References: <20130606085352.GZ4725@redhat.com> <20130620114745.GH5832@redhat.com> <51C361AA.8040705@web.de> <20130620202926.GB26747@redhat.com> <51C36DC2.4040508@web.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, yongjie.ren@intel.com To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:6403 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750811Ab3FWJNN (ORCPT ); Sun, 23 Jun 2013 05:13:13 -0400 Content-Disposition: inline In-Reply-To: <51C36DC2.4040508@web.de> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Jun 20, 2013 at 11:01:54PM +0200, Jan Kiszka wrote: > On 2013-06-20 22:29, Gleb Natapov wrote: > > On Thu, Jun 20, 2013 at 10:10:18PM +0200, Jan Kiszka wrote: > >> On 2013-06-20 13:47, Gleb Natapov wrote: > >>> Jan ping, are you OK with what I proposed below? > >>> > >>> On Thu, Jun 06, 2013 at 11:53:52AM +0300, Gleb Natapov wrote: > >>>> Hi Jan, > >>>> > >>>> I bisected [1] to f1ed0450a5fac7067590317cbf027f566b6ccbca. Fortunately > >>>> further investigation showed that it is not really related to removing > >>>> APIC timer interrupt reinjection and the real problem is that we cannot > >>>> assume that __apic_accept_irq() always injects interrupts like the patch > >>>> does because the function skips interrupt injection if APIC is disabled. > >>>> This misreporting screws RTC interrupt tracking, so further RTC interrupt > >>>> are stopped to be injected. The simplest solution that I see is to revert > >>>> most of the commit and only leave APIC timer interrupt reinjection. > >> > >> I'm not understanding the precise error yet and how __apic_accept_irq > >> should be (properly) involved in its solution. Which code path depend on > >> the information that the APIC is enabled? > >> > > RTC interrupt injection tracking in virt/kvm/ioapic.c depends on > > accurate information about which vcpus interrupt was injected into since it > > expects EOI from each vcpu before injection next RTC interrupt. Since > > now kvm_apic_set_irq() reports interrupt as injected for vcpus with > > disabled apic the logic breaks because EOI will never happen. > > OK, so we may have to enhance kvm_apic_set_irq. Or implement the > apic_enabled check properly in __apic_accept_irq (though only > kvm_apic_set_irq will have a use for it). > > The (historic) check looks very strange, misplaced, and carries a > comment that is probably also outdated. The check used to protect the > only functioning delivery mode, but then was left in place, ignoring all > the other modes. > The only other delivery mode that needs to be protected is NMI as far as I see and check can be relaxed to kvm_apic_sw_enabled() since if apic is hw disabled the function should not be called. I do not think the check is strange or misplaced, where do you propose to put it instead? The comment is probably outdated since I cannot figure out what it means :) > Yes, that's not directly related to this regression. We can revert first > and then rework this interface. But the latter should definitely be done > as the revert will make the interface even worse IMHO. > > Jan > > -- Gleb.