Re: KVM: fix i8259 reset irq acking

[Avi Kivity <avi@qumranet.com>]

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.