[Qemu-devel] [PATCH v5 0/6] Running Microport UNIX (ca 1987)

[Qemu-devel] [PATCH v5 0/6] Running Microport UNIX (ca 1987)

[Matthew Ogilvie]

Changes since previous version:

   * The first 4 patches haven't changed since version 3.
   * New patch 5: The 8259 patch has been totally redesigned again, this
     time based on a new understanding that on real hardware, if
     the trailing edge of an interrupt arrives before the interrupt
     is serviced, then it cancels the interrupt, just like a level
     triggered interrupt.  See earlier email discussion.
   * New patch 6 just refactors the code (no functionality change)
     after the one line fix in patch 5.

I'm also sending a couple of patches for related projects, separately:

   * Two KVM (Linux kernel) patches that do roughly the same thing as
     patches 5 and 6, only for the in-kernel PIC.
   * A patch for the kvm-unit-tests project that adds a test case
     to demonstrate the trailing edge behavior.

Matthew Ogilvie (6):
  fix some debug printf format strings
  vl: fix -hdachs/-hda argument order parsing issues
  qemu-options.hx: mention retrace= VGA option
  vga: add some optional CGA compatibility hacks
  i8259: fix so that dropping IRQ level always clears the interrupt
    request
  i8259: refactor pic_set_irq level logic

 hw/cirrus_vga.c |  4 ++--
 hw/i8259.c      | 28 ++++++++++----------------
 hw/ide/cmd646.c |  5 +++--
 hw/ide/via.c    |  5 +++--
 hw/pc.h         |  4 ++++
 hw/vga.c        | 37 ++++++++++++++++++++++++++--------
 qemu-options.hx | 27 ++++++++++++++++++++++++-
 vl.c            | 62 ++++++++++++++++++++++++++++++++++++++-------------------
 8 files changed, 119 insertions(+), 53 deletions(-)

-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH v5 1/6] fix some debug printf format strings

[Matthew Ogilvie]

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

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

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 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 v5 2/6] 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 v5 3/6] 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 v5 4/6] vga: add some optional CGA compatibility hacks

[Matthew Ogilvie]

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

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

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

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

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

diff --git a/hw/pc.h b/hw/pc.h
index e4db071..37e2f87 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -176,6 +176,10 @@ enum vga_retrace_method {
 
 extern enum vga_retrace_method vga_retrace_method;
 
+#define VGA_CGA_HACK_PALETTE_BLANKING  (1<<0)
+#define VGA_CGA_HACK_FONT_HEIGHT       (1<<1)
+extern int vga_cga_hacks;
+
 static inline DeviceState *isa_vga_init(ISABus *bus)
 {
     ISADevice *dev;
diff --git a/hw/vga.c b/hw/vga.c
index 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 v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Matthew Ogilvie]

Intel's definition of "edge triggered" means: "asserted with a
low-to-high transition at the time an interrupt is registered and
then kept high until the interrupt is served via one of the
EOI mechanisms or goes away unhandled."

So the only difference between edge triggered and level triggered
is in the leading edge, with no difference in the trailing edge.

This bug manifested itself when the guest was Microport UNIX
System V/386 v2.1 (ca. 1987), because it would sometimes mask
off IRQ14 in the slave IMR after it had already been asserted.
The master would still try to deliver an interrupt even though
IRQ2 had dropped again, resulting in a spurious interupt
(IRQ15) and a panicked kernel.

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

If you missed the previous thread about this, see
http://www.mail-archive.com/qemu-devel@nongnu.org/msg129071.html

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

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

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Avi Kivity]

On 09/10/2012 04:27 AM, Matthew Ogilvie wrote:
> Intel's definition of "edge triggered" means: "asserted with a
> low-to-high transition at the time an interrupt is registered and
> then kept high until the interrupt is served via one of the
> EOI mechanisms or goes away unhandled."
> 
> So the only difference between edge triggered and level triggered
> is in the leading edge, with no difference in the trailing edge.

Hard to believe.  So an edge while cpu interrupts are disabled is ignored?

