From a474ddfa74d516ff8fd409e32ba6073106426945 Mon Sep 17 00:00:00 2001 From: Jeff Parsons Date: Thu, 2 Oct 2014 10:48:41 -0700 Subject: [PATCH] Fix an inconsistency in Debugger address parsing (problems mixing segment and physical addresses) --- my_modules/pcjs-client/lib/chipset.js | 65 ++++++++++++++------------ my_modules/pcjs-client/lib/debugger.js | 26 ++++++----- my_modules/pcjs-client/lib/keyboard.js | 4 +- 3 files changed, 52 insertions(+), 43 deletions(-) diff --git a/my_modules/pcjs-client/lib/chipset.js b/my_modules/pcjs-client/lib/chipset.js index 6363f7e0f..70427203f 100644 --- a/my_modules/pcjs-client/lib/chipset.js +++ b/my_modules/pcjs-client/lib/chipset.js @@ -72,7 +72,7 @@ if (typeof module !== 'undefined') { * SW2[1-4] (bits 3-0) "NNNNxxxx": number of 32Kb blocks of I/O expansion RAM present * * TODO: There are cryptic references to SW2[5] in the original (5150) TechRef, and apparently the 8255A PPI can - * be programmed to return it (which we support), but its purpose is unclear to me (see PPI_B_ENABLE_SW2). + * be programmed to return it (which we support), but its purpose is unclear to me (see PPI_B.ENABLE_SW2). * * For example, sw1="01110011" indicates that all SW1 DIP switches are ON, except for SW1[1], SW1[5] and SW1[6], * which are OFF. Internally, the order of these bits must reversed (to 11001110) and then inverted (to 00110001) @@ -480,7 +480,7 @@ ChipSet.TIMER_TICKS_PER_SEC = 1193181; * with the usual 0x99. */ ChipSet.PPI_A = {}; // this.bPPIA -ChipSet.PPI_A.PORT = 0x60; // INPUT: keyboard scan code (PPI_B_CLEAR_KBD must be clear) +ChipSet.PPI_A.PORT = 0x60; // INPUT: keyboard scan code (PPI_B.CLEAR_KBD must be clear) ChipSet.PPI_B = {}; // this.bPPIB ChipSet.PPI_B.PORT = 0x61; // OUTPUT (although it has to be treated as INPUT, too: the keyboard interrupt handler reads it, OR's PPI_B.CLEAR_KBD, writes it, and then rewrites the original read value) @@ -496,7 +496,7 @@ ChipSet.PPI_B.CLEAR_KBD = 0x80; // ALL: clear to enable keyboard scan co ChipSet.PPI_C = {}; // this.bPPIC ChipSet.PPI_C.PORT = 0x62; // INPUT (see below) -ChipSet.PPI_C.SW = 0x0F; // MODEL_5150: SW2[1-4] or SW2[5], depending on whether PPI_B_ENABLE_SW2 is set or clear; MODEL_5160: SW1[1-4] or SW1[5-8], depending on whether PPI_B_ENABLE_SW_HI is clear or set +ChipSet.PPI_C.SW = 0x0F; // MODEL_5150: SW2[1-4] or SW2[5], depending on whether PPI_B.ENABLE_SW2 is set or clear; MODEL_5160: SW1[1-4] or SW1[5-8], depending on whether PPI_B.ENABLE_SW_HI is clear or set ChipSet.PPI_C.CASS_DATA_IN = 0x10; ChipSet.PPI_C.TIMER2_OUT = 0x20; ChipSet.PPI_C.IO_CHANNEL_CHK = 0x40; // used by NMI handler to detect I/O channel errors @@ -514,7 +514,7 @@ ChipSet.PPI_CTRL.A_MODE = 0x60; /* * On the MODEL_5150, the following PPI_SW bits are exposed through PPI_A. * - * On the MODEL_5160, either the low or high 4 bits are exposed through PPI_C_SW, if PPI_B_ENABLE_SW_HI is clear or set. + * On the MODEL_5160, either the low or high 4 bits are exposed through PPI_C_SW, if PPI_B.ENABLE_SW_HI is clear or set. */ ChipSet.PPI_SW = {}; ChipSet.PPI_SW.FDRIVE = {}; @@ -3288,29 +3288,6 @@ ChipSet.prototype.updateAllTimers = function(fCycleReset) if (this.model >= ChipSet.MODEL_5170) this.updateRTCDate(); }; -/** - * updateSpeaker(bOut) - * - * @this {ChipSet} - * @param {number} bOut - */ -ChipSet.prototype.updateSpeaker = function(bOut) -{ - var fNewSpeaker = !!(bOut & ChipSet.PPI_B.SPK_TIMER2); - var fOldSpeaker = !!(this.bPPIB & ChipSet.PPI_B.SPK_TIMER2); - this.bPPIB = bOut; - if (fNewSpeaker != fOldSpeaker) { - /* - * Originally, this code didn't catch the "ERROR_BEEP" case @F000:EC34, which first turns both PPI_B_CLK_TIMER2 (0x01) - * and PPI_B_SPK_TIMER2 (0x02) off, then turns on only PPI_B_SPK_TIMER2 (0x02), then restores the original port value. - * - * So, when the ROM BIOS keyboard buffer got full, we didn't issue a BEEP alert. I've fixed that by limiting the test - * to PPI_B_SPK_TIMER2 and ignoring PPI_B_CLK_TIMER2. - */ - this.setSpeaker(fNewSpeaker); - } -}; - /** * inPPIA(port, addrFrom) * @@ -3374,10 +3351,38 @@ ChipSet.prototype.inPPIB = function(port, addrFrom) ChipSet.prototype.outPPIB = function(port, bOut, addrFrom) { this.messagePort(port, bOut, addrFrom, "PPI_B", ChipSet.MESSAGE_CHIPSET); - this.updateSpeaker(bOut); + this.updatePPIB(bOut); if (this.kbd) this.kbd.setEnable((bOut & ChipSet.PPI_B.CLEAR_KBD)? false : true, (bOut & ChipSet.PPI_B.CLK_KBD)? true : false); }; +/** + * updatePPIB(bOut) + * + * On MODEL_5170 and up, this updates the "simulated" PPI_B. The only common (and well-documented) PPI_B bits + * across all models are PPI_B.CLK_TIMER2 and PPI_B.SPK_TIMER2, so its possible that this function may need to + * limit its updates to just those bits, and move any model-specific requirements back into the appropriate I/O + * handlers (PPIB or 8042RWReg). We'll see. + * + * @this {ChipSet} + * @param {number} bOut + */ +ChipSet.prototype.updatePPIB = function(bOut) +{ + var fNewSpeaker = !!(bOut & ChipSet.PPI_B.SPK_TIMER2); + var fOldSpeaker = !!(this.bPPIB & ChipSet.PPI_B.SPK_TIMER2); + this.bPPIB = bOut; + if (fNewSpeaker != fOldSpeaker) { + /* + * Originally, this code didn't catch the "ERROR_BEEP" case @F000:EC34, which first turns both PPI_B.CLK_TIMER2 (0x01) + * and PPI_B.SPK_TIMER2 (0x02) off, then turns on only PPI_B.SPK_TIMER2 (0x02), then restores the original port value. + * + * So, when the ROM BIOS keyboard buffer got full, we didn't issue a BEEP alert. I've fixed that by limiting the test + * to PPI_B.SPK_TIMER2 and ignoring PPI_B.CLK_TIMER2. + */ + this.setSpeaker(fNewSpeaker); + } +}; + /** * inPPIC(port, addrFrom) * @@ -3523,7 +3528,7 @@ ChipSet.prototype.out8042InBuffData = function(port, bOut, addrFrom) * Here's some relevant MODEL_5170 ROM BIOS code, "XMIT_8042" (missing from the original MODEL_5170 ROM BIOS listing), * which sends a command code in AL to the Keyboard and waits for a response, returning it in AL. Note that * the only "success" exit path from this function involves LOOPing 64K times before finally reading the Keyboard's - * response; either the hardware and/or this code seems a bit brain-damaged if that's REALLY what you had to do to get + * response; either the hardware and/or this code seems a bit brain-damaged if that's REALLY what you had to do to ensure * a valid response.... * * F000:1B25 86E0 XCHG AH,AL @@ -3624,7 +3629,7 @@ ChipSet.prototype.in8042RWReg = function(port, addrFrom) ChipSet.prototype.out8042RWReg = function(port, bOut, addrFrom) { this.messagePort(port, bOut, addrFrom, "8042_RWREG", ChipSet.MESSAGE_CHIPSET); - this.updateSpeaker(bOut); + this.updatePPIB(bOut); }; /** diff --git a/my_modules/pcjs-client/lib/debugger.js b/my_modules/pcjs-client/lib/debugger.js index 916501f94..3dd863d55 100644 --- a/my_modules/pcjs-client/lib/debugger.js +++ b/my_modules/pcjs-client/lib/debugger.js @@ -87,10 +87,10 @@ function Debugger(parmsDbg) * update aAddrNextData and aAddrNextCode, respectively, when they're done. * * The format of all aAddr variables is [off, seg, addr], where seg:off is the segmented - * address and addr is the corresponding physical address. For some segmented addresses - * (eg, breakpoint addresses), we pre-compute the physical address and save that in aAddr[2], - * so that the breakpoint will still operate as intended even if the mode changes later - * (eg, from real-mode to protected-mode). + * address and addr is the corresponding physical address (if known). For certain segmented + * addresses (eg, breakpoint addresses), we pre-compute the physical address and save that + * in aAddr[2], so that the breakpoint will still operate as intended even if the mode changes + * later (eg, from real-mode to protected-mode). * * Finally, for TEMPORARY breakpoint addresses, we set aAddr[3] to true, so that they can be * automatically cleared when they're hit. @@ -2112,7 +2112,9 @@ if (DEBUGGER) { } if (aAddr[1] != null) { aAddr[0] += inc; - // TODO: Shouldn't we be using the segment (aAddr[1]) limit instead of 0xffff? + /* + * TODO: Shouldn't we be using the segment (aAddr[1]) limit instead of 0xffff? + */ if (aAddr[0] != (aAddr[0] & 0xffff)) { aAddr[0] = aAddr[0] & 0xffff; aAddr[2] = null; @@ -2318,6 +2320,7 @@ if (DEBUGGER) { * or history data (see checkInstruction), since we might not actually execute the current instruction. */ var fBreak = false; + /* * Map addresses in the top 64Kb (at the top of the 16Mb range) to the top of the 1Mb range. * @@ -2326,6 +2329,7 @@ if (DEBUGGER) { * in the top 16Mb, yet after the first inter-segment JMP, you will be running in the first 1Mb. */ if ((addr & 0xFF0000) == 0xFF0000) addr &= 0x0FFFFF; + for (var i = 1; i < aBreak.length; i++) { var aAddrBreak = aBreak[i]; if (addr == this.getAddr(aAddrBreak)) { @@ -2731,9 +2735,8 @@ if (DEBUGGER) { * addresses. If/when we add support for processors with page tables, we will likely adopt the same * convention for linear addresses and provide a different syntax (eg, "%%") physical memory references. * - * Address evaluation and validation (eg, range checks) are no longer performed at this stage, which - * is why aAddr variables no longer contain an addr element; computing addr is now performed as-needed - * by getAddr(), which will return a negative result (-1) for invalid segments, out-of-range offsets, + * Address evaluation and validation (eg, range checks) are no longer performed at this stage. That's + * done later, by getAddr(), which returns a negative result (-1) for invalid segments, out-of-range offsets, * etc. The Debugger's low-level get/set memory functions verify all getAddr() results, but even if an * invalid address is passed through to the Bus memory interfaces, the address will simply be masked with * Bus.addrLimit; in the case of -1, that will generally refer to the last byte of physical memory. @@ -2762,7 +2765,7 @@ if (DEBUGGER) { var iColon = sAddr.indexOf(":"); if (iColon < 0) { - if (addr == null) { + if (seg != null) { off = this.parseValue(sAddr); } else { addr = this.parseValue(sAddr); @@ -2771,6 +2774,7 @@ if (DEBUGGER) { else { seg = this.parseValue(sAddr.substring(0, iColon)); off = this.parseValue(sAddr.substring(iColon + 1)); + addr = null; } } return [off, seg, addr]; @@ -4020,8 +4024,8 @@ if (DEBUGGER) { this.aAddrNextCode = this.newAddr(this.cpu.regIP, this.cpu.segCS.sel); break; /* - * I used to alias "PC" to "IP", until I discovered that early (perhaps even ALL?) versions of DEBUG - * treat "PC" as an alias for the 16-bit flags register. TODO: Add support for "PC" that matches DEBUG. + * I used to alias "PC" to "IP", until I discovered that early (perhaps ALL) versions of + * DEBUG.COM treat "PC" as an alias for the 16-bit flags register. TODO: Add support for "PC". */ case "IP": fIns = true; diff --git a/my_modules/pcjs-client/lib/keyboard.js b/my_modules/pcjs-client/lib/keyboard.js index 985d65767..899f55e1f 100644 --- a/my_modules/pcjs-client/lib/keyboard.js +++ b/my_modules/pcjs-client/lib/keyboard.js @@ -589,8 +589,8 @@ Keyboard.prototype.resetDevice = function() /** * setEnable(fEnable, fClock) * - * This is the ChipSet's primary interface for controlling "Model F" keyboards (ie, those used - * with MODEL_5150 and MODEL_5160 machines) + * This is the ChipSet's primary interface for controlling "Model F" keyboards (ie, those used with + * MODEL_5150 and MODEL_5160 machines). This function is called from the ChipSet's PPI_B output handler. * * @this {Keyboard} * @param {boolean} fEnable is true if the keyboard interface should be enabled