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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