> 
> This bug manifested itself when the guest was Microport UNIX
> System V/386 v2.1 (ca. 1987), because it would sometimes mask
> off IRQ14 in the slave IMR after it had already been asserted.
> The master would still try to deliver an interrupt even though
> IRQ2 had dropped again, resulting in a spurious interupt
> (IRQ15) and a panicked kernel.

This is something else.  It means that setting an edge must happen after
IMR is cleared to be picked up.  But this is not what the patch is doing.

> 
> Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
> ---
> 
> If you missed the previous thread about this, see
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg129071.html
> 
>  hw/i8259.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i8259.c b/hw/i8259.c
> index 6587666..c011787 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -157,6 +157,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
>              }
>              s->last_irr |= mask;
>          } else {
> +            s->irr &= ~mask;
>              s->last_irr &= ~mask;
>          }
>      }
> 


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

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Jan Kiszka]

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

On 2012-09-10 10:56, Avi Kivity wrote:
> On 09/10/2012 04:27 AM, Matthew Ogilvie wrote:
>> Intel's definition of "edge triggered" means: "asserted with a
>> low-to-high transition at the time an interrupt is registered and
>> then kept high until the interrupt is served via one of the
>> EOI mechanisms or goes away unhandled."
>>
>> So the only difference between edge triggered and level triggered
>> is in the leading edge, with no difference in the trailing edge.
> 
> Hard to believe.  So an edge while cpu interrupts are disabled is ignored?

No, this is about the PIC, not the CPU interrupt inputs.

Matthew, did you verify this on real hardware by reading back the IRR as
I suggested?

Jan



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

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Avi Kivity]

On 09/10/2012 12:09 PM, Jan Kiszka wrote:
> On 2012-09-10 10:56, Avi Kivity wrote:
>> On 09/10/2012 04:27 AM, Matthew Ogilvie wrote:
>>> Intel's definition of "edge triggered" means: "asserted with a
>>> low-to-high transition at the time an interrupt is registered and
>>> then kept high until the interrupt is served via one of the
>>> EOI mechanisms or goes away unhandled."
>>>
>>> So the only difference between edge triggered and level triggered
>>> is in the leading edge, with no difference in the trailing edge.
>> 
>> Hard to believe.  So an edge while cpu interrupts are disabled is ignored?
> 
> No, this is about the PIC, not the CPU interrupt inputs.

I see, the interrupt is still sent to the processor; but IRR reflects
that status of the input line, not a "pending interrupt" status.

Will this survive live migration?  If we clear IRR, then we must rely on
the other end to remember the IRQ, but if processor interrupts are
disabled there won't be an INTACK and the signal is lost.

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

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Jan Kiszka]

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

On 2012-09-10 11:18, Avi Kivity wrote:
> On 09/10/2012 12:09 PM, Jan Kiszka wrote:
>> On 2012-09-10 10:56, Avi Kivity wrote:
>>> On 09/10/2012 04:27 AM, Matthew Ogilvie wrote:
>>>> Intel's definition of "edge triggered" means: "asserted with a
>>>> low-to-high transition at the time an interrupt is registered and
>>>> then kept high until the interrupt is served via one of the
>>>> EOI mechanisms or goes away unhandled."
>>>>
>>>> So the only difference between edge triggered and level triggered
>>>> is in the leading edge, with no difference in the trailing edge.
>>>
>>> Hard to believe.  So an edge while cpu interrupts are disabled is ignored?
>>
>> No, this is about the PIC, not the CPU interrupt inputs.
> 
> I see, the interrupt is still sent to the processor; but IRR reflects
> that status of the input line, not a "pending interrupt" status.
> 
> Will this survive live migration?  If we clear IRR, then we must rely on
> the other end to remember the IRQ, but if processor interrupts are
> disabled there won't be an INTACK and the signal is lost.

We clear the IRR as there is nothing to deliver to the CPU anymore. No
IRQ source will drop its line as long as there is a reason for the IRQ,
I checked the edge-using devices. So we can't lose anything.

Jan



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

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Maciej W. Rozycki]

On Mon, 10 Sep 2012, Avi Kivity wrote:

