[Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Matthew Ogilvie]

This series makes a series of mostly-unrelated fixes to allow
running an old Microport UNIX (ca 1987) guest under qemu.

Changes since version 6:
   * Patches 1 through 6 haven't changed, other than resolving
     a couple of simple conflicts.
   * Patch 7 "fixes" IRQ0 by just making it work like before,
     rather than fixing it properly.  This avoids possible risk
     to cross-version migration, etc.
   * Patches 8, 9, and 10 provide one possible gradual transition path
     to properly fix the 8254 model with relatively little risk to
     migration/etc.  The idea is that 8 and 9 could be applied
     immediately in preparation for a future fix, and then the
     actual fix (10) could be applied sometime in the future when
     migrating to or from pre-patch-9 versions is no longer a concern.
        I am not actually aware of ANY guest that actually needs
     an improved 8254 model, but this provides one way to improve
     it if desired.

----------------
Split up this series?

I'm not sure what the next steps are to get these into qemu, other
than waiting for 1.4 for at least the non-trivial parts?

Patches 1 through 3 could be considered independent trivial patches.
Would splitting them apart improve the changes of getting them into qemu?

Patch 4 isn't quite trivial, but it is well isolated (other than
small documentation conflicts against patch 3).  Should it be split
off?  It hasn't changed since version 3, but nobody has really
commented on it.

Patches 5 through 10 are interrelated, and should remain related in
a series.

----------------
Still needed:

  * Corresponding KVM patches.  The best approach may depend
    on what option is selected for qemu above.
     * Note that KVM uses a simplified model that doesn't try
       to emulate the trailing edge of the interrupt very well
       at all.  I'm not proposing to change this aspect of it.
     * A patch analogous to 7 should be easy.
     * Patches 8 through 10 are also fairly easy by themselves.
       But now we start having an explosion of combinations
       of versions of KVM and qemu and migration to/from, and it
       might be better to:
     * Or more involved fixes would involve new ioctl()'s and
       command line arguments to select old or fixed 8254 models
       dynamically.  See below.

----------------
Alternative options for improving the i8254 model and migration:

1. Don't fix 8254 at all.  Just apply through patch 7 or 8, and don't try
   to make any additional fixes.  I don't know of any guests that need
   improvements, so this could be a viable option.

2. Just fix it immediately, and don't worry about migration.  Squash
   the last few patches together.  A single missed periodic
   timer tick that only happens when migrating
   between versions of qemu is probably not a significant
   concern.  (Unless someone knows of an OS that actually runs
   the i8254 in single shot mode 4, where a missed interrupt
   could cause a hang or something?)

3. Use patches 8 and 9 now, and patch 10 sometime in the future.
   If it was just qemu, this would be attractive.  But when you
   also need to worry about a bunch of combinations of versions of
   qemu and KVM and migration, this is looking less attractive.

4. Support both old and fixed i8254 models, selectable at runtime
   with a command line option.  (Question: What should such an
   option look like?)  This may be the best way to actually
   change the 8254, but I'm not sure changes are even needed.
   It's certainly getting rather far afield from running Microport
   UNIX...

----------------

Matthew Ogilvie (10):
  fix some debug printf format strings
  vl: fix -hdachs/-hda argument order parsing issues
  qemu-options.hx: mention retrace= VGA option
  vga: add some optional CGA compatibility hacks
  i8259: fix so that dropping IRQ level always clears the interrupt
    request
  i8259: refactor pic_set_irq level logic
  i8254/i8259: workaround to make IRQ0 work like before
  i8254: add comments about fixing timings
  i8254: prepare for migration compatibility with future fixes
  FOR FUTURE: fix i8254/i8259 IRQ0 line logic

 hw/cirrus_vga.c   |  4 ++--
 hw/i8254.c        | 24 +++++++++++++++++++--
 hw/i8254_common.c | 18 ++++++----------
 hw/i8259.c        | 28 ++++++++++---------------
 hw/ide/cmd646.c   |  5 +++--
 hw/ide/via.c      |  5 +++--
 hw/pc.h           |  4 ++++
 hw/vga.c          | 37 ++++++++++++++++++++++++++-------
 qemu-options.hx   | 27 +++++++++++++++++++++++-
 vl.c              | 62 ++++++++++++++++++++++++++++++++++++-------------------
 10 files changed, 147 insertions(+), 67 deletions(-)

-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v7 01/10] fix some debug printf format strings

[Matthew Ogilvie]

These are normally ifdefed out and don't matter.  But if you enable
them, they ought to be correct.

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 hw/cirrus_vga.c | 4 ++--
 hw/i8259.c      | 3 ++-
 hw/ide/cmd646.c | 5 +++--
 hw/ide/via.c    | 5 +++--
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 9bef96e..da60fc3 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2052,8 +2052,8 @@ static void cirrus_vga_mem_write(void *opaque,
 	}
     } else {
 #ifdef DEBUG_CIRRUS
-        printf("cirrus: mem_writeb " TARGET_FMT_plx " value %02x\n", addr,
-               mem_value);
+        printf("cirrus: mem_writeb " TARGET_FMT_plx " value %02" PRIx64 "\n",
+               addr, mem_value);
 #endif
     }
 }
diff --git a/hw/i8259.c b/hw/i8259.c
index af0ba4d..60c25ba 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -355,7 +355,8 @@ static uint64_t pic_ioport_read(void *opaque, hwaddr addr,
             ret = s->imr;
         }
     }
-    DPRINTF("read: addr=0x%02x val=0x%02x\n", addr, ret);
+    DPRINTF("read: addr=0x%02" TARGET_PRIxPHYS " val=0x%02x\n",
+            addr, ret);
     return ret;
 }
 
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 804db60..72ceaaf 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -154,7 +154,7 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
         break;
     }
 #ifdef DEBUG_IDE
-    printf("bmdma: readb 0x%02x : 0x%02x\n", addr, val);
+    printf("bmdma: readb 0x%02" TARGET_PRIxPHYS " : 0x%02x\n", addr, val);
 #endif
     return val;
 }
@@ -170,7 +170,8 @@ static void bmdma_write(void *opaque, hwaddr addr,
     }
 
 #ifdef DEBUG_IDE
-    printf("bmdma: writeb 0x%02x : 0x%02x\n", addr, val);
+    printf("bmdma: writeb 0x%02" TARGET_PRIxPHYS " : 0x%02" PRIx64 "\n",
+           addr, val);
 #endif
     switch(addr & 3) {
     case 0:
diff --git a/hw/ide/via.c b/hw/ide/via.c
index efda173..10ba9b5 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -55,7 +55,7 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
         break;
     }
 #ifdef DEBUG_IDE
-    printf("bmdma: readb 0x%02x : 0x%02x\n", addr, val);
+    printf("bmdma: readb 0x%02" TARGET_PRIxPHYS " : 0x%02x\n", addr, val);
 #endif
     return val;
 }
@@ -70,7 +70,8 @@ static void bmdma_write(void *opaque, hwaddr addr,
     }
 
 #ifdef DEBUG_IDE
-    printf("bmdma: writeb 0x%02x : 0x%02x\n", addr, val);
+    printf("bmdma: writeb 0x%02" TARGET_PRIxPHYS " : 0x%02" PRIx64 "\n",
+           addr, val);
 #endif
     switch (addr & 3) {
     case 0:
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v7 02/10] vl: fix -hdachs/-hda argument order parsing issues

[Matthew Ogilvie]

Without this patch, the -hdachs argument had to occur either
BEFORE the corresponding "-hda" option, or AFTER the plain
disk image name (if neither -hda nor -drive is used).  Otherwise
it would effectively be ignored.

Option -hdachs still has no effect on -drive, but that seems best.

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 vl.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/vl.c b/vl.c
index c8e9c78..c6c7d95 100644
--- a/vl.c
+++ b/vl.c
@@ -2533,8 +2533,9 @@ int main(int argc, char **argv, char **envp)
     const char *kernel_filename, *kernel_cmdline;
     char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */
     DisplayState *ds;
-    int cyls, heads, secs, translation;
-    QemuOpts *hda_opts = NULL, *opts, *machine_opts;
+    char hdachs_params[512];  /* save -hdachs to apply to later -hda */
+    QemuOpts *hda_opts = NULL; /* save -hda to be modified by later -hdachs */
+    QemuOpts *opts, *machine_opts;
     QemuOptsList *olist;
     int optind;
     const char *optarg;
@@ -2589,8 +2590,7 @@ int main(int argc, char **argv, char **envp)
     cpu_model = NULL;
     ram_size = 0;
     snapshot = 0;
