[Qemu-devel] [PATCH v4 0/5] Running Microport UNIX (ca 1987)

[Qemu-devel] [PATCH v4 0/5] Running Microport UNIX (ca 1987)

[Matthew Ogilvie]

This series is fixing issues I found when getting qemu to run
"Micoport UNIX System V/386, v 2.1" (ca 1987), although most of
the patches are completely independent of each other.

Changes since v2 and v3:

   - Drop the -no-spurious-interrupts patch.  (It might still be useful
     as an end-user workaround for other potential interrupt bugs, but I'm
     not going to push for it.)

   - Add a completely new patch to fix for how the master i8259
     handles IRQ2 when the original interrupt (say IRQ14) is
     dynamically masked off in the slave via the IMR register.
     This is supported by a test program I wrote.  There are probably
     some tweaks still desired (KVM at least), but I'm fairly
     confident this basic approach is correct.

   - Squash in the remaining two small v3 incremental patches into v2.

   - [applied] The mov to/from crN/drN patch has been applied (and not
     reverted), and is no longer included with this series.


Matthew Ogilvie (5):
  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 dynamically masking slave IRQs with IMR register

 hw/cirrus_vga.c     |  4 ++--
 hw/i8259.c          | 15 ++++++++-----
 hw/i8259_common.c   |  2 ++
 hw/i8259_internal.h |  1 +
 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, 121 insertions(+), 41 deletions(-)

-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v4 1/5] 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>
---

Changes since v2: The v3 tweak (adding back a dropped "02") has been
squashed in.

 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 e8dcc6b..68c36f3 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 v4 2/5] 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 7c577fa..febfd62 100644
--- a/vl.c
+++ b/vl.c
@@ -2352,8 +2352,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;
@@ -2408,8 +2409,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;
@@ -2457,7 +2457,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;
 
@@ -2475,21 +2475,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:
@@ -2523,7 +2510,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)
@@ -2555,7 +2545,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 v4 3/5] 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 3c411c4..3e8085d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -945,7 +945,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
@@ -970,6 +970,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 v4 4/5] 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>
---

Change since v2: The v3 tweak (fix conditions for palette blanking hack)
has been squashed in.

 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 f82ced8..fb08dc0 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -547,14 +547,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;
 
@@ -1886,7 +1903,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 3e8085d..68925f3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -975,6 +975,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 febfd62..16d04a2 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;
@@ -1748,6 +1749,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 v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Matthew Ogilvie]

Although I haven't found any specs that say so, on real hardware
I have a test program that shows if you mask off the slave
interrupt (say IRQ14) in the IMR after it has already been raised,
the master (IRQ2) gets canceled (that is, IRQ2 acts like it is level
triggered).  Without this patch, qemu delivers a
spurious interrupt (IRQ15) instead when running the test program.

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

I've written a test program (in the form of a floppy disk boot sector)
that demonstrates that qemu's emulation of IRQ2 propagation from the
slave i8259 to the master does not work correctly when the CPU has
interrupts disabled and it masks off the original interrupt (IRQ14)
in the slave's IMR register.  This was based on simplifying breakage
observed when trying to run an old Microport UNIX system (ca 1987).

Earlier I speculated that maybe the ELCR bit for IRQ2 was incorrect,
but now I don't think changing the bit (from the target's
perspective) would be a good idea.  See below.

You can download the source code for the test program from
http://home.comcast.net/~mmogilvi/downloads/i8259-imr-test-2012-09-02.tar.bz2
It can be compiled using a standard GNU i386 toolchain on Linux.

The heart of the test program is:

        cli

          # i8259:imm: mask off everything except IRQ2
        movb $0xfb,%al     # master (only IRQ2 clear)
        outb %al,$0x21
        movb $0xff,%al     # slave
        outb %al,$0xa1

        mov $.msgCmdRead,%ax
        call print
        call initIrqHandlers
        call scheduleIrq14

        call .largeDelay   # note: IRQ14 raised while this is waiting

        mov $.msgUnmask,%ax
        call print
        movb $0x3f,%al     # unmask IRQ14 and IRQ15
        outb %al,$0xa1

        call .largeDelay   # (probably not important)

        mov $.msgMask,%ax
        call print
        movb $0xff,%al     # mask IRQ14 and IRQ15 again
        outb %al,$0xa1

        call .largeDelay   # (probably not important)

        mov $.msgSti,%ax
        call print
        sti

        call .largeDelay   # note: spurious interrupt (IRQ15) from qemu

        cli
        mov $.msgUnmask,%ax
        call print
        movb $0x3f,%al     # unmask IRQ14 and IRQ15
        outb %al,$0xa1
        sti

        call .largeDelay   # we should finally see IRQ14 here?

          # DONE:
        cli
        movw $.msgDone, %ax
        call print
  .Lhalt:
        hlt
        jmp .Lhalt

In qemu, the master treats IRQ2 as edge triggered, and delivers a
spurious interrupt if the CPU re-enables interrupts after
the slave's IMR is masked off (it also doesn't seem to deliver
the real interrupt when the IMR is unmasked, but maybe that is
because I'm not correctly acknowledging the spurious interrupt).

  - Qemu output (without this patch):
        elcr=0c00 cmdRead ummask mask sti irq15 unmask DONE

But on real hardware, the master seems to treat IRQ2 as level triggered,
and doesn't deliver an interrupt to the CPU until the slave unmasks IRQ14.
I've tried this on several machines, including a non-PCI 486 that
does not seem to have ELCR registers.

  - Typical output (pentium/pentium pro/dual AMD Athlon/etc; slight
    variations in some elcr bits depending on machine, but bit 0x04
    is always clear):
        elcr=0c20 cmdRead unmask mask sti unmask irq14 DONE

  - 486 without elcr (just an ISA bus):
        elcr=e0e0 cmdRead unmask mask sti unmask irq14 DONE

  - One failure: It doesn't boot properly (no output) with a USB floppy
    drive on my Intel Core I7.  Guess: The test program just barely
    fits in a sector, with no room for any tables (partition/etc)
    that BIOS might check for if it isn't an original, native floppy
    drive.

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

I've found a few descriptions of programming the i8259.
The closest thing I've found to a detailed spec is in
"iAPX 86, 88 User's Manual", dated August 1981:
http://ebookbrowse.com/1981-iapx-86-88-users-manual-pdf-d3089962
It has a significant section titled "Using the 8259A Programmable
Interrupt Controller" starting on page 438 or A-135.

But none of my sources seem to specify how master/slave cascading
interacts with the IMR (mask register) and edge trigger mode,
although it talks about those things individually.
Also, given the date it isn't surprising that it doesn't mention
the elcr registers at all.

I have not found any real specs for the ELCR registers, but I've found a
few hints:

  - Two 8 bit registers: one for master (0x4d0) and one for slave (0x4d1).
  - One bit per IRQ line: 0 for edge trigger, 1 for level trigger.
  - Not present unless the machine has EISA or PCI buses.  Plain
    ISA doesn't have it.
  - Might be implemented completely external to the actual i8259s.
  - As seen in test program output above, ELCR bit 0x04 is clear,
    indicating that IRQ2 is supposedly edge triggered, even though
    actual tested behavior is level triggered.
  - A google search (8259 elcr -linux -qemu) [exclude the
    dominant Linux and qemu related pages] found at least one operating
    system that checks several ELCR bits (including IRQ2) as part
    of a probe to determine if ELCR exists.  So simply setting
    the 0x04 bit is probably a bad idea.  For example, line 119 of:
    https://bitbucket.org/npe/nix/src/35d39df17077/src/9kron2/k8/i8259.c

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

My guess is that if a master IRQ line is marked as coming from a slave
(in the ICW3 register), then the i8259 treats that line as level
triggered regardless of ELCR registers or the LTIM bit of ICW1 (in
addition to other special slave line logic).  Otherwise, the slave
IMR register would seem rather useless.

Under that theory, something like the following patch would be appropriate
for qemu.  This fixes both the test program, and the original Microport
UNIX problem.

Possible concerns with this patch:

  - KVM still needs to be fixed.  I haven't researched this at all,
    beyond noticing that it has the same broken behavior running the
    test program, and the high level symptoms from Microport UNIX
    are slightly different with KVM.
  - It might also be a good idea to add something like my test
    program to qemu/tests somewhere.  This would be a separate patch.
  - Best icw3 configuration strategy?: Should it be stored, or
    just assume it is correctly configured based on
    PICCommonState::master and standard PC IRQ2 configuration?
    Perhaps add sanity checks for reasonable values when the
    guest is configuring the 8259s?  Did I catch all the places
    that need to deal with a new state variable (assuming we
    decide to store it)?
  - This patch is adding code to what may be a relatively common
    code path (every time interrupts raised/lowered/acknowledged, masks
    changed, etc).  Potentially it could be optimized by adding an
    "effective_elcr" state variable, that is maintined as a combination
    of icw3 and elcr (and maybe add support for LTIM bit from ICW1 at the
    same time), so that we don't need to have extra code in the
    critical path.  Does anyone think this is worth it?  I'm leaning
    towards "no".

==========================================

 hw/i8259.c          | 12 ++++++++----
 hw/i8259_common.c   |  2 ++
 hw/i8259_internal.h |  1 +
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 6587666..8e2f9f4 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -140,7 +140,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
     }
 #endif
 
