Re: Regression after "Remove support for reporting coalesced APIC IRQs"

[Gleb Natapov <gleb@redhat.com>]

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.