-    cyls = heads = secs = 0;
-    translation = BIOS_ATA_TRANSLATION_AUTO;
+    snprintf(hdachs_params, sizeof(hdachs_params), "%s", HD_OPTS);
 
     for (i = 0; i < MAX_NODES; i++) {
         node_mem[i] = 0;
@@ -2638,7 +2638,7 @@ int main(int argc, char **argv, char **envp)
         if (optind >= argc)
             break;
         if (argv[optind][0] != '-') {
-	    hda_opts = drive_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS);
+            hda_opts = drive_add(IF_DEFAULT, 0, argv[optind++], hdachs_params);
         } else {
             const QEMUOption *popt;
 
@@ -2661,21 +2661,8 @@ int main(int argc, char **argv, char **envp)
                 cpu_model = optarg;
                 break;
             case QEMU_OPTION_hda:
-                {
-                    char buf[256];
-                    if (cyls == 0)
-                        snprintf(buf, sizeof(buf), "%s", HD_OPTS);
-                    else
-                        snprintf(buf, sizeof(buf),
-                                 "%s,cyls=%d,heads=%d,secs=%d%s",
-                                 HD_OPTS , cyls, heads, secs,
-                                 translation == BIOS_ATA_TRANSLATION_LBA ?
-                                 ",trans=lba" :
-                                 translation == BIOS_ATA_TRANSLATION_NONE ?
-                                 ",trans=none" : "");
-                    drive_add(IF_DEFAULT, 0, optarg, buf);
-                    break;
-                }
+                hda_opts = drive_add(IF_DEFAULT, 0, optarg, hdachs_params);
+                break;
             case QEMU_OPTION_hdb:
             case QEMU_OPTION_hdc:
             case QEMU_OPTION_hdd:
@@ -2709,7 +2696,10 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_hdachs:
                 {
+                    int cyls, heads, secs, translation;
                     const char *p;
+                    cyls = heads = secs = 0;
+                    translation = BIOS_ATA_TRANSLATION_AUTO;
                     p = optarg;
                     cyls = strtol(p, (char **)&p, 0);
                     if (cyls < 1 || cyls > 16383)
@@ -2741,7 +2731,14 @@ int main(int argc, char **argv, char **envp)
                         fprintf(stderr, "qemu: invalid physical CHS format\n");
                         exit(1);
                     }
-		    if (hda_opts != NULL) {
+                    snprintf(hdachs_params, sizeof(hdachs_params),
+                             "%s,cyls=%d,heads=%d,secs=%d%s",
+                             HD_OPTS , cyls, heads, secs,
+                             translation == BIOS_ATA_TRANSLATION_LBA ?
+                             ",trans=lba" :
+                             translation == BIOS_ATA_TRANSLATION_NONE ?
+                             ",trans=none" : "");
+                    if (hda_opts != NULL) {
                         char num[16];
                         snprintf(num, sizeof(num), "%d", cyls);
                         qemu_opt_set(hda_opts, "cyls", num);
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v7 03/10] qemu-options.hx: mention retrace= VGA option

[Matthew Ogilvie]

The feature was added in commit cb5a7aa8c32141bb Sep 2008.
My description is based on "Better VGA retrace emulation (needed
for some DOS games/demos)" from
http://www.boblycat.org/~malc/code/patches/qemu/index.html

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 qemu-options.hx | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 9bb29d3..ebc138d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1007,7 +1007,7 @@ DEF("vga", HAS_ARG, QEMU_OPTION_vga,
     "-vga [std|cirrus|vmware|qxl|xenfb|none]\n"
     "                select video card type\n", QEMU_ARCH_ALL)
 STEXI
-@item -vga @var{type}
+@item -vga @var{type}[,@var{prop}=@var{value}[,...]]
 @findex -vga
 Select type of VGA card to emulate. Valid values for @var{type} are
 @table @option
@@ -1032,6 +1032,12 @@ Recommended choice when using the spice protocol.
 @item none
 Disable VGA card.
 @end table
+Valid optional properties are
+@table @option
+@item retrace=dumb|precise
+Select dumb (default) or precise VGA retrace logic, useful for some
+DOS games/demos.
+@end table
 ETEXI
 
 DEF("full-screen", 0, QEMU_OPTION_full_screen,
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v7 04/10] vga: add some optional CGA compatibility hacks

[Matthew Ogilvie]

This patch adds some optional compatibility hacks (default
disabled) to allow Microport UNIX to function under qemu.

I've tried to structure it to be easy to add more hacks for other
old CGA programs, if anyone ever needs them.

Microport UNIX System V/386 v 2.1 (ca 1987) tries to program
the CGA registers directly with neither the assistance of BIOS, nor
with proper handling of EGA/VGA-only registers.  Note that it didn't
work on real VGA hardware, either (although in that case, the most
obvious problems seemed to be out-of-range hsync and/or vsync
signalling, rather than the issues in this patch).

Eventually real MDA and/or CGA support might provide an alternative to
this patch, although a hybrid approach like this patch might still
be useful in marginal cases.

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 hw/pc.h         |  4 ++++
 hw/vga.c        | 37 +++++++++++++++++++++++++++++--------
 qemu-options.hx | 19 +++++++++++++++++++
 vl.c            | 23 +++++++++++++++++++++++
 4 files changed, 75 insertions(+), 8 deletions(-)

diff --git a/hw/pc.h b/hw/pc.h
index e7993ca..d033ee3 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -149,6 +149,10 @@ enum vga_retrace_method {
 
 extern enum vga_retrace_method vga_retrace_method;
 
+#define VGA_CGA_HACK_PALETTE_BLANKING  (1<<0)
+#define VGA_CGA_HACK_FONT_HEIGHT       (1<<1)
+extern int vga_cga_hacks;
+
 int isa_vga_mm_init(hwaddr vram_base,
                     hwaddr ctrl_base, int it_shift,
                     MemoryRegion *address_space);
diff --git a/hw/vga.c b/hw/vga.c
index 2b0200a..660961b 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -552,14 +552,31 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         printf("vga: write CR%x = 0x%02x\n", s->cr_index, val);
 #endif
         /* handle CR0-7 protection */
-        if ((s->cr[VGA_CRTC_V_SYNC_END] & VGA_CR11_LOCK_CR0_CR7) &&
-            s->cr_index <= VGA_CRTC_OVERFLOW) {
-            /* can always write bit 4 of CR7 */
-            if (s->cr_index == VGA_CRTC_OVERFLOW) {
-                s->cr[VGA_CRTC_OVERFLOW] = (s->cr[VGA_CRTC_OVERFLOW] & ~0x10) |
-                    (val & 0x10);
+        if (s->cr[VGA_CRTC_V_SYNC_END] & VGA_CR11_LOCK_CR0_CR7) {
+            if (s->cr_index <= VGA_CRTC_OVERFLOW) {
+                /* can always write bit 4 of CR7 */
+                if (s->cr_index == VGA_CRTC_OVERFLOW) {
+                    s->cr[VGA_CRTC_OVERFLOW] =
+                        (s->cr[VGA_CRTC_OVERFLOW] & ~0x10) | (val & 0x10);
+                }
+                return;
+            } else if ((vga_cga_hacks & VGA_CGA_HACK_FONT_HEIGHT) &&
+                       !(s->sr[VGA_SEQ_CLOCK_MODE] & VGA_SR01_CHAR_CLK_8DOTS)) {
+                /* extra CGA compatibility hacks (not in standard VGA) */
+                if (s->cr_index == VGA_CRTC_MAX_SCAN &&
+                    val == 7 &&
+                    (s->cr[VGA_CRTC_MAX_SCAN] & 0xf) == 0xf) {
+                    return;
+                } else if (s->cr_index == VGA_CRTC_CURSOR_START &&
+                           val == 6 &&
+                           (s->cr[VGA_CRTC_MAX_SCAN] & 0xf) == 0xf) {
+                    val = 0xd;
+                } else if (s->cr_index == VGA_CRTC_CURSOR_END &&
+                           val == 7 &&
+                           (s->cr[VGA_CRTC_MAX_SCAN] & 0xf) == 0xf) {
+                    val = 0xe;
+                }
             }
-            return;
         }
         s->cr[s->cr_index] = val;
 
@@ -1896,7 +1913,11 @@ static void vga_update_display(void *opaque)
         /* nothing to do */
     } else {
         full_update = 0;
-        if (!(s->ar_index & 0x20)) {
+        if (!(s->ar_index & 0x20) &&
+            /* extra CGA compatibility hacks (not in standard VGA) */
+            (!(vga_cga_hacks & VGA_CGA_HACK_PALETTE_BLANKING) ||
+             s->ar_index != 0 ||
+             !s->ar_flip_flop)) {
             graphic_mode = GMODE_BLANK;
         } else {
             graphic_mode = s->gr[VGA_GFX_MISC] & VGA_GR06_GRAPHICS_MODE;
diff --git a/qemu-options.hx b/qemu-options.hx
index ebc138d..7d6fc72 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1037,6 +1037,25 @@ Valid optional properties are
 @item retrace=dumb|precise
 Select dumb (default) or precise VGA retrace logic, useful for some
 DOS games/demos.
+@item cga_hacks=@var{hack1}[+@var{hack2}[+...]]
+Enable various extra CGA compatibility hacks for programs that are
+trying to directly set CGA modes without BIOS assistance nor
+real knowledge of EGA/VGA.  These might only work with -vga std.
+Valid hacks are
+@table @option
+@item palette_blanking
+Wait to blank the screen until palette registers seem to actually be
+modified, instead of blanking it as soon as the palette address bit (0x10)
+of the attribute address register (0x3c0) is cleared.
+@item font_height
+Ignore attempts to change the VGA font height (index 9),
+cursor start (index 10), and cursor end (index 11) of the CRTC control
+registers (0x3d5) if trying to set them to the default for CGA fonts
+instead of VGA fonts.
+@item all
+Enable all CGA hacks.  More CGA hacks may be added in future versions
+of qemu.
+@end table
 @end table
 ETEXI
 
diff --git a/vl.c b/vl.c
index c6c7d95..188509f 100644
--- a/vl.c
+++ b/vl.c
@@ -180,6 +180,7 @@ int main(int argc, char **argv)
 static const char *data_dir;
 const char *bios_name = NULL;
 enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
+int vga_cga_hacks = 0;
 DisplayType display_type = DT_DEFAULT;
 static int display_remote;
 const char* keyboard_layout = NULL;
@@ -1886,6 +1887,28 @@ static void select_vgahw (const char *p)
             else if (strstart(opts, "precise", &nextopt))
                 vga_retrace_method = VGA_RETRACE_PRECISE;
             else goto invalid_vga;
+        } else if (strstart(opts, ",cga_hacks=", &nextopt)) {
+            opts = nextopt;
+            while (*opts) {
+                if (strstart(opts, "all", &nextopt)) {
+                    opts = nextopt;
+                    vga_cga_hacks |= ~0;
+                } else if (strstart(opts, "palette_blanking", &nextopt)) {
+                    opts = nextopt;
+                    vga_cga_hacks |= VGA_CGA_HACK_PALETTE_BLANKING;
+                } else if (strstart(opts, "font_height", &nextopt)) {
+                    opts = nextopt;
+                    vga_cga_hacks |= VGA_CGA_HACK_FONT_HEIGHT;
+                } else {
+                    break;
+                }
+
+                if (*opts == '+') {
+                    opts++;
+                } else {
+                    break;
+                }
+            }
         } else goto invalid_vga;
         opts = nextopt;
     }
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v7 05/10] i8259: fix so that dropping IRQ level always clears the interrupt request

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

Output from a test program:
-----------
Real hardware [Pentium 4]:
  cmdRead unmask IRR=4005 mask IRR=4001 sti unmask irq14 IRR=0001 DONE
[I also see a final IRR=0000 occasionally.  Probably just happened to
ask it while the timer (IRQ0) line is low.  It masks off most IRQ's, including
timer.]
-----------
Unpatched qemu:
  cmdRead unmask IRR=4015 mask IRR=4015 sti irq15 unmask IRR=4015 DONE
[Presumably IRQ4 (0x10 - qemu's serial device model?) had a transient
edge during initialization, but had been masked off even before I
masked it off?]
-----------
Patched qemu:
  cmdRead unmask IRR=4005 mask IRR=4001 sti unmask irq14 IRR=0001 DONE
-----------

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 hw/i8259.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i8259.c b/hw/i8259.c
index 60c25ba..95587cd 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -157,6 +157,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
             }
             s->last_irr |= mask;
         } else {
+            s->irr &= ~mask;
             s->last_irr &= ~mask;
         }
     }
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v7 06/10] 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>
---
 hw/i8259.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 95587cd..9b2ec40 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -140,26 +140,18 @@ static void pic_set_irq(void *opaque, int irq, int level)
     }
 #endif
 
