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

[Avi Kivity]

On 09/12/2012 11:57 AM, Jan Kiszka wrote:
> On 2012-09-12 10:51, Avi Kivity wrote:
>> 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.
> 
> I would suggest to solve this for the userspace model first, ensure that
> it works properly in all modes, maybe optimize it, and then decide how
> to map all this on kernel space. As long as we have two models, we can
> also make use of them.

Good idea.


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