-    if (s->elcr & mask) {
+    if ((s->elcr | s->icw3) & mask) {
         /* level triggered */
         if (level) {
             s->irr |= mask;
@@ -174,7 +174,7 @@ static void pic_intack(PICCommonState *s, int irq)
         s->isr |= (1 << irq);
     }
     /* We don't clear a level sensitive interrupt here */
-    if (!(s->elcr & (1 << irq))) {
+    if (!((s->elcr | s->icw3) & (1 << irq))) {
         s->irr &= ~(1 << irq);
     }
     pic_update_irq(s);
@@ -314,6 +314,10 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
             s->init_state = s->single_mode ? (s->init4 ? 3 : 0) : 2;
             break;
         case 2:
+            s->icw3 = val;
+            /* TODO: Enforce constraints?: Master is typically
+             *   0x04 for IRQ2 (maybe 0 is also OK).  Slave must be 0.
+             */
             if (s->init4) {
                 s->init_state = 3;
             } else {
@@ -419,9 +423,9 @@ void pic_info(Monitor *mon)
     for (i = 0; i < 2; i++) {
         s = i == 0 ? DO_UPCAST(PICCommonState, dev.qdev, isa_pic) : slave_pic;
         monitor_printf(mon, "pic%d: irr=%02x imr=%02x isr=%02x hprio=%d "
-                       "irq_base=%02x rr_sel=%d elcr=%02x fnm=%d\n",
+                       "irq_base=%02x icw3=%02x rr_sel=%d elcr=%02x fnm=%d\n",
                        i, s->irr, s->imr, s->isr, s->priority_add,
-                       s->irq_base, s->read_reg_select, s->elcr,
+                       s->irq_base, s->icw3, s->read_reg_select, s->elcr,
                        s->special_fully_nested_mode);
     }
 }
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..dcde5f2 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
@@ -33,6 +33,7 @@ void pic_reset_common(PICCommonState *s)
     s->isr = 0;
     s->priority_add = 0;
     s->irq_base = 0;
+    s->icw3 = 0;
     s->read_reg_select = 0;
     s->poll = 0;
     s->special_mask = 0;
@@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
         VMSTATE_UINT8(isr, PICCommonState),
         VMSTATE_UINT8(priority_add, PICCommonState),
         VMSTATE_UINT8(irq_base, PICCommonState),
+        VMSTATE_UINT8(icw3, PICCommonState),
         VMSTATE_UINT8(read_reg_select, PICCommonState),
         VMSTATE_UINT8(poll, PICCommonState),
         VMSTATE_UINT8(special_mask, PICCommonState),
diff --git a/hw/i8259_internal.h b/hw/i8259_internal.h
index 4137b61..de67d49 100644
--- a/hw/i8259_internal.h
+++ b/hw/i8259_internal.h
@@ -55,6 +55,7 @@ struct PICCommonState {
     uint8_t isr; /* interrupt service register */
     uint8_t priority_add; /* highest irq priority */
     uint8_t irq_base;
+    uint8_t icw3; /* bit set if line is connected to a slave */
     uint8_t read_reg_select;
     uint8_t poll;
     uint8_t special_mask;
-- 
1.7.10.2.484.gcd07cc5

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 03/09/2012 04:56, Matthew Ogilvie ha scritto:
> Although I haven't found any specs that say so, on real hardware
> I have a test program that shows if you mask off the slave
> interrupt (say IRQ14) in the IMR after it has already been raised,
> the master (IRQ2) gets canceled (that is, IRQ2 acts like it is level
> triggered).  Without this patch, qemu delivers a
> spurious interrupt (IRQ15) instead when running the test program.
> 
> Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>

Nice testing, thanks!

KVM's i8259 emulation can be patched at arch/x86/kvm/i8259.c in the
Linux tree.

You can write a test for kvm-unit-tests.  These tests can be written in
C, including interrupt handlers (see lib/x86/isr.h).  The git tree is at
git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git.

Paolo

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Andreas Färber]

Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
> index ab3d98b..dcde5f2 100644
> --- a/hw/i8259_common.c
> +++ b/hw/i8259_common.c
[...]
> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>          VMSTATE_UINT8(isr, PICCommonState),
>          VMSTATE_UINT8(priority_add, PICCommonState),
>          VMSTATE_UINT8(irq_base, PICCommonState),
> +        VMSTATE_UINT8(icw3, PICCommonState),
>          VMSTATE_UINT8(read_reg_select, PICCommonState),
>          VMSTATE_UINT8(poll, PICCommonState),
>          VMSTATE_UINT8(special_mask, PICCommonState),

Additional VMState needs to be versioned by incrementing .version_id and
by specifying the new version number here. Otherwise it breaks migration.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/03/2012 11:40 AM, Andreas Färber wrote:
> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>> index ab3d98b..dcde5f2 100644
>> --- a/hw/i8259_common.c
>> +++ b/hw/i8259_common.c
> [...]
>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>>          VMSTATE_UINT8(isr, PICCommonState),
>>          VMSTATE_UINT8(priority_add, PICCommonState),
>>          VMSTATE_UINT8(irq_base, PICCommonState),
>> +        VMSTATE_UINT8(icw3, PICCommonState),
>>          VMSTATE_UINT8(read_reg_select, PICCommonState),
>>          VMSTATE_UINT8(poll, PICCommonState),
>>          VMSTATE_UINT8(special_mask, PICCommonState),
> 
> Additional VMState needs to be versioned by incrementing .version_id and
> by specifying the new version number here. Otherwise it breaks migration.

And incrementing the version ID breaks backwards migration.  The correct
solution is subsections, copying Juan and booking a trip to the Mariana
trench.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Juan Quintela]

Avi Kivity <avi@redhat.com> wrote:
> On 09/03/2012 11:40 AM, Andreas Färber wrote:
>> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
>>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>>> index ab3d98b..dcde5f2 100644
>>> --- a/hw/i8259_common.c
>>> +++ b/hw/i8259_common.c
>> [...]
>>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>>>          VMSTATE_UINT8(isr, PICCommonState),
>>>          VMSTATE_UINT8(priority_add, PICCommonState),
>>>          VMSTATE_UINT8(irq_base, PICCommonState),
>>> +        VMSTATE_UINT8(icw3, PICCommonState),
>>>          VMSTATE_UINT8(read_reg_select, PICCommonState),
>>>          VMSTATE_UINT8(poll, PICCommonState),
>>>          VMSTATE_UINT8(special_mask, PICCommonState),
>> 
>> Additional VMState needs to be versioned by incrementing .version_id and
>> by specifying the new version number here. Otherwise it breaks migration.

For the subsection, only sending this when icw3 != 0 is enough?  I am
searching for a test about when we need to send it.

> And incrementing the version ID breaks backwards migration.  The correct
> solution is subsections, copying Juan and booking a trip to the Mariana
> trench.

Book also for me, no need for the return ticket.

Later, Juan.

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 17:42, Juan Quintela wrote:
> Avi Kivity <avi@redhat.com> wrote:
>> On 09/03/2012 11:40 AM, Andreas Färber wrote:
>>> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
>>>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>>>> index ab3d98b..dcde5f2 100644
>>>> --- a/hw/i8259_common.c
>>>> +++ b/hw/i8259_common.c
>>> [...]
>>>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>>>>          VMSTATE_UINT8(isr, PICCommonState),
>>>>          VMSTATE_UINT8(priority_add, PICCommonState),
>>>>          VMSTATE_UINT8(irq_base, PICCommonState),
>>>> +        VMSTATE_UINT8(icw3, PICCommonState),
>>>>          VMSTATE_UINT8(read_reg_select, PICCommonState),
>>>>          VMSTATE_UINT8(poll, PICCommonState),
>>>>          VMSTATE_UINT8(special_mask, PICCommonState),
>>>
>>> Additional VMState needs to be versioned by incrementing .version_id and
>>> by specifying the new version number here. Otherwise it breaks migration.
> 
> For the subsection, only sending this when icw3 != 0 is enough?  I am
> searching for a test about when we need to send it.

See my reply on this topic in the other branch of this thread.

> 
>> And incrementing the version ID breaks backwards migration.  The correct
>> solution is subsections, copying Juan and booking a trip to the Mariana
>> trench.
> 
> Book also for me, no need for the return ticket.
> 

Hey, this scenario is rather harmless (famous last words...). ;)

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/03/2012 06:42 PM, Juan Quintela wrote:
> Avi Kivity <avi@redhat.com> wrote:
>> On 09/03/2012 11:40 AM, Andreas Färber wrote:
>>> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
>>>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>>>> index ab3d98b..dcde5f2 100644
>>>> --- a/hw/i8259_common.c
>>>> +++ b/hw/i8259_common.c
>>> [...]
>>>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>>>>          VMSTATE_UINT8(isr, PICCommonState),
>>>>          VMSTATE_UINT8(priority_add, PICCommonState),
>>>>          VMSTATE_UINT8(irq_base, PICCommonState),
>>>> +        VMSTATE_UINT8(icw3, PICCommonState),
>>>>          VMSTATE_UINT8(read_reg_select, PICCommonState),
>>>>          VMSTATE_UINT8(poll, PICCommonState),
>>>>          VMSTATE_UINT8(special_mask, PICCommonState),
>>> 
>>> Additional VMState needs to be versioned by incrementing .version_id and
>>> by specifying the new version number here. Otherwise it breaks migration.
> 
> For the subsection, only sending this when icw3 != 0 is enough?  I am
> searching for a test about when we need to send it.

Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
bit set in icw3 but clear in eclr).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 17:52, Avi Kivity wrote:
> On 09/03/2012 06:42 PM, Juan Quintela wrote:
>> Avi Kivity <avi@redhat.com> wrote:
>>> On 09/03/2012 11:40 AM, Andreas Färber wrote:
>>>> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
>>>>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>>>>> index ab3d98b..dcde5f2 100644
>>>>> --- a/hw/i8259_common.c
>>>>> +++ b/hw/i8259_common.c
>>>> [...]
>>>>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>>>>>          VMSTATE_UINT8(isr, PICCommonState),
>>>>>          VMSTATE_UINT8(priority_add, PICCommonState),
>>>>>          VMSTATE_UINT8(irq_base, PICCommonState),
>>>>> +        VMSTATE_UINT8(icw3, PICCommonState),
>>>>>          VMSTATE_UINT8(read_reg_select, PICCommonState),
>>>>>          VMSTATE_UINT8(poll, PICCommonState),
>>>>>          VMSTATE_UINT8(special_mask, PICCommonState),
>>>>
>>>> Additional VMState needs to be versioned by incrementing .version_id and
>>>> by specifying the new version number here. Otherwise it breaks migration.
>>
>> For the subsection, only sending this when icw3 != 0 is enough?  I am
>> searching for a test about when we need to send it.
> 
> Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
> bit set in icw3 but clear in eclr).