-    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 */
             s->irr |= mask;
-            s->last_irr |= mask;
-        } else {
-            s->irr &= ~mask;
-            s->last_irr &= ~mask;
         }
+        s->last_irr |= mask;
     } else {
-        /* edge triggered */
-        if (level) {
-            if ((s->last_irr & mask) == 0) {
-                s->irr |= mask;
-            }
-            s->last_irr |= mask;
-        } else {
-            s->irr &= ~mask;
-            s->last_irr &= ~mask;
-        }
+        /* Dropping level clears the interrupt regardless of edge trigger
+         * vs level trigger.
+         */
+        s->irr &= ~mask;
+        s->last_irr &= ~mask;
     }
     pic_update_irq(s);
 }
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v7 07/10] i8254/i8259: workaround to make IRQ0 work like before

[Matthew Ogilvie]

Someday it should be fixed properly, but doing so may
break migration.  So go with an incremental approach instead.

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 hw/i8259.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 9b2ec40..71cc09a 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -150,8 +150,25 @@ static void pic_set_irq(void *opaque, int irq, int level)
         /* Dropping level clears the interrupt regardless of edge trigger
          * vs level trigger.
          */
-        s->irr &= ~mask;
         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;
     }
     pic_update_irq(s);
 }
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v7 08/10] i8254: add comments about fixing timings

[Matthew Ogilvie]

There may be risk of problems with migration if these are just
fixed blindly, but at least comment what it ought to be changed to.

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 hw/i8254.c        | 31 ++++++++++++++++++++++++++++++-
 hw/i8254_common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/hw/i8254.c b/hw/i8254.c
index bea5f92..982e8e7 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -39,6 +39,15 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
 
 static int pit_get_count(PITChannelState *s)
 {
+    /* FIXME: Add some way to represent a paused timer and return
+     *   the paused-at counter value, to better model:
+     *     - gate-triggered modes (1 and 5),
+     *     - gate-pausable modes,
+     *     - [maybe] the "wait until next CLK pulse to load counter" logic,
+     *     - [maybe/not clear] half-loaded counter logic for mode 0, and
+     *       the "null count" status bit,
+     *     - etc.
+     */
     uint64_t d;
     int counter;
 
@@ -52,7 +61,11 @@ static int pit_get_count(PITChannelState *s)
         counter = (s->count - d) & 0xffff;
         break;
     case 3:
-        /* XXX: may be incorrect for odd counts */
+        /* 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);
         break;
     default:
@@ -98,6 +111,22 @@ static inline void pit_load_count(PITChannelState *s, int val)
     if (val == 0)
         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);
+     *
+     * (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
+     * for pic_get_count().
+     */
+
     s->count = val;
     pit_irq_timer_update(s, s->count_load_time);
 }
diff --git a/hw/i8254_common.c b/hw/i8254_common.c
index a03d7cd..33f352f 100644
--- a/hw/i8254_common.c
+++ b/hw/i8254_common.c
@@ -53,9 +53,30 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
         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);
         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 {
@@ -63,10 +84,21 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
         }
         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);
         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);
         break;
     }
@@ -93,6 +125,14 @@ 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) {
             next_time = base + s->count;
         } else {
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v7 09/10] i8254: prepare for migration compatibility with future fixes

[Matthew Ogilvie]

Under normal operation this patch should have no effect.  But
it adds some redundant (no-change) calls to qemu_set_irq(),
so that if a running host image is migrated from a future version
of qemu that has various fixes to the i8254 output line logic,
this version can still deliver exactly the correct number of
IRQ0 leading edges to the i8259 at as close as possible to the
correct time.

The plan is to incorporate this now, and then much later (years?)
we can implement the actual fixes to the i8254, after migration to/from
qemu versions without this transitional fix is no longer a concern.

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---

Alternatives:

An alternative for mode 2 might involve tweaking
pit_get_next_transition_time() to create an extra pseudo-transition
at the same clock tick that will eventually have the "real" transition
(so it has 3 "transitions" per period).  But it may be best to keep
the migration hacks together in one place.

Or support both the old model and a new/fixed model, selectable at
runtime by command line switch.

 hw/i8254.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 78 insertions(+), 7 deletions(-)

diff --git a/hw/i8254.c b/hw/i8254.c
index 982e8e7..ffe6e27 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -35,7 +35,8 @@
 #define RW_STATE_WORD0 3
 #define RW_STATE_WORD1 4
 
-static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
+static void pit_irq_timer_update(PITChannelState *s, int64_t current_time,
+                                 bool edge);
 
 static int pit_get_count(PITChannelState *s)
 {
@@ -90,7 +91,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);
+            pit_irq_timer_update(sc, sc->count_load_time, false);
         }
         break;
     case 2:
@@ -98,7 +99,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);
+            pit_irq_timer_update(sc, sc->count_load_time, false);
         }
         /* XXX: disable/enable counting */
         break;
@@ -128,7 +129,7 @@ static inline void pit_load_count(PITChannelState *s, int val)
      */
 
     s->count = val;
-    pit_irq_timer_update(s, s->count_load_time);
+    pit_irq_timer_update(s, s->count_load_time, false);
 }
 
 /* if already latched, do not latch again */
@@ -262,7 +263,8 @@ static uint64_t pit_ioport_read(void *opaque, hwaddr addr,
     return ret;
 }
 
-static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
+static void pit_irq_timer_update(PITChannelState *s, int64_t current_time,
+                                 bool edge)
 {
     int64_t expire_time;
     int irq_level;
@@ -272,6 +274,75 @@ 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",
@@ -289,7 +360,7 @@ static void pit_irq_timer(void *opaque)
 {
     PITChannelState *s = opaque;
 
-    pit_irq_timer_update(s, s->next_transition_time);
+    pit_irq_timer_update(s, s->next_transition_time, true);
 }
 
 static void pit_reset(DeviceState *dev)
@@ -314,7 +385,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));
+        pit_irq_timer_update(s, qemu_get_clock_ns(vm_clock), false);
     } else {
         s->irq_disabled = 1;
         qemu_del_timer(s->irq_timer);
-- 
1.7.10.2.484.gcd07cc5

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

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Matthew Ogilvie]

On Sun, Nov 25, 2012 at 02:51:36PM -0700, Matthew Ogilvie wrote:
> This series makes a series of mostly-unrelated fixes to allow
> running an old Microport UNIX (ca 1987) guest under qemu.
> 
> Changes since version 6:
>    * Patches 1 through 6 haven't changed, other than resolving
>      a couple of simple conflicts.
>    * Patch 7 "fixes" IRQ0 by just making it work like before,
>      rather than fixing it properly.  This avoids possible risk
>      to cross-version migration, etc.
>    * Patches 8, 9, and 10 provide one possible gradual transition path
>      to properly fix the 8254 model with relatively little risk to
>      migration/etc.  The idea is that 8 and 9 could be applied
>      immediately in preparation for a future fix, and then the
>      actual fix (10) could be applied sometime in the future when
>      migrating to or from pre-patch-9 versions is no longer a concern.
>         I am not actually aware of ANY guest that actually needs
>      an improved 8254 model, but this provides one way to improve
>      it if desired.
> 

Ping?

What would it take to get some variation of this series
into 1.4?  The last feedback I've seen was against version 5, back
in September.
http://search.gmane.org/?query=ogilvie&group=gmane.comp.emulators.qemu

> ----------------
> Split up this series?
> 
> I'm not sure what the next steps are to get these into qemu, other
> than waiting for 1.4 for at least the non-trivial parts?
> 
> Patches 1 through 3 could be considered independent trivial patches.
> Would splitting them apart improve the changes of getting them into qemu?
> 
> Patch 4 isn't quite trivial, but it is well isolated (other than
> small documentation conflicts against patch 3).  Should it be split
> off?  It hasn't changed since version 3, but nobody has really
> commented on it.
> 
> Patches 5 through 10 are interrelated, and should remain related in
> a series.
> 
> ----------------
> Still needed:
> 
>   * Corresponding KVM patches.  The best approach may depend
>     on what option is selected for qemu above.
>      * Note that KVM uses a simplified model that doesn't try
>        to emulate the trailing edge of the interrupt very well
>        at all.  I'm not proposing to change this aspect of it.
>      * A patch analogous to 7 should be easy.
>      * Patches 8 through 10 are also fairly easy by themselves.
>        But now we start having an explosion of combinations
>        of versions of KVM and qemu and migration to/from, and it
>        might be better to:
>      * Or more involved fixes would involve new ioctl()'s and
>        command line arguments to select old or fixed 8254 models
>        dynamically.  See below.

Any preferences?

> 
> ----------------
> Alternative options for improving the i8254 model and migration:
> 
> 1. Don't fix 8254 at all.  Just apply through patch 7 or 8, and don't try
>    to make any additional fixes.  I don't know of any guests that need
>    improvements, so this could be a viable option.

Or:
1.1. Don't fix any 8259 lines either, except for the one line (IRQ2) that
     is giving me trouble.  (Recall that the original problem is the guest
     masking off IRQ14 in the 8259, and the resulting IRQ2 trailing edge
     isn't handled correctly in the master 8259, resulting in a
     spurious interrupt.)

> 
> 2. Just fix it immediately, and don't worry about migration.  Squash
>    the last few patches together.  A single missed periodic
>    timer tick that only happens when migrating
>    between versions of qemu is probably not a significant
>    concern.  (Unless someone knows of an OS that actually runs
>    the i8254 in single shot mode 4, where a missed interrupt
>    could cause a hang or something?)
> 
> 3. Use patches 8 and 9 now, and patch 10 sometime in the future.
>    If it was just qemu, this would be attractive.  But when you
>    also need to worry about a bunch of combinations of versions of
>    qemu and KVM and migration, this is looking less attractive.
> 
> 4. Support both old and fixed i8254 models, selectable at runtime
>    with a command line option.  (Question: What should such an
>    option look like?)  This may be the best way to actually
>    change the 8254, but I'm not sure changes are even needed.
>    It's certainly getting rather far afield from running Microport
>    UNIX...
> 
> ----------------
> 
> Matthew Ogilvie (10):
>   fix some debug printf format strings
>   vl: fix -hdachs/-hda argument order parsing issues
>   qemu-options.hx: mention retrace= VGA option
>   vga: add some optional CGA compatibility hacks
>   i8259: fix so that dropping IRQ level always clears the interrupt
>     request
>   i8259: refactor pic_set_irq level logic
>   i8254/i8259: workaround to make IRQ0 work like before
>   i8254: add comments about fixing timings
>   i8254: prepare for migration compatibility with future fixes
>   FOR FUTURE: fix i8254/i8259 IRQ0 line logic

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5436 bytes --]

On 2012-12-10 06:14, Matthew Ogilvie wrote:
> On Sun, Nov 25, 2012 at 02:51:36PM -0700, Matthew Ogilvie wrote:
>> This series makes a series of mostly-unrelated fixes to allow
>> running an old Microport UNIX (ca 1987) guest under qemu.
>>
>> Changes since version 6:
>>    * Patches 1 through 6 haven't changed, other than resolving
>>      a couple of simple conflicts.
>>    * Patch 7 "fixes" IRQ0 by just making it work like before,
>>      rather than fixing it properly.  This avoids possible risk
>>      to cross-version migration, etc.
>>    * Patches 8, 9, and 10 provide one possible gradual transition path
>>      to properly fix the 8254 model with relatively little risk to
>>      migration/etc.  The idea is that 8 and 9 could be applied
>>      immediately in preparation for a future fix, and then the
>>      actual fix (10) could be applied sometime in the future when
>>      migrating to or from pre-patch-9 versions is no longer a concern.
>>         I am not actually aware of ANY guest that actually needs
>>      an improved 8254 model, but this provides one way to improve
>>      it if desired.
>>
> 
> Ping?
> 
> What would it take to get some variation of this series
> into 1.4?  The last feedback I've seen was against version 5, back
> in September.
> http://search.gmane.org/?query=ogilvie&group=gmane.comp.emulators.qemu

I suppose it's primarily a question of time for some reviewer(s). Sorry,
I wasn't able to look at it yet, maybe I will have a chance next week.

> 
>> ----------------
>> Split up this series?
>>
>> I'm not sure what the next steps are to get these into qemu, other
>> than waiting for 1.4 for at least the non-trivial parts?
>>
>> Patches 1 through 3 could be considered independent trivial patches.
>> Would splitting them apart improve the changes of getting them into qemu?
>>
>> Patch 4 isn't quite trivial, but it is well isolated (other than
>> small documentation conflicts against patch 3).  Should it be split
>> off?  It hasn't changed since version 3, but nobody has really
>> commented on it.
>>
>> Patches 5 through 10 are interrelated, and should remain related in
>> a series.
>>
>> ----------------
>> Still needed:
>>
>>   * Corresponding KVM patches.  The best approach may depend
>>     on what option is selected for qemu above.
>>      * Note that KVM uses a simplified model that doesn't try
>>        to emulate the trailing edge of the interrupt very well
>>        at all.  I'm not proposing to change this aspect of it.
>>      * A patch analogous to 7 should be easy.
>>      * Patches 8 through 10 are also fairly easy by themselves.
>>        But now we start having an explosion of combinations
>>        of versions of KVM and qemu and migration to/from, and it
>>        might be better to:
>>      * Or more involved fixes would involve new ioctl()'s and
>>        command line arguments to select old or fixed 8254 models
>>        dynamically.  See below.
> 
> Any preferences?

As Avi left, I'm putting Gleb and Marcelo on CC.

> 
>>
>> ----------------
>> Alternative options for improving the i8254 model and migration:
>>
>> 1. Don't fix 8254 at all.  Just apply through patch 7 or 8, and don't try
>>    to make any additional fixes.  I don't know of any guests that need
>>    improvements, so this could be a viable option.
> 
> Or:
> 1.1. Don't fix any 8259 lines either, except for the one line (IRQ2) that
>      is giving me trouble.  (Recall that the original problem is the guest
>      masking off IRQ14 in the 8259, and the resulting IRQ2 trailing edge
>      isn't handled correctly in the master 8259, resulting in a
>      spurious interrupt.)
> 
>>
>> 2. Just fix it immediately, and don't worry about migration.  Squash
>>    the last few patches together.  A single missed periodic
>>    timer tick that only happens when migrating
>>    between versions of qemu is probably not a significant
>>    concern.  (Unless someone knows of an OS that actually runs
>>    the i8254 in single shot mode 4, where a missed interrupt
>>    could cause a hang or something?)
>>
>> 3. Use patches 8 and 9 now, and patch 10 sometime in the future.
>>    If it was just qemu, this would be attractive.  But when you
>>    also need to worry about a bunch of combinations of versions of
>>    qemu and KVM and migration, this is looking less attractive.
>>
>> 4. Support both old and fixed i8254 models, selectable at runtime
>>    with a command line option.  (Question: What should such an
>>    option look like?)  This may be the best way to actually
>>    change the 8254, but I'm not sure changes are even needed.
>>    It's certainly getting rather far afield from running Microport
>>    UNIX...
>>
>> ----------------
>>
>> Matthew Ogilvie (10):
>>   fix some debug printf format strings
>>   vl: fix -hdachs/-hda argument order parsing issues
>>   qemu-options.hx: mention retrace= VGA option
>>   vga: add some optional CGA compatibility hacks
>>   i8259: fix so that dropping IRQ level always clears the interrupt
>>     request
>>   i8259: refactor pic_set_irq level logic
>>   i8254/i8259: workaround to make IRQ0 work like before
>>   i8254: add comments about fixing timings
>>   i8254: prepare for migration compatibility with future fixes
>>   FOR FUTURE: fix i8254/i8259 IRQ0 line logic

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Anthony Liguori]

Jan Kiszka <jan.kiszka@web.de> writes:

> On 2012-12-10 06:14, Matthew Ogilvie wrote:
>> On Sun, Nov 25, 2012 at 02:51:36PM -0700, Matthew Ogilvie wrote:
>>> This series makes a series of mostly-unrelated fixes to allow
>>> running an old Microport UNIX (ca 1987) guest under qemu.
>>>
>>> Changes since version 6:
>>>    * Patches 1 through 6 haven't changed, other than resolving
>>>      a couple of simple conflicts.
>>>    * Patch 7 "fixes" IRQ0 by just making it work like before,
>>>      rather than fixing it properly.  This avoids possible risk
>>>      to cross-version migration, etc.
>>>    * Patches 8, 9, and 10 provide one possible gradual transition path
>>>      to properly fix the 8254 model with relatively little risk to
>>>      migration/etc.  The idea is that 8 and 9 could be applied
>>>      immediately in preparation for a future fix, and then the
>>>      actual fix (10) could be applied sometime in the future when
>>>      migrating to or from pre-patch-9 versions is no longer a concern.
>>>         I am not actually aware of ANY guest that actually needs
>>>      an improved 8254 model, but this provides one way to improve
>>>      it if desired.
>>>
>> 
>> Ping?
>> 
>> What would it take to get some variation of this series
>> into 1.4?  The last feedback I've seen was against version 5, back
>> in September.
>> http://search.gmane.org/?query=ogilvie&group=gmane.comp.emulators.qemu
>
> I suppose it's primarily a question of time for some reviewer(s). Sorry,
> I wasn't able to look at it yet, maybe I will have a chance next week.

If you added a test case for the i8254 using the mc146818rtc qtest test
case as an example, you would very likely attract more reviewers.

It would also make it easier to ensure that the issues you're fixing
here don't regress in the future too.

Regards,

Anthony Liguori

>
>> 
>>> ----------------
>>> Split up this series?
>>>
>>> I'm not sure what the next steps are to get these into qemu, other
>>> than waiting for 1.4 for at least the non-trivial parts?
>>>
>>> Patches 1 through 3 could be considered independent trivial patches.
>>> Would splitting them apart improve the changes of getting them into qemu?
>>>
>>> Patch 4 isn't quite trivial, but it is well isolated (other than
>>> small documentation conflicts against patch 3).  Should it be split
>>> off?  It hasn't changed since version 3, but nobody has really
>>> commented on it.
>>>
>>> Patches 5 through 10 are interrelated, and should remain related in
>>> a series.
>>>
>>> ----------------
>>> Still needed:
>>>
>>>   * Corresponding KVM patches.  The best approach may depend
>>>     on what option is selected for qemu above.
>>>      * Note that KVM uses a simplified model that doesn't try
>>>        to emulate the trailing edge of the interrupt very well
>>>        at all.  I'm not proposing to change this aspect of it.
>>>      * A patch analogous to 7 should be easy.
>>>      * Patches 8 through 10 are also fairly easy by themselves.
>>>        But now we start having an explosion of combinations
>>>        of versions of KVM and qemu and migration to/from, and it
>>>        might be better to:
>>>      * Or more involved fixes would involve new ioctl()'s and
>>>        command line arguments to select old or fixed 8254 models
>>>        dynamically.  See below.
>> 
>> Any preferences?
>
> As Avi left, I'm putting Gleb and Marcelo on CC.
>
>> 
>>>
>>> ----------------
>>> Alternative options for improving the i8254 model and migration:
>>>
>>> 1. Don't fix 8254 at all.  Just apply through patch 7 or 8, and don't try
>>>    to make any additional fixes.  I don't know of any guests that need
>>>    improvements, so this could be a viable option.
>> 
>> Or:
>> 1.1. Don't fix any 8259 lines either, except for the one line (IRQ2) that
>>      is giving me trouble.  (Recall that the original problem is the guest
>>      masking off IRQ14 in the 8259, and the resulting IRQ2 trailing edge
>>      isn't handled correctly in the master 8259, resulting in a
>>      spurious interrupt.)
>> 
>>>
>>> 2. Just fix it immediately, and don't worry about migration.  Squash
>>>    the last few patches together.  A single missed periodic
>>>    timer tick that only happens when migrating
>>>    between versions of qemu is probably not a significant
>>>    concern.  (Unless someone knows of an OS that actually runs
>>>    the i8254 in single shot mode 4, where a missed interrupt
>>>    could cause a hang or something?)
>>>
>>> 3. Use patches 8 and 9 now, and patch 10 sometime in the future.
>>>    If it was just qemu, this would be attractive.  But when you
>>>    also need to worry about a bunch of combinations of versions of
>>>    qemu and KVM and migration, this is looking less attractive.
>>>
>>> 4. Support both old and fixed i8254 models, selectable at runtime
>>>    with a command line option.  (Question: What should such an
>>>    option look like?)  This may be the best way to actually
>>>    change the 8254, but I'm not sure changes are even needed.
>>>    It's certainly getting rather far afield from running Microport
>>>    UNIX...
>>>
>>> ----------------
>>>
>>> Matthew Ogilvie (10):
>>>   fix some debug printf format strings
>>>   vl: fix -hdachs/-hda argument order parsing issues
>>>   qemu-options.hx: mention retrace= VGA option
>>>   vga: add some optional CGA compatibility hacks
>>>   i8259: fix so that dropping IRQ level always clears the interrupt
>>>     request
>>>   i8259: refactor pic_set_irq level logic
>>>   i8254/i8259: workaround to make IRQ0 work like before
>>>   i8254: add comments about fixing timings
>>>   i8254: prepare for migration compatibility with future fixes
>>>   FOR FUTURE: fix i8254/i8259 IRQ0 line logic
>
> Jan

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Matthew Ogilvie]

