Development #2
Development #2
Conversation
WalkthroughSystem call support is introduced to the kernel. This includes new syscall handler logic, syscall initialization, an assembly entry point, and syscall number definitions. The kernel now initializes the syscall subsystem and demonstrates a write syscall from a kernel task. The build system is updated to include the new files. Changes
Sequence Diagram(s)sequenceDiagram
participant UserTask as User Task
participant CPU as CPU/Interrupt
participant SyscallEntry as SyscallEntry (ASM)
participant Kernel as Kernel (SyscallHandler)
participant Console as Console/Output
UserTask->>CPU: int 0x80 (syscall)
CPU->>SyscallEntry: Transfer control to SyscallEntry
SyscallEntry->>Kernel: Call SyscallHandler(syscall_num, arg1, arg2, arg3)
Kernel->>Kernel: Handle syscall (e.g., SYS_WRITE)
Kernel->>Console: Output string (if SYS_WRITE)
Kernel->>SyscallEntry: Return result
SyscallEntry->>CPU: Restore registers, iretq
CPU->>UserTask: Resume execution
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Docstrings generation was requested by @assembler-0. * #2 (comment) The following files were modified: * `Kernel/Kernel.c` * `Kernel/Syscall.c`
|
Note Generated docstrings for this pull request at #3 |
![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
🧹 Nitpick comments (2)
Kernel/Syscall.h (1)
1-16: LGTM: Well-structured syscall header with standard conventions.The syscall numbers follow Unix conventions and the function declarations are appropriate for a syscall subsystem. Consider adding brief documentation comments for the functions to improve maintainability.
Consider adding documentation:
// System call handler +// Initialize the system call subsystem void SyscallInit(void); +// Handle system calls with up to 3 arguments uint64_t SyscallHandler(uint64_t syscall_num, uint64_t arg1, uint64_t arg2, uint64_t arg3);Kernel/SyscallEntry.asm (1)
8-23: Consider using more efficient register save/restore.While saving all registers is safe, you could optimize by only saving caller-saved registers since this is an interrupt handler.
For better performance, consider saving only the registers that the C ABI doesn't guarantee to preserve:
- ; Save all registers - push rax - push rbx - push rcx - push rdx - push rsi - push rdi - push rbp - push r8 - push r9 - push r10 - push r11 - push r12 - push r13 - push r14 - push r15 + ; Save caller-saved registers (C ABI) + push rax + push rcx + push rdx + push rsi + push rdi + push r8 + push r9 + push r10 + push r11 + ; rbx, rbp, r12-r15 are callee-saved in C ABIUpdate the restore section accordingly. This reduces memory traffic and improves syscall performance.
Also applies to: 35-50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Kernel/Idt.h(1 hunks)Kernel/Kernel.c(2 hunks)Kernel/Kernel.h(1 hunks)Kernel/Syscall.c(1 hunks)Kernel/Syscall.h(1 hunks)Kernel/SyscallEntry.asm(1 hunks)meson.build(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Kernel/Syscall.c (4)
Kernel/Syscall.h (2)
SyscallHandler(14-14)SyscallInit(13-13)Kernel/Kernel.h (1)
PrintKernelAt(9-9)Kernel/Kernel.c (1)
PrintKernelAt(101-109)Kernel/Idt.h (1)
IdtSetGate(25-25)
🔇 Additional comments (7)
Kernel/Idt.h (1)
25-25: LGTM: IDT gate setup function declaration added.The function signature is appropriate for installing interrupt handlers in the IDT, with correct parameter types for x86-64 architecture.
Kernel/Kernel.h (1)
1-2: Include guard rename is safe: no references to the old guard and no conflicts found
- Searched for
VOIDFRAME_KERNEL_H: no matches in the codebase- Searched for
KERNEL_H: only appears in Kernel/Kernel.h’s own guardNo remaining concerns.
meson.build (2)
37-37: LGTM: Syscall assembly entry point added to build.The addition of
SyscallEntry.asmto the assembly sources is appropriate for the syscall infrastructure.
50-51: Verify inclusion of new Process.cThe ls output confirms that
Kernel/Process.c,Kernel/Syscall.c, andKernel/SyscallEntry.asmall exist and were added in this PR. Notably,Kernel/Process.chas no prior git history. Please confirm that introducingProcess.cis intentional, as the PR description only mentions syscall additions.– Kernel/Process.c
– Kernel/Syscall.c
– Kernel/SyscallEntry.asmKernel/Kernel.c (3)
9-9: LGTM: Syscall header included correctly.The include statement properly integrates the syscall subsystem into the kernel.
146-147: LGTM: Syscall initialization added correctly.The syscall initialization is properly integrated into the kernel startup sequence.
137-138: Syscall handler and IDT entry are correctly implementedAll required components are present and wired up:
SyscallHandleris defined in Kernel/Syscall.c (line 6) and declared in Kernel/Syscall.h.SyscallInit()(defined at Kernel/Syscall.c:42) is called from Kernel/Kernel.c (line 147) and installs the syscall interrupt via
IdtSetGate(0x80, (uint64_t)SyscallEntry, 0x08, 0x8E).IdtSetGateis defined in Kernel/Idt.c (line 268) and declared in Kernel/Idt.h.No further changes needed.
| // Test syscall | ||
| asm volatile( | ||
| "mov $2, %%rax\n" // SYS_WRITE | ||
| "mov $1, %%rbx\n" // stdout | ||
| "mov %0, %%rcx\n" // message | ||
| "mov $13, %%r8\n" // length | ||
| "int $0x80\n" | ||
| : | ||
| : "r"("Syscall test!") | ||
| : "rax", "rbx", "rcx", "r8" | ||
| ); |
There was a problem hiding this comment.
Fix hardcoded syscall number and add error handling.
The syscall test has several issues:
- Hardcodes
mov $2, %%raxinstead of usingSYS_WRITEconstant - No error handling or result checking
- Assumes specific register calling convention that may not match the actual handler
Apply this diff to use the defined constant and add basic error handling:
// Test syscall
+uint64_t result;
asm volatile(
- "mov $2, %%rax\n" // SYS_WRITE
+ "mov %1, %%rax\n" // SYS_WRITE
"mov $1, %%rbx\n" // stdout
- "mov %0, %%rcx\n" // message
+ "mov %2, %%rcx\n" // message
"mov $13, %%r8\n" // length
"int $0x80\n"
- :
- : "r"("Syscall test!")
+ : "=a"(result)
+ : "i"(SYS_WRITE), "r"("Syscall test!")
: "rax", "rbx", "rcx", "r8"
);
+if (result != 0) {
+ PrintKernel("Syscall failed!\n");
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Kernel/Kernel.c around lines 124 to 134, replace the hardcoded syscall number
2 with the defined SYS_WRITE constant to improve clarity and maintainability.
Add error handling by checking the return value of the syscall in the
appropriate register and handle errors accordingly. Also, verify and adjust the
register usage to match the actual syscall calling convention on the target
architecture to ensure correctness.
|
|
||
| void SyscallInit(void) { | ||
| // Install syscall interrupt (0x80) | ||
| IdtSetGate(0x80, (uint64_t)SyscallEntry, 0x08, 0x8E); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define constants for interrupt vector and flags.
Consider defining constants for better maintainability and documentation.
Add these definitions at the top of the file:
#define SYSCALL_INTERRUPT_VECTOR 0x80
#define IDT_INTERRUPT_GATE_KERNEL 0x8E // Present, DPL=0, 64-bit interrupt gateThen update the code:
- IdtSetGate(0x80, (uint64_t)SyscallEntry, 0x08, 0x8E);
+ IdtSetGate(SYSCALL_INTERRUPT_VECTOR, (uint64_t)SyscallEntry, 0x08, IDT_INTERRUPT_GATE_KERNEL);🤖 Prompt for AI Agents
In Kernel/Syscall.c at line 44, the interrupt vector 0x80 and flags 0x8E are
used as magic numbers. Define constants SYSCALL_INTERRUPT_VECTOR as 0x80 and
IDT_INTERRUPT_GATE_KERNEL as 0x8E at the top of the file, then replace the
literal values in the IdtSetGate call with these constants for clarity and
maintainability.
There was a problem hiding this comment.
hmm i thought the 3rd args of idtsetgate is 0x08 but SYSCALL_INTERRUPT_VECTOR is 0x80?
There was a problem hiding this comment.
@assembler-0, you're absolutely correct! I made an error in my previous suggestion. The third argument (0x08) is the selector, not the interrupt vector. The interrupt vector 0x80 is the first argument.
The correct constant definitions should be:
#define SYSCALL_INTERRUPT_VECTOR 0x80
#define KERNEL_CODE_SELECTOR 0x08
#define IDT_INTERRUPT_GATE_KERNEL 0x8EAnd the corrected code:
IdtSetGate(SYSCALL_INTERRUPT_VECTOR, (uint64_t)SyscallEntry, KERNEL_CODE_SELECTOR, IDT_INTERRUPT_GATE_KERNEL);Thank you for catching that mistake!
✏️ Learnings added
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.290Z
Learning: In IdtSetGate function calls, the parameters are: (interrupt_vector, handler_address, selector, flags). The interrupt vector (like 0x80) is the first argument, and the selector (like 0x08 for kernel code segment) is the third argument.
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.
| return 0; | ||
|
|
||
| case SYS_GETPID: | ||
| return GetCurrentProcess()->pid; |
There was a problem hiding this comment.
Add null pointer check for GetCurrentProcess().
Similar to the SYS_EXIT case, validate the process pointer before dereferencing.
Apply this diff:
- return GetCurrentProcess()->pid;
+ Process* current = GetCurrentProcess();
+ return current ? current->pid : -1;📝 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.
| return GetCurrentProcess()->pid; | |
| Process* current = GetCurrentProcess(); | |
| return current ? current->pid : -1; |
🤖 Prompt for AI Agents
In Kernel/Syscall.c at line 35, add a null pointer check for the result of
GetCurrentProcess() before accessing its pid field. This prevents potential null
dereference errors. Implement a conditional check to verify that
GetCurrentProcess() returns a valid pointer, and handle the null case
appropriately, similar to the existing SYS_EXIT case.
| switch (syscall_num) { | ||
| case SYS_EXIT: | ||
| // Terminate current process | ||
| GetCurrentProcess()->state = PROC_TERMINATED; |
There was a problem hiding this comment.
Add null pointer check for GetCurrentProcess().
The code assumes GetCurrentProcess() always returns a valid pointer. Add a null check to prevent potential kernel panic.
Apply this diff:
- GetCurrentProcess()->state = PROC_TERMINATED;
+ Process* current = GetCurrentProcess();
+ if (current) {
+ current->state = PROC_TERMINATED;
+ }🤖 Prompt for AI Agents
In Kernel/Syscall.c at line 10, add a null pointer check for the result of
GetCurrentProcess() before accessing its state field. If GetCurrentProcess()
returns null, avoid dereferencing it to prevent kernel panic. Implement a
conditional check to ensure the pointer is valid before setting the state to
PROC_TERMINATED.
| char* buffer = (char*)arg2; | ||
| for (uint64_t i = 0; i < arg3; i++) { | ||
| if (buffer[i] == '\0') break; | ||
| PrintKernelAt(&buffer[i], CurrentLine, CurrentColumn++); | ||
| if (CurrentColumn >= 80) { | ||
| CurrentLine++; | ||
| CurrentColumn = 0; | ||
| } | ||
| } | ||
| return arg3; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical issues in SYS_WRITE implementation.
Several problems need to be addressed:
- Security: The buffer pointer is not validated before dereferencing, which could cause a kernel panic if userspace passes an invalid pointer.
- Logic error:
PrintKernelAt(&buffer[i], ...)passes the address of a single character, but based on the function signature in the relevant snippets, it expects a null-terminated string. - Missing declarations:
CurrentLineandCurrentColumnare used but not declared or imported. - Missing bounds check: No validation that
CurrentLinedoesn't exceed screen boundaries.
Run the following script to verify the missing declarations:
To fix the immediate issues, consider:
- if (arg1 == 1) { // stdout
- char* buffer = (char*)arg2;
- for (uint64_t i = 0; i < arg3; i++) {
- if (buffer[i] == '\0') break;
- PrintKernelAt(&buffer[i], CurrentLine, CurrentColumn++);
- if (CurrentColumn >= 80) {
- CurrentLine++;
- CurrentColumn = 0;
- }
- }
- return arg3;
- }
+ if (arg1 == 1) { // stdout
+ // TODO: Validate buffer pointer is in valid user memory range
+ char* buffer = (char*)arg2;
+ char single_char[2] = {0, 0};
+ static int CurrentLine = 0; // TODO: Move to proper location
+ static int CurrentColumn = 0; // TODO: Move to proper location
+
+ for (uint64_t i = 0; i < arg3; i++) {
+ if (buffer[i] == '\0') break;
+ single_char[0] = buffer[i];
+ PrintKernelAt(single_char, CurrentLine, CurrentColumn++);
+ if (CurrentColumn >= 80) {
+ CurrentLine++;
+ CurrentColumn = 0;
+ if (CurrentLine >= 25) { // Assuming 25 lines on screen
+ CurrentLine = 0; // Wrap around or implement scrolling
+ }
+ }
+ }
+ return arg3;
+ }🏁 Script executed:
#!/bin/bash
# Description: Check for CurrentLine and CurrentColumn declarations
# Search for CurrentLine and CurrentColumn declarations
rg -A 2 "CurrentLine|CurrentColumn" --type c --type hLength of output: 1410
---
🏁 Script executed:
```shell
#!/bin/bash
# 1. Check if Kernel/Syscall.c includes Kernel/Kernel.h
echo "=== Includes in Syscall.c ==="
grep -E "#include .*Kernel.h" -n Kernel/Syscall.c || echo "No Kernel.h include found."
# 2. Show first 50 lines of Kernel/Syscall.c
echo -e "\n=== Top of Kernel/Syscall.c ==="
sed -n '1,50p' Kernel/Syscall.c
# 3. Find PrintKernelAt declaration/signature
echo -e "\n=== PrintKernelAt in headers ==="
rg -n "PrintKernelAt" --type h
# 4. Show PrintKernelAt definition in C files
echo -e "\n=== PrintKernelAt definition in C files ==="
rg -n "PrintKernelAt" --type c
Length of output: 3511
Address SYS_WRITE buffer validation, single-char printing, and screen bounds
The SYS_WRITE path in Kernel/Syscall.c still needs:
- User‐pointer validation before dereference
- Correct use of PrintKernelAt (it expects a null-terminated string)
- Bounds checking on CurrentLine (wrap or scroll at screen height)
Locations to update:
- Kernel/Syscall.c, lines ~17–26 (case SYS_WRITE)
Suggested patch:
--- a/Kernel/Syscall.c
+++ b/Kernel/Syscall.c
@@ case SYS_WRITE:
- if (arg1 == 1) { // stdout
- char* buffer = (char*)arg2;
- for (uint64_t i = 0; i < arg3; i++) {
- if (buffer[i] == '\0') break;
- PrintKernelAt(&buffer[i], CurrentLine, CurrentColumn++);
- if (CurrentColumn >= 80) {
- CurrentLine++;
- CurrentColumn = 0;
- }
- }
- return arg3;
- }
+ if (arg1 == 1) { // stdout
+ // 1) Validate user buffer before use
+ if (!IsValidUserPtr(arg2, arg3)) return -1;
+ char* buffer = (char*)arg2;
+ char tmp[2] = {0, 0};
+
+ // 2) Print one char at a time using a null-terminated string
+ for (uint64_t i = 0; i < arg3 && buffer[i] != '\0'; i++) {
+ tmp[0] = buffer[i];
+ PrintKernelAt(tmp, CurrentLine, CurrentColumn++);
+
+ // 3) Handle column wrap
+ if (CurrentColumn >= 80) {
+ CurrentLine++;
+ CurrentColumn = 0;
+ // 4) Clamp or scroll at screen height
+ if (CurrentLine >= SCREEN_HEIGHT) {
+ CurrentLine = 0; // or invoke ScrollScreen()
+ }
+ }
+ }
+ return arg3;
+ }You’ll also need to:
- Define or import
IsValidUserPtr,SCREEN_HEIGHT(e.g. 25), or use your existing screen‐size/constants. - Optionally document that only fd 1 (stdout) is supported.
📝 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.
| char* buffer = (char*)arg2; | |
| for (uint64_t i = 0; i < arg3; i++) { | |
| if (buffer[i] == '\0') break; | |
| PrintKernelAt(&buffer[i], CurrentLine, CurrentColumn++); | |
| if (CurrentColumn >= 80) { | |
| CurrentLine++; | |
| CurrentColumn = 0; | |
| } | |
| } | |
| return arg3; | |
| case SYS_WRITE: | |
| if (arg1 == 1) { // stdout | |
| // 1) Validate user buffer before use | |
| if (!IsValidUserPtr(arg2, arg3)) { | |
| return -1; | |
| } | |
| char* buffer = (char*)arg2; | |
| char tmp[2] = {0, 0}; | |
| // 2) Print one char at a time using a null-terminated string | |
| for (uint64_t i = 0; i < arg3 && buffer[i] != '\0'; i++) { | |
| tmp[0] = buffer[i]; | |
| PrintKernelAt(tmp, CurrentLine, CurrentColumn++); | |
| // 3) Handle column wrap | |
| if (CurrentColumn >= 80) { | |
| CurrentLine++; | |
| CurrentColumn = 0; | |
| // 4) Clamp or scroll at screen height | |
| if (CurrentLine >= SCREEN_HEIGHT) { | |
| CurrentLine = 0; // or invoke ScrollScreen() | |
| } | |
| } | |
| } | |
| return arg3; | |
| } | |
| break; |
🤖 Prompt for AI Agents
In Kernel/Syscall.c around lines 17 to 26, the SYS_WRITE case lacks validation
of the user pointer before accessing the buffer, incorrectly uses PrintKernelAt
which expects a null-terminated string instead of single characters, and does
not check or handle screen line bounds. Fix this by first validating arg2 as a
user pointer using IsValidUserPtr before dereferencing. Then, modify the
printing logic to print single characters properly rather than passing pointers
to PrintKernelAt. Finally, add bounds checking for CurrentLine to wrap or scroll
when reaching SCREEN_HEIGHT (e.g., 25). Also, ensure to define or include
IsValidUserPtr and SCREEN_HEIGHT constants, and optionally document that only fd
1 (stdout) is supported in this syscall path.
| ; System call convention: rax=syscall_num, rdi=arg1, rsi=arg2, rdx=arg3 | ||
| mov rdi, rax ; syscall number | ||
| mov rsi, rbx ; arg1 | ||
| mov rdx, rcx ; arg2 | ||
| mov rcx, r8 ; arg3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify syscall convention in comments.
The comment on line 25 is misleading as it describes the target C calling convention, not the incoming syscall convention. Also, the syscall convention used here differs from the standard x86-64 Linux convention.
Update the comments to clarify both conventions:
- ; System call convention: rax=syscall_num, rdi=arg1, rsi=arg2, rdx=arg3
+ ; Incoming syscall convention: rax=syscall_num, rbx=arg1, rcx=arg2, r8=arg3
+ ; C calling convention target: rdi=syscall_num, rsi=arg1, rdx=arg2, rcx=arg3
mov rdi, rax ; syscall number
mov rsi, rbx ; arg1
mov rdx, rcx ; arg2
mov rcx, r8 ; arg3Note: The standard x86-64 Linux syscall convention uses: rax=syscall_num, rdi=arg1, rsi=arg2, rdx=arg3, r10=arg4, r8=arg5, r9=arg6. Consider aligning with this standard for compatibility.
📝 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.
| ; System call convention: rax=syscall_num, rdi=arg1, rsi=arg2, rdx=arg3 | |
| mov rdi, rax ; syscall number | |
| mov rsi, rbx ; arg1 | |
| mov rdx, rcx ; arg2 | |
| mov rcx, r8 ; arg3 | |
| ; Incoming syscall convention: rax=syscall_num, rbx=arg1, rcx=arg2, r8=arg3 | |
| ; C calling convention target: rdi=syscall_num, rsi=arg1, rdx=arg2, rcx=arg3 | |
| mov rdi, rax ; syscall number | |
| mov rsi, rbx ; arg1 | |
| mov rdx, rcx ; arg2 | |
| mov rcx, r8 ; arg3 |
🤖 Prompt for AI Agents
In Kernel/SyscallEntry.asm around lines 25 to 29, the comment describing the
syscall convention is misleading and does not match the actual registers used in
the code or the standard x86-64 Linux syscall convention. Update the comment to
clearly distinguish between the incoming syscall register usage and the target C
calling convention. Additionally, revise the code or comments to align with the
standard Linux syscall convention (rax=syscall_num, rdi=arg1, rsi=arg2,
rdx=arg3, r10=arg4, r8=arg5, r9=arg6) for better compatibility, or explicitly
document the custom convention used if not aligning.
1 participant
added syscalls
Summary by CodeRabbit
New Features
Chores
Style