From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@qumranet.com>
Subject: Re: KVM: fix i8259 reset irq acking
Date: Sun, 17 Aug 2008 17:36:45 +0300
Message-ID: <48A8377D.3000803@qumranet.com>
References: <20080731171556.GA2644@dmt.cnet> <48A2BA22.70805@qumranet.com> <20080814235325.GA14844@dmt.cnet> <48A7C396.3010603@qumranet.com> <48A7D662.8000905@qumranet.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Cc: kvm-devel <kvm@vger.kernel.org>
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from il.qumranet.com ([212.179.150.194]:31507 "EHLO il.qumranet.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753199AbYHQOgr (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 17 Aug 2008 10:36:47 -0400
In-Reply-To: <48A7D662.8000905@qumranet.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.