On Mon, Dec 10, 2012 at 10:47:59AM -0600, Anthony Liguori wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
> > On 2012-12-10 06:14, Matthew Ogilvie wrote:
> >> On Sun, Nov 25, 2012 at 02:51:36PM -0700, Matthew Ogilvie wrote:
> >>> This series makes a series of mostly-unrelated fixes to allow
> >>> running an old Microport UNIX (ca 1987) guest under qemu.
> >>>
> >>> Changes since version 6:
> >>>    * Patches 1 through 6 haven't changed, other than resolving
> >>>      a couple of simple conflicts.
> >>>    * Patch 7 "fixes" IRQ0 by just making it work like before,
> >>>      rather than fixing it properly.  This avoids possible risk
> >>>      to cross-version migration, etc.
> >>>    * Patches 8, 9, and 10 provide one possible gradual transition path
> >>>      to properly fix the 8254 model with relatively little risk to
> >>>      migration/etc.  The idea is that 8 and 9 could be applied
> >>>      immediately in preparation for a future fix, and then the
> >>>      actual fix (10) could be applied sometime in the future when
> >>>      migrating to or from pre-patch-9 versions is no longer a concern.
> >>>         I am not actually aware of ANY guest that actually needs
> >>>      an improved 8254 model, but this provides one way to improve
> >>>      it if desired.
> >>>
> >> 
> >> Ping?
> >> 
> >> What would it take to get some variation of this series
> >> into 1.4?  The last feedback I've seen was against version 5, back
> >> in September.
> >> http://search.gmane.org/?query=ogilvie&group=gmane.comp.emulators.qemu
> >
> > I suppose it's primarily a question of time for some reviewer(s). Sorry,
> > I wasn't able to look at it yet, maybe I will have a chance next week.
> 
> If you added a test case for the i8254 using the mc146818rtc qtest test
> case as an example, you would very likely attract more reviewers.
> 
> It would also make it easier to ensure that the issues you're fixing
> here don't regress in the future too.
> 
> Regards,
> 
> Anthony Liguori

I'll look into adding some qtest test case(s), starting along the same
lines as the i8259 "kvm-unit-tests" case I posted Sep 9, 2012, (and the
standalone test it was based on).  See also
http://home.comcast.net/~mmogilvi/downloads/i8259-imr-test-2012-09-02.tar.bz2

Note that for the i8259/i8254 stuff, strictly speaking
I only really need a fix to the trailing-edge handling of the
cascade (IRQ2) line of the i8259.  A more general fix (all IRQ lines)
might be nice, but such a general fix (especially IRQ0) exposes
bugs in the i8254 model.  And fixing that poses at least a
small risk to cross-version guest migration.

All of which raises the strategic question of how much scope creap of
fixing/changing low level device models should I worry about, to
address a problem only seen in a very rare guest?  Vs some kind
of narrower, non-general fix (IRQ2 only, or all-except-IRQ0, or
something else).

Also, there is a CGA compatibility hack I need.  As well as
three trivial patches that I don't actually need, but seem
like easy fixes.

> 
> >
> >> 
> >>> ----------------
> >>> Split up this series?
> >>>
> >>> I'm not sure what the next steps are to get these into qemu, other
> >>> than waiting for 1.4 for at least the non-trivial parts?
> >>>
> >>> Patches 1 through 3 could be considered independent trivial patches.
> >>> Would splitting them apart improve the changes of getting them into qemu?
> >>>
> >>> Patch 4 isn't quite trivial, but it is well isolated (other than
> >>> small documentation conflicts against patch 3).  Should it be split
> >>> off?  It hasn't changed since version 3, but nobody has really
> >>> commented on it.
> >>>
> >>> Patches 5 through 10 are interrelated, and should remain related in
> >>> a series.
> >>>
> >>> ----------------
> >>> Still needed:
> >>>
> >>>   * Corresponding KVM patches.  The best approach may depend
> >>>     on what option is selected for qemu above.
> >>>      * Note that KVM uses a simplified model that doesn't try
> >>>        to emulate the trailing edge of the interrupt very well
> >>>        at all.  I'm not proposing to change this aspect of it.
> >>>      * A patch analogous to 7 should be easy.
> >>>      * Patches 8 through 10 are also fairly easy by themselves.
> >>>        But now we start having an explosion of combinations
> >>>        of versions of KVM and qemu and migration to/from, and it
> >>>        might be better to:
> >>>      * Or more involved fixes would involve new ioctl()'s and
> >>>        command line arguments to select old or fixed 8254 models
> >>>        dynamically.  See below.
> >> 
> >> Any preferences?
> >
> > As Avi left, I'm putting Gleb and Marcelo on CC.
> >
> >> 
> >>>
> >>> ----------------
> >>> Alternative options for improving the i8254 model and migration:
> >>>
> >>> 1. Don't fix 8254 at all.  Just apply through patch 7 or 8, and don't try
> >>>    to make any additional fixes.  I don't know of any guests that need
> >>>    improvements, so this could be a viable option.
> >> 
> >> Or:
> >> 1.1. Don't fix any 8259 lines either, except for the one line (IRQ2) that
> >>      is giving me trouble.  (Recall that the original problem is the guest
> >>      masking off IRQ14 in the 8259, and the resulting IRQ2 trailing edge
> >>      isn't handled correctly in the master 8259, resulting in a
> >>      spurious interrupt.)
> >> 
> >>>
> >>> 2. Just fix it immediately, and don't worry about migration.  Squash
> >>>    the last few patches together.  A single missed periodic
> >>>    timer tick that only happens when migrating
> >>>    between versions of qemu is probably not a significant
> >>>    concern.  (Unless someone knows of an OS that actually runs
> >>>    the i8254 in single shot mode 4, where a missed interrupt
> >>>    could cause a hang or something?)
> >>>
> >>> 3. Use patches 8 and 9 now, and patch 10 sometime in the future.
> >>>    If it was just qemu, this would be attractive.  But when you
> >>>    also need to worry about a bunch of combinations of versions of
> >>>    qemu and KVM and migration, this is looking less attractive.
> >>>
> >>> 4. Support both old and fixed i8254 models, selectable at runtime
> >>>    with a command line option.  (Question: What should such an
> >>>    option look like?)  This may be the best way to actually
> >>>    change the 8254, but I'm not sure changes are even needed.
> >>>    It's certainly getting rather far afield from running Microport
> >>>    UNIX...
> >>>
> >>> ----------------
> >>>
> >>> Matthew Ogilvie (10):
> >>>   fix some debug printf format strings
> >>>   vl: fix -hdachs/-hda argument order parsing issues
> >>>   qemu-options.hx: mention retrace= VGA option
> >>>   vga: add some optional CGA compatibility hacks
> >>>   i8259: fix so that dropping IRQ level always clears the interrupt
> >>>     request
> >>>   i8259: refactor pic_set_irq level logic
> >>>   i8254/i8259: workaround to make IRQ0 work like before
> >>>   i8254: add comments about fixing timings
> >>>   i8254: prepare for migration compatibility with future fixes
> >>>   FOR FUTURE: fix i8254/i8259 IRQ0 line logic
> >
> > Jan

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Jamie Lokier]

Matthew Ogilvie wrote:
> 2. Just fix it immediately, and don't worry about migration.  Squash
>    the last few patches together.  A single missed periodic
>    timer tick that only happens when migrating
>    between versions of qemu is probably not a significant
>    concern.  (Unless someone knows of an OS that actually runs
>    the i8254 in single shot mode 4, where a missed interrupt
>    could cause a hang or something?)

Hi Matthew,

Such as Linux?  0x38 looks like mode 4 to me.  I suspect it's used in
tickless mode when there isn't a better clock event source.

linux/drivers/clocksource/i8253.c:

        #ifdef CONFIG_CLKEVT_I8253
           /* ... */

                case CLOCK_EVT_MODE_ONESHOT:
                        /* One shot setup */
                        outb_p(0x38, PIT_MODE);

           /* ... */

        /*
         * Program the next event in oneshot mode
         *
         * Delta is given in PIT ticks
         */
        static int pit_next_event(unsigned long delta, struct clock_event_device *evt)
        {
                raw_spin_lock(&i8253_lock);
                outb_p(delta & 0xff , PIT_CH0); /* LSB */
                outb_p(delta >> 8 , PIT_CH0);           /* MSB */
                raw_spin_unlock(&i8253_lock);

                return 0;
        }

           /* ... */
        #endif