The standard PC values are optimal: 4 for master, 2 for slave.

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/03/2012 06:54 PM, Jan Kiszka wrote:
> On 2012-09-03 17:52, Avi Kivity wrote:
>> On 09/03/2012 06:42 PM, Juan Quintela wrote:
>>> Avi Kivity <avi@redhat.com> wrote:
>>>> On 09/03/2012 11:40 AM, Andreas Färber wrote:
>>>>> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
>>>>>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>>>>>> index ab3d98b..dcde5f2 100644
>>>>>> --- a/hw/i8259_common.c
>>>>>> +++ b/hw/i8259_common.c
>>>>> [...]
>>>>>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>>>>>>          VMSTATE_UINT8(isr, PICCommonState),
>>>>>>          VMSTATE_UINT8(priority_add, PICCommonState),
>>>>>>          VMSTATE_UINT8(irq_base, PICCommonState),
>>>>>> +        VMSTATE_UINT8(icw3, PICCommonState),
>>>>>>          VMSTATE_UINT8(read_reg_select, PICCommonState),
>>>>>>          VMSTATE_UINT8(poll, PICCommonState),
>>>>>>          VMSTATE_UINT8(special_mask, PICCommonState),
>>>>>
>>>>> Additional VMState needs to be versioned by incrementing .version_id and
>>>>> by specifying the new version number here. Otherwise it breaks migration.
>>>
>>> For the subsection, only sending this when icw3 != 0 is enough?  I am
>>> searching for a test about when we need to send it.
>> 
>> Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
>> bit set in icw3 but clear in eclr).
> 
> The standard PC values are optimal: 4 for master, 2 for slave.

Can you explain why?  I saw that icw3 is always ORed with eclr, so my
condition will catch exactly those cases where a change in behaviour
occurs, and no more.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 17:57, Avi Kivity wrote:
> On 09/03/2012 06:54 PM, Jan Kiszka wrote:
>> On 2012-09-03 17:52, Avi Kivity wrote:
>>> On 09/03/2012 06:42 PM, Juan Quintela wrote:
>>>> Avi Kivity <avi@redhat.com> wrote:
>>>>> On 09/03/2012 11:40 AM, Andreas Färber wrote:
>>>>>> Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
>>>>>>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>>>>>>> index ab3d98b..dcde5f2 100644
>>>>>>> --- a/hw/i8259_common.c
>>>>>>> +++ b/hw/i8259_common.c
>>>>>> [...]
>>>>>>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>>>>>>>          VMSTATE_UINT8(isr, PICCommonState),
>>>>>>>          VMSTATE_UINT8(priority_add, PICCommonState),
>>>>>>>          VMSTATE_UINT8(irq_base, PICCommonState),
>>>>>>> +        VMSTATE_UINT8(icw3, PICCommonState),
>>>>>>>          VMSTATE_UINT8(read_reg_select, PICCommonState),
>>>>>>>          VMSTATE_UINT8(poll, PICCommonState),
>>>>>>>          VMSTATE_UINT8(special_mask, PICCommonState),
>>>>>>
>>>>>> Additional VMState needs to be versioned by incrementing .version_id and
>>>>>> by specifying the new version number here. Otherwise it breaks migration.
>>>>
>>>> For the subsection, only sending this when icw3 != 0 is enough?  I am
>>>> searching for a test about when we need to send it.
>>>
>>> Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
>>> bit set in icw3 but clear in eclr).
>>
>> The standard PC values are optimal: 4 for master, 2 for slave.
> 
> Can you explain why?  I saw that icw3 is always ORed with eclr, so my
> condition will catch exactly those cases where a change in behaviour
> occurs, and no more.

The values above are what every user of the PIC cascaded on our targets
must program to use them. So We will find them in the state once any
relevant guest code was able to run (e.g. the BIOS).

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/03/2012 07:02 PM, Jan Kiszka wrote:

>>>> Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
>>>> bit set in icw3 but clear in eclr).
>>>
>>> The standard PC values are optimal: 4 for master, 2 for slave.
>> 
>> Can you explain why?  I saw that icw3 is always ORed with eclr, so my
>> condition will catch exactly those cases where a change in behaviour
>> occurs, and no more.
> 
> The values above are what every user of the PIC cascaded on our targets
> must program to use them. So We will find them in the state once any
> relevant guest code was able to run (e.g. the BIOS).
> 

Suppose the bios has not run yet?


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 03/09/2012 18:15, Avi Kivity ha scritto:
>> > The values above are what every user of the PIC cascaded on our targets
>> > must program to use them. So We will find them in the state once any
>> > relevant guest code was able to run (e.g. the BIOS).
>> > 
> Suppose the bios has not run yet?

Then you transmit the subsection.

BTW this also means that simply checking against eclr|icw3 is wrong; the
right test is:

* against elcr if !s->master

* against elcr|icw3 if s->master

This makes precomputing the value more appealing.

Similarly, perhaps this:

    if (s->special_fully_nested_mode && s->master) {
        mask &= ~(1 << 2);
    }

should be changed to

    if (s->special_fully_nested_mode && s->master) {
        mask &= ~s->icw3;
    }

?

Paolo

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/03/2012 07:23 PM, Paolo Bonzini wrote:
> Il 03/09/2012 18:15, Avi Kivity ha scritto:
>>> > The values above are what every user of the PIC cascaded on our targets
>>> > must program to use them. So We will find them in the state once any
>>> > relevant guest code was able to run (e.g. the BIOS).
>>> > 
>> Suppose the bios has not run yet?
> 
> Then you transmit the subsection.

And the migration fails.  Needlessly, since icw3 == 0 doesn't affect
guest operation.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 03/09/2012 18:30, Avi Kivity ha scritto:
>>>>> The values above are what every user of the PIC cascaded on our targets
>>>>> >>> > must program to use them. So We will find them in the state once any
>>>>> >>> > relevant guest code was able to run (e.g. the BIOS).
>>>>> >>> > 
>>> >> Suppose the bios has not run yet?
>> > 
>> > Then you transmit the subsection.
> And the migration fails.  Needlessly, since icw3 == 0 doesn't affect
> guest operation.

But the point of subsections is to succeed migration in the common case,
assuming there is more than one case that doesn't affect guest operation.

Paolo

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 18:33, Paolo Bonzini wrote:
> Il 03/09/2012 18:30, Avi Kivity ha scritto:
>>>>>> The values above are what every user of the PIC cascaded on our targets
>>>>>>>>>> must program to use them. So We will find them in the state once any
>>>>>>>>>> relevant guest code was able to run (e.g. the BIOS).
>>>>>>>>>>
>>>>>> Suppose the bios has not run yet?
>>>>
>>>> Then you transmit the subsection.
>> And the migration fails.  Needlessly, since icw3 == 0 doesn't affect
>> guest operation.
> 
> But the point of subsections is to succeed migration in the common case,
> assuming there is more than one case that doesn't affect guest operation.

The point is that the common case is icw3 = 4 (for the master), not 0.
And if we don't save that case, we must save the rest. If we don't save
this as well, we lose state information. This can't work.

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 03/09/2012 18:40, Jan Kiszka ha scritto:
>>> >> And the migration fails.  Needlessly, since icw3 == 0 doesn't affect
>>> >> guest operation.
>> > 
>> > But the point of subsections is to succeed migration in the common case,
>> > assuming there is more than one case that doesn't affect guest operation.
> The point is that the common case is icw3 = 4 (for the master), not 0.
> And if we don't save that case, we must save the rest. If we don't save
> this as well, we lose state information. This can't work.

I was agreeing with you, not Avi. :)

Paolo

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/03/2012 07:33 PM, Paolo Bonzini wrote:
> Il 03/09/2012 18:30, Avi Kivity ha scritto:
>>>>>> The values above are what every user of the PIC cascaded on our targets
>>>>>> >>> > must program to use them. So We will find them in the state once any
>>>>>> >>> > relevant guest code was able to run (e.g. the BIOS).
>>>>>> >>> > 
>>>> >> Suppose the bios has not run yet?
>>> > 
>>> > Then you transmit the subsection.
>> And the migration fails.  Needlessly, since icw3 == 0 doesn't affect
>> guest operation.
> 
> But the point of subsections is to succeed migration in the common case,
> assuming there is more than one case that doesn't affect guest operation.

According to the patch, if icw3 == 4 && !(eclr & 4), then behaviour will
change.  With the standard configuration, if two pci interrupts hit at
once, then before the patch irr.2 will be clear, and afterwards set.

So we do have a behavioural change.  Is the rest of the code masking
this change under the standard configuration?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 04/09/2012 10:16, Avi Kivity ha scritto:
>> > But the point of subsections is to succeed migration in the common case,
>> > assuming there is more than one case that doesn't affect guest operation.
> According to the patch, if icw3 == 4 && !(eclr & 4), then behaviour will
> change.  With the standard configuration, if two pci interrupts hit at
> once, then before the patch irr.2 will be clear, and afterwards set.
> 
> So we do have a behavioural change.  Is the rest of the code masking
> this change under the standard configuration?

No, it is not masking the change.  The assumption is that nothing should
care about irr.2 or isr.2, because nothing attaches an handler to the
cascade interrupt.

You have to choose between assuming this, and breaking backwards
migration.  I would rather break backwards migration, but others disagree...

