Re: KVM: fix i8259 reset irq acking

[Avi Kivity <avi@qumranet.com>]

Avi Kivity wrote:
> Avi Kivity wrote:
>> Marcelo Tosatti wrote:
>>> On Wed, Aug 13, 2008 at 01:40:34PM +0300, Avi Kivity wrote:
>>>  
>>>> Marcelo Tosatti wrote:
>>>>   
>>>>> The irq ack during pic reset has three problems:
>>>>>
>>>>> - Ignores slave/master PIC, using gsi 0-8 for both.
>>>>> - Generates an ACK even if the APIC is in control.
>>>>> - Depends upon IMR being clear, which is broken if the irq was
>>>>> masked at the time it was generated.
>>>>>
>>>>> The last one causes the BIOS to hang after the first reboot of
>>>>> Windows
>>>>> installation, since PIT interrupts stop.
>>>>>
>>>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>>>
>>>>>
>>>>> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
>>>>> index de70499..2b42d48 100644
>>>>> --- a/arch/x86/kvm/i8259.c
>>>>> +++ b/arch/x86/kvm/i8259.c
>>>>> @@ -195,14 +195,18 @@ int kvm_pic_read_irq(struct kvm *kvm)
>>>>>   void kvm_pic_reset(struct kvm_kpic_state *s)
>>>>>  {
>>>>> -    int irq;
>>>>> +    int irq, irqbase;
>>>>>      struct kvm *kvm = s->pics_state->irq_request_opaque;
>>>>>  -    for (irq = 0; irq < PIC_NUM_PINS; irq++) {
>>>>> -        if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
>>>>> -            s->isr & (1 << irq)))
>>>>> -            kvm_notify_acked_irq(kvm, irq);
>>>>> -    }
>>>>> +    if (s == &s->pics_state->pics[0])
>>>>> +        irqbase = 0;
>>>>> +    else
>>>>> +        irqbase = 8;
>>>>> +
>>>>> +    if (!kvm_lapic_enabled(kvm->vcpus[0]))
>>>>>         
>>>> What if the lapic is enabled and passes through PIC interrupts in
>>>> EXTINT  mode?
>>>>
>>>> btw, this will oops if we have no vcpu 0.
>>>>     
>>>
>>> True. How's this:
>>> -        if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
>>> -            s->isr & (1 << irq)))
>>> -            kvm_notify_acked_irq(kvm, irq);
>>> +        union ioapic_redir_entry entry;
>>> +        entry = kvm->arch.vioapic->redirtbl[irq+irqbase];
>>> +        if ((vcpu0 && !kvm_lapic_enabled(vcpu0)) ||
>>> +                  entry.fields.delivery_mode == IOAPIC_EXTINT) {
>>> +            if (s->irr & (1 << irq) || s->isr & (1 << irq))
>>> +                kvm_notify_acked_irq(kvm, irq+irqbase);
>>> +        }
>>>   
>>
>> Why look at the ioapic? You need to check lapic lvt0 delivery mode
>> for extint, not the ioapic.
>>
>
> I changed it to use kvm_apic_accept_pic_intr() and applied.
>
>

It still triggered the warning when the pit pending interrupt count
dropped below zero (booting FC6-i386).  I "fixed" it by making the pit
tolerate this condition, but there may be a bigger problem lurking in there.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.