[Qemu-devel] [PATCH v6 0/8] i8254, i8259 and running Microport UNIX (ca 1987)

[Qemu-devel] [PATCH v6 0/8] i8254, i8259 and running Microport UNIX (ca 1987)

[Matthew Ogilvie]

Changes since previous version:
   * Patches 1, 2, 3, and 4, and 7 haven't changed at all.
   * The only change to patch 6 is I added test cases output to the
     commit message.
   * Patches 5 and 8 are new.  Patch 8 has downsides; see
     migration notes below.

Disclaimer: The PIT patches and the analysis below are mostly
based on reading the spec and the code, with only a minimal amount
of sanity testing so far.

---------------------
Remaining inaccuracies in the qemu (user mode) 8254 PIT (timer) model
after these patches:

   * Generally, a new guest-supplied initial count is only
     loaded into the actual counter on the next CLK tick.  This
     could be delayed by about a microsecond (CLK runs at about 1.1 MHz).
   * CLK ticks should be predefined; nothing done in software should
     change their timing.  But we essentially restart CLK (throwing
     out fractional ticks) whenever the guest programs the PIT.
   * It doesn't model a paused counter (for example, when GATE is
     low), and reading back the paused count from the counter.  Also,
     when it is unpaused, it always starts back at the beginning,
     rather than where it was paused.  This shouldn't affect interrupts
     (channel 0's GATE is hard-wired high), but it could affect
     reading back the count from channel 2 (PC speaker).
   * Lost tick risks:
      * When the guest leaves interrupts disabled for an extended
        period of time, the level might return low before the guest
        sees the inturrpt.  This can also happen on real hardware,
        so probably isn't worth worrying about.
      * Depending on the underlying qemu-timer model (used for CLK),
        there might be a risk that the timer could skip a significant
        chunk of time without providing any processor time to the
        8254 model or the guest (perhaps if the host is scheduling
        other processes).  I'm not sure if this is possible
        or likely, but if it happens, the 8254 will dutifully provide
        all the missing edges to the 8259.  But the 8254 will provide
        them all at once (until it catches up to the qemu-timer
        time), and the 8259 and guest will effectively not notice
        nor process the earlier edges in the batch.  Again, not sure
        when/if this can actually happen.  Maybe it is more
        likely to happen with KVM in the "kernel_irqchip=off" case,
        but not in the native case?  Not sure.

My general impression is that none of these are worth worrying about.
They are fairly tangential to any corrections to the interrupt trailing
edge model.

---------------------
Question about best way to handle migration issues:

There is a tradeoff between potentially losing an IRQ0 tick
in some cases, vs potentially gaining an IRQ0 tick in other cases,
and how to deal with interrupts.

NOTE: In addition to the lost leading edge issue mentioned in
the comment in the patch, some of my changes in patch 5 fixes
where the leading edge occurs by 1 CLK count.  Migration might risk
delivering an interrupt twice if it happens in the 1 CLK
interval.  Evaluate ways of dealing with this.  Perhaps
intentionally leave the off-by-one in perpetuity, or is there a
clever way to avoid issues?

I'm not sure how to resolve it, but here are some ideas:

1. Leave out patch 8 and do nothing.  Potentially lose an interrupt
   when migrating back to an older version, because the old version
   always expects to be able to produce a leading edge by simply
   setting IRQ0 high, but if it is normally high...

2. Use patch 8.  If we keep it, it probably belongs near patch 5 or
   6, or squashed in with one of them.  But if the guest uses
   something like the one-shot mode 4, it would get an extra
   interrupt whenever it restarts the timer if the previous
   interrupt has been serviced.  (Some of these could match
   real hardware, if mode 4 happens to be started when IRQ0
   happens to be low.  But this is relatively unlikely.)

3. Use something similar to patch 8, but only in the exported data
   used for migration, not in normal operation.  This could possibly
   generate a single extra interrupt when migrating, but with luck
   most guests should be able to handle that.

4. Or only prepare to fix it sometime in the future:
    * Explicitly do not fix the trailing edge logic for IRQ0 in the
      8259.  Only fix other IRQ's for now.
    * Leave the PIT functionally mostly as-is, except:
       * Try to tweak the PIT model so that it could handle migration
         to/from some future fixed version without problems.  For example,
         when it wants to generate a leading edge, make sure it is a leading
         edge by first explicitly lowering the IRQ, even if it "should
         have" been low already.
    * Wait until sometime in the future (years from now?) when
      back-migrating to 2012 version is no longer important to actually
      fix the PIT interrupt level model.

5. Or implement two models of operation, and a command line switch
   to select one.  At some point, change the default to the more accurate
   model.  And maybe drop the old model sometime in the distant future.

Anyone have any better ideas?  Or preferences for or against any
of the above?

---------------------
Some notes about the KVM (kernel mode) 8254 PIT (timer) model:

As far as it goes, the kernel model appears to need the
similar changes and have similar remaining inaccuracies
was the qemu model.  But it also has other issues:

But it will also have problems related to the different way the
kernel 8254 ties the OUT logic to the interrupt controller.
The kernel makes no attempt to model two edges separately; instead
it delivers both at the same time, in essentially a zero-width
pulse.

This model never looses a tick; it delays delivering a new leading
edge until the previous leading edge has been acknowledged with
an EOI in the 8259.  This functionality may be necessary due to how
the kernel schedules tasks, and the nature of the kernel's timers
(otherwise on a heavily loaded host, the guest could unavoidably
lose a large number of ticks).

It also has the edges backwards.  The almost-zero-width pulse is
currently briefly pulsing positive instead of briefly pulsing
negative, which is clearly won't work at all with a proper model
of the trailing edge.

Clearly the order of the edges in the brief pulse needs to be fixed
if we want to fix the interrupt trailing edge model.  (See migration
considerations above...)

What is less clear is if it is worth the effort to try to model the
trailing edge more accurately than that.  Note that if we want to
keep the tick catchup logic, then anytime it is "catching up" it
is probably not desirable to leave it low for long.  That would
delay or prevent catching up, so in the "catching up" case, we
would essentially fall back on the current simplified model.

Potentially we could model the low level in a few cases (like
when starting mode 0, or getting halfway through counting
mode 3, or the one-CLK-cylce low in various other modes, or we
get the EOI after the IRQ0 should be low, etc), as long as
it isn't trying to catch up.  This could lead to somewhat random
durations of IRQ0=low, which might cause more
intermitment/confusing bugs than just always modeling it as
short-duration.

Perhaps if some rare guest cares about IRQ0=low duration,
then user could just disable the in-kernel IRQ chip for that guest?

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

Matthew Ogilvie (8):
  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
  i8254: fix inaccuracies in pit_get_out()
  i8259: fix so that dropping IRQ level always clears the interrupt
    request
  i8259: refactor pic_set_irq level logic
  i8259/i8254: migration workaround for timer

 hw/cirrus_vga.c   |  4 ++--
 hw/i8254.c        | 24 ++++++++++++++++++--
 hw/i8254_common.c | 18 +++++----------
 hw/i8259.c        | 67 +++++++++++++++++++++++++++++++++++++++++--------------
 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, 186 insertions(+), 67 deletions(-)

-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v6 1/8] 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 9a0a565..107254d 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2055,8 +2055,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 53daf78..6587666 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -355,7 +355,8 @@ static uint64_t pic_ioport_read(void *opaque, target_phys_addr_t 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 e0b9443..dd2855e 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -154,7 +154,7 @@ static uint64_t bmdma_read(void *opaque, target_phys_addr_t 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, target_phys_addr_t 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 b20e4f0..948a469 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -55,7 +55,7 @@ static uint64_t bmdma_read(void *opaque, target_phys_addr_t 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, target_phys_addr_t 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 v6 2/8] 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 8d305ca..e1d2f7e 100644
--- a/vl.c
+++ b/vl.c
@@ -2362,8 +2362,9 @@ int main(int argc, char **argv, char **envp)
     char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */
     DisplayState *ds;
     DisplayChangeListener *dcl;
-    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;
@@ -2418,8 +2419,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;
@@ -2467,7 +2467,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;
 
@@ -2485,21 +2485,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:
@@ -2533,7 +2520,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)
@@ -2565,7 +2555,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 v6 3/8] 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 7d97f96..d473e1c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -975,7 +975,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
@@ -1000,6 +1000,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 v6 4/8] 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 e4db071..37e2f87 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -176,6 +176,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;
+
 static inline DeviceState *isa_vga_init(ISABus *bus)
 {
     ISADevice *dev;
diff --git a/hw/vga.c b/hw/vga.c
index afaef0d..15b5f0d 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;
 
@@ -1891,7 +1908,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 d473e1c..5ba9c19 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1005,6 +1005,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 e1d2f7e..eda5a6e 100644
--- a/vl.c
+++ b/vl.c
@@ -179,6 +179,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;
 int display_remote = 0;
 const char* keyboard_layout = NULL;
@@ -1758,6 +1759,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 v6 5/8] i8254: fix inaccuracies in pit_get_out()