Paolo

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/04/2012 12:15 PM, Paolo Bonzini wrote:
> Il 04/09/2012 10:16, Avi Kivity ha scritto:
>>> > But the point of subsections is to succeed migration in the common case,
>>> > assuming there is more than one case that doesn't affect guest operation.
>> According to the patch, if icw3 == 4 && !(eclr & 4), then behaviour will
>> change.  With the standard configuration, if two pci interrupts hit at
>> once, then before the patch irr.2 will be clear, and afterwards set.
>> 
>> So we do have a behavioural change.  Is the rest of the code masking
>> this change under the standard configuration?
> 
> No, it is not masking the change.  The assumption is that nothing should
> care about irr.2 or isr.2, because nothing attaches an handler to the
> cascade interrupt.

Won't the next call to pic_get_irq() notice the difference in s->irr?

> You have to choose between assuming this, and breaking backwards
> migration.  I would rather break backwards migration, but others disagree...

Normally I'd agree, but if the only known breakee is a 1987 guest then
I'd make an exception.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[BALATON Zoltan]

On Tue, 4 Sep 2012, Avi Kivity wrote:
> On 09/04/2012 12:15 PM, Paolo Bonzini wrote:
>> Il 04/09/2012 10:16, Avi Kivity ha scritto:
>>>>> But the point of subsections is to succeed migration in the common case,
>>>>> assuming there is more than one case that doesn't affect guest operation.
>>> According to the patch, if icw3 == 4 && !(eclr & 4), then behaviour will
>>> change.  With the standard configuration, if two pci interrupts hit at
>>> once, then before the patch irr.2 will be clear, and afterwards set.
>>>
>>> So we do have a behavioural change.  Is the rest of the code masking
>>> this change under the standard configuration?
>>
>> No, it is not masking the change.  The assumption is that nothing should
>> care about irr.2 or isr.2, because nothing attaches an handler to the
>> cascade interrupt.
>
> Won't the next call to pic_get_irq() notice the difference in s->irr?
>
>> You have to choose between assuming this, and breaking backwards
>> migration.  I would rather break backwards migration, but others disagree...
>
> Normally I'd agree, but if the only known breakee is a 1987 guest then
> I'd make an exception.

Another one affected by this is OpenStep 4.2 (probably NeXTstep and 
Rhapsody too) which are not exactly recent either but there are more than 
only one "breakee".

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/04/2012 12:29 PM, BALATON Zoltan wrote:
> On Tue, 4 Sep 2012, Avi Kivity wrote:
>> On 09/04/2012 12:15 PM, Paolo Bonzini wrote:
>>> Il 04/09/2012 10:16, Avi Kivity ha scritto:
>>>>>> But the point of subsections is to succeed migration in the common
>>>>>> case,
>>>>>> assuming there is more than one case that doesn't affect guest
>>>>>> operation.
>>>> According to the patch, if icw3 == 4 && !(eclr & 4), then behaviour
>>>> will
>>>> change.  With the standard configuration, if two pci interrupts hit at
>>>> once, then before the patch irr.2 will be clear, and afterwards set.
>>>>
>>>> So we do have a behavioural change.  Is the rest of the code masking
>>>> this change under the standard configuration?
>>>
>>> No, it is not masking the change.  The assumption is that nothing should
>>> care about irr.2 or isr.2, because nothing attaches an handler to the
>>> cascade interrupt.
>>
>> Won't the next call to pic_get_irq() notice the difference in s->irr?
>>
>>> You have to choose between assuming this, and breaking backwards
>>> migration.  I would rather break backwards migration, but others
>>> disagree...
>>
>> Normally I'd agree, but if the only known breakee is a 1987 guest then
>> I'd make an exception.
> 
> Another one affected by this is OpenStep 4.2 (probably NeXTstep and
> Rhapsody too) which are not exactly recent either but there are more
> than only one "breakee".

Those are all filed under "esoterics".

I don't mean to say we shouldn't care about them, but there are likely
to be a lot more users doing backwards migration than users running
those guests, let alone migrating them (forwards or backwards).  The
pragmatic choice is clear.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-04 11:37, Avi Kivity wrote:
> On 09/04/2012 12:29 PM, BALATON Zoltan wrote:
>> On Tue, 4 Sep 2012, Avi Kivity wrote:
>>> On 09/04/2012 12:15 PM, Paolo Bonzini wrote:
>>>> Il 04/09/2012 10:16, Avi Kivity ha scritto:
>>>>>>> But the point of subsections is to succeed migration in the common
>>>>>>> case,
>>>>>>> assuming there is more than one case that doesn't affect guest
>>>>>>> operation.
>>>>> According to the patch, if icw3 == 4 && !(eclr & 4), then behaviour
>>>>> will
>>>>> change.  With the standard configuration, if two pci interrupts hit at
>>>>> once, then before the patch irr.2 will be clear, and afterwards set.
>>>>>
>>>>> So we do have a behavioural change.  Is the rest of the code masking
>>>>> this change under the standard configuration?
>>>>
>>>> No, it is not masking the change.  The assumption is that nothing should
>>>> care about irr.2 or isr.2, because nothing attaches an handler to the
>>>> cascade interrupt.
>>>
>>> Won't the next call to pic_get_irq() notice the difference in s->irr?
>>>
>>>> You have to choose between assuming this, and breaking backwards
>>>> migration.  I would rather break backwards migration, but others
>>>> disagree...
>>>
>>> Normally I'd agree, but if the only known breakee is a 1987 guest then
>>> I'd make an exception.
>>
>> Another one affected by this is OpenStep 4.2 (probably NeXTstep and
>> Rhapsody too) which are not exactly recent either but there are more
>> than only one "breakee".
> 
> Those are all filed under "esoterics".
> 
> I don't mean to say we shouldn't care about them, but there are likely
> to be a lot more users doing backwards migration than users running
> those guests, let alone migrating them (forwards or backwards).  The
> pragmatic choice is clear.

BTW, did anyone actually test backward migration recently? I thought to
remember I effectively broke it in 1.1 with some changes to the i8259
(or was it the PIT?) vmstate, and no one really cared about this or my
first proposals to fix it.

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 04/09/2012 11:51, Jan Kiszka ha scritto:
>> > 
>> > I don't mean to say we shouldn't care about them, but there are likely
>> > to be a lot more users doing backwards migration than users running
>> > those guests, let alone migrating them (forwards or backwards).  The
>> > pragmatic choice is clear.
> BTW, did anyone actually test backward migration recently? I thought to
> remember I effectively broke it in 1.1 with some changes to the i8259
> (or was it the PIT?) vmstate, and no one really cared about this or my
> first proposals to fix it.

Correct: commit ce967e2 (i8254: Rework & fix interaction with HPET in
legacy mode, 2012-02-01) bumped the PIT version from 2 to 3.  RTC
changes will break it more in 1.3.

Honestly, backwards migration only works on "enterprise" qemu-kvm
because it is tested only there.  And so far the only sample across
major releases is that RHEL6->RHEL5 migration didn't work.

Here the choice is between changing guest behavior by defaulting to 4/2,
and always transmitting the subsection by defaulting to 0/0.  The latter
makes the subsection useless, so at that point we might as well bump the
version number and board said flight to the Pacific.

Paolo

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Avi Kivity]

On 09/04/2012 01:06 PM, Paolo Bonzini wrote:
> Il 04/09/2012 11:51, Jan Kiszka ha scritto:
>>> > 
>>> > I don't mean to say we shouldn't care about them, but there are likely
>>> > to be a lot more users doing backwards migration than users running
>>> > those guests, let alone migrating them (forwards or backwards).  The
>>> > pragmatic choice is clear.
>> BTW, did anyone actually test backward migration recently? I thought to
>> remember I effectively broke it in 1.1 with some changes to the i8259
>> (or was it the PIT?) vmstate, and no one really cared about this or my
>> first proposals to fix it.
> 
> Correct: commit ce967e2 (i8254: Rework & fix interaction with HPET in
> legacy mode, 2012-02-01) bumped the PIT version from 2 to 3.  

Gah, this is 1.1, so there's no way to fix it.

> RTC
> changes will break it more in 1.3.
> 
> Honestly, backwards migration only works on "enterprise" qemu-kvm
> because it is tested only there.  And so far the only sample across
> major releases is that RHEL6->RHEL5 migration didn't work.

I expect that we will want 7->6, though we may have to settle for minor
releases only.

> Here the choice is between changing guest behavior by defaulting to 4/2,
> and always transmitting the subsection by defaulting to 0/0.  The latter
> makes the subsection useless, so at that point we might as well bump the
> version number and board said flight to the Pacific.

So we do the 4/2 thing.  I expect if we go they'll tell us they don't do
backwards flights.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 18:15, Avi Kivity wrote:
> On 09/03/2012 07:02 PM, Jan Kiszka wrote:
> 
>>>>> Looks like the optimal condition is ((s->icw3 & ~s->eclr) != 0) (i.e.
>>>>> bit set in icw3 but clear in eclr).
>>>>
>>>> The standard PC values are optimal: 4 for master, 2 for slave.
>>>
>>> Can you explain why?  I saw that icw3 is always ORed with eclr, so my
>>> condition will catch exactly those cases where a change in behaviour
>>> occurs, and no more.
>>
>> The values above are what every user of the PIC cascaded on our targets
>> must program to use them. So We will find them in the state once any
>> relevant guest code was able to run (e.g. the BIOS).
>>
> 
> Suppose the bios has not run yet?

Then we must save the 0.

Your logic will force us to save in all standard cases (ELCR's bit for
IRQ2 must not be set by a guest). So it's not really helpful.

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

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

On 2012-09-03 04:56, Matthew Ogilvie wrote:
> Although I haven't found any specs that say so, on real hardware
> I have a test program that shows if you mask off the slave
> interrupt (say IRQ14) in the IMR after it has already been raised,
> the master (IRQ2) gets canceled (that is, IRQ2 acts like it is level
> triggered).  Without this patch, qemu delivers a
> spurious interrupt (IRQ15) instead when running the test program.
> 
> Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
> ---
> 
> I've written a test program (in the form of a floppy disk boot sector)
> that demonstrates that qemu's emulation of IRQ2 propagation from the
> slave i8259 to the master does not work correctly when the CPU has
> interrupts disabled and it masks off the original interrupt (IRQ14)
> in the slave's IMR register.  This was based on simplifying breakage
> observed when trying to run an old Microport UNIX system (ca 1987).
> 
> Earlier I speculated that maybe the ELCR bit for IRQ2 was incorrect,
> but now I don't think changing the bit (from the target's
> perspective) would be a good idea.  See below.

Yes, you cannot set IRQ0..2 to level triggered mode on modern chipsets,
look e.g. at the ICH9 spec:

"In edge mode, (bit[x] = 0), the interrupt is recognized by a low to
high transition. In level mode (bit[x] = 1), the interrupt is recognized
by a high level. The cascade channel, IRQ2, the heart beat timer (IRQ0),
and the keyboard controller (IRQ1), cannot be put into level mode."

So, fiddling with ELCR from guest perspective cannot be the solution.

> 
> You can download the source code for the test program from
> http://home.comcast.net/~mmogilvi/downloads/i8259-imr-test-2012-09-02.tar.bz2
> It can be compiled using a standard GNU i386 toolchain on Linux.

As Paolo said, we are better off with a KVM unit test. That should be
loadable via grub & co. on real HW as well.

> 
> The heart of the test program is:
> 

Initialization is also important to exclude leftovers of the BIOS.

>         cli
> 
>           # i8259:imm: mask off everything except IRQ2
>         movb $0xfb,%al     # master (only IRQ2 clear)
>         outb %al,$0x21
>         movb $0xff,%al     # slave
>         outb %al,$0xa1
> 
>         mov $.msgCmdRead,%ax
>         call print
>         call initIrqHandlers
>         call scheduleIrq14
> 
>         call .largeDelay   # note: IRQ14 raised while this is waiting
> 
>         mov $.msgUnmask,%ax
>         call print
>         movb $0x3f,%al     # unmask IRQ14 and IRQ15
>         outb %al,$0xa1
> 
>         call .largeDelay   # (probably not important)
> 
>         mov $.msgMask,%ax
>         call print
>         movb $0xff,%al     # mask IRQ14 and IRQ15 again
>         outb %al,$0xa1

And now check IRR of the master by reading it back. That would be a
smoking gun if bit 2 is set before and cleared afterward.

> 
>         call .largeDelay   # (probably not important)
> 
>         mov $.msgSti,%ax
>         call print
>         sti
> 
>         call .largeDelay   # note: spurious interrupt (IRQ15) from qemu
> 
>         cli
>         mov $.msgUnmask,%ax
>         call print
>         movb $0x3f,%al     # unmask IRQ14 and IRQ15
>         outb %al,$0xa1
>         sti
> 
>         call .largeDelay   # we should finally see IRQ14 here?
> 
>           # DONE:
>         cli
>         movw $.msgDone, %ax
>         call print
>   .Lhalt:
>         hlt
>         jmp .Lhalt
> 
> In qemu, the master treats IRQ2 as edge triggered, and delivers a
> spurious interrupt if the CPU re-enables interrupts after
> the slave's IMR is masked off (it also doesn't seem to deliver
> the real interrupt when the IMR is unmasked, but maybe that is
> because I'm not correctly acknowledging the spurious interrupt).

Yes, that is required.

> 
>   - Qemu output (without this patch):
>         elcr=0c00 cmdRead ummask mask sti irq15 unmask DONE
> 
> But on real hardware, the master seems to treat IRQ2 as level triggered,
> and doesn't deliver an interrupt to the CPU until the slave unmasks IRQ14.
> I've tried this on several machines, including a non-PCI 486 that
> does not seem to have ELCR registers.
> 
>   - Typical output (pentium/pentium pro/dual AMD Athlon/etc; slight
>     variations in some elcr bits depending on machine, but bit 0x04
>     is always clear):
>         elcr=0c20 cmdRead unmask mask sti unmask irq14 DONE
> 
>   - 486 without elcr (just an ISA bus):
>         elcr=e0e0 cmdRead unmask mask sti unmask irq14 DONE
> 
>   - One failure: It doesn't boot properly (no output) with a USB floppy
>     drive on my Intel Core I7.  Guess: The test program just barely
>     fits in a sector, with no room for any tables (partition/etc)
>     that BIOS might check for if it isn't an original, native floppy
>     drive.
> 
> -----------------------
> 
> I've found a few descriptions of programming the i8259.

I was mostly following the official "8259A PROGRAMMABLE INTERRUPT
CONTROLLER (8259A 8259A-2)" by Intel. But it also doesn't mention any
trigger mode changes for the cascading input lines.

> The closest thing I've found to a detailed spec is in
> "iAPX 86, 88 User's Manual", dated August 1981:
> http://ebookbrowse.com/1981-iapx-86-88-users-manual-pdf-d3089962
> It has a significant section titled "Using the 8259A Programmable
> Interrupt Controller" starting on page 438 or A-135.
> 
> But none of my sources seem to specify how master/slave cascading
> interacts with the IMR (mask register) and edge trigger mode,
> although it talks about those things individually.
> Also, given the date it isn't surprising that it doesn't mention
> the elcr registers at all.
> 
> I have not found any real specs for the ELCR registers, but I've found a
> few hints:

Intel chipset docs describe it, though only briefly.

> 
>   - Two 8 bit registers: one for master (0x4d0) and one for slave (0x4d1).
>   - One bit per IRQ line: 0 for edge trigger, 1 for level trigger.
>   - Not present unless the machine has EISA or PCI buses.  Plain
>     ISA doesn't have it.
>   - Might be implemented completely external to the actual i8259s.
>   - As seen in test program output above, ELCR bit 0x04 is clear,
>     indicating that IRQ2 is supposedly edge triggered, even though
>     actual tested behavior is level triggered.
>   - A google search (8259 elcr -linux -qemu) [exclude the
>     dominant Linux and qemu related pages] found at least one operating
>     system that checks several ELCR bits (including IRQ2) as part
>     of a probe to determine if ELCR exists.  So simply setting
>     the 0x04 bit is probably a bad idea.  For example, line 119 of:
>     https://bitbucket.org/npe/nix/src/35d39df17077/src/9kron2/k8/i8259.c
> 
> -----------------------
> 
> My guess is that if a master IRQ line is marked as coming from a slave
> (in the ICW3 register), then the i8259 treats that line as level
> triggered regardless of ELCR registers or the LTIM bit of ICW1 (in
> addition to other special slave line logic).  Otherwise, the slave
> IMR register would seem rather useless.

That IMR is surely not useless, even if we do not automatically revoke
the IRQ2 event. It would just require more careful unmasking in that case.

> 
> Under that theory, something like the following patch would be appropriate
> for qemu.  This fixes both the test program, and the original Microport
> UNIX problem.
> 
> Possible concerns with this patch:
> 
>   - KVM still needs to be fixed.  I haven't researched this at all,
>     beyond noticing that it has the same broken behavior running the
>     test program, and the high level symptoms from Microport UNIX
>     are slightly different with KVM.
>   - It might also be a good idea to add something like my test
>     program to qemu/tests somewhere.  This would be a separate patch.
>   - Best icw3 configuration strategy?: Should it be stored, or
>     just assume it is correctly configured based on
>     PICCommonState::master and standard PC IRQ2 configuration?

Let's avoid adding more logic that assumes cascading IRQ == 2.

>     Perhaps add sanity checks for reasonable values when the
>     guest is configuring the 8259s?  Did I catch all the places

No need for sanity checks if the logic is generic enough.

>     that need to deal with a new state variable (assuming we
>     decide to store it)?
>   - This patch is adding code to what may be a relatively common
>     code path (every time interrupts raised/lowered/acknowledged, masks
>     changed, etc).  Potentially it could be optimized by adding an
>     "effective_elcr" state variable, that is maintined as a combination
>     of icw3 and elcr (and maybe add support for LTIM bit from ICW1 at the
>     same time), so that we don't need to have extra code in the
>     critical path.  Does anyone think this is worth it?  I'm leaning
>     towards "no".

No need for such a micro-optimization.

> 
> ==========================================
> 
>  hw/i8259.c          | 12 ++++++++----
>  hw/i8259_common.c   |  2 ++
>  hw/i8259_internal.h |  1 +
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i8259.c b/hw/i8259.c
> index 6587666..8e2f9f4 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -140,7 +140,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
>      }
>  #endif
>  
> -    if (s->elcr & mask) {
> +    if ((s->elcr | s->icw3) & mask) {
>          /* level triggered */
>          if (level) {
>              s->irr |= mask;
> @@ -174,7 +174,7 @@ static void pic_intack(PICCommonState *s, int irq)
>          s->isr |= (1 << irq);
>      }
>      /* We don't clear a level sensitive interrupt here */
> -    if (!(s->elcr & (1 << irq))) {
> +    if (!((s->elcr | s->icw3) & (1 << irq))) {
>          s->irr &= ~(1 << irq);
>      }
>      pic_update_irq(s);
> @@ -314,6 +314,10 @@ static void pic_ioport_write(void *opaque, target_phys_addr_t addr64,
>              s->init_state = s->single_mode ? (s->init4 ? 3 : 0) : 2;
>              break;
>          case 2:
> +            s->icw3 = val;
> +            /* TODO: Enforce constraints?: Master is typically
> +             *   0x04 for IRQ2 (maybe 0 is also OK).  Slave must be 0.
> +             */

No constraints, see above (and slave must be 2, its ID == cascading IRQ#).

>              if (s->init4) {
>                  s->init_state = 3;
>              } else {
> @@ -419,9 +423,9 @@ void pic_info(Monitor *mon)
>      for (i = 0; i < 2; i++) {
>          s = i == 0 ? DO_UPCAST(PICCommonState, dev.qdev, isa_pic) : slave_pic;
>          monitor_printf(mon, "pic%d: irr=%02x imr=%02x isr=%02x hprio=%d "
> -                       "irq_base=%02x rr_sel=%d elcr=%02x fnm=%d\n",
> +                       "irq_base=%02x icw3=%02x rr_sel=%d elcr=%02x fnm=%d\n",
>                         i, s->irr, s->imr, s->isr, s->priority_add,
> -                       s->irq_base, s->read_reg_select, s->elcr,
> +                       s->irq_base, s->icw3, s->read_reg_select, s->elcr,
>                         s->special_fully_nested_mode);
>      }
>  }
> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
> index ab3d98b..dcde5f2 100644
> --- a/hw/i8259_common.c
> +++ b/hw/i8259_common.c
> @@ -33,6 +33,7 @@ void pic_reset_common(PICCommonState *s)
>      s->isr = 0;
>      s->priority_add = 0;
>      s->irq_base = 0;
> +    s->icw3 = 0;
>      s->read_reg_select = 0;
>      s->poll = 0;
>      s->special_mask = 0;
> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>          VMSTATE_UINT8(isr, PICCommonState),
>          VMSTATE_UINT8(priority_add, PICCommonState),
>          VMSTATE_UINT8(irq_base, PICCommonState),
> +        VMSTATE_UINT8(icw3, PICCommonState),

Let's add this as an optional subsection, only written when it's not
0x04 (for a master) or 0x2 (for a slave). See target-i386/machine.c for
examples, or read docs/migration.txt. That will mean you need to set
icw3 to 0x4 before loading a vmstate (=> pre_load handler).

The effect is that, generally, no additional state will be written in
practice - unless we save right after reset or some guest messes its PIC
configuration up.

>          VMSTATE_UINT8(read_reg_select, PICCommonState),
>          VMSTATE_UINT8(poll, PICCommonState),
>          VMSTATE_UINT8(special_mask, PICCommonState),
> diff --git a/hw/i8259_internal.h b/hw/i8259_internal.h
> index 4137b61..de67d49 100644
> --- a/hw/i8259_internal.h
> +++ b/hw/i8259_internal.h
> @@ -55,6 +55,7 @@ struct PICCommonState {
>      uint8_t isr; /* interrupt service register */
>      uint8_t priority_add; /* highest irq priority */
>      uint8_t irq_base;
> +    uint8_t icw3; /* bit set if line is connected to a slave */

Comment is wrong. This word is used for masters and slaves.

>      uint8_t read_reg_select;
>      uint8_t poll;
>      uint8_t special_mask;
> 

The only thing that worries me is that we just consider the PC so far
while the i8259 is also used on different architectures (PPC, MIPS, Alpha?).

Jan


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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

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

On 2012-09-03 10:51, Jan Kiszka wrote:
>> diff --git a/hw/i8259_common.c b/hw/i8259_common.c
>> index ab3d98b..dcde5f2 100644
>> --- a/hw/i8259_common.c
>> +++ b/hw/i8259_common.c
>> @@ -33,6 +33,7 @@ void pic_reset_common(PICCommonState *s)
>>      s->isr = 0;
>>      s->priority_add = 0;
>>      s->irq_base = 0;
>> +    s->icw3 = 0;
>>      s->read_reg_select = 0;
>>      s->poll = 0;
>>      s->special_mask = 0;
>> @@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
>>          VMSTATE_UINT8(isr, PICCommonState),
>>          VMSTATE_UINT8(priority_add, PICCommonState),
>>          VMSTATE_UINT8(irq_base, PICCommonState),
>> +        VMSTATE_UINT8(icw3, PICCommonState),
> 
> Let's add this as an optional subsection, only written when it's not
> 0x04 (for a master) or 0x2 (for a slave). See target-i386/machine.c for
> examples, or read docs/migration.txt. That will mean you need to set
> icw3 to 0x4 before loading a vmstate (=> pre_load handler).

Oh, and this code affects also the kvm-pic. So you have to maintain icw3
for that model as well, setting it to the PC defaults. Please test KVM
after your changes, including migration.

Jan



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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 03/09/2012 10:51, Jan Kiszka ha scritto:
> The only thing that worries me is that we just consider the PC so far
> while the i8259 is also used on different architectures (PPC, MIPS, Alpha?).

Why is this a problem?  All of them use IRQ2 for a cascade, and initialize
icw3 to 0x4/0x2 (I checked OpenBIOS, rth's palcode for Alpha, and Linux).

BTW, from the palcode it looks like Alpha wants LTIM=1, so it would be nice
to implement that one as well:

  /* ??? MILO initializes the PIC as edge triggered; I do not know how SRM
     initializes them.  However, Linux seems to expect that these are level
     triggered.  That may be a kernel bug, but level triggers are more
     reliable anyway so lets go with that.  */

  /* Initialize level triggers.  The CY82C693UB that's on real alpha
     hardware doesn't have this; this is a PIIX extension.  However,
     QEMU doesn't implement regular level triggers.  */
  outb(0xff, PORT_PIC2_ELCR);
  outb(0xff, PORT_PIC1_ELCR);

Paolo

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 11:34, Paolo Bonzini wrote:
> Il 03/09/2012 10:51, Jan Kiszka ha scritto:
>> The only thing that worries me is that we just consider the PC so far
>> while the i8259 is also used on different architectures (PPC, MIPS, Alpha?).
> 
> Why is this a problem?  All of them use IRQ2 for a cascade, and initialize
> icw3 to 0x4/0x2 (I checked OpenBIOS, rth's palcode for Alpha, and Linux).

IRQ2 is already hard-coded in QEMU (we had to patch this for our machine
emulation, but less in recent versions), that is not the point. I'm
concerned about the behavioral changes we are discussing here, ie. the
special handling of cascading interrupt inputs.

> 
> BTW, from the palcode it looks like Alpha wants LTIM=1, so it would be nice
> to implement that one as well:
> 
>   /* ??? MILO initializes the PIC as edge triggered; I do not know how SRM
>      initializes them.  However, Linux seems to expect that these are level
>      triggered.  That may be a kernel bug, but level triggers are more
>      reliable anyway so lets go with that.  */
> 
>   /* Initialize level triggers.  The CY82C693UB that's on real alpha
>      hardware doesn't have this; this is a PIIX extension.  However,
>      QEMU doesn't implement regular level triggers.  */
>   outb(0xff, PORT_PIC2_ELCR);
>   outb(0xff, PORT_PIC1_ELCR);

Just takes someone to write the patch, as usual. :)

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 03/09/2012 12:34, Jan Kiszka ha scritto:
>> > Why is this a problem?  All of them use IRQ2 for a cascade, and initialize
>> > icw3 to 0x4/0x2 (I checked OpenBIOS, rth's palcode for Alpha, and Linux).
> IRQ2 is already hard-coded in QEMU (we had to patch this for our machine
> emulation, but less in recent versions), that is not the point. I'm
> concerned about the behavioral changes we are discussing here, ie. the
> special handling of cascading interrupt inputs.

Yeah, it's quite interesting that the behavior isn't mentioned in the
8259 datasheets.  Still in retrospect it's hard to see how it can
possibly work with edge-triggered cascaded inputs.

Paolo

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-03 13:11, Paolo Bonzini wrote:
> Il 03/09/2012 12:34, Jan Kiszka ha scritto:
>>>> Why is this a problem?  All of them use IRQ2 for a cascade, and initialize
>>>> icw3 to 0x4/0x2 (I checked OpenBIOS, rth's palcode for Alpha, and Linux).
>> IRQ2 is already hard-coded in QEMU (we had to patch this for our machine
>> emulation, but less in recent versions), that is not the point. I'm
>> concerned about the behavioral changes we are discussing here, ie. the
>> special handling of cascading interrupt inputs.
> 
> Yeah, it's quite interesting that the behavior isn't mentioned in the
> 8259 datasheets.  Still in retrospect it's hard to see how it can
> possibly work with edge-triggered cascaded inputs.

As I said: by avoiding the pattern Matthew generated in his test case.
That's not impossible. All the other OSes running fine against out PIC
models are proving this.

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Maciej W. Rozycki]

On Mon, 3 Sep 2012, Jan Kiszka wrote:

> >   - Qemu output (without this patch):
> >         elcr=0c00 cmdRead ummask mask sti irq15 unmask DONE
> > 
> > But on real hardware, the master seems to treat IRQ2 as level triggered,

 That is not universally true, however in reality it does not matter, more 
on that below.

> > and doesn't deliver an interrupt to the CPU until the slave unmasks IRQ14.
> > I've tried this on several machines, including a non-PCI 486 that
> > does not seem to have ELCR registers.
> > 
> >   - Typical output (pentium/pentium pro/dual AMD Athlon/etc; slight
> >     variations in some elcr bits depending on machine, but bit 0x04
> >     is always clear):
> >         elcr=0c20 cmdRead unmask mask sti unmask irq14 DONE
> > 
> >   - 486 without elcr (just an ISA bus):
> >         elcr=e0e0 cmdRead unmask mask sti unmask irq14 DONE
> > 
> >   - One failure: It doesn't boot properly (no output) with a USB floppy
> >     drive on my Intel Core I7.  Guess: The test program just barely
> >     fits in a sector, with no room for any tables (partition/etc)
> >     that BIOS might check for if it isn't an original, native floppy
> >     drive.
> > 
> > -----------------------
> > 
> > I've found a few descriptions of programming the i8259.
> 
> I was mostly following the official "8259A PROGRAMMABLE INTERRUPT
> CONTROLLER (8259A 8259A-2)" by Intel. But it also doesn't mention any
> trigger mode changes for the cascading input lines.

 The datasheet is vague on the subject and there is apparently a common 
misconception on how the 8259A trigger modes work.  PIC core 
modifications, especially the ELCR (originally added for EISA support), 
seen in chipsets obfuscate the matter further.

 So first of all, the *output* of the 8259A is always edge triggered, 
regardless of whether it's the master or one of the slaves (only one slave 
is used in the PC/AT architecture, but up to eight are supported; the 
PC/XT had none).  If once an interrupt has been served any interrupts 
remain in the pending state, then the 8259A deasserts its output briefly 
before reasserting it.

 Secondly, for inputs the original 8259A only allowed a global (i.e. for 
all at once) edge/level selection via ICW1.  That somehow had to be 
compatible with the master/slave mode.

 However as of the 8259A "edge-triggered" in Intel's nomenclature meant: 
"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 (there are a few, including AEOI) or goes away unhandled."  
This is important for the output because the x86 architecture's definition 
for the INT input is level-triggered.

 The edge-triggered mode was also the only one supported for inputs with 
the original 8080-compatible 8259; note that for inputs a high-to-low 
transition cancels the interrupt as in the level-triggered mode.  The 
"level-triggered" mode was then added with the usual meaning -- 
essentially copying the interrupt input onto the output, ORing with the 
other inputs.

 So thus defined "edge-triggered" mode is in fact compatible with both 
edge-triggered and level-triggered inputs -- the former work by means of 
observing the low-to-high transition and the latter also work by only 
seeing the interrupt go away briefly -- but that does not really matter 
for level-triggered interrupts as the 8259A still knows an IRQ is pending 
and if a race causes an INTA cycle to arrive at the time the PIC's output 
is low it will still correctly serve the vector (I think in practice it 
does not really happen as the vector is served with the second INTA cycle 
only and by then the interrupt output must have recovered).

 There were some interesting errata in some early embedded 8259A cores in 
EISA chipses where the brief deassertion of the output did not happen.  
That worked just fine when wired directly to an x86 processor (because as 
noted above the INT input is level-triggered), however when connected to 
an i82489DX discrete APIC, that interprets the edge-triggered mode in the 
traditional way and implies that mode for inputs cascaded to 8259A, the 
problem had to be worked around in hardware, by adding extra logic to 
regenerate the missing high-to-low-to-high transition.

 Some of the information presented here was only noted by Intel in APIC 
documentation; I may track down and quote references, but these documents 
have never been available online anyway.

 Did that help?

  Maciej

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Paolo Bonzini]

Il 04/09/2012 16:29, Maciej W. Rozycki ha scritto:
>  So first of all, the *output* of the 8259A is always edge triggered, 
> regardless of whether it's the master or one of the slaves (only one slave 
> is used in the PC/AT architecture, but up to eight are supported; the 
> PC/XT had none).  

I swear I read all your message :) but this seems to be the crux.  It means
that something like this ought to fix the bug too.  Matthew, can you post
your code or test it?

diff --git a/hw/i8259.c b/hw/i8259.c
index 53daf78..3dc1dff 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -104,12 +104,11 @@ static void pic_update_irq(PICCommonState *s)
     int irq;
 
     irq = pic_get_irq(s);
+    qemu_irq_lower(s->int_out[0]);
     if (irq >= 0) {
         DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
                 s->master ? 0 : 1, s->imr, s->irr, s->priority_add);
         qemu_irq_raise(s->int_out[0]);
-    } else {
-        qemu_irq_lower(s->int_out[0]);
     }
 }
 
The logic of the in-kernel 8259 is a bit different, but something
like this should do it, too:

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 81cf4fa..feb6d5b 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -174,9 +174,11 @@ static void pic_update_irq(struct kvm_pic *s)
 		/*
 		 * if irq request by slave pic, signal master PIC
 		 */
+		pic_set_irq1(&s->pics[0], 2, 0);
 		pic_set_irq1(&s->pics[0], 2, 1);
+	} else if (s->pics[0].irr & (1 << 2))
 		pic_set_irq1(&s->pics[0], 2, 0);
-	}
+
 	irq = pic_get_irq(&s->pics[0]);
 	pic_irq_request(s->kvm, irq >= 0);
 }

Both patches untested.

Paolo

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-04 16:42, Paolo Bonzini wrote:
> Il 04/09/2012 16:29, Maciej W. Rozycki ha scritto:
>>  So first of all, the *output* of the 8259A is always edge triggered, 
>> regardless of whether it's the master or one of the slaves (only one slave 
>> is used in the PC/AT architecture, but up to eight are supported; the 
>> PC/XT had none).  
> 
> I swear I read all your message :) but this seems to be the crux.  It means
> that something like this ought to fix the bug too.  Matthew, can you post
> your code or test it?
> 
> diff --git a/hw/i8259.c b/hw/i8259.c
> index 53daf78..3dc1dff 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -104,12 +104,11 @@ static void pic_update_irq(PICCommonState *s)
>      int irq;
>  
>      irq = pic_get_irq(s);
> +    qemu_irq_lower(s->int_out[0]);
>      if (irq >= 0) {
>          DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
>                  s->master ? 0 : 1, s->imr, s->irr, s->priority_add);
>          qemu_irq_raise(s->int_out[0]);
> -    } else {
> -        qemu_irq_lower(s->int_out[0]);
>      }
>  }

I don't think this can be correct in all scenario. E.g., we also call
pic_update_irq in case a level-triggered input was updated but didn't
change the output state of the PIC (high-high transition). With your
patch, the output will now generate edges.

What I'm trying to understand and translate from the description is
rather "note that for inputs a high-to-low transition cancels the
interrupt as in the level-triggered mode." This is surely not what we do
right now. OTOH, I'm afraid that switching to this mode in the PIC can
cause problems elsewhere, with devices that actually inject short
low-high-low signals. Still wrapping my head around it...

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Maciej W. Rozycki]

On Tue, 4 Sep 2012, Jan Kiszka wrote:

> What I'm trying to understand and translate from the description is
> rather "note that for inputs a high-to-low transition cancels the
> interrupt as in the level-triggered mode." This is surely not what we do
> right now. OTOH, I'm afraid that switching to this mode in the PIC can
> cause problems elsewhere, with devices that actually inject short
> low-high-low signals. Still wrapping my head around it...

 That won't work reliably with true 8259A hardware -- for an 
edge-triggered interrupt to propagate up to the CPU first there must be a 
low-to-high transition and then the high logic state must be maintained up 
until the start of the second INTA cycle.  If the interrupt request drops 
before then (e.g. because CPU interrupts have been masked or a 
higher-priority 8259A has been serviced), then the corresponding IRR bit 
is cleared and either the interrupt is missed altogether or, if the CPU 
has already accepted the interrupt and started the first INTA cycle, then 
the spurious vector is supplied and no ISR bit is set.

 To put it in different words: the only actual difference between 
edge-triggered and level-triggered interrupts in the 8259A is that the 
formers require a leading edge to record another interrupt.  For both 
trigger modes the high level has to be maintained until the second INTA 
cycle for the interrupt to be correctly delivered to the CPU and also in 
both trigger modes a trailing edge cancels the interrupt.

 This is unlike the traditional edge-triggered mode where the level does 
not have to be maintained once a leading edge has been correctly recorded 
(there is usually spike filtering logic implemented on such IRQ inputs so 
appropriate timings have to be met; because of its unusual interpretation 
the 8259A obviously does not require such logic).

 The edge detector logic is also drawn in the 8259A datasheet (that for a 
change used to be available from one of the Intel sites in the PDF form) 
and I believe the functionality described can be inferred from that by the 
curious enough. ;)

  Maciej

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-04 19:41, Maciej W. Rozycki wrote:
> On Tue, 4 Sep 2012, Jan Kiszka wrote:
> 
>> What I'm trying to understand and translate from the description is
>> rather "note that for inputs a high-to-low transition cancels the
>> interrupt as in the level-triggered mode." This is surely not what we do
>> right now. OTOH, I'm afraid that switching to this mode in the PIC can
>> cause problems elsewhere, with devices that actually inject short
>> low-high-low signals. Still wrapping my head around it...
> 
>  That won't work reliably with true 8259A hardware -- for an 

Ok, then we have to scan our code base for such device models that won't
survive with real 8259A hardware. That can only be devices attached to
edge-only inputs of the PIC, namely the PIT, the keyboard controller,
the RTC and FPU emulation. They basically need to generate high-low-high
transitions on new events, instead of low-high-low (via qemu_irq_pulse
e.g.). I'm I on the right track?

Thanks,
Jan

> edge-triggered interrupt to propagate up to the CPU first there must be a 
> low-to-high transition and then the high logic state must be maintained up 
> until the start of the second INTA cycle.  If the interrupt request drops 
> before then (e.g. because CPU interrupts have been masked or a 
> higher-priority 8259A has been serviced), then the corresponding IRR bit 
> is cleared and either the interrupt is missed altogether or, if the CPU 
> has already accepted the interrupt and started the first INTA cycle, then 
> the spurious vector is supplied and no ISR bit is set.
> 
>  To put it in different words: the only actual difference between 
> edge-triggered and level-triggered interrupts in the 8259A is that the 
> formers require a leading edge to record another interrupt.  For both 
> trigger modes the high level has to be maintained until the second INTA 
> cycle for the interrupt to be correctly delivered to the CPU and also in 
> both trigger modes a trailing edge cancels the interrupt.
> 
>  This is unlike the traditional edge-triggered mode where the level does 
> not have to be maintained once a leading edge has been correctly recorded 
> (there is usually spike filtering logic implemented on such IRQ inputs so 
> appropriate timings have to be met; because of its unusual interpretation 
> the 8259A obviously does not require such logic).
> 
>  The edge detector logic is also drawn in the 8259A datasheet (that for a 
> change used to be available from one of the Intel sites in the PDF form) 
> and I believe the functionality described can be inferred from that by the 
> curious enough. ;)
> 
>   Maciej
> 

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Maciej W. Rozycki]

