[Qemu-devel] [PATCH KVM v2 0/4] fix KVM i8259 IRQ trailing-edge logic

[Qemu-devel] [PATCH KVM v2 0/4] fix KVM i8259 IRQ trailing-edge logic

[Matthew Ogilvie]

Changes since version 1 (from Sep 9):
   * Split off patch 1; this is the critical prerequisite to
     make the i8254 work with the fixed i8259.
   * Add patch 2, to make additional changes to the i8254
     to make it consistent with the spec and with proposed changes
     to qemu's i8254 model.

Background:

According to the spec, the i8259 will cancel an interrupt if
the line goes low before it starts servicing it, even when
it is considered edge-triggered.  This is a problem
with an old Microport UNIX guest (ca 1987), where the
guest masks off IRQ14 in the slave i8259, but the host's
master i8259 model will still try to deliver an interrupt even after
IRQ2 has been brought low, resulting in a spurious interrupt (IRQ15).

Before the i8259 can be fixed, the i8254 model needs to be fixed
so it doesn't depend on the broken i8259.

Alternative: This could be narrowly fixed for IRQ2 only with no
risk at all (and no need to touch the i8254), but if possible it
seems reasonable to fix it for all lines at the same time.

But there may be some risk:

First, I think I've convinced myself that between the i8254 and i8259,
the only substantial migration breakage should be when a
whole series of conditions are met, and the combination
should be rare enough not to worry about it.  See writeup
in my qemu patch series (version 8; Dec 16).

Second, there is also the possibility that other devices are relying
on the broken model.  I'm especially concerned with various users
of a function called

        qemu_irq_pulse()

in the qemu source tree, which immediately lowers IRQ line after
raising it.  If any of those calls are routed straight into
the i8259 (as opposed to an APIC or some other chip), then it
probably won't behave as desired.

I'll probably dig into qemu_irq_pulse() callers more carefully
eventually, but there are lot of them, and any high-level incite
anyone can provide into them would be helpful.

See the Dec 16 patch series I sent to the qemu mailing list for
more information.
http://search.gmane.org/?query=ogilvie&group=gmane.comp.emulators.qemu

Matthew Ogilvie (4):
  KVM: fix i8254 IRQ0 to be normally high
  KVM: additional i8254 output fixes
  KVM: fix i8259 interrupt high to low transition logic
  KVM: i8259: refactor pic_set_irq level logic

 arch/x86/kvm/i8254.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
 arch/x86/kvm/i8259.c | 36 ++++++++++++++----------------------
 2 files changed, 53 insertions(+), 33 deletions(-)

-- 
1.7.10.2.484.gcd07cc5

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

[Matthew Ogilvie]

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
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.)
 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 <mmogilvi_qemu@miniinfo.net>
---
 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

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

[Gleb Natapov]

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.

> 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.

>  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 <mmogilvi_qemu@miniinfo.net>
> ---
>  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.

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

[Gleb Natapov]

On Mon, Jan 07, 2013 at 11:39:18AM +0200, Gleb Natapov 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.
> 
> > 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.
> 
It looks like Linux prefer to use APIC timer anyway.

> >  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 <mmogilvi_qemu@miniinfo.net>
> > ---
> >  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.

--
			Gleb.

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 <gleb@redhat.com> 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 <mmogilvi_qemu@miniinfo.net>
>> ---
>>  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.

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

[Gleb Natapov]

On Mon, Jan 07, 2013 at 06:17:22PM -0600, mmogilvi@miniinfo.net wrote:
> On Mon, 7 Jan 2013 11:39:18 +0200, Gleb Natapov <gleb@redhat.com> 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?
> 
No, I missed that pic->last_irr/ioapic->irr  will be migrated as 1. But
this means that next interrupt after migration from new to old will
always be lost.  What about clearing pit bit from last_irr/irr before
migration? Should not affect new->new migration and should fix new->old
one. The only problem is that we may need to consult irq routing table
to know how pit is connected to ioapic.

Still do not see how can we gain one interrupt.

> 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.)
> 
Yes, drawing it definitely helps :)

> >> 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).
Migration may happen while guest is running firmaware. Who knows what
those are doing. If the fix is as easy as I described above we should go
for it.

> 
> > 
> >>  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 <mmogilvi_qemu@miniinfo.net>
> >> ---
> >>  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.

--
			Gleb.

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

[Matthew Ogilvie]

On Tue, Jan 08, 2013 at 09:31:19AM +0200, Gleb Natapov wrote:
> On Mon, Jan 07, 2013 at 06:17:22PM -0600, mmogilvi@miniinfo.net wrote:
> > On Mon, 7 Jan 2013 11:39:18 +0200, Gleb Natapov <gleb@redhat.com> 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?
> > 
> No, I missed that pic->last_irr/ioapic->irr  will be migrated as 1. But
> this means that next interrupt after migration from new to old will
> always be lost.  What about clearing pit bit from last_irr/irr before
> migration? Should not affect new->new migration and should fix new->old
> one. The only problem is that we may need to consult irq routing table
> to know how pit is connected to ioapic.

We should not clear both last_irr and irr.  That
cancels the interrupt early if the CPU hasn't started servicing
it yet:  If the guest CPU has interrupts disabled
and hasn't begun to service the interrupt, a new->new migration could
lose the interrupt much earlier in the countdown cycle than it otherwise
might be lost.

Potentially we could clear the last_irr bit only.  This effectively
makes migration behave like the old unfixed code.  But if this is
considered acceptable, I would be more inclined to not fix IRQ0 at all,
rather than make IRQ0 work subtly differently normally vs during
migration.  One of my earlier patch series (qemu v7 dated Nov 25)
attempts to use basically this strategy for the qemu model, at
least in the short term (and then potentially fix it properly
in the longer term), although the details of series might
be tweaked.

Or the minimal-risk strategy is to ONLY fix the cascade IRQ2's
trailing edge [the original i825* problem that spawned this whole
thing], leaving all other IRQs as-is.

> 
> Still do not see how can we gain one interrupt.

In most cases I don't think we get an extra interrupt from
a direct fix.  But some kinds of attempts to work around missing
interrupts during migration can cause cause extra interrupts in other
cases.  In the old qemu model (but perhaps not kvm), the mode 2 leading
edge occurs one clock tick earlier than it ought to.  So if you
try to be tricky with adjusting levels during migration, you
may introduce possible cases where it gets an interrupt in
the old model, then migrates and gets another interrupt one tick
later in the new model...

Also, it occurs to me that maybe there might be an off-by-one issue in
the kvm model of mode 2 that ought to be fixed as well?  Although the  
zero length pulse in kvm suggests that one-off issues in counters do  
not matter.  In the qemu model, the leading edge of OUT in mode 2
moves by one tick...

> 
> > 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.)
> > 
> Yes, drawing it definitely helps :)
> 
> > >> 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).
> Migration may happen while guest is running firmaware. Who knows what
> those are doing. If the fix is as easy as I described above we should go
> for it.
> 
> > 
> > > 
> > >>  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 <mmogilvi_qemu@miniinfo.net>
> > >> ---
> > >>  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.
> 
> --
> 			Gleb.

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

[Gleb Natapov]

