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

[Jan Kiszka]

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.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux