From b0c5640e5c1acca36d69af551a3cacfc7d129168 Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 20:34:37 +0200 Subject: [PATCH 1/3] Kernel: stop reserving the stack's image offset as a physical range `KernelArgs::kernel_stack_addr` is an offset into the kernel image, not an address. The loader writes `StackedImage::place`'s `stack` there (bootloader/src/main.rs, `stack_offset: placed.stack`), and both entries read it that way: x86-64 `_start` adds it to `kernel_memory_addr`, and the AArch64 `_start` loads it under the name `stack_offset`. `kernel_main` alone read it as physical and reserved `[kernel_stack_addr, +8 MiB)`. The kernel is a static PIE at base 0, so the offset is the image's `vaddr` extent rounded up to a page: 0x42c000 for main's x86-64 kernel, measured with llvm-readobj -l on target/kernel-x86_64-4be7ccf4859a9aa6 (the last PT_LOAD ends at 0x200240 + 0x22b940 = 0x42bb80). The bogus region [0x42c000, 0xc2c000) touches the 2 MiB frames at 0x400000, 0x600000, 0x800000, 0xa00000 and 0xc00000, so the PMM withheld each of those five (up to 10 MiB) that lies wholly inside a usable firmware entry, and nothing used them. On QEMU's AArch64 `virt` machine RAM starts at 0x4000_0000, so the range reached no RAM there. The real stack was never at risk. The loader allocates the image and the stack as one block: `mem_size = placed.size = stack + stack_size`, and it passes that block as `kernel_memory_size`. So the stack is the image's tail, [kernel_memory_addr + stack, kernel_memory_addr + kernel_memory_size), and the image's own reservation already keeps it. The line is deleted rather than corrected, because a corrected version would duplicate the image reservation. Two checks replace it: - an assert that `kernel_stack_addr + kernel_stack_size <= kernel_memory_size`, the loader fact the deletion relies on; - every reservation except the architecture's own page must lie inside one `LoaderData` descriptor of the firmware map. Each region in that list is an allocation made by the loader: the image, the ELF, the black-box page and ROOT. A region not held that way withholds memory nothing uses. This is the check that refuses the deleted line if it is ever reinstated. The architecture's page moves to the end of the array so the check can exclude it by position. The boot record printed the offset in an address's slot. It now reads `stack image++`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j --- kernel/src/main.rs | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/kernel/src/main.rs b/kernel/src/main.rs index ef64ca60a95..6e806d83849 100644 --- a/kernel/src/main.rs +++ b/kernel/src/main.rs @@ -304,7 +304,7 @@ pub(crate) unsafe extern "C" fn kernel_main(kernel_args: &KernelArgs) -> ! { // Split into six records: KernelArgs' derived Debug is the one message that exceeds the log's per-record bound. log!( - "boot: memory map {:#x}+{:#x}, kernel {:#x}+{:#x}, stack {:#x}+{:#x}", + "boot: memory map {:#x}+{:#x}, kernel {:#x}+{:#x}, stack image+{:#x}+{:#x}", kernel_args.memory_map_addr, kernel_args.memory_map_size, kernel_args.kernel_memory_addr, kernel_args.kernel_memory_size, kernel_args.kernel_stack_addr, kernel_args.kernel_stack_size @@ -348,11 +348,16 @@ pub(crate) unsafe extern "C" fn kernel_main(kernel_args: &KernelArgs) -> ! { ); let kernel_args = &kernel_args; + // `kernel_stack_addr` is an offset into the image, so the image's region is what keeps the stack. + assert!( + kernel_args.kernel_stack_addr.checked_add(kernel_args.kernel_stack_size) + .is_some_and(|end| end <= kernel_args.kernel_memory_size), + "boot: the loader put the stack at image+{:#x}+{:#x}, past the {:#x}-byte image", + kernel_args.kernel_stack_addr, kernel_args.kernel_stack_size, kernel_args.kernel_memory_size + ); let reserved = [ mm::Region { start: kernel_args.kernel_memory_addr, end: kernel_args.kernel_memory_addr + kernel_args.kernel_memory_size }, mm::Region { start: kernel_args.kernel_elf_addr, end: kernel_args.kernel_elf_addr + kernel_args.kernel_elf_size }, - mm::Region { start: kernel_args.kernel_stack_addr, end: kernel_args.kernel_stack_addr + kernel_args.kernel_stack_size }, - arch::boot::reserved(), // The loader's black-box page, which is ordinary `LoaderData` and so // memory the allocator would otherwise hand out. Empty on a boot whose // parameter line names none. @@ -360,7 +365,19 @@ pub(crate) unsafe extern "C" fn kernel_main(kernel_args: &KernelArgs) -> ! { // ROOT's image, `LoaderData` like the black box's page. Empty on a // boot the loader handed none. root_image, + // Last: the one region the loader did not allocate. + arch::boot::reserved(), ]; + // A region the loader did not allocate withholds memory nothing uses, so one the firmware map does not hold as `LoaderData` is refused. + for region in reserved[..reserved.len() - 1].iter().filter(|r| r.start < r.end) { + assert!( + maps.iter().any(|e| { + e.uefi_type == toyos_bootmap::EFI_LOADER_DATA && e.start <= region.start && region.end <= e.end + }), + "boot: reserving {:#x}..{:#x}, which no LoaderData descriptor in the firmware map holds", + region.start, region.end + ); + } // The last point before the first hash container (`mm::init`'s address // space), and not earlier: seeding fails only by panicking, and a panic From 4126414154da168db4cd82dc3628bc560e66acd4 Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 22:09:23 +0200 Subject: [PATCH 2/3] Round-1 review fixes: the containment check calls the tested held(), not a copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inline predicate in kernel_main's reservation loop duplicated toyos_rootimage::handoff::held without its checked_add, and no test covered the copy. Both crates were already kernel dependencies, so the loop now builds a Descriptor iterator and calls held(..., block=1) — block 1 because the ELF region is not page-aligned — the same call kernel/src/rootfs.rs already makes for ROOT's image. The architecture's own page is now named (a `loader` array of the four loader allocations, checked, with arch::boot::reserved() appended after) so a later region can't land inside the exempted slot by position. Files the misleading kernel_stack_addr name (toyos-abi/src/boot.rs:8) as an issue for after #583, and removes the tracker entry this branch's exit condition already closes. Co-Authored-By: Claude Opus 5.5 --- ...ack_addr-names-an-offset-not-an-address.md | 18 +++++++++++++ ...s-its-stack-offset-as-a-physical-region.md | 26 ------------------- kernel/src/main.rs | 22 +++++++++++----- 3 files changed, 33 insertions(+), 33 deletions(-) create mode 100644 issues/kernel/kernel_stack_addr-names-an-offset-not-an-address.md delete mode 100644 issues/kernel/the-kernel-reserves-its-stack-offset-as-a-physical-region.md diff --git a/issues/kernel/kernel_stack_addr-names-an-offset-not-an-address.md b/issues/kernel/kernel_stack_addr-names-an-offset-not-an-address.md new file mode 100644 index 00000000000..6b39d18e59d --- /dev/null +++ b/issues/kernel/kernel_stack_addr-names-an-offset-not-an-address.md @@ -0,0 +1,18 @@ +--- +status: open +kind: defect +opened: 2026-09-28 +--- + +# `kernel_stack_addr` names an offset, not an address + +`toyos_abi::boot::KernelArgs::kernel_stack_addr` (`toyos-abi/src/boot.rs:8`) +holds an offset into the kernel's own image — `StackedImage::place`'s `stack` +— never a physical address. Both architectures' entry code already treat it +as an offset (`kernel/src/arch/aarch64/boot.rs` reads it into a local named +`stack_offset`); `kernel/src/main.rs` reads it under its `KernelArgs` name and +must not repeat the confusion the name invites. + +Owner: whoever lands #583, which is already changing `KernelArgs`'s layout. +Exit: the field renamed to `kernel_stack_offset` (or equivalent), landed as +part of that change so `KernelArgs` moves once. diff --git a/issues/kernel/the-kernel-reserves-its-stack-offset-as-a-physical-region.md b/issues/kernel/the-kernel-reserves-its-stack-offset-as-a-physical-region.md deleted file mode 100644 index 64db710285a..00000000000 --- a/issues/kernel/the-kernel-reserves-its-stack-offset-as-a-physical-region.md +++ /dev/null @@ -1,26 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-09-27 ---- - -# The kernel reserves its stack's offset as a physical region - -`KernelArgs::kernel_stack_addr` is an offset into the kernel's own allocation: -the loader sets it to `StackedImage::place`'s `stack` (`bootloader/src/main.rs`), -and both entries add it to `kernel_memory_addr` before loading the stack -pointer (`kernel/src/arch/x86_64/boot.rs`'s `_start`, -`kernel/src/arch/aarch64/boot.rs`). - -`kernel/src/main.rs`'s `reserved` list takes it as an address: -`mm::Region { start: kernel_stack_addr, end: kernel_stack_addr + -kernel_stack_size }`. On the rust-lld kernel that is physical -`0x411000..0xc11000`, low memory that is nobody's stack, withheld from the -allocator for the whole boot. The real stack is inside `kernel_memory_addr .. -+ kernel_memory_size`, which the first entry of the same list already reserves, -so nothing is handed out twice; the defect is 8 MiB of memory reserved for -nothing, at whatever physical range the image's size names. - -Exit: the region goes, or names `kernel_memory_addr + kernel_stack_addr`, and -a host-checkable decision or a boot assertion states which physical ranges the -reserved list covers. diff --git a/kernel/src/main.rs b/kernel/src/main.rs index 6e806d83849..0139931e8fe 100644 --- a/kernel/src/main.rs +++ b/kernel/src/main.rs @@ -118,6 +118,7 @@ use arch::{cpu, percpu, smp}; pub(crate) use arch::hw; use drivers::{acpi, gop, nvme, pci, serial, virtio_console, virtio_gpu, virtio_sound, xhci}; use toyos_abi::boot::{KernelArgs, MemoryMapEntry}; +use toyos_rootimage::handoff::{held, Descriptor}; #[panic_handler] fn panic(info: &core::panic::PanicInfo) -> ! { @@ -355,7 +356,7 @@ pub(crate) unsafe extern "C" fn kernel_main(kernel_args: &KernelArgs) -> ! { "boot: the loader put the stack at image+{:#x}+{:#x}, past the {:#x}-byte image", kernel_args.kernel_stack_addr, kernel_args.kernel_stack_size, kernel_args.kernel_memory_size ); - let reserved = [ + let loader = [ mm::Region { start: kernel_args.kernel_memory_addr, end: kernel_args.kernel_memory_addr + kernel_args.kernel_memory_size }, mm::Region { start: kernel_args.kernel_elf_addr, end: kernel_args.kernel_elf_addr + kernel_args.kernel_elf_size }, // The loader's black-box page, which is ordinary `LoaderData` and so @@ -365,19 +366,26 @@ pub(crate) unsafe extern "C" fn kernel_main(kernel_args: &KernelArgs) -> ! { // ROOT's image, `LoaderData` like the black box's page. Empty on a // boot the loader handed none. root_image, - // Last: the one region the loader did not allocate. - arch::boot::reserved(), ]; // A region the loader did not allocate withholds memory nothing uses, so one the firmware map does not hold as `LoaderData` is refused. - for region in reserved[..reserved.len() - 1].iter().filter(|r| r.start < r.end) { + // Block 1: the ELF region (`kernel_elf_addr`+`kernel_elf_size`) is not page-aligned. + for region in loader.iter().filter(|r| r.start < r.end) { assert!( - maps.iter().any(|e| { - e.uefi_type == toyos_bootmap::EFI_LOADER_DATA && e.start <= region.start && region.end <= e.end - }), + held( + maps.iter().map(|e| Descriptor { ty: e.uefi_type, start: e.start, end: e.end }), + toyos_bootmap::EFI_LOADER_DATA, + region.start, + region.end - region.start, + 1, + ) + .is_some(), "boot: reserving {:#x}..{:#x}, which no LoaderData descriptor in the firmware map holds", region.start, region.end ); } + // The architecture's own page (the AP trampoline on x86-64, empty on AArch64) is not a + // loader allocation, so it is named here rather than folded into `loader` above. + let reserved = [loader[0], loader[1], loader[2], loader[3], arch::boot::reserved()]; // The last point before the first hash container (`mm::init`'s address // space), and not earlier: seeding fails only by panicking, and a panic From 65591b9f1516898280fa37571ffe426502665544 Mon Sep 17 00:00:00 2001 From: japabu Date: Tue, 29 Sep 2026 02:37:40 +0200 Subject: [PATCH 3/3] Round-2 review fixes: reserved is destructured by name, not indexed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `reserved`'s construction copied `loader` by position, so a fifth region added to `loader` compiled and passed the containment check but was silently dropped from what `mm::init` withholds — exactly what mutation-r2.patch had done to `root_image`. Destructuring `loader` by name (`image`, `elf`, `black_box`, `root`) makes a fifth element a compile error instead. The issue's owner line named a role ("whoever lands #583"), not a concrete owner; it now names PR #583 (wt/toyos-loader1) itself, and drops the line number citation that rots with the next edit to toyos-abi/src/boot.rs. The same rotting citation is dropped from main.rs's comment on the architecture's own reserved page. Co-Authored-By: Claude Opus 5.5 --- ...ernel_stack_addr-names-an-offset-not-an-address.md | 11 ++++++----- kernel/src/main.rs | 10 +++++++--- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/issues/kernel/kernel_stack_addr-names-an-offset-not-an-address.md b/issues/kernel/kernel_stack_addr-names-an-offset-not-an-address.md index 6b39d18e59d..8bf1eca1ede 100644 --- a/issues/kernel/kernel_stack_addr-names-an-offset-not-an-address.md +++ b/issues/kernel/kernel_stack_addr-names-an-offset-not-an-address.md @@ -6,13 +6,14 @@ opened: 2026-09-28 # `kernel_stack_addr` names an offset, not an address -`toyos_abi::boot::KernelArgs::kernel_stack_addr` (`toyos-abi/src/boot.rs:8`) -holds an offset into the kernel's own image — `StackedImage::place`'s `stack` -— never a physical address. Both architectures' entry code already treat it -as an offset (`kernel/src/arch/aarch64/boot.rs` reads it into a local named +`toyos_abi::boot::KernelArgs::kernel_stack_addr` holds an offset into the +kernel's own image — `StackedImage::place`'s `stack` — never a physical +address. Both architectures' entry code already treat it as an offset +(`kernel/src/arch/aarch64/boot.rs` reads it into a local named `stack_offset`); `kernel/src/main.rs` reads it under its `KernelArgs` name and must not repeat the confusion the name invites. -Owner: whoever lands #583, which is already changing `KernelArgs`'s layout. +Owner: the author of PR #583 (`wt/toyos-loader1`), which is already changing +`KernelArgs`'s layout. Exit: the field renamed to `kernel_stack_offset` (or equivalent), landed as part of that change so `KernelArgs` moves once. diff --git a/kernel/src/main.rs b/kernel/src/main.rs index 0139931e8fe..9adfc3da996 100644 --- a/kernel/src/main.rs +++ b/kernel/src/main.rs @@ -383,9 +383,13 @@ pub(crate) unsafe extern "C" fn kernel_main(kernel_args: &KernelArgs) -> ! { region.start, region.end ); } - // The architecture's own page (the AP trampoline on x86-64, empty on AArch64) is not a - // loader allocation, so it is named here rather than folded into `loader` above. - let reserved = [loader[0], loader[1], loader[2], loader[3], arch::boot::reserved()]; + // The architecture's own page is not a loader allocation, so it is named + // here rather than folded into `loader` above. Destructuring `loader` by + // name, rather than indexing it, means a region added to `loader` fails + // to compile here instead of compiling and being silently dropped from + // what `mm::init` withholds. + let [image, elf, black_box, root] = loader; + let reserved = [image, elf, black_box, root, arch::boot::reserved()]; // The last point before the first hash container (`mm::init`'s address // space), and not earlier: seeding fails only by panicking, and a panic