From a5f239fb8daefacd8bae28e0f8757075f5abc966 Mon Sep 17 00:00:00 2001 From: Jeff Parsons Date: Thu, 4 Dec 2014 11:33:05 -0800 Subject: [PATCH] More hard disk (AT) and interrupt-handling fixes Both real-mode and protected-mode hard disk I/O should be relatively happy now --- modules/pcjs/lib/chipset.js | 71 +++++++++++++++++----------------- modules/pcjs/lib/debugger.js | 42 +++++++++++--------- modules/pcjs/lib/disk.js | 4 +- modules/pcjs/lib/hdc.js | 74 ++++++++++++++++++++++++++++-------- modules/pcjs/lib/x86cpu.js | 3 ++ modules/pcjs/lib/x86help.js | 1 - modules/pcjs/lib/x86seg.js | 2 +- 7 files changed, 124 insertions(+), 73 deletions(-) diff --git a/modules/pcjs/lib/chipset.js b/modules/pcjs/lib/chipset.js index 97731b4c4..605c59208 100644 --- a/modules/pcjs/lib/chipset.js +++ b/modules/pcjs/lib/chipset.js @@ -2225,7 +2225,7 @@ ChipSet.prototype.dumpPIC = function() var b = pic.aICW[i]; sDump += " IC" + (i + 1) + "=" + str.toHexByte(b); } - sDump += " IMR=" + str.toHexByte(pic.bIMR) + " IRR=" + str.toHexByte(pic.bIRR) + " ISR=" + str.toHexByte(pic.bISR); + sDump += " IMR=" + str.toHexByte(pic.bIMR) + " IRR=" + str.toHexByte(pic.bIRR) + " ISR=" + str.toHexByte(pic.bISR) + " DELAY=" + pic.nDelay; this.dbg.println(sDump); } } @@ -2712,7 +2712,7 @@ ChipSet.prototype.requestDMA = function(iDMAChannel, done) var channel = controller.aChannels[iChannel]; if (!channel.component || !channel.fnTransfer || !channel.obj) { - if (DEBUG && this.messageEnabled(Debugger.MESSAGE.DMA | Debugger.MESSAGE.OTHER)) { + if (DEBUG && this.messageEnabled(Debugger.MESSAGE.DMA | Debugger.MESSAGE.DATA)) { this.messageDebugger("requestDMA(" + iDMAChannel + "): not connected to a component", true); } if (done) done(true); @@ -2729,7 +2729,7 @@ ChipSet.prototype.requestDMA = function(iDMAChannel, done) if (done) channel.done = done; if (channel.masked) { - if (DEBUG && this.messageEnabled(Debugger.MESSAGE.DMA | Debugger.MESSAGE.OTHER)) { + if (DEBUG && this.messageEnabled(Debugger.MESSAGE.DMA | Debugger.MESSAGE.DATA)) { this.messageDebugger("requestDMA(" + iDMAChannel + "): channel masked, request queued", true); } return; @@ -3174,23 +3174,22 @@ ChipSet.prototype.setIRR = function(nIRQ, nDelay) var iPIC = nIRQ >> 3; var nIRL = nIRQ & 0x7; var pic = this.aPICs[iPIC]; - pic.bIRR |= 1 << nIRL; - if (DEBUG && this.messageEnabled(this.messageBitsIRQ(nIRQ) | Debugger.MESSAGE.CHIPSET)) { - this.messageDebugger("setIRR(" + nIRQ + ")", true); + var bIRR = (1 << nIRL); + if (!(pic.bIRR & bIRR)) { + pic.bIRR |= bIRR; + if (DEBUG && this.messageEnabled(this.messageBitsIRQ(nIRQ) | Debugger.MESSAGE.CHIPSET)) { + this.messageDebugger("setIRR(" + nIRQ + ")", true); + } + pic.nDelay = nDelay || 0; + this.checkIRR(); } - pic.nDelay = nDelay || 0; - /* - * When any (unmasked) slave IRR goes high, the master's slave IRR line should go high as well - */ - if (iPIC == 1 && (pic.bIRR & ~pic.bIMR)) this.aPICs[0].bIRR |= (1 << ChipSet.IRQ.SLAVE); - this.checkIRR(); }; /** * clearIRR(nIRQ) * * @this {ChipSet} - * @param {number} nIRQ (IRQ 0-7 implies iPIC 0, which is all we currently support anyway) + * @param {number} nIRQ (IRQ 0-7 implies iPIC 0, and IRQ 8-15 implies iPIC 1) */ ChipSet.prototype.clearIRR = function(nIRQ) { @@ -3203,17 +3202,6 @@ ChipSet.prototype.clearIRR = function(nIRQ) if (DEBUG && this.messageEnabled(this.messageBitsIRQ(nIRQ) | Debugger.MESSAGE.CHIPSET)) { this.messageDebugger("clearIRR(" + nIRQ + ")", true); } - /* - * When all (unmasked) slave IRRs go low, the master's slave IRR line should go low as well - */ - if (iPIC == 1 && !(pic.bIRR & ~pic.bIMR)) this.aPICs[0].bIRR &= ~(1 << ChipSet.IRQ.SLAVE); - /* - * NOTE: I don't think calling checkIRR(), and by extension, cpu.updateINTR(false), is strictly necessary, - * because when the CPU gets around to acknowledging the INTR signal, it still has to call getIRRVector(), - * which will inform the CPU that there are no longer any requested interrupts. - * - * However, some efficiency may be gained by clearing INTR sooner rather than later, so that's what we'll do. - */ this.checkIRR(); } }; @@ -3227,20 +3215,24 @@ ChipSet.prototype.clearIRR = function(nIRQ) ChipSet.prototype.checkIRR = function(nDelay) { /* - * Look for any IRR bits that aren't masked and aren't already in service; in theory, all we have to - * check is the master PIC (which is the *only* PIC on pre-5170 models), because when any IRQs are set - * or cleared on the slave, that should automatically be reflected in IRQ.SLAVE on the master. + * Look for any IRR bits that aren't masked and aren't already in service; in theory, all we'd have to + * check is the master PIC (which is the *only* PIC on pre-5170 models), because when any IRQs are set or + * cleared on the slave, that would automatically be reflected in IRQ.SLAVE on the master; that's what + * setIRR() and clearIRR() used to do. * - * HOWEVER, when a slave interrupt is acknowledged, getIRRVector() ends up clearing the IRR bits for BOTH - * the slave's IRQ and the master's IRQ.SLAVE, setting the corresponding ISR bits, and then when interrupt - * handler send an EOI for both, the corresponding ISR bits are cleared as well. + * Unfortunately, despite setIRR() and clearIRR()'s efforts, whenever a slave interrupt is acknowledged, + * getIRRVector() ends up clearing the IRR bits for BOTH the slave's IRQ and the master's IRQ.SLAVE. + * So if another lower-priority slave IRQ is waiting to be dispatched, that fact is no longer reflected + * in IRQ.SLAVE. * - * As a result, if another lower-priority slave IRQ was waiting to be dispatched, that fact would no longer - * be reflected in the master's IRR bit for IRQ.SLAVE. We resolve that problem here, by first checking the - * slave PIC for any unmasked, unserviced interrupts and updating the master's IRR bit for IRQ.SLAVE. + * Since checkIRR() is called on every EOI, we can resolve that problem here, by first checking the slave + * PIC for any unmasked, unserviced interrupts and updating the master's IRQ.SLAVE. + * + * And since this is ALSO called by both setIRR() and clearIRR(), those functions no longer need to perform + * their own IRQ.SLAVE updates. This function consolidates the propagation of slave interrupts to the master. */ var pic; - var bIR = 0; + var bIR = -1; if (this.cPICs > 1) { pic = this.aPICs[1]; @@ -3248,7 +3240,14 @@ ChipSet.prototype.checkIRR = function(nDelay) } pic = this.aPICs[0]; - if (bIR) pic.bIRR |= (1 << ChipSet.IRQ.SLAVE); + + if (bIR >= 0) { + if (bIR) { + pic.bIRR |= (1 << ChipSet.IRQ.SLAVE); + } else { + pic.bIRR &= ~(1 << ChipSet.IRQ.SLAVE); + } + } bIR = ~(pic.bISR | pic.bIMR) & pic.bIRR; @@ -4769,7 +4768,7 @@ ChipSet.prototype.setSpeaker = function(fOn) */ ChipSet.prototype.messageBitsDMA = function(iChannel) { - var bitsMessage = Debugger.MESSAGE.DMA; + var bitsMessage = Debugger.MESSAGE.DATA; if (iChannel == ChipSet.DMA_FDC) { bitsMessage |= Debugger.MESSAGE.FDC; } else if (iChannel == ChipSet.DMA_HDC) { diff --git a/modules/pcjs/lib/debugger.js b/modules/pcjs/lib/debugger.js index c9353f9e3..fd6eb2369 100644 --- a/modules/pcjs/lib/debugger.js +++ b/modules/pcjs/lib/debugger.js @@ -58,7 +58,7 @@ if (DEBUGGER) { * commands: string containing zero or more commands, separated by ';' * * messages: string containing zero or more message categories to enable; - * multiple categories must be separated by '|' or ';'. Parsed by initMessages(). + * multiple categories must be separated by '|' or ';'. Parsed by messageInit(). * * The Debugger component is an optional component that implements a variety of user * commands for controlling the CPU, dumping and editing memory, etc. @@ -137,17 +137,17 @@ function Debugger(parmsDbg) this.clearBreakpoints(); /* - * Execution history is allocated by initHistory() whenever checksEnabled() conditions change. + * Execution history is allocated by historyInit() whenever checksEnabled() conditions change. * Execution history is updated whenever the CPU calls checkInstruction(), which will happen only * when checksEnabled() returns true (eg, whenever one or more breakpoints have been set). * This ensures that, by default, the CPU runs as fast as possible. */ - this.initHistory(); + this.historyInit(); /* * Initialize Debugger message support */ - this.initMessages(parmsDbg['messages']); + this.messageInit(parmsDbg['messages']); /* * This object is filled in by messageRegs() whenever we need a fresh snapshot. @@ -218,7 +218,7 @@ Debugger.MESSAGE = { MOUSE: 0x01000000, COMPUTER: 0x02000000, DOS: 0x04000000, - OTHER: 0x08000000, + DATA: 0x08000000, LOG: 0x10000000, WARN: 0x20000000, HALT: 0x40000000 @@ -537,7 +537,7 @@ if (DEBUGGER) { "mouse": Debugger.MESSAGE.MOUSE, "computer": Debugger.MESSAGE.COMPUTER, "dos": Debugger.MESSAGE.DOS, - "other": Debugger.MESSAGE.OTHER, + "data": Debugger.MESSAGE.DATA, "log": Debugger.MESSAGE.LOG, "warn": Debugger.MESSAGE.WARN, /* @@ -1551,12 +1551,12 @@ if (DEBUGGER) { }; /** - * initMessages(sEnable) + * messageInit(sEnable) * * @this {Debugger} * @param {string|undefined} sEnable contains zero or more message categories to enable, separated by '|' or ';' */ - Debugger.prototype.initMessages = function(sEnable) + Debugger.prototype.messageInit = function(sEnable) { this.dbg = this; this.bitsMessage = this.bitsWarning = Debugger.MESSAGE.WARN; @@ -1633,9 +1633,17 @@ if (DEBUGGER) { */ Debugger.prototype.messageInt = function(nInt, addr) { + var AH; + var fMessage = false; var nCategory = Debugger.INT_MESSAGE[nInt]; - var fMessage = nCategory && this.messageEnabled(nCategory); - var AH = this.cpu.regAX >> 8; + if (nCategory) { + AH = this.cpu.regAX >> 8; + if (this.messageEnabled(nCategory)) { + fMessage = true; + } else { + fMessage = (nCategory == Debugger.MESSAGE.FDC && this.messageEnabled(nCategory = Debugger.MESSAGE.HDC)); + } + } if (fMessage) { var DL = this.cpu.regDX & 0xff; if (nInt == Debugger.INT.DOS && AH == 0x0b || @@ -1798,7 +1806,7 @@ if (DEBUGGER) { }; /** - * initHistory() + * historyInit() * * This function is intended to be called by the constructor, reset(), addBreakpoint(), findBreakpoint() * and any other function that changes the checksEnabled() criteria used to decide whether checkInstruction() @@ -1809,7 +1817,7 @@ if (DEBUGGER) { * * @this {Debugger} */ - Debugger.prototype.initHistory = function() + Debugger.prototype.historyInit = function() { var i; if (!this.checksEnabled()) { @@ -2008,7 +2016,7 @@ if (DEBUGGER) { */ Debugger.prototype.reset = function(fQuiet) { - this.initHistory(); + this.historyInit(); this.cInstructions = 0; this.nCycles = 0; this.aAddrNextCode = this.newAddr(this.cpu.regIP, this.cpu.segCS.sel); @@ -2185,7 +2193,7 @@ if (DEBUGGER) { } /* - * The rest of the instruction tracking logic can only be performed if initHistory() has allocated + * The rest of the instruction tracking logic can only be performed if historyInit() has allocated * the necessary data structures; note that there is no explicit UI for enabling/disabling history, * other than adding/removing breakpoints, simply because it's breakpoints that trigger the call to * checkInstruction() -- well, OK, and a few other things now, like enabling MESSAGE_INT messages. @@ -2198,7 +2206,7 @@ if (DEBUGGER) { /* * This is a good example of what NOT to do in a high-frequency function, and defeats - * the purpose of preallocating and preinitializing the history array in initHistory(): + * the purpose of preallocating and preinitializing the history array in historyInit(): * * this.aOpcodeHistory[this.iOpcodeHistory] = this.newAddr(this.cpu.regIP, this.cpu.segCS.sel, addr); * @@ -2542,7 +2550,7 @@ if (DEBUGGER) { this.bus.addMemoryBreakpoint(this.getAddr(aAddr), aBreak == this.aBreakWrite); } if (!fTemp) this.println("breakpoint enabled: " + this.hexAddr(aAddr) + " (" + aBreak[0] + ")"); - this.initHistory(); + this.historyInit(); return true; } return false; @@ -2571,7 +2579,7 @@ if (DEBUGGER) { this.bus.removeMemoryBreakpoint(addr, aBreak == this.aBreakWrite); } if (!aAddrBreak[3]) this.println("breakpoint cleared: " + this.hexAddr(aAddrBreak) + " (" + aBreak[0] + ")"); - this.initHistory(); + this.historyInit(); break; } this.println("breakpoint exists: " + this.hexAddr(aAddrBreak) + " (" + aBreak[0] + ")"); diff --git a/modules/pcjs/lib/disk.js b/modules/pcjs/lib/disk.js index 39d8f9893..2ce3cedfa 100644 --- a/modules/pcjs/lib/disk.js +++ b/modules/pcjs/lib/disk.js @@ -1669,12 +1669,12 @@ Disk.prototype.dumpSector = function(sector) var cdwData = sector['data'].length; var dw = 0; for (var i = 0; i < cbSector; i++) { - if (i % 16 == 0) { + if ((i % 16) === 0) { if (sDump) sDump += sBytes + ' ' + sChars + '\n'; sDump += str.toHexWord(i) + ": "; sBytes = sChars = ""; } - if (i % 4 == 0) { + if ((i % 4) === 0) { var idw = i >> 2; dw = (idw < cdwData? sector['data'][idw] : sector['pattern']); } diff --git a/modules/pcjs/lib/hdc.js b/modules/pcjs/lib/hdc.js index 9e373ba01..a2b1181a2 100644 --- a/modules/pcjs/lib/hdc.js +++ b/modules/pcjs/lib/hdc.js @@ -1412,7 +1412,7 @@ HDC.prototype.inATCData = function(port, addrFrom) } else if (this.drive.ibSector == this.drive.cbSector) { - if (this.messageEnabled(Debugger.MESSAGE.OTHER | Debugger.MESSAGE.HDC)) { + if (this.messageEnabled(Debugger.MESSAGE.DATA | Debugger.MESSAGE.HDC)) { var sDump = this.drive.disk.dumpSector(this.drive.sector); if (sDump) this.dbg.message(sDump); } @@ -1483,7 +1483,7 @@ HDC.prototype.outATCData = function(port, bOut, addrFrom) } else if (this.drive.ibSector == this.drive.cbSector) { - if (this.messageEnabled(Debugger.MESSAGE.OTHER | Debugger.MESSAGE.HDC)) { + if (this.messageEnabled(Debugger.MESSAGE.DATA | Debugger.MESSAGE.HDC)) { var sDump = this.drive.disk.dumpSector(this.drive.sector); if (sDump) this.dbg.message(sDump); } @@ -1508,7 +1508,7 @@ HDC.prototype.outATCData = function(port, bOut, addrFrom) } } else { /* - * TODO: What to do about unexpected writes? The number of bytes has exceeded what the command specified. + * TODO: What to do about unexpected writes? No command was specified. */ if (DEBUG && this.messageEnabled()) { this.messageDebugger("HDC.outATCData(" + str.toHexByte(bOut) + "): write without command"); @@ -1723,7 +1723,24 @@ HDC.prototype.inATCStatus = function(port, addrFrom) { var bIn = this.regStatus; this.messagePort(port, null, addrFrom, "STATUS", bIn); - if (this.chipset) this.chipset.clearIRR(ChipSet.IRQ.ATC); + /* + * Despite what IBM's documentation for the "Personal Computer AT Fixed Disk and Diskette Drive Adapter" + * (August 31, 1984) says (ie, "A read of the status register clears interrupt request 14"), we cannot + * unilaterally clear the IRQ on any read of STATUS. For starters, that would completely break the PC AT + * ROM BIOS; here's what it does for multi-sector reads: + * + * (1) read sector (REP INSW) + * (2) check STATUS + * (3) check sector count, exit if done + * (4) wait for interrupt + * (5) repeat + * + * Since we set the IRR immediately after (1), we cannot immediately clear the IRR at (2), otherwise the + * interrupt at (4) never happens. So, maybe there are SOME situations where IRR should be cleared on + * a read, but I don't know what they are. + * + * if (this.chipset) this.chipset.clearIRR(ChipSet.IRQ.ATC); + */ return bIn; }; @@ -1921,7 +1938,29 @@ HDC.prototype.setATCIRR = function(fWrite) { if (this.chipset) { if (!(this.regFDR & HDC.ATC.FDR.INT_DISABLE)) { - this.chipset.setIRR(ChipSet.IRQ.ATC); + /* + * TODO: Determine what the "correct" instruction delay should be here. When the OS/2 1.0 Install Disk + * begins copying files to the hard disk, at one point it performs the following 125-sector write (use the + * Debugger's "m hdc on" and "m pic on" commands to enable HDC and PIC messages, along with "m data on" + * if you also want to see the actual sector data being written): + * + * HDC.doATC(0x30): Write + * HDC.doWrite(0,2:0:5,125) + * + * As the write progresses, you'll notice that the HDC interrupt after each sector occurs at decreasingly + * lower points in the stack, until we eventually start overwriting non-stack data: + * + * getIRRVector(): IRQ 14 interrupting @0090:52A6 stack=0050:1906 + * getIRRVector(): IRQ 14 interrupting @0318:196B stack=0050:18D6 + * getIRRVector(): IRQ 14 interrupting @0318:196B stack=0050:18A6 + * ... + * getIRRVector(): IRQ 14 interrupting @0318:196B stack=0050:1156 + * + * At roughly this point, very bad things start happening. I decided to try an arbitrarily large delay + * on the setIRR() call here (120), and the problem vanished, so it seems likely that the OS/2 disk driver + * has a low tolerance for fast controller interrupts during multi-sector operations. + */ + this.chipset.setIRR(ChipSet.IRQ.ATC, 120); if (DEBUG) this.messageDebugger("HDC.setATCIRR(): enabled", Debugger.MESSAGE.PIC | Debugger.MESSAGE.HDC); } else { if (DEBUG) this.messageDebugger("HDC.setATCIRR(): disabled", Debugger.MESSAGE.PIC | Debugger.MESSAGE.HDC); @@ -2375,10 +2414,12 @@ HDC.prototype.doDMAWriteBuffer = function(drive, done) * * and we expect the DMA controller to provide C, H, R and N (ie, 4 bytes) for each sector to be formatted. * + * NOTE: This function is not currently used. + * * @this {HDC} * @param {Object} drive * @param {function(number)} done (dataStatus is XTC.DATA.STATUS_OK or XTC.DATA.STATUS_ERROR; if error, then drive.errorCode should be set as well) - */ + * HDC.prototype.doDMAFormat = function(drive, done) { drive.errorCode = HDC.XTC.DATA.ERR.NOT_READY; @@ -2390,20 +2431,20 @@ HDC.prototype.doDMAFormat = function(drive, done) drive.abFormat = new Array(4); drive.bFormatting = true; drive.cSectorsFormatted = 0; - /* - * We need to reverse the original logic, and default to success unless/until an actual error occurs; - * otherwise dmaWriteFormat() will bail on us. The original approach would work because requestDMA() - * would immediately call us back with fComplete set to true EVEN if the DMA channel was not yet unmasked; - * now the callback is deferred until the DMA channel has been unmasked and the DMA request has finished. - */ + // + // We need to reverse the original logic, and default to success unless/until an actual error occurs; + // otherwise dmaWriteFormat() will bail on us. The original approach would work because requestDMA() + // would immediately call us back with fComplete set to true EVEN if the DMA channel was not yet unmasked; + // now the callback is deferred until the DMA channel has been unmasked and the DMA request has finished. + // drive.errorCode = HDC.XTC.DATA.ERR.NONE; this.chipset.connectDMA(ChipSet.DMA_HDC, this, 'dmaWriteFormat', drive); this.chipset.requestDMA(ChipSet.DMA_HDC, function(fComplete) { if (!fComplete) { - /* - * If an incomplete request wasn't triggered by an explicit error, then let's make explicit - * (ie, revert to the default failure code that we originally set above). - */ + // + // If an incomplete request wasn't triggered by an explicit error, then let's make explicit + // (ie, revert to the default failure code that we originally set above). + // if (drive.errorCode == HDC.XTC.DATA.ERR.NONE) { drive.errorCode = HDC.XTC.DATA.ERR.NOT_READY; } @@ -2416,6 +2457,7 @@ HDC.prototype.doDMAFormat = function(drive, done) } done(drive.errorCode? HDC.XTC.DATA.STATUS_ERROR : HDC.XTC.DATA.STATUS_OK); }; + */ /** * readByte(drive, done) diff --git a/modules/pcjs/lib/x86cpu.js b/modules/pcjs/lib/x86cpu.js index 6ee82017d..aea2c602a 100644 --- a/modules/pcjs/lib/x86cpu.js +++ b/modules/pcjs/lib/x86cpu.js @@ -1038,6 +1038,9 @@ X86CPU.prototype.setProtMode = function(fProt) if (fProt === undefined) { fProt = !!(this.regMSW & X86.MSW.PE); } + if (!fProt) { + this.messageDebugger("returning to real-mode"); + } this.aOpGrp6 = (fProt? X86Op0F.aOpGrp6Prot : X86Op0F.aOpGrp6Real); this.segCS.updateMode(fProt); this.segDS.updateMode(fProt); diff --git a/modules/pcjs/lib/x86help.js b/modules/pcjs/lib/x86help.js index d5fa674f9..23fb5e6fd 100644 --- a/modules/pcjs/lib/x86help.js +++ b/modules/pcjs/lib/x86help.js @@ -433,7 +433,6 @@ var X86Help = { * case, but this instruction isn't used frequently enough to warrant it). */ if (this.regMSW & X86.MSW.PE) this.setProtMode(true); - }, /** * opHelpCALLF(off, sel) diff --git a/modules/pcjs/lib/x86seg.js b/modules/pcjs/lib/x86seg.js index a76eb98a9..4036a9c04 100644 --- a/modules/pcjs/lib/x86seg.js +++ b/modules/pcjs/lib/x86seg.js @@ -699,7 +699,7 @@ X86Seg.prototype.loadDesc8 = function(addrDesc, sel, fSuppress) * * Anyway, because of this, if acc is zero, we won't set fHalt on this GP_FAULT. */ - if (!fSuppress) X86Help.opHelpFault.call(cpu, X86.EXCEPTION.GP_FAULT, sel, acc != 0); + if (!fSuppress) X86Help.opHelpFault.call(cpu, X86.EXCEPTION.GP_FAULT, sel, !!acc); base = X86.ADDR_INVALID; break; }