Re: [Qemu-devel] [PATCH KVM v2 1/4] KVM: fix i8254 IRQ0 to be normally high

[mmogilvi]

On Mon, 7 Jan 2013 11:39:18 +0200, Gleb Natapov <address@hidden> wrote:
> On Wed, Dec 26, 2012 at 10:39:53PM -0700, Matthew Ogilvie wrote:
>> Reading the spec, it is clear that most modes normally leave the IRQ
>> output line high, and only pulse it low to generate a leading edge.
>> Especially the most commonly used mode 2.
>> 
>> The KVM i8254 model does not try to emulate the duration of the pulse at
>> all, so just swap the high/low settings it to leave it high most of
>> the time.
>> 
>> This fix is a prerequisite to improving the i8259 model to handle
>> the trailing edge of an interupt request as indicated in its spec:
>> If it gets a trailing edge of an IRQ line before it starts to service
>> the interrupt, the request should be canceled.
>> 
>> See http://bochs.sourceforge.net/techspec/intel-82c54-timer.pdf.gz
>> or search the net for 23124406.pdf.
>> 
>> Risks:
>> 
>> There is a risk that migrating a running guest between versions
>> with and without this patch will lose or gain a single timer
>> interrupt during the migration process.  The only case where
> Can you elaborate on how exactly this can happen? Do not see it.
> 

KVM 8254: In the corrected model, when the count expires, the model
briefly pulses output low and then high again, with the low to high
transition being what triggers the interrupt.  In the old model,
when the count expires, the model expects the output line
to already be low, and briefly pulses it high (triggering the
interrupt) and then low again.  But if the line was already
high (because it migrated from the corrected model),
this won't generate a new leading edge (low to high) and won't
trigger a new interrupt (the first post-back-migration pulse turns
into a simple trailing edge instead of a pulse).

Unless there is something I'm missing?

The qemu 8254 model actually models each edge at independent
clock ticks instead of combining both into a very brief pulse at one time.
I've found it handy to draw out old and new timing diagrams on paper
(for each mode), and then carefully think about what happens with respect
to levels and edges when you transition back and forth between old and
new models at various points in the timing cycle.  (Note I've spent more
time examining the qemu models rather than the kvm models.)

>> this is likely to be serious is probably losing a single-shot (mode 4)
>> interrupt, but if my understanding of how things work is good, then
>> that should only be possible if a whole slew of conditions are
>> all met:
>> 
>>  1. The guest is configured to run in a "tickless" mode (like
>>     modern Linux).
>>  2. The guest is for some reason still using the i8254 rather
>>     than something more modern like an HPET.  (The combination
>>     of 1 and 2 should be rare.)
> This is not so rare. For performance reason it is better to not have
> HPET at all.  In fact -no-hpet is how I would advice anyone to run qemu.

In a later email you mention that Linux prefers a timer in the APIC.
I don't know much about the APIC (advanced interrupt controller), and
wasn't
even aware had it's own timer.

The big question is if we can safely just fix the i825* models, or if
we need something more subtle to avoid breaking commonly used guests
like modern Linux (support both corrected and old models,
or only fix IRQ2 instead of all IRQs, or similar subtlety).

> 
>>  3. The migration is going from a fixed version back to the
>>     old version.  (Not sure how common this is, but it should
>>     be rarer than migrating from old to new.)
>>  4. There are not going to be any "timely" events/interrupts
>>     (keyboard, network, process sleeps, etc) that cause the guest
>>     to reset the PIT mode 4 one-shot counter "soon enough".
>> 
>> This combination should be rare enough that more complicated
>> solutions are not worth the effort.
>> 
>> Signed-off-by: Matthew Ogilvie <address@hidden>
>> ---
>>  arch/x86/kvm/i8254.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
>> index c1d30b2..cd4ec60 100644
>> --- a/arch/x86/kvm/i8254.c
>> +++ b/arch/x86/kvm/i8254.c
>> @@ -290,8 +290,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);
>>  
>>              /*
>>               * Provides NMI watchdog support via Virtual Wire mode.
>> -- 
>> 1.7.10.2.484.gcd07cc5
> 
> --
>                       Gleb.