On Thu, Jan 10, 2013 at 11:40:07PM -0700, Matthew Ogilvie wrote:
> On Tue, Jan 08, 2013 at 09:31:19AM +0200, Gleb Natapov wrote:
> > On Mon, Jan 07, 2013 at 06:17:22PM -0600, mmogilvi@miniinfo.net wrote:
> > > On Mon, 7 Jan 2013 11:39:18 +0200, Gleb Natapov <gleb@redhat.com> 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?
> > > 
> > No, I missed that pic->last_irr/ioapic->irr  will be migrated as 1. But
> > this means that next interrupt after migration from new to old will
> > always be lost.  What about clearing pit bit from last_irr/irr before
> > migration? Should not affect new->new migration and should fix new->old
> > one. The only problem is that we may need to consult irq routing table
> > to know how pit is connected to ioapic.
> 
> We should not clear both last_irr and irr.  That
> cancels the interrupt early if the CPU hasn't started servicing
> it yet:  If the guest CPU has interrupts disabled
> and hasn't begun to service the interrupt, a new->new migration could
> lose the interrupt much earlier in the countdown cycle than it otherwise
> might be lost.
> 
I am talking about last_irr in pic and irr in ioapic. Of course we
shouldn't clear pic->irr on migration. ioapic uses irr to detect edge.

> Potentially we could clear the last_irr bit only.  This effectively
> makes migration behave like the old unfixed code.  But if this is
> considered acceptable, I would be more inclined to not fix IRQ0 at all,
If we do not fix IRQ0 next timer interrupt after migration will always be
lost.

> rather than make IRQ0 work subtly differently normally vs during
> migration.  One of my earlier patch series (qemu v7 dated Nov 25)
> attempts to use basically this strategy for the qemu model, at
> least in the short term (and then potentially fix it properly
> in the longer term), although the details of series might
> be tweaked.
> 
> Or the minimal-risk strategy is to ONLY fix the cascade IRQ2's
> trailing edge [the original i825* problem that spawned this whole
> thing], leaving all other IRQs as-is.
> 
> > 
> > Still do not see how can we gain one interrupt.
> 
> In most cases I don't think we get an extra interrupt from
> a direct fix.  But some kinds of attempts to work around missing
> interrupts during migration can cause cause extra interrupts in other
> cases.  In the old qemu model (but perhaps not kvm), the mode 2 leading
> edge occurs one clock tick earlier than it ought to.  So if you
> try to be tricky with adjusting levels during migration, you
> may introduce possible cases where it gets an interrupt in
> the old model, then migrates and gets another interrupt one tick
> later in the new model...
> 
> Also, it occurs to me that maybe there might be an off-by-one issue in
> the kvm model of mode 2 that ought to be fixed as well?  Although the  
> zero length pulse in kvm suggests that one-off issues in counters do  
> not matter.  In the qemu model, the leading edge of OUT in mode 2
> moves by one tick...
> 
> > 
> > > 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.)
> > > 
> > Yes, drawing it definitely helps :)
> > 
> > > >> 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).
> > Migration may happen while guest is running firmaware. Who knows what
> > those are doing. If the fix is as easy as I described above we should go
> > for it.
> > 
> > > 
> > > > 
> > > >>  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 <mmogilvi_qemu@miniinfo.net>
> > > >> ---
> > > >>  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.
> > 
> > --
> > 			Gleb.

--
			Gleb.

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

[Matthew Ogilvie]

On Fri, Jan 11, 2013 at 05:45:28PM +0200, Gleb Natapov wrote:
> On Thu, Jan 10, 2013 at 11:40:07PM -0700, Matthew Ogilvie wrote:
> > On Tue, Jan 08, 2013 at 09:31:19AM +0200, Gleb Natapov wrote:
> > > On Mon, Jan 07, 2013 at 06:17:22PM -0600, mmogilvi@miniinfo.net wrote:
> > > > On Mon, 7 Jan 2013 11:39:18 +0200, Gleb Natapov <gleb@redhat.com> 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?
> > > > 
> > > No, I missed that pic->last_irr/ioapic->irr  will be migrated as 1. But
> > > this means that next interrupt after migration from new to old will
> > > always be lost.  What about clearing pit bit from last_irr/irr before
> > > migration? Should not affect new->new migration and should fix new->old
> > > one. The only problem is that we may need to consult irq routing table
> > > to know how pit is connected to ioapic.
> > 
> > We should not clear both last_irr and irr.  That
> > cancels the interrupt early if the CPU hasn't started servicing
> > it yet:  If the guest CPU has interrupts disabled
> > and hasn't begun to service the interrupt, a new->new migration could
> > lose the interrupt much earlier in the countdown cycle than it otherwise
> > might be lost.
> > 
> I am talking about last_irr in pic and irr in ioapic. Of course we
> shouldn't clear pic->irr on migration. ioapic uses irr to detect edge.

I probably need to look into the details of how the ioapic is supposed
to work.  Sigh.

> 
> > Potentially we could clear the last_irr bit only.  This effectively
> > makes migration behave like the old unfixed code.  But if this is
> > considered acceptable, I would be more inclined to not fix IRQ0 at all,
> If we do not fix IRQ0 next timer interrupt after migration will always be
> lost.

Obviously true if the trailing edge consumption logic for the
IRQ0 line is corrected in the 8259.  But if it isn't:

I probably could have been clearer that we could leave the 8254 unchanged,
and adjust the 8259 fix to leave IRQ0 as-is (with the incorrect handling
of a trailing edge - only other IRQ lines would be fixed), and then
timer interrupts would just work exactly as they do now.
This minimal fix would would probably be the lowest risk of
breaking something that currently works, but I don't know if
people would go for a patch that intentionally leaves in known
breakage in IRQ0...  This is option 1 below.

------

If we cleared last_irr in the qemu model during migration,
we risk getting an EXTRA interrupt when
migrating mode 4 (misremembered as mode 2 in an earlier
email below) from old to new models.  (If the sequence goes: line
is asserted, guest migrates, interrupt is handled [all in less than
a 8254 clock tick (approx 1 MHz)], then the off-by-one edge
in the new model triggers another leading edge...)  In contrast,
this might not be a risk in the KVM model as currently implemented.

Maybe this objection is not important, and you like option 4
listed below.

-------

So I'm still not sure what overall fix strategy the main qemu and kvm
maintainers would like the best.  There are several possibilities,
but they all seem to have notable downsides.

Some possible fix strategies:

1. Only fix the IRQ2 (cascade) line in 8259.  Leave trailing edge
   logic for other lines as-is, and don't touch the 8254.  (Or
   similar: only fix lines that don't risk breaking
   anything [everything except IRQ0?])
    * For: This has no risk of breaking anything that currently
      works, and fixes my own problem.
    * Against: It somehow feels wrong to intentionally leave incorrect
      models for IRQ0 (both generation and consumption).  It also feels
      wrong to add more assumptions about PC-style
      slave/master PIC and PIT wiring in the device model code.
2. Just fix the 8254 and 8259 directly, with nothing fancy for
   compatibility.  My v8 (Dec 16) patch series does this.
    * For: simple
    * For: A single lost interrupt during migration between versions
      is probably not serious.
    * Against: Unless it looses a one-shot mode interrupt, like with
      a "tickless" Linux kernel.  (Although if the guest gets
      some other interrupt "soon", and it resets the 8254
      as part of handling that interrupt, then maybe
      this isn't serious either.)
    * For: But on the other hand, I get the impression that
      anything new enough to do such a "tickless" thing is
      likely to prefer using some other timer besides the 8254.  But
      maybe this impression is wrong?
3. Fix it in stages, with years between stages, where it can safely
   migrate between adjacent stages, but not as safely migrate between
   skipped stages.  My v7 (Nov 25) patch series was attempting one way of
   doing this, although there are probably other possibilities.
4. Fix the 8254 and 8259, but add in some migration logic to force
   IRQ0's pic->last_irr bit to 0 when migrating.
    * For: Avoid lost interrupts.
    * Against: This basically sets the migration state as if we did
      not fix the IRQ0 line trailing edge handling consumption
      logic in the 8259 at all, which is subtly different from
      normal operation.  I would tend prefer to avoid making
      post-migration state any different from normal
      operation state.
    * For: But maybe this could be the first stage of a
      multi-stage approach, where we can eventually
      eliminate the special logic in the migration someday
      in the future (when migrating to versions with the old
      models is no longer something we care about).  So the
      ugly inconsistency would only be in the code for a
      few years.  (See also strategy 3 above.)
    * Against: In the qemu model (but not the kvm model), clearing
      last_irr may occasionally cause an EXTRA interrupt for mode
      4 during part of the output cycle.  [Sequence: Old version
      raises IRQ0 one clock tick early, guest migrates, interrupt
      is handled, then new version raises IRQ0 at the correct
      time...]
    * Question: How to handle the migration hack for the
      KVM model?  It may needs to do this correction both in
      the kernel and in qemu, which feels ugly.   Or potentially
      add new ioctl()'s (and fallback code if the ioctl is
      missing) to support both the "compatible" state and
      the "actual" state, but then that is drifting towards
      option 5 below:
5. Make the IRQ0 generation/consumption behavior user-selectable
   with a command line option (the cross-version migration "promise"
   is allowed to be broken if qemu is invoked with different options).
   Sometime in the future, perhaps change the default, and/or
   eliminate the old option.
    * For: By defaulting to current model behavior, we could avoid
      breaking anything that currently depends on current
      behavior in hard-to-forsee ways.
    * Against: More complicated code, and depending on details
      may either be slightly slower (more conditional branches in common
      code paths), or even more complicated code (another level
      of abstraction and more C-style virtual methods...).
    * Against: The non-default case probably wont be tested much,
      and risks bit-rotting.
    * Question: What should the command line option look like?


Anyone have any preference for any of these?  Or other alternatives?

-------------------
Note that my tentative assignment of requirement priorities is
as described below.

High priority goals:

1. Handle the "trailing edge" of IRQ2 in the qemu 8259 model correctly,
   specifically to fix the case of a guest (Microport UNIX) that
   dynamically manipulated the IMR (mask register) of the slave
   while an IRQ line is asserted and unhandled.
2. Do not break any common use cases, at least not badly (perhaps
   a single lost or gained interrupt would be OK as long as it
   doesn't cause longer term breakage in a guest).
    * all device models should still work
    * basic operation of modern/common guest operating systems
    * cross version migration when KVM is in use

Moderate priority goals (I wouldn't mind if these
weren't met, but I would like to try to meet them):

3. Support cross version migration with plain qemu (not KVM). (or
   is this high priority like KVM migration?)
4. Completely fix all IRQ lines in both 8259 models.  (I only really need
   the cascade line (IRQ2) to be fixed.)
5. Update the KVM models to continue to match the qemu models, at least
   to the same extent they are similar now.  (I don't actually care about
   the KVM 825* models much, but I would like to avoid making them
   any less consistent.  [see also 7 below])
6. Improve the output logic of 8254 models to match the spec better,
   instead of depending on the broken trailing edge logic in the
   8259 models in order to work.  And as long as it is changing,
   fix some other small related details, like the exact clock
   tick on which edges occur, some GATE stuff, etc.

Low priority (non)-goals (I'm not inclined to worry about these):

7. Enhance the KVM 8254 model to be able to model the leading and trailing
   edges of the output line at distinct times (similar to the
   qemu 8254 model).  Perhaps it only does this when it isn't
   trying to "catch up" with missed interrupts.  It could still
   do zero-time pulses when catching up.
8. Accurately model the 8254's GATE line.  PC hardware hardwires it
   in the main channel 0, so modeling changes to it isn't
   particularly important.  Currently the models don't have
   a good way to accurately model a paused countdown when GATE is low.
    * (exception): However, there is already some imperfect handling
      of GATE in the 8254 models, and the speaker channel has a
      software-controlled GATE.  So I don't want to make GATE any
      worse than it already is, and if I can make partial improvements
      by changing just a couple of lines, I'm inclined to do so as
      part of item 6 above.


- Matthew Ogilvie

> 
> > rather than make IRQ0 work subtly differently normally vs during
> > migration.  One of my earlier patch series (qemu v7 dated Nov 25)
> > attempts to use basically this strategy for the qemu model, at
> > least in the short term (and then potentially fix it properly
> > in the longer term), although the details of series might
> > be tweaked.
> > 
> > Or the minimal-risk strategy is to ONLY fix the cascade IRQ2's
> > trailing edge [the original i825* problem that spawned this whole
> > thing], leaving all other IRQs as-is.
> > 
> > > 
> > > Still do not see how can we gain one interrupt.
> > 
> > In most cases I don't think we get an extra interrupt from
> > a direct fix.  But some kinds of attempts to work around missing
> > interrupts during migration can cause cause extra interrupts in other
> > cases.  In the old qemu model (but perhaps not kvm), the mode 2 leading
> > edge occurs one clock tick earlier than it ought to.  So if you
> > try to be tricky with adjusting levels during migration, you
> > may introduce possible cases where it gets an interrupt in
> > the old model, then migrates and gets another interrupt one tick
> > later in the new model...
> > 
> > Also, it occurs to me that maybe there might be an off-by-one issue in
> > the kvm model of mode 2 that ought to be fixed as well?  Although the  
> > zero length pulse in kvm suggests that one-off issues in counters do  
> > not matter.  In the qemu model, the leading edge of OUT in mode 2
> > moves by one tick...
> > 
> > > 
> > > > 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.)
> > > > 
> > > Yes, drawing it definitely helps :)
> > > 
> > > > >> 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).
> > > Migration may happen while guest is running firmaware. Who knows what
> > > those are doing. If the fix is as easy as I described above we should go
> > > for it.
> > > 
> > > > 
> > > > > 
> > > > >>  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 <mmogilvi_qemu@miniinfo.net>
> > > > >> ---
> > > > >>  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.
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.

[Qemu-devel] [PATCH KVM v2 2/4] KVM: additional i8254 output fixes

[Matthew Ogilvie]

Make git_get_out() consistent with spec.  Currently pit_get_out()
doesn't affect IRQ0, but it can be read by the guest in other ways.
This makes it consistent with proposed changes in qemu's i8254 model
as well.

See http://bochs.sourceforge.net/techspec/intel-82c54-timer.pdf.gz
or search the net for 23124406.pdf.

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index cd4ec60..fd38938 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -144,6 +144,10 @@ static int pit_get_count(struct kvm *kvm, int channel)
 
 	WARN_ON(!mutex_is_locked(&kvm->arch.vpit->pit_state.lock));
 
+	/* FIXME: Add some way to represent a paused timer and return
+	 *   the paused-at counter value, to better model gate pausing,
+	 *   "wait until next CLK pulse to load counter" logic, etc.
+	 */
 	t = kpit_elapsed(kvm, c, channel);
 	d = muldiv64(t, KVM_PIT_FREQ, NSEC_PER_SEC);
 
@@ -155,8 +159,7 @@ static int pit_get_count(struct kvm *kvm, int channel)
 		counter = (c->count - d) & 0xffff;
 		break;
 	case 3:
-		/* XXX: may be incorrect for odd counts */
-		counter = c->count - (mod_64((2 * d), c->count));
+		counter = (c->count - (mod_64((2 * d), c->count))) & 0xfffe;
 		break;
 	default:
 		counter = c->count - mod_64(d, c->count);
@@ -180,20 +183,18 @@ static int pit_get_out(struct kvm *kvm, int channel)
 	switch (c->mode) {
 	default:
 	case 0:
-		out = (d >= c->count);
-		break;
 	case 1:
-		out = (d < c->count);
+		out = (d >= c->count);
 		break;
 	case 2:
-		out = ((mod_64(d, c->count) == 0) && (d != 0));
+		out = (mod_64(d, c->count) != (c->count - 1) || c->gate == 0);
 		break;
 	case 3:
-		out = (mod_64(d, c->count) < ((c->count + 1) >> 1));
+		out = (mod_64(d, c->count) < ((c->count + 1) >> 1) || c->gate == 0);
 		break;
 	case 4:
 	case 5:
-		out = (d == c->count);
+		out = (d != c->count);
 		break;
 	}
 
@@ -367,7 +368,7 @@ static void pit_load_count(struct kvm *kvm, int channel, u32 val)
 
 	/*
 	 * The largest possible initial count is 0; this is equivalent
-	 * to 216 for binary counting and 104 for BCD counting.
+	 * to pow(2,16) for binary counting and pow(10,4) for BCD counting.
 	 */
 	if (val == 0)
 		val = 0x10000;
@@ -376,6 +377,26 @@ static void pit_load_count(struct kvm *kvm, int channel, u32 val)
 
 	if (channel != 0) {
 		ps->channels[channel].count_load_time = ktime_get();
+
+		/* In gate-triggered one-shot modes,
+		 * indirectly model some pit_get_out()
+		 * cases by setting the load time way
+		 * back until gate-triggered.
+		 * (Generally only affects reading status
+		 * from channel 2 speaker,
+		 * due to hard-wired gates on other
+		 * channels.)
+		 *
+		 * FIXME: This might be redesigned if a paused
+		 * timer state is added for pit_get_count().
+		 */
+		if (ps->channels[channel].mode == 1 ||
+		    ps->channels[channel].mode == 5) {
+	            u64 delta = muldiv64(val+2, NSEC_PER_SEC, KVM_PIT_FREQ);
+		    ps->channels[channel].count_load_time =
+                            ktime_sub(ps->channels[channel].count_load_time,
+                                      ns_to_ktime(delta));
+		}
 		return;
 	}
 
@@ -383,7 +404,6 @@ static void pit_load_count(struct kvm *kvm, int channel, u32 val)
 	 * mode 1 is one shot, mode 2 is period, otherwise del timer */
 	switch (ps->channels[0].mode) {
 	case 0:
-	case 1:
         /* FIXME: enhance mode 4 precision */
 	case 4:
 		create_pit_timer(kvm, val, 0);
@@ -393,6 +413,10 @@ static void pit_load_count(struct kvm *kvm, int channel, u32 val)
 		create_pit_timer(kvm, val, 1);
 		break;
 	default:
+		/* Modes 1 and 5 are triggered by gate leading edge,
+		 * but channel 0's gate is hard-wired high and has
+		 * no edges (on normal real hardware).
+		 */
 		destroy_pit_timer(kvm->arch.vpit);
 	}
 }
-- 
1.7.10.2.484.gcd07cc5

Re: [Qemu-devel] [PATCH KVM v2 2/4] KVM: additional i8254 output fixes

[Gleb Natapov]

On Wed, Dec 26, 2012 at 10:39:54PM -0700, Matthew Ogilvie wrote:
> Make git_get_out() consistent with spec.  Currently pit_get_out()
> doesn't affect IRQ0, but it can be read by the guest in other ways.
> This makes it consistent with proposed changes in qemu's i8254 model
> as well.
> 
> See http://bochs.sourceforge.net/techspec/intel-82c54-timer.pdf.gz
> or search the net for 23124406.pdf.
> 
> Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
> ---
>  arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index cd4ec60..fd38938 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -144,6 +144,10 @@ static int pit_get_count(struct kvm *kvm, int channel)
>  
>  	WARN_ON(!mutex_is_locked(&kvm->arch.vpit->pit_state.lock));
>  
> +	/* FIXME: Add some way to represent a paused timer and return
> +	 *   the paused-at counter value, to better model gate pausing,
> +	 *   "wait until next CLK pulse to load counter" logic, etc.
> +	 */
>  	t = kpit_elapsed(kvm, c, channel);
>  	d = muldiv64(t, KVM_PIT_FREQ, NSEC_PER_SEC);
>  
> @@ -155,8 +159,7 @@ static int pit_get_count(struct kvm *kvm, int channel)
>  		counter = (c->count - d) & 0xffff;
>  		break;
>  	case 3:
> -		/* XXX: may be incorrect for odd counts */
> -		counter = c->count - (mod_64((2 * d), c->count));
> +		counter = (c->count - (mod_64((2 * d), c->count))) & 0xfffe;
>  		break;
>  	default:
>  		counter = c->count - mod_64(d, c->count);
> @@ -180,20 +183,18 @@ static int pit_get_out(struct kvm *kvm, int channel)
>  	switch (c->mode) {
>  	default:
>  	case 0:
> -		out = (d >= c->count);
> -		break;
>  	case 1:
> -		out = (d < c->count);
> +		out = (d >= c->count);
>  		break;
>  	case 2:
> -		out = ((mod_64(d, c->count) == 0) && (d != 0));
> +		out = (mod_64(d, c->count) != (c->count - 1) || c->gate == 0);
>  		break;
>  	case 3:
> -		out = (mod_64(d, c->count) < ((c->count + 1) >> 1));
> +		out = (mod_64(d, c->count) < ((c->count + 1) >> 1) || c->gate == 0);
>  		break;
>  	case 4:
>  	case 5:
> -		out = (d == c->count);
> +		out = (d != c->count);
>  		break;
>  	}
>  
> @@ -367,7 +368,7 @@ static void pit_load_count(struct kvm *kvm, int channel, u32 val)
>  
>  	/*
>  	 * The largest possible initial count is 0; this is equivalent
> -	 * to 216 for binary counting and 104 for BCD counting.
> +	 * to pow(2,16) for binary counting and pow(10,4) for BCD counting.
>  	 */
>  	if (val == 0)
>  		val = 0x10000;
> @@ -376,6 +377,26 @@ static void pit_load_count(struct kvm *kvm, int channel, u32 val)
>  
>  	if (channel != 0) {
>  		ps->channels[channel].count_load_time = ktime_get();
> +
> +		/* In gate-triggered one-shot modes,
> +		 * indirectly model some pit_get_out()
> +		 * cases by setting the load time way
> +		 * back until gate-triggered.
> +		 * (Generally only affects reading status
> +		 * from channel 2 speaker,
> +		 * due to hard-wired gates on other
> +		 * channels.)
> +		 *
> +		 * FIXME: This might be redesigned if a paused
> +		 * timer state is added for pit_get_count().
> +		 */
> +		if (ps->channels[channel].mode == 1 ||
> +		    ps->channels[channel].mode == 5) {
> +	            u64 delta = muldiv64(val+2, NSEC_PER_SEC, KVM_PIT_FREQ);
> +		    ps->channels[channel].count_load_time =
> +                            ktime_sub(ps->channels[channel].count_load_time,
> +                                      ns_to_ktime(delta));
I do not understand what are you trying to do here. You assume that
trigger will happen 2 clocks after counter is loaded?

> +		}
>  		return;
>  	}
>  
> @@ -383,7 +404,6 @@ static void pit_load_count(struct kvm *kvm, int channel, u32 val)
>  	 * mode 1 is one shot, mode 2 is period, otherwise del timer */
>  	switch (ps->channels[0].mode) {
>  	case 0:
> -	case 1:
>          /* FIXME: enhance mode 4 precision */
>  	case 4:
>  		create_pit_timer(kvm, val, 0);
> @@ -393,6 +413,10 @@ static void pit_load_count(struct kvm *kvm, int channel, u32 val)
>  		create_pit_timer(kvm, val, 1);
>  		break;
>  	default:
> +		/* Modes 1 and 5 are triggered by gate leading edge,
> +		 * but channel 0's gate is hard-wired high and has
> +		 * no edges (on normal real hardware).
> +		 */
>  		destroy_pit_timer(kvm->arch.vpit);
>  	}
>  }
> -- 
> 1.7.10.2.484.gcd07cc5

