Re: KVM: fix i8259 reset irq acking

[Avi Kivity <avi@qumranet.com>]

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.