From 89631006f2fbb019fb5e270acaa298e7827d25ef Mon Sep 17 00:00:00 2001 From: Scott Duensing Date: Thu, 20 Aug 2026 01:22:50 -0500 Subject: [PATCH] GS Serial fixes. --- LLVM816-ASKS.md | 34 ++++++++++++ src/iigs/serial.c | 132 +++++++++++++++++++++++++++++++++++++++++----- tools/xdftool.py | 30 +++++++++-- 3 files changed, 178 insertions(+), 18 deletions(-) diff --git a/LLVM816-ASKS.md b/LLVM816-ASKS.md index 82d8856..f7e885d 100644 --- a/LLVM816-ASKS.md +++ b/LLVM816-ASKS.md @@ -179,3 +179,37 @@ exit path. jlShutdown therefore does not return on IIgs. The root cause still deserves a runtime fix: any app that returns from main WITHOUT calling jlShutdown (or any non-JoeyLib program built on this runtime) still crashes. + +## 7. Volatile uint8_t loads through variable pointers widen to 16-bit bus reads (2026-08-18) -- EMPIRICALLY PROVEN + +A `volatile uint8_t` LOAD through a variable (long) pointer is emitted as a +16-bit `lda [dp],y` with m=0 and the value masked afterwards (`and #$ff`) -- +so the BUS performs a phantom read of address+1. For memory that is harmless; +for I/O it reads a neighbouring register, and hardware with adjacent registers +has read side effects. Concrete damage on the IIgs SCC (Z8530, four adjacent +registers $C038-$C03B): a data-register read of channel B ($C03A) phantom-pops +channel A's RX FIFO ($C03B -- the AppleTalk port), and in a printer-port +configuration a status poll of $C039 phantom-pops the MODEM port's data at +$C03A. (A per-poll RR1 harvest also correlates with MAME 0.264 SCC RX-deafness, but +that is NOT this bug: it reproduced through a guaranteed 8-bit read too, so +the phantom read is refuted as its mechanism and the harvest ships disabled +under emulation. The width violation itself stands proven by disassembly.) + +The width handling is inconsistent, which is what makes it look like a bug +rather than a policy: + +- STORES through the same pointers are correctly sep #$20-wrapped 8-bit. +- LOADS from CONSTANT addresses are correctly sep-wrapped 8-bit + (`sep #$20 / lda $c038 / rep #$20` -- verified via a probe TU). +- Only variable-pointer loads take the 16-bit `lda [dp],y` + mask form. + +Ask: honour the declared access width for volatile loads in the long-indirect +form -- sep-wrap them exactly like the constant-address and store cases. + +WORKAROUND LANDED IN JOEYLIB: src/iigs/serial.c routes every serial register +read through a hand-asm `jlpIoRead8` (global function, so no call site can be +inlined back into the widened form): `sep #$20 / lda [$e0],y / rep #$30 / +and #$ff`. ABI verified from a compiled probe: pointer lo16 in A, bank in X, +return in A, $e0-$e3 imaginary-register scratch. Any OTHER volatile I/O read +through a variable pointer (input/video HALs, future code) is still exposed +until the codegen fix lands. diff --git a/src/iigs/serial.c b/src/iigs/serial.c index 3e26be8..de5f9af 100644 --- a/src/iigs/serial.c +++ b/src/iigs/serial.c @@ -52,9 +52,15 @@ #define SCC_RR0_RX_AVAIL 0x01 #define SCC_RR0_TX_EMPTY 0x04 +// RR1 bit 5: RX overrun - the 3-byte hardware FIFO was full when another byte finished, and that byte +// is GONE. Latched until an Error Reset (WR0 command 0x30), so it survives to the next pump pass. +#define SCC_RR1_RX_OVRN 0x20 +#define SCC_WR0_ERR_RESET 0x30 + // 6551 status bits. #define ACIA_ST_RX_FULL 0x08 #define ACIA_ST_TX_EMPTY 0x10 +#define ACIA_ST_RX_OVRN 0x04 // SCC baud time constant: BRG output = PCLK(3.6864MHz) / (2*(TC+2)) with the // /16 clock mode, so baud = 115200 / (TC+2) -> TC = 115200/baud - 2. @@ -76,6 +82,14 @@ static uint8_t gRxRing[RX_RING_SIZE]; // ring->caller since open. Cheap enough to keep unconditionally. uint16_t gIigsRxPumped = 0u; uint16_t gIigsRxRead = 0u; + +// Receive-loss counters, same read-via-extern contract. Overruns = loss EVENTS the hardware latched +// (SCC RR1 overrun / 6551 status bit 2) - one latch observation may stand for SEVERAL destroyed bytes, +// so this is a loss indicator, not a byte count. Before it existed a pump-discipline gap was invisible +// except as a frame CRC drop and a retransmit stall. Dropped = bytes the SOFT ring refused because it +// was full (read from the chip, then discarded); saturating, like the overrun count. +uint16_t gIigsRxOverruns = 0u; +uint16_t gIigsRxDropped = 0u; static uint16_t gRxHead; static uint16_t gRxTail; static bool gOpen; @@ -86,13 +100,44 @@ static volatile uint8_t *gData; // data register (both) // ----- Prototypes ----- +extern uint8_t jlpIoRead8(const volatile uint8_t *p); static void serialInitAcia(volatile uint8_t *base, const jlSerialConfigT *cfg); static void serialInitScc(volatile uint8_t *ctrl, volatile uint8_t *data, uint8_t reset, const jlSerialConfigT *cfg); -static bool serialRxReady(void); static bool serialTxReady(void); +static uint8_t sccRead(volatile uint8_t *ctrl, uint8_t reg); static void sccWrite(volatile uint8_t *ctrl, uint8_t reg, uint8_t val); +// ----- Guaranteed 8-bit I/O read ----- +// +// llvm816 widens a volatile uint8_t LOAD through a variable long pointer to a 16-bit `lda [dp],y` +// (m=0), so every such register read also performs a phantom bus read of address+1 - and the SCC's +// four registers are adjacent, so a data read on one channel phantom-pops the OTHER channel's data +// FIFO ($C03A reads $C03B: channel A, the AppleTalk port). Constant-address loads are sep-wrapped +// correctly; only the pointer form is affected, and STORES are always sep-wrapped. Filed in +// llvm816/LLVM816-ASKS.md; until the toolchain honours volatile width, every serial register READ +// goes through this hand-asm helper - a global function so no call site can be inlined back into +// the widened form. ABI from a compiled probe: pointer lo16 in A, bank in X; return in A (high +// byte cleared), X zeroed; $e0-$e3 are call-clobbered imaginary-register scratch, D points at the +// imaginary page on entry (the same assumption every compiled function makes). +__asm__( + ".section .text.jlpIoRead8,\"ax\",@progbits\n" + ".globl jlpIoRead8\n" + "jlpIoRead8:\n" + "\trep #$30\n" // known width state: 16-bit A and index + "\tsta $e0\n" // pointer low 16 -> $e0-$e1 + "\ttxa\n" + "\tsta $e2\n" // bank -> $e2 ($e3 catches the junk high byte - scratch) + "\tldy #0\n" + "\tsep #$20\n" // 8-bit accumulator: the actual fix + "\tlda [$e0],y\n" // ONE byte on the bus - no phantom companion read + "\trep #$30\n" + "\tand #$ff\n" // the hidden B accumulator held junk; clear it for the return + "\tldx #0\n" + "\trtl\n" +); + + // ----- SCC helpers ----- // Register-pointer protocol: point at WRn (if not WR0), then write the value. @@ -104,6 +149,17 @@ static void sccWrite(volatile uint8_t *ctrl, uint8_t reg, uint8_t val) { } +// Point at RRn, then read it. Same adjacency rule as sccWrite: nothing may touch the SCC between the +// pointer write and the read (safe here - WR1=0 means this channel never interrupts, and the HAL is +// single-threaded polled). +static uint8_t sccRead(volatile uint8_t *ctrl, uint8_t reg) { + if (reg != 0u) { + *ctrl = reg; + } + return jlpIoRead8(ctrl); +} + + static void serialInitScc(volatile uint8_t *ctrl, volatile uint8_t *data, uint8_t reset, const jlSerialConfigT *cfg) { uint8_t wr4; uint8_t wr3; @@ -218,19 +274,11 @@ static void serialInitAcia(volatile uint8_t *base, const jlSerialConfigT *cfg) { // ----- Status polling ----- -static bool serialRxReady(void) { - if (gIsScc) { - return (*gStatus & SCC_RR0_RX_AVAIL) != 0u; // RR0 read directly - } - return (*gStatus & ACIA_ST_RX_FULL) != 0u; -} - - static bool serialTxReady(void) { if (gIsScc) { - return (*gStatus & SCC_RR0_TX_EMPTY) != 0u; + return (jlpIoRead8(gStatus) & SCC_RR0_TX_EMPTY) != 0u; } - return (*gStatus & ACIA_ST_TX_EMPTY) != 0u; + return (jlpIoRead8(gStatus) & ACIA_ST_TX_EMPTY) != 0u; } @@ -316,18 +364,74 @@ void jlpSerialPoll(void) { // DBR trap that file-scope `++` lowers to on the 65816. tail = gRxTail; guard = RX_RING_SIZE; - while (guard != 0u && serialRxReady()) { - uint16_t next = (uint16_t)((tail + 1u) & RX_RING_MASK); - uint8_t b = *gData; + // Counter updates below use the guarded local read-modify-write from input.c's joystick counter - + // NOT because of the spelling (clang canonicalizes `x++` and `x = x + 1` identically) but because + // the saturation compare keeps the loaded value LIVE, which is what stops the backend fusing the + // update into a DBR-relative `inc abs`. Saturating is also the right behaviour for a loss counter. + while (guard != 0u) { + uint16_t next; + uint8_t st; + uint8_t b; + st = jlpIoRead8(gStatus); // RR0 (SCC) or 6551 status + if (gIsScc) { + if ((st & SCC_RR0_RX_AVAIL) == 0u) { + break; + } + } else { + // The 6551 latches overrun ONLY while RDRF is still set, and the DATA read below retires + // both bits together - so the overrun must be sampled HERE, from the same status read that + // gates the byte. (A post-loop check can never see it: the drain already cleared it.) + if ((st & ACIA_ST_RX_OVRN) != 0u) { + uint16_t ovr = gIigsRxOverruns; + + if (ovr < 0xFFFFu) { + gIigsRxOverruns = (uint16_t)(ovr + 1u); + } + } + if ((st & ACIA_ST_RX_FULL) == 0u) { + break; + } + } + next = (uint16_t)((tail + 1u) & RX_RING_MASK); + b = jlpIoRead8(gData); if (next != gRxHead) { gRxRing[tail] = b; tail = next; gIigsRxPumped++; + } else { + uint16_t drop = gIigsRxDropped; + + if (drop < 0xFFFFu) { + gIigsRxDropped = (uint16_t)(drop + 1u); + } } guard--; } gRxTail = tail; + // Harvest the SCC's own loss record: real silicon latches RX overrun in RR1 until an Error Reset, + // so one check per pump names a deaf-window loss even though the pump never saw the byte. + // + // DEFAULT OFF - REAL HARDWARE ONLY, on two measured grounds: (1) under MAME the counter is INERT + // (its z80scc never surfaces the flag into RR1), so enabling it under emulation buys nothing; and + // (2) with it enabled the MAME 0.264 session goes RX-deaf after HELLO - 3 of 4 runs, INCLUDING one + // through jlpIoRead8's true 8-bit reads, so the phantom companion read is NOT the mechanism (one + // green run said otherwise and the next red run retracted it - the two-greens-prove-nothing rule). + // Whatever the per-pass WR0-pointer/RR1 dance disturbs in MAME's SCC remains unpinned; it is not + // worth more rig time for a diagnostic that cannot fire there. JL_IIGS_RR1_HARVEST enables it + // for silicon validation. +#ifdef JL_IIGS_RR1_HARVEST + if (gIsScc) { + if ((sccRead(gStatus, 1u) & SCC_RR1_RX_OVRN) != 0u) { + uint16_t ovr = gIigsRxOverruns; + + if (ovr < 0xFFFFu) { + gIigsRxOverruns = (uint16_t)(ovr + 1u); + } + sccWrite(gStatus, 0u, SCC_WR0_ERR_RESET); + } + } +#endif } diff --git a/tools/xdftool.py b/tools/xdftool.py index e386c18..6a9ad30 100755 --- a/tools/xdftool.py +++ b/tools/xdftool.py @@ -117,18 +117,40 @@ class Xdf: raise ValueError(f"'{name}' does not fit 8.3") return stem.ljust(8).encode("ascii") + ext.ljust(3).encode("ascii") + @staticmethod + def encode_name_ext(name): + # Human68k EXTENDED names: the stem may run to 18 chars - the first 8 live in the classic + # name field, chars 9-18 in the dir entry's bytes 12-21 (MS-DOS's reserved area). Returns + # (main11, ext10) for matching; ext10 is all-NUL for a plain 8.3 name. RetroNet's blob names + # ('A' + 8 hex = 9 chars) need this - without it a guest-written blob reads as "not in image". + name = name.upper() + stem, _, ext = name.partition(".") + if len(stem) > 18 or len(ext) > 3: + raise ValueError(f"'{name}' does not fit Human68k 18.3") + main = stem[:8].ljust(8).encode("ascii") + ext.ljust(3).encode("ascii") + extended = stem[8:].encode("ascii").ljust(10, b"\x00") + return main, extended + + @staticmethod + def entry_matches(e, main11, ext10): + if bytes(e[0:11]).upper() != main11: + return False + got = bytes(e[12:22]).rstrip(b"\x00 ").upper() + want = ext10.rstrip(b"\x00 ") + return got == want + def find(self, name): # Human68k PRESERVES filename case in the directory entry (unlike # MS-DOS, which upcases), so a program writing "joeylog.txt" leaves it # lowercase on disk. Match case-insensitively or extracting a # guest-written file fails with a confusing "not in image". - want = self.encode_name(name).upper() + main11, ext10 = self.encode_name_ext(name) for i, off, e in self._entries(): if e[0] == FREE_MARKER: break if e[0] == DELETED_MARKER: continue - if bytes(e[0:11]).upper() == want: + if self.entry_matches(e, main11, ext10): return i, off, e return None, None, None @@ -140,7 +162,7 @@ class Xdf: _, _, de = self.find(dirname) if de is None or not (de[11] & ATTR_DIR): return None - want = self.encode_name(leaf).upper() + main11, ext10 = self.encode_name_ext(leaf) cluster = struct.unpack("