Development #52
Development #52
Conversation
WalkthroughThe changes introduce a significant expansion to the x86_64 boot paging setup, mapping 4GB instead of 16MB, and update kernel memory management constants accordingly. The editor receives major enhancements, including a dirty flag, goto line, improved navigation, and robust file handling. String utility functions are centralized, and console cursor synchronization is added. Various formatting and user feedback improvements are made across the kernel and shell. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant FileSystem
User->>Editor: Open file
Editor->>FileSystem: Read file contents
FileSystem-->>Editor: File data
loop Editing
User->>Editor: Edit text / Move cursor / Goto line
Editor->>Editor: Update dirty flag, cursor, scroll
end
User->>Editor: Save file (Ctrl+S)
Editor->>FileSystem: Write file
FileSystem-->>Editor: Write status
User->>Editor: Quit (Ctrl+Q)
alt Unsaved changes
Editor->>User: Prompt for confirmation
User->>Editor: Confirm quit
end
Editor-->>User: Exit editor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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: 6
🔭 Outside diff range comments (3)
kernel/etc/Console.c (1)
198-216: Cursor desync and missing locking in PrintKernelAtThis function restores console.line/column but does not update the hardware cursor, leaving it pointing at the temporary print position. It also operates without the console spinlock, risking interleaving with other console writes.
Apply this patch to lock during the positioned print and resync the cursor when restoring:
void PrintKernelAt(const char* str, uint32_t line, uint32_t col) { if (!str || line >= VGA_HEIGHT || col >= VGA_WIDTH) return; + SpinLock(&lock); const uint32_t saved_line = console.line; const uint32_t saved_col = console.column; console.line = line; console.column = col; // Print until end of line or string for (const char* p = str; *p && console.column < VGA_WIDTH; p++) { if (*p == '\n') break; ConsolePutchar(*p); } // Restore cursor position console.line = saved_line; console.column = saved_col; + UpdateCursor(); + SpinUnlock(&lock); }kernel/etc/Shell.c (1)
111-152: Simplify ‘cd’ path resolution and unify behaviorYou manually rebuild paths for absolute/relative cases. You already have ResolvePath(); use it here to reduce duplication and off-by-one risks. Also keeps behavior consistent with other commands.
- char* dir = GetArg(cmd, 1); + char* dir = GetArg(cmd, 1); if (!dir) { FastMemcpy(current_dir, "/", 2); PrintKernel("[VFRFS] DIRECTORY SWITCHED TO /\n"); } else { - char new_path[256]; - if (dir[0] == '/') { - // Absolute path - int len = 0; - while (dir[len] && len < 255) len++; - FastMemcpy(new_path, dir, len); - new_path[len] = '\0'; - } else { - // Relative path - int curr_len = 0; - while (current_dir[curr_len]) curr_len++; - - FastMemcpy(new_path, current_dir, curr_len); - if (current_dir[curr_len - 1] != '/') { - new_path[curr_len++] = '/'; - } - - int dir_len = 0; - while (dir[dir_len] && curr_len + dir_len < 255) { - new_path[curr_len + dir_len] = dir[dir_len]; - dir_len++; - } - new_path[curr_len + dir_len] = '\0'; - } + char new_path[256]; + ResolvePath(dir, new_path, 256); FsNode* target = FsFind(new_path); if (!target) { PrintKernel("cd: directory not found\n"); } else if (target->type != FS_DIRECTORY) { PrintKernel("cd: not a directory\n"); } else { FastMemcpy(current_dir, new_path, 256); PrintKernel("[VFRFS] DIRECTORY SWITCHED TO "); PrintKernel(current_dir); PrintKernel("\n"); } }kernel/memory/VMem.c (1)
129-136: Page-table access will fault for frames ≥ 1 GiB
pt_phys < IDENTITY_MAP_SIZEgates identity-mapped access. With the constant now 1 GiB, any PT allocated above that limit is dereferenced throughPHYS_TO_VIRT, but no mapping exists for such frames (bootstrap only maps kernel + stack).
This makesVMemMap,VMemFree,VMemGetPhysAddrunsafe once physical memory >1 GiB is in use.Suggested fix (one of):
-#define IDENTITY_MAP_SIZE (1024 * 1024 * 1024) // 1GB +#define IDENTITY_MAP_SIZE (4ULL * 1024 * 1024 * 1024) // 4 GB – matches bootloader tablesor keep 1 GiB and force low-memory allocation for paging structures:
/* inside VMemGetPageTablePhys before AllocPage() */ uint64_t frame = AllocPageBelow(IDENTITY_MAP_SIZE); /* new helper */until this is addressed the kernel will crash on systems with >1 GiB RAM.
Also applies to: 243-249, 282-288
🧹 Nitpick comments (6)
kernel/etc/Editor.h (1)
4-4: Parameter name nit: prefer “path” over “file”“file” often implies a handle/stream, while this is a path string. Consider renaming to “path” for clarity and consistency with callers (e.g., Shell uses full_path).
kernel/etc/Console.c (2)
46-50: Minor: stray extra semicolon after SpinUnlockHarmless but noisy; clean it up.
- SpinUnlock(&lock);; + SpinUnlock(&lock);
105-107: Consider batching hardware cursor updates for performanceCalling UpdateCursor after every character incurs 4 outb ops per char. Consider updating once per higher-level print (e.g., after PrintKernel loops) and after ClearScreen/PrintKernelAt to reduce I/O.
Option A (conservative): keep here, but add a fast path to skip UpdateCursor if line/column unchanged.
Option B (aggressive): remove this call and add UpdateCursor:
- at the end of PrintKernel (after the loop, before unlocking)
- in ClearScreen (already present)
- at the end of PrintKernelAt (see next comment)
This keeps correctness while significantly cutting I/O.kernel/etc/StringOps.c (1)
10-16: Consider overlap safety & return value inFastStrCopy.
FastStrCopyuses a forward copy withdst[i] = src[i].
Ifdstandsrccan alias, this gives undefined behaviour; usememmove/FastMemmoveor document non-overlap pre-condition.
Additionally, most copy helpers return the number of bytes copied – that would let callers detect truncation and avoidstrlenre-scans.kernel/etc/Editor.c (1)
368-376: Quit-confirmation message suggests saving but doesn’t handle it.After warning about unsaved changes the code only checks for a second Ctrl-Q; pressing Ctrl-S as instructed just falls through to the next loop without saving.
Consider capturing Ctrl-S in this prompt or invokingEditorSave()automatically.kernel/core/Kernel.c (1)
296-303: Stack guard page silently removedPrevious version mapped a guard (unmapped) page below the kernel stack to catch overflows; this refactor omits it. Consider restoring the guard for basic stack-overflow detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
arch/x86_64/asm/pxs.asm(3 hunks)kernel/core/Kernel.c(2 hunks)kernel/etc/Console.c(3 hunks)kernel/etc/Editor.c(1 hunks)kernel/etc/Editor.h(1 hunks)kernel/etc/Shell.c(4 hunks)kernel/etc/StringOps.c(1 hunks)kernel/etc/StringOps.h(1 hunks)kernel/memory/MemOps.c(0 hunks)kernel/memory/MemOps.h(0 hunks)kernel/memory/Memory.c(1 hunks)kernel/memory/VMem.c(3 hunks)kernel/memory/VMem.h(1 hunks)meson.build(1 hunks)
💤 Files with no reviewable changes (2)
- kernel/memory/MemOps.h
- kernel/memory/MemOps.c
🧰 Additional context used
🧠 Learnings (1)
📚 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/memory/Memory.ckernel/memory/VMem.hkernel/memory/VMem.ckernel/core/Kernel.carch/x86_64/asm/pxs.asm
🔇 Additional comments (8)
meson.build (2)
79-79: LGTM: New StringOps.c added to buildAddition is correct and consistent with the new StringOps module; include paths already cover kernel/etc.
65-92: All good: StringOps.c present & no duplicate FastStrCmp found
StringOps.c exists at kernel/etc/StringOps.c and FastStrCmp is only defined there. No duplicates in MemOps or elsewhere—no further action needed.kernel/etc/Console.c (2)
2-2: Required include for outb is presentIncluding Io.h for port I/O is correct.
8-14: VGA cursor update logic looks correctCRTC registers 0x0F/0x0E with ports 0x3D4/0x3D5 are used properly; position computation matches VGA text mode layout.
kernel/etc/Shell.c (1)
9-9: LGTM: include StringOps.h for new string helpersConsistent with moving FastStrCmp out of MemOps.
kernel/memory/Memory.c (1)
170-171: Good micro-formatting tweakThe added newline makes log output consistent.
kernel/core/Kernel.c (1)
276-285: Bootstrap maps only 1 GiB – mismatches paging-structure assumptionsThe boot ASM pre-maps 0-4 GiB. Shrinking the C-side identity-map loop to 1 GiB is safe but, combined with the 1 GiB
IDENTITY_MAP_SIZE, means later page-table frames above 1 GiB are not reachable via identity mapping, yet VMem code assumes they are.Either increase this loop back to 4 GiB, or adopt the low-memory-only allocation strategy proposed in VMem.h/VMem.c.
arch/x86_64/asm/pxs.asm (1)
80-88: Assembly maps 4 GiB while C side expects 1 GiBBootloader correctly covers 0-4 GiB, but higher-level code now believes only 0-1 GiB is identity-mapped, causing inconsistent assumptions (see VMem comments). Align both layers to the same size to avoid hard-to-debug faults.
Also applies to: 90-116
| static int EditorSave(void) { | ||
| int fd = FsOpen(filename, FS_WRITE); | ||
| if (fd < 0) { | ||
| PrintKernel("\nError: Cannot save file\n"); | ||
| return; | ||
| PrintKernel("\nError: Cannot save file - "); | ||
| PrintKernel(filename); | ||
| PrintKernel("\n"); | ||
| return 0; | ||
| } | ||
|
|
||
| for (int i = 0; i < total_lines; i++) { | ||
| int len = 0; | ||
| while (editor_buffer[i][len]) len++; | ||
|
|
||
| for (int i = 0; i < total_lines && i < MAX_LINES; i++) { | ||
| int len = get_line_length(i); | ||
| if (len > 0) { | ||
| FsWrite(fd, editor_buffer[i], len); | ||
| int written = FsWrite(fd, editor_buffer[i], len); | ||
| if (written != len) { | ||
| FsClose(fd); | ||
| PrintKernel("\nError: Write failed\n"); | ||
| return 0; | ||
| } | ||
| } | ||
| if (i < total_lines - 1) { | ||
| FsWrite(fd, "\n", 1); | ||
| if (FsWrite(fd, "\n", 1) != 1) { | ||
| FsClose(fd); | ||
| PrintKernel("\nError: Write failed\n"); | ||
| return 0; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| FsClose(fd); | ||
| PrintKernel("\nFile saved!\n"); | ||
| dirty = 0; | ||
| PrintKernel("\nFile saved successfully!\n"); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Saved file may keep stale tail – open with truncate flag.
EditorSave opens the file with FS_WRITE only.
If the new buffer is shorter than the previous contents, the filesystem will leave the old tail bytes intact, corrupting the result.
Open with a FS_TRUNCATE/FS_CREATE flag or call an explicit FsTruncate(fd, 0) before writing.
🤖 Prompt for AI Agents
In kernel/etc/Editor.c around lines 103 to 135, the EditorSave function opens
the file with FS_WRITE only, which can leave stale data if the new content is
shorter than the old. To fix this, open the file with an additional FS_TRUNCATE
or FS_CREATE flag to ensure the file is truncated on open, or alternatively call
FsTruncate(fd, 0) immediately after opening the file and before writing any
data.
| char buffer[2048]; // Larger read buffer | ||
| int bytes_read = FsRead(fd, buffer, sizeof(buffer) - 1); | ||
| FsClose(fd); | ||
|
|
||
| if (bytes_read <= 0) { | ||
| // Empty file | ||
| FastMemset(editor_buffer, 0, sizeof(editor_buffer)); | ||
| total_lines = 1; | ||
| current_line = 0; | ||
| current_col = 0; | ||
| dirty = 0; | ||
| return; | ||
| } | ||
| buffer[bytes_read] = 0; | ||
|
|
||
| buffer[bytes_read] = '\0'; | ||
| FastMemset(editor_buffer, 0, sizeof(editor_buffer)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
2 KiB read buffer silently chops bigger files.
EditorLoad allocates char buffer[2048]; any file > 2047 B is truncated with no warning.
Either loop with repeated FsRead or pre-check file size and bail out with an error.
🤖 Prompt for AI Agents
In kernel/etc/Editor.c around lines 149 to 165, the fixed 2 KiB buffer for
reading files causes silent truncation of files larger than 2047 bytes. To fix
this, modify the code to either loop with repeated FsRead calls to read the
entire file in chunks or check the file size before reading and return an error
if the file is too large. This prevents silent data loss by ensuring the full
file content is loaded or an explicit error is raised.
| if (current_col > 0) { | ||
| current_col--; | ||
| // Shift characters left | ||
| for (int i = current_col; i < MAX_LINE_LEN - 1; i++) { | ||
| editor_buffer[current_line][i] = editor_buffer[current_line][i + 1]; | ||
| } | ||
| dirty = 1; | ||
| } else if (current_line > 0) { | ||
| // Join with previous line | ||
| int prev_len = get_line_length(current_line - 1); | ||
|
|
||
| // Check if we can fit the current line | ||
| int curr_len = get_line_length(current_line); | ||
| if (prev_len + curr_len < MAX_LINE_LEN - 1) { | ||
| // Copy current line to end of previous line | ||
| int remaining_space = MAX_LINE_LEN - 1 - prev_len; | ||
| if (remaining_space > 0 && curr_len <= remaining_space) { | ||
| FastMemcpy(&editor_buffer[current_line - 1][prev_len], | ||
| editor_buffer[current_line], curr_len); | ||
| editor_buffer[current_line - 1][prev_len + curr_len] = '\0'; | ||
| } | ||
| // Move lines up | ||
| for (int i = current_line; i < total_lines - 1 && i < MAX_LINES - 1; i++) { | ||
| FastMemcpy(editor_buffer[i], editor_buffer[i + 1], MAX_LINE_LEN); | ||
| } | ||
|
|
||
| // Clear last line | ||
| if (total_lines > 0 && total_lines <= MAX_LINES) { | ||
| FastMemset(editor_buffer[total_lines - 1], 0, MAX_LINE_LEN); | ||
| } | ||
|
|
||
| current_line--; | ||
| current_col = prev_len; | ||
| total_lines--; | ||
| if (total_lines <= 0) total_lines = 1; | ||
| dirty = 1; | ||
| } | ||
| } | ||
| clamp_cursor(); | ||
| } |
There was a problem hiding this comment.
Possible data loss from overlapping FastMemcpy when lifting lines.
EditorDeleteChar shifts the whole buffer up with FastMemcpy(editor_buffer[i], editor_buffer[i+1], …).
If FastMemcpy is a plain memcpy, the source and destination regions overlap — behaviour is undefined.
Use an overlap-safe routine (memmove/FastMemmove) or reverse the copy direction.
🤖 Prompt for AI Agents
In kernel/etc/Editor.c around lines 260 to 299, the code uses FastMemcpy to
shift lines up in the editor buffer, but this causes overlapping memory regions
which leads to undefined behavior if FastMemcpy is a plain memcpy. To fix this,
replace FastMemcpy with an overlap-safe function like memmove or FastMemmove
when copying overlapping regions, or change the loop to copy lines in reverse
order to avoid overlap.
| PrintKernelSuccess("VoidFrame Shell v0.0.1-beta\n"); | ||
| ClearScreen(); | ||
| PrintKernelSuccess("VoidFrame Shell. Press ENTER to start shell\n"); | ||
| while (1) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clear after banner erases the banner
You print the banner, then ClearScreen, which immediately wipes it. Swap the order so both messages remain visible.
void ShellProcess(void) {
- PrintKernelSuccess("VoidFrame Shell v0.0.1-beta\n");
- ClearScreen();
- PrintKernelSuccess("VoidFrame Shell. Press ENTER to start shell\n");
+ ClearScreen();
+ PrintKernelSuccess("VoidFrame Shell v0.0.1-beta\n");
+ PrintKernelSuccess("VoidFrame Shell. Press ENTER to start shell\n");📝 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.
| PrintKernelSuccess("VoidFrame Shell v0.0.1-beta\n"); | |
| ClearScreen(); | |
| PrintKernelSuccess("VoidFrame Shell. Press ENTER to start shell\n"); | |
| while (1) { | |
| void ShellProcess(void) { | |
| ClearScreen(); | |
| PrintKernelSuccess("VoidFrame Shell v0.0.1-beta\n"); | |
| PrintKernelSuccess("VoidFrame Shell. Press ENTER to start shell\n"); | |
| while (1) { | |
| // … |
🤖 Prompt for AI Agents
In kernel/etc/Shell.c around lines 265 to 268, the ClearScreen call is placed
after printing the banner messages, which causes the banner to be erased
immediately. To fix this, move the ClearScreen call before printing the banner
messages so that the screen is cleared first and then the banner remains
visible.
| #include "stdint.h" | ||
|
|
||
| int FastStrCmp(const char* str1, const char* str2); | ||
| size_t FastStrlen(const char* s, size_t max); | ||
| void FastStrCopy(char* dst, const char* src, size_t max_len); |
There was a problem hiding this comment.
Missing include for size_t
This header uses size_t but doesn’t include a header that guarantees its definition. Add stddef.h (or your project’s size_t definition header) to avoid build breaks in freestanding mode.
-#include "stdint.h"
+#include "stdint.h"
+#include "stddef.h"📝 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.
| #include "stdint.h" | |
| int FastStrCmp(const char* str1, const char* str2); | |
| size_t FastStrlen(const char* s, size_t max); | |
| void FastStrCopy(char* dst, const char* src, size_t max_len); | |
| #include "stdint.h" | |
| #include "stddef.h" | |
| int FastStrCmp(const char* str1, const char* str2); | |
| size_t FastStrlen(const char* s, size_t max); | |
| void FastStrCopy(char* dst, const char* src, size_t max_len); |
🤖 Prompt for AI Agents
In kernel/etc/StringOps.h around lines 3 to 7, the header uses size_t but does
not include the necessary header to define it. Add #include <stddef.h> at the
top of the file to ensure size_t is properly defined and prevent build errors in
freestanding mode.
| #define IDENTITY_MAP_SIZE (1024 * 1024 * 1024) // 1GB | ||
|
|
There was a problem hiding this comment.
Identity-map size now conflicts with runtime page-table allocation
Reducing IDENTITY_MAP_SIZE to 1 GiB while AllocPage() can hand out frames anywhere below 4 GiB means newly-created paging structures above 1 GiB are accessed via PHYS_TO_VIRT() although no mapping exists, leading to a page fault the first time a >1 GiB frame is used for a page table.
Either
- raise the constant to cover the whole 4 GiB region the boot assembly already identity-maps, or
- constrain page-table allocations (
AllocPage/VMemGetPageTablePhys) to return frames< IDENTITY_MAP_SIZE.
Failing to address this will make the kernel non-bootable on machines that place free RAM above 1 GiB.
🤖 Prompt for AI Agents
In kernel/memory/VMem.h around lines 15 to 16, the IDENTITY_MAP_SIZE is set to 1
GiB, which conflicts with AllocPage() potentially allocating frames above 1 GiB
but below 4 GiB, causing page faults due to missing mappings. To fix this,
either increase IDENTITY_MAP_SIZE to cover the full 4 GiB region already
identity-mapped by the boot assembly, or modify AllocPage() and
VMemGetPageTablePhys to only allocate frames below the current
IDENTITY_MAP_SIZE. Choose one approach to ensure all page tables are properly
mapped and prevent kernel boot failures.
There was a problem hiding this comment.
also just asking i move the enable paging and jump to C like this, can you have a look?, asm is too fire
inline void EnablePaginAndJump(uint64_t cr3, uint64_t new_rip, uint64_t new_rsp) {
asm volatile (
"mov %0, %%cr3\n\t"
"mov %2, %%rsp\n\t"
"jmp *%1"
:
: "r" (cr3), "r" (new_rip), "r" (new_rsp)
: "memory"
);
}
There was a problem hiding this comment.
@assembler-0, this approach has several critical issues:
-
Paging is already enabled: In x86_64 long mode, paging must be enabled before entering 64-bit mode. Your boot assembly (
pxs.asm) already handles this before jumping toKernelMain. You can't "enable paging" from C code - it's a prerequisite for running 64-bit code. -
Stack corruption: Changing
rspwithout saving the current stack frame means this function can never return. All local variables, return addresses, and function parameters are lost. -
Page fault risk: Reloading CR3 while executing can cause immediate page faults if the new page tables don't map the current code location.
-
Missing clobbers: The inline assembly should declare
"rsp"in the clobber list, though GCC doesn't actually support this.
If you're trying to switch page tables (not enable paging), here's a safer approach:
static inline void SwitchPageTable(uint64_t new_cr3) {
asm volatile (
"mov %0, %%cr3"
:
: "r" (new_cr3)
: "memory"
);
}If you need to jump to kernel entry from bootloader with a new stack, that must be done in assembly before entering C code. The pattern is:
- Set up page tables (assembly)
- Enable paging (assembly)
- Set stack pointer (assembly)
- Jump to C entry point (assembly)
Your current pxs.asm already does this correctly. Once in C code, you should only switch CR3 for context switches or address space changes, not for initial setup.
What specific problem are you trying to solve by moving this to C? That would help me suggest a better approach.
🧠 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.
There was a problem hiding this comment.
migrate the asmhelpers.asm? to c
There was a problem hiding this comment.
like this
section .text
global EnablePagingAndJump
EnablePagingAndJump:
mov cr3, rdi
mov rsp, rdx
jmp rsi
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the asmhelpers.asm file
fd -t f "asmhelpers.asm"
# Let's also check for any .asm files that might contain helper functions
fd -e asm | head -20
Length of output: 291
🏁 Script executed:
#!/bin/bash
# Examine the AsmHelpers.asm file
cat kernel/memory/AsmHelpers.asm
Length of output: 143
@assembler-0, I found the AsmHelpers.asm file. Here's how to migrate it to C, but with important caveats:
Current Assembly Code
EnablePagingAndJump:
mov cr3, rdi
mov rsp, rdx
jmp rsiMigration Options
Option 1: Direct Migration (⚠️ Unsafe for general use)
// kernel/memory/AsmHelpers.h
#ifndef ASM_HELPERS_H
#define ASM_HELPERS_H
#include <stdint.h>
// This function never returns - mark with noreturn
__attribute__((noreturn))
static inline void EnablePagingAndJump(uint64_t new_cr3, uint64_t jump_target, uint64_t new_stack) {
asm volatile(
"mov %0, %%cr3\n\t" // Load new page tables
"mov %2, %%rsp\n\t" // Switch stack (destroys current frame!)
"jmp *%1\n\t" // Jump to target
:
: "r"(new_cr3), "r"(jump_target), "r"(new_stack)
: "memory"
);
__builtin_unreachable();
}
#endifOption 2: Safer Decomposed Functions (Recommended)
// kernel/memory/AsmHelpers.h
#ifndef ASM_HELPERS_H
#define ASM_HELPERS_H
#include <stdint.h>
// Switch page tables (safe to call from C)
static inline void SwitchPageTable(uint64_t new_cr3) {
asm volatile(
"mov %0, %%cr3"
:
: "r"(new_cr3)
: "memory"
);
}
// Read current CR3 value
static inline uint64_t GetCR3(void) {
uint64_t cr3;
asm volatile(
"mov %%cr3, %0"
: "=r"(cr3)
);
return cr3;
}
// Flush entire TLB
static inline void FlushTLB(void) {
uint64_t cr3 = GetCR3();
SwitchPageTable(cr3);
}
// Flush single TLB entry
static inline void InvalidatePage(void* addr) {
asm volatile(
"invlpg (%0)"
:
: "r"(addr)
: "memory"
);
}
// For context switching - must be called from assembly wrapper
__attribute__((noreturn))
static inline void JumpToTask(uint64_t rip, uint64_t rsp) {
asm volatile(
"mov %1, %%rsp\n\t"
"jmp *%0"
:
: "r"(rip), "r"(rsp)
);
__builtin_unreachable();
}
#endifImportant Notes
-
The original function is misleading: It doesn't "enable" paging - paging is already enabled in long mode. It switches page tables and jumps.
-
Stack switching from C is dangerous: When you change RSP, you lose the current stack frame. This should only be done:
- During initial boot (in assembly)
- During context switches (with proper assembly wrapper)
- When jumping to user mode (with iretq)
-
Where to use each approach:
- Use
SwitchPageTable()for normal page table switches - Keep context switching in assembly (
Switch.asm) - Initial boot setup should stay in
pxs.asm
- Use
-
Delete the old file after migration:
rm kernel/memory/AsmHelpers.asm- Update meson.build to remove the assembly file and update any callers to use the new inline functions.
The decomposed approach (Option 2) is much safer and more maintainable. Would you like me to also check where EnablePagingAndJump is currently being called so we can update those call sites?
🧬 Code Graph Analysis Results
Relevant snippets from kernel/memory/VMem.c
File: kernel/memory/VMem.c
Lines: 34-49
Description:
VMemInit initializes the virtual memory manager by retrieving the current PML4 physical address from the CR3 register using inline assembly. It clears the lower 12 bits (flags) from the CR3 value to get the base address of the PML4 table. It then initializes the kernel virtual address space tracking structure with the starting virtual address, used pages, total mapped bytes, and the PML4 pointer. This function uses inline assembly to access the CR3 register, which is a low-level CPU register operation typically done in assembly.
File: kernel/memory/VMem.c
Lines: 357-363
Description:
VMemFlushTLB flushes the entire Translation Lookaside Buffer (TLB) by reloading the CR3 register. This is done using inline assembly that moves the current CR3 value into RAX and then back into CR3, causing the CPU to flush the TLB. This is a common assembly idiom for TLB flush.
File: kernel/memory/VMem.c
Lines: 365-368
Description:
VMemFlushTLBSingle flushes a single TLB entry corresponding to a given virtual address. It uses the invlpg instruction via inline assembly to invalidate the page in the TLB for the specified address. This is a low-level CPU instruction that must be done in assembly.
These three snippets contain inline assembly instructions related to CPU register manipulation and TLB flushing, which are typically implemented in assembly (asmhelpers.asm). The user comment about migrating asmhelpers.asm to C likely relates to these or similar functions that currently use inline assembly.
[End of relevant snippets]
🧠 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.
There was a problem hiding this comment.
@assembler-0, while you can technically migrate this to C using inline assembly, this particular function is better kept in pure assembly. Here's why and how to do it properly if you must:
Why keep it in assembly:
- Stack frame destruction: Changing
rspin C code invalidates the entire call stack - no return is possible - Compiler assumptions: C compilers expect consistent stack frames for generated code
- No real benefit: The inline assembly version is just assembly wrapped in C syntax
If you must use C, here's the correct approach:
// Mark as noreturn since it never comes back
__attribute__((noreturn))
static inline void EnablePagingAndJump(uint64_t cr3, void (*target)(void), uint64_t new_rsp) {
asm volatile (
"mov %0, %%cr3\n\t"
"mov %2, %%rsp\n\t"
"xor %%rbp, %%rbp\n\t" // Clear frame pointer
"jmp *%1"
:
: "r" (cr3), "r" (target), "r" (new_rsp)
: "memory"
);
__builtin_unreachable(); // Tell compiler this never returns
}Better alternative - keep assembly helpers:
Keep asmhelpers.asm with critical functions like this one, and call them from C:
// In a header file
extern void EnablePagingAndJump(uint64_t cr3, void (*target)(void), uint64_t new_rsp) __attribute__((noreturn));Then link the assembly object file. This maintains clear separation between low-level operations and C code, making the codebase more maintainable.
When to migrate vs. keep in assembly:
- Keep in ASM: Stack switches, CPU mode changes, boot code, interrupt entry/exit
- Migrate to C: Logic-heavy code, data structure manipulation, algorithms
- Inline ASM: Simple privileged instructions (like just reloading CR3 without stack changes)
Your original assembly function is clean and correct - I'd recommend keeping it as-is rather than forcing it into C.
🧠 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.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores