[Qemu-devel] [PATCH v7 10/10] FOR FUTURE: fix i8254/i8259 IRQ0 line logic

[Matthew Ogilvie]

This patch fixes i8254 output line logic to match the spec, and
eliminates compatibility logic that was added as a transition
strategy for fixing the i8254 without breaking migration.

Tentatively, the idea is to apply this patch sometime in the
distant future (years from now?) after versions of qemu that this
can't migrate to/from reliably are no longer in use.  See the
previous patch for the preparatory support for this patch.

This patch probably shouldn't go in now, unless it is determined
that the migration concerns are overblown - in which case it should
probably be squashed in with earlier patches instead of kept separate.

Signed-off-by: Matthew Ogilvie <address@hidden>
---
 hw/i8254.c        | 106 +++++++-----------------------------------------------
 hw/i8254_common.c |  58 ++++--------------------------
 hw/i8259.c        |  19 +---------
 3 files changed, 20 insertions(+), 163 deletions(-)

diff --git a/hw/i8254.c b/hw/i8254.c
index ffe6e27..edb5b7a 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -35,8 +35,7 @@
 #define RW_STATE_WORD0 3
 #define RW_STATE_WORD1 4
 
-static void pit_irq_timer_update(PITChannelState *s, int64_t current_time,
-                                 bool edge);
+static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
 
 static int pit_get_count(PITChannelState *s)
 {
@@ -62,12 +61,7 @@ static int pit_get_count(PITChannelState *s)
         counter = (s->count - d) & 0xffff;
         break;
     case 3:
-        /* XXX: may be incorrect for odd counts
-         * FIXME i8254_timing_fixes: fix this by changing it to something
-         * like:
-         *   counter = (s->count - ((2 * d) % s->count)) & 0xfffe;
-         */
-        counter = s->count - ((2 * d) % s->count);
+        counter = (s->count - ((2 * d) % s->count)) & 0xfffe;
         break;
     default:
         counter = s->count - (d % s->count);
@@ -91,7 +85,7 @@ static void pit_set_channel_gate(PITCommonState *s, 
PITChannelState *sc,
         if (sc->gate < val) {
             /* restart counting on rising edge */
             sc->count_load_time = qemu_get_clock_ns(vm_clock);
-            pit_irq_timer_update(sc, sc->count_load_time, false);
+            pit_irq_timer_update(sc, sc->count_load_time);
         }
         break;
     case 2:
@@ -99,7 +93,7 @@ static void pit_set_channel_gate(PITCommonState *s, 
PITChannelState *sc,
         if (sc->gate < val) {
             /* restart counting on rising edge */
             sc->count_load_time = qemu_get_clock_ns(vm_clock);
-            pit_irq_timer_update(sc, sc->count_load_time, false);
+            pit_irq_timer_update(sc, sc->count_load_time);
         }
         /* XXX: disable/enable counting */
         break;
@@ -113,23 +107,19 @@ static inline void pit_load_count(PITChannelState *s, int 
val)
         val = 0x10000;
     s->count_load_time = qemu_get_clock_ns(vm_clock);
 
-    /* FIXME i8254_timing_fixes:
-     * In gate-triggered one-shot modes, indirectly model some pit_get_out()
-     * cases by setting the load time way back until gate-triggered,
-     * using code such as:
-     *
-     * if (s->mode == 1 || s->mode == 5)
-     *     s->count_load_time -= muldiv64(val+2, get_ticks_per_sec(), 
PIT_FREQ);
-     *
+    /* 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 Or this might be redesigned if a paused timer state is added
+     * FIXME: This might be redesigned if a paused timer state is added
      * for pic_get_count().
      */
+    if (s->mode == 1 || s->mode == 5)
+        s->count_load_time -= muldiv64(val+2, get_ticks_per_sec(), PIT_FREQ);
 
     s->count = val;
-    pit_irq_timer_update(s, s->count_load_time, false);
+    pit_irq_timer_update(s, s->count_load_time);
 }
 
 /* if already latched, do not latch again */
@@ -263,8 +253,7 @@ static uint64_t pit_ioport_read(void *opaque, hwaddr addr,
     return ret;
 }
 
-static void pit_irq_timer_update(PITChannelState *s, int64_t current_time,
-                                 bool edge)
+static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
 {
     int64_t expire_time;
     int irq_level;
@@ -274,75 +263,6 @@ static void pit_irq_timer_update(PITChannelState *s, 
int64_t current_time,
     }
     expire_time = pit_get_next_transition_time(s, current_time);
     irq_level = pit_get_out(s, current_time);
-
-    /* FIXME i8254_timing_fixes: Fixing pic_get_out() timings needs
-     *  to happen in stages.  For now, ensure that exactly one leading
-     *  edge is detected in the 8259 somewhere near counter expiration,
-     *  even if migrating to/from a future version that has corrected
-     *  (different) IRQ0 transitions from pic_get_out().
-     *
-     *  Quick description of how this works for various modes when
-     *  migrating between old and new versions.  To understand this,
-     *  it can help to draw out a timing diagram showing both this/old and
-     *  future version IRQ0 levels over time.
-     *     mode 0: no change.
-     *     mode 2: The leading edge stays at the same counter value,
-     *             but the timing of the trailing edge moves
-     *             from one CLK tick after the leading edge (this/old
-     *             version) to one CLK tick before the next trailing
-     *             edge (future version).  Migration cases:
-     *       - This high IRQ0 migrated to future version: Nothing special;
-     *         always correct.
-     *       - This low IRQ0 migrated to future version: May be set low
-     *         again a second time (noop) when counter gets to 1, but
-     *         otherwise nothing special.
-     *       - Future low IRQ0 migrated to this/old version: Nothing special;
-     *         always correct.
-     *       - Future high migrated to this/old version: Do a
-     *         high-low-high sequence when the next leading edge is needed.
-     *         The sequence may simplify to noop-low-high in some cases
-     *         depending on if migrates less than a tick since it went
-     *         high (so that this/old trailing edge logic applies), but it
-     *         should work correctly in any case.
-     *     mode 3: Effectively no change: The only change involves
-     *             gate, which is hard-coded for channel 0 (IRQ0).
-     *     mode 4: IRQ0 is inverted, so the leading edge is off by 1.
-     *             Migration cases:
-     *       - This high IRQ0 migrated to future version: At end of current
-     *         CLK tick, future code will set IRQ0 high again (noop).  Only
-     *         previous tick will have been delivered.
-     *       - This low IRQ0 migrated to future version: Future code will
-     *         set it low again when counter gets to 0 (noop), then
-     *         deliver the leading edge one CLK tick after that.
-     *       - Future low IRQ0 migrated to this/old version: This code
-     *         will do a low-high-low sequence at the next CLK tick.
-     *         (Normally it is already high, and so it simplifies
-     *         to noop-high-low.)
-     *       - Future high migrated to this/old version: The force-edge
-     *         code below will cause an immediate high-low-high
-     *         sequence as soon as counter reaches 0, delivering
-     *         an edge immediately.  (Normally it is already low, and
-     *         so it simplifies to noop-low-high.)
-     *     mode 1 or 5: Gate-triggered modes are never used for
-     *                  IRQ0 because gate0 is hard-wired.  But
-     *                  both of them have inverted logic similar to
-     *                  mode 4.
-     *
-     *  Also, under normal operation (this/old version, no-migration),
-     *  the if case is a noop, because it should already have the
-     *  indicated IRQ0 level.
-     *
-     *  When pic_get_out() logic is fixed, this migration compatibility
-     *  logic will need to be changed (probably just removed).
-     */
-    if (edge) {
-        if (s->mode == 2 && irq_level) {
-            qemu_set_irq(s->irq, 0);
-        } else if (s->mode == 4 || s->mode == 5 || s->mode == 1) {
-            qemu_set_irq(s->irq, !irq_level);
-        }
-    }
-
     qemu_set_irq(s->irq, irq_level);
 #ifdef DEBUG_PIT
     printf("irq_level=%d next_delay=%f\n",
@@ -360,7 +280,7 @@ static void pit_irq_timer(void *opaque)
 {
     PITChannelState *s = opaque;
 
-    pit_irq_timer_update(s, s->next_transition_time, true);
+    pit_irq_timer_update(s, s->next_transition_time);
 }
 
 static void pit_reset(DeviceState *dev)
@@ -385,7 +305,7 @@ static void pit_irq_control(void *opaque, int n, int enable)
 
     if (enable) {
         s->irq_disabled = 0;
-        pit_irq_timer_update(s, qemu_get_clock_ns(vm_clock), false);
+        pit_irq_timer_update(s, qemu_get_clock_ns(vm_clock));
     } else {
         s->irq_disabled = 1;
         qemu_del_timer(s->irq_timer);
diff --git a/hw/i8254_common.c b/hw/i8254_common.c
index 33f352f..dc13c99 100644
--- a/hw/i8254_common.c
+++ b/hw/i8254_common.c
@@ -50,56 +50,18 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
     switch (s->mode) {
     default:
     case 0:
-        out = (d >= s->count);
-        break;
     case 1:
-        /* FIXME i8254_timing_fixes: There are various problems
-         *  with qemu's 8254 model (especially when IRQ0's trailing
-         *  edge occurs), but fixing them directly might cause
-         *  problems with migration.  So just comment the fixes for
-         *  now.
-         *    (Based on reading timing diagrams in Intel's 8254 spec
-         *  sheet, downloadable from
-         *  http://bochs.sourceforge.net/techspec/intel-82c54-timer.pdf.gz
-         *  or other places.)
-         * FIXME i8254_timing_fixes: Both mode 0 and mode 1 should use
-         *    out = (d >= s->count);
-         *  So mode 0 can fall-through into a fixed mode 1.
-         *  The difference between modes 0 and 1 is in the gate triggering.
-         *  See also an adjustment to make to count_load_time
-         *  when count is loaded for mode 1.
-         */
-        out = (d < s->count);
+        out = (d >= s->count);
         break;
     case 2:
-        /* FIXME i8254_timing_fixes: This should simply be:
-         *    out = (d % s->count) != (s->count - 1) || s->gate == 0;
-         *  Gate is hard-wired 1 for channel 0 (IRQ0), but it can be
-         *  adjusted in software for channel 2 (PC speaker).
-         */
-        if ((d % s->count) == 0 && d != 0) {
-            out = 1;
-        } else {
-            out = 0;
-        }
+        out = (d % s->count) != (s->count - 1) || s->gate == 0;
         break;
     case 3:
-        /* FIXME i8254_timing_fixes: This should be:
-         *    out = (d % s->count) < ((s->count + 1) >> 1) || s->gate == 0;
-         *  Gate is hard-wired 1 for channel 0 (IRQ0), but it can be
-         *  adjusted in software for channel 2 (PC speaker).
-         */
-        out = (d % s->count) < ((s->count + 1) >> 1);
+        out = (d % s->count) < ((s->count + 1) >> 1) || s->gate == 0;
         break;
     case 4:
     case 5:
-        /* FIXME i8254_timing_fixes: The logic is backwards, and should be:
-         *    out = (d != s->count);
-         *  The difference between modes 4 and 5 is in the gate triggering.
-         *  See also an adjustment to make to count_load_time
-         *  when count is loaded for mode 5.
-         */
-        out = (d == s->count);
+        out = (d != s->count);
         break;
     }
     return out;
@@ -125,18 +87,10 @@ int64_t pit_get_next_transition_time(PITChannelState *s, 
int64_t current_time)
         break;
     case 2:
         base = (d / s->count) * s->count;
-        /* FIXME i8254_timing_fixes: The if condition and else case should
-         *  be changed to:
-         *   if ((d - base) == (s->count - 1)) {
-         *       next_time = base + s->count;
-         *   } else {
-         *       next_time = base + s->count - 1;
-         *   }
-         */
-        if ((d - base) == 0 && d != 0) {
+        if ((d - base) == s->count-1) {
             next_time = base + s->count;
         } else {
-            next_time = base + s->count + 1;
+            next_time = base + s->count - 1;
         }
         break;
     case 3:
diff --git a/hw/i8259.c b/hw/i8259.c
index 71cc09a..9b2ec40 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -150,25 +150,8 @@ static void pic_set_irq(void *opaque, int irq, int level)
         /* Dropping level clears the interrupt regardless of edge trigger
          * vs level trigger.
          */
-        s->last_irr &= ~mask;
-
-        /* Migration compatibility hack:
-         *
-         * The i8254 timer model is wrong in a number of ways,
-         * including lowering IRQ0 much earlier than it should.
-         *
-         * FIXME i8254_timing_fixes: Eventually the i8254
-         *  should be fixed, but it isn't
-         *  trivial to do so in a way that avoids possible problems with
-         *  migration (lost or gained timer ticks).  So for now, make the
-         *  i8254 work the same way that it worked in qemu 1.2, and
-         *  leave irr for IRQ0 alone in the i8259 here:
-         */
-        if (irq == 0 && s->master) {
-            mask = 0;
-        }
-
         s->irr &= ~mask;
+        s->last_irr &= ~mask;
     }
     pic_update_irq(s);
 }
-- 
1.7.10.2.484.gcd07cc5