--
			Gleb.

Re: [Qemu-devel] [PATCH KVM v2 2/4] KVM: additional i8254 output fixes

[mmogilvi]

On Mon, 7 Jan 2013 14:04:03 +0200, Gleb Natapov <gleb@redhat.com> wrote:
> On Wed, Dec 26, 2012 at 10:39:54PM -0700, Matthew Ogilvie wrote:
>> Make git_get_out() consistent with spec.  Currently pit_get_out()
>> doesn't affect IRQ0, but it can be read by the guest in other ways.
>> This makes it consistent with proposed changes in qemu's i8254 model
>> as well.
>> 
>> See http://bochs.sourceforge.net/techspec/intel-82c54-timer.pdf.gz
>> or search the net for 23124406.pdf.
>> 
>> Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
>> ---
>>  arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 34 insertions(+), 10 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
>> index cd4ec60..fd38938 100644
>> --- a/arch/x86/kvm/i8254.c
>> +++ b/arch/x86/kvm/i8254.c
>> @@ -144,6 +144,10 @@ static int pit_get_count(struct kvm *kvm, int
>> channel)
>>  
>>  	WARN_ON(!mutex_is_locked(&kvm->arch.vpit->pit_state.lock));
>>  
>> +	/* FIXME: Add some way to represent a paused timer and return
>> +	 *   the paused-at counter value, to better model gate pausing,
>> +	 *   "wait until next CLK pulse to load counter" logic, etc.
>> +	 */
>>  	t = kpit_elapsed(kvm, c, channel);
>>  	d = muldiv64(t, KVM_PIT_FREQ, NSEC_PER_SEC);
>>  
>> @@ -155,8 +159,7 @@ static int pit_get_count(struct kvm *kvm, int
>> channel)
>>  		counter = (c->count - d) & 0xffff;
>>  		break;
>>  	case 3:
>> -		/* XXX: may be incorrect for odd counts */
>> -		counter = c->count - (mod_64((2 * d), c->count));
>> +		counter = (c->count - (mod_64((2 * d), c->count))) & 0xfffe;
>>  		break;
>>  	default:
>>  		counter = c->count - mod_64(d, c->count);
>> @@ -180,20 +183,18 @@ static int pit_get_out(struct kvm *kvm, int
>> channel)
>>  	switch (c->mode) {
>>  	default:
>>  	case 0:
>> -		out = (d >= c->count);
>> -		break;
>>  	case 1:
>> -		out = (d < c->count);
>> +		out = (d >= c->count);
>>  		break;
>>  	case 2:
>> -		out = ((mod_64(d, c->count) == 0) && (d != 0));
>> +		out = (mod_64(d, c->count) != (c->count - 1) || c->gate == 0);
>>  		break;
>>  	case 3:
>> -		out = (mod_64(d, c->count) < ((c->count + 1) >> 1));
>> +		out = (mod_64(d, c->count) < ((c->count + 1) >> 1) || c->gate == 0);
>>  		break;
>>  	case 4:
>>  	case 5:
>> -		out = (d == c->count);
>> +		out = (d != c->count);
>>  		break;
>>  	}
>>  
>> @@ -367,7 +368,7 @@ static void pit_load_count(struct kvm *kvm, int
>> channel, u32 val)
>>  
>>  	/*
>>  	 * The largest possible initial count is 0; this is equivalent
>> -	 * to 216 for binary counting and 104 for BCD counting.
>> +	 * to pow(2,16) for binary counting and pow(10,4) for BCD counting.
>>  	 */
>>  	if (val == 0)
>>  		val = 0x10000;
>> @@ -376,6 +377,26 @@ static void pit_load_count(struct kvm *kvm, int
>> channel, u32 val)
>>  
>>  	if (channel != 0) {
>>  		ps->channels[channel].count_load_time = ktime_get();
>> +
>> +		/* In gate-triggered one-shot modes,
>> +		 * indirectly model some pit_get_out()
>> +		 * cases by setting the load time way
>> +		 * back until gate-triggered.
>> +		 * (Generally only affects reading status
>> +		 * from channel 2 speaker,
>> +		 * due to hard-wired gates on other
>> +		 * channels.)
>> +		 *
>> +		 * FIXME: This might be redesigned if a paused
>> +		 * timer state is added for pit_get_count().
>> +		 */
>> +		if (ps->channels[channel].mode == 1 ||
>> +		    ps->channels[channel].mode == 5) {
>> +	            u64 delta = muldiv64(val+2, NSEC_PER_SEC, KVM_PIT_FREQ);
>> +		    ps->channels[channel].count_load_time =
>> +                           
>> ktime_sub(ps->channels[channel].count_load_time,
>> +                                      ns_to_ktime(delta));
> I do not understand what are you trying to do here. You assume that
> trigger will happen 2 clocks after counter is loaded?
> 

Modes 1 and 5 are single-shot, and they do not start counting until GATE
is triggered, potentially well after count is loaded.  So this is
attempting to model the "start of countdown has not been triggered"
state as being mostly identical to the "already triggered and also
expired some number of clocks (2) ago" state.

It might be clearer to have a way to explicitly model a
paused countdown, but such a mechanism doesn't currently exist.

Note that modeling modes 1 and 5 is fairly low priority,
because channel 0's GATE line is generally hard-wired such that
GATE edges/triggers are impossible.  But it may still be
somewhat relevant to the PC speaker channel, or if someone
might want to use this in a model of non-PC hardware.

>> +		}
>>  		return;
>>  	}
>>  
>> @@ -383,7 +404,6 @@ static void pit_load_count(struct kvm *kvm, int
>> channel, u32 val)
>>  	 * mode 1 is one shot, mode 2 is period, otherwise del timer */
>>  	switch (ps->channels[0].mode) {
>>  	case 0:
>> -	case 1:
>>          /* FIXME: enhance mode 4 precision */
>>  	case 4:
>>  		create_pit_timer(kvm, val, 0);
>> @@ -393,6 +413,10 @@ static void pit_load_count(struct kvm *kvm, int
>> channel, u32 val)
>>  		create_pit_timer(kvm, val, 1);
>>  		break;
>>  	default:
>> +		/* Modes 1 and 5 are triggered by gate leading edge,
>> +		 * but channel 0's gate is hard-wired high and has
>> +		 * no edges (on normal real hardware).
>> +		 */
>>  		destroy_pit_timer(kvm->arch.vpit);
>>  	}
>>  }
>> -- 
>> 1.7.10.2.484.gcd07cc5
> 
> --
> 			Gleb.

Re: [Qemu-devel] [PATCH KVM v2 2/4] KVM: additional i8254 output fixes

[Gleb Natapov]