> >>> So the only difference between edge triggered and level triggered
> >>> is in the leading edge, with no difference in the trailing edge.
> >> 
> >> Hard to believe.  So an edge while cpu interrupts are disabled is ignored?

 Please note that x86 CPU's INT input is level-triggered, not 
edge-triggered -- I mean the "naked" input to the core, not anything that 
may be presented to the outside world by any local APIC present (IOW the 
line at the *output* of the local APIC where one is present).

 There's more confusion about the ExtINTA mode (even the name itself is 
spelled differently as ExtINTA vs ExtINT across various documents) as its 
trigger mode is implied and not configurable with the vector/redirection 
table entry's trigger-mode bit and various APIC implementations treat it 
differently and hardwire as either edge-triggered or level-triggered as 
the designers saw fit.

> > No, this is about the PIC, not the CPU interrupt inputs.
> 
> I see, the interrupt is still sent to the processor; but IRR reflects
> that status of the input line, not a "pending interrupt" status.

 Not really, this is still a "pending interrupt" status.

 For level-triggered inputs the state of IRR bits do indeed follow the 
respective IRx inputs (taking the IMR into account).  For edge-triggered 
inputs the relevant IRR bit is set by a leading edge on its corresponding 
IRx input and cleared when the interrupt is acknowledged (either with an 
INTA bus cycle or by a data read bus cycle issued to the PIC armed with an 
OCW3 that has had the POLL command bit set) OR with a trailing edge on IRx 
(again, all this takes the IMR into account).  At this point another 
leading edge is required for the IRR bit to be set again, that is merely 
keeping the IRx input's level active won't trigger another interrupt.

  Maciej

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Avi Kivity]

On 09/10/2012 04:09 PM, Maciej W. Rozycki wrote:
> 
>> > No, this is about the PIC, not the CPU interrupt inputs.
>> 
>> I see, the interrupt is still sent to the processor; but IRR reflects
>> that status of the input line, not a "pending interrupt" status.
> 
>  Not really, this is still a "pending interrupt" status.
> 
>  For level-triggered inputs the state of IRR bits do indeed follow the 
> respective IRx inputs (taking the IMR into account).  For edge-triggered 
> inputs the relevant IRR bit is set by a leading edge on its corresponding 
> IRx input and cleared when the interrupt is acknowledged (either with an 
> INTA bus cycle or by a data read bus cycle issued to the PIC armed with an 
> OCW3 that has had the POLL command bit set) OR with a trailing edge on IRx 
> (again, all this takes the IMR into account).  At this point another 
> leading edge is required for the IRR bit to be set again, that is merely 
> keeping the IRx input's level active won't trigger another interrupt.


Ok, thanks, that explains it for me.


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

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Matthew Ogilvie]

On Mon, Sep 10, 2012 at 11:09:27AM +0200, Jan Kiszka wrote:
> On 2012-09-10 10:56, Avi Kivity wrote:
> > On 09/10/2012 04:27 AM, Matthew Ogilvie wrote:
> >> Intel's definition of "edge triggered" means: "asserted with a
> >> low-to-high transition at the time an interrupt is registered and
> >> then kept high until the interrupt is served via one of the
> >> EOI mechanisms or goes away unhandled."
> >>
> >> So the only difference between edge triggered and level triggered
> >> is in the leading edge, with no difference in the trailing edge.
> > 
> > Hard to believe.  So an edge while cpu interrupts are disabled is ignored?
> 
> No, this is about the PIC, not the CPU interrupt inputs.
> 
> Matthew, did you verify this on real hardware by reading back the IRR as
> I suggested?
> 
> Jan

I hadn't before, but now that I've checked, it's as expected:

-----------
Real hardware [Pentium 4]:

  cmdRead unmask IRR=4005 mask IRR=4001 sti unmask irq14 IRR=0001 DONE

[I also see a final IRR=0000 occasionally.  Probably just happened to
ask it while the timer (IRQ0) line is low (without the new understanding
of the trailing edge of an edge triggered interrupt, this would
have been confusing).  I have most IRQ's (including
timer) masked off.]

-----------
Unpatched qemu:

  cmdRead unmask IRR=4015 mask IRR=4015 sti irq15 unmask IRR=4015 DONE