[Matthew Ogilvie]

   * Fix high-vs-low counting logic to match the timing diagrams
     and descriptions in Intel's documentation (23124406.pdf).
   * Improve reading back the count in mode 3.
   * Handle GATE input properly with respect to the OUT line, and add
     a FIXME comment for reading back the counter.  GATE is hard wired
     high for channel 0 (IRQ0), but it can be software controlled on
     channel 2 (PC speaker).

The output line is now high much more often than before, especially
in modes 2 and 4, which might cause updates of the timer chip
to generate extra interrupts.  But this should only be an issue for
some migration cases.

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

There are still some things not quite modeled correctly.
See the cover letter of this patch series for details, and
the FIXME comments added to the code.

 hw/i8254.c        | 24 ++++++++++++++++++++++--
 hw/i8254_common.c | 18 ++++++------------
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/hw/i8254.c b/hw/i8254.c
index 77bd5e8..9168016 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,8 +61,7 @@ static int pit_get_count(PITChannelState *s)
         counter = (s->count - d) & 0xffff;
         break;
     case 3:
-        /* XXX: may be incorrect for odd counts */
-        counter = s->count - ((2 * d) % s->count);
+        counter = (s->count - ((2 * d) % s->count)) & 0xfffe;
         break;
     default:
         counter = s->count - (d % s->count);
@@ -98,6 +106,18 @@ 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);
+
+    /* In gate-triggered one-shot modes, indirectly model some pit_get_out()
+     * cases by setting the load time way back until gate-triggered.
+     * (Generally only affects reading status from channel 2 speaker,
+     * due to hard-wired gates on other channels.)
+     *
+     * FIXME: This might be redesigned if a paused timer state is added
+     * for 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);
 }
diff --git a/hw/i8254_common.c b/hw/i8254_common.c
index a03d7cd..dc13c99 100644
--- a/hw/i8254_common.c
+++ b/hw/i8254_common.c
@@ -50,24 +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:
-        out = (d < s->count);
+        out = (d >= s->count);
         break;
     case 2:
-        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:
-        out = (d % s->count) < ((s->count + 1) >> 1);
+        out = (d % s->count) < ((s->count + 1) >> 1) || s->gate == 0;
         break;
     case 4:
     case 5:
-        out = (d == s->count);
+        out = (d != s->count);
         break;
     }
     return out;
@@ -93,10 +87,10 @@ int64_t pit_get_next_transition_time(PITChannelState *s, int64_t current_time)
         break;
     case 2:
         base = (d / s->count) * s->count;
-        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:
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v6 6/8] 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 6587666..c011787 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 v6 7/8] 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 c011787..1ba9b3a 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 v6 8/8] i8259/i8254: migration workaround for timer

[Matthew Ogilvie]

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

It is not at all clear that this is the best way to handle this.
See the detailed notes in the cover letter of this patch series.

UPDATE: Also, some fixes moved the leading edge by 1 CLK
tick (CLK ticks at about 1.1 MHz), and some strategies like this
might risk extra interrupts just from that.

------

 hw/i8259.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/hw/i8259.c b/hw/i8259.c
index 1ba9b3a..4b3c6c9 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -153,6 +153,45 @@ static void pic_set_irq(void *opaque, int irq, int level)
         s->irr &= ~mask;
         s->last_irr &= ~mask;
     }
+
+    /* Back-migration compatibility hack:
+     * As of late 2012, the PIT timer model was incorrectly
+     * pulsing the the IRQ0 line high for only a short time to
+     * indicate an interrupt.  It counted on a conceptual bug in
+     * the PIC irq model to latch onto and and deliver the
+     * interrupt even after it became low again.  (Normally lowering
+     * an IRQ line before it is serviced should cancel the
+     * interrupt.)
+     *
+     * In late 2012 the model has been improved to match hardware
+     * much better by only pulsing low for a short time (in most
+     * PIT modes), but unfortunately that means if you back-migrate
+     * a guest to a version without this fix, the next interrupt
+     * won't have its own leading edge at all, and will
+     * be lost.
+     *
+     * The following hack will allow both the current
+     * interrupt to be serviced properly, and the next one
+     * as well, regardless of which version the migration is
+     * restored on.
+     *
+     * Unfortunately, this has a small possibility of causing
+     * an extra IRQ0 in cases that would not have in the old 2012
+     * model, nor on real hardware.  Specifically, if the current
+     * interrupt is processed, and then something causes an
+     * pit_irq_timer_update() to the same high level it was previously
+     * updated with.  Re-setting various PIT modes (like 4) could
+     * do this, for example.
+     *
+     * At some point in the future (years from now?),
+     * when back-migration to the old 2012 version is
+     * no longer important, it should be safe to just delete
+     * this hack.
+     */
+    if (irq==0 && s->master) {
+        s->last_irr &= ~1;
+    }
+
     pic_update_irq(s);
 }
 
-- 
1.7.10.2.484.gcd07cc5