From patchwork Mon Sep 3 02:56:14 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matthew Ogilvie X-Patchwork-Id: 181283 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 70EC22C0090 for ; Mon, 3 Sep 2012 12:58:04 +1000 (EST) Received: from localhost ([::1]:33982 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8MrS-0001hV-M5 for incoming@patchwork.ozlabs.org; Sun, 02 Sep 2012 22:58:02 -0400 Received: from eggs.gnu.org ([208.118.235.92]:42445) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8MqY-0007kh-DQ for qemu-devel@nongnu.org; Sun, 02 Sep 2012 22:57:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T8MqW-0008I1-DZ for qemu-devel@nongnu.org; Sun, 02 Sep 2012 22:57:06 -0400 Received: from qmta15.emeryville.ca.mail.comcast.net ([76.96.27.228]:51981) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8MqW-0008Hr-4Q for qemu-devel@nongnu.org; Sun, 02 Sep 2012 22:57:04 -0400 Received: from omta23.emeryville.ca.mail.comcast.net ([76.96.30.90]) by qmta15.emeryville.ca.mail.comcast.net with comcast id uQnQ1j00P1wfjNsAFSx39x; Mon, 03 Sep 2012 02:57:03 +0000 Received: from mmogilvi.homeip.net ([24.9.53.136]) by omta23.emeryville.ca.mail.comcast.net with comcast id uSx11j00h2wKXRC8jSx2WL; Mon, 03 Sep 2012 02:57:02 +0000 Received: by mmogilvi.homeip.net (Postfix, from userid 501) id B97B11E9601C; Sun, 2 Sep 2012 20:57:01 -0600 (MDT) From: Matthew Ogilvie To: qemu-devel@nongnu.org Date: Sun, 2 Sep 2012 20:56:14 -0600 Message-Id: <1346640974-30974-6-git-send-email-mmogilvi_qemu@miniinfo.net> X-Mailer: git-send-email 1.7.10.2.484.gcd07cc5 In-Reply-To: <1346640974-30974-1-git-send-email-mmogilvi_qemu@miniinfo.net> References: <1346640974-30974-1-git-send-email-mmogilvi_qemu@miniinfo.net> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 76.96.27.228 Cc: Paolo Bonzini , Jan Kiszka , Matthew Ogilvie Subject: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- 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;