On Mon, Jan 07, 2013 at 06:35:47PM -0600, mmogilvi@miniinfo.net wrote:
> On Mon, 7 Jan 2013 14:04:03 +0200, Gleb Natapov <gleb@redhat.com> wrote:
> > On Wed, Dec 26, 2012 at 10:39:54PM -0700, Matthew Ogilvie wrote:
> >> Make git_get_out() consistent with spec.  Currently pit_get_out()
> >> doesn't affect IRQ0, but it can be read by the guest in other ways.
> >> This makes it consistent with proposed changes in qemu's i8254 model
> >> as well.
> >> 
> >> See http://bochs.sourceforge.net/techspec/intel-82c54-timer.pdf.gz
> >> or search the net for 23124406.pdf.
> >> 
> >> Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
> >> ---
> >>  arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++++++----------
> >>  1 file changed, 34 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> >> index cd4ec60..fd38938 100644
> >> --- a/arch/x86/kvm/i8254.c
> >> +++ b/arch/x86/kvm/i8254.c
> >> @@ -144,6 +144,10 @@ static int pit_get_count(struct kvm *kvm, int
> >> channel)
> >>  
> >>  	WARN_ON(!mutex_is_locked(&kvm->arch.vpit->pit_state.lock));
> >>  
> >> +	/* FIXME: Add some way to represent a paused timer and return
> >> +	 *   the paused-at counter value, to better model gate pausing,
> >> +	 *   "wait until next CLK pulse to load counter" logic, etc.
> >> +	 */
> >>  	t = kpit_elapsed(kvm, c, channel);
> >>  	d = muldiv64(t, KVM_PIT_FREQ, NSEC_PER_SEC);
> >>  
> >> @@ -155,8 +159,7 @@ static int pit_get_count(struct kvm *kvm, int
> >> channel)
> >>  		counter = (c->count - d) & 0xffff;
> >>  		break;
> >>  	case 3:
> >> -		/* XXX: may be incorrect for odd counts */
> >> -		counter = c->count - (mod_64((2 * d), c->count));
> >> +		counter = (c->count - (mod_64((2 * d), c->count))) & 0xfffe;
> >>  		break;
> >>  	default:
> >>  		counter = c->count - mod_64(d, c->count);
> >> @@ -180,20 +183,18 @@ static int pit_get_out(struct kvm *kvm, int
> >> channel)
> >>  	switch (c->mode) {
> >>  	default:
> >>  	case 0:
> >> -		out = (d >= c->count);
> >> -		break;
> >>  	case 1:
> >> -		out = (d < c->count);
> >> +		out = (d >= c->count);
> >>  		break;
> >>  	case 2:
> >> -		out = ((mod_64(d, c->count) == 0) && (d != 0));
> >> +		out = (mod_64(d, c->count) != (c->count - 1) || c->gate == 0);
> >>  		break;
> >>  	case 3:
> >> -		out = (mod_64(d, c->count) < ((c->count + 1) >> 1));
> >> +		out = (mod_64(d, c->count) < ((c->count + 1) >> 1) || c->gate == 0);
> >>  		break;
> >>  	case 4:
> >>  	case 5:
> >> -		out = (d == c->count);
> >> +		out = (d != c->count);
> >>  		break;
> >>  	}
> >>  
> >> @@ -367,7 +368,7 @@ static void pit_load_count(struct kvm *kvm, int
> >> channel, u32 val)
> >>  
> >>  	/*
> >>  	 * The largest possible initial count is 0; this is equivalent
> >> -	 * to 216 for binary counting and 104 for BCD counting.
> >> +	 * to pow(2,16) for binary counting and pow(10,4) for BCD counting.
> >>  	 */
> >>  	if (val == 0)
> >>  		val = 0x10000;
> >> @@ -376,6 +377,26 @@ static void pit_load_count(struct kvm *kvm, int
> >> channel, u32 val)
> >>  
> >>  	if (channel != 0) {
> >>  		ps->channels[channel].count_load_time = ktime_get();
> >> +
> >> +		/* In gate-triggered one-shot modes,
> >> +		 * indirectly model some pit_get_out()
> >> +		 * cases by setting the load time way
> >> +		 * back until gate-triggered.
> >> +		 * (Generally only affects reading status
> >> +		 * from channel 2 speaker,
> >> +		 * due to hard-wired gates on other
> >> +		 * channels.)
> >> +		 *
> >> +		 * FIXME: This might be redesigned if a paused
> >> +		 * timer state is added for pit_get_count().
> >> +		 */
> >> +		if (ps->channels[channel].mode == 1 ||
> >> +		    ps->channels[channel].mode == 5) {
> >> +	            u64 delta = muldiv64(val+2, NSEC_PER_SEC, KVM_PIT_FREQ);
> >> +		    ps->channels[channel].count_load_time =
> >> +                           
> >> ktime_sub(ps->channels[channel].count_load_time,
> >> +                                      ns_to_ktime(delta));
> > I do not understand what are you trying to do here. You assume that
> > trigger will happen 2 clocks after counter is loaded?
> > 
> 
> Modes 1 and 5 are single-shot, and they do not start counting until GATE
> is triggered, potentially well after count is loaded.  So this is
> attempting to model the "start of countdown has not been triggered"
> state as being mostly identical to the "already triggered and also
> expired some number of clocks (2) ago" state.
> 
So this is still not accurate. This assumes that guest loads counter and
then immediately triggers the gate. If between loading the counter and
triggering the gate guest does something else for a long time the result
will still not be accurate.

> It might be clearer to have a way to explicitly model a
> paused countdown, but such a mechanism doesn't currently exist.
If it worth doing it worth doing right. Should not be hard. Like setting
channels[channel].count_load_time on trigger instead of during count
loading.

> 
> Note that modeling modes 1 and 5 is fairly low priority,
> because channel 0's GATE line is generally hard-wired such that
> GATE edges/triggers are impossible.  But it may still be
> somewhat relevant to the PC speaker channel, or if someone
> might want to use this in a model of non-PC hardware.
> 
And that is why I thing it is not worth doing :) In kernel pit is not
suitable for non-PC hardware modeling either.

> >> +		}
> >>  		return;
> >>  	}
> >>  
> >> @@ -383,7 +404,6 @@ static void pit_load_count(struct kvm *kvm, int
> >> channel, u32 val)
> >>  	 * mode 1 is one shot, mode 2 is period, otherwise del timer */
> >>  	switch (ps->channels[0].mode) {
> >>  	case 0:
> >> -	case 1:
> >>          /* FIXME: enhance mode 4 precision */
> >>  	case 4:
> >>  		create_pit_timer(kvm, val, 0);
> >> @@ -393,6 +413,10 @@ static void pit_load_count(struct kvm *kvm, int
> >> channel, u32 val)
> >>  		create_pit_timer(kvm, val, 1);
> >>  		break;
> >>  	default:
> >> +		/* Modes 1 and 5 are triggered by gate leading edge,
> >> +		 * but channel 0's gate is hard-wired high and has
> >> +		 * no edges (on normal real hardware).
> >> +		 */
> >>  		destroy_pit_timer(kvm->arch.vpit);
> >>  	}
> >>  }
> >> -- 
> >> 1.7.10.2.484.gcd07cc5
> > 
> > --
> > 			Gleb.

--
			Gleb.

Re: [Qemu-devel] [PATCH KVM v2 2/4] KVM: additional i8254 output fixes

[Matthew Ogilvie]

