From 8e90e9553d224ff9a7d08983272ee4ab8e792d1b Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Sun, 6 Sep 2026 23:34:09 +0000 Subject: [PATCH 01/12] Code review 2026-09-03: system_destroy owns both expansion buses (F-22) Two expansion buses hang off config_t and were freed by two different owners at two different points in teardown: cfg->nubus by the core, in system_destroy, before it calls substrate->teardown; cfg->pci by the machine, partway down tnt_teardown. Both need the same ordering rule -- bus controllers die before the peripherals their cards point at -- but only the NuBus site stated it. The PCI side needs it just as much: the Network Servers' two 53C825As borrow cfg->scsi and machine.scsi2, which the substrate owns, so the cards must die first. They did, but only because pci_root_delete happened to sit above scsi_delete in tnt_teardown. Nothing enforced it, a second PCI family would have had to rediscover it, and system_destroy left a dangling cfg->pci if a substrate forgot. Moves the PCI root's teardown into system_destroy beside NuBus and deletes it from tnt_teardown, so the invariant is stated once, in one place, for both buses. Checked what the call now jumps over in tnt_teardown: the NVRAM carry and the AWACS/GBUS/LCD teardowns. None of awacs.c, gbus.c or lcd.c mentions PCI -- they drop object nodes and free a staging buffer. The carry is the one real coupling, because Grand Central is itself a pci_device_t (tnt_gc_pci_attach), so it now runs after the PCI root is gone; harmless today since gc_pci_ops declares no .teardown and the store lives in tnt_state_t rather than in a PCI allocation, and there is now a comment at the carry saying so. The order between the two buses is not load-bearing and the comment says so: no profile declares both nubus_slots and pci_slots, so a machine has at most one of them. Verified: make headless and make both build with no new warnings; clang-format clean; 49/49 unit-tier integration tests pass against the pinned test-data revision, including tnt-pci-slots, ans-pci-slots, machine-restart, ans-machine-restart, tnt-hd-boot and tnt-pci-voodoo2. The matrix and extended tiers have not been run locally. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/system.c | 17 ++++++++++++++--- src/machines/tnt/tnt.c | 17 +++++++++++------ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/core/system.c b/src/core/system.c index bb335fc4..0a67fc5c 100644 --- a/src/core/system.c +++ b/src/core/system.c @@ -816,9 +816,20 @@ void system_destroy(config_t *config) { // pinned a fresh emulator that still references the rom object. root_uninstall_if(config); - // Tear down NuBus before peripherals so cards (which hold device - // pointers via cfg->via2 etc.) free cleanly first. No-op when nubus - // is NULL (Plus today; future 68000-family machines). + // Tear down the expansion buses before the peripherals, so cards -- + // which hold pointers to devices the substrate owns -- free cleanly + // first. NuBus cards hold cfg->via2 and friends; the Network + // Servers' two 53C825As borrow cfg->scsi and machine.scsi2, so a card + // outliving its bus is a use-after-free either way. Both are no-ops + // when the machine has no such bus. + // + // The order BETWEEN the two is not load-bearing: no profile declares + // both nubus_slots and pci_slots, so a machine has at most one of + // these. Do not read a dependency into it. + if (config->pci) { + pci_root_delete(config->pci); + config->pci = NULL; + } if (config->nubus) { nubus_delete(config->nubus); config->nubus = NULL; diff --git a/src/machines/tnt/tnt.c b/src/machines/tnt/tnt.c index 7d01dae4..0f1b2c34 100644 --- a/src/machines/tnt/tnt.c +++ b/src/machines/tnt/tnt.c @@ -682,6 +682,13 @@ static void tnt_teardown(config_t *cfg) { // Power-cycle (machine.restart): the soldered part comes back with // the machine. New machine (machine.boot): it gets a virgin store, // and the previous machine's goes with the previous machine. + // + // This must read st->gc.nvram before anything tears Grand Central + // down. GC is itself a pci_device_t (tnt_gc_pci_attach), and + // system_destroy now deletes the PCI root before calling us -- + // harmless today because gc_pci_ops declares no .teardown and the + // store lives in st, not in a PCI allocation, but the coupling is + // real the moment that op appears. if (machine_boot_is_restart()) { memcpy(tnt_nvram_carry, st->gc.nvram, TNT_NVRAM_SIZE); tnt_nvram_carry_valid = true; @@ -694,12 +701,10 @@ static void tnt_teardown(config_t *cfg) { tnt_gbus_teardown(cfg); tnt_lcd_teardown(cfg); } - // Deleting the PCI root tears down every seated device, which is what - // frees Control's VRAM and display buffers (its ops->teardown). - if (cfg->pci) { - pci_root_delete(cfg->pci); - cfg->pci = NULL; - } + // The PCI root is NOT deleted here: system_destroy owns both expansion + // buses and tears them down before calling this, which is what frees + // Control's VRAM and display buffers (its ops->teardown) and what puts + // the 53C825As in their graves before the buses they borrow below. if (st && st->scsi96) { scsi_53c96_delete(st->scsi96); st->scsi96 = NULL; From c2ef8711de64cb96a579104c16f34c795367b16c Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:24:15 +0000 Subject: [PATCH 02/12] Code review 2026-09-03: wire the AppleTalk stack on the TNT family (F-23) Seven of the eight Mac families build, checkpoint and tear down the host AppleTalk stack in the same three places. The TNT (7500/8500/9500 and the two Network Servers) did none of the three, though it has the same SCC model behind Grand Central's ESCC apertures. So `appletalk` -- a documented root-level object -- simply was not there on the five newest models, and AppleShare was unavailable on them. Adds the three calls in the same relative stream positions the other families use: appletalk_init right after scc_set_clocks, appletalk_delete immediately before scc_delete (it holds the scc pointer it was given), and appletalk_checkpoint between scc_checkpoint and via_checkpoint. TNT's construction order already matched its save order, so one slot serves both halves of the positional stream. LocalTalk is the only AppleTalk path these machines have in the model: the Grand Central MACE window is a #define and nothing else, so there is no EtherTalk to prefer. Two notes on what this does NOT do. The finding suggested gating the checkpoint-format change on the build id. That is already how checkpoints work -- checkpoint.c rejects any file whose build id differs from the running binary -- so a stream change needs no gating; every rebuild already invalidates prior checkpoints. And this wires the family up rather than proving it. The stack has only ever been exercised against a Mac Plus guest: appletalk-afp, appletalk-afp-e2e and appletalk-ppc all boot plus-v3, and object-toplevel (which asserts the appletalk.* tree exists) is a Plus too. So on seven of the eight families the call sites are uniform and the datapath is unproven -- the more so here, where the ESCC sits behind four DBDMA channels and .MPP runs under the PowerPC's 68k emulator. That retake is logged as proposal-test-fixes.md section 9, and the comment at the init site says so. Also corrects proposal-localtalk-networking.md section 1.1, which claimed the stack was "only wired on the Plus" -- stale before this change, since seven families already wired it. Verified: make headless and make both build, no new warnings; a pm7500 boots with appletalk.enabled true and survives a checkpoint save/load round-trip through the reordered stream; 49/49 unit-tier integration tests pass against pinned test data. The matrix and extended tiers have not been run locally -- and the extended tier is where the three appletalk rows live. Co-Authored-By: Claude Opus 5 (1M context) --- src/machines/tnt/tnt.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/machines/tnt/tnt.c b/src/machines/tnt/tnt.c index 0f1b2c34..862b0c5a 100644 --- a/src/machines/tnt/tnt.c +++ b/src/machines/tnt/tnt.c @@ -32,6 +32,7 @@ #include "dbdma.h" #include "adb.h" +#include "appletalk.h" #include "checkpoint_images.h" #include "debug.h" #include "debug_mac.h" @@ -488,6 +489,16 @@ static int tnt_init(config_t *cfg, checkpoint_t *cp) { cfg->scc = scc_init(NULL, cfg->scheduler, tnt_scc_irq, cfg, cp); scc_set_clocks(cfg->scc, 15667200, 3672000); + // AppleTalk rides the SCC's LocalTalk channel, so it is built as soon as + // the SCC exists -- and, because the checkpoint stream is positional, in + // the same relative place the save writes it (right after scc_checkpoint). + // LocalTalk is the only AppleTalk path these machines have here: the + // Grand Central MACE window is a #define and nothing else, so there is no + // EtherTalk to prefer. NOTE: the stack has only ever been exercised + // against a Mac Plus guest (tests/integration/appletalk-*), so this wires + // the family up rather than proving it -- see proposal-test-fixes.md. + appletalk_init(cfg->scheduler, cfg->scc, cp); + // VIA1: one real 6522 behind the Grand Central decode, byte-wide on // $200 centres. Timer clock: 783.36 kHz is the classic rate and the // starting assumption — the actual TNT VIA input clock is pinned at @@ -737,6 +748,9 @@ static void tnt_teardown(config_t *cfg) { via_delete(cfg->via1); cfg->via1 = NULL; } + // The AppleTalk stack is a client of the SCC's LocalTalk channel, so it + // goes first -- it holds the scc pointer it was given at init. + appletalk_delete(); if (cfg->scc) { scc_delete(cfg->scc); cfg->scc = NULL; @@ -776,6 +790,7 @@ static void tnt_checkpoint_save(config_t *cfg, checkpoint_t *cp) { scheduler_checkpoint(cfg->scheduler, cp); rtc_checkpoint(cfg->rtc, cp); scc_checkpoint(cfg->scc, cp); + appletalk_checkpoint(cp); via_checkpoint(cfg->via1, cp); adb_checkpoint(cfg->adb, cp); av_cuda_checkpoint(st->cuda, cp); From fc230acf0bebb505ffe592c87a793627fa9807a8 Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Mon, 7 Sep 2026 18:47:15 +0000 Subject: [PATCH 03/12] Code review 2026-09-03: finish the NuBus slot-IRQ migration, delete update_ipl (F-24) F-24 reported that tnt_update_ipl is a logging no-op bound into the substrate vtable where the hook is documented NULL-safe and two families already leave it NULL. True, and the same construct existed in pdm.c -- but deleting two no-ops would have left the structure that produced them. What was actually there: `update_ipl` was the last rung of a half-finished migration. nubus.c once had a "non-VIA2 path" that converted a slot number to an IRQ source mask and handed it to a generic per-machine IPL setter; mac030_nubus_slot_irq_via_ipl was the shim preserving that, and the vtable slot existed for it. Families have been peeling off onto direct nubus_slot_irq handlers ever since, and the IIfx was the only one left -- so substrate->update_ipl was dereferenced in exactly one place in the tree (inside the shim) and reached on exactly one machine. That sorted the eight substrates into four groups for one hook: pointer actually followed (IIfx alone, and iifx_update_ipl had no direct callers at all); bound but never followed (mac030/mdu/mcu/av, whose functions are called directly by name); bound to a logging no-op (pdm, tnt); and NULL (plus, lisa). Only the last group was right. So: give the IIfx a direct iifx_nubus_slot_irq -- the shim's slot->source conversion plus the three-line adapter it called, in the file that owns the OSS -- then delete mac030_nubus_slot_irq_via_ipl, iifx_update_ipl, tnt_update_ipl, pdm_update_ipl, the four remaining bindings, and the update_ipl member itself. Every NuBus machine now reaches its controller the same way, in one hop. The four real *functions* are untouched; only the pointers are gone. mdu.c already recorded why the shared shim was a bad idea, and that comment is kept as history rather than deleted: it passed `1 << (slot - 9)` as the machine's IRQ source mask, and on the IIci those bits are already VIA1/RBV/SCC/NMI -- so a card in slot $C asserted the NMI source and the machine took a level-7 autovector forever ("a 24AC beside the live built-in RBV hangs the boot at Welcome"). Slot numbering matches a machine's interrupt-source numbering only by coincidence. The nubus_slot_irq contract in machine_profile.h now says that, and says there is deliberately no shared helper. Also fixes the AV, which had NuBus but bound no nubus_slot_irq at all, so nubus.c's NULL check would have dropped a card's /NMRQ silently. Latent, not live -- both AV boards declare .slots = NULL ("declared but unpopulated") -- but the first AV declaration-ROM card should not have to discover it. Slots C/D/E map to PSC SInt bits 3-5, which is what the guest's PSCVIA2SlotInt reads under mask ~$78, and the PSC aggregates into the VIA2 window's CA1 bit itself, so the umbrella edge is the chip's business as on the MDU's RBV. Two stale comments corrected along the way: machine_profile.h and nubus.c both still said "MDU/OSS -> the chipset's own IRQ controller via update_ipl", true of neither since MDU migrated. Verified: make headless and make both build with no new warnings; 49/49 unit tier plus suite-iifx, iifx-mactest, iifx-marathon, quadra-dual-card and iicx-display-card-24ac -- 54 rows, 0 failures. Those five are the ones that matter: the IIfx rewrite is on the live path a NuBus display card's VBL takes, and suite-iifx matches its goldens bit-exact including iifx-7.5-832x624x8-24ac-finder.png, with iifx-marathon covering the 8-24 GC card. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/machine_profile.h | 18 ++++++++++++------ src/core/peripherals/nubus/nubus.c | 6 ++++-- src/machines/av/av.c | 27 ++++++++++++++++++++++++++- src/machines/mac030/mac030_glue.c | 13 ------------- src/machines/mac030/mac030_glue.h | 6 ------ src/machines/mcu/mcu.c | 1 - src/machines/mdu/mdu.c | 19 +++++++++++-------- src/machines/oss/iifx.c | 23 ++++++++++++++++++----- src/machines/pdm/pdm.c | 9 --------- src/machines/tnt/tnt.c | 8 -------- 10 files changed, 71 insertions(+), 59 deletions(-) diff --git a/src/core/machine_profile.h b/src/core/machine_profile.h index 0c3b866d..ff900205 100644 --- a/src/core/machine_profile.h +++ b/src/core/machine_profile.h @@ -198,7 +198,8 @@ typedef struct media_slot { // Machine lifecycle + host-input vtable. The behavior half of a machine // (proposal §4.4): hw_profile_t is pure descriptor DATA and points at one of -// these. system.c / nubus.c dispatch through it; every hook is NULL-safe. +// these. system.c / nubus.c / pci.c dispatch through it; every hook is +// NULL-safe. // (memory_layout_init and checkpoint_restore are deliberately absent — they // were never dispatched: each init runs its own layout directly and restore // is folded into init.) @@ -214,15 +215,20 @@ typedef struct machine_substrate { void (*teardown)(struct config *cfg); void (*checkpoint_save)(struct config *cfg, checkpoint_t *cp); - void (*update_ipl)(struct config *cfg, int source, bool active); // NuBus IRQ routing void (*trigger_vbl)(struct config *cfg); // Drive NuBus slot `slot` ($9..$E) /NMRQ active/inactive. `umbrella_edge` // is true when this transition flips the "any slot asserted" aggregate. - // Every NuBus machine implements it (GLUE → VIA2 port-A bit + CA1 on the - // umbrella edge; MDU/OSS → the chipset's own IRQ controller via update_ipl); - // keeps nubus.c machine-agnostic — no cfg->via2 poke (proposal §4.4). NULL - // on non-NuBus machines (Plus / Lisa), which never reach it. + // Every NuBus machine implements it, and each converts the slot number to + // its own controller's numbering ITSELF: GLUE → VIA2 port-A bit + CA1 on + // the umbrella edge; MCU → VIA2 PA1-5 + /SLOTIRQ; MDU → the RBV's slot + // register; OSS → OSS source bits; AV → PSC SInt bits 3-5; PDM → BART. + // There is deliberately no shared "convert slot to an IRQ source mask" + // helper: slot numbering matches a machine's interrupt-source numbering + // only by coincidence, and the one that existed put a IIci's slot $C on + // its NMI source (mdu.c). Keeps nubus.c machine-agnostic — no cfg->via2 + // poke (proposal §4.4). NULL on non-NuBus machines (Plus / Lisa), which + // never reach it. void (*nubus_slot_irq)(struct config *cfg, int slot, bool active, bool umbrella_edge); // Drive PCI slot `slot`'s strapped INTA-D line active/inactive. The diff --git a/src/core/peripherals/nubus/nubus.c b/src/core/peripherals/nubus/nubus.c index ef894422..7956df6d 100644 --- a/src/core/peripherals/nubus/nubus.c +++ b/src/core/peripherals/nubus/nubus.c @@ -546,8 +546,10 @@ void nubus_reset(nubus_bus_t *bus) { // Drive a slot's /NMRQ line through the machine substrate (proposal §4.4): the // bus owns the slot-IRQ aggregate mask and the umbrella transition, the chipset -// owns HOW the line reaches the CPU (GLUE → VIA2; MDU/OSS → its own IRQ -// controller). nubus.c stays machine-agnostic — no cfg->via2 here. +// owns HOW the line reaches the CPU (GLUE/MCU → VIA2; MDU → the RBV; OSS → +// the OSS; AV → the PSC; PDM → BART), including converting the slot number +// into whatever its controller numbers sources by. nubus.c stays +// machine-agnostic — no cfg->via2 here. static void nubus_route_slot_irq(config_t *cfg, int slot, bool active, bool umbrella_edge) { if (cfg && cfg->machine && cfg->machine->substrate->nubus_slot_irq) cfg->machine->substrate->nubus_slot_irq(cfg, slot, active, umbrella_edge); diff --git a/src/machines/av/av.c b/src/machines/av/av.c index 223edabd..5e349f1e 100644 --- a/src/machines/av/av.c +++ b/src/machines/av/av.c @@ -272,6 +272,31 @@ static void av_via1_irq(void *context, bool active) { av_update_ipl((config_t *)context, AV_IRQ_VIA1, active); } +// substrate.nubus_slot_irq — the PSC aggregates NuBus slot interrupts itself, +// so the bus drives one SInt source per slot and the PSC raises the VIA2 +// window's CA1 bit while any of them is asserted (psc.c psc_update_slot_bit). +// The umbrella edge is therefore the chip's business, not ours, exactly as on +// the MDU's RBV. +// +// Slots C/D/E map to SInt bits 3/4/5 (psc.h; the guest's PSCVIA2SlotInt reads +// PSCVIA2SInt under mask ~$78 -- slots C/D/E plus on-board VBL on bit 6 -- +// and inverts, the register reading active-LOW). So bit = slot - $9, and +// av_psc_slot_source owns the inversion. +// +// No AV board declares a slot table yet (.slots = NULL on both, "declared but +// unpopulated"), so nothing reaches this today. It exists so the first AV +// declaration-ROM card does not have to discover that its /NMRQ went nowhere. +static void av_nubus_slot_irq(config_t *cfg, int slot, bool active, bool umbrella_edge) { + (void)umbrella_edge; // the PSC aggregates internally + av_state_t *st = (av_state_t *)cfg->machine_context; + if (!st || !st->psc) + return; + int bit = slot - 0x9; + if (bit < 3 || bit > 5) // only C/D/E exist on this family + return; + av_psc_slot_source(st->psc, bit, active); +} + // ============================================================ // SCSI: the Curio's 53C96 at island $18000 ($10 register stride) // ============================================================ @@ -953,7 +978,7 @@ const machine_substrate_t av_substrate = { .reset = av_reset, .teardown = av_teardown, .checkpoint_save = av_checkpoint_save, - .update_ipl = av_update_ipl, // VIA1→1, VIA2→2, L3-L6→3-6, NMI→7 + .nubus_slot_irq = av_nubus_slot_irq, // slots C/D/E → PSC SInt bits 3-5 .trigger_vbl = av_trigger_vbl, .fd_insert = mac_fd_insert, .fd_present = mac_fd_present, diff --git a/src/machines/mac030/mac030_glue.c b/src/machines/mac030/mac030_glue.c index e68713b5..6387184d 100644 --- a/src/machines/mac030/mac030_glue.c +++ b/src/machines/mac030/mac030_glue.c @@ -271,18 +271,6 @@ void mac030_glue_nubus_slot_irq(config_t *cfg, int slot, bool active, bool umbre via_input_c(cfg->via2, /*CA1*/ 0, /*pin*/ 0, active ? 0 : 1); } -// substrate.nubus_slot_irq for chipsets whose own controller aggregates the -// slots (MDU's RBV, OSS): route the slot source through the substrate's own -// update_ipl, exactly as the former nubus.c non-VIA2 path did. -void mac030_nubus_slot_irq_via_ipl(config_t *cfg, int slot, bool active, bool umbrella_edge) { - (void)umbrella_edge; // the controller aggregates internally - int source = slot - 0x9; - if (source < 0 || source > 5) - return; - if (cfg->machine->substrate->update_ipl) - cfg->machine->substrate->update_ipl(cfg, 1 << source, active); -} - // Family-shared teardown delete-chain. Order matches the (identical) // per-machine teardowns; NuBus cards are already gone (system_destroy calls // nubus_delete before machine teardown — §6.2 ownership invariant). @@ -436,7 +424,6 @@ const machine_substrate_t glue_substrate = { .reset = glue_reset, .teardown = glue_teardown, .checkpoint_save = glue_checkpoint_save, - .update_ipl = mac030_glue_update_ipl, .trigger_vbl = glue_trigger_vbl, .nubus_slot_irq = mac030_glue_nubus_slot_irq, .fd_insert = mac_fd_insert, diff --git a/src/machines/mac030/mac030_glue.h b/src/machines/mac030/mac030_glue.h index 1ad6af05..95da01e2 100644 --- a/src/machines/mac030/mac030_glue.h +++ b/src/machines/mac030/mac030_glue.h @@ -222,12 +222,6 @@ void mac030_glue_update_ipl(config_t *cfg, int source, bool active); // pulses CA1. (se30/iicx/iix.) void mac030_glue_nubus_slot_irq(config_t *cfg, int slot, bool active, bool umbrella_edge); -// substrate.nubus_slot_irq for chipsets whose own IRQ controller aggregates the -// slots (MDU's RBV, OSS): route the slot source through the substrate's own -// update_ipl. umbrella_edge is irrelevant (the controller aggregates -// internally). (iici/iisi/iifx.) -void mac030_nubus_slot_irq_via_ipl(config_t *cfg, int slot, bool active, bool umbrella_edge); - // Family-shared teardown delete-chain: scheduler_stop → mmu → floppy → asc → // adb → scsi → via2 → via1 → scc → rtc → scheduler → cpu → mem_map → debugger. // The machine-owned devices (which live in its private state, not config_t) diff --git a/src/machines/mcu/mcu.c b/src/machines/mcu/mcu.c index 9073fb39..a2f9973a 100644 --- a/src/machines/mcu/mcu.c +++ b/src/machines/mcu/mcu.c @@ -909,7 +909,6 @@ const machine_substrate_t mcu_substrate = { .reset = mcu_reset, .teardown = mcu_teardown, .checkpoint_save = mcu_checkpoint_save, - .update_ipl = mac030_glue_update_ipl, // VIA1→1, VIA2→2, SCC→4, NMI→7 (ref §13) .trigger_vbl = mcu_trigger_vbl, .nubus_slot_irq = mcu_nubus_slot_irq, // slots → VIA2 PA1-PA5 + /SLOTIRQ aggregate .fd_insert = mac_fd_insert, diff --git a/src/machines/mdu/mdu.c b/src/machines/mdu/mdu.c index da13ca79..a2aacab0 100644 --- a/src/machines/mdu/mdu.c +++ b/src/machines/mdu/mdu.c @@ -209,14 +209,18 @@ static void mdu_checkpoint_save(config_t *cfg, checkpoint_t *cp) { // substrate.nubus_slot_irq — the RBV aggregates NuBus slot interrupts itself // (RvSInt & RvSEnb -> RvAnySlot -> the chip's combined interrupt -> IPL 2), so a -// slot source has to go to the chip rather than straight to update_ipl. +// slot source has to go to the chip, not to a generic IPL setter. // -// The shared mac030_nubus_slot_irq_via_ipl passes `1 << (slot - 9)` as the -// machine's IRQ SOURCE mask, and on this family every one of those bits is -// already spoken for: IICI_IRQ_VIA1/RBV/SCC/NMI are 1<<0 .. 1<<3. So a card in -// slot $C asserted the NMI source and the machine took a level-7 autovector -// every few instructions forever — which is what "a 24AC beside the live -// built-in RBV hangs the boot at Welcome" actually was (ledger §8). +// Worth keeping as history, because it is why the generic path no longer +// exists. nubus.c once dispatched families like this one through a shared +// shim that passed `1 << (slot - 9)` as the machine's IRQ SOURCE mask -- but +// on this family every one of those bits is already spoken for: +// IICI_IRQ_VIA1/RBV/SCC/NMI are 1<<0 .. 1<<3. So a card in slot $C asserted +// the NMI source and the machine took a level-7 autovector every few +// instructions forever -- which is what "a 24AC beside the live built-in RBV +// hangs the boot at Welcome" actually was (ledger §8). Slot numbering only +// coincidentally matches a machine's interrupt-source numbering; every family +// now converts it itself. // // RvSInt numbering is logical: 0 is the built-in video (RvIRQ0, bit 6) and // 1..6 are RvIRQ1..6, so NuBus $9..$E map to 1..6. @@ -245,7 +249,6 @@ const machine_substrate_t mdu_substrate = { .reset = mdu_reset, .teardown = mdu_teardown, .checkpoint_save = mdu_checkpoint_save, - .update_ipl = mac030_glue_update_ipl, .trigger_vbl = mdu_trigger_vbl, .nubus_slot_irq = mdu_nubus_slot_irq, // straight to the RBV's slot-interrupt register .fd_insert = mac_fd_insert, diff --git a/src/machines/oss/iifx.c b/src/machines/oss/iifx.c index dca18d4e..c70626c7 100644 --- a/src/machines/oss/iifx.c +++ b/src/machines/oss/iifx.c @@ -294,7 +294,7 @@ static void iifx_teardown(config_t *cfg); static void iifx_reset(config_t *cfg); static void iifx_checkpoint_save(config_t *cfg, checkpoint_t *cp); static void iifx_memory_layout_init(config_t *cfg); -static void iifx_update_ipl(config_t *cfg, int source, bool active); +static void iifx_nubus_slot_irq(config_t *cfg, int slot, bool active, bool umbrella_edge); static void iifx_trigger_vbl(config_t *cfg); // Fills one page-table entry with a direct host mapping. @@ -1361,11 +1361,25 @@ static void iifx_scsi_irq(void *context, bool irq, bool drq) { } // Handles external machine IRQ requests such as NuBus slots. -static void iifx_update_ipl(config_t *cfg, int source, bool active) { +// substrate.nubus_slot_irq — the OSS is its own interrupt controller and +// aggregates the slots internally, so the umbrella edge is the chip's +// business (the MDU/RBV shape). Slots $9..$E are OSS source bits 0..5. +// +// This used to go the long way round: nubus.c dispatched to the shared +// mac030_nubus_slot_irq_via_ipl, which converted the slot to a source mask +// and called back out through substrate.update_ipl into a three-line +// adapter here. That indirection was the last survivor of nubus.c's old +// "non-VIA2 path"; the IIfx was the only machine still using it, so both +// hops and the vtable slot behind them are gone. +static void iifx_nubus_slot_irq(config_t *cfg, int slot, bool active, bool umbrella_edge) { + (void)umbrella_edge; // the OSS aggregates internally + int source = slot - 0x9; + if (source < 0 || source > 5) + return; iifx_state_t *st = iifx_state(cfg); if (!st || !st->oss) return; - oss_set_source_mask(st->oss, (uint16_t)source, active); + oss_set_source_mask(st->oss, (uint16_t)(1u << source), active); } // Pulses the IIfx 60 Hz sources. @@ -1735,9 +1749,8 @@ static const machine_substrate_t iifx_substrate = { .reset = iifx_reset, .teardown = iifx_teardown, .checkpoint_save = iifx_checkpoint_save, - .update_ipl = iifx_update_ipl, .trigger_vbl = iifx_trigger_vbl, - .nubus_slot_irq = mac030_nubus_slot_irq_via_ipl, + .nubus_slot_irq = iifx_nubus_slot_irq, // slots $9-$E → OSS source bits 0-5 .fd_insert = mac_fd_insert, .fd_present = mac_fd_present, .input_key = mac_input_key, diff --git a/src/machines/pdm/pdm.c b/src/machines/pdm/pdm.c index 1dfcaba9..ca0f8a07 100644 --- a/src/machines/pdm/pdm.c +++ b/src/machines/pdm/pdm.c @@ -556,14 +556,6 @@ static void pdm_trigger_vbl(config_t *cfg) { nubus_tick_vbl(cfg->nubus); } -// Chipset IRQ spine. Nothing on this family routes through it: the NuBus -// slots have their own hook below, and every on-board source is already an -// AMIC ICR bit (pdm_amic_set_source). -static void pdm_update_ipl(config_t *cfg, int source, bool active) { - (void)cfg; - LOG(1, "update_ipl source=%d active=%d (PDM sources drive the AMIC ICR directly)", source, active); -} - // A NuBus card's /NMRQ. The umbrella edge is AMIC's own business (the // pseudo-VIA2 "any slot" bit is recomputed from the slot levels on every // read), so the bus controller's edge hint is not needed here. @@ -597,7 +589,6 @@ const machine_substrate_t pdm_substrate = { .reset = pdm_reset, .teardown = pdm_teardown, .checkpoint_save = pdm_checkpoint_save, - .update_ipl = pdm_update_ipl, .nubus_slot_irq = pdm_nubus_slot_irq, .trigger_vbl = pdm_trigger_vbl, .fd_insert = pdm_fd_insert, diff --git a/src/machines/tnt/tnt.c b/src/machines/tnt/tnt.c index 862b0c5a..f4ba9c9d 100644 --- a/src/machines/tnt/tnt.c +++ b/src/machines/tnt/tnt.c @@ -895,13 +895,6 @@ static void tnt_pci_slot_irq(config_t *cfg, int slot, bool active) { tnt_gc_set_source(cfg, d->int_line, active); } -// Chipset IRQ spine. Nothing routes through it: every on-board source is -// a Grand Central interrupt number (tnt_gc_set_source). -static void tnt_update_ipl(config_t *cfg, int source, bool active) { - (void)cfg; - LOG(1, "update_ipl source=%d active=%d (TNT sources drive Grand Central directly)", source, active); -} - // Floppy: the one internal SuperDrive behind SWIM3 (swim3.c). Drive 1 is // the only bay the family has — no external port — so slot 1 refuses // whatever the caller asks. @@ -931,7 +924,6 @@ const machine_substrate_t tnt_substrate = { .reset = tnt_reset, .teardown = tnt_teardown, .checkpoint_save = tnt_checkpoint_save, - .update_ipl = tnt_update_ipl, .pci_slot_irq = tnt_pci_slot_irq, .trigger_vbl = tnt_trigger_vbl, .fd_insert = tnt_fd_insert, From 04c81e7cb41bc9dde31cbc5a7d6254f0682cf08d Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:14:49 +0000 Subject: [PATCH 04/12] Code review 2026-09-03: name memory regions by what they are; collapse the GLUE layout (F-25) F-25 reported that iicx_memory_layout_init hardcodes "IIcx I/O", and the IIx shares that function, so a IIx memory-map dump says IIcx. True. It then listed "SE/30 I/O", "IIci I/O" and "IIsi I/O" as the same problem -- "four hand-written strings for one concept". They are not: those three name one machine each and are never wrong. Of the eight " I/O" strings in the tree, exactly one was a defect, and the families that DO share a layout across models (MCU, PDM, AV) had named their region after the chipset or family, which stays true. But the machine name does not belong in any of them. memory_map_print lists the regions of ONE machine's map, so naming the machine in a row repeats what the map already says -- and that redundancy is what made the IIx bug possible. Every region name that instead says what the thing IS ("Hammerhead", "Grand Central", "BART registers", "DAFB regs", "CIVIC", "ROM aperture", "Machine ID", "Phase Read") reads correctly on every machine. TNT already names all four of its regions that way and is the only family that never had this class of bug. So: strip the machine from all eight, plus "IIfx ROM switch" -> "ROM switch". Nine string literals; no descriptor field, no lookup, no plumbing. The IIx defect stops being fixed and starts being impossible -- "I/O" is true on every machine, so no shared layout function can mislabel. Underneath the nit was the real finding. The SE/30, IIcx and IIx are one motherboard design with one GLUE, and se30_memory_layout_init and iicx_memory_layout_init were the same function twice: identical RAM SIMM-wrap logic (same 1/4/16 MB standard_bank test), identical ROM mirror loop, identical I/O registration, and four address constants duplicated under two names with byte-identical values (SE30_ROM_START == IICX_ROM_START == $40000000, and so on). mac030_glue_memory_layout(cfg, desc) now does RAM, ROM and the I/O dispatcher once for all three, taking the ROM window from the board descriptor -- which already carried it for mac030_build_mmu, so the machines were ignoring data they already had in favour of their own #defines of the same numbers. The memory_layout hook becomes memory_layout_tail and keeps only what genuinely differs: the SE/30 maps its on-board framebuffer's VRAM/VROM, the IIcx and IIx fill page entries for NuBus cards' host-backed regions. Six now-unused constants deleted. Verified: make headless and make both build; the new function adds no warnings (it also drops the pre-existing `(int)p < g_page_count` sign-compare idiom, since g_page_count is uint32_t -- the copies elsewhere are left alone). All 58 C unit suites pass. 55 integration rows, 0 failures: the unit tier plus suite-se30, suite-iicx, suite-iix, se30-mactest, iicx-mactest and iix-aux3-boot. The two MacTest rows are the ones that matter -- MacTest is what exercises the ROM's ram_address_test, which is precisely what the shared SIMM-wrap code has to get right. Co-Authored-By: Claude Opus 5 (1M context) --- src/machines/av/av.c | 2 +- src/machines/glue/iicx.c | 32 +++------------ src/machines/glue/iicx_internal.h | 8 ++-- src/machines/glue/iix.c | 2 +- src/machines/glue/se30.c | 65 +++---------------------------- src/machines/mac030/mac030_glue.c | 56 +++++++++++++++++++++++++- src/machines/mac030/mac030_glue.h | 17 +++++++- src/machines/mcu/mcu.c | 2 +- src/machines/mdu/iici.c | 2 +- src/machines/mdu/iisi.c | 2 +- src/machines/oss/iifx.c | 5 +-- src/machines/pdm/pdm.c | 2 +- 12 files changed, 93 insertions(+), 102 deletions(-) diff --git a/src/machines/av/av.c b/src/machines/av/av.c index 5e349f1e..e9ce4575 100644 --- a/src/machines/av/av.c +++ b/src/machines/av/av.c @@ -574,7 +574,7 @@ static void av_memory_layout(config_t *cfg) { // I/O island: the serialized window at $50F00000 plus its non-serialized // alias at $50F40000, folded by the $3FFFF mirror mask. mac030_io_fill_interface(&st->io_interface); - memory_map_add(cfg->mem_map, 0x50F00000u, 0x00080000u, "AV I/O", &st->io_interface, &st->io); + memory_map_add(cfg->mem_map, 0x50F00000u, 0x00080000u, "I/O", &st->io_interface, &st->io); // CPU-ID register page at $5FFFF000 (the register itself is $5FFFFFFC). st->cpuid_interface.read_uint8 = av_cpuid_read8; diff --git a/src/machines/glue/iicx.c b/src/machines/glue/iicx.c index 2ac7c954..08388830 100644 --- a/src/machines/glue/iicx.c +++ b/src/machines/glue/iicx.c @@ -97,33 +97,13 @@ void iicx_set_rom_overlay(config_t *cfg, bool overlay) { // Memory layout // ============================================================ -void iicx_memory_layout_init(config_t *cfg) { +// The IIcx/IIx share of the memory layout: NuBus cards' host-backed regions. +// RAM, the ROM window and the I/O dispatcher are the family's +// (mac030_glue_memory_layout); these two machines differ from the SE/30 only +// in having card slots instead of a framebuffer on the board. +void iicx_memory_layout_tail(config_t *cfg) { iicx_state_t *st = iicx_state(cfg); - uint32_t ram_size = cfg->ram_size; - uint32_t rom_size = cfg->machine->rom_size; - uint8_t *ram_base = ram_native_pointer(cfg->mem_map, 0); - uint8_t *rom_data = ram_native_pointer(cfg->mem_map, ram_size); - - uint32_t ram_pages = ram_size >> PAGE_SHIFT; - bool standard_bank = (ram_size == 1 * 1024 * 1024 || ram_size == 4 * 1024 * 1024 || ram_size == 16 * 1024 * 1024); - uint32_t map_end_page = standard_bank ? ram_pages : (ram_pages * 2); - for (uint32_t p = 0; p < map_end_page && (int)p < g_page_count; p++) - mac030_fill_page(p, ram_base + ((p % ram_pages) << PAGE_SHIFT), true); - - uint32_t rom_pages = rom_size >> PAGE_SHIFT; - uint32_t rom_start_page = IICX_ROM_START >> PAGE_SHIFT; - uint32_t rom_end_page = IICX_ROM_END >> PAGE_SHIFT; - if (rom_pages > 0) { - for (uint32_t p = rom_start_page; p < rom_end_page && (int)p < g_page_count; p++) { - uint32_t offset_in_rom = (p - rom_start_page) % rom_pages; - mac030_fill_page(p, rom_data + (offset_in_rom << PAGE_SHIFT), false); - } - } - - mac030_io_fill_interface(&st->io_interface); - memory_map_add(cfg->mem_map, IICX_IO_BASE, IICX_IO_SIZE, "IIcx I/O", &st->io_interface, &st->glue_io); - // Populate page-table entries for any host-backed slot regions // registered by NuBus cards (matches the SE/30 pattern of explicit // se30_fill_page calls). Without this, slot-space reads fall to the @@ -257,7 +237,7 @@ static const mac030_glue_board_t iicx_board = { .via2_output = iicx_via2_output, .via2_shift_out = iicx_via2_shift_out, .setup_id = iicx_setup_id, - .memory_layout = iicx_memory_layout_init, + .memory_layout_tail = iicx_memory_layout_tail, }; // ============================================================ diff --git a/src/machines/glue/iicx_internal.h b/src/machines/glue/iicx_internal.h index 5e243f8d..4cf82a7d 100644 --- a/src/machines/glue/iicx_internal.h +++ b/src/machines/glue/iicx_internal.h @@ -45,8 +45,9 @@ void iicx_via1_shift_out(void *context, uint8_t byte); // SCC IRQ (identical). -// Memory layout (RAM/ROM aliasing + I/O dispatcher registration). -void iicx_memory_layout_init(struct config *cfg); +// The IIcx/IIx memory-layout tail (NuBus card host regions + ROM overlay); +// RAM/ROM/IO are the family's, in mac030_glue_memory_layout(). +void iicx_memory_layout_tail(struct config *cfg); // IRQ source bit assignments. #define IICX_IRQ_VIA1 (1 << 0) @@ -58,8 +59,5 @@ void iicx_memory_layout_init(struct config *cfg); // Address-space constants. #define IICX_ROM_START 0x40000000UL -#define IICX_ROM_END 0x50000000UL -#define IICX_IO_BASE 0x50000000UL -#define IICX_IO_SIZE 0x10000000UL #endif // IICX_INTERNAL_H diff --git a/src/machines/glue/iix.c b/src/machines/glue/iix.c index be4577cf..0a43e768 100644 --- a/src/machines/glue/iix.c +++ b/src/machines/glue/iix.c @@ -135,7 +135,7 @@ static const mac030_glue_board_t iix_board = { .via2_output = iix_via2_output, .via2_shift_out = iix_via2_shift_out, .setup_id = iix_setup_id, - .memory_layout = iicx_memory_layout_init, + .memory_layout_tail = iicx_memory_layout_tail, }; // ============================================================ diff --git a/src/machines/glue/se30.c b/src/machines/glue/se30.c index bdcd9fd9..f3d099bf 100644 --- a/src/machines/glue/se30.c +++ b/src/machines/glue/se30.c @@ -58,11 +58,8 @@ LOG_USE_CATEGORY_NAME("board"); // SE/30 ROM region: 256 KB mirrored across 256 MB. RAM occupies the // first 1 GiB (so RAM_END == ROM_START). #define SE30_ROM_START 0x40000000UL -#define SE30_ROM_END 0x50000000UL // SE/30 I/O region: 256 MB, mirrored every $20000 -#define SE30_IO_BASE 0x50000000UL -#define SE30_IO_SIZE 0x10000000UL // (I/O window offsets + the dispatcher are shared with IIcx/IIx — see // mac030_glue_io.c.) @@ -155,64 +152,12 @@ static void se30_set_rom_overlay(config_t *cfg, bool overlay) { // VRAM: $FE000000-$FE00FFFF (64 KB, writable) // VROM: $FEFFE000-$FEFFFFFF (8 KB, read-only, synthesised declaration ROM) // ROM overlay at $00000000 is active on reset. -static void se30_memory_layout_init(config_t *cfg) { +// The SE/30's share of the memory layout: its built-in video. RAM, the ROM +// window and the I/O dispatcher are the family's (mac030_glue_memory_layout); +// this is the part only a machine with a framebuffer on the board has. +static void se30_memory_layout_tail(config_t *cfg) { se30_state_t *se30 = se30_state(cfg); - uint32_t ram_size = cfg->ram_size; - uint32_t rom_size = cfg->machine->rom_size; - uint8_t *ram_base = ram_native_pointer(cfg->mem_map, 0); - // ROM data is stored immediately after RAM in the flat buffer - uint8_t *rom_data = ram_native_pointer(cfg->mem_map, ram_size); - - // --- RAM pages: $00000000 - ram_size (writable, with SIMM aliasing) --- - // - // Physical RAM is mapped directly at $0. An additional mirror of the full - // RAM image is placed immediately above, at ram_size .. 2*ram_size-1. - // This emulates the real SE/30 SIMM address-line wrapping: SIMMs ignore - // address bits above their capacity, so the byte at is the same - // physical cell as the byte at 0. The ROM's ram_address_test writes to - // the top-of-RAM address and checks whether the pattern appears at a lower - // alias; without this mirror, the write falls into unmapped space and the - // test fails with a spurious address-bus error. - // - // The ROM's address test table uses BMI rows (alias=$FFFFFFFF) for 1, 4, - // and 16 MB — these expect NO aliasing at the boundary. All other sizes - // (2, 5, 8, 32, 64 … MB) use non-BMI rows that expect the top-of-RAM - // write to alias back to a lower address. We map one extra mirror for - // non-BMI sizes so the alias check succeeds. - uint32_t ram_pages = ram_size >> PAGE_SHIFT; - - // Determine whether SIMM aliasing is needed. The ROM's ram_address_test - // table has two kinds of entries: "BMI" rows (alias = $FFFFFFFF) that - // expect NO aliasing, and "non-BMI" rows (alias = an address) that - // expect the top-of-RAM write to alias back. BMI rows correspond to - // the GLUE's standard bank sizes (1, 4, 16, 64 MB); non-BMI rows cover - // intermediate totals (2, 5, 8, 32 … MB). We only need a mirror for - // sizes whose top-of-RAM entry is non-BMI. - // BMI rows in the ROM table: 1 MB ($100000), 4 MB ($400000), 16 MB ($1000000). - // All other sizes (including 64 MB) use non-BMI rows that expect aliasing. - bool standard_bank = (ram_size == 1 * 1024 * 1024 || ram_size == 4 * 1024 * 1024 || ram_size == 16 * 1024 * 1024); - uint32_t map_end_page = standard_bank ? ram_pages : (ram_pages * 2); - - for (uint32_t p = 0; p < map_end_page && (int)p < g_page_count; p++) - mac030_fill_page(p, ram_base + ((p % ram_pages) << PAGE_SHIFT), true); - - // --- ROM pages: $40000000 - $4FFFFFFF (256 KB mirrored, read-only) --- - uint32_t rom_pages = rom_size >> PAGE_SHIFT; - uint32_t rom_start_page = SE30_ROM_START >> PAGE_SHIFT; - uint32_t rom_end_page = SE30_ROM_END >> PAGE_SHIFT; - - if (rom_pages > 0) { - for (uint32_t p = rom_start_page; p < rom_end_page && (int)p < g_page_count; p++) { - uint32_t offset_in_rom = (p - rom_start_page) % rom_pages; - mac030_fill_page(p, rom_data + (offset_in_rom << PAGE_SHIFT), false); - } - } - - // --- I/O dispatcher: $50000000 - $5FFFFFFF --- - mac030_io_fill_interface(&se30->io_interface); - memory_map_add(cfg->mem_map, SE30_IO_BASE, SE30_IO_SIZE, "SE/30 I/O", &se30->io_interface, &se30->glue_io); - // --- VRAM: $FEE00000 - $FEE0FFFF (64 KB writable) --- // Mirror the 64 KB across the 1 MB decode window $FEE00000-$FEEFFFFF if (se30->vram) { @@ -462,7 +407,7 @@ static const mac030_glue_board_t se30_board = { .via2_output = se30_via2_output, .via2_shift_out = se30_via2_shift_out, .setup_id = se30_setup_id, - .memory_layout = se30_memory_layout_init, + .memory_layout_tail = se30_memory_layout_tail, .pre_devices = se30_pre_devices, .post_nubus = se30_post_nubus, .ckpt_restore_extra = se30_ckpt_restore_extra, diff --git a/src/machines/mac030/mac030_glue.c b/src/machines/mac030/mac030_glue.c index 6387184d..c2bffbc7 100644 --- a/src/machines/mac030/mac030_glue.c +++ b/src/machines/mac030/mac030_glue.c @@ -75,6 +75,58 @@ struct mmu_state *mac030_build_mmu(config_t *cfg, uint32_t rom_base, uint32_t ro return mmu; } +// The GLUE family's memory layout — RAM, ROM and the I/O dispatcher. +// +// The SE/30, IIcx and IIx are one motherboard design with one GLUE, so this +// was three copies of the same function differing only in the constant NAMES +// (SE30_ROM_START vs IICX_ROM_START, both $40000000) and in a short tail. +// The tail is what actually differs and stays per-board (memory_layout_tail): +// the SE/30 maps its built-in video's VRAM/VROM; the IIcx and IIx fill page +// entries for NuBus cards' host-backed regions. Both then arm the overlay. +// +// The ROM window comes from the board descriptor, which already carried it +// for mac030_build_mmu — so the duplicated per-machine #defines are gone. +// I/O is the $10000000 window immediately above the ROM window on all three. +void mac030_glue_memory_layout(config_t *cfg, const mac030_board_desc_t *desc) { + mac030_glue_state_t *st = (mac030_glue_state_t *)cfg->machine_context; + + uint32_t ram_size = cfg->ram_size; + uint32_t rom_size = cfg->machine->rom_size; + uint8_t *ram_base = ram_native_pointer(cfg->mem_map, 0); + uint8_t *rom_data = ram_native_pointer(cfg->mem_map, ram_size); // ROM follows RAM in the flat buffer + + // --- RAM, with the SIMM address-line wrap the ROM's test depends on --- + // + // SIMMs ignore address bits above their capacity, so the byte at + // is the same cell as the byte at 0. The ROM's + // ram_address_test writes to the top-of-RAM address and checks whether the + // pattern appears at a lower alias; without the mirror the write falls + // into unmapped space and the test reports a spurious address-bus error. + // + // Its table has two kinds of row: "BMI" rows (alias = $FFFFFFFF) that + // expect NO aliasing, and non-BMI rows that expect the wrap. BMI rows are + // 1, 4 and 16 MB; every other total (2, 5, 8, 32, 64 …) expects the wrap, + // so those get one extra mirror. + uint32_t ram_pages = ram_size >> PAGE_SHIFT; + bool standard_bank = (ram_size == 1 * 1024 * 1024 || ram_size == 4 * 1024 * 1024 || ram_size == 16 * 1024 * 1024); + uint32_t map_end_page = standard_bank ? ram_pages : (ram_pages * 2); + for (uint32_t p = 0; p < map_end_page && p < g_page_count; p++) + mac030_fill_page(p, ram_base + ((p % ram_pages) << PAGE_SHIFT), true); + + // --- ROM, mirrored across the board's window (read-only) --- + uint32_t rom_pages = rom_size >> PAGE_SHIFT; + uint32_t rom_start_page = desc->rom_base >> PAGE_SHIFT; + uint32_t rom_end_page = desc->rom_end >> PAGE_SHIFT; + if (rom_pages > 0) { + for (uint32_t p = rom_start_page; p < rom_end_page && p < g_page_count; p++) + mac030_fill_page(p, rom_data + (((p - rom_start_page) % rom_pages) << PAGE_SHIFT), false); + } + + // --- I/O dispatcher, the window directly above the ROM window --- + mac030_io_fill_interface(&st->io_interface); + memory_map_add(cfg->mem_map, desc->rom_end, MAC030_GLUE_IO_SIZE, "I/O", &st->io_interface, &st->glue_io); +} + // Finish init: debugger, scheduler start, cold-boot IRQ/IPL reset. void mac030_glue_finish(config_t *cfg, checkpoint_t *cp) { cfg->debugger = debug_init(); @@ -136,7 +188,9 @@ int mac030_glue_init(config_t *cfg, checkpoint_t *cp, const mac030_glue_board_t board->post_nubus(cfg); memory_set_bus_error_range(cfg->mem_map, board->desc->bus_err_lo, board->desc->bus_err_hi); - board->memory_layout(cfg); + mac030_glue_memory_layout(cfg, board->desc); + if (board->memory_layout_tail) + board->memory_layout_tail(cfg); if (cp) { if (board->ckpt_restore_extra) diff --git a/src/machines/mac030/mac030_glue.h b/src/machines/mac030/mac030_glue.h index 95da01e2..51c25abc 100644 --- a/src/machines/mac030/mac030_glue.h +++ b/src/machines/mac030/mac030_glue.h @@ -146,6 +146,16 @@ typedef struct mac030_board_desc { asc_mix_t asc_mix; // speaker fold of the ASC stereo pair (SE/30 sums; IIx/IIcx take A) } mac030_board_desc_t; +// The I/O window sits directly above the ROM window and is 256 MB on every +// GLUE machine ($50000000-$5FFFFFFF), so its base is desc->rom_end. +#define MAC030_GLUE_IO_SIZE 0x10000000UL + +// The GLUE family's shared memory layout: RAM (with the SIMM address wrap the +// ROM's ram_address_test needs), the ROM window from desc->rom_base/rom_end, +// and the I/O dispatcher. mac030_glue_init calls this, then the board's +// memory_layout_tail. One motherboard design, one function. +void mac030_glue_memory_layout(config_t *cfg, const mac030_board_desc_t *desc); + // Create the 68030 PMMU over a board's ROM window, make it the global MMU and // attach it to the CPU. Returns the MMU; the caller sets any TT registers. struct mmu_state *mac030_build_mmu(config_t *cfg, uint32_t rom_base, uint32_t rom_end); @@ -165,7 +175,12 @@ typedef struct mac030_glue_board { void (*setup_id)(config_t *cfg); // machine-ID strap + VIA2 idle lines - void (*memory_layout)(config_t *cfg); // RAM/ROM/IO page-table setup + overlay + // Per-board remainder of the memory layout, run right after the shared + // mac030_glue_memory_layout() has done RAM, ROM and the I/O dispatcher: + // the SE/30 maps its built-in video's VRAM/VROM, the IIcx and IIx fill + // page entries for NuBus cards' host-backed regions, and both then arm + // the ROM overlay. Optional, like the other tail hooks. + void (*memory_layout_tail)(config_t *cfg); void (*pre_devices)(config_t *cfg); // optional: before device construction (SE/30 VBL event type) void (*post_nubus)(config_t *cfg); // optional: after nubus_init (SE/30 VRAM/VROM wiring) diff --git a/src/machines/mcu/mcu.c b/src/machines/mcu/mcu.c index a2f9973a..2d947ed2 100644 --- a/src/machines/mcu/mcu.c +++ b/src/machines/mcu/mcu.c @@ -583,7 +583,7 @@ static void mcu_memory_layout_init(config_t *cfg) { // through $53FFFFFF, current RE through $50FFFFFF — we register the // Apple-documented extent and let the mirror mask fold accesses; ref §6). mac030_io_fill_interface(&st->io_interface); - memory_map_add(cfg->mem_map, 0x50000000u, 0x04000000u, "MCU I/O", &st->io_interface, &st->io); + memory_map_add(cfg->mem_map, 0x50000000u, 0x04000000u, "I/O", &st->io_interface, &st->io); // DAFB registers at $F9800000; VRAM pages direct at $F9000000. memory_map_add(cfg->mem_map, DAFB_REG_BASE, DAFB_REG_APERTURE, "DAFB regs", diff --git a/src/machines/mdu/iici.c b/src/machines/mdu/iici.c index 0bbbf82d..81a9c07f 100644 --- a/src/machines/mdu/iici.c +++ b/src/machines/mdu/iici.c @@ -166,7 +166,7 @@ static void iici_memory_layout_init(config_t *cfg) { } mac030_io_fill_interface(&st->io_interface); - memory_map_add(cfg->mem_map, IICI_IO_BASE, IICI_IO_SIZE, "IIci I/O", &st->io_interface, &st->mdu_io); + memory_map_add(cfg->mem_map, IICI_IO_BASE, IICI_IO_SIZE, "I/O", &st->io_interface, &st->mdu_io); // Wire the built-in framebuffer (a registered host region) and its // Mode-24 slot-$B alias into the page table — same machinery as the diff --git a/src/machines/mdu/iisi.c b/src/machines/mdu/iisi.c index 8ae52a20..9da00f5a 100644 --- a/src/machines/mdu/iisi.c +++ b/src/machines/mdu/iisi.c @@ -125,7 +125,7 @@ static void iisi_memory_layout_init(config_t *cfg) { } mac030_io_fill_interface(&st->io_interface); - memory_map_add(cfg->mem_map, IISI_IO_BASE, IISI_IO_SIZE, "IIsi I/O", &st->io_interface, &st->mdu_io); + memory_map_add(cfg->mem_map, IISI_IO_BASE, IISI_IO_SIZE, "I/O", &st->io_interface, &st->mdu_io); // No separate VRAM aperture to wire: the on-board frame buffer IS the bottom // of Bank A (physical 0). The OS reaches the screen through its PMMU tree diff --git a/src/machines/oss/iifx.c b/src/machines/oss/iifx.c index c70626c7..c0d323b4 100644 --- a/src/machines/oss/iifx.c +++ b/src/machines/oss/iifx.c @@ -1426,8 +1426,7 @@ static void iifx_memory_layout_init(config_t *cfg) { .write_uint16 = iifx_rom_write_uint16, .write_uint32 = iifx_rom_write_uint32, }; - memory_map_add(cfg->mem_map, IIFX_ROM_START, IIFX_ROM_END - IIFX_ROM_START, "IIfx ROM switch", &st->rom_interface, - cfg); + memory_map_add(cfg->mem_map, IIFX_ROM_START, IIFX_ROM_END - IIFX_ROM_START, "ROM switch", &st->rom_interface, cfg); // Reads keep the machID pre-check (above the mirror) then delegate to the // shared engine; writes go straight to the engine. ctx is the engine's @@ -1440,7 +1439,7 @@ static void iifx_memory_layout_init(config_t *cfg) { .write_uint16 = mac030_io_write_uint16, .write_uint32 = mac030_io_write_uint32, }; - memory_map_add(cfg->mem_map, IIFX_IO_BASE, IIFX_IO_SIZE, "IIfx I/O", &st->io_interface, &st->iifx_io); + memory_map_add(cfg->mem_map, IIFX_IO_BASE, IIFX_IO_SIZE, "I/O", &st->io_interface, &st->iifx_io); // Project card host regions (VRAM/declaration ROMs) plus their Mode-24 // slot aliases into the page table (shared helper; see iicx.c). diff --git a/src/machines/pdm/pdm.c b/src/machines/pdm/pdm.c index ca0f8a07..b79f19e1 100644 --- a/src/machines/pdm/pdm.c +++ b/src/machines/pdm/pdm.c @@ -182,7 +182,7 @@ static void pdm_memory_layout(config_t *cfg) { st->io_interface.write_uint8 = pdm_io_write8; st->io_interface.write_uint16 = pdm_io_write16; st->io_interface.write_uint32 = pdm_io_write32; - memory_map_add(cfg->mem_map, 0x50F00000u, 0x00050000u, "PDM I/O", &st->io_interface, cfg); + memory_map_add(cfg->mem_map, 0x50F00000u, 0x00050000u, "I/O", &st->io_interface, cfg); // Machine-ID page. st->id_interface.read_uint8 = pdm_id_read8; From 7ca2c0b769d012a1efb7f177ea33be22bd353a30 Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:38:32 +0000 Subject: [PATCH 05/12] ppc: VXSOFT and VXSQRT do not exist on the 601 The 601 model implemented FPSCR bits 21 (VXSOFT) and 22 (VXSQRT). 601UM Table 2-1 marks both "Not implemented in the 601". Found because the powerpc-test submodule had been left off-pin, and that revision's mtfsb1 vectors cover crbD=22: it expects fpscr.fx = 0 after `mtfsb1 22`, and we produced 1. The vectors are right and the model was wrong -- checked against the manuals rather than taken on trust, and the first reading (that the vector was wrong) did not survive the check. What settles "not implemented" as "the bit does not exist" rather than "hardware never raises it": VXSOFT can ONLY ever be set by software. MPCFPE32B bit 21 -- "can be altered only by the mcrfs, mtfsfi, mtfsf, mtfsb0, or mtfsb1 instructions". If the 601 held the storage the bit would be fully functional and there would be nothing to call unimplemented. VXSQRT sits in the same row of the same table, and its stated purpose is likewise to let software simulate the fsqrt/frsqrte the 601 does not have (601UM Table 5-17). Both bits are ordinary sticky bits in the architecture (MPCFPE32B Table 2-1), and the 604UM has no FPSCR table of its own -- it defers to the architecture -- so the 604 keeps them. Adds ppc_fpscr_nowrite(p), modelled on the existing ppc_msr_mask(p): FEX and VX on both models (derived summaries, never explicitly writable), plus VXSOFT and VXSQRT on the 601. Applied to all three write paths -- mtfsb0/mtfsb1, mtfsf, mtfsfi. Writing an unimplemented bit is now a no-op, so it causes no 0->1 transition and therefore does not set FX either, which is the observable the vector checks. The FX logic itself was already correct: ppc_fpscr_raise sets FX only when the target bit was previously 0. No test-side change. At the pinned corpus both model passes are green without one (601: 4612 executions, 604: 3400, 0 failures each) -- the pinned vectors simply never exercise this case, which is why it went unnoticed. Anyone bumping the pin to e2c2907 will need "mtfsb1" added to skip_604 in tests/unit/suites/ppc_vectors/test.c, because there the 604 correctly sets FX while the vector records the 601 answer; that belongs with the pin bump. I added it, confirmed it was unnecessary at the pin, and reverted it rather than lose 604 coverage. Verified: make headless and make both build with no new warnings; all 58 C unit suites pass; 51 integration rows, 0 failures -- the unit tier plus suite-pdm and the TNT boot rows, the machines whose guests run this code. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/cpu/ppc/ppc_fpu.c | 13 +++++++++---- src/core/cpu/ppc/ppc_internal.h | 22 ++++++++++++++++++++++ src/core/cpu/ppc/ppc_softfp.h | 4 ++-- 3 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/core/cpu/ppc/ppc_fpu.c b/src/core/cpu/ppc/ppc_fpu.c index 892d9ad4..a414448f 100644 --- a/src/core/cpu/ppc/ppc_fpu.c +++ b/src/core/cpu/ppc/ppc_fpu.c @@ -230,7 +230,7 @@ void ppc_do_mcrfs(ppc_t *p, uint32_t iw) { // are never writable and re-derive. mtfsb0/mtfsb1 (folios 10-131/132): // bits 1 and 2 cannot be explicitly written. void ppc_do_mtfsf(ppc_t *p, uint32_t iw) { - uint32_t m = ppc_crm_mask((iw >> 17) & 0xFFu) & ~PPC_FPSCR_UNWRITABLE; + uint32_t m = ppc_crm_mask((iw >> 17) & 0xFFu) & ~ppc_fpscr_nowrite(p); p->fpscr = ppc_fpscr_derive(((uint32_t)p->fpr[PPC_RB(iw)] & m) | (p->fpscr & ~m)); if (PPC_RC(iw)) ppc_set_cr_field(p, 1, p->fpscr >> 28); @@ -239,7 +239,7 @@ void ppc_do_mtfsf(ppc_t *p, uint32_t iw) { void ppc_do_mtfsfi(ppc_t *p, uint32_t iw) { uint32_t sh = 28 - 4 * PPC_CRFD(iw); - uint32_t m = (0xFu << sh) & ~PPC_FPSCR_UNWRITABLE; + uint32_t m = (0xFu << sh) & ~ppc_fpscr_nowrite(p); p->fpscr = ppc_fpscr_derive(((((iw >> 12) & 0xFu) << sh) & m) | (p->fpscr & ~m)); if (PPC_RC(iw)) ppc_set_cr_field(p, 1, p->fpscr >> 28); @@ -253,10 +253,15 @@ void ppc_do_mtfsfi(ppc_t *p, uint32_t iw) { // registers altered" line names only FPSCR[crbD], but that list also omits // the derived VX, so it reads as a summary rather than an exhaustive action // list — unlike §5.4.7.4.1, which IS one and does override the same table -// for FR/FI on disabled overflow. powerpc-test's model agrees.) +// for FR/FI on disabled overflow.) +// +// The transition rule is what makes this conditional: ppc_fpscr_raise sets +// FX only when the bit was previously 0. And on the 601, VXSOFT and VXSQRT +// are not implemented at all (ppc_fpscr_nowrite), so mtfsb1 of either is a +// no-op there and sets nothing — FX included. void ppc_do_mtfsb(ppc_t *p, uint32_t iw, bool set) { uint32_t bit = 0x80000000u >> PPC_RT(iw); - if (!(bit & PPC_FPSCR_UNWRITABLE)) { + if (!(bit & ppc_fpscr_nowrite(p))) { if (!set) p->fpscr &= ~bit; else if (bit & PPC_FPSCR_EXCEPTIONS) diff --git a/src/core/cpu/ppc/ppc_internal.h b/src/core/cpu/ppc/ppc_internal.h index 5165bc2d..f4518829 100644 --- a/src/core/cpu/ppc/ppc_internal.h +++ b/src/core/cpu/ppc/ppc_internal.h @@ -16,6 +16,7 @@ #include "machine_profile.h" // CPU_MODEL_PPC601 / CPU_MODEL_PPC604 #include "memory.h" +#include "ppc_softfp.h" // FPSCR bit masks (leaf header: stdint only) #include #include @@ -200,6 +201,27 @@ static inline uint32_t ppc_msr_mask(const ppc_t *p) { return ppc_is_604(p) ? PPC_MSR_MASK_604 : PPC_MSR_MASK; } +// FPSCR bits no "move to FPSCR" instruction may write on the active model. +// Always FEX and VX (derived summaries — MPCFPE32B Table 2-1: "cannot alter +// explicitly"), plus VXSOFT and VXSQRT on the 601, which 601UM Table 2-1 +// marks "Not implemented in the 601" (bits 21 and 22). +// +// Not implemented means the bits do not exist, not merely that hardware +// never raises them: VXSOFT can ONLY ever be set by software — MPCFPE32B +// bit 21, "can be altered only by the mcrfs, mtfsfi, mtfsf, mtfsb0, or +// mtfsb1 instructions" — so if the 601 held the storage the bit would be +// fully functional and there would be nothing to call unimplemented. The +// same row style marks VXSQRT, whose purpose is likewise to let software +// simulate the fsqrt/frsqrte the 601 does not have (601UM Table 5-17). +// +// Consequence for mtfsb1: writing an unimplemented bit is a no-op, so it +// causes no 0->1 transition and therefore does NOT set FX. The 604 keeps +// both bits — the 604UM has no FPSCR table of its own and defers to the +// architecture, where bits 21 and 22 are ordinary sticky bits. +static inline uint32_t ppc_fpscr_nowrite(const ppc_t *p) { + return PPC_FPSCR_UNWRITABLE | (ppc_is_604(p) ? 0u : (PPC_FPSCR_VXSOFT | PPC_FPSCR_VXSQRT)); +} + // MSR bits an exception entry preserves: ME and EP on both models, plus PM // on the 604 (unlisted in the 604UM per-exception MSR rows — unaltered). static inline uint32_t ppc_msr_exception_keep(const ppc_t *p) { diff --git a/src/core/cpu/ppc/ppc_softfp.h b/src/core/cpu/ppc/ppc_softfp.h index 700daa4e..9fb89c0e 100644 --- a/src/core/cpu/ppc/ppc_softfp.h +++ b/src/core/cpu/ppc/ppc_softfp.h @@ -44,8 +44,8 @@ #define PPC_FPSCR_C 0x00010000u // bit 15: result class descriptor #define PPC_FPSCR_FPCC 0x0000F000u // bits 16-19: FL/FG/FE/FU #define PPC_FPSCR_FPRF 0x0001F000u // bits 15-19: C + FPCC -#define PPC_FPSCR_VXSOFT 0x00000400u // bit 21: software-request invalid (601: storage only) -#define PPC_FPSCR_VXSQRT 0x00000200u // bit 22: invalid sqrt (601: storage only) +#define PPC_FPSCR_VXSOFT 0x00000400u // bit 21: software-request invalid (not on the 601) +#define PPC_FPSCR_VXSQRT 0x00000200u // bit 22: invalid sqrt (not on the 601) #define PPC_FPSCR_VXCVI 0x00000100u // bit 23: invalid integer convert #define PPC_FPSCR_VE 0x00000080u // bit 24: invalid-op exception enable #define PPC_FPSCR_OE 0x00000040u // bit 25: overflow exception enable From fa33cd787784bbf7585d040df4e6d9cf4a1a8b35 Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:49:36 +0000 Subject: [PATCH 06/12] Code review 2026-09-03: duplicate includes, and a CI guard for SPDX headers (F-26) F-26 reported two things about core/peripherals/swim3.c: a missing SPDX header and a doubled #include "floppy.h". The header half is already done -- swim3.c line 1 has been the SPDX identifier since branch 1's SPDX pass, and a sweep of all 355 .c/.h files under src/ finds none missing it. So "the only file in the swim3/awacs group without the mandated header" no longer holds. The include half is real, and there are three, not one. F-26 names swim3.c; the same sweep also finds swim3_xfer.c (also "floppy.h", the same shape -- the header sits in the own-header group AND the alphabetical project group, and the project group is its right home since swim3.h only forward-declares struct floppy) and mouse.c (, a stray one-include group above the system group that already had it). All three removed; the sweep now returns zero. Also takes up the finding's suggestion of a CI guard, as a step in the existing Clang-Format Check job -- same trigger and same file set, so no extra checkout or runner. Three things checked rather than assumed: - The find idiom the existing step uses (unparenthesised -name '*.c' -o -name '*.h') really does enumerate both extensions -- 206 + 149 = 355, identical to the parenthesised form. That also validates the sweep above, which used the same construct; had it been wrong, "none missing" would have been a false negative on every header in the tree. - The workflow still parses and the job keeps its steps. - A negative test: stripping the SPDX line from mouse.c makes the guard report exactly that file and fail. A check that can only pass is not a check. Note the job is named for clang-format and this step is not formatting. Kept there deliberately -- a separate workflow for one grep costs a second checkout for no benefit -- but the job name is now slightly narrow, and renaming it is a one-line change if that reads better. Verified: make headless and make both build, no new warnings (mouse.c's unused 'ev_name' reproduces on HEAD and is unrelated); all 58 C unit suites pass; 51 integration rows, 0 failures -- the unit tier plus ans-diag-floppy, which boots a machine from floppy through SWIM3, and suite-pdm. Removing a duplicate include is a no-op given include guards; the runs are due diligence, not doubt. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/clang-format.yml | 13 +++++++++++++ src/core/peripherals/mouse.c | 2 -- src/core/peripherals/swim3.c | 1 - src/core/peripherals/swim3_xfer.c | 1 - 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/.github/workflows/clang-format.yml b/.github/workflows/clang-format.yml index b534f033..de9527e0 100644 --- a/.github/workflows/clang-format.yml +++ b/.github/workflows/clang-format.yml @@ -31,3 +31,16 @@ jobs: run: | find src -name '*.c' -o -name '*.h' \ | xargs clang-format-18 --dry-run --Werror + + # STYLE_GUIDE.md "File Structure": every source file begins with the + # two-line copyright notice. The tree is clean; this keeps it that way + # (the 2026-09-03 review found four files missing it, and F-26 a fifth). + - name: Check SPDX headers (src/) + run: | + missing=$(find src \( -name '*.c' -o -name '*.h' \) \ + -exec sh -c 'head -1 "$1" | grep -q "^// SPDX-License-Identifier:" || echo "$1"' _ {} \;) + if [ -n "$missing" ]; then + echo "Files whose first line is not an SPDX identifier:" + echo "$missing" + exit 1 + fi diff --git a/src/core/peripherals/mouse.c b/src/core/peripherals/mouse.c index 16266bc8..ad83db6b 100644 --- a/src/core/peripherals/mouse.c +++ b/src/core/peripherals/mouse.c @@ -12,8 +12,6 @@ #include "system.h" #include "value.h" -#include - #include #include #include diff --git a/src/core/peripherals/swim3.c b/src/core/peripherals/swim3.c index ed73e5a4..a3afb28b 100644 --- a/src/core/peripherals/swim3.c +++ b/src/core/peripherals/swim3.c @@ -26,7 +26,6 @@ // on this emulator (`debug.log swim3 5`), not from its source. #include "swim3.h" -#include "floppy.h" #include "floppy.h" #include "log.h" diff --git a/src/core/peripherals/swim3_xfer.c b/src/core/peripherals/swim3_xfer.c index 641110fe..8771330f 100644 --- a/src/core/peripherals/swim3_xfer.c +++ b/src/core/peripherals/swim3_xfer.c @@ -36,7 +36,6 @@ // nibblisation, the 3-byte checksum); and the byte streams the ROM's own // .Sony driver puts on the DMA channel, observed on this emulator. -#include "floppy.h" #include "swim3.h" #include "floppy.h" From 5b7bcfde3c33cf93f9148d427d273482d393a41c Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Mon, 7 Sep 2026 21:34:18 +0000 Subject: [PATCH 07/12] Code review 2026-09-03: one config_t teardown chain for the 68k families (F-27) Five 68k families each carried a hand-written copy of the chain that frees the config_t-owned devices. Diffed against the GLUE copy with comments and whitespace stripped, the ENTIRE difference was: mcu 4 lines the free(st) epilogue -- otherwise byte-identical mdu 8 lines no via2 block, plus the epilogue iifx 8 lines same av 8 lines same and the via2 block is already `if (cfg->via2)` in the shared copy, so all four can call it unchanged. ~150 lines of literal duplication on the path where use-after-free lives, with the AppleTalk-before-SCC ordering comment copied verbatim five times -- and a change to the chain (F-22's PCI addition, say) had to be made five times or made inconsistently. Two corrections to the finding. It says iifx also lacks the scsi block, "already freed earlier"; iifx.c has it, and iifx's diff is identical to mdu's. Its per-family counts are stale too (iifx 12 -> 8, av 10 -> 8). The chain moves to src/machines/runtime/machine_teardown.{c,h} rather than mac030_glue.h as the finding suggests. PDM and TNT adopt it in the next commit and neither is a mac030 machine -- neither includes that header, while both already use mac_host_io.h from runtime/, which is where the shared machine-layer helpers live. Putting it there now avoids moving it again. It carries cfg->ppc beside cfg->cpu for the same reason. The header states the contract the five copies only implied: the canonical order, that NULL handles are skipped, which two orderings are load-bearing and why (AppleTalk holds the SCC pointer; VIA holders must already be gone), and that scheduler_stop() runs first -- so no callback can fire during the chain, and the only hazard ordering guards against is a delete reading through a pointer into an already-freed object. Net -165 lines across the five families for a 116-line shared pair. Verified: make headless AND make -- the second one mattered. Relocating the helper left three files without the include (mcu.c and av.c pull mac030_glue.h in via their own headers; mdu.c's include has a trailing comment my edit did not match), which GCC reports as an implicit declaration and emcc rejects outright. Fixed before committing; both toolchains are now clean. 58 integration rows, 0 failures: the unit tier plus suite-se30/iicx/iix/iici/iisi/iifx/quadra/av and both machine-restart rows, which drive teardown directly. Co-Authored-By: Claude Opus 5 (1M context) --- src/machines/av/av.c | 40 ++------------ src/machines/mac030/mac030_glue.c | 43 +-------------- src/machines/mcu/mcu.c | 43 ++------------- src/machines/mdu/mdu.c | 41 ++------------ src/machines/oss/iifx.c | 39 ++----------- src/machines/runtime/machine_teardown.c | 73 +++++++++++++++++++++++++ src/machines/runtime/machine_teardown.h | 43 +++++++++++++++ 7 files changed, 136 insertions(+), 186 deletions(-) create mode 100644 src/machines/runtime/machine_teardown.c create mode 100644 src/machines/runtime/machine_teardown.h diff --git a/src/machines/av/av.c b/src/machines/av/av.c index e9ce4575..a9f235a3 100644 --- a/src/machines/av/av.c +++ b/src/machines/av/av.c @@ -13,6 +13,8 @@ #include "av.h" #include "appletalk.h" +#include "machine_teardown.h" // the shared config_t-owned delete chain + #include "civic.h" #include "cuda.h" #include "dsp.h" @@ -876,41 +878,9 @@ static void av_teardown(config_t *cfg) { st->bus_mmu = NULL; } } - if (cfg->scsi) { - scsi_delete(cfg->scsi); - cfg->scsi = NULL; - } - if (cfg->via1) { - via_delete(cfg->via1); - cfg->via1 = NULL; - } - // The AppleTalk stack is a client of the SCC's LocalTalk channel, so it - // goes first — it holds the scc pointer it was given at init. - appletalk_delete(); - if (cfg->scc) { - scc_delete(cfg->scc); - cfg->scc = NULL; - } - if (cfg->rtc) { - rtc_delete(cfg->rtc); - cfg->rtc = NULL; - } - if (cfg->scheduler) { - scheduler_delete(cfg->scheduler); - cfg->scheduler = NULL; - } - if (cfg->cpu) { - cpu_delete(cfg->cpu); - cfg->cpu = NULL; - } - if (cfg->mem_map) { - memory_map_delete(cfg->mem_map); - cfg->mem_map = NULL; - } - if (cfg->debugger) { - debug_cleanup(cfg->debugger); - cfg->debugger = NULL; - } + // The config_t-owned devices, in the family-shared canonical order + // (machine_teardown.h). Was a byte-identical copy in five families. + machine_teardown_config_devices(cfg); if (st) { free(st); cfg->machine_context = NULL; diff --git a/src/machines/mac030/mac030_glue.c b/src/machines/mac030/mac030_glue.c index c2bffbc7..2160fe22 100644 --- a/src/machines/mac030/mac030_glue.c +++ b/src/machines/mac030/mac030_glue.c @@ -5,7 +5,9 @@ // Shared GLUE-family lifecycle leaves — see mac030_glue.h. #include "mac030_glue.h" + #include "appletalk.h" +#include "machine_teardown.h" #include "mac_host_io.h" // mac_fd_*/mac_input_* substrate methods (shared by all Macs) #include "machine_profile.h" // machine_substrate_t @@ -347,46 +349,7 @@ void mac030_glue_teardown(config_t *cfg, struct adb *adb, struct asc *asc, struc cfg->adb = NULL; } - // config_t-owned devices. - if (cfg->scsi) { - scsi_delete(cfg->scsi); - cfg->scsi = NULL; - } - if (cfg->via2) { - via_delete(cfg->via2); - cfg->via2 = NULL; - } - if (cfg->via1) { - via_delete(cfg->via1); - cfg->via1 = NULL; - } - // The AppleTalk stack is a client of the SCC's LocalTalk channel, so it - // goes first — it holds the scc pointer it was given at init. - appletalk_delete(); - if (cfg->scc) { - scc_delete(cfg->scc); - cfg->scc = NULL; - } - if (cfg->rtc) { - rtc_delete(cfg->rtc); - cfg->rtc = NULL; - } - if (cfg->scheduler) { - scheduler_delete(cfg->scheduler); - cfg->scheduler = NULL; - } - if (cfg->cpu) { - cpu_delete(cfg->cpu); - cfg->cpu = NULL; - } - if (cfg->mem_map) { - memory_map_delete(cfg->mem_map); - cfg->mem_map = NULL; - } - if (cfg->debugger) { - debug_cleanup(cfg->debugger); - cfg->debugger = NULL; - } + machine_teardown_config_devices(cfg); } // ============================================================ diff --git a/src/machines/mcu/mcu.c b/src/machines/mcu/mcu.c index 2d947ed2..324d6990 100644 --- a/src/machines/mcu/mcu.c +++ b/src/machines/mcu/mcu.c @@ -11,6 +11,7 @@ #include "appletalk.h" #include "mac_host_io.h" // mac_fd_*/mac_input_* +#include "machine_teardown.h" // the shared config_t-owned delete chain #include "mmu040.h" #include "adb.h" @@ -752,45 +753,9 @@ static void mcu_teardown(config_t *cfg) { cfg->adb = NULL; } } - if (cfg->scsi) { - scsi_delete(cfg->scsi); - cfg->scsi = NULL; - } - if (cfg->via2) { - via_delete(cfg->via2); - cfg->via2 = NULL; - } - if (cfg->via1) { - via_delete(cfg->via1); - cfg->via1 = NULL; - } - // The AppleTalk stack is a client of the SCC's LocalTalk channel, so it - // goes first — it holds the scc pointer it was given at init. - appletalk_delete(); - if (cfg->scc) { - scc_delete(cfg->scc); - cfg->scc = NULL; - } - if (cfg->rtc) { - rtc_delete(cfg->rtc); - cfg->rtc = NULL; - } - if (cfg->scheduler) { - scheduler_delete(cfg->scheduler); - cfg->scheduler = NULL; - } - if (cfg->cpu) { - cpu_delete(cfg->cpu); - cfg->cpu = NULL; - } - if (cfg->mem_map) { - memory_map_delete(cfg->mem_map); - cfg->mem_map = NULL; - } - if (cfg->debugger) { - debug_cleanup(cfg->debugger); - cfg->debugger = NULL; - } + // The config_t-owned devices, in the family-shared canonical order + // (machine_teardown.h). Was a byte-identical copy in five families. + machine_teardown_config_devices(cfg); if (st) { free(st); cfg->machine_context = NULL; diff --git a/src/machines/mdu/mdu.c b/src/machines/mdu/mdu.c index a2aacab0..da59ce57 100644 --- a/src/machines/mdu/mdu.c +++ b/src/machines/mdu/mdu.c @@ -13,6 +13,7 @@ #include "mac030_glue.h" // shared core/finish/reset/irq/build_mmu + board desc #include "mac_host_io.h" // mac_fd_*/mac_input_* +#include "machine_teardown.h" // the shared config_t-owned delete chain #include "mdu_io.h" // mac030_mdu_state_t + mdu_io_bind #include "adb.h" @@ -140,43 +141,9 @@ static void mdu_teardown(config_t *cfg) { cfg->adb = NULL; } } - // cfg->nubus is freed by system_destroy (nubus_delete runs before machine - // teardown), matching the GLUE lifecycle. - if (cfg->scsi) { - scsi_delete(cfg->scsi); - cfg->scsi = NULL; - } - if (cfg->via1) { - via_delete(cfg->via1); - cfg->via1 = NULL; - } - // The AppleTalk stack is a client of the SCC's LocalTalk channel, so it - // goes first — it holds the scc pointer it was given at init. - appletalk_delete(); - if (cfg->scc) { - scc_delete(cfg->scc); - cfg->scc = NULL; - } - if (cfg->rtc) { - rtc_delete(cfg->rtc); - cfg->rtc = NULL; - } - if (cfg->scheduler) { - scheduler_delete(cfg->scheduler); - cfg->scheduler = NULL; - } - if (cfg->cpu) { - cpu_delete(cfg->cpu); - cfg->cpu = NULL; - } - if (cfg->mem_map) { - memory_map_delete(cfg->mem_map); - cfg->mem_map = NULL; - } - if (cfg->debugger) { - debug_cleanup(cfg->debugger); - cfg->debugger = NULL; - } + // The config_t-owned devices, in the family-shared canonical order + // (machine_teardown.h). Was a byte-identical copy in five families. + machine_teardown_config_devices(cfg); if (st) { free(st); cfg->machine_context = NULL; diff --git a/src/machines/oss/iifx.c b/src/machines/oss/iifx.c index c0d323b4..4fe6a3b2 100644 --- a/src/machines/oss/iifx.c +++ b/src/machines/oss/iifx.c @@ -8,6 +8,7 @@ #include "mac030_glue.h" #include "mac_host_io.h" #include "machine.h" +#include "machine_teardown.h" #include "mmu_checkpoint.h" #include "system_config.h" @@ -1650,41 +1651,9 @@ static void iifx_teardown(config_t *cfg) { cfg->adb = NULL; } } - if (cfg->scsi) { - scsi_delete(cfg->scsi); - cfg->scsi = NULL; - } - if (cfg->via1) { - via_delete(cfg->via1); - cfg->via1 = NULL; - } - // The AppleTalk stack is a client of the SCC's LocalTalk channel, so it - // goes first — it holds the scc pointer it was given at init. - appletalk_delete(); - if (cfg->scc) { - scc_delete(cfg->scc); - cfg->scc = NULL; - } - if (cfg->rtc) { - rtc_delete(cfg->rtc); - cfg->rtc = NULL; - } - if (cfg->scheduler) { - scheduler_delete(cfg->scheduler); - cfg->scheduler = NULL; - } - if (cfg->cpu) { - cpu_delete(cfg->cpu); - cfg->cpu = NULL; - } - if (cfg->mem_map) { - memory_map_delete(cfg->mem_map); - cfg->mem_map = NULL; - } - if (cfg->debugger) { - debug_cleanup(cfg->debugger); - cfg->debugger = NULL; - } + // The config_t-owned devices, in the family-shared canonical order + // (machine_teardown.h). Was a byte-identical copy in five families. + machine_teardown_config_devices(cfg); if (st) { free(st); cfg->machine_context = NULL; diff --git a/src/machines/runtime/machine_teardown.c b/src/machines/runtime/machine_teardown.c new file mode 100644 index 00000000..4c92e8fd --- /dev/null +++ b/src/machines/runtime/machine_teardown.c @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) pappadf + +// machine_teardown.c +// The shared config_t-owned teardown chain -- see machine_teardown.h for the +// contract and for why the ordering is load-bearing. + +#include "machine_teardown.h" + +#include "appletalk.h" +#include "cpu.h" +#include "debug.h" +#include "memory.h" +#include "ppc.h" +#include "rtc.h" +#include "scc.h" +#include "scheduler.h" +#include "scsi.h" +#include "system_config.h" +#include "via.h" + +void machine_teardown_config_devices(config_t *cfg) { + if (!cfg) + return; + + if (cfg->scsi) { + scsi_delete(cfg->scsi); + cfg->scsi = NULL; + } + // NULL on the single-VIA machines (MDU, OSS, AV, and both PowerPC + // families) -- they simply pass through. + if (cfg->via2) { + via_delete(cfg->via2); + cfg->via2 = NULL; + } + if (cfg->via1) { + via_delete(cfg->via1); + cfg->via1 = NULL; + } + // The AppleTalk stack is a client of the SCC's LocalTalk channel, so it + // goes first -- it holds the scc pointer it was given at init. + appletalk_delete(); + if (cfg->scc) { + scc_delete(cfg->scc); + cfg->scc = NULL; + } + if (cfg->rtc) { + rtc_delete(cfg->rtc); + cfg->rtc = NULL; + } + if (cfg->scheduler) { + scheduler_delete(cfg->scheduler); + cfg->scheduler = NULL; + } + // One of the two, never both: the 68k families build cfg->cpu, the + // PowerPC families cfg->ppc. + if (cfg->cpu) { + cpu_delete(cfg->cpu); + cfg->cpu = NULL; + } + if (cfg->ppc) { + ppc_delete(cfg->ppc); + cfg->ppc = NULL; + } + if (cfg->mem_map) { + memory_map_delete(cfg->mem_map); + cfg->mem_map = NULL; + } + if (cfg->debugger) { + debug_cleanup(cfg->debugger); + cfg->debugger = NULL; + } +} diff --git a/src/machines/runtime/machine_teardown.h b/src/machines/runtime/machine_teardown.h new file mode 100644 index 00000000..d1b70e99 --- /dev/null +++ b/src/machines/runtime/machine_teardown.h @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) pappadf + +// machine_teardown.h +// The one config_t-owned teardown chain, shared by every machine family. +// +// Before this existed, seven families each carried a hand-written copy of it +// (the 2026-09-03 review's F-27). Measured with comments and whitespace +// stripped, four of the five 68k copies differed from the GLUE one only in +// whether they had a VIA2 -- which the NULL guard below already covers -- and +// the MCU's was byte-identical. A change to the chain had to be made seven +// times or it was made inconsistently. + +#ifndef GS_MACHINES_RUNTIME_MACHINE_TEARDOWN_H +#define GS_MACHINES_RUNTIME_MACHINE_TEARDOWN_H + +struct config; + +// Free every config_t-owned device, in the one canonical order: +// +// scsi -> via2 -> via1 -> appletalk -> scc -> rtc -> scheduler -> +// cpu | ppc -> mem_map -> debugger +// +// Any NULL handle is skipped, so a machine with one VIA or no SCSI passes +// straight through those steps, and the 68k/PowerPC split falls out of which +// of cfg->cpu / cfg->ppc the family built. +// +// Two orderings in here are load-bearing, and are why this is one function +// rather than a per-family list: the AppleTalk stack holds the SCC pointer it +// was given at init, so it goes before scc_delete; and every device that holds +// a VIA must already be gone by the time the VIAs are freed. +// +// Call it AFTER freeing whatever lives in the machine's private state, and +// free that state afterwards. Every family's teardown is therefore: +// +// scheduler_stop -> the family's own devices -> this -> free(st) +// +// Note that scheduler_stop() comes first, so no device callback can fire +// while the chain runs; the only hazard ordering guards against is one +// delete reading through a pointer into an already-freed object. +void machine_teardown_config_devices(struct config *cfg); + +#endif // GS_MACHINES_RUNTIME_MACHINE_TEARDOWN_H From 2e50ef528d614304d3203230b3c4a3a1cbca1b5a Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Mon, 7 Sep 2026 21:42:25 +0000 Subject: [PATCH 08/12] Code review 2026-09-03: PDM and TNT join the shared teardown chain (F-27) Completes F-27: seven hand-written config_t teardown chains become one. The previous commit did the five 68k families; these two are the PowerPC ones, and they are separate because they carry a judgement the others did not. Both families keep devices that no other family has, and those sat BETWEEN cfg->scsi and cfg->via1 in their own copies of the chain -- floppy, cuda and adb on the PDM; scsi2, dbdma, cuda and adb on the TNT. They stay per-family and are hoisted above the shared chain in their existing relative order, so the one behavioural change is that cfg->scsi is freed after them rather than before the first. Why that is safe, since this is the use-after-free path: - None of scsi_delete(scsi2), tnt_dbdma_delete, av_cuda_delete, adb_delete or floppy_delete reads a SCSI handle -- checked against the function bodies, with scsi_delete's own 26 references as the control that the check was looking in the right place. - The controllers that DO hold the buses are already gone: MESH and the 53C96 are freed immediately above, and the two 53C825As go with the PCI root, which system_destroy now frees before any substrate teardown runs (F-22, earlier on this branch). - DBDMA still goes after the floppy and before the SCC whose channels it serves; Cuda still goes before the via1, rtc and adb it was handed at init (av_cuda_init takes all three), which is the ordering that actually matters here. - scheduler_stop() is the first statement of every teardown, so no device callback can fire while the chain runs at all. The only hazard left is one delete reading through a pointer into an already-freed object, which is what the three points above cover. That reasoning is recorded at both call sites rather than in this message alone, because it is the part a future reader needs. Verified: make headless and make both clean. 57 integration rows, 0 failures -- the unit tier plus suite-pdm, all three TNT disk-boot variants (7500, 8500/604, 9500 + Mach64), tnt-pci-slots, ans-diag-floppy, and both machine-restart rows, which are the ones that actually drive teardown. Co-Authored-By: Claude Opus 5 (1M context) --- src/machines/pdm/pdm.c | 46 ++++++++++---------------------------- src/machines/tnt/tnt.c | 50 +++++++++++++----------------------------- 2 files changed, 26 insertions(+), 70 deletions(-) diff --git a/src/machines/pdm/pdm.c b/src/machines/pdm/pdm.c index b79f19e1..7dd6191c 100644 --- a/src/machines/pdm/pdm.c +++ b/src/machines/pdm/pdm.c @@ -33,6 +33,7 @@ #include "image.h" #include "log.h" #include "mac_host_io.h" +#include "machine_teardown.h" // the shared config_t-owned delete chain #include "nubus.h" #include "ppc.h" #include "rtc.h" @@ -464,10 +465,15 @@ static void pdm_teardown(config_t *cfg) { } } } - if (cfg->scsi) { - scsi_delete(cfg->scsi); - cfg->scsi = NULL; - } + // The three devices that used to sit between cfg->scsi and cfg->via1 in + // this family's own copy of the chain. They move above the shared chain + // (machine_teardown.h) rather than into it, because only PDM and TNT have + // them; the single ordering change is that cfg->scsi is now freed after + // these three instead of before. That is safe: none of floppy_delete, + // av_cuda_delete or adb_delete reads a SCSI handle, the 53C96 controllers + // that DO hold the bus are already freed above, and scheduler_stop() ran + // first so nothing can fire in between. Cuda still goes before the via1, + // rtc and adb it was handed at init, which is the ordering that matters. if (cfg->floppy) { floppy_delete(cfg->floppy); cfg->floppy = NULL; @@ -480,37 +486,7 @@ static void pdm_teardown(config_t *cfg) { adb_delete(cfg->adb); cfg->adb = NULL; } - if (cfg->via1) { - via_delete(cfg->via1); - cfg->via1 = NULL; - } - // The AppleTalk stack is a client of the SCC's LocalTalk channel, so it - // goes first — it holds the scc pointer it was given at init. - appletalk_delete(); - if (cfg->scc) { - scc_delete(cfg->scc); - cfg->scc = NULL; - } - if (cfg->rtc) { - rtc_delete(cfg->rtc); - cfg->rtc = NULL; - } - if (cfg->scheduler) { - scheduler_delete(cfg->scheduler); - cfg->scheduler = NULL; - } - if (cfg->ppc) { - ppc_delete(cfg->ppc); - cfg->ppc = NULL; - } - if (cfg->mem_map) { - memory_map_delete(cfg->mem_map); - cfg->mem_map = NULL; - } - if (cfg->debugger) { - debug_cleanup(cfg->debugger); - cfg->debugger = NULL; - } + machine_teardown_config_devices(cfg); if (st) { free(st); cfg->machine_context = NULL; diff --git a/src/machines/tnt/tnt.c b/src/machines/tnt/tnt.c index f4ba9c9d..bb51ac99 100644 --- a/src/machines/tnt/tnt.c +++ b/src/machines/tnt/tnt.c @@ -41,6 +41,7 @@ #include "log.h" #include "mac_host_io.h" #include "machine_config.h" // machine_boot_is_restart (the NVRAM carry rule) +#include "machine_teardown.h" // the shared config_t-owned delete chain #include "pci.h" #include "ppc.h" #include "rtc.h" @@ -724,10 +725,19 @@ static void tnt_teardown(config_t *cfg) { floppy_delete(cfg->floppy); cfg->floppy = NULL; } - if (cfg->scsi) { - scsi_delete(cfg->scsi); - cfg->scsi = NULL; - } + // The four devices that used to sit between cfg->scsi and cfg->via1 in + // this family's own copy of the chain, kept in the same relative order and + // simply hoisted above the shared one (machine_teardown.h). Only PDM and + // TNT have them, so they stay here rather than joining the shared chain. + // + // The single ordering change is that cfg->scsi is now freed after these + // four instead of before the first of them. Safe on both counts that + // matter: none of scsi_delete(scsi2), tnt_dbdma_delete, av_cuda_delete or + // adb_delete reads cfg->scsi, and the controllers that DO hold the two + // buses are already gone -- MESH/53C96 just above, and the 53C825As with + // the PCI root, which system_destroy frees before this runs. DBDMA still + // goes after the floppy and before the SCC whose channels it serves, and + // Cuda still goes before the via1, rtc and adb it was handed at init. if (st && st->scsi2) { scsi_delete(st->scsi2); st->scsi2 = NULL; @@ -744,37 +754,7 @@ static void tnt_teardown(config_t *cfg) { adb_delete(cfg->adb); cfg->adb = NULL; } - if (cfg->via1) { - via_delete(cfg->via1); - cfg->via1 = NULL; - } - // The AppleTalk stack is a client of the SCC's LocalTalk channel, so it - // goes first -- it holds the scc pointer it was given at init. - appletalk_delete(); - if (cfg->scc) { - scc_delete(cfg->scc); - cfg->scc = NULL; - } - if (cfg->rtc) { - rtc_delete(cfg->rtc); - cfg->rtc = NULL; - } - if (cfg->scheduler) { - scheduler_delete(cfg->scheduler); - cfg->scheduler = NULL; - } - if (cfg->ppc) { - ppc_delete(cfg->ppc); - cfg->ppc = NULL; - } - if (cfg->mem_map) { - memory_map_delete(cfg->mem_map); - cfg->mem_map = NULL; - } - if (cfg->debugger) { - debug_cleanup(cfg->debugger); - cfg->debugger = NULL; - } + machine_teardown_config_devices(cfg); if (st) { free(st); cfg->machine_context = NULL; From 51289e2f6992277a77bf1bdebfcb4364177540dd Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Tue, 8 Sep 2026 11:16:45 +0000 Subject: [PATCH 09/12] iifx: fix the checkpoint block order, and cover the three families that had none checkpoint.load has been failing on every IIfx. iifx_checkpoint_save wrote its blocks asc -> adb -> floppy while iifx_init restores them asc -> floppy -> adb (iifx.c:1545, :1547, :1557). The checkpoint stream is positional -- no per-block tag, no size field -- so the two cross-loaded and the load died at the first block whose size disagreed: Checkpoint size mismatch: expected 9840 at src/core/peripherals/floppy.c:708 but file contains 336 at src/core/peripherals/adb.c:891 The save side moves, because the convention the other families follow (and tnt_checkpoint_save states outright) is that save mirrors the init construction order. Found while scoping F-28, which proposes extracting the shared checkpoint prologue. Before reshaping a positional format across seven families it was worth asking what would catch a mistake, and the answer was: for three of them, nothing. IIfx, AV and Lisa had no checkpoint round-trip anywhere in the tree, and the IIfx was already broken. So each gains a row. All three save a settled state, boot a DIFFERENT configuration so a restore that quietly kept the old machine cannot pass, load, and assert the restored identity and RAM size; the IIfx and AV rows re-match their pre-save golden as the oracle, following suite-iici's existing checkpoint row. The IIfx row is negative-tested: with the fix reverted and rebuilt it fails with exactly the diagnostic above, and passes with the fix restored. A row that can only pass would not have been worth adding. The Lisa row is deliberately lean and has no golden, and says so at the site. A positional mismatch fails at LOAD with a size error naming both ends -- it does not need a booted OS to surface, which is how the IIfx bug showed itself. Booting LOS would mean duplicating the ProFile + PRAM setup that lisa-los31-profile carries, for a stronger raster oracle than this row needs. AV and Lisa round-tripped correctly already; they simply had nothing watching. Verified: 52 integration rows, 0 failures -- suite-iifx, suite-av and suite-lisa in full, plus the unit tier. Co-Authored-By: Claude Opus 5 (1M context) --- src/machines/oss/iifx.c | 10 ++++++- tests/integration/suite-av/test.script | 36 ++++++++++++++++++++++++ tests/integration/suite-iifx/test.script | 32 +++++++++++++++++++++ tests/integration/suite-lisa/rows.script | 31 ++++++++++++++++++++ tests/integration/suite-lisa/test.script | 1 + 5 files changed, 109 insertions(+), 1 deletion(-) diff --git a/src/machines/oss/iifx.c b/src/machines/oss/iifx.c index 4fe6a3b2..59029844 100644 --- a/src/machines/oss/iifx.c +++ b/src/machines/oss/iifx.c @@ -1673,9 +1673,17 @@ static void iifx_checkpoint_save(config_t *cfg, checkpoint_t *cp) { via_checkpoint(cfg->via1, cp); mac_checkpoint_save_images(cfg, cp); scsi_checkpoint(cfg->scsi, cp); + // Save order must mirror iifx_init's construction order exactly: the + // checkpoint stream is positional, with no per-block tag or size field, so + // a swapped pair does not fail loudly at the swap -- it cross-loads, and + // the size mismatch surfaces later at whichever block first disagrees. + // These three were saved asc -> adb -> floppy while init restores + // asc -> floppy -> adb (:1545, :1547, :1557), which made every + // checkpoint.load on this machine fail with "expected 9840 at + // floppy.c:708 but file contains 336 at adb.c:891". asc_checkpoint(st->asc, cp); - adb_checkpoint(st->adb, cp); floppy_checkpoint(st->floppy, cp); + adb_checkpoint(st->adb, cp); oss_checkpoint(st->oss, cp); iop_checkpoint(st->scc_iop, cp); iop_checkpoint(st->swim_iop, cp); diff --git a/tests/integration/suite-av/test.script b/tests/integration/suite-av/test.script index 48870a7e..5301973f 100644 --- a/tests/integration/suite-av/test.script +++ b/tests/integration/suite-av/test.script @@ -248,6 +248,35 @@ def row_q840av_71_hd_desktop() { report_perf("suite-av/q840av-71-hd-desktop", scheduler.instr_count) } +# ---- av-checkpoint ---------------------------------------------------------- +# The AV family had no checkpoint round-trip anywhere in the tree. Neither did +# the IIfx, and the IIfx's was broken -- its save wrote asc -> adb -> floppy +# while its init restored asc -> floppy -> adb, and nothing existed to catch a +# positional stream going out of step. This row is the AV's guard against the +# same class of mistake: the family builds PSC, Cuda, CIVIC, Singer and the DSP +# into its stream, none of which was ever save/restore tested. +# +# Save a settled desktop, boot a DIFFERENT configuration so a restore that +# quietly kept the old machine cannot pass, load, and re-match the pre-save +# golden once the restored guest is quiescent again. +def row_av_checkpoint() { + machine.rtc.time = 3000000000 + machine.boot model="q840av" ram=16384 rom="${$av_rom}" + machine.scsi.attach_hd "${$av_hd}" 0 + scheduler.run 600000000 + wait_stable(1500000000) + check("goldens/q840av-7.1-640x480x8-hd-finder.png") + assert checkpoint.save("${$WORK_DIR}/suite-av.gsc") "av-checkpoint: save failed" + machine.boot model="q840av" ram=8192 rom="${$av_rom}" + assert checkpoint.load("${$WORK_DIR}/suite-av.gsc") "av-checkpoint: load failed" + assert machine.ram == 16384 "av-checkpoint: restored RAM size wrong" + assert machine.id == "q840av" "av-checkpoint: restored machine.id wrong" + scheduler.run 5000000 + wait_stable(400000000) + check("goldens/q840av-7.1-640x480x8-hd-finder.png") + report_perf("suite-av/av-checkpoint", scheduler.instr_count) +} + # ---- q660av-71-hd-desktop --------------------------------------------------- # The same volume on the sibling board: 25 MHz, strap nibble $B, and no MUNI. def row_q660av_71_hd_desktop() { @@ -836,6 +865,13 @@ if row_on("q660av-71-hd-desktop") { echo "skip: q660av-71-hd-desktop — ${$av_hd} not fetched" } } +if row_on("av-checkpoint") { + if have_media($av_rom) && have_media($av_hd) { + row_end("av-checkpoint", try(row_av_checkpoint(), "FAILED")) + } else { + echo "skip: av-checkpoint — ${$av_hd} not fetched" + } +} if row_on("q840av-71-video-in") { if have_media($av_rom) && have_media($av_hd) { row_end("q840av-71-video-in", try(row_video_in("q840av", "q840av-7.1-640x480x8-video-monitor.png"), "FAILED")) diff --git a/tests/integration/suite-iifx/test.script b/tests/integration/suite-iifx/test.script index 93683072..17b78f6a 100644 --- a/tests/integration/suite-iifx/test.script +++ b/tests/integration/suite-iifx/test.script @@ -241,6 +241,37 @@ def row_iifx_8m() { report_perf("suite-iifx/iifx-8m", scheduler.instr_count) } +# ---- iifx-checkpoint --------------------------------------------------------- +# The IIfx had no checkpoint round-trip anywhere in the tree, and it was broken: +# iifx_checkpoint_save wrote asc -> adb -> floppy while iifx_init restores +# asc -> floppy -> adb. The stream is positional with no per-block tag, so the +# blocks cross-loaded and every load died with +# "expected 9840 at floppy.c:708 but file contains 336 at adb.c:891". +# A save/restore pair is the only thing that catches that class of mistake, so +# the family that had the bug is now the family that tests for it. +# +# Modelled on suite-iici's checkpoint row: save a settled desktop, boot a +# DIFFERENT configuration so a stale machine cannot pass by accident, load, and +# re-match the pre-save golden once the restored guest is quiescent again. +def row_iifx_checkpoint() { + machine.boot model="iifx" ram=16384 video_card="mdc_8_24" rom="${$ROM}" + machine.floppy.drive[0].insert "${$sys_701}" + scheduler.run 200000000 + wait_stable(900000000) + check("goldens/iifx-7.0.1-640x480x1-finder.png") + assert checkpoint.save("${$WORK_DIR}/suite-iifx.gsc") "iifx-checkpoint: save failed" + # A different RAM size, so a restore that quietly kept the old machine fails + # the assert below rather than passing on the golden. + machine.boot model="iifx" ram=8192 video_card="mdc_8_24" rom="${$ROM}" + assert checkpoint.load("${$WORK_DIR}/suite-iifx.gsc") "iifx-checkpoint: load failed" + assert machine.ram == 16384 "iifx-checkpoint: restored RAM size wrong" + assert machine.id == "iifx" "iifx-checkpoint: restored machine.id wrong" + scheduler.run 5000000 + wait_stable(400000000) + check("goldens/iifx-7.0.1-640x480x1-finder.png") + report_perf("suite-iifx/iifx-checkpoint", scheduler.instr_count) +} + # ---- row dispatch ----------------------------------------------------------- if row_on("iifx-chime") { row_end("iifx-chime", try(row_iifx_chime(), "FAILED")) } @@ -263,6 +294,7 @@ if row_on("iifx-aux3-login") { } } if row_on("iifx-8m") { row_end("iifx-8m", try(row_iifx_8m(), "FAILED")) } +if row_on("iifx-checkpoint") { row_end("iifx-checkpoint", try(row_iifx_checkpoint(), "FAILED")) } suite_done() quit diff --git a/tests/integration/suite-lisa/rows.script b/tests/integration/suite-lisa/rows.script index 6a255513..835afad2 100644 --- a/tests/integration/suite-lisa/rows.script +++ b/tests/integration/suite-lisa/rows.script @@ -347,3 +347,34 @@ def row_xl_no_media() { check("goldens/xl-608x431x1-no-media.png") report_perf("suite-lisa/xl-no-media", scheduler.instr_count) } + +# ---- lisa-checkpoint -------------------------------------------------------- +# The Lisa/XL family had no checkpoint round-trip anywhere in the tree. Neither +# did the IIfx, and the IIfx's turned out to be broken -- its save wrote +# asc -> adb -> floppy while its init restored asc -> floppy -> adb, and the +# stream is positional with no per-block tag, so the blocks cross-loaded. +# Nothing existed to catch it. This row is the Lisa's guard against the same +# class of mistake: the family serialises a segment MMU, COPS, its own FDC and +# the ProFile parallel port, none of which was ever save/restore tested. +# +# Deliberately lean, and no golden. A positional-stream mismatch fails at +# LOAD with a size error naming both sites -- it does not need a booted OS to +# surface, which is exactly how the IIfx bug was found. Booting LOS here would +# mean duplicating the ProFile + PRAM setup lisa-los31-profile carries, for a +# stronger raster oracle than this row's purpose needs. If the Lisa ever grows +# a state bug that only a running OS reveals, that is the row to extend. +def row_lisa_checkpoint() { + machine.boot model="lisa" ram=2048 rom="../../data/roms/lisa2-revh-098917b2.rom" + scheduler.run 5000000 + assert checkpoint.save("${$WORK_DIR}/suite-lisa.gsc") "lisa-checkpoint: save failed" + # A different RAM size, so a restore that quietly kept the old machine fails + # the assert below rather than passing by accident. + machine.boot model="lisa" ram=1024 rom="../../data/roms/lisa2-revh-098917b2.rom" + assert checkpoint.load("${$WORK_DIR}/suite-lisa.gsc") "lisa-checkpoint: load failed" + assert machine.id == "lisa" "lisa-checkpoint: restored machine.id wrong" + assert machine.ram == 2048 "lisa-checkpoint: restored RAM size wrong" + assert machine.rom.checksum == "098917B2" "lisa-checkpoint: restored rom.checksum wrong" + # The restored machine must keep executing, not just load. + scheduler.run 5000000 + report_perf("suite-lisa/lisa-checkpoint", scheduler.instr_count) +} diff --git a/tests/integration/suite-lisa/test.script b/tests/integration/suite-lisa/test.script index 6dc196b3..2e97da2e 100644 --- a/tests/integration/suite-lisa/test.script +++ b/tests/integration/suite-lisa/test.script @@ -29,6 +29,7 @@ if row_on("lisa-xenix-boot") { row_end("lisa-xenix-boot", try(row_lisa_xenix_boo if row_on("lisa-xenix-nofloppy") { row_end("lisa-xenix-nofloppy", try(row_lisa_xenix_nofloppy(), "FAILED")) } if row_on("xl-macworks-boot") { row_end("xl-macworks-boot", try(row_xl_macworks_boot(), "FAILED")) } if row_on("xl-no-media") { row_end("xl-no-media", try(row_xl_no_media(), "FAILED")) } +if row_on("lisa-checkpoint") { row_end("lisa-checkpoint", try(row_lisa_checkpoint(), "FAILED")) } suite_done() quit From 089d8bf2abd3976ea59dc81c37399f7527bedade Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Tue, 8 Sep 2026 11:22:58 +0000 Subject: [PATCH 10/12] Code review 2026-09-03: one checkpoint prologue for the 68k families (F-28) The nine writes that open every 68k family's checkpoint stream existed in five copies: memory_map -> cpu -> scheduler -> cfg->irq -> rtc -> scc -> appletalk -> via1 -> [via2] The stream is positional -- no per-block tag, no size field -- so those nine lines are not merely duplicated code, they are a replicated FILE FORMAT, and a copy that dropped one silently wrote a different one. That is not hypothetical: F-23 was exactly this, TNT's copy omitting appletalk_checkpoint. mac030_checkpoint_save_core(cfg, cp) replaces all five. On the via2 question the finding is mistaken, though harmlessly. It warns that guarding via2 "is not stream-neutral for families that today always write it, so keep the unconditional call". Keeping the unconditional call is right, but the reason is the opposite: via_checkpoint(NULL, cp) returns before writing anything, so the four families that previously omitted the call now emit exactly nothing -- byte-identical -- and the restore side stays symmetric because those families never call via_init() for a second VIA either. Both forms would have worked. The PowerPC families deliberately do NOT adopt it. Unlike F-27's teardown chain, where NULL guards made a merge provably equivalent, PDM and TNT emit a genuinely different stream: ppc_checkpoint in place of cpu_checkpoint, and no cfg->irq, because they keep interrupt state in their own register blobs. Sharing would mean one function whose emitted layout varies by CPU, which is worse than two honest ones. The header says so, and says why. It also records what the five copies only implied: that the sequence IS the format; that each family keeps the obligation to mirror its own init order, because a swapped pair does not fail at the swap but cross-loads and dies later at whichever block first disagrees on size -- with the IIfx, fixed in the previous commit, as what that looks like. The "// includes the 040 MMU register file" note that two copies carried on their cpu_checkpoint line is preserved on the shared writer rather than lost. Verified: make headless and make both clean. 58 integration rows, 0 failures, including eight distinct checkpoint round-trips -- se30, iifx, q700, q700-sense, iici, av, lisa and q900 -- plus the cross-process checkpoint row on the Plus and boot-config. That coverage exists for all seven families only because of the previous commit; before it, three of them could not have caught a mistake here at all. Co-Authored-By: Claude Opus 5 (1M context) --- src/machines/av/av.c | 9 +-------- src/machines/mac030/mac030_glue.c | 33 ++++++++++++++++++++++--------- src/machines/mac030/mac030_glue.h | 22 +++++++++++++++++++++ src/machines/mcu/mcu.c | 10 +--------- src/machines/mdu/mdu.c | 9 +-------- src/machines/oss/iifx.c | 9 +-------- 6 files changed, 50 insertions(+), 42 deletions(-) diff --git a/src/machines/av/av.c b/src/machines/av/av.c index a9f235a3..46377a47 100644 --- a/src/machines/av/av.c +++ b/src/machines/av/av.c @@ -889,14 +889,7 @@ static void av_teardown(config_t *cfg) { static void av_checkpoint_save(config_t *cfg, checkpoint_t *cp) { av_state_t *st = av_st(cfg); - memory_map_checkpoint(cfg->mem_map, cp); - cpu_checkpoint(cfg->cpu, cp); // includes the 040 MMU register file - scheduler_checkpoint(cfg->scheduler, cp); - system_write_checkpoint_data(cp, &cfg->irq, sizeof(cfg->irq)); - rtc_checkpoint(cfg->rtc, cp); - scc_checkpoint(cfg->scc, cp); - appletalk_checkpoint(cp); - via_checkpoint(cfg->via1, cp); + mac030_checkpoint_save_core(cfg, cp); // Device order mirrors the checkpoint READS in av_build_devices — the // stream is sequential, so save and restore must walk it identically. av_psc_checkpoint(st->psc, cp); diff --git a/src/machines/mac030/mac030_glue.c b/src/machines/mac030/mac030_glue.c index 2160fe22..67995ee1 100644 --- a/src/machines/mac030/mac030_glue.c +++ b/src/machines/mac030/mac030_glue.c @@ -129,6 +129,29 @@ void mac030_glue_memory_layout(config_t *cfg, const mac030_board_desc_t *desc) { memory_map_add(cfg->mem_map, desc->rom_end, MAC030_GLUE_IO_SIZE, "I/O", &st->io_interface, &st->glue_io); } +// The head of every 68k family's checkpoint stream, in the one order -- see +// the header. Nine writes that were replicated across five families, which +// made a replicated FILE FORMAT: the stream is positional, so those nine +// lines ARE the layout, and a family that dropped one silently wrote a +// different format (the review's F-23: TNT's copy omitted appletalk). +// +// via2 is written unconditionally on purpose. via_checkpoint(NULL, cp) +// returns before writing anything, so a single-VIA machine emits nothing +// here -- byte-identical to the four families that used to omit the call -- +// and the restore side stays symmetric because those families never call +// via_init() for a second VIA either. +void mac030_checkpoint_save_core(config_t *cfg, checkpoint_t *cp) { + memory_map_checkpoint(cfg->mem_map, cp); + cpu_checkpoint(cfg->cpu, cp); // on the 040 families this carries the MMU register file too + scheduler_checkpoint(cfg->scheduler, cp); + system_write_checkpoint_data(cp, &cfg->irq, sizeof(cfg->irq)); + rtc_checkpoint(cfg->rtc, cp); + scc_checkpoint(cfg->scc, cp); + appletalk_checkpoint(cp); + via_checkpoint(cfg->via1, cp); + via_checkpoint(cfg->via2, cp); +} + // Finish init: debugger, scheduler start, cold-boot IRQ/IPL reset. void mac030_glue_finish(config_t *cfg, checkpoint_t *cp) { cfg->debugger = debug_init(); @@ -393,15 +416,7 @@ static void glue_teardown(config_t *cfg) { static void glue_checkpoint_save(config_t *cfg, checkpoint_t *cp) { mac030_glue_state_t *st = (mac030_glue_state_t *)cfg->machine_context; - memory_map_checkpoint(cfg->mem_map, cp); - cpu_checkpoint(cfg->cpu, cp); - scheduler_checkpoint(cfg->scheduler, cp); - system_write_checkpoint_data(cp, &cfg->irq, sizeof(cfg->irq)); - rtc_checkpoint(cfg->rtc, cp); - scc_checkpoint(cfg->scc, cp); - appletalk_checkpoint(cp); - via_checkpoint(cfg->via1, cp); - via_checkpoint(cfg->via2, cp); + mac030_checkpoint_save_core(cfg, cp); adb_checkpoint(st->adb, cp); mac_checkpoint_save_images(cfg, cp); scsi_checkpoint(cfg->scsi, cp); diff --git a/src/machines/mac030/mac030_glue.h b/src/machines/mac030/mac030_glue.h index 51c25abc..13aa5809 100644 --- a/src/machines/mac030/mac030_glue.h +++ b/src/machines/mac030/mac030_glue.h @@ -156,6 +156,28 @@ typedef struct mac030_board_desc { // memory_layout_tail. One motherboard design, one function. void mac030_glue_memory_layout(config_t *cfg, const mac030_board_desc_t *desc); +// Write the head of a 68k family's checkpoint stream, in the one canonical +// order: +// +// mem_map -> cpu -> scheduler -> cfg->irq -> rtc -> scc -> appletalk -> +// via1 -> via2 +// +// The stream is positional -- no per-block tag, no size field -- so this +// sequence IS the file format, and a family that replicated it could silently +// write a different one by dropping a line. That is not hypothetical: the +// review's F-23 was exactly this, a copy that omitted appletalk_checkpoint. +// +// Each family calls this first, then writes its own devices in its own +// construction order. That ordering rule is the family's to keep: save must +// mirror what its init reads back, because a swapped pair does not fail at +// the swap, it cross-loads and dies later at whichever block first disagrees +// on size (see the IIfx, which did exactly that). +// +// The PowerPC families do NOT use this: their stream substitutes +// ppc_checkpoint for cpu_checkpoint and omits cfg->irq, because PDM and TNT +// keep interrupt state in their own register blobs instead. +void mac030_checkpoint_save_core(config_t *cfg, checkpoint_t *cp); + // Create the 68030 PMMU over a board's ROM window, make it the global MMU and // attach it to the CPU. Returns the MMU; the caller sets any TT registers. struct mmu_state *mac030_build_mmu(config_t *cfg, uint32_t rom_base, uint32_t rom_end); diff --git a/src/machines/mcu/mcu.c b/src/machines/mcu/mcu.c index 324d6990..51de5761 100644 --- a/src/machines/mcu/mcu.c +++ b/src/machines/mcu/mcu.c @@ -764,15 +764,7 @@ static void mcu_teardown(config_t *cfg) { static void mcu_checkpoint_save(config_t *cfg, checkpoint_t *cp) { mcu_state_t *st = mcu_st(cfg); - memory_map_checkpoint(cfg->mem_map, cp); - cpu_checkpoint(cfg->cpu, cp); // includes the 040 MMU register file - scheduler_checkpoint(cfg->scheduler, cp); - system_write_checkpoint_data(cp, &cfg->irq, sizeof(cfg->irq)); - rtc_checkpoint(cfg->rtc, cp); - scc_checkpoint(cfg->scc, cp); - appletalk_checkpoint(cp); - via_checkpoint(cfg->via1, cp); - via_checkpoint(cfg->via2, cp); + mac030_checkpoint_save_core(cfg, cp); adb_checkpoint(st->adb, cp); mac_checkpoint_save_images(cfg, cp); // Device order mirrors the build_devices construction order exactly diff --git a/src/machines/mdu/mdu.c b/src/machines/mdu/mdu.c index da59ce57..d2c88394 100644 --- a/src/machines/mdu/mdu.c +++ b/src/machines/mdu/mdu.c @@ -152,14 +152,7 @@ static void mdu_teardown(config_t *cfg) { static void mdu_checkpoint_save(config_t *cfg, checkpoint_t *cp) { mac030_mdu_state_t *st = mdu_st(cfg); - memory_map_checkpoint(cfg->mem_map, cp); - cpu_checkpoint(cfg->cpu, cp); - scheduler_checkpoint(cfg->scheduler, cp); - system_write_checkpoint_data(cp, &cfg->irq, sizeof(cfg->irq)); - rtc_checkpoint(cfg->rtc, cp); - scc_checkpoint(cfg->scc, cp); - appletalk_checkpoint(cp); - via_checkpoint(cfg->via1, cp); + mac030_checkpoint_save_core(cfg, cp); adb_checkpoint(st->adb, cp); if (st->egret) // IIsi only; IIci leaves egret NULL egret_checkpoint(st->egret, cp); diff --git a/src/machines/oss/iifx.c b/src/machines/oss/iifx.c index 59029844..f09a8d85 100644 --- a/src/machines/oss/iifx.c +++ b/src/machines/oss/iifx.c @@ -1663,14 +1663,7 @@ static void iifx_teardown(config_t *cfg) { // Saves an IIfx checkpoint. static void iifx_checkpoint_save(config_t *cfg, checkpoint_t *cp) { iifx_state_t *st = iifx_state(cfg); - memory_map_checkpoint(cfg->mem_map, cp); - cpu_checkpoint(cfg->cpu, cp); - scheduler_checkpoint(cfg->scheduler, cp); - system_write_checkpoint_data(cp, &cfg->irq, sizeof(cfg->irq)); - rtc_checkpoint(cfg->rtc, cp); - scc_checkpoint(cfg->scc, cp); - appletalk_checkpoint(cp); - via_checkpoint(cfg->via1, cp); + mac030_checkpoint_save_core(cfg, cp); mac_checkpoint_save_images(cfg, cp); scsi_checkpoint(cfg->scsi, cp); // Save order must mirror iifx_init's construction order exactly: the From 0b74d15f2f445763f35b6ae9f82e0d183402d378 Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:07:56 +0000 Subject: [PATCH 11/12] Code review 2026-09-03: share the 68k low-speed construction spine (F-29) Five families each built RTC, SCC and the AppleTalk stack with the same four lines, the same scc_set_clocks(7833600, 3686400) literals and the same verbatim four-line comment. mac030_build_lowspeed(cfg, cp, scc_irq) replaces them; NULL selects the family default, which subsumes the MCU's `board->scc_irq ? board->scc_irq : mac030_glue_scc_irq` exactly. Narrower than the finding proposes, and for a different reason than it gives. All three of F-29's motivating claims are stale. "This is where F-12 lives: via_set_exact_clock was added to the two PPC families and to none of these five" -- branch 1 added it to mdu, iifx, mcu (both VIAs) and av. "IIfx hardcodes 51" -- it calls via_freq_factor_for_clock. "GLUE hardcodes 20" -- true, and correct: 15667200 / 783360 is exactly 20, which is why branch 1's F-12 pass rightly skipped it and why it needs no exact-clock call. So the stated payoff, closing F-12 and F-42 structurally, was already banked. The VIAs deliberately stay per-family. The finding's proposed mac030_build_lowspeed(cfg, cp, hooks, n_vias, scc_irq) would need a parameter list about as long as the code it absorbs, because the variation there is real: one VIA or two, different port hooks, different IRQ sinks, and the GLUE machines' exact 20:1 ratio against the derived factor everyone else needs. The argument the finding misses is the better one, and it is now in the header. Those four lines each consume a checkpoint block as they build, so this sequence is the READ side of the stream mac030_checkpoint_save_core() writes -- construction order here is restore order. Until the previous two commits, the save half was shared while the restore half sat in five copies; that asymmetry is exactly how the IIfx drifted out of order and broke every checkpoint.load on that machine. Both halves are now one function each, and each says the other must change with it. The MCU's comment about the towers intercepting the SCC chip INT is kept at the new call site rather than lost -- it explains the argument now being passed. Verified: make headless and make both clean. 59 integration rows, 0 failures -- the unit tier plus suite-se30/iicx/iix/iici/iisi/iifx/quadra/av, q900-checkpoint and boot-config, including six checkpoint round-trips, which are the rows that would catch a construction/restore order slip. Co-Authored-By: Claude Opus 5 (1M context) --- src/machines/av/av.c | 9 +-------- src/machines/mac030/mac030_glue.c | 31 +++++++++++++++++++++++-------- src/machines/mac030/mac030_glue.h | 19 +++++++++++++++++++ src/machines/mcu/mcu.c | 12 +++--------- src/machines/mdu/mdu.c | 9 +-------- src/machines/oss/iifx.c | 9 +-------- 6 files changed, 48 insertions(+), 41 deletions(-) diff --git a/src/machines/av/av.c b/src/machines/av/av.c index 46377a47..c705979f 100644 --- a/src/machines/av/av.c +++ b/src/machines/av/av.c @@ -774,14 +774,7 @@ static int av_init(config_t *cfg, checkpoint_t *cp) { if (cp) system_read_checkpoint_data(cp, &cfg->irq, sizeof(cfg->irq)); - cfg->rtc = rtc_init(cfg->scheduler, cp, true); - cfg->scc = scc_init(NULL, cfg->scheduler, av_scc_irq, cfg, cp); - scc_set_clocks(cfg->scc, 7833600, 3686400); - - // AppleTalk rides the SCC's LocalTalk channel, so it is built as soon as - // the SCC exists — and, because the checkpoint stream is positional, in - // the same relative place the save writes it (right after scc_checkpoint). - appletalk_init(cfg->scheduler, cfg->scc, cp); + mac030_build_lowspeed(cfg, cp, av_scc_irq); uint8_t via_ff = via_freq_factor_for_clock(cfg->machine->freq); cfg->via1 = diff --git a/src/machines/mac030/mac030_glue.c b/src/machines/mac030/mac030_glue.c index 67995ee1..a0b9ca8d 100644 --- a/src/machines/mac030/mac030_glue.c +++ b/src/machines/mac030/mac030_glue.c @@ -152,6 +152,28 @@ void mac030_checkpoint_save_core(config_t *cfg, checkpoint_t *cp) { via_checkpoint(cfg->via2, cp); } +// Build the low-speed spine every 68k family shares: the RTC, the SCC at the +// Mac's clocks, and the AppleTalk stack that rides its LocalTalk channel. +// +// This is the READ side of the stream mac030_checkpoint_save_core() writes, +// and the two must stay in step: construction order here is restore order, +// because rtc_init, scc_init and appletalk_init each consume their own block +// from the checkpoint as they build. Keeping both halves in one function +// each is the point -- when the save half was shared and the restore half was +// copied per family, the IIfx drifted out of order and every checkpoint.load +// on that machine failed. +// +// `scc_irq` is the only genuine per-family variation at this level; the VIAs +// below it differ enough (one or two, different hooks, different IRQ sinks) +// that they stay with each family. +void mac030_build_lowspeed(config_t *cfg, checkpoint_t *cp, void (*scc_irq)(void *, bool)) { + cfg->rtc = rtc_init(cfg->scheduler, cp, true); + cfg->scc = scc_init(NULL, cfg->scheduler, scc_irq ? scc_irq : mac030_glue_scc_irq, cfg, cp); + // 3.6864 MHz PCLK / 7.8336 MHz RTxC -- the same pair on every 68k Mac. + scc_set_clocks(cfg->scc, 7833600, 3686400); + appletalk_init(cfg->scheduler, cfg->scc, cp); +} + // Finish init: debugger, scheduler start, cold-boot IRQ/IPL reset. void mac030_glue_finish(config_t *cfg, checkpoint_t *cp) { cfg->debugger = debug_init(); @@ -183,14 +205,7 @@ int mac030_glue_init(config_t *cfg, checkpoint_t *cp, const mac030_glue_board_t if (cp) system_read_checkpoint_data(cp, &cfg->irq, sizeof(cfg->irq)); - cfg->rtc = rtc_init(cfg->scheduler, cp, true); - cfg->scc = scc_init(NULL, cfg->scheduler, mac030_glue_scc_irq, cfg, cp); - scc_set_clocks(cfg->scc, 7833600, 3686400); - - // AppleTalk rides the SCC's LocalTalk channel, so it is built as soon as - // the SCC exists — and, because the checkpoint stream is positional, in - // the same relative place the save writes it (right after scc_checkpoint). - appletalk_init(cfg->scheduler, cfg->scc, cp); + mac030_build_lowspeed(cfg, cp, NULL); // NULL: the family-default SCC IRQ cfg->via1 = via_init(NULL, cfg->scheduler, 20, "via1", board->via1_output, board->via1_shift_out, mac030_glue_via1_irq, cfg, cp); diff --git a/src/machines/mac030/mac030_glue.h b/src/machines/mac030/mac030_glue.h index 13aa5809..de1bcb73 100644 --- a/src/machines/mac030/mac030_glue.h +++ b/src/machines/mac030/mac030_glue.h @@ -156,6 +156,25 @@ typedef struct mac030_board_desc { // memory_layout_tail. One motherboard design, one function. void mac030_glue_memory_layout(config_t *cfg, const mac030_board_desc_t *desc); +// Build the low-speed spine every 68k family shares: RTC, the SCC at the Mac's +// clocks (3.6864 MHz PCLK / 7.8336 MHz RTxC), and the AppleTalk stack that +// rides its LocalTalk channel. Pass NULL for `scc_irq` to take the family +// default (mac030_glue_scc_irq). +// +// This is the READ side of the stream mac030_checkpoint_save_core() writes +// below, and the pairing is the point: rtc_init, scc_init and appletalk_init +// each consume their own block from `cp` as they build, so construction order +// here IS restore order. While the save half was shared and the restore half +// was copied into five families, the IIfx drifted out of order and every +// checkpoint.load on that machine failed. Change one of these two functions +// and you must change the other. +// +// The VIAs are deliberately NOT here: one machine or two, different port +// hooks, different IRQ sinks, and the GLUE machines' exact 20:1 clock ratio +// versus the derived factor everyone else needs. That variation is real, and +// a parameter list long enough to absorb it would be longer than the code. +void mac030_build_lowspeed(config_t *cfg, checkpoint_t *cp, void (*scc_irq)(void *, bool)); + // Write the head of a 68k family's checkpoint stream, in the one canonical // order: // diff --git a/src/machines/mcu/mcu.c b/src/machines/mcu/mcu.c index 51de5761..23a2d305 100644 --- a/src/machines/mcu/mcu.c +++ b/src/machines/mcu/mcu.c @@ -633,16 +633,10 @@ static int mcu_init(config_t *cfg, checkpoint_t *cp) { if (cp) system_read_checkpoint_data(cp, &cfg->irq, sizeof(cfg->irq)); - cfg->rtc = rtc_init(cfg->scheduler, cp, true); // Towers intercept the SCC chip INT (OR with the SCC IOP host INT); - // the Q700 routes it straight to the level-4 source. - cfg->scc = scc_init(NULL, cfg->scheduler, board->scc_irq ? board->scc_irq : mac030_glue_scc_irq, cfg, cp); - scc_set_clocks(cfg->scc, 7833600, 3686400); - - // AppleTalk rides the SCC's LocalTalk channel, so it is built as soon as - // the SCC exists — and, because the checkpoint stream is positional, in - // the same relative place the save writes it (right after scc_checkpoint). - appletalk_init(cfg->scheduler, cfg->scc, cp); + // the Q700 routes it straight to the level-4 source, which is the + // family default the NULL branch selects. + mac030_build_lowspeed(cfg, cp, board->scc_irq); // Derived from the CPU clock (see the same note in mdu.c): the towers run // 25 MHz (Q700/Q900) and 33 MHz (Q950), so the previous hardcoded 20/21 — diff --git a/src/machines/mdu/mdu.c b/src/machines/mdu/mdu.c index d2c88394..a5172487 100644 --- a/src/machines/mdu/mdu.c +++ b/src/machines/mdu/mdu.c @@ -64,14 +64,7 @@ int mac030_mdu_init(config_t *cfg, checkpoint_t *cp, const mac030_mdu_board_t *b if (cp) system_read_checkpoint_data(cp, &cfg->irq, sizeof(cfg->irq)); - cfg->rtc = rtc_init(cfg->scheduler, cp, true); - cfg->scc = scc_init(NULL, cfg->scheduler, mac030_glue_scc_irq, cfg, cp); - scc_set_clocks(cfg->scc, 7833600, 3686400); - - // AppleTalk rides the SCC's LocalTalk channel, so it is built as soon as - // the SCC exists — and, because the checkpoint stream is positional, in - // the same relative place the save writes it (right after scc_checkpoint). - appletalk_init(cfg->scheduler, cfg->scc, cp); + mac030_build_lowspeed(cfg, cp, NULL); // NULL: the family-default SCC IRQ // Derived from the CPU clock, not hardcoded: this substrate serves the // 25 MHz IIci and the 20 MHz IIsi, so a single literal is wrong for one of diff --git a/src/machines/oss/iifx.c b/src/machines/oss/iifx.c index f09a8d85..912a2908 100644 --- a/src/machines/oss/iifx.c +++ b/src/machines/oss/iifx.c @@ -1503,14 +1503,7 @@ static int iifx_init(config_t *cfg, checkpoint_t *checkpoint) { if (checkpoint) system_read_checkpoint_data(checkpoint, &cfg->irq, sizeof(cfg->irq)); - cfg->rtc = rtc_init(cfg->scheduler, checkpoint, true); - cfg->scc = scc_init(NULL, cfg->scheduler, iifx_scc_irq, cfg, checkpoint); - scc_set_clocks(cfg->scc, 7833600, 3686400); - - // AppleTalk rides the SCC's LocalTalk channel, so it is built as soon as - // the SCC exists — and, because the checkpoint stream is positional, in - // the same relative place the save writes it (right after scc_checkpoint). - appletalk_init(cfg->scheduler, cfg->scc, checkpoint); + mac030_build_lowspeed(cfg, checkpoint, iifx_scc_irq); // Divisor derived from the profile clock rather than the literal 51 this // used to carry -- via.h asks for exactly that, since a literal that suits From 88384e9eb5d5f33ac72bea6b786cf83067948f33 Mon Sep 17 00:00:00 2001 From: pappadf <95528397+pappadf@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:20:42 +0000 Subject: [PATCH 12/12] Code review 2026-09-03: the IIfx is not bespoke; say so in the header (F-30) F-30 argues that oss/iifx.c is called "bespoke" in a public header twice, as if it were an architectural fact, so nobody revisits a 1700-line file that is mostly not bespoke. That part is right, and it is the part worth fixing. Most of the rest of the finding has already been overtaken. Of its six duplication claims, four are closed: the teardown chain (F-27), the checkpoint-save prologue (F-28), the init spine (F-29), and the missing nubus_checkpoint_save it attributes to F-03, which branch 1 had already fixed -- it is at iifx.c:1685. The IIfx now builds through mac030_build_core + mac030_build_lowspeed, checkpoints through mac030_checkpoint_save_core, and tears down through machine_teardown_config_devices. A fifth is genuine and taken here: the init tail was byte-identical to mac030_glue_finish and now calls it. The sixth is wrong, and worth recording as wrong. F-30 reads iifx_reset as "a five-statement duplicate of mac030_glue_reset with the overlay_flag / rom_start parameters inlined", and suggests replacing it outright. The five statements resemble each other, but the set_rom_overlay they call does not: iifx_set_rom_overlay restores the WRAPPED RAM mapping iifx_memory_layout_init installs rather than a linear one (equivalent today, and the comment there says to keep the two expressions in lock-step), fills through iifx_fill_page so read-only pages get their write-SoA entries zeroed, and re-arms the overlay trip-wire -- the ROM window sitting on the rom_interface device so the first genuine ROM access clears the overlay, which is real OSS behaviour, plus the FMC ROM-invert POST window re-point. That last part carries a measured justification: leaving the device in place cost about 3.6x whole-machine throughput, because Mac OS runs QuickDraw out of ROM. Taking the suggestion would have dropped all three silently. So the header comments are corrected rather than the code restructured. They now separate two things they had run together: a substrate serving one machine is a statement about how many machines share the board, not about how much code the family writes for itself. A NULL `board` reads as "this substrate serves exactly one machine" -- and the IIfx does define a mac030_board_desc_t; it simply has no second machine to vary against, so routing it through the profile would add a cast without adding sharing. Also not done, deliberately: the mac030_oss_board_t conversion the finding proposes. Its purpose was to unlock the sharing that F-27/F-28/F-29 have since delivered by other means, and for a one-machine family it buys indirection rather than reuse. What the IIfx keeps for itself now is what its hardware genuinely does differently. The finding estimated a day and about 250 lines; it came to ten, because three other commits had already taken the substance. Verified: make headless and make both clean. 52 integration rows, 0 failures -- the unit tier plus suite-iifx (including its checkpoint round-trip), iifx-mactest and iifx-marathon. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/machine_profile.h | 22 ++++++++++++++++++---- src/machines/oss/iifx.c | 7 +------ 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/core/machine_profile.h b/src/core/machine_profile.h index ff900205..68e49c1e 100644 --- a/src/core/machine_profile.h +++ b/src/core/machine_profile.h @@ -353,15 +353,29 @@ typedef struct hw_profile { const struct builtin_video_desc *builtin_video; // Behavior: the lifecycle + host-input vtable for this machine. Machines - // of the same chipset family SHARE one substrate (glue_substrate / - // mdu_substrate; iifx is bespoke). + // of the same chipset family SHARE one substrate (glue_substrate for + // SE/30-IIcx-IIx, mdu_substrate for IIci-IIsi, and so on). A family with + // one machine still gets its own -- the IIfx and both PowerPC families -- + // which is a statement about how many machines share the board, not about + // how much code the family writes for itself. + // + // "Bespoke substrate" is not "bespoke machine": every 68k family, the IIfx + // included, builds through mac030_build_core + mac030_build_lowspeed, + // checkpoints through mac030_checkpoint_save_core, and tears down through + // machine_teardown_config_devices. What a family keeps for itself is what + // its hardware actually does differently -- for the IIfx, the OSS + // interrupt controller, the FMC ROM-invert POST window, the SCSI DMA + // engine, and a ROM overlay that doubles as a trip-wire. const machine_substrate_t *substrate; // Per-machine board descriptor — chipset-family data the shared substrate // interprets (proposal §4.2.2/§4.4). Typed by convention: the family // substrate casts it to its concrete type (mac030_glue_board_t for - // GLUE/MDU). NULL for families whose substrate needs no board (Plus, - // Lisa, and the bespoke IIfx, which carry their data directly). + // GLUE/MDU). NULL where a substrate serves exactly one machine and can + // therefore reach its data directly (Plus, Lisa, IIfx). The IIfx does + // define a mac030_board_desc_t of its own -- it simply has no second + // machine to vary against, so routing it through here would add a cast + // without adding sharing. const void *board; } hw_profile_t; diff --git a/src/machines/oss/iifx.c b/src/machines/oss/iifx.c index 912a2908..4cd0a410 100644 --- a/src/machines/oss/iifx.c +++ b/src/machines/oss/iifx.c @@ -1594,12 +1594,7 @@ static int iifx_init(config_t *cfg, checkpoint_t *checkpoint) { via_redrive_outputs(cfg->via1); } - cfg->debugger = debug_init(); - scheduler_start(cfg->scheduler); - if (!checkpoint) { - cfg->irq = 0; - cpu_set_ipl(cfg->cpu, 0); - } + mac030_glue_finish(cfg, checkpoint); return 0; }