Code review 2026-09-03, branch 2: expansion-bus teardown ownership (F-22) by pappadf · Pull Request #139 · pappadf/granny-smith

pappadf · 2026-09-06T23:34:40Z

Continues the 2026-09-03 code review in report order, picking up from
HANDOVER.md at F-20. First code change on the branch; opened early so CI
starts while the rest of the list is worked.

F-22 — `system_destroy` owns both expansion buses

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 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. 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.

This moves the PCI teardown into system_destroy beside NuBus and deletes it
from tnt_teardown, so the invariant is stated once, in one place, for both.

What the call now jumps over

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: Grand Central is itself a pci_device_t
(tnt_gc_pci_attach), so it now runs after the PCI root is gone. Harmless today
— gc_pci_ops declares no .teardown and the store lives in tnt_state_t, not
in a PCI allocation — and there is now a comment at the carry saying so, since it
becomes a real bug the moment that op appears.

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.

Verification

make headless and make both build, no new warnings in either file
clang-format clean (pre-commit hook passed)
49/49 unit-tier integration tests pass against the pinned test-data
revision 0642091bbbc8, including tnt-pci-slots, ans-pci-slots,
machine-restart, ans-machine-restart, tnt-hd-boot, tnt-pci-voodoo2

Not run locally: the matrix and extended tiers. system_destroy runs for
every machine, so those are the tiers that would surface a teardown regression on
a family the unit tier does not cover — that is what this PR is opened early for.

Also on this branch (docs only, in gs-docs — not in this diff)

F-20 (two spellings of "this machine has no floppy") — superseded.
The empty floppy_slots[] tables on the AV Quadras are not a spelling
inconsistency; they are an honest report that av/new_age.c is a stub
reporting ST3 = $FF. Both machines shipped a 1.44 MB SuperDrive. Written up
as a new proposal for a real NEC µPD72070 model. Also: the finding's "field
absent → NULL" half no longer exists — all 22 profiles now set the field, which
F-01 fixed on branch 1.
F-21 (system_reset_devices resets SCSI and NuBus but not PCI) —
closed, folded into proposal-reset-and-nonvolatile-state.md. The omission
is real but the suggested fix would have added unreachable code:
system_reset_devices has exactly one caller (OP_RESET, needing a 68K core),
cfg->pci is set only by TNT, and TNT never constructs cfg->cpu. That
proposal's §3.1 was rewritten around the primary sources — /RESET is one
bidirectional net, so a machine reset is a bus reset plus a CPU reset, not a
second device list — which is where pci_reset belongs and is reachable.

🤖 Generated with Claude Code

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) <noreply@anthropic.com>

…-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) <noreply@anthropic.com>

…pdate_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) <noreply@anthropic.com>

…e 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 "<Machine> 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) <noreply@anthropic.com>

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) <noreply@anthropic.com>

…eaders (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 (<string.h>, 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) <noreply@anthropic.com>

…ies (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) <noreply@anthropic.com>

…-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) <noreply@anthropic.com>

pappadf and others added 8 commits September 6, 2026 23:34

Navigation Menu

Code review 2026-09-03, branch 2: expansion-bus teardown ownership (F-22) - #139

Code review 2026-09-03, branch 2: expansion-bus teardown ownership (F-22)#139
pappadf wants to merge 8 commits into
mainfrom
code-review-2026-09-03-b2

pappadf commented Sep 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

pappadf commented Sep 6, 2026

F-22 — system_destroy owns both expansion buses

What the call now jumps over

Verification

Also on this branch (docs only, in gs-docs — not in this diff)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

F-22 — `system_destroy` owns both expansion buses