Development #4
Development #4
Conversation
WalkthroughThis update introduces symbolic constants for syscall interrupt vector configuration, refactors the syscall handler to cache the current process pointer and improve null checks, and updates the build script to use a hardcoded ISO filename. A commented line is also removed from a kernel task function, with no other logic changes. Changes
Sequence Diagram(s)sequenceDiagram
participant UserApp as User Application
participant CPU as CPU
participant Kernel as Kernel
participant Proc as Process Manager
UserApp->>CPU: Triggers syscall (int SYSCALL_INTERRUPT_VECTOR)
CPU->>Kernel: Enters syscall handler
Kernel->>Kernel: Cache current process pointer
alt Valid current process
Kernel->>Proc: Handle syscall (e.g., SYS_EXIT, SYS_GETPID)
else No current process
Kernel->>Kernel: Return error (-1 for SYS_GETPID)
end
Kernel-->>CPU: Return to user mode
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Kernel/Syscall.h (1)
11-12: Consider renaming the constant to reflect its actual purpose.While using symbolic constants is excellent for maintainability,
SYSCALL_INTERRUPT_VECTORset to0x08is misleading since0x08represents the kernel code segment selector, not an interrupt vector. The actual interrupt vector is0x80.Consider renaming to something like
KERNEL_CODE_SELECTORorSYSCALL_CODE_SELECTORto better reflect its purpose.-#define SYSCALL_INTERRUPT_VECTOR 0x08 +#define KERNEL_CODE_SELECTOR 0x08meson.build (1)
104-104: Consider the maintainability implications of hardcoding the ISO filename.While this change simplifies the command, hardcoding
'VoidFrame.iso'instead of using theisotarget variable makes the build less flexible. If the ISO target name changes in the future, this will break.Consider keeping the variable reference for better maintainability:
- command : ['qemu-system-x86_64', '-cdrom', 'VoidFrame.iso', '-m', '1G'] + command : ['qemu-system-x86_64', '-cdrom', iso, '-m', '1G']
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Kernel/Kernel.c(1 hunks)Kernel/Syscall.c(3 hunks)Kernel/Syscall.h(1 hunks)meson.build(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
Kernel/Syscall.c (1)
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.
Kernel/Syscall.h (1)
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.
🧬 Code Graph Analysis (1)
Kernel/Syscall.c (1)
Kernel/Idt.h (1)
IdtSetGate(25-25)
🔇 Additional comments (4)
Kernel/Kernel.c (1)
134-134: Good cleanup.Removing the unnecessary comment improves code cleanliness.
Kernel/Syscall.c (3)
7-7: Excellent optimization: caching the current process pointer.This change improves performance by avoiding repeated calls to
GetCurrentProcess()throughout the handler execution.
11-13: Good defensive programming with null check.Adding the null check before accessing the current process prevents potential null pointer dereferences in the SYS_EXIT case.
38-38: Proper error handling for SYS_GETPID.Returning -1 when no current process is found is a good error handling practice that prevents null pointer dereferences.
| void SyscallInit(void) { | ||
| // Install syscall interrupt (0x80) | ||
| IdtSetGate(0x80, (uint64_t)SyscallEntry, 0x08, 0x8E); | ||
| IdtSetGate(0x80, (uint64_t)SyscallEntry, SYSCALL_INTERRUPT_VECTOR, IDT_INTERRUPT_GATE_KERNEL); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the symbolic constant usage is correct.
The use of symbolic constants is excellent for maintainability. However, there's a naming issue with SYSCALL_INTERRUPT_VECTOR - it's set to 0x08 which is actually the kernel code segment selector, not an interrupt vector.
Based on the retrieved learnings, the IdtSetGate parameters are correct:
0x80(interrupt vector)SyscallEntry(handler address)SYSCALL_INTERRUPT_VECTOR(0x08 - kernel code selector)IDT_INTERRUPT_GATE_KERNEL(0x8E - gate flags)
The constant naming could be clearer. Please verify that the syscall setup works correctly with these constants:
🏁 Script executed:
#!/bin/bash
# Description: Verify syscall interrupt vector setup and usage
# Expected: Find consistent usage of 0x80 as interrupt vector and 0x08 as selector
echo "=== Searching for syscall interrupt vector usage ==="
rg -A 2 -B 2 "0x80|syscall.*interrupt"
echo "=== Searching for IdtSetGate function calls ==="
rg -A 2 -B 2 "IdtSetGate"
echo "=== Searching for symbolic constants usage ==="
rg -A 2 -B 2 "SYSCALL_INTERRUPT_VECTOR|IDT_INTERRUPT_GATE_KERNEL"Length of output: 21978
Rename SYSCALL_INTERRUPT_VECTOR to clarify its role as a segment selector
The parameters to IdtSetGate are:
- 0x80 = interrupt vector
- SyscallEntry = handler address
- 0x08 = segment selector, not an interrupt vector
- 0x8E = gate flags
Everything is wired up correctly, but the constant name is misleading. Please rename it and update its use:
- Kernel/Syscall.h
-#define SYSCALL_INTERRUPT_VECTOR 0x08
+#define SYSCALL_SEGMENT_SELECTOR 0x08- Kernel/Syscall.c
- IdtSetGate(0x80, (uint64_t)SyscallEntry, SYSCALL_INTERRUPT_VECTOR, IDT_INTERRUPT_GATE_KERNEL);
+ IdtSetGate(0x80, (uint64_t)SyscallEntry, SYSCALL_SEGMENT_SELECTOR, IDT_INTERRUPT_GATE_KERNEL);🤖 Prompt for AI Agents
In Kernel/Syscall.c at line 47 and Kernel/Syscall.h, rename the constant
SYSCALL_INTERRUPT_VECTOR to a name that clearly indicates it is a segment
selector, such as SYSCALL_SEGMENT_SELECTOR. Update all references to this
constant in both files accordingly to reflect its true purpose as a segment
selector rather than an interrupt vector.
1 participant
Suggested Fix applied, check SYSCALL_INTERRUPT_VECTOR
Summary by CodeRabbit
Bug Fixes
Chores
Style