[Presumably IRQ4 (0x10 - qemu's serial device model?) had a transient
edge during initialization, but had been masked off even before I
masked it off?]

-----------
Patched qemu:

  cmdRead unmask IRR=4005 mask IRR=4001 sti unmask irq14 IRR=0001 DONE


          - Matthew

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Jan Kiszka]

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

On 2012-09-11 06:32, Matthew Ogilvie wrote:
> On Mon, Sep 10, 2012 at 11:09:27AM +0200, Jan Kiszka wrote:
>> On 2012-09-10 10:56, Avi Kivity wrote:
>>> On 09/10/2012 04:27 AM, Matthew Ogilvie wrote:
>>>> Intel's definition of "edge triggered" means: "asserted with a
>>>> low-to-high transition at the time an interrupt is registered and
>>>> then kept high until the interrupt is served via one of the
>>>> EOI mechanisms or goes away unhandled."
>>>>
>>>> So the only difference between edge triggered and level triggered
>>>> is in the leading edge, with no difference in the trailing edge.
>>>
>>> Hard to believe.  So an edge while cpu interrupts are disabled is ignored?
>>
>> No, this is about the PIC, not the CPU interrupt inputs.
>>
>> Matthew, did you verify this on real hardware by reading back the IRR as
>> I suggested?
>>
>> Jan
> 
> I hadn't before, but now that I've checked, it's as expected:
> 
> -----------
> Real hardware [Pentium 4]:
> 
>   cmdRead unmask IRR=4005 mask IRR=4001 sti unmask irq14 IRR=0001 DONE
> 
> [I also see a final IRR=0000 occasionally.  Probably just happened to
> ask it while the timer (IRQ0) line is low (without the new understanding
> of the trailing edge of an edge triggered interrupt, this would
> have been confusing).  I have most IRQ's (including
> timer) masked off.]
> 
> -----------
> Unpatched qemu:
> 
>   cmdRead unmask IRR=4015 mask IRR=4015 sti irq15 unmask IRR=4015 DONE
> 
> [Presumably IRQ4 (0x10 - qemu's serial device model?) had a transient
> edge during initialization, but had been masked off even before I
> masked it off?]
> 
> -----------
> Patched qemu:
> 
>   cmdRead unmask IRR=4005 mask IRR=4001 sti unmask irq14 IRR=0001 DONE
> 

I think this is convincing, maybe worth documenting in the related
changelogs of QEMU and KVM. Avi, doubts remaining?

Jan



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

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Avi Kivity]

On 09/11/2012 12:05 PM, Jan Kiszka wrote:

> I think this is convincing, maybe worth documenting in the related
> changelogs of QEMU and KVM. Avi, doubts remaining?
> 

Nope, I think I've understood it finally.


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

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[BALATON Zoltan]

On Sun, 9 Sep 2012, Matthew Ogilvie wrote:
> Intel's definition of "edge triggered" means: "asserted with a
> low-to-high transition at the time an interrupt is registered and
> then kept high until the interrupt is served via one of the
> EOI mechanisms or goes away unhandled."
>
> So the only difference between edge triggered and level triggered
> is in the leading edge, with no difference in the trailing edge.
>
> This bug manifested itself when the guest was Microport UNIX
> System V/386 v2.1 (ca. 1987), because it would sometimes mask
> off IRQ14 in the slave IMR after it had already been asserted.
> The master would still try to deliver an interrupt even though
> IRQ2 had dropped again, resulting in a spurious interupt
> (IRQ15) and a panicked kernel.
>
> Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
> ---
>
> If you missed the previous thread about this, see
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg129071.html
>
> hw/i8259.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/i8259.c b/hw/i8259.c
> index 6587666..c011787 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -157,6 +157,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
>             }
>             s->last_irr |= mask;
>         } else {
> +            s->irr &= ~mask;
>             s->last_irr &= ~mask;
>         }
>     }
>

What happened to this patch? Any chance it will be in 1.3?

Thanks,
BALATON Zoltan

Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request

[Matthew Ogilvie]

