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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

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.

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]