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

[Jan Kiszka <jan.kiszka@siemens.com>]

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.

Jan

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