FCKED #15
FCKED #15
Conversation
WalkthroughThis update introduces extensive debug printing throughout the bootloader and kernel, replaces VGA text output with port-based debug output, and refactors process context management to use a unified register structure. The Programmable Interrupt Controller (PIC) remapping is added, and the process scheduler is consolidated with improved security checks and context handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Bootloader
participant Kernel
participant PIC
participant Scheduler
participant Process
Bootloader->>Bootloader: debug_print("A") ... ("K")
Bootloader->>Kernel: Jump to KernelMain
Kernel->>Kernel: debug_print("KernelMain")
Kernel->>PIC: RemapPIC()
Kernel->>Kernel: ClearScreen()
Kernel->>Scheduler: RequestSchedule()
Scheduler->>Process: ScheduleFromInterrupt()
Process->>Process: Save/restore context, check security, switch process
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 (4)
Kernel/Drivers/Interrupts.c (1)
90-92: Consider removing or implementing the commented page fault handler.The commented-out page fault handler should either be implemented properly or removed to avoid confusion.
Kernel/Core/Kernel.c (1)
18-19: Debug print placement could affect screen clearing.Adding
PrintKernel("ClearScreen\n")before actually clearing the screen means this debug message will be immediately erased. Consider moving it after the clear operation or using a different debug output method.Kernel/Process/Switch.asm (1)
42-42: Consider parameterizing segment selectorsThe code hardcodes CS=0x08 and SS=0x10, which assumes all processes run in kernel mode. This will need modification when adding user-mode process support.
Consider storing the appropriate segment selectors in the process structure or deriving them from the process privilege level to support future user-mode processes.
Also applies to: 55-55
Kernel/Process/Process.c (1)
57-57: Consider conditional compilation for debug printsThe extensive debug printing throughout the process management code could impact performance in production. Consider wrapping these in a debug macro that can be disabled at compile time.
+#ifdef DEBUG_PROCESS PrintKernel("ProcessInit\n"); +#endifAlso applies to: 86-86, 92-92, 99-99, 178-178, 242-242, 251-251, 262-262, 271-271, 280-280
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Boot/boot.asm(8 hunks)Kernel/Core/Kernel.c(3 hunks)Kernel/Drivers/Interrupts.c(2 hunks)Kernel/Process/Process.c(9 hunks)Kernel/Process/Process.h(1 hunks)Kernel/Process/Switch.asm(1 hunks)Kernel/System/Syscall.c(1 hunks)grub.cfg(1 hunks)meson.build(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
Kernel/Drivers/Interrupts.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/Process/Process.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/System/Syscall.c (2)
Kernel/Process/Process.h (1)
RequestSchedule(57-57)Kernel/Process/Process.c (1)
RequestSchedule(52-54)
🔇 Additional comments (16)
meson.build (1)
124-124: LGTM! Debug console configuration is appropriate.The addition of
-debugcon stdioenables the QEMU debug console to output to standard I/O, which aligns with the extensive debug printing added throughout the kernel.grub.cfg (1)
6-6: LGTM! Terminal output configuration ensures console visibility.Setting
terminal_output consoleexplicitly directs GRUB's output to the console device, ensuring debug messages are visible during boot.Kernel/System/Syscall.c (1)
21-21: Good architectural change to request-based scheduling.Replacing direct
Schedule()call withRequestSchedule()aligns with the new scheduling architecture where scheduling is deferred to interrupt context. This is a safer approach for process termination.Kernel/Drivers/Interrupts.c (1)
78-88: Debug additions and timer interrupt handling look good.The stack dump provides valuable debugging information, and the re-enabled timer interrupt handling with scheduling is properly implemented with EOI signaling.
Kernel/Core/Kernel.c (3)
160-172: PIC remapping implementation is correct.The
RemapPIC()function correctly implements the standard 8259 PIC initialization sequence with proper ICW1-4 configuration and masks all interrupts initially.
203-203: Verify if keyboard interrupt should remain masked.The change from
& ~0x03to& ~1means only the timer interrupt (IRQ0) is unmasked, while the keyboard interrupt (IRQ1) remains masked. Is this intentional?
211-211: Scheduler refactoring properly integrated.The change from
Schedule()toRequestSchedule()correctly aligns with the new deferred scheduling architecture.Boot/boot.asm (3)
17-22: LGTM! Debug output implementation is appropriateThe debug macro correctly outputs characters to port 0xE9, which is a standard debug port supported by most x86 emulators and virtual machines.
194-195: Correct data size for multiboot variablesThe change from
dq(64-bit) todd(32-bit) is appropriate since these values come from 32-bit registers (EAX/EBX) in protected mode.
6-6: Hardcoded Multiboot2 Checksum VerifiedThe provided checksum (0x17ADAF12) matches the calculated value and the sum of all header fields equals zero. No changes required.
Kernel/Process/Process.h (2)
28-37: Well-structured interrupt frame layoutThe
Registersstruct correctly represents the x86-64 interrupt frame with all general-purpose registers, segment registers, and interrupt metadata. Thepackedattribute ensures no padding interferes with the layout.
39-40: Good design: unified context representationUsing the same structure for both interrupt handling and context switching eliminates potential mismatches and simplifies the code.
Kernel/Process/Switch.asm (1)
9-56: Assembly Offsets Match C Struct LayoutVerified via a ctypes-based script that every offset in
Switch.asm(0 through 200) aligns exactly with the packedRegistersstruct (total size 208 bytes). No mismatches found—no changes needed.Kernel/Process/Process.c (3)
153-172: Thorough interrupt frame initializationThe code properly initializes all fields of the interrupt frame, including segment selectors, RFLAGS with interrupts enabled, and proper stack alignment. The setup of a return stub address on the stack is a good safety measure.
224-226: Efficient context switching with unified structureUsing
FastMemcpyto save/restore the entire interrupt frame is more efficient than field-by-field copying and leverages the unifiedProcessContext/Registersstructure design.Also applies to: 235-237
200-215: Good security enhancement in schedulerThe addition of security token validation during scheduling helps prevent compromised processes from continuing execution. Immediate termination and resource cleanup of invalid processes is the correct approach.
| void PrintStack(uint64_t* ptr, int count) { | ||
| for (int i = 0; i < count; i++) { | ||
| PrintKernelHex(ptr[i]); | ||
| PrintKernel(" "); | ||
| } | ||
| PrintKernel("\n"); | ||
| } |
There was a problem hiding this comment.
Add bounds checking for stack dump.
The PrintStack function doesn't validate that accessing ptr[i] is safe. Consider adding bounds checking or documenting the safety assumptions.
-void PrintStack(uint64_t* ptr, int count) {
+void PrintStack(uint64_t* ptr, int count) {
+ if (!ptr || count <= 0) return;
for (int i = 0; i < count; i++) {
PrintKernelHex(ptr[i]);
PrintKernel(" ");
}
PrintKernel("\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.
| void PrintStack(uint64_t* ptr, int count) { | |
| for (int i = 0; i < count; i++) { | |
| PrintKernelHex(ptr[i]); | |
| PrintKernel(" "); | |
| } | |
| PrintKernel("\n"); | |
| } | |
| void PrintStack(uint64_t* ptr, int count) { | |
| if (!ptr || count <= 0) return; | |
| for (int i = 0; i < count; i++) { | |
| PrintKernelHex(ptr[i]); | |
| PrintKernel(" "); | |
| } | |
| PrintKernel("\n"); | |
| } |
🤖 Prompt for AI Agents
In Kernel/Drivers/Interrupts.c around lines 68 to 74, the PrintStack function
accesses ptr[i] without verifying that the pointer is valid or that the index is
within safe bounds. To fix this, add explicit bounds checking to ensure ptr is
not null and that i is less than the actual size of the stack or the accessible
memory region before accessing ptr[i]. Alternatively, document the assumptions
clearly if bounds checking is not feasible here.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores