Just some additional Kernel designs. - #6
Conversation
WalkthroughThe PR replaces the PMM with a buddy allocator and changes its public API to page-based functions, adds a cooperative tasking subsystem and syscall entry (interrupt 128), adjusts kernel init to spawn tasks and reduce verbose logs, updates NIC RX buffer allocation to a block-based pmm allocation, and modifies interrupt handling and paging metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PIT as Timer IRQ
participant IDT as ISR Dispatcher
participant Scheduler as switch_task
participant RunQ as ready_queue
participant CPU as CPU/Context
PIT->>IDT: IRQ0 (timer)
IDT->>Scheduler: switch_task(registers_t* r)
rect rgb(235,245,255)
note right of Scheduler: Save current regs\nDequeue next task\nLoad next regs
Scheduler->>RunQ: Enqueue/Dequeue operations
end
Scheduler-->>IDT: return (next task regs)
IDT-->>CPU: iret to next task
sequenceDiagram
autonumber
participant User as User code
participant IDT as ISR (int 128)
participant Syscall as syscall_handler
User->>IDT: int 128 (syscall)
IDT->>Syscall: syscall_handler(registers_t* r)
alt eax == 0
Syscall-->>User: prints "SYSTEM CALL RECEIVED!, SIMPLE OS WORKING"
else
Syscall-->>User: no-op / unhandled
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
NIC.C (3)
68-76: API mismatch: pmm_alloc_blocks is undeclared/undefinedThis will not compile with the current pmm.h/c. Either switch to the page API or add pmm_alloc_blocks to PMM. See pmm.h/pmm.c comments and proposed implementation.
40-60: Constrain NIC DMA buffers to <4 MB or expand identity mapping
Either restrict PMM to return frames below 4 MB for rx_buffer allocations in NIC.C, or identity-map additional RAM in paging_install (paging.c §34–44).
68-76: Fix RTL8139 receive buffer allocation and RCR configuration
- Allocate 5 blocks (20 KB) instead of 4 to include the required 16-byte slack:
rx_buffer = (uint8_t*)pmm_alloc_blocks(5);- Set RCR RBLEN bits [12:11]=01 for 16 KB + 16 bytes:
outl(rtl8139_io_base + REG_RCR, 0x0F | (1u << 11));
🧹 Nitpick comments (1)
pmm.c (1)
73-107: Add basic input validation in free; optionalConsider asserting page alignment and non-NULL inputs in pmm_free_page to catch misuse early during development.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
NIC.C(2 hunks)kernel.c(0 hunks)pmm.c(1 hunks)pmm.h(1 hunks)task.c(1 hunks)task.h(1 hunks)
💤 Files with no reviewable changes (1)
- kernel.c
🧰 Additional context used
🧬 Code graph analysis (3)
pmm.h (1)
pmm.c (3)
pmm_init(17-47)pmm_alloc_page(49-71)pmm_free_page(73-107)
task.h (1)
idt.h (1)
struct registers {(19-24)
pmm.c (1)
paging.c (1)
paging_install(9-62)
🔇 Additional comments (1)
task.h (1)
15-19: Typedefregisters_texists; no action required. The typedef is declared in idt.h (typedef struct registers { … } registers_t;), so the code compiles as is.Likely an incorrect or invalid review comment.
| #define PAGE_SIZE 4096 | ||
| #define MAX_ORDER 10 // Max block size: 4KB * 2^10 = 4MB | ||
| #define KERNEL_RESERVED_END 0x200000 // Reserve the first 2MB for kernel/modules/metadata |
There was a problem hiding this comment.
Do not hardcode kernel reservation without validating actual end-of-kernel
KERNEL_RESERVED_END is fixed at 2MB. If the kernel grows beyond this, you’ll hand out memory still occupied by kernel/modules. Consider taking the actual end address from the linker symbol or bootloader and seeding from that.
🤖 Prompt for AI Agents
In pmm.c around lines 4 to 6, KERNEL_RESERVED_END is hardcoded to 0x200000 which
can overlap with a larger kernel; replace the constant with a runtime-determined
end-of-kernel value obtained from the linker or bootloader (e.g. an extern
symbol like _end or kernel_end provided by the linker, or a bootloader-provided
memory map field), validate and PAGE_SIZE-align that address, ensure it is
capped/validated against available physical memory and the allocator base, and
use that aligned runtime value when seeding/reserving pages (add a fallback and
an explicit error/log if the symbol is missing or the computed reservation is
out of range).
| void pmm_init(uint32_t memory_end) { | ||
| uint32_t total_pages = memory_end / PAGE_SIZE; | ||
|
|
||
| // Validate memory_top alignment | ||
| if (memory_top & 15) { | ||
| memory_top = (memory_top + 15) & ~15; | ||
| // Initialize all free lists to be empty | ||
| for (int i = 0; i <= MAX_ORDER; i++) { | ||
| free_lists[i] = NULL; | ||
| } | ||
|
|
||
| // Find free allocation slot | ||
| for (int i = 0; i < MAX_ALLOCATIONS; i++) { | ||
| if (!allocations[i].used) { | ||
| allocations[i].address = memory_top; | ||
| allocations[i].size = size; | ||
| allocations[i].used = 1; | ||
| allocation_count++; | ||
|
|
||
| void* ptr = (void*)memory_top; | ||
| memory_top += size; | ||
|
|
||
| // Clear allocated memory for security | ||
| uint8_t* clear_ptr = (uint8_t*)ptr; | ||
| for (size_t j = 0; j < size; j++) { | ||
| clear_ptr[j] = 0; | ||
| // Correctly initialize by creating maximal aligned blocks | ||
| uint32_t start_page = KERNEL_RESERVED_END / PAGE_SIZE; | ||
| uint32_t current_page = start_page; | ||
|
|
||
| while (current_page < total_pages) { | ||
| // Find the largest possible block that fits and is aligned | ||
| int order = MAX_ORDER; | ||
| while (order >= 0) { | ||
| uint32_t block_size = 1 << order; | ||
| if ((current_page % block_size == 0) && (current_page + block_size <= total_pages)) { | ||
| break; | ||
| } | ||
|
|
||
| return ptr; | ||
| order--; | ||
| } | ||
| } | ||
|
|
||
| return NULL; | ||
| } | ||
| // Add this maximal block to the correct free list | ||
| buddy_node_t* block = (buddy_node_t*)addr_from_page_index(current_page); | ||
| block->next = free_lists[order]; | ||
| free_lists[order] = block; | ||
|
|
||
| void pmm_free(void* ptr) { | ||
| if (ptr == NULL) { | ||
| return; | ||
| current_page += (1 << order); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potentially returning unmapped frames while only 4MB is identity-mapped
pmm_init seeds all memory up to memory_end, but paging.c only identity-maps the first 4MB. Any allocation >4MB used as a pointer will fault (TCBs, stacks, rx buffer).
Options:
- Restrict PMM to [KERNEL_RESERVED_END, 4MB) until paging maps more RAM.
- Or expand identity mapping to cover memory_end.
I can propose a paging_install update to identity-map N MB based on a detected limit if you prefer that route.
🤖 Prompt for AI Agents
In pmm.c around lines 17-47, pmm_init currently seeds free lists up to
memory_end which can include physical frames beyond the 4MB identity-mapped
region and thus produce pointers into unmapped memory; fix by constraining PMM
seeding to the identity-mapped range or by expanding identity mapping: either
(A) compute a safe_limit = min(memory_end, IDENTITY_MAPPED_BYTES) (e.g.,
4*1024*1024) and use safe_limit/PAGE_SIZE as total_pages so only mapped frames
are inserted, or (B) modify paging_install to identity-map up to memory_end (or
a detected usable RAM limit) before pmm_init runs so all seeded frames are
accessible; implement one of these two options and ensure any callers that
expect physical->virtual pointer use only frames within the mapped range.
| void* pmm_alloc_page(void) { | ||
| for (int order = 0; order <= MAX_ORDER; order++) { | ||
| if (free_lists[order] != NULL) { | ||
| // Found a block, take it from the list | ||
| buddy_node_t* block = free_lists[order]; | ||
| free_lists[order] = block->next; | ||
|
|
||
| // Split the block if it's larger than a single page (order 0) | ||
| int current_order = order; | ||
| while (current_order > 0) { | ||
| current_order--; | ||
| // The buddy is the second half of the split block | ||
| buddy_node_t* buddy = (buddy_node_t*)((uint32_t)block + (PAGE_SIZE * (1 << current_order))); | ||
|
|
||
| // Add the new buddy to the lower-order free list | ||
| buddy->next = free_lists[current_order]; | ||
| free_lists[current_order] = buddy; | ||
| } | ||
| return block; | ||
| } | ||
| } | ||
| return NULL; // Out of memory | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Provide a contiguous multi-page allocation API to match NIC usage
Implement pmm_alloc_blocks(count) (contiguous pages) by generalizing the split logic to a requested order. This also enables 8K/16K/32K allocations cleanly.
Apply this diff to add pmm_alloc_blocks after pmm_alloc_page:
return NULL; // Out of memory
}
+// Allocate 'count' contiguous pages (count >= 1). Returns base or NULL.
+void* pmm_alloc_blocks(uint32_t count) {
+ if (count == 0) return NULL;
+ // Find smallest order with capacity >= count
+ int req_order = 0;
+ uint32_t sz = 1;
+ while (sz < count && req_order < MAX_ORDER) { sz <<= 1; req_order++; }
+ if (req_order > MAX_ORDER) return NULL;
+
+ // Find a block at order >= req_order
+ int order = req_order;
+ while (order <= MAX_ORDER && free_lists[order] == NULL) order++;
+ if (order > MAX_ORDER) return NULL;
+
+ // Take a block from 'order' and split down to req_order
+ buddy_node_t* block = free_lists[order];
+ free_lists[order] = block->next;
+ while (order > req_order) {
+ order--;
+ buddy_node_t* buddy = (buddy_node_t*)((uint32_t)block + (PAGE_SIZE * (1u << order)));
+ buddy->next = free_lists[order];
+ free_lists[order] = buddy;
+ }
+ return block;
+}If you add pmm_free_blocks later, pass the same count to allow proper coalescing, or store order metadata per allocation.
📝 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.
| void* pmm_alloc_page(void) { | |
| for (int order = 0; order <= MAX_ORDER; order++) { | |
| if (free_lists[order] != NULL) { | |
| // Found a block, take it from the list | |
| buddy_node_t* block = free_lists[order]; | |
| free_lists[order] = block->next; | |
| // Split the block if it's larger than a single page (order 0) | |
| int current_order = order; | |
| while (current_order > 0) { | |
| current_order--; | |
| // The buddy is the second half of the split block | |
| buddy_node_t* buddy = (buddy_node_t*)((uint32_t)block + (PAGE_SIZE * (1 << current_order))); | |
| // Add the new buddy to the lower-order free list | |
| buddy->next = free_lists[current_order]; | |
| free_lists[current_order] = buddy; | |
| } | |
| return block; | |
| } | |
| } | |
| return NULL; // Out of memory | |
| } | |
| void* pmm_alloc_page(void) { | |
| for (int order = 0; order <= MAX_ORDER; order++) { | |
| if (free_lists[order] != NULL) { | |
| // Found a block, take it from the list | |
| buddy_node_t* block = free_lists[order]; | |
| free_lists[order] = block->next; | |
| // Split the block if it's larger than a single page (order 0) | |
| int current_order = order; | |
| while (current_order > 0) { | |
| current_order--; | |
| // The buddy is the second half of the split block | |
| buddy_node_t* buddy = (buddy_node_t*)((uint32_t)block + (PAGE_SIZE * (1 << current_order))); | |
| // Add the new buddy to the lower-order free list | |
| buddy->next = free_lists[current_order]; | |
| free_lists[current_order] = buddy; | |
| } | |
| return block; | |
| } | |
| } | |
| return NULL; // Out of memory | |
| } | |
| // Allocate 'count' contiguous pages (count >= 1). Returns base or NULL. | |
| void* pmm_alloc_blocks(uint32_t count) { | |
| if (count == 0) return NULL; | |
| // Find smallest order with capacity >= count | |
| int req_order = 0; | |
| uint32_t sz = 1; | |
| while (sz < count && req_order < MAX_ORDER) { | |
| sz <<= 1; | |
| req_order++; | |
| } | |
| if (req_order > MAX_ORDER) return NULL; | |
| // Find a block at order >= req_order | |
| int order = req_order; | |
| while (order <= MAX_ORDER && free_lists[order] == NULL) { | |
| order++; | |
| } | |
| if (order > MAX_ORDER) return NULL; | |
| // Take a block from 'order' and split down to req_order | |
| buddy_node_t* block = free_lists[order]; | |
| free_lists[order] = block->next; | |
| while (order > req_order) { | |
| order--; | |
| buddy_node_t* buddy = (buddy_node_t*)((uint32_t)block + (PAGE_SIZE * (1u << order))); | |
| buddy->next = free_lists[order]; | |
| free_lists[order] = buddy; | |
| } | |
| return block; | |
| } |
| void pmm_init(uint32_t memory_end); | ||
| void* pmm_alloc_page(void); | ||
| void pmm_free_page(void* p); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Expose missing PMM APIs and PAGE_SIZE used by other modules
- NIC.C calls pmm_alloc_blocks(4) but pmm.h does not declare it; compilation will fail.
- task.c uses PAGE_SIZE but it’s not defined in any included header; compilation will fail.
Add the missing declarations and make PAGE_SIZE available to consumers.
Apply this diff:
#include <stdint.h>
#include <stddef.h>
-void pmm_init(uint32_t memory_end);
-void* pmm_alloc_page(void);
-void pmm_free_page(void* p);
+#define PAGE_SIZE 4096
+
+void pmm_init(uint32_t memory_end);
+void* pmm_alloc_page(void);
+void pmm_free_page(void* p);
+// Allocate contiguous physical pages (count >= 1). Returns page-aligned base or NULL.
+void* pmm_alloc_blocks(uint32_t count);
+void pmm_free_blocks(void* p, uint32_t count);📝 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.
| void pmm_init(uint32_t memory_end); | |
| void* pmm_alloc_page(void); | |
| void pmm_free_page(void* p); | |
| #include <stdint.h> | |
| #include <stddef.h> | |
| #define PAGE_SIZE 4096 | |
| void pmm_init(uint32_t memory_end); | |
| void* pmm_alloc_page(void); | |
| void pmm_free_page(void* p); | |
| // Allocate contiguous physical pages (count >= 1). Returns page-aligned base or NULL. | |
| void* pmm_alloc_blocks(uint32_t count); | |
| void pmm_free_blocks(void* p, uint32_t count); |
🤖 Prompt for AI Agents
In pmm.h around lines 7 to 9, add missing public API declarations and the
PAGE_SIZE macro: declare a function prototype for pmm_alloc_blocks that takes a
uint32_t count and returns void* (so NIC.C can call pmm_alloc_blocks(4)),
declare a prototype for pmm_free_blocks that takes a void* pointer and a
uint32_t count, and ensure PAGE_SIZE is defined (e.g., via a macro or constant)
so task.c can include it; keep parameter types consistent with existing
pmm_alloc_page/pmm_free_page prototypes and include any needed standard integer
headers if not already present.
| void tasking_install(void) { | ||
| vga_print_string("Initializing multitasking... "); | ||
| current_task = (task_t*)pmm_alloc_page(); | ||
| memset(current_task, 0, sizeof(task_t)); | ||
| current_task->id = next_pid++; | ||
| current_task->state = TASK_RUNNING; | ||
| current_task->kernel_stack = NULL; | ||
| current_task->next = NULL; | ||
| ready_queue = current_task; | ||
| irq_install_handler(0, switch_task); | ||
|
|
||
| vga_print_string("[OK]\n"); | ||
| } |
There was a problem hiding this comment.
Initialize and verify tasking install allocations
Add NULL checks and consider allocating the bootstrap task TCB/stack from known low memory to avoid mapping issues until paging maps more than 4MB.
🤖 Prompt for AI Agents
In task.c around lines 9 to 21, the bootstrap task TCB/stack allocation and
setup lack NULL checks and may be placed above low memory causing page mapping
issues; check the result of pmm_alloc_page() and any other allocations, print a
clear error and halt if allocation fails, allocate the bootstrap task control
block and its kernel stack from a known low physical memory region (below 4MB)
or use a dedicated low-memory allocator, initialize kernel_stack to the
allocated stack top, ensure memset only after a successful allocation, set
ready_queue and install the IRQ handler only after all allocations succeed, and
avoid leaving partially initialized task state on failure.
| current_task = (task_t*)pmm_alloc_page(); | ||
| memset(current_task, 0, sizeof(task_t)); | ||
| current_task->id = next_pid++; | ||
| current_task->state = TASK_RUNNING; | ||
| current_task->kernel_stack = NULL; | ||
| current_task->next = NULL; | ||
| ready_queue = current_task; | ||
| irq_install_handler(0, switch_task); |
There was a problem hiding this comment.
Do not allocate scheduler state on potentially unmapped memory; check alloc results
pmm_alloc_page can return frames above the first 4MB, but paging.c only identity maps 4MB. current_task may live in unmapped memory and cause page faults. Also, no NULL checks.
- Ensure allocations come from mapped/low memory or expand identity mapping before using the memory as pointers.
- Add NULL checks and fail-safe.
🤖 Prompt for AI Agents
In task.c around lines 11-18, the code allocates current_task with
pmm_alloc_page and immediately uses it but pmm_alloc_page may return frames
above the 4MB identity-mapped region and may return NULL; update the logic to
(1) call a low-memory allocation API or ensure the returned physical frame is
identity-mapped into the kernel virtual space before dereferencing, (2) check
the allocation result for NULL and handle failure (log and abort/return error or
retry), and (3) if choosing to map the frame, create an identity mapping for
that page (or expand the identity range) before calling memset or accessing
fields, then set up the task fields and queue as before.
| new_task->kernel_stack = pmm_alloc_page() + PAGE_SIZE; | ||
|
|
||
| // Set up the initial register state | ||
| new_task->regs.eip = (uint32_t)entry_point; | ||
| new_task->regs.esp = (uint32_t)new_task->kernel_stack; | ||
| new_task->regs.eflags = 0x202; // Enable interrupts |
There was a problem hiding this comment.
Fix void arithmetic; initialize a runnable kernel context correctly*
- void* + PAGE_SIZE is non‑standard; cast to uint8_t* first.
- Only setting eip/esp/eflags is insufficient; cs/ss remain zeroed by memset and esp won’t be switched by iret/popad (popa ignores ESP). Tasks will share the same kernel stack and corrupt each other.
Apply this diff for the cast and minimal guards (stack switching still needs the ISR stub fix below):
- new_task->kernel_stack = pmm_alloc_page() + PAGE_SIZE;
+ void* ks_page = pmm_alloc_page();
+ if (!ks_page) { return; }
+ new_task->kernel_stack = (uint8_t*)ks_page + PAGE_SIZE;
+
+ // Ensure code/data segments are sane (kernel segments). Adjust to your GDT.
+ new_task->regs.cs = 0x08;
+ new_task->regs.ss = 0x10;Follow-up required: adjust the ISR/interrupt stub to actually load ESP from the next task’s kernel_stack before returning from interrupt; otherwise tasks still share the current stack. This cannot be fixed purely in C via registers_t because popa ignores ESP and iret won’t pop ESP in ring0. You need a small asm trampoline to save current ESP into current_task and load next_task->kernel_stack into ESP before restoring regs and iret.
I can draft the ISR stub changes if you share the common interrupt/IRQ entry assembly.
🤖 Prompt for AI Agents
In task.c around lines 30 to 35, the code does pointer arithmetic on void* and
only initializes eip/esp/eflags leaving cs/ss zero (and risks sharing kernel
stacks); cast the allocated page to a byte pointer (e.g. uint8_t*) before adding
PAGE_SIZE to compute the top of the stack, set new_task->kernel_stack to that
computed pointer, and initialize the saved register context's segment selectors
(regs.cs and regs.ss) to the kernel code/data selectors (the ring0 selectors
your kernel uses) so the task has correct segment state; also ensure each task
gets its own kernel stack by allocating per-task pages (do not reuse a global
stack) and add a minimal guard or assertion that kernel_stack is
unique/non-null; separately, follow up by updating the ISR/interrupt assembly
stub to save current ESP into current_task and load next_task->kernel_stack into
ESP before restoring registers and iret (popa does not restore ESP), otherwise
tasks will still share the same stack.
| if (!current_task->next) { | ||
| return; | ||
| } | ||
|
|
||
| // Move to the next task in the round-robin queue | ||
| task_t* prev_task = current_task; | ||
| current_task = current_task->next; | ||
|
|
||
| // If we've reached the end of the list, loop back to the start | ||
| if (!current_task) { | ||
| current_task = ready_queue; | ||
| } | ||
|
|
There was a problem hiding this comment.
Round-robin bug: last task never advances; remove early return
If current_task is at the tail (next == NULL), switch_task returns and never wraps to the head; scheduling stalls on the last task.
Apply this diff:
- // If there's no other task, just return
- if (!current_task->next) {
- return;
- }
-
- // Move to the next task in the round-robin queue
- task_t* prev_task = current_task;
- current_task = current_task->next;
-
- // If we've reached the end of the list, loop back to the start
- if (!current_task) {
- current_task = ready_queue;
- }
+ // Move to next task or wrap to the head
+ task_t* prev_task = current_task;
+ current_task = current_task->next ? current_task->next : ready_queue;Optional: set prev_task->state = TASK_READY.
📝 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.
| if (!current_task->next) { | |
| return; | |
| } | |
| // Move to the next task in the round-robin queue | |
| task_t* prev_task = current_task; | |
| current_task = current_task->next; | |
| // If we've reached the end of the list, loop back to the start | |
| if (!current_task) { | |
| current_task = ready_queue; | |
| } | |
| // Move to next task or wrap to the head | |
| task_t* prev_task = current_task; | |
| current_task = current_task->next ? current_task->next : ready_queue; |
🤖 Prompt for AI Agents
In task.c around lines 51 to 63 the code returns early when current_task->next
is NULL which prevents wrapping to the head and stalls scheduling; remove the
early "return", instead record prev_task = current_task, advance current_task =
current_task->next and if current_task is NULL set current_task = ready_queue so
the round-robin wraps; optionally set prev_task->state = TASK_READY before
advancing.
Add files via upload
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kernel.c (1)
36-46: Remove duplicate shell initialization.The shell is initialized twice:
- Line 36: Creates a "shell" task with entry point
shell_init- Line 46: Calls
shell_init()directly in kernel contextThis dual initialization means the shell will run concurrently in two contexts:
- As a scheduled task (cooperative multitasking)
- In the kernel main thread
This creates a race condition for keyboard input and VGA output, as both instances will compete for the same resources without synchronization.
Remove the direct call if task-based execution is intended:
tasking_install(); create_task("shell",shell_init); create_task("counter",counter_task); uint32_t free_mem = pmm_get_free_memory(); vga_print_dec(free_mem / 1024); vga_print_string(" KB\n"); vga_print_string("\nKernel initialization complete!\n"); vga_print_string("Security features: Buffer overflow protection, bounds checking\n"); vga_print_string("Starting shell...\n\n"); - shell_init(); // Main kernel loop - wait for interrupts while(1) { asm volatile("hlt"); }Alternatively, if the direct call is intentional (for initial setup before task switching), add a comment explaining this design decision.
🧹 Nitpick comments (3)
syscall.h (1)
1-8: Consider declaringsyscall_handlerin this header.The
syscall_handlerfunction defined insyscall.cis likely called from the interrupt handling path (viaisr128). Without a declaration in a header, the linker must resolve it by symbol name alone, which can lead to type-safety issues and makes the API less clear.Add the handler declaration after line 6:
void syscalls_install(void); +void syscall_handler(registers_t *r);pmm.h (1)
5-11: Clarify the purpose and usage of page_frame_t.
page_frame_tis defined in the public header but appears unused by the current buddy allocator implementation. The allocator usesbuddy_node_tinternally (pmm.c line 8) and maintainspage_frame_databaseas an unused static variable (pmm.c line 16).Either document the intended future use of this structure or remove it until needed.
pmm.c (1)
16-17: Remove unused page_frame_database and total_pages.The static variables
page_frame_databaseandtotal_pages(line 17) are declared but never used. Line 19 shadows the globaltotal_pageswith a local variable.Apply this diff:
static inline uint32_t get_buddy_index(uint32_t index, uint32_t order) { return index ^ (1 << order); } -static page_frame_t* page_frame_database; -static uint32_t total_pages; + void pmm_init(uint32_t memory_end) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
idt.c(2 hunks)interrupt.asm(2 hunks)kernel.c(1 hunks)paging.c(1 hunks)paging.h(1 hunks)pmm.c(1 hunks)pmm.h(1 hunks)syscall.c(1 hunks)syscall.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- paging.c
🧰 Additional context used
🧬 Code graph analysis (3)
syscall.c (1)
vga.c (1)
vga_print_string(70-74)
paging.h (1)
paging.c (1)
paging_install(9-62)
kernel.c (3)
vga.c (2)
vga_putentryat(35-38)vga_print_string(70-74)task.c (2)
tasking_install(9-21)create_task(22-41)shell.c (1)
shell_init(140-146)
🔇 Additional comments (16)
idt.c (2)
3-3: LGTM!The include is appropriate for referencing the syscall installation function.
212-212: LGTM! Correct user-mode syscall gate setup.The IDT gate 128 is correctly configured with flags
0xEE(DPL=3, present, 32-bit interrupt gate), enabling user-mode processes to invoke system calls viaint 0x80equivalent.interrupt.asm (2)
51-51: LGTM! Syscall vector symbol exported.The
isr128symbol is correctly exported for the syscall IDT entry.
152-173: STI removal from IRQ path is correct.The
stiinstruction was removed fromirq_common_stub(line 149 was inisr_common_stub, notirq_common_stub). This is the proper approach for IRQ handling:
- The
iretinstruction at line 173 automatically restores EFLAGS from the stack, including the interrupt flag. If interrupts were enabled before the IRQ, they will be re-enabled upon return.- This allows proper nested interrupt handling without forcing interrupts on.
- Note that
isr_common_stub(line 149) still containssti, which is appropriate for exception handlers that need different behavior.The change correctly differentiates IRQ handling from exception handling.
kernel.c (3)
12-13: LGTM! Correct includes for new subsystems.The includes for task and syscall headers are appropriate for the new functionality.
28-28: LGTM! Syscall installation added to kernel init.The call to
syscalls_install()is correctly positioned in the initialization sequence.However, verify that
syscalls_install()is implemented. The function is declared insyscall.hbut the implementation is not visible insyscall.c.
35-37: LGTM! Task subsystem initialization.The calls to
tasking_install()and task creation are correctly placed after the necessary subsystems (GDT, IDT, paging, PMM, interrupts) are initialized.paging.h (3)
12-22: LGTM! pt_entry_t structure changes align with x86 paging specification.The structure modifications correctly reflect standard x86 page table entry fields:
user_supervisor(bit 2) for U/S flagpwt(bit 3) for write-through caching policypcd(bit 4) for cache disablereservedbits properly sizedThe corresponding implementation in paging.c (lines 16-28) correctly initializes these fields.
26-27: New reserved region helpers support buddy allocator integration.The addition of
paging_get_reserved_start/endprovides a clean interface for the PMM to query paging metadata boundaries, which is essential for proper memory region exclusion during initialization.
7-8:PAGING_STRUCTURES_START macro correctly references extern array.
The macro
PAGING_STRUCTURES_STARTuses&page_directorywherepage_directoryis declaredexternin the header. Verification confirms that bothpage_directory[1024]andfpage_table[1024]are properly defined inpaging.c(lines 6-7) without theexternkeyword and with appropriate 4096-byte alignment attributes. This follows correct C conventions: extern declarations in the header with a single definition in the implementation file.pmm.c (5)
74-108: LGTM! Buddy coalescing logic is correct.The
pmm_free_pageimplementation properly:
- Calculates the buddy index using XOR
- Searches the appropriate free list for the buddy
- Removes the buddy if found and merges to the next order
- Stops coalescing when buddy is not free or MAX_ORDER is reached
- Inserts the final block into the correct free list
The pointer manipulation and list traversal are correct.
110-124: LGTM! Free memory calculation correctly traverses all orders.The function properly:
- Iterates through all orders from 0 to MAX_ORDER
- Calculates the correct block size for each order (PAGE_SIZE * 2^order)
- Traverses the linked list and accumulates total free memory
4-6: Hardcoded KERNEL_RESERVED_END may overlap with actual kernel size.
KERNEL_RESERVED_ENDis fixed at 2MB. If the kernel, modules, or paging structures exceed this size, the allocator will hand out pages still in use by the kernel, causing memory corruption.Replace the hardcoded constant with a runtime-determined value:
+// External linker symbol marking actual end of kernel +extern uint32_t _kernel_end; + #define PAGE_SIZE 4096 #define MAX_ORDER 10 // Max block size: 4KB * 2^10 = 4MB -#define KERNEL_RESERVED_END 0x200000 // Reserve the first 2MB for kernel/modules/metadataThen in
pmm_init:void pmm_init(uint32_t memory_end) { uint32_t total_pages = memory_end / PAGE_SIZE; + + // Get actual kernel end from linker, align up to page boundary + uint32_t kernel_end = ((uint32_t)&_kernel_end + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1); + + // Also reserve paging structures + uint32_t reserved_end = kernel_end; + // Add paging structures if needed: reserved_end += PAGING_STRUCTURES_SIZE; // Initialize all free lists to be empty for (int i = 0; i <= MAX_ORDER; i++) { free_lists[i] = NULL; } // Correctly initialize by creating maximal aligned blocks - uint32_t start_page = KERNEL_RESERVED_END / PAGE_SIZE; + uint32_t start_page = reserved_end / PAGE_SIZE; uint32_t current_page = start_page;Ensure your linker script exports
_kernel_endat the end of the kernel image.
18-48: Critical: PMM seeds pages beyond the 4MB identity-mapped region.
pmm_initadds all pages fromKERNEL_RESERVED_ENDtomemory_endinto free lists. However,paging.conly identity-maps the first 4MB (paging.c lines 28-39). Any allocation beyond 4MB will return an unmapped physical address, causing page faults when dereferenced (e.g., TCBs, task stacks, NIC rx_buffer).Solution A (simpler): Constrain PMM to the identity-mapped region:
void pmm_init(uint32_t memory_end) { - uint32_t total_pages = memory_end / PAGE_SIZE; + // Only manage memory within the identity-mapped region (first 4MB) + #define IDENTITY_MAPPED_SIZE (4 * 1024 * 1024) // 4MB + uint32_t safe_limit = (memory_end < IDENTITY_MAPPED_SIZE) ? memory_end : IDENTITY_MAPPED_SIZE; + uint32_t total_pages = safe_limit / PAGE_SIZE;Solution B (better long-term): Expand identity mapping in
paging_installto cover detected RAM, then callpmm_initwith that limit. This requires allocating/mapping additional page tables before the PMM is available.I recommend Solution A for now, and defer dynamic paging expansion to a follow-up task.
Would you like me to propose a paging_install update for Solution B?
50-72: Missing pmm_alloc_blocks API required by NIC.C.
pmm_alloc_pagecorrectly implements single-page allocation with buddy splitting. However,NIC.Ccallspmm_alloc_blocks(4)to allocate a contiguous 16KB rx_buffer, but this function is not implemented.Add the multi-page allocation function after
pmm_alloc_page:return NULL; // Out of memory } +// Allocate 'count' contiguous pages (count >= 1). Returns page-aligned base or NULL. +void* pmm_alloc_blocks(uint32_t count) { + if (count == 0) return NULL; + + // Find smallest order that can hold 'count' pages + int req_order = 0; + uint32_t block_pages = 1; + while (block_pages < count && req_order < MAX_ORDER) { + block_pages <<= 1; + req_order++; + } + if (req_order > MAX_ORDER) return NULL; // Request too large + + // Find a block at order >= req_order + int order = req_order; + while (order <= MAX_ORDER && free_lists[order] == NULL) { + order++; + } + if (order > MAX_ORDER) return NULL; // Out of memory + + // Take block from list and split down to req_order + buddy_node_t* block = free_lists[order]; + free_lists[order] = block->next; + + while (order > req_order) { + order--; + buddy_node_t* buddy = (buddy_node_t*)((uint32_t)block + (PAGE_SIZE * (1u << order))); + buddy->next = free_lists[order]; + free_lists[order] = buddy; + } + + return block; +} + +// Free 'count' contiguous pages starting at 'p' +void pmm_free_blocks(void* p, uint32_t count) { + if (p == NULL || count == 0) return; + + // For now, free each page individually + // TODO: Optimize to free as a single block of the appropriate order + for (uint32_t i = 0; i < count; i++) { + pmm_free_page((void*)((uint32_t)p + i * PAGE_SIZE)); + } +}Also declare these in
pmm.h(already covered in the pmm.h review comment).pmm.h (1)
1-11: Add include guards, standard includes, and missing public API declarations.This header is missing:
- Include guards to prevent multiple inclusion
#include <stdint.h>foruint8_tinpage_frame_t- Public function declarations:
pmm_init,pmm_alloc_page,pmm_free_page,pmm_alloc_blocks,pmm_free_blocks,pmm_get_free_memoryPAGE_SIZEconstant (used bytask.cand required by consumers)Without these declarations, compilation will fail when other modules call PMM functions.
Apply this diff:
+#ifndef PMM_H +#define PMM_H + #include <stdint.h> + +#define PAGE_SIZE 4096 #define PAGE_STATE_FREE 0x01 #define PAGE_STATE_USED 0x02 #define PAGE_STATE_RESERVED 0x04 #define PAGE_STATE_KERNEL 0x08 + typedef struct page_frame { uint8_t state; uint8_t order; struct page_frame* next; struct page_frame* prev; - } page_frame_t; + +// PMM initialization +void pmm_init(uint32_t memory_end); + +// Single page allocation/deallocation +void* pmm_alloc_page(void); +void pmm_free_page(void* p); + +// Multi-page allocation/deallocation (contiguous blocks) +void* pmm_alloc_blocks(uint32_t count); +void pmm_free_blocks(void* p, uint32_t count); + +// Memory statistics +uint32_t pmm_get_free_memory(void); + +#endif
| isr_no_error 30 | ||
| isr_no_error 31 | ||
|
|
||
| isr_no_error 128 |
There was a problem hiding this comment.
Critical: Syscall routing to wrong handler.
The isr_no_error 128 macro routes isr128 through isr_common_stub to isr_handler, which handles CPU exceptions (interrupt numbers < 32). System calls on vector 128 will not be dispatched to syscall_handler as intended.
You need to create a separate entry path for syscalls:
Add after line 127:
+syscall_common_stub:
+ pusha
+ mov ax, ds
+ push eax
+
+ mov ax, 0x10
+ mov ds, ax
+ mov es, ax
+ mov fs, ax
+ mov gs, ax
+
+ call syscall_handler
+
+ pop eax
+ mov ds, ax
+ mov es, ax
+ mov fs, ax
+ mov gs, ax
+
+ popa
+ add esp, 8
+ sti
+ iretThen change line 110 to use a custom macro:
-isr_no_error 128
+isr128:
+ cli
+ push byte 0
+ push byte 128
+ jmp syscall_common_stubCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In interrupt.asm around line 110 (and after line 127), the syscall vector 128 is
currently using isr_no_error which routes through isr_common_stub and ultimately
dispatches to isr_handler for CPU exceptions; instead add a dedicated syscall
entry path after line 127 that mirrors the no-error/with-error macros but
targets a syscall_common_stub which pushes the same minimal context and then
jumps to syscall_handler, and then change line 110 to use this new macro (e.g.,
replace isr_no_error 128 with isr_syscall 128) so vector 128 is dispatched to
syscall_handler rather than isr_handler.
| void counter_task(){ | ||
| int i=0; | ||
| while(1){ | ||
| vga_putentryat('A' + (i++%26),0x0F,79,0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add synchronization for VGA access.
The counter_task writes to VGA memory at a fixed position (79,0) without synchronization. This creates a race condition with other tasks and kernel code that may write to the VGA buffer concurrently, potentially causing visual corruption or tearing.
Consider adding a spinlock or disabling interrupts around VGA writes, or use a VGA subsystem with built-in synchronization.
🤖 Prompt for AI Agents
In kernel.c around lines 14 to 19, counter_task writes directly to VGA at (79,0)
without synchronization; wrap the VGA write in a synchronization primitive
(preferably a kernel spinlock with IRQ-save) to prevent races with other
writers: declare or use an existing vga_lock, call spin_lock_irqsave(&vga_lock,
flags) before vga_putentryat and spin_unlock_irqrestore(&vga_lock, flags) after,
or if a lock is not available at this layer, disable interrupts around the write
(cli()/sti()) as a last resort; ensure the lock/irq state is properly
saved/restored to avoid deadlocks and interrupt-loss.
Fix integer overflow and negative modulo in counter.
The counter_task function has an integer overflow issue: when i exceeds INT_MAX, it wraps to negative, causing (i++ % 26) to produce negative values, resulting in incorrect characters (potentially non-printable or control characters).
Apply this diff to fix the overflow:
void counter_task(){
- int i=0;
+ uint32_t i=0;
while(1){
- vga_putentryat('A' + (i++%26),0x0F,79,0);
+ vga_putentryat('A' + ((i++)%26),0x0F,79,0);
}
}📝 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.
| void counter_task(){ | |
| int i=0; | |
| while(1){ | |
| vga_putentryat('A' + (i++%26),0x0F,79,0); | |
| } | |
| } | |
| void counter_task(){ | |
| uint32_t i=0; | |
| while(1){ | |
| vga_putentryat('A' + ((i++)%26),0x0F,79,0); | |
| } | |
| } |
🤖 Prompt for AI Agents
In kernel.c around lines 14-19, counter_task uses a signed int i that can
overflow and become negative, making (i++ % 26) yield negative results; change i
to an unsigned type or keep it bounded by computing index = (i % 26) where i is
unsigned (or update i with i = (i + 1) % 26) before using it, and use that
non-negative index to compute the character passed to vga_putentryat so the
value never goes negative or overflows.
| void syscall_handler(registers_t *r){ | ||
| if (r->eax==0){ | ||
| vga_print_string("SYSTEM CALL RECEIVED!, SIMPLE OS WORKING\n"); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Critical: Missing syscalls_install() implementation and null pointer check.
This file declares syscall_handler but does not implement syscalls_install(), which is called in kernel.c (line 28). The missing implementation will cause a linker error.
Additionally, syscall_handler lacks a null pointer check on r, which could lead to undefined behavior if called with a NULL pointer.
Add the syscalls_install() implementation and null check:
#include "syscall.h"
#include "vga.h"
+#include "idt.h"
+
+extern void syscall_dispatcher(void);
+
+void syscalls_install(void) {
+ // Register the syscall handler with the interrupt system
+ // Note: IDT gate 128 already points to isr128 in idt.c
+ // isr128 will call isr_handler, which needs to route to syscall_handler
+ // This requires modifying isr_handler or creating a separate path
+}
+
void syscall_handler(registers_t *r){
+ if (!r) {
+ return;
+ }
if (r->eax==0){
vga_print_string("SYSTEM CALL RECEIVED!, SIMPLE OS WORKING\n");
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In syscall.c around lines 3 to 7, implement the missing syscalls_install()
function (it is referenced from kernel.c line 28 and causes a linker error) and
add a defensive null-pointer check in syscall_handler: check if the incoming
registers_t *r is NULL and return early if so, then proceed with existing
handling; implement syscalls_install() to register syscall_handler with the
interrupt/gate table (or wrapper used by this kernel) and perform any necessary
ISR/IRQ setup so the symbol is defined and the syscall handler is properly
wired.
🧩 Analysis chain
Verify syscall routing from isr128 to syscall_handler.
The current interrupt architecture routes isr128 through isr_common_stub to isr_handler (in idt.c), which handles exceptions, not system calls. You need to either:
- Modify
isr_handlerto check forint_no == 128and dispatch tosyscall_handler, or - Create a separate
syscall_common_stubininterrupt.asmthat callssyscall_handlerdirectly, and pointisr128to it instead.
The current implementation will not correctly route syscalls to this handler.
Run the following script to verify the interrupt routing:
🏁 Script executed:
#!/bin/bash
# Check how isr_handler routes interrupt 128
rg -nP -C5 'void\s+isr_handler\s*\(' --type=c
# Check if there's a syscall-specific stub
ast-grep --pattern $'syscall_common_stub'Length of output: 935
🏁 Script executed:
#!/bin/bash
# Get the full isr_handler implementation
rg -nP -A30 'void\s+isr_handler\s*\(' --type=c idt.c
# Check how isr128 is defined in assembly
rg -nP -C10 'isr128'
# Check if syscall_handler is called anywhere
rg -nP '\bsyscall_handler\s*\(' --type=cLength of output: 2954
🏁 Script executed:
#!/bin/bash
# Get the complete isr_handler function to see what happens for int_no >= 32
cat idt.c | sed -n '57,150p'
# Check the isr_common_stub implementation to confirm it calls isr_handler
rg -nP -A20 'isr_common_stub:' interrupt.asm
# Double-check if syscall_handler is referenced anywhere else
rg -n 'syscall_handler' --type=c --type=hLength of output: 3038
Verify syscall routing from isr128 to syscall_handler.
The current interrupt architecture routes isr128 through isr_common_stub to isr_handler (idt.c:57), which only handles exceptions (int_no < 32) and then returns for all other interrupts. When interrupt 128 fires, isr_handler will simply return without dispatching to syscall_handler.
Verified routing path:
isr128(interrupt.asm:51) → usesisr_no_errormacro → jumps toisr_common_stub(interrupt.asm:128)isr_common_stubcallsisr_handler(interrupt.asm:139)isr_handleronly processesif (r->int_no < 32)(idt.c:69), then returns
syscall_handler in syscall.c is orphaned—no code path invokes it.
Fix by either:
- Adding
if (r->int_no == 128) { syscall_handler(r); return; }inisr_handlerbefore line 69, or - Creating a separate
syscall_common_stubin interrupt.asm that callssyscall_handlerdirectly, and pointingisr128to it instead of using theisr_no_errormacro.
🤖 Prompt for AI Agents
In syscall.c around lines 3-7 the syscall_handler is never invoked because
isr128 is routed through isr_common_stub → isr_handler which only handles int_no
< 32; update the interrupt dispatch so syscalls reach syscall_handler by
modifying isr_handler (idt.c) to check for r->int_no == 128 before the existing
exception branch and call syscall_handler(r) then return, or alternatively
implement a dedicated syscall_common_stub in interrupt.asm and point isr128 to
it so it calls syscall_handler directly.
1 participant
These are the baby steps for the task scheduler
Summary by CodeRabbit