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 09:22:14 +0300
Message-ID: <48A7C396.3010603@qumranet.com>
References: <20080731171556.GA2644@dmt.cnet> <48A2BA22.70805@qumranet.com> <20080814235325.GA14844@dmt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii; 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]:39516 "EHLO il.qumranet.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751399AbYHQGWN (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 17 Aug 2008 02:22:13 -0400
In-Reply-To: <20080814235325.GA14844@dmt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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 have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.