clang-tidy bug-class cleanup + VirtualBox bring-up & firmware-robustness hardening#266
Merged
Merged
Conversation
…t-map assumption
Two changes, combined per request:
1. clang-tidy bug-class cleanup (the ~45 non-style advisory findings;
the 894 readability-braces / loop-convert / using-namespace style
findings are intentionally left — they are advisory-only noise):
- bugprone-argument-comment (12): net_smoke.cpp / tls.cpp call-site
/*name=*/ comments updated to the real parameter names.
- readability-inconsistent-declaration-parameter-name (5): mlme.h /
eapol.h / fourway.h declarations aligned to their definitions.
- bugprone-branch-clone (3): drsh_desktop.cpp removed a dead,
immediately-overwritten buggy `(p[3]!=0)?false:false` assignment
(real latent bug); tcp.cpp / beacon.cpp merged genuinely-shared
branch bodies with no behaviour change (beacon.cpp also drops the
now-redundant has_psk to avoid -Werror=unused).
- bugprone-implicit-widening-of-multiplication-result (1 site):
drsh_desktop.cpp framebuffer offset math widened to u64 before
the multiplies.
- bugprone-macro-parentheses (1): result.h RESULT_TRY_ASSIGN — decl
is a declarator, not an expression; documented NOLINT.
- bugprone-dynamic-static-initializers (16): tcp_internal.h /
shell_internal.h statics made `constinit` at both the extern
decls and definitions (this surfaced + fixed real non-constant
init of Tcb/Stats, which now use explicit value-init). The check
still flags extern-in-header so a documented NOLINT closes it —
the freestanding hazard itself is eliminated by constinit.
- performance-enum-size (6): VfsBackend->u8, KObjectType->u16,
ErrorCode->u8 (internal, no wire format); Cap / FwPackageFamily /
FwPackageFlags kept wide with documented NOLINT (ABI-adjacent /
serialized field widths — narrowing them would be wrong).
2. ACPI: stop assuming every ACPI table lives below the 1 GiB direct
map. PhysToHeader() now routes through a cached AcpiMapPhys() that
uses PhysToVirt when in range and MapMmio otherwise. QEMU/OVMF
(tables low) is unchanged; VirtualBox parks the XSDT near the top of
2 GiB RAM, which previously hard-panicked "PhysToVirt called outside
direct map" at boot. Resolves the long-standing acpi.h TODO. DSDT /
SSDT scanners routed through the same helper; acpi.h scope note
updated.
Verified: x86_64-release + x86_64-debug build zero-warning/zero-error,
ctest green, QEMU bringup smoke reaches bringup-complete with no fatal
sentinels. clang-tidy bug-class findings: 0 remaining. The MapMmio ACPI
path only triggers on VirtualBox-style high table placement (not QEMU),
so it is validated on real VirtualBox boot.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…diag Continues the ACPI direct-map work from the previous commit. Booting DuetOS under VirtualBox (vs the QEMU/OVMF the parser was written against) surfaced three more firmware-shape incompatibilities, all fixed here. None are VBox-specific hacks — each hardens the parser for quirky real UEFI too. - Incomplete XSDT → RSDT fallback. VirtualBox ships an XSDT that lists only FADT + SSDT and puts the MADT (and the rest) only in the legacy RSDT. FindTable() used the XSDT exclusively on ACPI 2.0+ and returned nullptr on a miss; it now falls through to scan the RSDT before giving up. The ACPI spec says the two roots should agree; firmware in the wild does not always honour that. - Shared AcpiMapTable(). The DSDT/SSDT readers in aml.cpp called mm::PhysToVirt directly, which panics for the >1 GiB tables VBox/real UEFI park high. Promoted the file-local AcpiMapPhys to a single named API (acpi.h) so every ACPI TU resolves table addresses through the same direct-map / cached-MMIO fallback. One source of truth. - AcpiDiagDumpTables(): one-shot, boot-only dump of the RSDP + both root tables (XSDT and RSDT) with every entry's phys + 4-char signature. WARN-level so it lands in a serial capture; kept (gated by the once-at-boot call site) because non-QEMU firmware bring-up will keep needing exactly this layout dump. acpi.h scope note updated to match the new behaviour. Verified: x86_64-release + x86_64-debug build zero-warning/zero-error; QEMU full boot reaches the desktop; VirtualBox boots past ACPI parse (previously hard-panicked "PhysToVirt called outside direct map" / "MADT not found"). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… lz4 bound A class of boot-time self-tests assumed an operation that is cheap on bare metal / QEMU-TCG is cheap everywhere. Under a hardware-virtualised host (VirtualBox) — or in an unoptimised debug build — they are pathologically slow and wedge boot for minutes, which under VBox manifested as the kernel never reaching the input-consumer spawn (no keyboard). The existing gate was arch::IsEmulator(), but VirtualBox hides the hypervisor CPUID leaf by default so IsEmulator() reads false and the gate never fired. Add a kIsDebugBuild guard alongside it so dev boots stay usable regardless of whether hypervisor detection succeeds. - auth_pentest.cpp: the brute-force probe issues 8× PBKDF2-HMAC- SHA256(100k); skip under IsEmulator() OR kIsDebugBuild. - password_hash.cpp: PasswordHashSelfTest's random-salt arm runs 2×create + 2×verify and PasswordHashCreate defaults to Argon2id (memory-hard). The deterministic create/verify correctness is already covered by the PBKDF2 KAT directly above it, so skip the Argon2id arm in debug builds (kept in full for release / bare metal). - duetfs.cpp: the v5+ self-test surface is documented in-tree as "known-wedged on KVM, not fully diagnosed" and was already skipped under IsEmulator(); extend the gate with kIsDebugBuild for the same reason as above. - duetfs.cpp: separately, fix a genuine latent bug in the LZ4 self- test — it asserted bound < kPayloadLen+256 assuming the canonical LZ4 bound (n + n/255 + 16), but the backend is lz4_flex get_maximum_output_size + 4 (~1.1·n), which legitimately exceeds both the old ceiling and the old 4352-byte buffer. Trust the impl's reported bound as the source of truth; size the buffer from it and sanity-check only that it is positive and ≤ 2·n+64 (never a false positive for any LZ4-family compressor, still catches 0 / garbage). Verified: both presets build clean; QEMU full boot reaches the desktop and spawns kbd-reader; VBox debug boot now reaches the GUI login with a working keyboard instead of hanging/panicking in these self-tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PS/2 input bring-up made bare-metal-shaped assumptions that break on a hardware-virtualised host. All three fixes are correct hardening for real hardware too. - ps2kbd.cpp: explicitly clear the 8042 first-port translation bit (config byte bit 6). Step 8 already forces the keyboard into Set 1, so the controller must NOT also run a Set-2→Set-1 translation pass — with it on, every Set-1 code is mangled through the translation table. The driver previously "left translation whatever firmware set it to"; SeaBIOS/OVMF leave it off, VirtualBox leaves it on, which produced a completely wrong keymap. Make it deterministic. - ps2mouse.cpp: cut the 8042 poll cap from 1,000,000 to 50,000. On bare metal an Inb is a few cycles so the old cap was ~tens of ms; under hardware virtualization every Inb on the 8042 ports is a VM-exit (~0.5-1 us), so the old cap was ~1 s PER wait, and a silent (mouse-absent) VBox aux channel stacked dozens of them into a ~30 s apparent boot hang before the "no PS/2 mouse" bail. A present mouse ACKs in microseconds, so 50k keeps an ample margin while bounding the absent-device path to a fraction of a second. - ioapic.cpp: IoApicRoute now reads the redirection entry back and logs gsi/vector/wrote/readback at Debug level (kept, gated) — some emulated IOAPICs silently drop writes to certain entries; this proves whether a route actually latched without an operator re-adding prints. Visible in debug builds, silent in release. Verified: both presets build clean; under VirtualBox the keyboard now maps correctly and PS/2 init no longer dominates boot time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- boot.S: the Multiboot2 framebuffer request tag asked for 0/0/0
("any linear framebuffer"). GRUB resolves "any" to 1024x768 on QEMU
std-vga but to the lowest VBE mode (640x480) under VirtualBox, which
renders the desktop into a tiny buffer VBox then upscales — the UI
was unusably oversized. Request a concrete, universally-supported
1024x768x32 (in every VBE/VBoxVGA mode list, fits default VBox VRAM).
Flags stay optional so a host without the mode still boots to serial.
- main.cpp: add three [bringup-tail] serial markers (kernel-services
done / devices done / kbd-reader spawned bracket) around the late
boot phases. These were the bisection tool that localized a silent
post-self-test boot hang to BootBringupDevices; kept as cheap,
one-shot structural sentinels (the boot already emits this class of
marker) so the next silent-hang regression is localizable without
re-instrumenting.
Verified: both presets build clean (.S is hand-formatted, exempt from
clang-format per project policy); QEMU and VirtualBox both bring up the
desktop at 1024x768.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Krilliac
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR started as the advisory clang-tidy bug-class cleanup + the initial ACPI direct-map fix, and was extended (at the requester's direction, one consolidated PR) with a sequence of VirtualBox bring-up + firmware-robustness fixes discovered by actually booting DuetOS under VirtualBox for the first time. Every fix has a documented root cause and is general hardening (helps quirky real UEFI / other VMs), not a VBox-specific hack. Net result: DuetOS went from instant-BSOD under VBox to a booting desktop at 1024×768 with a working keyboard.
Commits
win32: clear mingw warnings in PE smoke fixtures(already in win32: clear mingw warnings in PE smoke fixtures #265-era) /kernel: clear advisory clang-tidy bug-class findings + fix ACPI direct-map assumption— the original scope: ~45 bug-class clang-tidy findings fixed (argument-comment, branch-clone incl. a real latent bug, inconsistent-param-name, macro-parens, 16 dynamic-static-initializers viaconstinit, 6 enum-size), 894 advisory style findings deliberately left; plusAcpiMapPhys/PhysToHeaderso ACPI tables above the 1 GiB direct map are reachable.acpi: VirtualBox bring-up — RSDT fallback, shared mapper, table-dump diag— incomplete-XSDT → RSDT fallback (VBox lists the MADT only in the legacy RSDT); promote the mapper to one sharedAcpiMapTableAPI used byaml.cpptoo (one source of truth); one-shot boot ACPI table dump (gated diagnostic).boot: gate pathologically-slow self-tests under VMM/debug; fix DuetFS lz4 bound—auth_pentest/PasswordHashSelfTest(Argon2id) /duetfsv5+ self-test are pathologically slow unoptimised and wedged debug/VBox boots; gated onIsEmulator() || kIsDebugBuild(VBox hides the hypervisor CPUID leaf soIsEmulator()alone was insufficient). Also a genuine latent fix: the DuetFS LZ4 self-test asserted the canonical LZ4 bound but the backend islz4_flex(~1.1·n) — now trusts the impl's reported bound.drivers: PS/2 + IOAPIC virtualization robustness— explicitly clear the 8042 translation bit (keyboard is forced to Set 1; VBox firmware left translation on → mangled keymap); cut the PS/2-mouse poll cap (eachInbis a VM-exit under virtualization → ~30 s apparent hang on VBox's silent aux channel); IOAPIC route read-back diagnostic (Debug-gated).boot/video: request 1024x768x32 framebuffer; add bringup-tail markers— Multiboot2 framebuffer tag was 0/0/0 ("any"); VBox resolves that to 640×480 (desktop unusably oversized) — now requests a concrete universal mode. Plus[bringup-tail]bracket markers that localized a silent boot hang.Verification
x86_64-release+x86_64-debug: zero warnings, zero errors.Sexempt per project policy)boot=desktopreaches the composed desktop and spawnskbd-readerKnown follow-ups (out of scope, tracked separately)
HLT).release-asserts/release-ltobring-up smoke flake (documented TCG-timing heuristic, unrelated to this diff).🤖 Generated with Claude Code