On Tue, Jan 08, 2013 at 09:41:36AM +0200, Gleb Natapov wrote:
> On Mon, Jan 07, 2013 at 06:35:47PM -0600, mmogilvi@miniinfo.net wrote:
> > On Mon, 7 Jan 2013 14:04:03 +0200, Gleb Natapov <gleb@redhat.com> wrote:
> > > On Wed, Dec 26, 2012 at 10:39:54PM -0700, Matthew Ogilvie wrote:
> > >> Make git_get_out() consistent with spec.  Currently pit_get_out()
> > >> doesn't affect IRQ0, but it can be read by the guest in other ways.
> > >> This makes it consistent with proposed changes in qemu's i8254 model
> > >> as well.
> > >> 
> > >> See http://bochs.sourceforge.net/techspec/intel-82c54-timer.pdf.gz
> > >> or search the net for 23124406.pdf.
> > >> 
> > >> Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
> > >> ---
> > >>  arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++++++----------
> > >>  1 file changed, 34 insertions(+), 10 deletions(-)
> > >> 
> > >> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> > >> index cd4ec60..fd38938 100644
> > >> --- a/arch/x86/kvm/i8254.c
> > >> +++ b/arch/x86/kvm/i8254.c
> > >> @@ -144,6 +144,10 @@ static int pit_get_count(struct kvm *kvm, int
> > >> channel)
> > >>  
> > >>  	WARN_ON(!mutex_is_locked(&kvm->arch.vpit->pit_state.lock));
> > >>  
> > >> +	/* FIXME: Add some way to represent a paused timer and return
> > >> +	 *   the paused-at counter value, to better model gate pausing,
> > >> +	 *   "wait until next CLK pulse to load counter" logic, etc.
> > >> +	 */
> > >>  	t = kpit_elapsed(kvm, c, channel);
> > >>  	d = muldiv64(t, KVM_PIT_FREQ, NSEC_PER_SEC);
> > >>  
> > >> @@ -155,8 +159,7 @@ static int pit_get_count(struct kvm *kvm, int
> > >> channel)
> > >>  		counter = (c->count - d) & 0xffff;
> > >>  		break;
> > >>  	case 3:
> > >> -		/* XXX: may be incorrect for odd counts */
> > >> -		counter = c->count - (mod_64((2 * d), c->count));
> > >> +		counter = (c->count - (mod_64((2 * d), c->count))) & 0xfffe;
> > >>  		break;
> > >>  	default:
> > >>  		counter = c->count - mod_64(d, c->count);
> > >> @@ -180,20 +183,18 @@ static int pit_get_out(struct kvm *kvm, int
> > >> channel)
> > >>  	switch (c->mode) {
> > >>  	default:
> > >>  	case 0:
> > >> -		out = (d >= c->count);
> > >> -		break;
> > >>  	case 1:
> > >> -		out = (d < c->count);
> > >> +		out = (d >= c->count);
> > >>  		break;
> > >>  	case 2:
> > >> -		out = ((mod_64(d, c->count) == 0) && (d != 0));
> > >> +		out = (mod_64(d, c->count) != (c->count - 1) || c->gate == 0);
> > >>  		break;
> > >>  	case 3:
> > >> -		out = (mod_64(d, c->count) < ((c->count + 1) >> 1));
> > >> +		out = (mod_64(d, c->count) < ((c->count + 1) >> 1) || c->gate == 0);
> > >>  		break;
> > >>  	case 4:
> > >>  	case 5:
> > >> -		out = (d == c->count);
> > >> +		out = (d != c->count);
> > >>  		break;
> > >>  	}
> > >>  
> > >> @@ -367,7 +368,7 @@ static void pit_load_count(struct kvm *kvm, int
> > >> channel, u32 val)
> > >>  
> > >>  	/*
> > >>  	 * The largest possible initial count is 0; this is equivalent
> > >> -	 * to 216 for binary counting and 104 for BCD counting.
> > >> +	 * to pow(2,16) for binary counting and pow(10,4) for BCD counting.
> > >>  	 */
> > >>  	if (val == 0)
> > >>  		val = 0x10000;
> > >> @@ -376,6 +377,26 @@ static void pit_load_count(struct kvm *kvm, int
> > >> channel, u32 val)
> > >>  
> > >>  	if (channel != 0) {
> > >>  		ps->channels[channel].count_load_time = ktime_get();
> > >> +
> > >> +		/* In gate-triggered one-shot modes,
> > >> +		 * indirectly model some pit_get_out()
> > >> +		 * cases by setting the load time way
> > >> +		 * back until gate-triggered.
> > >> +		 * (Generally only affects reading status
> > >> +		 * from channel 2 speaker,
> > >> +		 * due to hard-wired gates on other
> > >> +		 * channels.)
> > >> +		 *
> > >> +		 * FIXME: This might be redesigned if a paused
> > >> +		 * timer state is added for pit_get_count().
> > >> +		 */
> > >> +		if (ps->channels[channel].mode == 1 ||
> > >> +		    ps->channels[channel].mode == 5) {
> > >> +	            u64 delta = muldiv64(val+2, NSEC_PER_SEC, KVM_PIT_FREQ);
> > >> +		    ps->channels[channel].count_load_time =
> > >> +                           
> > >> ktime_sub(ps->channels[channel].count_load_time,
> > >> +                                      ns_to_ktime(delta));
> > > I do not understand what are you trying to do here. You assume that
> > > trigger will happen 2 clocks after counter is loaded?
> > > 
> > 
> > Modes 1 and 5 are single-shot, and they do not start counting until GATE
> > is triggered, potentially well after count is loaded.  So this is
> > attempting to model the "start of countdown has not been triggered"
> > state as being mostly identical to the "already triggered and also
> > expired some number of clocks (2) ago" state.
> > 
> So this is still not accurate. This assumes that guest loads counter and
> then immediately triggers the gate. If between loading the counter and
> triggering the gate guest does something else for a long time the result
> will still not be accurate.
> 
> > It might be clearer to have a way to explicitly model a
> > paused countdown, but such a mechanism doesn't currently exist.
> If it worth doing it worth doing right. Should not be hard. Like setting
> channels[channel].count_load_time on trigger instead of during count
> loading.

Note the pic_set_gate() function already has logic to set
count_load_time when gate is triggered in modes 1 and 5
(as well as some others).  But until it is triggered, you want
to set the OUT line high (and not risk any change until
it is triggered) as indicated in the timing diagrams, and
setting the load time sufficiently far back should do
that.

On the other hand, setting the load time
back does not fix reading back the counter, but
that can't be fixed completely without some kind of paused
or "not triggered" state.  Especially because in some modes,
lowering gate simply pauses the counter wherever it is at
the time.  Even though a "not triggered" state might be flagged
with special values in existing a variables, a partially-counted
state would probably need an additional variable in the structure
to store the count as of when it was paused (or equivalent)...

Also, in some modes (like 2), a low GATE should immediatly force
OUT high (in addition to pausing the counter), but neither the
existing code nor any of my patches tries to model this aspect
of operation.

> 
> > 
> > Note that modeling modes 1 and 5 is fairly low priority,
> > because channel 0's GATE line is generally hard-wired such that
> > GATE edges/triggers are impossible.  But it may still be
> > somewhat relevant to the PC speaker channel, or if someone
> > might want to use this in a model of non-PC hardware.
> > 
> And that is why I thing it is not worth doing :) In kernel pit is not
> suitable for non-PC hardware modeling either.
> 
> > >> +		}
> > >>  		return;
> > >>  	}
> > >>  
> > >> @@ -383,7 +404,6 @@ static void pit_load_count(struct kvm *kvm, int
> > >> channel, u32 val)
> > >>  	 * mode 1 is one shot, mode 2 is period, otherwise del timer */
> > >>  	switch (ps->channels[0].mode) {
> > >>  	case 0:
> > >> -	case 1:
> > >>          /* FIXME: enhance mode 4 precision */
> > >>  	case 4:
> > >>  		create_pit_timer(kvm, val, 0);
> > >> @@ -393,6 +413,10 @@ static void pit_load_count(struct kvm *kvm, int
> > >> channel, u32 val)
> > >>  		create_pit_timer(kvm, val, 1);
> > >>  		break;
> > >>  	default:
> > >> +		/* Modes 1 and 5 are triggered by gate leading edge,
> > >> +		 * but channel 0's gate is hard-wired high and has
> > >> +		 * no edges (on normal real hardware).
> > >> +		 */
> > >>  		destroy_pit_timer(kvm->arch.vpit);
> > >>  	}
> > >>  }
> > >> -- 
> > >> 1.7.10.2.484.gcd07cc5
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.