> 4. Support both old and fixed i8254 models, selectable at runtime
>    with a command line option.  (Question: What should such an
>    option look like?)  This may be the best way to actually
>    change the 8254, but I'm not sure changes are even needed.
>    It's certainly getting rather far afield from running Microport
>    UNIX...

I can't see a reason to have the old behaviour, if every guest works
with the new one, except for this awkward cross-version migration
thing.

I guess ideally, device emulations would be versioned when their
behaviour changes, rather like shared libraries are, and the
appropriate old version kept around to be loaded for a particular
machine that's still running with it.  Sounds a bit complicated though.

Best,
-- Jamie

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Gleb Natapov]

On Sun, Nov 25, 2012 at 02:51:36PM -0700, Matthew Ogilvie wrote:
> This series makes a series of mostly-unrelated fixes to allow
> running an old Microport UNIX (ca 1987) guest under qemu.
> 
> Changes since version 6:
>    * Patches 1 through 6 haven't changed, other than resolving
>      a couple of simple conflicts.
>    * Patch 7 "fixes" IRQ0 by just making it work like before,
>      rather than fixing it properly.  This avoids possible risk
>      to cross-version migration, etc.
>    * Patches 8, 9, and 10 provide one possible gradual transition path
>      to properly fix the 8254 model with relatively little risk to
>      migration/etc.  The idea is that 8 and 9 could be applied
>      immediately in preparation for a future fix, and then the
>      actual fix (10) could be applied sometime in the future when
>      migrating to or from pre-patch-9 versions is no longer a concern.
>         I am not actually aware of ANY guest that actually needs
>      an improved 8254 model, but this provides one way to improve
>      it if desired.
> 
> ----------------
> Split up this series?
> 
> I'm not sure what the next steps are to get these into qemu, other
> than waiting for 1.4 for at least the non-trivial parts?
> 
> Patches 1 through 3 could be considered independent trivial patches.
> Would splitting them apart improve the changes of getting them into qemu?
> 
> Patch 4 isn't quite trivial, but it is well isolated (other than
> small documentation conflicts against patch 3).  Should it be split
> off?  It hasn't changed since version 3, but nobody has really
> commented on it.
> 
> Patches 5 through 10 are interrelated, and should remain related in
> a series.
> 
> ----------------
> Still needed:
> 
>   * Corresponding KVM patches.  The best approach may depend
>     on what option is selected for qemu above.
>      * Note that KVM uses a simplified model that doesn't try
>        to emulate the trailing edge of the interrupt very well
>        at all.  I'm not proposing to change this aspect of it.
>      * A patch analogous to 7 should be easy.
>      * Patches 8 through 10 are also fairly easy by themselves.
>        But now we start having an explosion of combinations
>        of versions of KVM and qemu and migration to/from, and it
>        might be better to:
Why explosion of combinations? I do not see any changes in
migration code in your series, so as long as we care about migration
from in-kernel irqchip to in-kernel irqchip we should be independent
from qemu version, no?

>      * Or more involved fixes would involve new ioctl()'s and
>        command line arguments to select old or fixed 8254 models
>        dynamically.  See below.
> 
> ----------------
> Alternative options for improving the i8254 model and migration:
> 
> 1. Don't fix 8254 at all.  Just apply through patch 7 or 8, and don't try
>    to make any additional fixes.  I don't know of any guests that need
>    improvements, so this could be a viable option.
> 
> 2. Just fix it immediately, and don't worry about migration.  Squash
>    the last few patches together.  A single missed periodic
>    timer tick that only happens when migrating
>    between versions of qemu is probably not a significant
>    concern.  (Unless someone knows of an OS that actually runs
>    the i8254 in single shot mode 4, where a missed interrupt
>    could cause a hang or something?)
> 
If migration can fail only with the single, rarely (if ever) used mode,
I honestly like this option the most.

> 3. Use patches 8 and 9 now, and patch 10 sometime in the future.
>    If it was just qemu, this would be attractive.  But when you
>    also need to worry about a bunch of combinations of versions of
>    qemu and KVM and migration, this is looking less attractive.
> 
> 4. Support both old and fixed i8254 models, selectable at runtime
>    with a command line option.  (Question: What should such an
>    option look like?)  This may be the best way to actually
>    change the 8254, but I'm not sure changes are even needed.
>    It's certainly getting rather far afield from running Microport
>    UNIX...
If we will start doing it for each bug...

> 
> ----------------
> 
> Matthew Ogilvie (10):
>   fix some debug printf format strings
>   vl: fix -hdachs/-hda argument order parsing issues
>   qemu-options.hx: mention retrace= VGA option
>   vga: add some optional CGA compatibility hacks
>   i8259: fix so that dropping IRQ level always clears the interrupt
>     request
>   i8259: refactor pic_set_irq level logic
>   i8254/i8259: workaround to make IRQ0 work like before
>   i8254: add comments about fixing timings
>   i8254: prepare for migration compatibility with future fixes
>   FOR FUTURE: fix i8254/i8259 IRQ0 line logic
> 
>  hw/cirrus_vga.c   |  4 ++--
>  hw/i8254.c        | 24 +++++++++++++++++++--
>  hw/i8254_common.c | 18 ++++++----------
>  hw/i8259.c        | 28 ++++++++++---------------
>  hw/ide/cmd646.c   |  5 +++--
>  hw/ide/via.c      |  5 +++--
>  hw/pc.h           |  4 ++++
>  hw/vga.c          | 37 ++++++++++++++++++++++++++-------
>  qemu-options.hx   | 27 +++++++++++++++++++++++-
>  vl.c              | 62 ++++++++++++++++++++++++++++++++++++-------------------
>  10 files changed, 147 insertions(+), 67 deletions(-)
> 
> -- 
> 1.7.10.2.484.gcd07cc5
> 

--
			Gleb.

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Matthew Ogilvie]

On Tue, Dec 11, 2012 at 11:20:05AM +0000, Jamie Lokier wrote:
> Matthew Ogilvie wrote:
> > 2. Just fix it immediately, and don't worry about migration.  Squash
> >    the last few patches together.  A single missed periodic
> >    timer tick that only happens when migrating
> >    between versions of qemu is probably not a significant
> >    concern.  (Unless someone knows of an OS that actually runs
> >    the i8254 in single shot mode 4, where a missed interrupt
> >    could cause a hang or something?)
> 
> Hi Matthew,
> 
> Such as Linux?  0x38 looks like mode 4 to me.  I suspect it's used in
> tickless mode when there isn't a better clock event source.
> 
> linux/drivers/clocksource/i8253.c:
> 
>         #ifdef CONFIG_CLKEVT_I8253
>            /* ... */
> 
>                 case CLOCK_EVT_MODE_ONESHOT:
>                         /* One shot setup */
>                         outb_p(0x38, PIT_MODE);
> 
>            /* ... */

I'm not very familiar with how Linux selects and uses timers, but
I think the full qemu fix will only lose a mode 4 interrupt during
migration if several conditions are all met:

  1. Guest's kernel is configured to be tickless.
  2. Guest's kernel is using the 8254, and not something better
     like the HPET.  (Perhaps the better device is configured out
     of the kernel, and/or explicitly left out of qemu's list of
     devices.  Both of which I would expect to be rare.)
  3. The user is migrating from a newer/fixed version of qemu, back
     to an older version of qemu.  (Not sure how common this is.  It
     may be common in some kind of cluster environment where you
     take down one real machine at a time for upgrades and one
     of the upgraded machines needs to be taken back down, or if you
     migrate between completely different data centers that are on
     different upgrade cyles, or something.)

If any of these three conditions is NOT met, then I don't expect it
to be possible to lose an interrupt in mode 4.  Perhaps the combination
can be considered rare enough not to worry about?

It may generate a immediate interrupt (instead of delayed
properly) if the guest resets the one-shot mode due to some
other device's interrupt (fixed normal high vs old normal low),
but hopefully the guest won't be bothered too much by a single
such interrupt.

Also, for guests that use periodic modes (2 or 3), it is still possible
it might gain or lose 1 periodic timer interrupt during migration
between versions of qemu, but that probably isn't a big deal.

NOTE: I'm assumming that the HPET qemu model doesn't have similar trailing
edge or off-by-one-tick problems as the 8254 model.  Nor any other
device, for that matter.

If such breakage is considered minor enough and/or rare enough, then
I certainly think the simplicity of this option is attractive.