On Mon, Nov 19, 2012 at 04:28:31PM +0100, BALATON Zoltan wrote:
> On Sun, 9 Sep 2012, Matthew Ogilvie wrote:
> > Intel's definition of "edge triggered" means: "asserted with a
> > low-to-high transition at the time an interrupt is registered and
> > then kept high until the interrupt is served via one of the
> > EOI mechanisms or goes away unhandled."
> >
> > So the only difference between edge triggered and level triggered
> > is in the leading edge, with no difference in the trailing edge.
> 
> What happened to this patch? Any chance it will be in 1.3?
> 
> Thanks,
> BALATON Zoltan

I kind of doubt it will make it into 1.3, although I would like to
get it in eventually.  Maybe the first few essentially unrelated
trivial patches can make it in?

Yours is the first comment I've seen since I've posted version 6 of
the series (this particular patch is the same as version 5).  The lack
of feedback combined with other demands on my time have prevented me
from progressing on this for over a month and a half.

Status:

   * We can't completely fix the i8259 model without addressing some
     issues in the i8254 model as well.  The i8254 issues (very short
     duty cycle on IRQ0 causing lost timer ticks) become rather
     severe if you try to fix the i8259 without fixing the i8254.
   * But the obvious direct fixes to the i8254 may risk causing issues with
     breaking migration between pre-fix and post-fix versions of qemu,
     due to lost and/or extra timer interrupts.  I'm not sure how big
     of an issue this is in practice, but it is a concern.
   * I've outlined some possible ways to address the migration issue
     in the cover letter of version 6 of the patch series.
   * Similar issue cascades exist in the in-kernel KVM model as well.
     The i8254 issues are even more severe in KVM: timer interrupts
     are always lost [0-length duty cycle in KVM model], instead of
     sometimes lost [non-0 but tiny duty cycle in qemu model].
   * I'm not aware of any similar problems in other device interrupt
     models, but I haven't really checked them, either.

Next step?:

In the absense of any other suggestions, I'm thinking about rolling
a version of the series that leaves IRQ0 (timer) working the way
it does in qemu 1.2.  Except in the i8254 I'll immediately preceed
each "intended to be leading edge transition" with an
explicit lowering of IRQ0, so that if migrating from some
future really-fixed version that leaves IRQ0 high most
of the time, it will still recognize the new edge.  A "real" fix
would then be delayed (years?) until versions without this first
stage fix are no longer in production.  I can probably
get this done this weekend.  [Although I'm not sure how to
deal with the issue that some of the i8254 modes' leading edge
transitions are off by one count.  Perhaps we'll need multiple
intermediate stages, or one of the other strategies outlined in
the version 6 cover letter.]

Or does anyone have a better suggestion?

                         - Matthew Ogilvie

[Qemu-devel] [PATCH v5 6/6] i8259: refactor pic_set_irq level logic

[Matthew Ogilvie]

No change in functionality.

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

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

diff --git a/hw/i8259.c b/hw/i8259.c
index c011787..1ba9b3a 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -140,26 +140,18 @@ static void pic_set_irq(void *opaque, int irq, int level)
     }
 #endif
 
-    if (s->elcr & mask) {
-        /* level triggered */
-        if (level) {
+    if (level) {
+        if ((s->last_irr & mask) == 0 ||  /* edge for edge triggered */
+            (s->elcr & mask)) {           /* or level triggered */
             s->irr |= mask;
-            s->last_irr |= mask;
-        } else {
-            s->irr &= ~mask;
-            s->last_irr &= ~mask;
         }
+        s->last_irr |= mask;
     } else {
-        /* edge triggered */
-        if (level) {
-            if ((s->last_irr & mask) == 0) {
-                s->irr |= mask;
-            }
-            s->last_irr |= mask;
-        } else {
-            s->irr &= ~mask;
-            s->last_irr &= ~mask;
-        }
+        /* Dropping level clears the interrupt regardless of edge trigger
+         * vs level trigger.
+         */
+        s->irr &= ~mask;
+        s->last_irr &= ~mask;
     }
     pic_update_irq(s);
 }
-- 
1.7.10.2.484.gcd07cc5