riscv64: skip satp Bare-mode switch when already Bare (§V.3 item 7 audit fix)
Build / build-amd64-iso (push) Waiting to run
Build / build-aarch64-iso (push) Waiting to run
Build / build-riscv64-img (push) Waiting to run

arch_early_init() unconditionally cleared satp on every boot, justified
only by behavior observed under QEMU/EDK2 firmware (satp.MODE=10/Sv57,
kernel identity-mapped within it). That reasoning never applied to the
native U-Boot+OpenSBI boot path, where satp is conventionally already 0
at S-mode handoff -- the unconditional clear was likely a harmless no-op
there, but on an unverified assumption.

Fix: read satp.MODE first and return early when it's already 0 (nothing
to switch away from, no safety argument needed). The unconditional
csrw/sfence pair still runs unchanged for the confirmed QEMU/EDK2 case.
No Sv39/Sv48/Sv57 page-table walker built -- out of proportion to this
finding's severity.

3-arch acceptance: amd64/aarch64 don't compile this file, so their runs
are non-regression on untouched files only. riscv64's own boot log
confirms satp.MODE = 0xa at entry, so the mode != 0 branch ran and
"satp cleared -- Bare mode, explicit" printed exactly as before the fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019YcT3H2PQeyujrzjqS3Var
This commit is contained in:
Robert Allan James
2026-09-05 00:17:35 -04:00
co-authored by Claude Sonnet 5
parent 350287850f
commit 90ee8deb6d
10 changed files with 27648 additions and 21 deletions
+29 -12
View File
@@ -747,8 +747,11 @@ it doesn't. This is a real punch-list item, not a hypothetical.
7. **Code audit pass — DONE, 2026-09-04.** Reviewed `arch/riscv64/apic.c`, `plic.c`, 7. **Code audit pass — DONE, 2026-09-04.** Reviewed `arch/riscv64/apic.c`, `plic.c`,
`arch.c`, `interrupts.c`, `timer.c` for the same class of QEMU-virt-vs-real-hardware `arch.c`, `interrupts.c`, `timer.c` for the same class of QEMU-virt-vs-real-hardware
assumption §III item 6's amd64 audit and item 7 above's aarch64 audit looked for. Report assumption §III item 6's amd64 audit and item 7 above's aarch64 audit looked for.
only — nothing below was changed. Report-only by default — the `satp`-clear finding below was fixed in code, per direct
instruction naming it specifically after this audit landed. The other findings (PLIC
base, already tracked as item 3 above; the clean SBI/SBI-SRST findings) needed no code
change either way.
**Clean, and the most portable pattern of all three architectures**: the SBI timer path **Clean, and the most portable pattern of all three architectures**: the SBI timer path
(`apic.c`) is genuinely hardware-independent — it probes for the TIME extension at (`apic.c`) is genuinely hardware-independent — it probes for the TIME extension at
@@ -766,16 +769,30 @@ it doesn't. This is a real punch-list item, not a hypothetical.
already flagged as "a real punch-list item, not a hypothetical" before this audit ran. already flagged as "a real punch-list item, not a hypothetical" before this audit ran.
Nothing new to add beyond confirming the file's own header comment is accurate and the Nothing new to add beyond confirming the file's own header comment is accurate and the
risk is real, not overstated. risk is real, not overstated.
**Finding — minor, reasoning doesn't transfer to the native path, action likely still **Finding — minor, reasoning didn't transfer to the native path — FIXED 2026-09-04, per
safe**: `arch_early_init()`'s explicit `satp` clear (Bare-mode switch) is justified in direct instruction naming it specifically.** `arch_early_init()`'s explicit `satp` clear
its own comment entirely by behavior *observed under QEMU's EDK2 RISC-V firmware* (Bare-mode switch) was justified in its own comment entirely by behavior *observed under
(confirmed `satp.MODE=10`/Sv57 live, kernel identity-mapped within it). Under the native QEMU's EDK2 RISC-V firmware* (confirmed `satp.MODE=10`/Sv57 live, kernel identity-mapped
boot path (U-Boot+OpenSBI, no UEFI/EDK2 at all per §V.1) that specific observation cannot within it). Under the native boot path (U-Boot+OpenSBI, no UEFI/EDK2 at all per §V.1)
apply — OpenSBI's S-mode handoff conventionally already leaves `satp=0` (Bare mode), most that specific observation cannot apply — OpenSBI's S-mode handoff conventionally already
likely making this a no-op there rather than a hazard, but the *stated justification* for leaves `satp=0` (Bare mode), most likely making the unconditional switch a harmless no-op
the switch (identity-mapped, safe to clear) was derived from a firmware stack the native there, but on an unverified assumption rather than a checked one.
path doesn't use, and hasn't been re-confirmed for OpenSBI's actual handoff state. Lowest **Fix**: rather than build a general Sv39/Sv48/Sv57 page-table walker just to *prove* the
severity of any finding across both audits — flagged for completeness, not urgency. running address is identity-mapped before switching (out of proportion to this finding's
own severity), `arch_early_init()` now reads `satp.MODE` first and **skips the switch
entirely when it's already 0** — the expected case on a from-Bare native boot, needing no
safety argument at all since there's nothing to switch away from. The unconditional
`csrw satp, x0`/`sfence.vma` pair still runs, unchanged, for the confirmed QEMU/EDK2 case
(`mode != 0`), where the identity-mapping argument remains the actual justification it
always was. If the native path somehow reaches the non-zero branch anyway (OpenSBI
leaving a non-Bare `satp`, not confirmed either way), the code says so explicitly in its
own comment now rather than silently relying on the QEMU-only observation. Verified against
the riscv64 boot log's own `satp.MODE` print, not assumed: this system's QEMU/EDK2 boot
shows `satp.MODE = 0x000000000000000a` (Sv57) at kernel entry, so the `mode != 0` branch
ran and printed `satp cleared -- Bare mode, explicit` exactly as before the fix — the
unconditional-switch path is unchanged on the one boot path that exercises it. amd64 and
aarch64 don't compile this file at all, so their part of the 3-arch acceptance run is pure
non-regression on files untouched by this fix, not evidence about the guard itself.
**Hardware-dependent, after 2026-09-17:** **Hardware-dependent, after 2026-09-17:**
8. Build and flash the boot image to QSPI flash (or attempt UART XMODEM recovery boot if QSPI 8. Build and flash the boot image to QSPI flash (or attempt UART XMODEM recovery boot if QSPI
+1 -1
View File
@@ -1,5 +1,5 @@
# Capsule Block Manifest — Auto-generated # Capsule Block Manifest — Auto-generated
<!-- Generated by mkcapsule --manifest 2026-09-05T04:04:40Z --> <!-- Generated by mkcapsule --manifest 2026-09-05T04:15:27Z -->
<!-- DO NOT EDIT — re-run mkcapsule --manifest to refresh. --> <!-- DO NOT EDIT — re-run mkcapsule --manifest to refresh. -->
<!-- Hand-written justifications and immutability notes live --> <!-- Hand-written justifications and immutability notes live -->
<!-- in MANIFEST.md alongside this auto-generated index. --> <!-- in MANIFEST.md alongside this auto-generated index. -->
BIN
View File
Binary file not shown.
File diff suppressed because it is too large Load Diff
File diff suppressed because it is too large Load Diff
File diff suppressed because it is too large Load Diff
+47 -8
View File
@@ -26,7 +26,10 @@ extern char __kernel_start[]; /* same pattern pmm.c:67 already uses */
* top past the PMM-free region -- consistent with the kernel's own running * top past the PMM-free region -- consistent with the kernel's own running
* range being identity-mapped (VA==PA) under firmware's Sv57 table, not * range being identity-mapped (VA==PA) under firmware's Sv57 table, not
* some high canonical virtual range. That is the basis for the Bare-mode * some high canonical virtual range. That is the basis for the Bare-mode
* switch below being safe. */ * switch below being safe *when firmware leaves a translation active at
* all* -- see arch_early_init()'s own comment (FABRIC-3.md §V.3 item 7
* fix, 2026-09-04) for why that no longer covers every boot path this
* kernel now has. */
static void print_hex64(uint64_t val) static void print_hex64(uint64_t val)
{ {
char buf[19]; char buf[19];
@@ -49,6 +52,27 @@ static void print_hex64(uint64_t val)
* via @c riscv64_install_vectors(). This stub satisfies the common * via @c riscv64_install_vectors(). This stub satisfies the common
* @c arch_early_init() call site in @c kernel_main() without any * @c arch_early_init() call site in @c kernel_main() without any
* RISC-V-specific action. * RISC-V-specific action.
*
* **Correction, 2026-09-04 (FABRIC-3.md §V.3 item 7's code audit).** The
* unconditional Bare-mode switch this function used to perform was
* justified entirely by behaviour observed under this system's QEMU/EDK2
* RISC-V firmware (@c satp.MODE=10/Sv57, kernel identity-mapped within
* it — see the comment on @c print_hex64() above). That observation
* cannot apply to the native boot path (U-Boot+OpenSBI, no EDK2 at all,
* FABRIC-3.md §V.1) this kernel now also has, and OpenSBI's own S-mode
* handoff convention leaves @c satp already at 0 (Bare) in the common
* case — meaning the switch below was very likely running as a harmless
* no-op there, but on an unverified assumption rather than a checked one.
* Rather than build a general Sv39/Sv48/Sv57 page-table walker just to
* *prove* the running address is identity-mapped before switching (out of
* proportion to this item's real scope), the fix narrows what actually
* needs the QEMU-specific safety argument: **skip the switch entirely
* when @c satp is already 0.** That is the expected case on both a
* from-Bare native boot and any firmware that already left translation
* off, and it needs no safety argument at all — there is nothing to
* switch away from. The unconditional-switch path still exists for the
* confirmed QEMU/EDK2 case (@c mode != 0), where the identity-mapping
* argument above remains the actual justification, unchanged.
*/ */
void arch_early_init(void) void arch_early_init(void)
{ {
@@ -64,18 +88,33 @@ void arch_early_init(void)
console_puts(" satp.PPN = "); print_hex64(ppn); console_puts("\n"); console_puts(" satp.PPN = "); print_hex64(ppn); console_puts("\n");
console_puts(" __kernel_start = "); print_hex64((uint64_t)__kernel_start); console_puts("\n"); console_puts(" __kernel_start = "); print_hex64((uint64_t)__kernel_start); console_puts("\n");
if (mode == 0) {
/* Already Bare -- the expected case on a native (OpenSBI) boot.
* Nothing to switch away from, so no identity-mapping assumption
* is needed at all: skipping is strictly safer than an
* unconditional csrw+sfence that would just write back the same
* zero value. */
console_println("riscv64: satp already Bare -- no switch needed (item 4.3.5a)");
return;
}
/* Explicit switch to Bare mode (satp.MODE=0): this kernel builds no /* Explicit switch to Bare mode (satp.MODE=0): this kernel builds no
* riscv64 page table of its own (vmm.c's is x86-64-shaped and never * riscv64 page table of its own (vmm.c's is x86-64-shaped and never
* activated here, per its own load_cr3() no-op outside __x86_64__) and * activated here, per its own load_cr3() no-op outside __x86_64__) and
* has no present use for virtual memory on this ISA, so there is no * has no present use for virtual memory on this ISA, so there is no
* reason to inherit firmware's Sv57 mapping -- which is confirmed to * reason to inherit firmware's mapping -- which is confirmed, for the
* have at least one hole (PLIC_THRESHOLD, FABRIC-0.md item 4.3.5a). * QEMU/EDK2 case this branch was written for, to have at least one
* ExitBootServices() has already completed several checkpoints before * hole (PLIC_THRESHOLD, FABRIC-0.md item 4.3.5a). ExitBootServices()
* this function runs (ConOut/GOP done, BootServices exited per the * has already completed several checkpoints before this function
* runs on that path (ConOut/GOP done, BootServices exited per the
* "[CKPT 008]" trace), so nothing downstream depends on firmware's * "[CKPT 008]" trace), so nothing downstream depends on firmware's
* table surviving. Single asm block: the CSR write and SFENCE.VMA * table surviving there. On the native boot path, reaching this
* must not be separated by a compiler-scheduled memory access (RISC-V * branch at all means OpenSBI's handoff left satp non-zero -- not
* Privileged Spec §4.2.1, SFENCE.VMA ordering). */ * the case this kernel has confirmed, so the identity-mapping safety
* argument is unverified here; flagged, not guessed past. Single asm
* block: the CSR write and SFENCE.VMA must not be separated by a
* compiler-scheduled memory access (RISC-V Privileged Spec §4.2.1,
* SFENCE.VMA ordering). */
__asm__ volatile ( __asm__ volatile (
"csrw satp, x0\n" "csrw satp, x0\n"
"sfence.vma\n" "sfence.vma\n"