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 10:42:26 +0300
Message-ID: <48A7D662.8000905@qumranet.com>
References: <20080731171556.GA2644@dmt.cnet> <48A2BA22.70805@qumranet.com> <20080814235325.GA14844@dmt.cnet> <48A7C396.3010603@qumranet.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
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]:49800 "EHLO il.qumranet.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751736AbYHQHmY (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 17 Aug 2008 03:42:24 -0400
In-Reply-To: <48A7C396.3010603@qumranet.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.