Development #130
Development #130
Conversation
…, minor whitespace changes
…, minor whitespace changes
WalkthroughAdds Multiboot2 MODULE parsing and InitRDLoad to import multiboot modules into the VFS, exposes the multiboot info address globally, integrates InitRD into the build and shell flow, updates VFS mount base, adds an IDE ATAPI query prototype, and includes minor ISO9660 formatting edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GRUB as Bootloader (GRUB)
participant K as Kernel
participant IRD as InitRDLoad
participant VFS as VFS
GRUB->>K: multiboot2 handoff (info addr + tags)
Note right of K: g_multiboot_info_addr (global)
alt VF_CONFIG_LOAD_MB_MODULES enabled
K->>IRD: InitRDLoad()
IRD->>IRD: iterate tags (align 8 bytes)
loop for each MODULE tag
IRD->>IRD: validate mod_start/end and size
IRD->>K: map module physical -> temp vaddr
IRD->>VFS: VfsWriteFile(cmdline, data, size)
VFS-->>IRD: write status
IRD->>K: unmap/free temp region
end
IRD-->>K: done
else
Note over K: module loading skipped
end
K-->>GRUB: continue boot
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
drivers/Ide.h (1)
66-66: Clarify IdeIsAtapi() contract (return semantics) and align with existing flags.Please document whether it returns 1/0 vs negative error codes (e.g., IDE_ERROR_NO_DRIVE). Consider returning bool for presence and using out-param for errors, or keep int but specify the convention.
Suggested header doc:
int IdeReadLBA2048(uint8_t drive, uint32_t lba, void* buffer); -int IdeIsAtapi(uint8_t drive); +// Returns 1 if ATAPI, 0 if not; negative IDE_ERROR_* on failure (e.g., IDE_ERROR_NO_DRIVE). +int IdeIsAtapi(uint8_t drive);fs/Iso9660.c (1)
145-156: Gate verbose PVD diagnostics behind a debug flag to keep boot logs clean.The added hex dump and signature logs are useful, but they’re quite chatty during normal boots.
Example:
-PrintKernel("[ISO] First 16 bytes of sector 16: "); -for (int i = 0; i < 16; i++) { - PrintKernelF("%02X ", sector_buffer[i]); -} -PrintKernel("\n"); + #ifdef VF_DEBUG_ISO + PrintKernel("[ISO] First 16 bytes of sector 16: "); + for (int i = 0; i < 16; i++) { + PrintKernelF("%02X ", sector_buffer[i]); + } + PrintKernel("\n"); + #endifAlso applies to: 155-167, 173-175
grub.cfg (1)
8-9: Confirm intent of bundling the kernel as a module too.module2 /boot/voidframe.krnl → /System/Kernel/voidframe.krnl duplicates the running kernel as a module. Likely intentional for VFS population, but it increases memory footprint. If that’s desired, all good; otherwise drop it or use a smaller test artifact.
kernel/core/Multiboot2.h (2)
11-11: Macro naming consistency.You now have both MULTIBOOT2_TAG_TYPE_FRAMEBUFFER and MULTIBOOT_TAG_FRAMEBUFFER defined (same value). Consider standardizing on MULTIBOOT2_TAG_TYPE_* to avoid confusion.
54-60: Module tag struct LGTM; consider static layout checks.Struct matches MB2 spec. For future-proofing, add a static assertion on field offsets or sizeof to catch packing regressions.
Example:
_Static_assert(offsetof(struct MultibootModuleTag, mod_start) == 8, "MB2 module tag layout");kernel/core/InitRD.h (1)
1-3: Header addition LGTM.Simple, focused API. Consider guarding the declaration with VF_CONFIG_LOAD_MB_MODULES for clearer feature surfacing.
fs/VFS.c (1)
66-70: Verify VFSystemDrive mount vs mkfs root; centralize path constants.MkfsHandler creates the VFSystemDrive layout under "/Devices/Storage/VFSystemDrive/..." (kernel/etc/Shell.c L1004–L1056) while fs/VFS.c mounts VFSystemDrive at FormatS("%s/VFSystemDrive", RuntimeMounts) (fs/VFS.c L66–84); fs/VFS.h already defines DevicesStorage = "/Devices/Storage" and RuntimeMounts = "/Runtime/Mounts". Confirm intended semantics (device image roots vs mountpoints) and either:
- Make both sides use the canonical constants (DevicesStorage/RuntimeMounts) or
- Update MkfsHandler to populate the canonical runtime mount root, or
- Add a VFSystemDrivePath() helper and use it everywhere to avoid string drift.
kernel/core/InitRD.c (1)
61-66: Destination path trust: normalize to an absolute, non-empty VFS path.
cmdline may be empty or relative; normalize to something under a known mount (e.g., “/System/Boot/” or “/Runtime/Modules/”).For example:
- if (VfsWriteFile(mod->cmdline, mod_data, mod_size) >= 0) { + const char* dest = mod->cmdline && mod->cmdline[0] ? mod->cmdline : "/Runtime/Modules/module.bin"; + char pathbuf[256]; + if (dest[0] != '/') { FormatA(pathbuf, sizeof(pathbuf), "/Runtime/Modules/%s", dest); dest = pathbuf; } + if (VfsWriteFile(dest, mod_data, mod_size) >= 0) { - PrintKernelF("[INITRD] Copied %s to VFS\n", mod->cmdline); + PrintKernelF("[INITRD] Copied %s to VFS\n", dest); } else { - PrintKernelF("[INITRD] Failed to copy %s\n", mod->cmdline); + PrintKernelF("[INITRD] Failed to copy %s\n", dest); }kernel/core/Kernel.c (4)
74-84: Module tag logging added — consider also bounding the surrounding walk by total_size.
The added logging is fine. As a minor hardening, mirror the InitRD.c guard: stop walking when current tag exceeds info+total_size or when tag->size is invalid.
531-589: MakeRoot scaffolding — consider checking mkdir/create return codes.
Silent failures here can make later VFS ops confusing.
711-716: Surface how many modules were loaded.
InitRDLoad() currently returns void; consider returning an int count and printing it to aid debugging (0 vs N modules is important signal).Apply (example):
-#ifdef VF_CONFIG_LOAD_MB_MODULES - // Load multiboot modules - PrintKernel("Info: Loading multiboot modules...\n"); - InitRDLoad(); - PrintKernelSuccess("System: Multiboot modules loaded\n"); -#endif +#ifdef VF_CONFIG_LOAD_MB_MODULES + PrintKernel("Info: Loading multiboot modules...\n"); + int loaded = InitRDLoad(); + PrintKernelF("System: Multiboot modules loaded: %d\n", loaded); +#endifAnd adjust the InitRDLoad signature accordingly.
711-716: Small mismatch: AHCI init message is under GENERIC_SOUND guard.
Currently “Initializing AHCI Driver...” prints only when VF_CONFIG_ENABLE_GENERIC_SOUND is set, not when AHCI is. Move the log into the AHCI guard with the init call.Apply:
-#ifdef VF_CONFIG_ENABLE_GENERIC_SOUND - PrintKernel("Info: Initializing PC Speaker...\n"); - PCSpkr_Init(); - PrintKernelSuccess("System: PC Speaker initialized\n"); - - PrintKernel("Info: Initializing AHCI Driver...\n"); -#endif +#ifdef VF_CONFIG_ENABLE_GENERIC_SOUND + PrintKernel("Info: Initializing PC Speaker...\n"); + PCSpkr_Init(); + PrintKernelSuccess("System: PC Speaker initialized\n"); +#endif @@ #ifdef VF_CONFIG_ENABLE_AHCI + PrintKernel("Info: Initializing AHCI Driver...\n"); if (AHCI_Init() == 0) { PrintKernelSuccess("System: AHCI Driver initialized\n"); } else { PrintKernelWarning("AHCI initialization failed\n"); } #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
drivers/Ide.h(1 hunks)fs/Iso9660.c(2 hunks)fs/Iso9660.h(1 hunks)fs/VFS.c(2 hunks)grub.cfg(1 hunks)kernel/core/InitRD.c(1 hunks)kernel/core/InitRD.h(1 hunks)kernel/core/Kernel.c(6 hunks)kernel/core/Multiboot2.h(2 hunks)kernel/etc/Shell.c(2 hunks)meson.build(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
📚 Learning: 2025-09-06T07:51:33.673Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#111
File: kernel/sched/MLFQ.c:999-1002
Timestamp: 2025-09-06T07:51:33.673Z
Learning: In ISO9660 filesystem implementation, the Primary Volume Descriptor (PVD) id field is exactly 5 bytes ['C','D','0','0','1'] with NO null terminator. Using FastStrCmp() or other null-terminated string functions will fail because they expect null terminators. Use exact byte-by-byte comparison instead: candidate->id[0] == 'C' && candidate->id[1] == 'D' etc.
Applied to files:
fs/Iso9660.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
Applied to files:
grub.cfg
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
grub.cfg
📚 Learning: 2025-09-01T02:35:28.353Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: fs/VFS.c:331-335
Timestamp: 2025-09-01T02:35:28.353Z
Learning: VfsIsFile function was initially implemented as a placeholder that only checked for mount existence. The proper implementation should follow the same pattern as VfsIsDir: find mount, strip mount path, then use filesystem-specific functions to check if the path points to a file (FS_FILE for RAMFS, Fat12 functions for FAT12).
Applied to files:
fs/VFS.c
📚 Learning: 2025-08-30T07:32:52.338Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#99
File: meson.build:103-103
Timestamp: 2025-08-30T07:32:52.338Z
Learning: FAT12 filesystem has a hard mathematical limit of 4,086 clusters due to 12-bit cluster addressing. This means FAT12 cannot handle disk sizes larger than approximately 4MB, making 128MB disks incompatible with FAT12 format. The user's meson.build was creating 128M disks with FAT12 format (`mkfs.fat -F 12`), which caused kernel panics when accessing files. The solution is to either use smaller disk sizes (≤4MB, preferably 1440K for traditional floppy compatibility) with FAT12, or use FAT16/FAT32 for larger disk sizes.
Applied to files:
fs/VFS.c
🧬 Code graph analysis (7)
fs/Iso9660.c (2)
mm/KernelHeap.c (1)
KernelFree(480-525)kernel/etc/Console.c (3)
PrintKernelError(213-218)PrintKernel(174-192)PrintKernelF(253-260)
fs/Iso9660.h (1)
fs/Iso9660.c (1)
Iso9660CopyFile(439-471)
kernel/core/InitRD.h (1)
kernel/core/InitRD.c (1)
InitRDLoad(10-69)
fs/VFS.c (2)
kernel/etc/Format.c (1)
FormatS(288-295)kernel/etc/Console.c (1)
SerialWriteF(289-296)
kernel/core/InitRD.c (2)
kernel/etc/Console.c (3)
PrintKernelWarning(220-225)PrintKernelF(253-260)PrintKernel(174-192)fs/VFS.c (1)
VfsWriteFile(434-460)
kernel/etc/Shell.c (1)
kernel/core/InitRD.c (1)
InitRDLoad(10-69)
kernel/core/Kernel.c (3)
kernel/etc/Console.c (3)
PrintKernel(174-192)PrintKernelHex(228-251)PrintKernelSuccess(206-211)kernel/core/InitRD.c (1)
InitRDLoad(10-69)kernel/sched/MLFQ.c (1)
MLFQSchedInit(1715-1795)
🔇 Additional comments (6)
fs/Iso9660.h (1)
76-76: No‑op header churn.Declaration unchanged functionally. No issues.
kernel/etc/Shell.c (1)
11-11: Include looks good.Brings InitRDLoad into the shell without side effects.
meson.build (2)
113-114: InitRD.c added to build — good integration.
InitRD.c is correctly included in c_sources; matches the new feature entry point.
195-196: GRUB module2 present — confirm referenced payloads are staged into the ISOgrub.cfg has module2 entries for /boot/voidframe.krnl and /boot/grub/grub.cfg (your script found them); no repo-local modules/ tree was detected — ensure those two files are copied into isodir or included in the generated .iso so InitRDLoad() can load them.
Quick checks to run from repo/build root (or on the built ISO):
fd -HI -t f 'voidframe.krnl' || true fd -HI -t f 'grub.cfg' || true # if you have an ISO file: for iso in *.iso; do bsdtar -tf "$iso" | rg -n "boot/voidframe.krnl|boot/grub/grub.cfg" || true; donekernel/core/Kernel.c (2)
49-50: Expose g_multiboot_info_addr globally — OK.
This unblocks InitRD.c; linkage is correct.
782-787: Scheduler gating — looks good.
MLFQ initialization remains gated and positioned sensibly before interrupts are unmasked.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
kernel/core/InitRD.c (2)
1-8: Include std headers for fixed-width types and bool.This TU uses uint8_t/uint32_t/uint64_t and bool but doesn’t include the defining headers.
Apply:
#include "InitRD.h" #include "Multiboot2.h" #include "Console.h" #include "VFS.h" #include "KernelHeap.h" #include "MemOps.h" #include "VMem.h" + +#include <stdint.h> +#include <stdbool.h>
17-24: Don’t dereference Multiboot info as physical; convert to virtual and bound the tag walk.Current code reads g_multiboot_info_addr directly and walks tags without limiting by total_size; this risks UB/infinite loop on corrupt sizes.
Apply:
- PrintKernelF("[INITRD] Multiboot info at 0x%08X\n", g_multiboot_info_addr); - uint32_t total_size = *(uint32_t*)g_multiboot_info_addr; - PrintKernelF("[INITRD] Total size: %u bytes\n", total_size); - - struct MultibootTag* tag = (struct MultibootTag*)(g_multiboot_info_addr + 8); - - while (tag->type != MULTIBOOT2_TAG_TYPE_END) { + PrintKernelF("[INITRD] Multiboot info at 0x%08X\n", g_multiboot_info_addr); + uint8_t* mb_base = (uint8_t*)PHYS_TO_VIRT((uintptr_t)g_multiboot_info_addr); + uint32_t total_size = *(uint32_t*)mb_base; + PrintKernelF("[INITRD] Total size: %u bytes\n", total_size); + + uint8_t* mb_end = mb_base + total_size; + struct MultibootTag* tag = (struct MultibootTag*)(mb_base + 8); + + while ((uint8_t*)tag + sizeof(*tag) <= mb_end && tag->type != MULTIBOOT2_TAG_TYPE_END) { + if (tag->size < sizeof(struct MultibootTag) || + (uint8_t*)tag + tag->size > mb_end) { + PrintKernelWarning("[INITRD] Corrupted tag size; aborting tag walk\n"); + break; + } /* ... existing MODULE handling ... */ - tag = (struct MultibootTag*)((uint8_t*)tag + ((tag->size + 7) & ~7)); + uint32_t adv = (tag->size + 7u) & ~7u; + if (adv == 0) { + PrintKernelWarning("[INITRD] tag->size==0; aborting tag walk\n"); + break; + } + tag = (struct MultibootTag*)((uint8_t*)tag + adv); }Also applies to: 105-106
🧹 Nitpick comments (3)
kernel/core/InitRD.c (3)
41-70: Avoid alloc-then-unmap for temporary VA; reserve VA without backing if available.Allocating RAM pages only to immediately unmap wastes work and briefly consumes frames. Prefer a VA reservation API (e.g., VMemReserve/VMemAllocVirtual) and then map MMIO.
If such API exists elsewhere in the repo, switch to it; otherwise I can provide a minimal VMemReserve/VMemRelease helper.
73-84: Gate verbose hex dumps behind a debug flag.Per-byte PrintKernelF in a loop is chatty and slow on serial paths.
Apply:
- PrintKernelF("[INITRD] First 16 bytes: "); - for (int i = 0; i < 16 && i < mod_size; i++) { - PrintKernelF("%02X ", mod_data[i]); - } - PrintKernel("\n"); +#ifdef VF_DEBUG_INITRD + PrintKernelF("[INITRD] First 16 bytes:"); + for (int i = 0; i < 16 && i < (int)mod_size; i++) + PrintKernelF(" %02X", mod_data[i]); + PrintKernel("\n"); +#endif
32-35: Defensive use of cmdline string from MODULE tag.cmdline comes from untrusted boot info; consider clamping to tag->size to ensure NUL within bounds before using as path.
I can provide a small helper to compute a safe, bounded cmdline length and fall back to a default filename if not NUL-terminated.
Also applies to: 95-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kernel/core/InitRD.c(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-08-31T02:11:29.291Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#100
File: kernel/memory/VMem.h:44-47
Timestamp: 2025-08-31T02:11:29.291Z
Learning: VoidFrame kernel's MAX_PAGES is hardcoded to exactly 4GB (1,048,576 pages), causing false "Memory detected exceeds MAX_PAGES" warnings when memory holes push the highest physical address slightly above 4GB. The solution is to sum only MULTIBOOT2_MEMORY_AVAILABLE regions instead of using max_physical_address for total_pages calculation.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-17T08:16:43.539Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#93
File: kernel/memory/VMem.c:72-79
Timestamp: 2025-08-17T08:16:43.539Z
Learning: In VoidFrame kernel, VMemGetPhysAddr and related page table walking functions were causing panics because they assumed identity mapping existed for physical addresses < IDENTITY_MAP_SIZE (4GB), but the bootloader doesn't provide this 1:1 mapping. The fix is to remove all identity mapping assumptions and always use PHYS_TO_VIRT() for page table access in functions like VMemGetPageTablePhys, VMemMap, VMemMapHuge, VMemGetPhysAddr, VMemUnmap, VMemMapMMIO, and VMemUnmapMMIO.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-17T05:53:48.040Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#92
File: meson.build:31-34
Timestamp: 2025-08-17T05:53:48.040Z
Learning: The VoidFrame kernel's memory detection was reporting incorrect memory amounts (like 9200MB) because it used the highest physical address (including memory holes and reserved regions) as total memory size, rather than summing only the MULTIBOOT2_MEMORY_AVAILABLE regions. The fix involves counting only available memory regions and using the highest available address only for bitmap sizing.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-13T07:19:55.274Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:19:55.274Z
Learning: VMemUnmapMMIO in VoidFrame kernel should use the same page table access pattern as the working VMemUnmap function: `uint64_t* pt_table = (uint64_t*)(pt_phys + KERNEL_VIRTUAL_OFFSET);` rather than PHYS_TO_VIRT macro or identity mapping checks.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-16T05:28:34.925Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#90
File: kernel/memory/MemOps.c:103-124
Timestamp: 2025-08-16T05:28:34.925Z
Learning: In VoidFrame kernel, the virtual address space management uses a simple linear allocator (kernel_space.next_vaddr) without any reclamation mechanism. VMemFree and VMemUnmapMMIO clear page tables but don't free virtual address space for reuse, causing eventual virtual address space exhaustion. The solution requires implementing a free list with coalescing in the VirtAddrSpace structure and updating allocation/deallocation functions to reuse freed ranges.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-07-16T01:04:23.790Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#47
File: kernel/core/Kernel.c:434-434
Timestamp: 2025-07-16T01:04:23.790Z
Learning: VMemAlloc() in VoidFrame kernel fails with error code -2 (VMEM_ERROR_INVALID_ADDR) due to virtual address space layout conflict where the heap start address (_bss_end + KERNEL_VIRTUAL_OFFSET + 1MB margin) exceeds VIRT_ADDR_SPACE_END (0xFFFFFFFF80000000ULL), causing VMemMap validation to fail.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-09T04:47:56.883Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#53
File: kernel/core/Kernel.c:256-259
Timestamp: 2025-08-09T04:47:56.883Z
Learning: The VoidFrame kernel memory crash occurs when VIRT_ADDR_SPACE_START is set to 0xFFFF800000000000ULL instead of 0xFFFFFFFF80000000ULL, causing the heap to allocate 128TB away from where the kernel is mapped, resulting in page faults when accessing heap memory.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-13T07:17:35.941Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:17:35.941Z
Learning: In VoidFrame kernel, VMemGetPageTablePhys uses a 3-level hierarchy: Level 0 = PML4->PDP, Level 1 = PDP->PD, Level 2 = PD->PT. To access Page Tables, always use level=2, not level=1.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-12T02:13:46.266Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#72
File: arch/x86_64/syscall/SyscallEntry.asm:24-30
Timestamp: 2025-08-12T02:13:46.266Z
Learning: User assembler-0 has existing ELF infrastructure in VoidFrame kernel with Elf.h header definitions and CreateProcess() function that properly sets up process context including stack allocation, ProcessExitStub return address, and register initialization. The issue was missing ELF loader implementation, not compilation problems.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-31T14:19:11.366Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: docs/ARCHITECHTURE.md:1-3
Timestamp: 2025-08-31T14:19:11.366Z
Learning: assembler-0 wants to implement deep fault tracing for VoidFrame kernel exceptions (Page Fault, General Protection Fault, Invalid Opcode) instead of just panicking. The current interrupt handler has complete register state available in the Registers struct (defined in arch/x86_64/cpu/Cpu.h) including RBP for stack walking, RIP for fault location, and error codes for analysis.
Applied to files:
kernel/core/InitRD.c
🧬 Code graph analysis (1)
kernel/core/InitRD.c (3)
kernel/etc/Console.c (3)
PrintKernelWarning(220-225)PrintKernelF(253-260)PrintKernel(174-192)mm/VMem.c (5)
VMemAlloc(385-423)VMemUnmap(539-590)VMemFree(425-449)VMemMapMMIO(622-673)VMemUnmapMMIO(675-707)fs/VFS.c (1)
VfsWriteFile(434-460)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
kernel/core/InitRD.c (5)
25-29: Validate MODULE tag size before casting.The code casts to
MultibootModuleTagwithout verifying the tag size is sufficient to contain all the expected fields.Add size validation:
if (tag->type == MULTIBOOT2_TAG_TYPE_MODULE) { + if (tag->size < sizeof(struct MultibootModuleTag)) { + PrintKernelWarning("[INITRD] MODULE tag too small; skipping\n"); + continue; + } struct MultibootModuleTag* mod = (struct MultibootModuleTag*)tag;
1-7: Include required standard headers for type definitions.This file uses
bool,uint8_t,uint32_t, anduint64_ttypes but doesn't include the standard headers that define them. This will cause compilation errors.Add the required headers:
#include "InitRD.h" #include "Multiboot2.h" #include "Console.h" #include "VFS.h" -#include "KernelHeap.h" -#include "MemOps.h" #include "VMem.h" +#include <stdint.h> +#include <stdbool.h>Note:
KernelHeap.handMemOps.happear to be unused and can be removed.
23-106: Harden multiboot tag traversal against corruption.The loop trusts
tag->typeandtag->sizewithout bounds checking, which could cause infinite loops or out-of-bounds reads if the multiboot structure is corrupted.Add bounds checking to the tag walk:
+ uint8_t* mb_end = mb_virt + total_size; - while (tag->type != MULTIBOOT2_TAG_TYPE_END) { + while ((uint8_t*)tag + sizeof(*tag) <= mb_end && tag->type != MULTIBOOT2_TAG_TYPE_END) { + // Validate tag size + if (tag->size < sizeof(struct MultibootTag) || (uint8_t*)tag + tag->size > mb_end) { + PrintKernelWarning("[INITRD] Corrupted tag size; aborting tag walk\n"); + break; + } + if (tag->type == MULTIBOOT2_TAG_TYPE_MODULE) {Also add validation at the loop advancement:
- tag = (struct MultibootTag*)((uint8_t*)tag + ((tag->size + 7) & ~7)); + uint32_t adv = (tag->size + 7u) & ~7u; + if (adv == 0) { + PrintKernelWarning("[INITRD] Zero tag size; aborting tag walk\n"); + break; + } + tag = (struct MultibootTag*)((uint8_t*)tag + adv); }
17-22: Critical: Convert physical multiboot address to virtual address before access.The code is directly dereferencing
g_multiboot_info_addrwhich is a physical address. Based on the learnings from PR #93, the bootloader doesn't provide identity mapping, so this will read from the wrong virtual address and cause corrupted data or crashes.Convert to virtual address before accessing:
PrintKernelF("[INITRD] Multiboot info at 0x%08X\n", g_multiboot_info_addr); - uint32_t total_size = *(uint32_t*)g_multiboot_info_addr; + uint8_t* mb_virt = (uint8_t*)PHYS_TO_VIRT(g_multiboot_info_addr); + uint32_t total_size = *(uint32_t*)mb_virt; PrintKernelF("[INITRD] Total size: %u bytes\n", total_size); - struct MultibootTag* tag = (struct MultibootTag*)(g_multiboot_info_addr + 8); + struct MultibootTag* tag = (struct MultibootTag*)(mb_virt + 8);
64-69: Critical: Map entire module range, not just first page.The code only maps a single page but the module can span multiple pages. This will cause page faults when accessing data beyond the first page.
Map all pages in the range:
- int map_res = VMemMap((uint64_t)temp_vaddr, aligned_paddr, PAGE_WRITABLE | PAGE_PRESENT); - if (map_res != VMEM_SUCCESS) { - PrintKernelF("[INITRD] Failed to map module phys -> virt: %d\n", map_res); - VMemFree(temp_vaddr, map_size); - continue; - } + // Map all pages in the range + uint64_t num_pages = map_size / PAGE_SIZE; + bool mapping_failed = false; + + for (uint64_t i = 0; i < num_pages; i++) { + uint64_t vaddr_page = (uint64_t)temp_vaddr + (i * PAGE_SIZE); + uint64_t paddr_page = aligned_paddr + (i * PAGE_SIZE); + + if (VMemMap(vaddr_page, paddr_page, PAGE_PRESENT | PAGE_WRITABLE) != VMEM_SUCCESS) { + PrintKernelF("[INITRD] Failed to map page %llu\n", i); + mapping_failed = true; + // Clean up any partial mappings + for (uint64_t j = 0; j < i; j++) { + VMemUnmap((uint64_t)temp_vaddr + (j * PAGE_SIZE), PAGE_SIZE); + } + break; + } + } + + if (mapping_failed) { + VMemFree(temp_vaddr, map_size); + continue; + }
🧹 Nitpick comments (2)
kernel/core/InitRD.c (2)
88-91: Text detection logic misses null terminator.The text detection checks for control characters but doesn't allow null bytes (
\0), which are valid in text files as terminators.Update the text detection to allow null terminators:
for (int i = 0; i < 32 && i < mod_size; i++) { - if (mod_data[i] < 32 && mod_data[i] != '\n' && mod_data[i] != '\r' && mod_data[i] != '\t') { + if (mod_data[i] < 32 && mod_data[i] != '\n' && mod_data[i] != '\r' && mod_data[i] != '\t' && mod_data[i] != '\0') { is_text = false; break; }
80-82: Use consistent format specifier for loop variable.Minor inconsistency: using
intfor loop variable but comparing withuint32_t.- for (int i = 0; i < 16 && i < mod_size; i++) { + for (uint32_t i = 0; i < 16 && i < mod_size; i++) { PrintKernelF("%02X ", mod_data[i]);Also apply to line 87:
- for (int i = 0; i < 32 && i < mod_size; i++) { + for (uint32_t i = 0; i < 32 && i < mod_size; i++) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kernel/core/InitRD.c(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#130
File: kernel/core/InitRD.c:25-35
Timestamp: 2025-09-14T11:01:02.669Z
Learning: In VoidFrame kernel's InitRD.c, multiboot modules should be mapped using VMemMap() with PAGE_PRESENT | PAGE_WRITABLE flags instead of VMemMapMMIO(), as the latter applies cache-disabling flags (PAGE_NOCACHE | PAGE_WRITETHROUGH) that can corrupt data when reading regular RAM content.
📚 Learning: 2025-09-14T11:01:02.669Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#130
File: kernel/core/InitRD.c:25-35
Timestamp: 2025-09-14T11:01:02.669Z
Learning: In VoidFrame kernel's InitRD.c, multiboot modules should be mapped using VMemMap() with PAGE_PRESENT | PAGE_WRITABLE flags instead of VMemMapMMIO(), as the latter applies cache-disabling flags (PAGE_NOCACHE | PAGE_WRITETHROUGH) that can corrupt data when reading regular RAM content.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-31T02:11:29.291Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#100
File: kernel/memory/VMem.h:44-47
Timestamp: 2025-08-31T02:11:29.291Z
Learning: VoidFrame kernel's MAX_PAGES is hardcoded to exactly 4GB (1,048,576 pages), causing false "Memory detected exceeds MAX_PAGES" warnings when memory holes push the highest physical address slightly above 4GB. The solution is to sum only MULTIBOOT2_MEMORY_AVAILABLE regions instead of using max_physical_address for total_pages calculation.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-17T08:16:43.539Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#93
File: kernel/memory/VMem.c:72-79
Timestamp: 2025-08-17T08:16:43.539Z
Learning: In VoidFrame kernel, VMemGetPhysAddr and related page table walking functions were causing panics because they assumed identity mapping existed for physical addresses < IDENTITY_MAP_SIZE (4GB), but the bootloader doesn't provide this 1:1 mapping. The fix is to remove all identity mapping assumptions and always use PHYS_TO_VIRT() for page table access in functions like VMemGetPageTablePhys, VMemMap, VMemMapHuge, VMemGetPhysAddr, VMemUnmap, VMemMapMMIO, and VMemUnmapMMIO.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-17T05:53:48.040Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#92
File: meson.build:31-34
Timestamp: 2025-08-17T05:53:48.040Z
Learning: The VoidFrame kernel's memory detection was reporting incorrect memory amounts (like 9200MB) because it used the highest physical address (including memory holes and reserved regions) as total memory size, rather than summing only the MULTIBOOT2_MEMORY_AVAILABLE regions. The fix involves counting only available memory regions and using the highest available address only for bitmap sizing.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-13T07:19:55.274Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:19:55.274Z
Learning: VMemUnmapMMIO in VoidFrame kernel should use the same page table access pattern as the working VMemUnmap function: `uint64_t* pt_table = (uint64_t*)(pt_phys + KERNEL_VIRTUAL_OFFSET);` rather than PHYS_TO_VIRT macro or identity mapping checks.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-16T05:28:34.925Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#90
File: kernel/memory/MemOps.c:103-124
Timestamp: 2025-08-16T05:28:34.925Z
Learning: In VoidFrame kernel, the virtual address space management uses a simple linear allocator (kernel_space.next_vaddr) without any reclamation mechanism. VMemFree and VMemUnmapMMIO clear page tables but don't free virtual address space for reuse, causing eventual virtual address space exhaustion. The solution requires implementing a free list with coalescing in the VirtAddrSpace structure and updating allocation/deallocation functions to reuse freed ranges.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-07-16T01:04:23.790Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#47
File: kernel/core/Kernel.c:434-434
Timestamp: 2025-07-16T01:04:23.790Z
Learning: VMemAlloc() in VoidFrame kernel fails with error code -2 (VMEM_ERROR_INVALID_ADDR) due to virtual address space layout conflict where the heap start address (_bss_end + KERNEL_VIRTUAL_OFFSET + 1MB margin) exceeds VIRT_ADDR_SPACE_END (0xFFFFFFFF80000000ULL), causing VMemMap validation to fail.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-09T04:47:56.883Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#53
File: kernel/core/Kernel.c:256-259
Timestamp: 2025-08-09T04:47:56.883Z
Learning: The VoidFrame kernel memory crash occurs when VIRT_ADDR_SPACE_START is set to 0xFFFF800000000000ULL instead of 0xFFFFFFFF80000000ULL, causing the heap to allocate 128TB away from where the kernel is mapped, resulting in page faults when accessing heap memory.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-13T07:17:35.941Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:17:35.941Z
Learning: In VoidFrame kernel, VMemGetPageTablePhys uses a 3-level hierarchy: Level 0 = PML4->PDP, Level 1 = PDP->PD, Level 2 = PD->PT. To access Page Tables, always use level=2, not level=1.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-12T02:13:46.266Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#72
File: arch/x86_64/syscall/SyscallEntry.asm:24-30
Timestamp: 2025-08-12T02:13:46.266Z
Learning: User assembler-0 has existing ELF infrastructure in VoidFrame kernel with Elf.h header definitions and CreateProcess() function that properly sets up process context including stack allocation, ProcessExitStub return address, and register initialization. The issue was missing ELF loader implementation, not compilation problems.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-31T14:19:11.366Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: docs/ARCHITECHTURE.md:1-3
Timestamp: 2025-08-31T14:19:11.366Z
Learning: assembler-0 wants to implement deep fault tracing for VoidFrame kernel exceptions (Page Fault, General Protection Fault, Invalid Opcode) instead of just panicking. The current interrupt handler has complete register state available in the Registers struct (defined in arch/x86_64/cpu/Cpu.h) including RBP for stack walking, RIP for fault location, and error codes for analysis.
Applied to files:
kernel/core/InitRD.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
kernel/core/InitRD.c
🧬 Code graph analysis (1)
kernel/core/InitRD.c (3)
kernel/etc/Console.c (3)
PrintKernelWarning(220-225)PrintKernelF(253-260)PrintKernel(174-192)mm/VMem.c (4)
VMemAlloc(385-423)VMemUnmap(539-590)VMemFree(425-449)VMemMap(305-346)fs/VFS.c (1)
VfsWriteFile(434-460)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
kernel/core/InitRD.c (1)
102-103: Do not call BuddyAlloc/BuddyFree from InitRD.c — fix allocation/mapping orderBuddyAlloc/BuddyFree are file-local (static) in mm/VMem.c (they exist at ~mm/VMem.c:121/147) and cannot be used from kernel/core/InitRD.c. Also VMemFree expects the pages to still be mapped (it clears page tables and frees physical pages), so calling VMemUnmap() before VMemFree(...) will orphan the allocated physical pages.
- Replace the current unmap/free sequence with a single VMemFree(temp_vaddr, map_size) (do not call VMemUnmap beforehand).
- If you need to remap the same virtual address to module physical pages, do not use VMemAlloc (which returns backed pages). Options:
- Add/export a public "reserve VA only" API (wrap BuddyAlloc/BuddyFree as a non-static VMemReserve/VFree) and use that, or
- Map the module into a different virtual address (use VMemMap with PAGE_PRESENT | PAGE_WRITABLE rather than remapping or VMemMapMMIO).
- Also ensure InitRD uses VMemMap(...) with normal PAGE_PRESENT|PAGE_WRITABLE flags when mapping module RAM (VMemMapMMIO disables caches and can corrupt regular RAM reads).
kernel/core/InitRD.c:102-103 remain the problematic lines — remove the explicit VMemUnmap and either use VMemFree only or change to the VA-reservation / separate-VA mapping approach.
Likely an incorrect or invalid review comment.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores