Development #58
Development #58
Conversation
WalkthroughThis update introduces a VESA BIOS Extensions (VBE) graphics driver, including header and implementation files, and expands the serial driver with robust error handling, multi-port support, and new I/O utilities. The kernel is updated to initialize the serial and VBE drivers at startup, displaying a graphical splash if VBE is available. Supporting changes include new absolute value functions, build script updates, multiboot2 header enhancements for framebuffer and VBE info, and minor fixes in other drivers and assembly headers. Changes
Sequence Diagram(s)Kernel Startup with VBE and Serial InitializationsequenceDiagram
participant Bootloader
participant KernelMain
participant SerialDriver
participant VBEDriver
Bootloader->>KernelMain: Pass multiboot2 info
KernelMain->>SerialDriver: SerialInit()
SerialDriver-->>KernelMain: Status (success/fail)
KernelMain->>VBEDriver: VBEInit(multiboot_info_addr)
VBEDriver-->>KernelMain: Status (success/fail)
alt VBE available
KernelMain->>VBEDriver: VBEShowSplash()
KernelMain->>VBEDriver: VBESwitchToTextMode()
else
KernelMain->>SerialDriver: SerialWrite("No graphics, text mode.")
end
KernelMain->>...: Continue system init
Serial Driver: Transmitting a CharactersequenceDiagram
participant Caller
participant SerialDriver
Caller->>SerialDriver: SerialWriteChar(c)
SerialDriver->>SerialDriver: Check initialized
alt Not initialized
SerialDriver-->>Caller: Error code
else
SerialDriver->>SerialDriver: Wait for transmit buffer empty (with timeout)
alt Timeout
SerialDriver-->>Caller: Error code
else
SerialDriver->>SerialDriver: Write char to port
SerialDriver-->>Caller: Success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
arch/x86_64/asm/pxs.asm (1)
23-26: Optional: request-required flagIf the kernel truly needs VBE info, set bit 0 in ‘flags’ so the bootloader aborts gracefully when unavailable.
- dw 0 ; flags + dw 1 ; flags – tag is requireddrivers/VesaBIOSExtension.c (2)
170-205: Consider allowing force switch to text mode even if VBEInit failedThe early return on !vbe_initialized prevents forcing VGA text mode in scenarios where the bootloader left graphics mode but VBE init failed. A force path can be useful for recovery.
Option: add a VBESwitchToTextModeForce() or accept a flag.
141-151: Char rendering is a placeholderAcknowledged. If this is intentional for the demo, add a TODO to wire in a tiny bitmap font (e.g., 8x8) to make text usable in graphics mode.
drivers/Serial.c (2)
103-122: Add CPU relax while polling; avoid hot spins on slow UARTsTight polling loops can unnecessarily hammer the pipeline. Insert a PAUSE to be kinder to SMT/OOO cores and reduce power, and optionally extend the timeout.
Apply this diff:
- while (!SerialTransmitEmpty() && --timeout > 0); + while (!SerialTransmitEmpty() && --timeout > 0) { + __asm__ volatile("pause"); + } @@ - while (!SerialTransmitEmpty() && --timeout > 0); + while (!SerialTransmitEmpty() && --timeout > 0) { + __asm__ volatile("pause"); + }
181-208: SerialReadLine busy-waits; optionally yield/relaxThe loop spins continuously if no data arrives. If feasible in your scheduling model, add pause/hlt (post-sti) or a small backoff to reduce CPU burn.
Example minimal change:
- if (c < 0) continue; // No data, keep waiting + if (c < 0) { __asm__ volatile("pause"); continue; } // No data, keep waiting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
arch/x86_64/asm/pxs.asm(1 hunks)drivers/Keyboard.c(0 hunks)drivers/Pic.c(2 hunks)drivers/Serial.c(1 hunks)drivers/Serial.h(1 hunks)drivers/VesaBIOSExtension.c(1 hunks)drivers/VesaBIOSExtension.h(1 hunks)include/stdlib.h(1 hunks)kernel/core/Kernel.c(2 hunks)meson.build(1 hunks)
💤 Files with no reviewable changes (1)
- drivers/Keyboard.c
🧰 Additional context used
🧠 Learnings (2)
📚 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:
arch/x86_64/asm/pxs.asmkernel/core/Kernel.cdrivers/VesaBIOSExtension.c
📚 Learning: 2025-08-09T04:47:56.871Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#53
File: kernel/core/Kernel.c:256-259
Timestamp: 2025-08-09T04:47:56.871Z
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/Kernel.c
🧬 Code Graph Analysis (4)
drivers/VesaBIOSExtension.h (1)
drivers/VesaBIOSExtension.c (12)
VBEInit(32-75)VBEPutPixel(77-84)VBEGetPixel(86-93)VBEFillScreen(95-103)VBEDrawRect(105-113)VBEDrawLine(115-139)VBEDrawChar(141-151)VBEDrawString(153-168)VBEShowSplash(210-232)VBEGetInfo(234-236)VBEIsInitialized(238-240)VBESwitchToTextMode(170-205)
drivers/Serial.c (1)
include/Io.h (2)
outb(6-8)inb(10-14)
drivers/Serial.h (1)
drivers/Serial.c (10)
SerialInit(45-47)SerialInitPort(49-91)SerialTransmitEmpty(93-96)SerialDataAvailable(98-101)SerialWriteChar(103-122)SerialReadChar(124-132)SerialWrite(134-143)SerialReadLine(182-208)SerialWriteHex(145-157)SerialWriteDec(159-179)
drivers/VesaBIOSExtension.c (3)
drivers/Serial.c (3)
SerialWrite(134-143)SerialWriteHex(145-157)SerialWriteDec(159-179)include/stdlib.h (1)
ABSi(4-9)include/Io.h (1)
outb(6-8)
🔇 Additional comments (5)
drivers/Pic.c (1)
15-15: No functional change detected — OK to ignore.Also applies to: 53-53
meson.build (1)
82-98: Build-list re-ordering looks saneSource list update correctly adds
VesaBIOSExtension.c; no duplicate entries spotted.kernel/core/Kernel.c (1)
26-26: Header inclusion OKIncluding VesaBIOSExtension.h here is appropriate and isolates VBE usage to Kernel.c.
drivers/VesaBIOSExtension.h (1)
28-41: API surface looks goodInit, drawing, text mode switch, and info accessors are coherent for early boot usage.
drivers/Serial.c (1)
49-57: Scratch register probe is goodThis is a simple, effective presence test before full init.
| // Number output | ||
| void SerialWriteHex(uint64_t value); | ||
| void SerialWriteDec(uint64_t value); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return-type inconsistency hampers error handling
All other I/O functions now return int for status; SerialWriteHex / SerialWriteDec still return void, so callers cannot detect transmit time-outs or un-initialised port.
-void SerialWriteHex(uint64_t value);
-void SerialWriteDec(uint64_t value);
+int SerialWriteHex(uint64_t value);
+int SerialWriteDec(uint64_t value);Propagate the return codes inside the implementations for uniform API.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Number output | |
| void SerialWriteHex(uint64_t value); | |
| void SerialWriteDec(uint64_t value); | |
| // Number output | |
| int SerialWriteHex(uint64_t value); | |
| int SerialWriteDec(uint64_t value); |
🤖 Prompt for AI Agents
In drivers/Serial.h at lines 30 to 33, change the return type of SerialWriteHex
and SerialWriteDec from void to int to match other I/O functions. Update their
implementations to return status codes indicating success or failure, such as
transmit time-outs or uninitialized port errors, so callers can handle errors
uniformly.
| int VBEInit(uint32_t multiboot_info_addr) { | ||
| SerialWrite("VBE: Parsing Multiboot2 info...\n"); | ||
|
|
||
| // Skip the total size field (first 8 bytes) | ||
| uint8_t *tag_ptr = (uint8_t*)(multiboot_info_addr + 8); | ||
|
|
||
| while (1) { | ||
| multiboot_tag_t *tag = (multiboot_tag_t*)tag_ptr; | ||
|
|
||
| if (tag->type == 0) break; // End tag | ||
|
|
||
| if (tag->type == MULTIBOOT_TAG_FRAMEBUFFER) { | ||
| multiboot_tag_framebuffer_t *fb_tag = (multiboot_tag_framebuffer_t*)tag; | ||
|
|
||
| vbe_info.framebuffer = (uint32_t*)fb_tag->framebuffer_addr; | ||
| vbe_info.width = fb_tag->framebuffer_width; | ||
| vbe_info.height = fb_tag->framebuffer_height; | ||
| vbe_info.bpp = fb_tag->framebuffer_bpp; | ||
| vbe_info.pitch = fb_tag->framebuffer_pitch; | ||
|
|
||
| SerialWrite("VBE: Found framebuffer!\n"); | ||
| SerialWrite(" Address: 0x"); | ||
| SerialWriteHex(fb_tag->framebuffer_addr); | ||
| SerialWrite("\n Resolution: "); | ||
| SerialWriteDec(vbe_info.width); | ||
| SerialWrite("x"); | ||
| SerialWriteDec(vbe_info.height); | ||
| SerialWrite("x"); | ||
| SerialWriteDec(vbe_info.bpp); | ||
| SerialWrite("\n Pitch: "); | ||
| SerialWriteDec(vbe_info.pitch); | ||
| SerialWrite("\n"); | ||
|
|
||
| vbe_initialized = 1; | ||
| return 0; | ||
| } | ||
|
|
||
| // Move to next tag (align to 8 bytes) | ||
| tag_ptr += ((tag->size + 7) & ~7); | ||
| } | ||
|
|
||
| SerialWrite("VBE: No framebuffer found in Multiboot info\n"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Bounded tag parsing; validate pixel format; store a volatile framebuffer
Tag walk should be bounded by total_size, and the driver must reject unsupported formats. Current code assumes 32bpp and will corrupt memory on 24bpp due to pitch/4 math.
Apply this diff:
int VBEInit(uint32_t multiboot_info_addr) {
SerialWrite("VBE: Parsing Multiboot2 info...\n");
- // Skip the total size field (first 8 bytes)
- uint8_t *tag_ptr = (uint8_t*)(multiboot_info_addr + 8);
+ // Multiboot2 header: [total_size(4)][reserved(4)] followed by tags
+ uint32_t total_size = *(uint32_t*)(uintptr_t)multiboot_info_addr;
+ uint8_t *tag_ptr = (uint8_t*)(uintptr_t)(multiboot_info_addr + 8);
+ uint8_t *tag_end = (uint8_t*)(uintptr_t)(multiboot_info_addr + total_size);
- while (1) {
+ while (tag_ptr + sizeof(multiboot_tag_t) <= tag_end) {
multiboot_tag_t *tag = (multiboot_tag_t*)tag_ptr;
- if (tag->type == 0) break; // End tag
+ if (tag->type == 0) break; // End tag (size must be 8)
if (tag->type == MULTIBOOT_TAG_FRAMEBUFFER) {
multiboot_tag_framebuffer_t *fb_tag = (multiboot_tag_framebuffer_t*)tag;
- vbe_info.framebuffer = (uint32_t*)fb_tag->framebuffer_addr;
+ if (fb_tag->framebuffer_bpp != 32) {
+ SerialWrite("VBE: Unsupported bpp (expect 32)\n");
+ return -2;
+ }
+ vbe_info.framebuffer = (volatile uint32_t*)(uintptr_t)fb_tag->framebuffer_addr;
vbe_info.width = fb_tag->framebuffer_width;
vbe_info.height = fb_tag->framebuffer_height;
vbe_info.bpp = fb_tag->framebuffer_bpp;
vbe_info.pitch = fb_tag->framebuffer_pitch;
SerialWrite("VBE: Found framebuffer!\n");
SerialWrite(" Address: 0x");
SerialWriteHex(fb_tag->framebuffer_addr);
SerialWrite("\n Resolution: ");
SerialWriteDec(vbe_info.width);
SerialWrite("x");
SerialWriteDec(vbe_info.height);
SerialWrite("x");
SerialWriteDec(vbe_info.bpp);
SerialWrite("\n Pitch: ");
SerialWriteDec(vbe_info.pitch);
SerialWrite("\n");
vbe_initialized = 1;
return 0;
}
// Move to next tag (align to 8 bytes)
- tag_ptr += ((tag->size + 7) & ~7);
+ if (tag->size == 0) break; // Corrupt tag guard
+ tag_ptr += ((tag->size + 7) & ~7);
}
SerialWrite("VBE: No framebuffer found in Multiboot info\n");
return -1;
}🤖 Prompt for AI Agents
In drivers/VesaBIOSExtension.c between lines 32 and 75, the tag parsing loop
lacks bounds checking against the total size of the multiboot info, risking
out-of-bounds reads. Also, the code assumes a 32bpp pixel format without
validating it, which can cause memory corruption on 24bpp formats due to
incorrect pitch calculations. To fix this, add a boundary check to ensure the
tag pointer does not exceed the total size field from the multiboot info header,
validate that the framebuffer pixel format is supported (e.g., 32bpp), and
declare the framebuffer pointer as volatile to prevent unwanted compiler
optimizations.
| void VBEPutPixel(uint32_t x, uint32_t y, uint32_t color) { | ||
| if (!vbe_initialized || x >= vbe_info.width || y >= vbe_info.height) { | ||
| return; | ||
| } | ||
|
|
||
| uint32_t offset = y * (vbe_info.pitch / 4) + x; | ||
| vbe_info.framebuffer[offset] = color; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pixel write/read assume 32bpp; guard and use stride in pixels
Current offset math divides pitch by 4 and writes 32 bits. Guard on bpp to avoid accidental over/under-writes.
Apply this diff:
void VBEPutPixel(uint32_t x, uint32_t y, uint32_t color) {
- if (!vbe_initialized || x >= vbe_info.width || y >= vbe_info.height) {
+ if (!vbe_initialized || x >= vbe_info.width || y >= vbe_info.height || vbe_info.bpp != 32) {
return;
}
- uint32_t offset = y * (vbe_info.pitch / 4) + x;
- vbe_info.framebuffer[offset] = color;
+ const uint32_t stride = vbe_info.pitch >> 2; // bytes->dwords
+ vbe_info.framebuffer[y * stride + x] = color;
}
uint32_t VBEGetPixel(uint32_t x, uint32_t y) {
- if (!vbe_initialized || x >= vbe_info.width || y >= vbe_info.height) {
+ if (!vbe_initialized || x >= vbe_info.width || y >= vbe_info.height || vbe_info.bpp != 32) {
return 0;
}
- uint32_t offset = y * (vbe_info.pitch / 4) + x;
- return vbe_info.framebuffer[offset];
+ const uint32_t stride = vbe_info.pitch >> 2;
+ return vbe_info.framebuffer[y * stride + x];
}Also applies to: 86-93
🤖 Prompt for AI Agents
In drivers/VesaBIOSExtension.c around lines 77 to 85, the pixel write function
assumes a 32 bits per pixel (bpp) format by dividing the pitch by 4 and writing
32-bit colors without checking the actual bpp. To fix this, add a guard to
verify that the vbe_info.bpp is 32 before performing the write to avoid
accidental over- or under-writes. Also, use the pitch as stride in pixels
correctly without hardcoding division by 4. Apply the same fix to lines 86 to 93
where pixel read/write occurs.
There was a problem hiding this comment.
also just asking will i able to go back the real mode service stage to switched to 0xb8000 after i change to long mode and enable all cpu memory protection 😇? also we have a critical issue, when a process returns, rather than terminating that process, it GPF the system.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for process termination, exit handling, and GPF handlers
echo "=== Searching for process exit/termination handling ==="
rg -A 5 "process.*exit|process.*terminate|ProcessExit|ProcessTerminate" --type c
echo -e "\n=== Searching for GPF handlers ==="
rg -A 10 "GPF|GeneralProtectionFault|general_protection" --type c
echo -e "\n=== Looking for process return/exit syscalls ==="
ast-grep --pattern 'exit($_) {
$$$
}'
echo -e "\n=== Searching for process management code ==="
fd -e c -e h "Process|Task|Scheduler" --exec head -20 {}
echo -e "\n=== Checking syscall implementations for exit ==="
rg -A 10 "SYS_EXIT|sys_exit|syscall.*exit" --type c
Length of output: 5232
🏁 Script executed:
#!/bin/bash
# Find KillProcess and TerminateProcess implementations
echo "=== Searching for KillProcess implementation ==="
rg -A 20 "^[^/]*KillProcess\s*\(" --type c
echo -e "\n=== Searching for TerminateProcess implementation ==="
rg -A 20 "^[^/]*TerminateProcess\s*\(" --type c
echo -e "\n=== Looking for process entry point and return handling ==="
rg -A 10 "process.*entry|process.*return|ret.*process" --type c
echo -e "\n=== Checking how processes are created and their stack setup ==="
rg -A 15 "CreateProcess|SpawnProcess|process.*create" --type c | head -50
Length of output: 14421
🏁 Script executed:
#!/bin/bash
# Find stack allocation and setup
echo "=== Searching for process stack allocation ==="
rg -A 10 "stack.*alloc|AllocStack|process.*stack" --type c
echo -e "\n=== Looking at ProcessExitStub and how it's used ==="
rg -B 5 -A 10 "ProcessExitStub" --type c
echo -e "\n=== Checking CreateSecureProcess for stack setup ==="
rg -B 5 -A 30 "CreateSecureProcess" --type c | head -80
echo -e "\n=== Looking for process context setup ==="
ast-grep --pattern 'processes[$_].context.rsp = $_'
Length of output: 8037
🏁 Script executed:
#!/bin/bash
# Find where process context (especially RSP) is initialized
echo "=== Looking for context initialization ==="
rg -A 20 "processes\[.*\]\.context\." --type c
echo -e "\n=== Finding STACK_SIZE definition ==="
rg "define.*STACK_SIZE" --type c --type h
echo -e "\n=== Looking for stack pointer setup in assembly ==="
fd -e asm -e s | xargs grep -l "rsp" | head -5
echo -e "\n=== Check if there's a wrapper that sets return address ==="
rg -A 10 "push.*ProcessExitStub|call.*entry_point" --type c --type asm
Length of output: 2253
🏁 Script executed:
#!/bin/bash
# Find the complete CreateSecureProcess function to see rsp calculation
echo "=== Complete CreateSecureProcess function ==="
rg -B 10 "processes\[slot\]\.context\.rsp = rsp" --type c
echo -e "\n=== Looking for process wrapper or trampoline ==="
rg -A 5 -B 5 "trampoline|wrapper|process_start" --type c
echo -e "\n=== Check interrupt/syscall return path ==="
rg -A 10 "iretq|sysretq" --type asm
Length of output: 3005
@assembler-0, let me address both of your questions:
1. Returning to Real Mode from Long Mode
No, you cannot go back to real mode services once you're in long mode with memory protection enabled. The transition path would be: Long Mode → Protected Mode → Real Mode, which requires:
- Disabling paging
- Switching GDT
- Clearing control registers
- This is impractical and dangerous in a running OS
However, you don't need real mode to access text mode at 0xB8000! This is memory-mapped I/O that works perfectly from long mode. Your VBESwitchToTextMode() function already does this correctly by:
- Writing VGA registers directly using port I/O
- Clearing the text buffer at 0xB8000
- No real mode needed!
2. Critical GPF Issue on Process Return
I found the problem! When a process is created, the stack pointer is set but no return address is pushed:
// In CreateSecureProcess:
uint64_t rsp = (uint64_t)stack + STACK_SIZE;
rsp &= ~0xF; // 16-byte alignment
processes[slot].context.rsp = rsp;
processes[slot].context.rip = (uint64_t)entry_point; // Direct entry, no return!When your process function returns, it tries to pop a non-existent return address → GPF!
Solution:
Push ProcessExitStub as the return address on the stack:
// In CreateSecureProcess, after allocating stack:
uint64_t rsp = (uint64_t)stack + STACK_SIZE;
rsp &= ~0xF; // 16-byte alignment
// Push ProcessExitStub as return address
rsp -= 8;
*(uint64_t*)rsp = (uint64_t)ProcessExitStub;
processes[slot].context.rsp = rsp;
processes[slot].context.rip = (uint64_t)entry_point;This way, when a process returns, it will "return" to ProcessExitStub, which properly terminates the process instead of causing a GPF.
Would you like me to open an issue to track this critical process termination bug?
🧠 Learnings used
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.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#53
File: kernel/core/Kernel.c:256-259
Timestamp: 2025-08-09T04:47:56.871Z
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.
There was a problem hiding this comment.
ok ok, it works (the switch to 0xb800 doesnt work), but this [MEMORY] FreePage: Page index out of bounds: 4503565267632165, huh?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for FreePage implementation and the error message
echo "=== Searching for FreePage implementation ==="
rg -B 5 -A 15 "FreePage.*out of bounds|FreePage.*Page index" --type c
echo -e "\n=== Looking for FreePage function ==="
ast-grep --pattern 'FreePage($_) {
$$$
}'
echo -e "\n=== Searching for any FreePage function ==="
rg -B 5 -A 20 "^[^/]*FreePage\s*\(" --type c
echo -e "\n=== Looking for memory page management structures ==="
rg -A 10 "PAGE_SIZE|page_index|PageIndex" --type c --type h | head -50
echo -e "\n=== Check VBESwitchToTextMode implementation ==="
rg -A 30 "VBESwitchToTextMode" drivers/VesaBIOSExtension.c
Length of output: 17121
🏁 Script executed:
#!/bin/bash
# Check how process stacks are allocated and freed
echo "=== Looking for process stack allocation ==="
rg -B 5 -A 10 "proc->stack.*=|processes\[.*\]\.stack.*=" --type c
echo -e "\n=== Check PAGE_SIZE definition ==="
rg "#define\s+PAGE_SIZE" --type h --type c
echo -e "\n=== Look for virtual to physical address conversion ==="
rg "VIRT_TO_PHYS|VirtToPhys|virtual.*physical" --type c --type h | head -20
echo -e "\n=== Check how KernelMemoryAlloc/Free work with addresses ==="
rg -A 10 "KernelMemoryFree" --type c | head -30
echo -e "\n=== Check text mode buffer access in kernel ==="
rg "0xB8000|text_buffer" --type c | head -20
Length of output: 3448
@assembler-0, I found both issues!
1. Memory Corruption - FreePage Error
The massive page index (4503565267632165 ≈ 0x10011E03AB00025) indicates a virtual address is being passed to FreePage which expects a physical address.
The problem is in process stack handling:
AllocPage()returns a physical address- But when setting up the process context, you use this as RSP (which needs to be virtual)
- Later,
FreePage(proc->stack)tries to free this address
The fix: Track both physical and virtual addresses for the stack:
// In CreateSecureProcess:
void* stack_phys = AllocPage(); // Physical address
if (!stack_phys) return -1;
// Convert to virtual for process use
uint64_t stack_virt = (uint64_t)stack_phys + KERNEL_VIRTUAL_OFFSET;
uint64_t rsp = stack_virt + STACK_SIZE;
// Store physical address for freeing
processes[slot].stack = stack_phys; // Keep physical for FreePage
processes[slot].context.rsp = rsp; // Use virtual for process2. Text Mode Switch (0xB8000)
The issue is that 0xB8000 needs to be accessed as part of the identity-mapped region in virtual addressing mode:
// In VBESwitchToTextMode, change:
uint16_t* text_buffer = (uint16_t*)0xB8000;
// To (if in higher-half kernel):
uint16_t* text_buffer = (uint16_t*)(0xB8000 + KERNEL_VIRTUAL_OFFSET);
// OR if it's identity-mapped and accessible:
uint16_t* text_buffer = (uint16_t*)0xB8000; // Should work if properly mappedActually, since 0xB8000 is in the first 1GB which you identity-map, it should work. The problem might be:
- VGA registers aren't being set correctly
- The framebuffer is still active and overriding text mode
Try adding this after setting VGA registers:
// Disable graphics mode completely
outb(0x3C0, 0x10); outb(0x3C0, 0x0C); // Graphics mode off
outb(0x3CE, 0x05); outb(0x3CF, 0x00); // Graphics mode register
outb(0x3CE, 0x06); outb(0x3CF, 0x0E); // Memory map modeThe root cause is mixing physical and virtual addresses. Would you like me to create an issue to track these memory management fixes?
🧠 Learnings used
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.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#53
File: kernel/core/Kernel.c:256-259
Timestamp: 2025-08-09T04:47:56.871Z
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.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#57
File: kernel/memory/VMem.h:64-66
Timestamp: 2025-08-09T09:29:00.591Z
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.
There was a problem hiding this comment.
what? i tought the allocation process is like this
void* stack = VMemAllocWithGuards(STACK_SIZE);
if (UNLIKELY(!stack)) {
FreeSlotFast(slot);
SpinUnlockIrqRestore(&scheduler_lock, flags);
PANIC("CreateSecureProcess: Failed to allocate stack");
}
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the actual CreateSecureProcess implementation
echo "=== Complete CreateSecureProcess function ==="
rg -B 10 -A 50 "void\*\s+stack\s*=\s*VMemAllocWithGuards" --type c
echo -e "\n=== Find VMemAllocWithGuards implementation ==="
ast-grep --pattern 'VMemAllocWithGuards($_) {
$$$
}'
echo -e "\n=== Search for VMemAllocWithGuards ==="
rg -B 5 -A 20 "VMemAllocWithGuards" --type c --type h
echo -e "\n=== Find where process stack is freed ==="
rg -B 5 -A 10 "FreePage.*stack|free.*process.*stack" --type c
echo -e "\n=== Check TerminateProcess implementation ==="
rg -B 5 -A 25 "^[^/]*TerminateProcess\s*\(" --type c
Length of output: 18355
🏁 Script executed:
#!/bin/bash
# Find VMemFreeWithGuards and VMemFree implementations
echo "=== Search for VMemFreeWithGuards ==="
rg -B 5 -A 20 "VMemFreeWithGuards" --type c --type h
echo -e "\n=== Search for VMemFree ==="
rg -B 5 -A 15 "^[^/]*VMemFree\s*\(" --type c
echo -e "\n=== Check what FreePage expects ==="
rg -B 5 -A 10 "^[^/]*FreePage\s*\(" --type c | head -40
Length of output: 11734
@assembler-0, you're absolutely right! I apologize for the confusion. I found the critical bug:
The Memory Bug
The issue is a mismatch between allocation and deallocation:
- Stack allocation: Uses
VMemAllocWithGuards()which returns a virtual address - Stack deallocation: Uses
FreePage()which expects a physical address
In TerminateProcess:
if (proc->stack) {
FreePage(proc->stack); // ❌ WRONG! proc->stack is virtual, not physical!
proc->stack = NULL;
}This explains the huge page index (4503565267632165) - it's interpreting a virtual address as physical!
The Fix
Replace FreePage(proc->stack) with:
if (proc->stack) {
VMemFreeWithGuards(proc->stack, STACK_SIZE); // ✅ Correct!
proc->stack = NULL;
}VMemFreeWithGuards will:
- Check the guard pages for corruption
- Properly unmap the virtual memory
- Free the underlying physical pages
Text Mode Switch Issue
For the 0xB8000 not working, try accessing it directly without offset since it should be identity-mapped:
uint16_t* text_buffer = (uint16_t*)0xB8000;If that still doesn't work, the VGA registers might not be switching modes properly. The framebuffer could still be active and overriding text mode.
Would you like me to create an issue to track this critical memory management bug where process stacks are being freed incorrectly?
✏️ Learnings added
Learnt from: assembler-0
PR: assembler-0/VoidFrame#58
File: drivers/VesaBIOSExtension.c:77-85
Timestamp: 2025-08-09T10:31:44.413Z
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).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#57
File: kernel/memory/VMem.h:64-66
Timestamp: 2025-08-09T09:29:00.591Z
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.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#53
File: kernel/core/Kernel.c:256-259
Timestamp: 2025-08-09T04:47:56.871Z
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.
| // Common colors (32-bit RGBA format) | ||
| #define VBE_COLOR_BLACK 0x00000000 | ||
| #define VBE_COLOR_WHITE 0x00FFFFFF | ||
| #define VBE_COLOR_RED 0x00FF0000 | ||
| #define VBE_COLOR_GREEN 0x0000FF00 | ||
| #define VBE_COLOR_BLUE 0x000000FF | ||
| #define VBE_COLOR_YELLOW 0x00FFFF00 | ||
| #define VBE_COLOR_CYAN 0x0000FFFF | ||
| #define VBE_COLOR_MAGENTA 0x00FF00FF | ||
| #define VBE_COLOR_GRAY 0x00808080 | ||
| #define VBE_COLOR_DARK_GRAY 0x00404040 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Color constants assume 32-bit X8R8G8B8; document and/or derive from framebuffer format
These hard-coded 0x00RRGGBB values only display correctly if the framebuffer is 32bpp with the expected channel order. The VBE driver currently ignores the format fields from Multiboot2.
- Document the assumption in a comment.
- Or extend vbe_info_t with channel offsets/masks and provide a VBE_MAKE_RGB() helper derived from the Multiboot2 color info.
🤖 Prompt for AI Agents
In drivers/VesaBIOSExtension.h around lines 6 to 17, the color constants are
hard-coded assuming a 32-bit X8R8G8B8 framebuffer format, which may not match
the actual framebuffer format from Multiboot2. To fix this, add a comment
clearly documenting this assumption. Alternatively, extend the vbe_info_t
structure to include channel offsets and masks from the Multiboot2 framebuffer
info, and implement a VBE_MAKE_RGB() helper macro or function that constructs
color values dynamically based on these parameters instead of using fixed
constants.
drivers/VesaBIOSExtension.h
Outdated
| uint32_t *framebuffer; // Pointer to framebuffer | ||
| uint32_t width; // Screen width | ||
| uint32_t height; // Screen height | ||
| uint32_t pitch; // Bytes per scanline | ||
| uint32_t bpp; // Bits per pixel | ||
| } vbe_info_t; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make framebuffer pointer volatile; consider exposing get-info as const
Access is MMIO-like; mark the pointer volatile to prevent the compiler from eliding writes. Returning a const pointer from VBEGetInfo prevents accidental mutation of geometry.
Apply this diff:
- uint32_t *framebuffer; // Pointer to framebuffer
+ volatile uint32_t *framebuffer; // Pointer to framebufferOptionally:
-vbe_info_t* VBEGetInfo(void);
+const vbe_info_t* VBEGetInfo(void);🤖 Prompt for AI Agents
In drivers/VesaBIOSExtension.h around lines 20 to 25, the framebuffer pointer
should be marked volatile because it points to memory-mapped I/O and the
compiler must not optimize away reads or writes. Change the framebuffer
declaration to a volatile uint32_t pointer. Additionally, modify the VBEGetInfo
function to return a pointer to a const vbe_info_t structure to prevent
accidental modification of the VBE geometry information.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
drivers/Serial.h (1)
35-37: Return-type inconsistency for number-printing helpers
SerialWriteHex/SerialWriteDecstill returnvoidwhile every other public I/O routine now returnsintfor status. This was raised earlier; please align the signatures and propagate error codes.kernel/core/Kernel.c (1)
370-381: Serial fallback logic implemented as recommended – LGTMKernel now retries COM2-4 when COM1 fails and logs the selected outcome. Good adoption of earlier feedback.
drivers/VesaBIOSExtension.c (1)
32-71: Tag-parsing still lacks bounds & bpp validationThe loop walks past the multiboot structure until it hits type 0 with no upper bound; a malformed tag size will over-read memory.
Also, you still accept anyframebuffer_bppbut later computepitch/4, assuming 32 bpp, and cast to non-volatileuint32_t*.
See prior review diff for a hardened version.drivers/Serial.c (1)
140-153: Still no error propagation from Hex/Dec writers
SerialWriteHex/SerialWriteDecignore failures fromSerialWriteand expose no status to callers, unlike the rest of the API. Please convert them tointand bubble up errors for uniform handling.
🧹 Nitpick comments (3)
drivers/Serial.h (1)
6-16: Drop one set of duplicate COM port macrosBoth
COMxandSERIAL_COMxdefine the same port addresses, creating two authoritative sources that can diverge. Keep a single naming scheme (preferablySERIAL_COMx) and delete the other to avoid accidental mismatch.kernel/core/Kernel.c (1)
357-360: Busy-wait delay wastes CPU cycles
for (volatile int i = 0; i < 100000000; i++);spins for hundreds of ms and still varies per clock speed. Prefer a calibrated PIT/APIC delay or at least insertpausein the loop to throttle power draw.drivers/Serial.c (1)
176-203: SerialReadLine spins without back-offWhen no character is ready the loop busy-waits indefinitely, burning CPU in early boot. Consider a small
pauseinstruction or timeout/yield hook to make this polling friendlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
arch/x86_64/asm/pxs.asm(1 hunks)drivers/Serial.c(1 hunks)drivers/Serial.h(1 hunks)drivers/VesaBIOSExtension.c(1 hunks)drivers/VesaBIOSExtension.h(1 hunks)include/stdlib.h(1 hunks)kernel/core/Kernel.c(4 hunks)kernel/process/Process.c(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- arch/x86_64/asm/pxs.asm
- include/stdlib.h
- drivers/VesaBIOSExtension.h
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-09T10:31:44.413Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#58
File: drivers/VesaBIOSExtension.c:77-85
Timestamp: 2025-08-09T10:31:44.413Z
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:
kernel/process/Process.ckernel/core/Kernel.cdrivers/VesaBIOSExtension.c
📚 Learning: 2025-08-09T09:29:00.591Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#57
File: kernel/memory/VMem.h:64-66
Timestamp: 2025-08-09T09:29:00.591Z
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/process/Process.cdrivers/VesaBIOSExtension.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/process/Process.ckernel/core/Kernel.cdrivers/VesaBIOSExtension.c
📚 Learning: 2025-08-09T04:47:56.871Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#53
File: kernel/core/Kernel.c:256-259
Timestamp: 2025-08-09T04:47:56.871Z
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/Kernel.cdrivers/VesaBIOSExtension.c
🔇 Additional comments (2)
kernel/process/Process.c (2)
982-985: Nice: safe return path addedPushing
ProcessExitStubensures a clean process exit instead of a GPF – good fix. The 8-byte push preserves the expected “RSP % 16 == 8” alignment inside the entry routine, so no ABI issue.
1045-1045: Correct allocator pairingSwitching from
FreePagetoVMemFreeWithGuardsmatches the stack allocator and removes the “page index out of bounds” fault.
| void VBEPutPixel(uint32_t x, uint32_t y, uint32_t color) { | ||
| if (!vbe_initialized || x >= vbe_info.width || y >= vbe_info.height) { | ||
| return; | ||
| } | ||
|
|
||
| uint32_t offset = y * (vbe_info.pitch / 4) + x; | ||
| vbe_info.framebuffer[offset] = color; | ||
| } | ||
|
|
||
| uint32_t VBEGetPixel(uint32_t x, uint32_t y) { | ||
| if (!vbe_initialized || x >= vbe_info.width || y >= vbe_info.height) { | ||
| return 0; | ||
| } | ||
|
|
||
| uint32_t offset = y * (vbe_info.pitch / 4) + x; | ||
| return vbe_info.framebuffer[offset]; | ||
| } |
There was a problem hiding this comment.
Undefined ABSi and 32-bpp assumption break build & correctness
ABSi is not defined/included and will not compile.
Offset calculation uses pitch / 4 and 32-bit writes—unsafe for 24 bpp modes. Guard on vbe_info.bpp == 32 (or add full format handling) before touching the framebuffer.
🤖 Prompt for AI Agents
In drivers/VesaBIOSExtension.c around lines 77 to 93, the code uses an undefined
identifier `ABSi` and assumes 32 bits per pixel by dividing pitch by 4, which
breaks compilation and correctness for other bpp modes. Remove or define `ABSi`
properly if needed, and add a guard to ensure the pixel operations only proceed
if vbe_info.bpp equals 32, or implement handling for other bpp formats before
accessing the framebuffer.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Style