Development #129
Development #129
Conversation
WalkthroughStandardizes fault/exception reporting to build and print a unified register dump, replaces per-pixel VESA rendering with clipped bulk memory operations, adjusts boot messaging and console logging, adds a shell "snooze" command with scheduler integration, removes some compiler warning flags, and introduces major heap and VMem allocator/management changes (buddy allocator, TLB batching, page-table caching). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Shell
participant SnoozeHandler
participant Kernel as Kernel (Snooze API)
participant Scheduler as MLFQ
User->>Shell: "snooze on" / "snooze off"
Shell->>SnoozeHandler: dispatch command
SnoozeHandler->>Kernel: Snooze() / Unsnooze()
Note right of Kernel: Toggle kernel print throttling state
Scheduler->>Scheduler: VFCompositorRequestInit()
alt compositor enabled
Scheduler->>Kernel: Snooze()
end
sequenceDiagram
autonumber
participant CPU
participant ISR as Interrupts.c (ISR)
participant Dump as RegistersDumpT
participant Logger as PrintRegisters
CPU-->>ISR: Exception / Fault (e.g., #PF / #GP / #DF)
ISR->>Dump: DumpRegisters()
ISR->>Dump: override RIP, CS, RFLAGS, RSP, SS
ISR->>Logger: PrintRegisters(Dump)
Note over ISR,Logger: Applies to cases 6, 8, 13, 14 and default
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
arch/x86_64/interrupts/Interrupts.c (1)
98-107: Fix: Populate RegistersDumpT from the trappedregs(don’t mix live GPRs with fault-time state)DumpRegisters() snapshots live CPU GPRs; current code calls it then only overwrites RIP/CS/RFLAGS/RSP/SS, producing a mixed and misleading dump. Verified: arch/x86_64/cpu/Cpu.h defines both Registers and RegistersDumpT with the GPR/context fields used. Update arch/x86_64/interrupts/Interrupts.c (occurrences ≈ lines 98–107, 122–128, 136–146) to overwrite all GPRs from
regs(or build dump fromregsand use DumpRegisters() only for CR*/DR*/MSR).Apply this change:
- RegistersDumpT dump = {0}; - DumpRegisters(&dump); - // Override with fault context where applicable - dump.rip = regs->rip; - dump.cs = regs->cs; - dump.rflags = regs->rflags; - dump.rsp = regs->rsp; - dump.ss = regs->ss; - PrintRegisters(&dump); + RegistersDumpT dump = (RegistersDumpT){0}; + DumpRegisters(&dump); // keep for CR*/DR*/MSR + // Overwrite ALL GPRs + context from the trapped frame + dump.rax = regs->rax; dump.rbx = regs->rbx; dump.rcx = regs->rcx; dump.rdx = regs->rdx; + dump.rsi = regs->rsi; dump.rdi = regs->rdi; dump.rbp = regs->rbp; dump.rsp = regs->rsp; + dump.r8 = regs->r8; dump.r9 = regs->r9; dump.r10 = regs->r10; dump.r11 = regs->r11; + dump.r12 = regs->r12; dump.r13 = regs->r13; dump.r14 = regs->r14; dump.r15 = regs->r15; + dump.rip = regs->rip; dump.cs = regs->cs; dump.rflags = regs->rflags; dump.ss = regs->ss; + /* If regs carries DS/ES/FS/GS, overwrite them too for accuracy */ + PrintRegisters(&dump);Optional helper to reuse:
static inline void BuildRegistersDumpFromRegs(const Registers* regs, RegistersDumpT* dump) { memset(dump, 0, sizeof(*dump)); DumpRegisters(dump); // fills CR*/DR*/MSR/seg bases dump->rax = regs->rax; dump->rbx = regs->rbx; dump->rcx = regs->rcx; dump->rdx = regs->rdx; dump->rsi = regs->rsi; dump->rdi = regs->rdi; dump->rbp = regs->rbp; dump->rsp = regs->rsp; dump->r8 = regs->r8; dump->r9 = regs->r9; dump->r10 = regs->r10; dump->r11 = regs->r11; dump->r12 = regs->r12; dump->r13 = regs->r13; dump->r14 = regs->r14; dump->r15 = regs->r15; dump->rip = regs->rip; dump->cs = regs->cs; dump->rflags = regs->rflags; dump->ss = regs->ss; }kernel/etc/Shell.c (1)
915-940: Fix infinite loop in PrintCacheInfo() — CPUID subleaf (ECX) must be variedThe loop calls CPUID leaf 4 without varying ECX and then overwrites EAX with the loop index; cache_type is derived from the wrong value so the loop never terminates. Confirmed in kernel/etc/Shell.c (PrintCacheInfo, ~lines 915–940). Change the CPUID call to pass subleaf i in ECX and use the returned EAX.
void PrintCacheInfo() { uint32_t eax, ebx, ecx, edx; - for (int i = 0; ; ++i) { - cpuid(4, &eax, &ebx, &ecx, &edx); - eax = i; // Subleaf + for (int i = 0; ; ++i) { + __asm__ volatile("cpuid" + : "=a"(eax), "=b"(ebx), "=c"(ecx), "=d"(edx) + : "a"(4), "c"(i)); uint32_t cache_type = eax & 0x1F; if (cache_type == 0) { break; }kernel/core/Kernel.c (1)
79-99: Critical: remove identity-mapping assumptions — use PHYS_TO_VIRT() whenever converting physical page addresses to pointers.
- kernel/core/Kernel.c: AllocPageTable (lines 79–99) returns a physical address but calls FastZeroPage((void*)table_phys). Change to FastZeroPage(PHYS_TO_VIRT(table_phys)) and stop assuming AllocPage() yields identity-mapped pointers.
- kernel/core/Kernel.c: BootstrapMapPage (lines 101–147) builds pointers by casting _phys to uint64_t (pml4 at 109, pdpt at 123, pd at 134, pt at 145). Replace those casts with PHYS_TO_VIRT(...) before dereferencing (e.g., uint64_t pml4 = (uint64_t)PHYS_TO_VIRT(pml4_phys)). Keep PTEs storing physical addresses only.
- kernel/core/Kernel.c: PML4 bootstrap allocation & zeroing (lines ~464–476 and the identity-map loop starting ~478) also rely on identity mapping (FastZeroPage(pml4_phys), mapping paddr->paddr). Zero and dereference via PHYS_TO_VIRT or make identity-mapping explicitly required and asserted.
- Evidence: repository search found ~30 occurrences of *_phys and 0 uses of PHYS_TO_VIRT in kernel; all page-table pointer dereferences must be updated.
Minimal actionable fix: replace any (uint64_t*)_phys with (uint64_t*)PHYS_TO_VIRT(_phys) and FastZeroPage((void*)paddr) with FastZeroPage(PHYS_TO_VIRT(paddr)); do not cast raw physical values to pointers anywhere.
🧹 Nitpick comments (5)
arch/x86_64/interrupts/Interrupts.c (1)
138-146: Unify the default-exception dump and fix GPR accuracy.Apply the same GPR override or helper usage to keep dumps accurate and consistent across all exception paths.
- RegistersDumpT dump = {0}; - DumpRegisters(&dump); - // Override with fault context where applicable - dump.rip = regs->rip; - dump.cs = regs->cs; - dump.rflags = regs->rflags; - dump.rsp = regs->rsp; - dump.ss = regs->ss; - PrintRegisters(&dump); + RegistersDumpT dump; + BuildRegistersDumpFromRegs(regs, &dump); + PrintRegisters(&dump);drivers/Vesa.c (1)
166-176: Consider adding a FastMemset32 for fillsA dedicated 32-bit memset (vectorized, cache-aware) would avoid sourcing from a 1 KiB pattern and reduce L1 pressure in very wide rects. If you add
FastMemset32(void* dst, uint32_t value, uint64_t count_dwords), this loop shrinks to a single call per row.kernel/sched/MLFQ.c (1)
1803-1803: Snooze persists; ensure a matching Unsnooze when the compositor is ready.Calling Snooze() here suppresses PrintKernel globally until someone toggles it. Add Unsnooze() when VFCompositor signals “ready” (or gate Snooze behind a build flag/CLI).
Would you like a tiny IPC “ready” hook in VFCompositor to call Unsnooze()? I can draft it.
kernel/etc/Shell.c (2)
170-182: Help text mismatch: “snoozer” vs command “snooze”.Fix to avoid confusing users.
- {"snoozer <on/off>", "Snooze messages from PrintKernel"}, + {"snooze <on/off>", "Snooze messages from PrintKernel"},
1205-1205: Stray line‑continuation backslash.The trailing “\” after the array initializer is unnecessary and brittle.
-static const ShellCommand commands[] = {\ +static const ShellCommand commands[] = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
arch/x86_64/interrupts/Interrupts.c(3 hunks)drivers/Vesa.c(3 hunks)kernel/core/Kernel.c(2 hunks)kernel/core/Panic.c(0 hunks)kernel/etc/Console.c(0 hunks)kernel/etc/Shell.c(4 hunks)kernel/sched/MLFQ.c(1 hunks)meson.build(0 hunks)
💤 Files with no reviewable changes (3)
- kernel/etc/Console.c
- kernel/core/Panic.c
- meson.build
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, after VBEShowPanic() draws a panic image to the framebuffer, continuing to use text console operations like PrintKernelAt() causes display corruption because it tries to render text on top of the raw pixel data. The panic handler must choose either pure VBE graphics mode (show image only) or pure text console mode, but not mix both.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, mixing VBE graphics mode operations (like VBEShowPanic drawing to framebuffer) with text console operations causes display corruption with random pixels. The panic handler must choose either pure VBE graphics mode or pure text mode, not both simultaneously.
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.
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.
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-06T05:31:15.868Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#111
File: kernel/sched/MLFQ.c:1514-1514
Timestamp: 2025-09-06T05:31:15.868Z
Learning: In the MLFQ scheduler, removing MLFQYield() from critical system processes like DynamoX and Astra improves system responsiveness, indicating that the yield mechanism may be too aggressive for system-critical processes that need quick response times.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-08-10T06:31:43.779Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:31:43.779Z
Learning: In VoidFrame kernel's VBEConsolePutChar() function, the color_buffer must be updated with the current attribute byte whenever a character is stored in the buffer. Without this, characters inherit stale colors from VBEConsoleClear() causing all text to appear in the wrong color (gray/0x08) after refresh operations.
Applied to files:
kernel/core/Kernel.cdrivers/Vesa.c
📚 Learning: 2025-08-23T10:06:02.997Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
Applied to files:
kernel/core/Kernel.carch/x86_64/interrupts/Interrupts.cdrivers/Vesa.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, after VBEShowPanic() draws a panic image to the framebuffer, continuing to use text console operations like PrintKernelAt() causes display corruption because it tries to render text on top of the raw pixel data. The panic handler must choose either pure VBE graphics mode (show image only) or pure text console mode, but not mix both.
Applied to files:
kernel/core/Kernel.cdrivers/Vesa.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, mixing VBE graphics mode operations (like VBEShowPanic drawing to framebuffer) with text console operations causes display corruption with random pixels. The panic handler must choose either pure VBE graphics mode or pure text mode, not both simultaneously.
Applied to files:
kernel/core/Kernel.cdrivers/Vesa.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel's VBEConsoleClear() function, the color is hardcoded to 0x08 (gray on black) instead of using the current console color state, causing color washing issues. The function should use console.color or the current color state to maintain color consistency across screen clears.
Applied to files:
kernel/core/Kernel.cdrivers/Vesa.c
📚 Learning: 2025-08-10T02:19:46.948Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.948Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-03T12:55:44.837Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/core/Kernel.c:736-740
Timestamp: 2025-09-03T12:55:44.837Z
Learning: In VoidFrame kernel's window manager initialization, the Snooze() call in WindowManagerInit() suppresses display output by redirecting console operations to serial-only mode, causing the GUI to render but remain invisible to the user.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-09T09:29:00.630Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#57
File: kernel/memory/VMem.h:64-66
Timestamp: 2025-08-09T09:29:00.630Z
Learning: In the VoidFrame kernel, removing locks from KernelMemoryAlloc/KernelMemoryFree causes heap corruption where shell command strings (like "clear" = 0x61656C63) overwrite heap block magic values due to race conditions between allocation and free operations.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-01T01:10:22.123Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: README.md:9-9
Timestamp: 2025-09-01T01:10:22.123Z
Learning: VoidFrame's interrupt handlers in arch/x86_64/interrupts/Interrupts.asm only save basic GPRs and segment registers, missing critical registers for deep fault tracing: control registers (CR0/CR2/CR3/CR4), SIMD state (XMM/YMM/ZMM), debug registers (DR0-DR7), and extended FPU state. The CPU initialization code enables SSE/AVX/XSAVE features but interrupt handlers don't preserve this extended state. Enhanced fault tracing requires selective extended state saving for critical exceptions (ISR 6, 8, 13, 14) while keeping lightweight handling for regular interrupts.
Applied to files:
arch/x86_64/interrupts/Interrupts.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:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-09-01T01:10:22.123Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: README.md:9-9
Timestamp: 2025-09-01T01:10:22.123Z
Learning: The current VoidFrame Registers struct in arch/x86_64/cpu/Cpu.h only saves basic GPRs, segment registers, and exception context (interrupt_number, error_code, rip, cs, rflags, rsp, ss). It's missing critical registers for deep fault tracing: SIMD registers (XMM/YMM/ZMM), control registers (CR0/CR2/CR3/CR4), debug registers (DR0-DR7), and extended FPU state. The struct has a "DO NOT TOUCH" comment indicating it must match interrupt ASM stack layout.
Applied to files:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-08-13T07:26:56.892Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:26:56.892Z
Learning: VoidFrame kernel uses direct interrupt dispatch in arch/x86_64/interrupts/Interrupts.c rather than RegisterInterruptHandler pattern. Hardware interrupts are handled in switch cases by interrupt number (32+ for IRQs), with proper PIC EOI handling required.
Applied to files:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-08-10T09:50:21.823Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T09:50:21.823Z
Learning: VoidFrame's VBE graphics can be optimized by using FastMemcpy for entire scanlines instead of pixel-by-pixel VBEPutPixel calls, reducing function calls from millions to thousands for full-screen operations.
Applied to files:
drivers/Vesa.c
📚 Learning: 2025-08-10T09:50:21.823Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T09:50:21.823Z
Learning: VoidFrame's VBE graphics can be optimized by using FastMemcpy for entire scanlines instead of pixel-by-pixel VBEPutPixel calls, and SSE2 instructions can fill/gradient 4 pixels at once for 4-8x performance gains.
Applied to files:
drivers/Vesa.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, the console color state (current_color) needs to be explicitly preserved and restored when switching between display modes or clearing the screen. ConsoleSetColor() should update both text mode and VBE mode color states when vbe_enabled is true.
Applied to files:
drivers/Vesa.c
🧬 Code graph analysis (5)
kernel/sched/MLFQ.c (1)
kernel/etc/Console.c (1)
Snooze(50-58)
kernel/core/Kernel.c (1)
kernel/etc/Console.c (2)
ClearScreen(87-109)PrintKernelSuccess(212-217)
kernel/etc/Shell.c (4)
mm/VMem.c (1)
PrintVMemStats(556-570)mm/KernelHeap.c (2)
PrintHeapStats(491-541)KernelFree(444-489)kernel/etc/Console.c (3)
PrintKernel(180-198)Snooze(50-58)Unsnooze(59-61)kernel/etc/StringOps.c (1)
FastStrCmp(43-53)
arch/x86_64/interrupts/Interrupts.c (2)
mm/trace/StackTrace.c (1)
PrintDetailedFaultInfo(196-234)arch/x86_64/cpu/Cpu.c (2)
DumpRegisters(171-176)PrintRegisters(178-201)
drivers/Vesa.c (1)
mm/MemOps.c (1)
FastMemcpy(75-162)
⏰ 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 (7)
arch/x86_64/interrupts/Interrupts.c (1)
98-99: Good call: detailed fault info before the expanded register dump.Ordering improves readability; stack trace + context first, then the full machine-state dump.
drivers/Vesa.c (2)
162-178: Nice speedup on rectangle fillPattern buffer + FastMemcpy per scanline is a solid improvement over per‑pixel stores.
215-217: Direct glyph blit with early clipping looks goodPre-mapped FG/BG and row pointer math are correct; inner loop respects bounds. Good change.
Also applies to: 220-226, 235-237, 239-241, 243-249
kernel/core/Kernel.c (2)
803-805: Snooze exit ordering is fine.Clearing the screen before Unsnooze avoids spurious prints during transition. No change needed.
813-815: Good: move status prints before sti().Safer to emit logs pre-interrupts. Looks good.
kernel/etc/Shell.c (2)
323-336: Nice: extended memory stats.Printing VMEM and heap stats here is useful for ops triage.
1187-1204: SnoozeHandler: free logic OK; fix help entry typo
- Freeing: status is freed on all non-null paths — no change required.
- Help mismatch: kernel/etc/Shell.c shows
"snoozer <on/off>"at line 181 but the command table registers"snooze"at line 1255 — change the help entry to"snooze <on/off>".Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
kernel/etc/Console.c (4)
324-333: Honor snooze in PrintKernelAt and avoid mixed semantics.
PrintKernelAtstill writes to the console even when snoozed, unlikePrintKernel. Unify behavior to prevent unexpected on-screen writes during snooze.void PrintKernelAt(const char* str, uint32_t line, uint32_t col) { if (!str) return; - SerialWrite(str); - SerialWrite("\n"); - // if (snooze) return; + if (__atomic_load_n(&snooze, __ATOMIC_RELAXED)) { + SerialWrite(str); + SerialWrite("\n"); + return; + } + SerialWrite(str); + SerialWrite("\n");Optionally also guard the VGA path with
SpinLock(&lock)/SpinUnlock(&lock)to avoid races withPrintKernel.
54-55: Potential null deref: guard MLFQGetCurrentProcess().If the scheduler is transitioning,
MLFQGetCurrentProcess()could be null. Add a quick check.- if (MLFQGetCurrentProcess()->privilege_level != PROC_PRIV_SYSTEM) + Process* p = MLFQGetCurrentProcess(); + if (!p || p->privilege_level != PROC_PRIV_SYSTEM) return;
194-200: Single-char path now takes the slow route.Wrapping a char into a string funnels through
PrintKernel(lock + serial) per char. If used in loops, this regresses perf. Either avoid calling this in tight loops or reintroduce a direct char path (VBE:VBEConsolePutChar, VGA:ConsolePutchar, Serial:SerialWriteChar).Can you confirm
PrintKernelCharisn’t used in hot per‑char loops (e.g., progress spinners or TTY echo)?
94-99: Use current color when clearing VGA to preserve UI state.Clearing with
VGA_COLOR_DEFAULTignoresconsole.color; after themed messages the screen reverts to default.- const uint16_t blank = MakeVGAEntry(' ', VGA_COLOR_DEFAULT); + const uint16_t blank = MakeVGAEntry(' ', console.color);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
drivers/Vesa.c(3 hunks)kernel/etc/Console.c(1 hunks)meson.build(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- meson.build
- drivers/Vesa.c
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
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.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, mixing VBE graphics mode operations (like VBEShowPanic drawing to framebuffer) with text console operations causes display corruption with random pixels. The panic handler must choose either pure VBE graphics mode or pure text mode, not both simultaneously.
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel's VBEConsoleClear() function, the color is hardcoded to 0x08 (gray on black) instead of using the current console color state, causing color washing issues. The function should use console.color or the current color state to maintain color consistency across screen clears.
Applied to files:
kernel/etc/Console.c
📚 Learning: 2025-08-10T06:31:43.779Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:31:43.779Z
Learning: In VoidFrame kernel's VBEConsolePutChar() function, the color_buffer must be updated with the current attribute byte whenever a character is stored in the buffer. Without this, characters inherit stale colors from VBEConsoleClear() causing all text to appear in the wrong color (gray/0x08) after refresh operations.
Applied to files:
kernel/etc/Console.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, the console color state (current_color) needs to be explicitly preserved and restored when switching between display modes or clearing the screen. ConsoleSetColor() should update both text mode and VBE mode color states when vbe_enabled is true.
Applied to files:
kernel/etc/Console.c
🧬 Code graph analysis (1)
kernel/etc/Console.c (1)
drivers/Serial.c (1)
SerialWrite(129-138)
⏰ 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/etc/Console.c (1)
89-91: Resolved — VBEConsoleClear respects console color (regression fixed).
VBEConsoleClear initializes color_buffer to VBE_CONSOLE_DEFAULT_COLOR (0x07); VBEConsolePutChar writes the computed current_attr into color_buffer and VBEConsoleSetColor updates fg/bg — no further changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mm/VMem.c (1)
636-637: Consider validating virtual address for unmapping.While the function returns early for invalid alignment, it doesn't validate if the virtual address is in a valid range like
VMemMapMMIOdoes.Consider adding
IsValidVirtAddrcheck for consistency:void VMemUnmapMMIO(uint64_t vaddr, uint64_t size) { if (!IS_PAGE_ALIGNED(vaddr) || !IS_PAGE_ALIGNED(size) || size == 0) { return; } + if (!IsValidVirtAddr(vaddr)) { + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)mm/KernelHeap.c(8 hunks)mm/VMem.c(9 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
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.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, mixing VBE graphics mode operations (like VBEShowPanic drawing to framebuffer) with text console operations causes display corruption with random pixels. The panic handler must choose either pure VBE graphics mode or pure text mode, not both simultaneously.
📚 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:
mm/KernelHeap.cmm/VMem.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:
mm/VMem.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:
mm/VMem.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:
mm/VMem.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:
mm/VMem.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:
mm/VMem.c
📚 Learning: 2025-08-16T05:27:10.760Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#90
File: kernel/memory/MemOps.c:103-124
Timestamp: 2025-08-16T05:27:10.760Z
Learning: In VoidFrame kernel VMemAlloc implementation, different virtual addresses on consecutive allocations after freeing is expected behavior because VMemAlloc uses a simple bump allocator (kernel_space.next_vaddr) that only moves forward and doesn't track or reuse freed virtual address ranges.
Applied to files:
mm/VMem.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:
mm/VMem.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:
mm/VMem.c
📚 Learning: 2025-08-09T10:31:44.439Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#58
File: drivers/VesaBIOSExtension.c:77-85
Timestamp: 2025-08-09T10:31:44.439Z
Learning: In VoidFrame kernel, process stacks are allocated using VMemAllocWithGuards() which returns virtual addresses, but TerminateProcess incorrectly uses FreePage() (which expects physical addresses) to free them, causing "Page index out of bounds" errors. The correct function to use is VMemFreeWithGuards(proc->stack, STACK_SIZE).
Applied to files:
mm/VMem.c
📚 Learning: 2025-09-02T01:55:54.511Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T01:55:54.511Z
Learning: In VoidFrame's Cerberus system, CerberusInstallStackCanary fails because it incorrectly calculates the canary address. VMemAllocStack returns stack_top (highest address) but the canary calculation uses stack_base + stack_size - 8, which results in an address beyond the allocated stack region. The correct canary location should be calculated as actual_stack_base + sizeof(uint64_t) where actual_stack_base = stack_top - stack_size, placing the canary just above the unmapped guard page.
Applied to files:
mm/VMem.c
🧬 Code graph analysis (2)
mm/KernelHeap.c (1)
kernel/atomic/Spinlock.h (2)
SpinLockIrqSave(154-159)SpinUnlockIrqRestore(161-164)
mm/VMem.c (3)
mm/PMem.c (2)
AllocPage(178-234)FreePage(276-314)mm/MemOps.c (1)
FastZeroPage(164-202)kernel/atomic/Spinlock.h (2)
SpinLockIrqSave(154-159)SpinUnlockIrqRestore(161-164)
🔇 Additional comments (28)
.gitignore (1)
11-12: LGTM! Reasonable addition to gitignore.Adding
.amazonqto the gitignore file is appropriate for excluding Amazon Q Developer IDE extension files from version control.mm/KernelHeap.c (9)
50-51: Good addition of periodic coalescing counter.The
alloc_counterfor triggering periodic coalescing is a reasonable approach to combat fragmentation over time.
59-61: Forward declaration follows good C practice.Properly declaring
CoalesceBlockbefore use maintains clean code organization.
171-176: Excellent improvement: coalesce instead of dropping on cache overflow.This change prevents memory waste by coalescing blocks when the cache is full rather than silently dropping them. This should improve memory utilization.
294-322: Well-implemented bidirectional coalescing logic.The new
CoalesceBlockfunction properly:
- Removes adjacent blocks from caches before merging
- Handles both forward and backward coalescing
- Updates checksums after structural changes
- Returns the potentially relocated block pointer
The logic correctly handles the case where backward coalescing changes the block pointer.
324-338: Clean implementation of cache flush with coalescing.
FlushCacheAndCoalesceproperly drains all caches and coalesces each block, which should help reduce fragmentation during low-memory situations or periodic maintenance.
365-370: Smart periodic coalescing trigger.Triggering coalescing every 1000 allocations is a reasonable heuristic. The implementation correctly:
- Uses IRQ-safe locking
- Releases the lock after flushing
- Increments the counter atomically (via prefix increment)
285-285: Good defensive programming in cache removal.Setting
cache_next = NULLafter removing from cache prevents potential use-after-free issues.
341-355: Proper initialization of cache statistics.The initialization correctly sets up all cache fields including the new statistics counters.
300-301: Ensure CacheRemove is thread-safe / called under the proper lockCacheRemove only checks blk->in_cache before removing, while coalescing calls CacheRemove on adjacent blocks (mm/KernelHeap.c — coalesce with next: ~299-304; coalesce with prev: ~309-315). Verify one of the following is true and fix if not: either callers hold the appropriate cache/heap lock when calling CacheRemove, or CacheRemove itself acquires the cache lock and performs an atomic test-and-clear of in_cache to avoid races (double-remove / use-after-free).
mm/VMem.c (18)
18-22: Good addition of free block pool for region-based allocator.The pre-allocated pool avoids dynamic allocation issues when managing the free list.
23-27: Excellent TLB batching implementation.Batching TLB invalidations can significantly improve performance by reducing the number of expensive TLB flush operations.
28-32: Smart optimization with identity-mapped page table cache.Caching identity-mapped page tables reduces allocation overhead and improves locality of reference for page table operations.
67-76: Critical improvement: proper address validation.The new validation functions add essential safety checks:
IsValidPhysAddr: Validates against total physical memoryIsValidVirtAddr: Properly checks canonical address rangesThis prevents invalid memory accesses and potential security issues.
78-81: Clever optimization for page table access.
GetTableVirtcorrectly uses identity mapping when available (< 4GB) and falls back to kernel virtual mapping otherwise. This aligns with the learned context about VoidFrame's memory layout.
83-95: Well-designed TLB flush batching logic.The implementation intelligently chooses between:
- Full TLB flush for >8 entries (more efficient)
- Individual invalidations for ≤8 entries
This is a good performance trade-off.
121-127: Good page table caching logic.The cache correctly stores identity-mapped pages for reuse and falls back to freeing non-identity-mapped pages.
158-179: Critical fix: proper page table access with validation.The updated
VMemGetPageTablePhysnow:
- Validates physical addresses before use
- Uses
GetTableVirtfor safe access- Changed level parameter to
uint32_t(more appropriate for bit shifting)This addresses the learned issues about identity mapping assumptions causing panics.
185-187: Essential safety check added.Adding
IsValidPhysAddrandIsValidVirtAddrchecks prevents mapping invalid addresses that could cause system instability.
218-218: Good integration of TLB batching.Using
add_to_tlb_batchinstead of immediate invalidation improves performance. The subsequentflush_tlb_batchensures consistency.Also applies to: 255-255, 335-335
261-265: Smart allocation strategy for region selection.Routing large allocations (≥2MB) to the lower canonical region for better TLB efficiency is a good optimization that can reduce TLB misses for large memory blocks.
325-326: Excellent error recovery with rollback.Calling
VMemFreeon allocation failure properly cleans up partially allocated virtual address space, preventing leaks.Also applies to: 330-331
460-468: Critical fix for large page handling.The updated logic correctly:
- Masks the base address for 2MB pages
- Preserves the offset within the huge page
This fixes potential address calculation errors with large pages.
506-548: Improved unmapping with batched TLB operations.The refactored
VMemUnmap:
- Properly uses TLB batching for better performance
- Correctly handles both 4KB and 2MB pages
- Uses continue statements for cleaner control flow
586-588: Good validation for MMIO mappings.Adding
IsValidVirtAddrcheck prevents mapping MMIO to invalid virtual addresses.
593-594: Consistent batching in MMIO operations.The MMIO map/unmap functions now properly use TLB batching with appropriate locking, improving performance for multi-page MMIO regions.
Also applies to: 629-630
162-163: Shift calculation is safe — callers use level 0–2All occurrences of VMemGetPageTablePhys in mm/VMem.c pass level 0, 1, or 2, so
39U - (level * 9U)is safe for current callers.
104-119: Re-evaluate alloc_identity_page_table retry limit (32 attempts)alloc_identity_page_table (mm/VMem.c:104–119) returns NULL after 32 attempts; callers (VMemGetPageTablePhys at mm/VMem.c:165–172) already propagate this as VMEM_ERROR_NOMEM — decide whether 32 retries are acceptable under memory pressure (options: increase the limit, implement backoff/scanning for a low phys page, or document/accept the behavior).
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
mm/VMem.c (5)
78-81: Potential integer overflow in GetOrder calculationThe order calculation could overflow for very large sizes near the upper boundary of uint64_t.
static inline uint32_t GetOrder(uint64_t size) { if (size <= PAGE_SIZE) return 0; + if (size > (1ULL << BUDDY_MAX_ORDER)) return BUDDY_NUM_ORDERS; // Too large return 64 - __builtin_clzll(size - 1) - BUDDY_MIN_ORDER; }
203-218: alloc_identity_page_table may fail unnecessarilyThe function tries 32 times to get an identity-mapped page, but this could exhaust quickly on systems with limited low memory.
Consider:
- Maintaining a dedicated pool of identity-mapped pages for page tables
- Using a more sophisticated allocation strategy
- Falling back to high memory with proper PHYS_TO_VIRT mapping
static void* alloc_identity_page_table(void) { if (pt_cache_count > 0) { return pt_cache[--pt_cache_count]; } + // Try identity-mapped first for (uint32_t attempt = 0; attempt < 32; attempt++) { void* candidate = AllocPage(); if (!candidate) break; if ((uint64_t)candidate < IDENTITY_MAP_SIZE) { FastZeroPage(candidate); return candidate; } - FreePage(candidate); + // Cache for later use if it's not identity-mapped + cache_page_table(candidate); } - return NULL; + + // Fall back to any page and handle mapping properly + void* page = AllocPage(); + if (page) FastZeroPage(page); + return page; }
282-303: VMemGetPageTablePhys level calculation uses magic numbersThe shift calculation
39U - (level * 9U)uses magic numbers that make the code harder to understand and maintain.+// Define shifts for each level +static const uint32_t level_shifts[] = { + PML4_SHIFT, // Level 0: 39 + PDP_SHIFT, // Level 1: 30 + PD_SHIFT, // Level 2: 21 + PT_SHIFT // Level 3: 12 +}; + static uint64_t VMemGetPageTablePhys(uint64_t pml4_phys, uint64_t vaddr, uint32_t level, int create) { if (!IsValidPhysAddr(pml4_phys)) return 0; + if (level > 3) return 0; uint64_t* table_virt = GetTableVirt(pml4_phys); - uint32_t shift = 39U - (level * 9U); + uint32_t shift = level_shifts[level]; uint32_t index = (vaddr >> shift) & PT_INDEX_MASK;
626-629: MMIO validation only checks virtual addressVMemMapMMIO validates the virtual address but not the physical address, unlike VMemMap which validates both. MMIO addresses may be outside the normal RAM range, but some validation might still be useful.
Consider adding a warning or a separate validation for MMIO physical addresses to catch obvious errors:
int VMemMapMMIO(uint64_t vaddr, uint64_t paddr, uint64_t size, uint64_t flags) { if (!IS_PAGE_ALIGNED(vaddr) || !IS_PAGE_ALIGNED(paddr) || !IS_PAGE_ALIGNED(size)) { return VMEM_ERROR_ALIGN; } if (!IsValidVirtAddr(vaddr)) { return VMEM_ERROR_INVALID_ADDR; } + + // Warn if MMIO address looks like regular RAM + if (paddr < (total_pages * PAGE_SIZE)) { + PrintKernelWarning("[VMEM] MMIO mapping to RAM address range: "); + PrintKernelHex(paddr); + PrintKernel("\n"); + }
18-21: Increase BUDDY_MAX_ORDER or add large-region handling — 1GB max yields ~130k blocks for 128TBBUDDY_MIN_ORDER=12 .. BUDDY_MAX_ORDER=30 => max block = 1GB. Verified: low region ≈128.0 TB requires 131,071 1GB blocks; high region ≈126.0 TB requires 129,024 1GB blocks. Raise BUDDY_MAX_ORDER (order 47 would represent 128 TB in one block) or implement region-level coalescing / a separate large-region allocator to avoid massive free-list/metadata overhead.
File: mm/VMem.c lines 18-21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mm/VMem.c(9 hunks)mm/VMem.h(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
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.
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.
📚 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:
mm/VMem.hmm/VMem.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:
mm/VMem.hmm/VMem.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:
mm/VMem.h
📚 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:
mm/VMem.hmm/VMem.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:
mm/VMem.hmm/VMem.c
📚 Learning: 2025-08-16T05:27:10.760Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#90
File: kernel/memory/MemOps.c:103-124
Timestamp: 2025-08-16T05:27:10.760Z
Learning: In VoidFrame kernel VMemAlloc implementation, different virtual addresses on consecutive allocations after freeing is expected behavior because VMemAlloc uses a simple bump allocator (kernel_space.next_vaddr) that only moves forward and doesn't track or reuse freed virtual address ranges.
Applied to files:
mm/VMem.hmm/VMem.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:
mm/VMem.hmm/VMem.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:
mm/VMem.c
📚 Learning: 2025-08-09T10:31:44.439Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#58
File: drivers/VesaBIOSExtension.c:77-85
Timestamp: 2025-08-09T10:31:44.439Z
Learning: In VoidFrame kernel, process stacks are allocated using VMemAllocWithGuards() which returns virtual addresses, but TerminateProcess incorrectly uses FreePage() (which expects physical addresses) to free them, causing "Page index out of bounds" errors. The correct function to use is VMemFreeWithGuards(proc->stack, STACK_SIZE).
Applied to files:
mm/VMem.c
📚 Learning: 2025-09-02T01:55:54.511Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T01:55:54.511Z
Learning: In VoidFrame's Cerberus system, CerberusInstallStackCanary fails because it incorrectly calculates the canary address. VMemAllocStack returns stack_top (highest address) but the canary calculation uses stack_base + stack_size - 8, which results in an address beyond the allocated stack region. The correct canary location should be calculated as actual_stack_base + sizeof(uint64_t) where actual_stack_base = stack_top - stack_size, placing the canary just above the unmapped guard page.
Applied to files:
mm/VMem.c
🧬 Code graph analysis (1)
mm/VMem.c (4)
mm/PMem.c (2)
AllocPage(178-234)FreePage(276-314)mm/MemOps.c (1)
FastZeroPage(164-202)kernel/etc/Console.c (4)
PrintKernelSuccess(206-211)PrintKernelHex(228-251)PrintKernel(174-192)PrintKernelInt(298-322)kernel/atomic/Spinlock.h (2)
SpinLockIrqSave(154-159)SpinUnlockIrqRestore(161-164)
🔇 Additional comments (4)
mm/VMem.h (1)
86-92: Structure simplified correctly for buddy allocator architectureThe removal of
next_vaddr_low,next_vaddr_high,free_list_low, andfree_list_highfields fromVirtAddrSpacealigns perfectly with the new buddy allocator implementation. The structure now only tracks essential state: the page table root and usage metrics.mm/VMem.c (3)
507-509: Large page physical address calculation looks correctThe 2MB large page handling properly masks the physical base address and combines it with the page offset.
183-194: Efficient TLB batch flushing implementationThe TLB batching optimization with the threshold of 8 for full flush vs individual invalidations is a good performance improvement that reduces the overhead of repeated TLB operations.
177-180: Verified — bootstrap establishes 4GB identity mapping and VMem validates it.IDENTITY_MAP_SIZE is defined (mm/VMem.h:15); the bootstrap maps 0..IDENTITY_MAP_SIZE via BootstrapMapPage (kernel/core/Kernel.c:478-481) and VMem performs runtime checks that PANIC if the identity mapping is not present (mm/VMem.c:269-276). GetTableVirt (mm/VMem.c:177-180) therefore relies on a validated identity map.
1 participant
Summary by CodeRabbit
New Features
Performance
Improvements
Chores