> 
> > 4. Support both old and fixed i8254 models, selectable at runtime
> >    with a command line option.  (Question: What should such an
> >    option look like?)  This may be the best way to actually
> >    change the 8254, but I'm not sure changes are even needed.
> >    It's certainly getting rather far afield from running Microport
> >    UNIX...
> 
> I can't see a reason to have the old behaviour, if every guest works
> with the new one, except for this awkward cross-version migration
> thing.

Yes, the only reason to even consider having both old and new behavior
around is if the migration thing is considered significant enough to
warrant it.

One argument for making it configurable is if the user has an environment
where you expect to occasionally migrate back to old versions, then you
might want to start with the old model even if you initially boot
the guest on a new version qemu.

> 
> I guess ideally, device emulations would be versioned when their
> behaviour changes, rather like shared libraries are, and the
> appropriate old version kept around to be loaded for a particular
> machine that's still running with it.  Sounds a bit complicated though.
> 
> Best,
> -- Jamie

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Matthew Ogilvie]

On Tue, Dec 11, 2012 at 06:19:56PM +0200, Gleb Natapov wrote:
> On Sun, Nov 25, 2012 at 02:51:36PM -0700, Matthew Ogilvie wrote:
> > ----------------
> > Still needed:
> > 
> >   * Corresponding KVM patches.  The best approach may depend
> >     on what option is selected for qemu above.
> >      * Note that KVM uses a simplified model that doesn't try
> >        to emulate the trailing edge of the interrupt very well
> >        at all.  I'm not proposing to change this aspect of it.
> >      * A patch analogous to 7 should be easy.
> >      * Patches 8 through 10 are also fairly easy by themselves.
> >        But now we start having an explosion of combinations
> >        of versions of KVM and qemu and migration to/from, and it
> >        might be better to:
> Why explosion of combinations? I do not see any changes in
> migration code in your series, so as long as we care about migration
> from in-kernel irqchip to in-kernel irqchip we should be independent
> from qemu version, no?

You may be correct.  I'm a little hazy on the details of how things are
split between KVM and QEMU.  Are there situations that do ioport read/write
handling within qemu rather than KVM?  How about things like pit_get_out(),
pit_get_next_transition_time(), etc in qemu/hw/i8254_common.c?  (If
not used when KVM is enabled, then why are they "common"?)  What
are the implications if qemu and KVM implementations of such
functions disagree?

> 
> >      * Or more involved fixes would involve new ioctl()'s and
> >        command line arguments to select old or fixed 8254 models
> >        dynamically.  See below.
> > 
> > ----------------
> > Alternative options for improving the i8254 model and migration:
> > 
> > 1. Don't fix 8254 at all.  Just apply through patch 7 or 8, and don't try
> >    to make any additional fixes.  I don't know of any guests that need
> >    improvements, so this could be a viable option.
> > 
> > 2. Just fix it immediately, and don't worry about migration.  Squash
> >    the last few patches together.  A single missed periodic
> >    timer tick that only happens when migrating
> >    between versions of qemu is probably not a significant
> >    concern.  (Unless someone knows of an OS that actually runs
> >    the i8254 in single shot mode 4, where a missed interrupt
> >    could cause a hang or something?)
> > 
> If migration can fail only with the single, rarely (if ever) used mode,
> I honestly like this option the most.

As long as it is truly rare, I agree.  I'm just not sure if the "rare"
qualification is actually true or not.  See also my response to Jamie
Lokier about Linux's tickless configuration.

> 
> > 3. Use patches 8 and 9 now, and patch 10 sometime in the future.
> >    If it was just qemu, this would be attractive.  But when you
> >    also need to worry about a bunch of combinations of versions of
> >    qemu and KVM and migration, this is looking less attractive.
> > 
> > 4. Support both old and fixed i8254 models, selectable at runtime
> >    with a command line option.  (Question: What should such an
> >    option look like?)  This may be the best way to actually
> >    change the 8254, but I'm not sure changes are even needed.
> >    It's certainly getting rather far afield from running Microport
> >    UNIX...
> If we will start doing it for each bug...
> 

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Gleb Natapov]

On Wed, Dec 12, 2012 at 12:46:41AM -0700, Matthew Ogilvie wrote:
> On Tue, Dec 11, 2012 at 06:19:56PM +0200, Gleb Natapov wrote:
> > On Sun, Nov 25, 2012 at 02:51:36PM -0700, Matthew Ogilvie wrote:
> > > ----------------
> > > Still needed:
> > > 
> > >   * Corresponding KVM patches.  The best approach may depend
> > >     on what option is selected for qemu above.
> > >      * Note that KVM uses a simplified model that doesn't try
> > >        to emulate the trailing edge of the interrupt very well
> > >        at all.  I'm not proposing to change this aspect of it.
> > >      * A patch analogous to 7 should be easy.
> > >      * Patches 8 through 10 are also fairly easy by themselves.
> > >        But now we start having an explosion of combinations
> > >        of versions of KVM and qemu and migration to/from, and it
> > >        might be better to:
> > Why explosion of combinations? I do not see any changes in
> > migration code in your series, so as long as we care about migration
> > from in-kernel irqchip to in-kernel irqchip we should be independent
> > from qemu version, no?
> 
> You may be correct.  I'm a little hazy on the details of how things are
> split between KVM and QEMU.  Are there situations that do ioport read/write
> handling within qemu rather than KVM? 
No, all io is handled either by KVM or qemu.

>                                        How about things like pit_get_out(),
> pit_get_next_transition_time(), etc in qemu/hw/i8254_common.c?  (If
> not used when KVM is enabled, then why are they "common"?)  What
> are the implications if qemu and KVM implementations of such
> functions disagree?
> 
They are common because they work on device state that can comes from
either QEMU device emulation or kvm device emulation. Why QEMU even touches
KVM's device state other than for migration I do not know. Jan?

I see that your patch 10 changes pit_get_next_transition_time() and
pit_get_out(). If the result of those functions will be different on
patched and un-patched kernels it may indeed be a problem, but if
kernels are different only in logic and the sate is the same then it
should not be different from patched and un-patched QEMU case.

> > 
> > >      * Or more involved fixes would involve new ioctl()'s and
> > >        command line arguments to select old or fixed 8254 models
> > >        dynamically.  See below.
> > > 
> > > ----------------
> > > Alternative options for improving the i8254 model and migration:
> > > 
> > > 1. Don't fix 8254 at all.  Just apply through patch 7 or 8, and don't try
> > >    to make any additional fixes.  I don't know of any guests that need
> > >    improvements, so this could be a viable option.
> > > 
> > > 2. Just fix it immediately, and don't worry about migration.  Squash
> > >    the last few patches together.  A single missed periodic
> > >    timer tick that only happens when migrating
> > >    between versions of qemu is probably not a significant
> > >    concern.  (Unless someone knows of an OS that actually runs
> > >    the i8254 in single shot mode 4, where a missed interrupt
> > >    could cause a hang or something?)
> > > 
> > If migration can fail only with the single, rarely (if ever) used mode,
> > I honestly like this option the most.
> 
> As long as it is truly rare, I agree.  I'm just not sure if the "rare"
> qualification is actually true or not.  See also my response to Jamie
> Lokier about Linux's tickless configuration.
> 
Will look.

--
			Gleb.

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Jan Kiszka]

On 2012-12-12 12:36, Gleb Natapov wrote:
>>                                        How about things like pit_get_out(),
>> pit_get_next_transition_time(), etc in qemu/hw/i8254_common.c?  (If
>> not used when KVM is enabled, then why are they "common"?)  What
>> are the implications if qemu and KVM implementations of such
>> functions disagree?
>>
> They are common because they work on device state that can comes from
> either QEMU device emulation or kvm device emulation. Why QEMU even touches
> KVM's device state other than for migration I do not know. Jan?

PC speaker emulation.

Jan

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

Re: [Qemu-devel] [PATCH v7 00/10] i8254, i8259 and running Microport UNIX (ca 1987)

[Gleb Natapov]

On Wed, Dec 12, 2012 at 12:38:33PM +0100, Jan Kiszka wrote:
> On 2012-12-12 12:36, Gleb Natapov wrote:
> >>                                        How about things like pit_get_out(),
> >> pit_get_next_transition_time(), etc in qemu/hw/i8254_common.c?  (If
> >> not used when KVM is enabled, then why are they "common"?)  What
> >> are the implications if qemu and KVM implementations of such
> >> functions disagree?
> >>
> > They are common because they work on device state that can comes from
> > either QEMU device emulation or kvm device emulation. Why QEMU even touches
> > KVM's device state other than for migration I do not know. Jan?
> 
> PC speaker emulation.
> 
If it produces a wrong sound during migration I think we can live with
it.

--
			Gleb.