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

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

On 2012-09-13 07:49, Matthew Ogilvie wrote:
> On Wed, Sep 12, 2012 at 10:57:57AM +0200, 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.
> 
> Thoughts about the 8254 PIT:
> 
> First, this summary of (real) 8254 PIT behavior seems fairly
> good, as far it goes:
> 
> On Tue, Sep 04, 2012 at 07:27:38PM +0100, Maciej W. Rozycki wrote:
>>  * The 8254 PIT is normally configured in mode 2 or 3 in the PC/AT
>>    architecture.  In the former its output is high (active) all the time
>>    except from one (last) clock cycle.  In the latter a wave that has a
>>    duty cycle close or equal to 0.5 (depending on whether the divider is
>>    odd or even) is produced, so no short pulses either.  I don't remember
>>    the other four modes -- have a look at the datasheet if interested, but
>>    I reckon they're not really compatible with the wiring anyway, e.g. the
>>    gate is hardwired enabled.
> 
> I've also just skimmed parts of the 8254 section of "The Indispensable PC
> Hardware Book", by Hans-Peter Messmer, Copyright 1994 Addison-Wesley,
> although I probably ought to read it more carefully.

http://download.intel.com/design/archives/periphrl/docs/23124406.pdf
should be the primary reference - as long as it leaves no open questions.

> 
> Under "normal" conditions, the 8254 part of the patch above should be
> indistinguishable from previous behavior.  The 8259's IRR will
> still show up as 1 until the interrupt is actually serviced,
> and no new interrupt will be serviced after one is serviced until
> another edge is injected via the high-low-high transition of the new
> code.  (Unless the guest resets the 8259 or maybe messes with IMR,
> but real hardware would generate extra interrupts in such cases as
> well.)
> 
> The new code sounds much closer to mode 2 described by
> Maciej, compared to the old code - except the duty cycle is
> effectively 100 percent instead of 99.[some number of 9's] percent.
> 
> -----------------
> But there might be some concerns in abnormal conditions:
> 
>    * If some guest is actually depending on a 50 percent duty cycle
>      (maybe some kind of polling rather than interrupts), I would
>      expect it to be just as broken before this patch as after,
>      unless it is really weird (handles continuous 1's more
>      gracefully than continuous 0's).
> 
>         According to the my book, mode 3 isn't normally
>      used to create interrupts: "The generated square-wave signal
>      can be used, for example, to trasmit data via serial interfaces.
>      The PIT then operates as a baud rate generator.  In the PC,
>      counters 1 and 2 are operated in mode 3 to drive memory refresh
>      and the speaker, respectively." (page 369)
> 
>         I wouldn't be inclined to worry about the 50 percent duty
>      cycle too much unless someone comes up with a real guest OS
>      that depends on it.
> 
>    * To be correct there are probably some cases when the 8254 should
>      force the IRQ0 line when the guest is setting up the 8254.  Based on
>      a very quick reading of some of the 8254 section of my book:
> 
>       * Note that modes 0, 2, 3 and 4 look usable with a hard-wired GATE=1,
>         but not modes 1 or 5.
>       * Maybe force IRQ0=0 when the 8254 is disabled (however that is done;
>         I haven't found it yet)?
>       * Force IRQ0=0 when starting the timer in "mode 0" (one-shot).
>       * Modes 2, 3, and 4 all apparently start with IRQ0=1.  I guess
>         they generate an interrupt when first enabled?
>          * Mode 2 (periodic) has a 99 percent duty cycle (high).
>          * Mode 3 (periodic) a 50 percent duty cycle (see above).
>          * Mode 4 (one-shot) is distinguished from mode 0 in that
>            it generates both a high-low and low-high transition when
>            it expires, instead of just a low-high.
> 
>    * I don't know anything about the HPET.  I didn't even realize
>      it re-uses IRQ0.

In modern chipsets, the HPET can drive IRQ0 instead of the PIT. A muxer
selects the input for that purpose. And we disable the timer of the PIT
in that case to avoid needless background activity. See e.g.
pit_irq_control().

> 
>    * If you back-migrate from after this change to before this change,
>      maybe there is a risk that it will lose one timer interrupt?
>      Forward migration shouldn't have an issue (the first trailing
>      edge in the new code becomes a nop).  Perhaps hack it to encourage
>      extra interrupts in some migration cases rather than lost interrupts
>      in others?  Some kind of subsection thing like was being
>      discussed for the 8259 earlier?  Or:
> 
> Also, how big of a concern is a very rare gained or lost IRQ0
> actually?  Under normal conditions, I would expect this to at most
> cause a one time clock drift in the guest OS of a fraction of
> a second.  If that only happens when rebooting or migrating the
> guest...
> 
> On the other hand, lost or gained interrupts might be a more
> serious concern (especially if lost) if the 8254 is operating
> in a one-shot mode (0 or 4): Something in the guest doesn't
> stop (hangs) if not canceled by the interrupt.
> 
> 
> Can anyone confirm or contradict any of this?  Other thoughts?

See my other reply: Until we really understand the impact of some
optimization / simplification, we are forced to do it accurately.

Jan

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