[Qemu-devel] [PATCH KVM v2 3/4] KVM: fix i8259 interrupt high to low transition logic

[Matthew Ogilvie]

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 to the CPU even
though IRQ2 had dropped again, resulting in a spurious interupt
(IRQ15) and a panicked UNIX kernel.

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 arch/x86/kvm/i8259.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index cc31f7c..76d8dc1 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -111,8 +111,10 @@ static inline int pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
 				s->irr |= mask;
 			}
 			s->last_irr |= mask;
-		} else
+		} else {
+			s->irr &= ~mask;
 			s->last_irr &= ~mask;
+		}
 
 	return (s->imr & mask) ? -1 : ret;
 }
@@ -169,14 +171,10 @@ static void pic_update_irq(struct kvm_pic *s)
 {
 	int irq2, irq;
 
+	/* slave PIC notifies master PIC via IRQ2 */
 	irq2 = pic_get_irq(&s->pics[1]);
-	if (irq2 >= 0) {
-		/*
-		 * if irq request by slave pic, signal master PIC
-		 */
-		pic_set_irq1(&s->pics[0], 2, 1);
-		pic_set_irq1(&s->pics[0], 2, 0);
-	}
+	pic_set_irq1(&s->pics[0], 2, irq2 >= 0);
+
 	irq = pic_get_irq(&s->pics[0]);
 	pic_irq_request(s->kvm, irq >= 0);
 }
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH KVM v2 4/4] KVM: i8259: refactor pic_set_irq level logic

[Matthew Ogilvie]

No change in functionality.

Clarify that the only difference between level triggered and
edge triggered interrupts is on the leading edge.

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 arch/x86/kvm/i8259.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 76d8dc1..63f731b 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -95,26 +95,20 @@ static inline int pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
 {
 	int mask, ret = 1;
 	mask = 1 << irq;
-	if (s->elcr & mask)	/* level triggered */
-		if (level) {
+	if (level) {
+		if ((s->last_irr & mask) == 0 ||  /* edge for edge-triggered */
+		    (s->elcr & mask)) {           /* or level triggered */
 			ret = !(s->irr & mask);
 			s->irr |= mask;
-			s->last_irr |= mask;
-		} else {
-			s->irr &= ~mask;
-			s->last_irr &= ~mask;
-		}
-	else	/* edge triggered */
-		if (level) {
-			if ((s->last_irr & mask) == 0) {
-				ret = !(s->irr & mask);
-				s->irr |= mask;
-			}
-			s->last_irr |= mask;
-		} else {
-			s->irr &= ~mask;
-			s->last_irr &= ~mask;
 		}
+		s->last_irr |= mask;
+	} else {
+		/* Dropping level clears the interrupt regardless of edge
+		 * trigger vs level trigger.
+		 */
+		s->irr &= ~mask;
+		s->last_irr &= ~mask;
+	}
 
 	return (s->imr & mask) ? -1 : ret;
 }
-- 
1.7.10.2.484.gcd07cc5

Re: [Qemu-devel] [PATCH KVM v2 0/4] fix KVM i8259 IRQ trailing-edge logic

[Gleb Natapov]

On Wed, Dec 26, 2012 at 10:39:52PM -0700, Matthew Ogilvie wrote:
> Changes since version 1 (from Sep 9):
>    * Split off patch 1; this is the critical prerequisite to
>      make the i8254 work with the fixed i8259.
>    * Add patch 2, to make additional changes to the i8254
>      to make it consistent with the spec and with proposed changes
>      to qemu's i8254 model.
> 
> Background:
> 
> According to the spec, the i8259 will cancel an interrupt if
> the line goes low before it starts servicing it, even when
> it is considered edge-triggered.  This is a problem
> with an old Microport UNIX guest (ca 1987), where the
> guest masks off IRQ14 in the slave i8259, but the host's
> master i8259 model will still try to deliver an interrupt even after
> IRQ2 has been brought low, resulting in a spurious interrupt (IRQ15).
> 
> Before the i8259 can be fixed, the i8254 model needs to be fixed
> so it doesn't depend on the broken i8259.
> 
> Alternative: This could be narrowly fixed for IRQ2 only with no
> risk at all (and no need to touch the i8254), but if possible it
> seems reasonable to fix it for all lines at the same time.
> 
> But there may be some risk:
> 
> First, I think I've convinced myself that between the i8254 and i8259,
> the only substantial migration breakage should be when a
> whole series of conditions are met, and the combination
> should be rare enough not to worry about it.  See writeup
> in my qemu patch series (version 8; Dec 16).
> 
> Second, there is also the possibility that other devices are relying
> on the broken model.  I'm especially concerned with various users
> of a function called
> 
>         qemu_irq_pulse()
> 
> in the qemu source tree, which immediately lowers IRQ line after
> raising it.  If any of those calls are routed straight into
> the i8259 (as opposed to an APIC or some other chip), then it
> probably won't behave as desired.
> 
> I'll probably dig into qemu_irq_pulse() callers more carefully
> eventually, but there are lot of them, and any high-level incite
> anyone can provide into them would be helpful.
> 
$ git grep qemu_irq_pulse | wc -l
34

Files are:
hw/bonito.c
hw/dma.c
hw/grlib_apbuart.c
hw/grlib_gptimer.c
hw/hpet.c
hw/irq.h
hw/milkymist-ac97.c
hw/milkymist-minimac2.c
hw/milkymist-pfpu.c
hw/milkymist-softusb.c
hw/milkymist-sysctl.c
hw/milkymist-tmu2.c
hw/omap1.c
hw/omap_gptimer.c
hw/onenand.c
hw/spapr_events.c
hw/spapr_llan.c
hw/spapr_pci.c
hw/spapr_vio.c
hw/spapr_vty.c
hw/stellaris.c
hw/xilinx_ethlite.c

Looks like only two of those are relevant to PC platform hw/dma.c and
hw/hpet.c. In hw/dma.c it is used for internal qemu communication, not
real device. In hw/hpet.c from a quick glance it looks like _pulse is
only used when HPET in not in legacy emulation mode which means that
pulse should be directed to APIC.

> See the Dec 16 patch series I sent to the qemu mailing list for
> more information.
> http://search.gmane.org/?query=ogilvie&group=gmane.comp.emulators.qemu
> 
> Matthew Ogilvie (4):
>   KVM: fix i8254 IRQ0 to be normally high
>   KVM: additional i8254 output fixes
>   KVM: fix i8259 interrupt high to low transition logic
>   KVM: i8259: refactor pic_set_irq level logic
> 
>  arch/x86/kvm/i8254.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
>  arch/x86/kvm/i8259.c | 36 ++++++++++++++----------------------
>  2 files changed, 53 insertions(+), 33 deletions(-)
> 
> -- 
> 1.7.10.2.484.gcd07cc5

--
			Gleb.