On Tue, 4 Sep 2012, Jan Kiszka wrote:

> >> What I'm trying to understand and translate from the description is
> >> rather "note that for inputs a high-to-low transition cancels the
> >> interrupt as in the level-triggered mode." This is surely not what we do
> >> right now. OTOH, I'm afraid that switching to this mode in the PIC can
> >> cause problems elsewhere, with devices that actually inject short
> >> low-high-low signals. Still wrapping my head around it...
> > 
> >  That won't work reliably with true 8259A hardware -- for an 
> 
> Ok, then we have to scan our code base for such device models that won't
> survive with real 8259A hardware. That can only be devices attached to
> edge-only inputs of the PIC, namely the PIT, the keyboard controller,
> the RTC and FPU emulation. They basically need to generate high-low-high
> transitions on new events, instead of low-high-low (via qemu_irq_pulse
> e.g.). I'm I on the right track?

 They shouldn't be the problem unless we've got an implementation problem:

 * The 8254 PIT is normally configured in mode 2 or 3 in the PC/AT 
   architecture.  In the former its output is high (active) all the time 
   except from one (last) clock cycle.  In the latter a wave that has a 
   duty cycle close or equal to 0.5 (depending on whether the divider is 
   odd or even) is produced, so no short pulses either.  I don't remember 
   the other four modes -- have a look at the datasheet if interested, but 
   I reckon they're not really compatible with the wiring anyway, e.g. the 
   gate is hardwired enabled.

 * The 8042 keyboard controller requires an interrupt acknowledge (by 
   reading its data port, normally at the address 0x60 in the PC/AT port 
   I/O space) to clear its output, so no concern here either (this is BTW 
   how many years ago I actually learnt the hard way how the 
   edge-triggered mode works in the 8259A -- reading the KBC's data port 
   in the main program a tight loop with the keyboard interrupt enabled 
   will lead to a spurious interrupt each time data is supplied).  I 
   believe it's otherwise edge-triggered though (I don't have an 8042 
   datasheet handy, I would have to dig for a hardcopy that I left 
   abroad).

 * The MC146818 RTC interrupt is level triggered and has to be 
   acknowledged by reading Register C.

 * The i287/i387 FPU interrupt (in the PC/AT compatibility mode) is 
   latched and requires the location at the address 0xf0 in the PC/AT port 
   I/O space to be poked at to acknowledge.  It can therefore be 
   considered level-triggered (overall the design is busted and you have 
   to be very careful not to freeze the system when handling the FPU error 
   this way rather than via the FPU exception).

 Did I miss anything?

  Maciej

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-04 20:27, Maciej W. Rozycki wrote:
> On Tue, 4 Sep 2012, Jan Kiszka wrote:
> 
>>>> What I'm trying to understand and translate from the description is
>>>> rather "note that for inputs a high-to-low transition cancels the
>>>> interrupt as in the level-triggered mode." This is surely not what we do
>>>> right now. OTOH, I'm afraid that switching to this mode in the PIC can
>>>> cause problems elsewhere, with devices that actually inject short
>>>> low-high-low signals. Still wrapping my head around it...
>>>
>>>  That won't work reliably with true 8259A hardware -- for an 
>>
>> Ok, then we have to scan our code base for such device models that won't
>> survive with real 8259A hardware. That can only be devices attached to
>> edge-only inputs of the PIC, namely the PIT, the keyboard controller,
>> the RTC and FPU emulation. They basically need to generate high-low-high
>> transitions on new events, instead of low-high-low (via qemu_irq_pulse
>> e.g.). I'm I on the right track?
> 
>  They shouldn't be the problem unless we've got an implementation problem:
> 
>  * The 8254 PIT is normally configured in mode 2 or 3 in the PC/AT 
>    architecture.  In the former its output is high (active) all the time 
>    except from one (last) clock cycle.  In the latter a wave that has a 
>    duty cycle close or equal to 0.5 (depending on whether the divider is 
>    odd or even) is produced, so no short pulses either.  I don't remember 
>    the other four modes -- have a look at the datasheet if interested, but 
>    I reckon they're not really compatible with the wiring anyway, e.g. the 
>    gate is hardwired enabled.
> 
>  * The 8042 keyboard controller requires an interrupt acknowledge (by 
>    reading its data port, normally at the address 0x60 in the PC/AT port 
>    I/O space) to clear its output, so no concern here either (this is BTW 
>    how many years ago I actually learnt the hard way how the 
>    edge-triggered mode works in the 8259A -- reading the KBC's data port 
>    in the main program a tight loop with the keyboard interrupt enabled 
>    will lead to a spurious interrupt each time data is supplied).  I 
>    believe it's otherwise edge-triggered though (I don't have an 8042 
>    datasheet handy, I would have to dig for a hardcopy that I left 
>    abroad).
> 
>  * The MC146818 RTC interrupt is level triggered and has to be 
>    acknowledged by reading Register C.
> 
>  * The i287/i387 FPU interrupt (in the PC/AT compatibility mode) is 
>    latched and requires the location at the address 0xf0 in the PC/AT port 
>    I/O space to be poked at to acknowledge.  It can therefore be 
>    considered level-triggered (overall the design is busted and you have 
>    to be very careful not to freeze the system when handling the FPU error 
>    this way rather than via the FPU exception).
> 
>  Did I miss anything?

Nope, and it looks like I was too pessimistic. I've checked PIT, 8042,
and RTC, and they all set their output levels properly. So it should be
safe to start clearing an IRQ on falling edges at a PIC input.

Jan

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

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Matthew Ogilvie]

On Tue, Sep 04, 2012 at 04:42:35PM +0200, Paolo Bonzini wrote:
> Il 04/09/2012 16:29, Maciej W. Rozycki ha scritto:
> >  So first of all, the *output* of the 8259A is always edge triggered, 
> > regardless of whether it's the master or one of the slaves (only one slave 
> > is used in the PC/AT architecture, but up to eight are supported; the 
> > PC/XT had none).  
> 
> I swear I read all your message :) but this seems to be the crux.  It means
> that something like this ought to fix the bug too.  Matthew, can you post
> your code or test it?

The test program source can be downloaded from
http://home.comcast.net/~mmogilvi/downloads/i8259-imr-test-2012-09-02.tar.bz2

I intend to rewrite it for kvm-unit-tests as you suggested earlier, but
likely not before this weekend.

> 
> diff --git a/hw/i8259.c b/hw/i8259.c
> index 53daf78..3dc1dff 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -104,12 +104,11 @@ static void pic_update_irq(PICCommonState *s)
>      int irq;
>  
>      irq = pic_get_irq(s);
> +    qemu_irq_lower(s->int_out[0]);
>      if (irq >= 0) {
>          DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
>                  s->master ? 0 : 1, s->imr, s->irr, s->priority_add);
>          qemu_irq_raise(s->int_out[0]);
> -    } else {
> -        qemu_irq_lower(s->int_out[0]);
>      }
>  }

This doesn't work; the master still locks onto the original low to high
edge, and doesn't cancel it on the high to low edge.  I haven't had a
chance to look into or test your (or any other) KVM patches yet,
although I'll probably get to it this weekend.

According to later discussion, the main issue is actually the input
IRQ behavior on a high to low transition; hence the following fixes
both the test program and the Microport UNIX problem:

diff --git a/hw/i8259.c b/hw/i8259.c
index 6587666..c011787 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -143,22 +143,23 @@ static void pic_set_irq(void *opaque, int irq, int level)
     if (s->elcr & mask) {
         /* level triggered */
         if (level) {
             s->irr |= mask;
             s->last_irr |= mask;
         } else {
             s->irr &= ~mask;
             s->last_irr &= ~mask;
         }
     } else {
         /* edge triggered */
         if (level) {
             if ((s->last_irr & mask) == 0) {
                 s->irr |= mask;
             }
             s->last_irr |= mask;
         } else {
+            s->irr &= ~mask;
             s->last_irr &= ~mask;
         }
     }
     pic_update_irq(s);
 }
 

Perhaps it would be worth it to swap around the "if"s a little bit
to avoid the (!level) duplication, and clarify that the only difference
is in the low to high transition?

             - Matthew Ogilvie

Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Jan Kiszka]

On 2012-09-05 06:33, Matthew Ogilvie wrote:
> According to later discussion, the main issue is actually the input
> IRQ behavior on a high to low transition; hence the following fixes
> both the test program and the Microport UNIX problem:
> 
> diff --git a/hw/i8259.c b/hw/i8259.c
> index 6587666..c011787 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -143,22 +143,23 @@ static void pic_set_irq(void *opaque, int irq, int level)
>      if (s->elcr & mask) {
>          /* level triggered */
>          if (level) {
>              s->irr |= mask;
>              s->last_irr |= mask;
>          } else {
>              s->irr &= ~mask;
>              s->last_irr &= ~mask;
>          }
>      } else {
>          /* edge triggered */
>          if (level) {
>              if ((s->last_irr & mask) == 0) {
>                  s->irr |= mask;
>              }
>              s->last_irr |= mask;
>          } else {
> +            s->irr &= ~mask;
>              s->last_irr &= ~mask;
>          }
>      }
>      pic_update_irq(s);
>  }
>  
> 
> Perhaps it would be worth it to swap around the "if"s a little bit
> to avoid the (!level) duplication, and clarify that the only difference
> is in the low to high transition?

Yes, refactoring would be welcome. But I would suggest to do this in two
patches, leaving the above change (+ changelog that explains the reason)
in its current clarity.

Hurray, no vmstate subsections needed! :)

Jan

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