Re: [Qemu-devel] [PATCH 1/2] KVM: fix i8259 interrupt high to low transition logic

[Avi Kivity]

On 09/12/2012 11:48 AM, Jan Kiszka wrote:
> On 2012-09-12 10:01, Avi Kivity wrote:
>> On 09/10/2012 04:29 AM, Matthew Ogilvie wrote:
>>> Intel's definition of "edge triggered" means: "asserted with a
>>> low-to-high transition at the time an interrupt is registered
>>> and then kept high until the interrupt is served via one of the
>>> EOI mechanisms or goes away unhandled."
>>>
>>> So the only difference between edge triggered and level triggered
>>> is in the leading edge, with no difference in the trailing edge.
>>>
>>> This bug manifested itself when the guest was Microport UNIX
>>> System V/386 v2.1 (ca. 1987), because it would sometimes mask
>>> off IRQ14 in the slave IMR after it had already been asserted.
>>> The master would still try to deliver an interrupt even though
>>> IRQ2 had dropped again, resulting in a spurious interupt
>>> (IRQ15) and a panicked UNIX kernel.
>>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
>>> index adba28f..5cbba99 100644
>>> --- a/arch/x86/kvm/i8254.c
>>> +++ b/arch/x86/kvm/i8254.c
>>> @@ -302,8 +302,12 @@ static void pit_do_work(struct kthread_work *work)
>>>     }
>>>     spin_unlock(&ps->inject_lock);
>>>     if (inject) {
>>> -           kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
>>> +           /* Clear previous interrupt, then create a rising
>>> +            * edge to request another interupt, and leave it at
>>> +            * level=1 until time to inject another one.
>>> +            */
>>>             kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
>>> +           kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
>>>  
>>>             /*
>> 
>> I thought I understood this, now I'm not sure.  How can this be correct?
>>  Real hardware doesn't act like this.
>> 
>> What if the PIT is disabled after this?  You're injecting a spurious
>> interrupt then.
> 
> Yes, the PIT has to raise the output as long as specified, i.e.
> according to the datasheet. That's important now due to the corrections
> to the PIC. We can then carefully check if there is room for
> simplifications / optimizations. I also cannot imagine that the above
> already fulfills these requirements.
> 
> And if the PIT is disabled by the HPET, we need to clear the output
> explicitly as we inject the IRQ#0 under a different source ID than
> userspace HPET does (which will logically take over IRQ#0 control). The
> kernel would otherwise OR both sources to an incorrect result.
> 

I guess we need to double the hrtimer rate then in order to generate a
square wave.  It's getting ridiculous how accurate our model needs to be.


-- 
error compiling committee.c: too many arguments to function