Development #169
Development #169
Conversation
WalkthroughThis pull request establishes foundational x86_64 assembly infrastructure and refactors the scheduler abstraction layer. New assembly modules define memory structures (paging tables, stack), CPU feature detection (CPUID, SSE, NXE, WP), Global Descriptor Table setup, and Multiboot2 boot protocol compliance. The scheduler system is redesigned with a unified Scheduler.c providing backend-agnostic public functions that dispatch to MLFQ, EEVDF, or CFS implementations at compile time. EEVDF transitions from flag-based to capability-based privilege model with cryptographic token and PCB hash validation. MLFQ APIs are renamed for consistency. Spinlock contention handling is optimized to use scheduler yield instead of backoff. Build configuration is updated to include new modules and sources. Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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. 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
kernel/core/Kernel.c (1)
266-303: CR4 clobber: final write uses stale value; features may be cleared.You apply new_cr4 to hardware (Line 267), then later set CET/FSGSBASE on the old local cr4 and write that back (Lines 301-303). This likely disables SMEP/SMAP/PCID/UMIP/PKE you set earlier.
Fix by syncing cr4 = new_cr4 right after the first write so subsequent ORs accumulate on the updated mask.
Apply this minimal diff:
@@ // Apply all CR4 changes at once if (new_cr4 != cr4) { - __asm__ volatile("mov %0, %%cr4" :: "r"(new_cr4) : "memory"); + __asm__ volatile("mov %0, %%cr4" :: "r"(new_cr4) : "memory"); + // Keep local CR4 in sync for later CET/FSGSBASE updates + cr4 = new_cr4; } @@ // Write back the modified CR4 if (protection_enabled) { __asm__ volatile("mov %0, %%cr4" :: "r"(cr4) : "memory"); PrintKernelSuccess("System: Memory protection configured\n"); } else {include/Scheduler.h (1)
5-11: Remove duplicate/unconditional MLFQ include.Line 5 includes "MLFQ.h" unconditionally and again under VF_CONFIG_SCHED_MLFQ (Line 8). This will break non-MLFQ builds.
Apply this diff:
-#include "MLFQ.h" #if defined(VF_CONFIG_SCHED_MLFQ) #include "MLFQ.h" #elif defined(VF_CONFIG_SCHED_EEVDF) #include "EEVDF.h" #endifcmake/source.cmake (1)
4-15: Add multiboot2.asm to ASM_SOURCES in cmake/source.cmake.The file arch/x86_64/asm/multiboot2.asm exists but is missing from the ASM_SOURCES list. It must be linked into the bootable image to be discoverable by the bootloader within the first 32 KiB.
Apply this diff at cmake/source.cmake lines 4-6:
set(ASM_SOURCES arch/x86_64/asm/pxs.asm + arch/x86_64/asm/multiboot2.asm arch/x86_64/idt/IdtLoad.asmkernel/sched/MLFQ.c (3)
954-993: Queue mutation without scheduler_lock in MLFQProcessBlocked.
RemoveFromScheduler()andAddToScheduler()are called without takingscheduler_lock, racing with the scheduler.Take the lock around the block:
void MLFQProcessBlocked(uint32_t slot) { - MLFQProcessControlBlock* proc = &processes[slot]; + irq_flags_t flags = rust_spinlock_lock_irqsave(scheduler_lock); + MLFQProcessControlBlock* proc = &processes[slot]; @@ - if (proc->scheduler_node) { - RemoveFromScheduler(slot); - } + if (proc->scheduler_node) { + RemoveFromScheduler(slot); + } @@ - AddToScheduler(slot); + AddToScheduler(slot); } } + rust_spinlock_unlock_irqrestore(scheduler_lock, flags); }
236-244: Double-decrement of MLFQscheduler.total_processes confirmed.RemoveFromScheduler() decrements total_processes at line 239. Both TerminateProcess() (line 354) and ASTerminate() (line 413) call RemoveFromScheduler() but then decrement the counter again, corrupting the count.
Remove the extra decrements—RemoveFromScheduler() is the single source of truth for queue removal:
@@ TerminateProcess(...) - if (MLFQscheduler.total_processes > 0) { - MLFQscheduler.total_processes--; - } + /* already decremented in RemoveFromScheduler() */ @@ ASTerminate(...) - if (MLFQscheduler.total_processes > 0) { - MLFQscheduler.total_processes--; - } + /* already decremented in RemoveFromScheduler() */
900-905: Deadlock confirmed: ASTerminate re-acquires scheduler_lock held by MLFQSchedule.MLFQSchedule acquires
scheduler_lock(line 766) then callsAstraPreflightCheck(next_slot)(line 900), which callsASTerminate()at lines 740, 751, and 757. EachASTerminate()attempts to re-acquire the same non-reentrantscheduler_lock(line 386), causing deadlock.Replace all three
ASTerminate()calls withinAstraPreflightCheck(lines 730–762) with in-lock termination:@@ AstraPreflightCheck(...) - ASTerminate(proc->pid, "Pre-flight token validation failure"); + // In-lock terminate to avoid deadlock with MLFQSchedule + uint32_t slot = proc - processes; + proc->state = PROC_DYING; + proc->term_reason = TERM_SECURITY; + proc->exit_code = 0; + RemoveFromScheduler(slot); + ready_process_bitmap &= ~(1ULL << slot); + AddToTerminationQueueAtomic(slot); return 0; - ASTerminate(proc->pid, "Unauthorized privilege escalation"); + uint32_t slot = proc - processes; + proc->state = PROC_DYING; + proc->term_reason = TERM_SECURITY; + proc->exit_code = 0; + RemoveFromScheduler(slot); + ready_process_bitmap &= ~(1ULL << slot); + AddToTerminationQueueAtomic(slot); return 0; - ASTerminate(proc->pid, "ProcINFOPath tampering detected"); + uint32_t slot = proc - processes; + proc->state = PROC_DYING; + proc->term_reason = TERM_SECURITY; + proc->exit_code = 0; + RemoveFromScheduler(slot); + ready_process_bitmap &= ~(1ULL << slot); + AddToTerminationQueueAtomic(slot); return 0;Alternatively, create
ASTerminateLocked()that assumes the lock is held, avoiding code duplication.arch/x86_64/asm/pxs.asm (1)
61-68: Write full 64-bit paging entries instead of relying on prior zeroing.The code currently writes only 32-bit values to 64-bit PTE/PDPTE/PDE slots (lines 61–79 for the main init and lines 264, 276, 294 in
setup_dynamic_paging). While this works becauserep stosbclears memory beforehand, it creates a fragile dependency: any change to initialization order or scope will silently corrupt page tables.Explicitly zero the upper 32 bits after each 32-bit write:
lea eax, [pdp_table + 3] mov [edi], eax + mov dword [edi + 4], 0 lea eax, [pd_table + 3] mov [edi], eax + mov dword [edi + 4], 0Apply the same pattern to:
- Lines 71, 75, 79 (PDPTE entries for PD tables 2–4)
- Lines 86, 94, 102, 110 (PDE loops in
.map_pd1–.map_pd4)- Lines 264, 276, 294 (in
setup_dynamic_pagingfunction)kernel/sched/EEVDF.c (1)
614-620: Track removal of deprecated function.The deprecated
EEVDFCalculateSecureChecksumfunction should be removed once all dependencies are migrated to the newEEVDFCalculateTokenChecksumandEEVDFCalculatePCBHashfunctions.Do you want me to search the codebase for remaining usages and open an issue to track this cleanup?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
arch/x86_64/asm/bss.asm(1 hunks)arch/x86_64/asm/features.asm(1 hunks)arch/x86_64/asm/gdt.asm(1 hunks)arch/x86_64/asm/macros.asm(1 hunks)arch/x86_64/asm/multiboot2.asm(1 hunks)arch/x86_64/asm/pxs.asm(3 hunks)cmake/source.cmake(2 hunks)include/Scheduler.h(2 hunks)kernel/atomic/rust/src/ffi.rs(1 hunks)kernel/atomic/rust/src/spinlock.rs(1 hunks)kernel/core/Kernel.c(1 hunks)kernel/sched/EEVDF.c(13 hunks)kernel/sched/EEVDF.h(4 hunks)kernel/sched/MLFQ.c(12 hunks)kernel/sched/Scheduler.c(1 hunks)kernel/sched/Shared.h(0 hunks)
💤 Files with no reviewable changes (1)
- kernel/sched/Shared.h
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-06T05:31:15.868Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#111
File: kernel/sched/MLFQ.c:1514-1514
Timestamp: 2025-09-06T05:31:15.868Z
Learning: In the MLFQ scheduler, removing MLFQYield() from critical system processes like DynamoX and Astra improves system responsiveness, indicating that the yield mechanism may be too aggressive for system-critical processes that need quick response times.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-08-10T08:52:13.313Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T08:52:13.313Z
Learning: In VoidFrame's Process.c, PIDs are allocated using `__sync_fetch_and_add(&next_pid, 1)` which only increments and never reuses freed PIDs, causing PID exhaustion after MAX_PROCESSES terminations.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
arch/x86_64/asm/pxs.asmarch/x86_64/asm/multiboot2.asm
📚 Learning: 2025-09-01T01:10:22.123Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: README.md:9-9
Timestamp: 2025-09-01T01:10:22.123Z
Learning: The current VoidFrame Registers struct in arch/x86_64/cpu/Cpu.h only saves basic GPRs, segment registers, and exception context (interrupt_number, error_code, rip, cs, rflags, rsp, ss). It's missing critical registers for deep fault tracing: SIMD registers (XMM/YMM/ZMM), control registers (CR0/CR2/CR3/CR4), debug registers (DR0-DR7), and extended FPU state. The struct has a "DO NOT TOUCH" comment indicating it must match interrupt ASM stack layout.
Applied to files:
arch/x86_64/asm/pxs.asm
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
Applied to files:
arch/x86_64/asm/pxs.asmarch/x86_64/asm/multiboot2.asm
📚 Learning: 2025-09-14T11:01:02.698Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#130
File: kernel/core/InitRD.c:25-35
Timestamp: 2025-09-14T11:01:02.698Z
Learning: In VoidFrame kernel's InitRD.c, multiboot modules should be mapped using VMemMap() with PAGE_PRESENT | PAGE_WRITABLE flags instead of VMemMapMMIO(), as the latter applies cache-disabling flags (PAGE_NOCACHE | PAGE_WRITETHROUGH) that can corrupt data when reading regular RAM content.
Applied to files:
arch/x86_64/asm/pxs.asm
🧬 Code graph analysis (7)
kernel/atomic/rust/src/spinlock.rs (2)
kernel/atomic/rust/src/ffi.rs (1)
Yield(9-9)kernel/sched/Scheduler.c (1)
Yield(56-64)
include/Scheduler.h (2)
kernel/sched/Scheduler.c (13)
SchedulerInit(4-12)CreateProcess(15-23)CreateSecureProcess(25-33)GetCurrentProcess(35-43)GetCurrentProcessByPID(45-53)Yield(56-64)Schedule(67-75)KillProcess(78-86)KillCurrentProcess(88-96)ListProcesses(99-107)DumpPerformanceStats(110-118)DumpSchedulerState(121-129)GetSystemTicks(131-139)kernel/atomic/rust/src/ffi.rs (1)
Yield(9-9)
kernel/sched/EEVDF.h (1)
kernel/sched/EEVDF.c (1)
EEVDFCreateSecureProcess(905-1019)
kernel/atomic/rust/src/ffi.rs (1)
kernel/sched/Scheduler.c (1)
Yield(56-64)
kernel/sched/MLFQ.c (2)
kernel/atomic/rust/src/ffi.rs (2)
rust_spinlock_lock_irqsave(211-222)rust_spinlock_unlock_irqrestore(225-232)kernel/sched/EEVDF.c (1)
FindFreeSlotFast(622-637)
kernel/sched/EEVDF.c (3)
kernel/etc/Shell.c (1)
ShellProcess(1297-1325)kernel/etc/Console.c (1)
PrintKernelErrorF(290-297)kernel/atomic/rust/src/ffi.rs (1)
rust_spinlock_unlock_irqrestore(225-232)
kernel/sched/Scheduler.c (2)
kernel/sched/MLFQ.c (13)
MLFQSchedInit(1710-1801)MLFQCreateProcess(1168-1170)MLFQCreateSecureProcess(1015-1166)MLFQGetCurrentProcess(1227-1232)MLFQGetCurrentProcessByPID(1234-1246)MLFQYield(995-998)MLFQSchedule(765-952)MLFQKillProcess(456-458)MLFQKillCurrentProcess(1917-1921)MLFQListProcesses(1844-1881)MLFQDumpPerformanceStats(1803-1830)MLFQDumpSchedulerState(1883-1915)MLFQGetSystemTicks(163-166)kernel/sched/EEVDF.c (13)
EEVDFSchedInit(827-903)EEVDFCreateProcess(1021-1023)EEVDFCreateSecureProcess(905-1019)EEVDFGetCurrentProcess(1025-1030)EEVDFGetCurrentProcessByPID(1032-1043)EEVDFYield(1045-1048)EEVDFSchedule(706-825)EEVDFKillProcess(1317-1319)EEVDFKillCurrentProcess(1321-1324)EEVDFListProcesses(1424-1458)EEVDFDumpPerformanceStats(1477-1489)EEVDFDumpSchedulerState(1408-1422)EEVDFGetSystemTicks(121-123)
🪛 Clang (14.0.6)
kernel/sched/MLFQ.c
[warning] 1015-1015: 2 adjacent parameters of 'MLFQCreateSecureProcess' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 1015-1015: the first parameter in the range is 'privilege'
(clang)
[note] 1015-1015: the last parameter in the range is 'initial_flags'
(clang)
[warning] 1015-1015: parameter 'privilege' is unused
(misc-unused-parameters)
[warning] 1015-1015: parameter 'initial_flags' is unused
(misc-unused-parameters)
[note] 1609-1609: last of these clones ends here
(clang)
kernel/sched/EEVDF.c
[warning] 905-905: 2 adjacent parameters of 'EEVDFCreateSecureProcess' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 905-905: the first parameter in the range is 'priv'
(clang)
[note] 905-905: the last parameter in the range is 'capabilities'
(clang)
[warning] 905-905: parameter 'priv' is unused
(misc-unused-parameters)
[warning] 905-905: parameter 'capabilities' is unused
(misc-unused-parameters)
[warning] 1055-1055: statement should be inside braces
(readability-braces-around-statements)
[warning] 1056-1056: statement should be inside braces
(readability-braces-around-statements)
[warning] 1075-1075: parameter 'slot' is unused
(misc-unused-parameters)
[warning] 1076-1076: statement should be inside braces
(readability-braces-around-statements)
[warning] 1078-1078: variable 'proc' is not initialized
(cppcoreguidelines-init-variables)
kernel/sched/Scheduler.c
[error] 1-1: 'Scheduler.h' file not found
(clang-diagnostic-error)
[warning] 25-25: 2 adjacent parameters of 'CreateSecureProcess' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 25-25: the first parameter in the range is 'priv'
(clang)
[note] 25-25: the last parameter in the range is 'flag'
(clang)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
kernel/atomic/rust/src/ffi.rs (1)
9-9: LGTM - FFI declaration is correct.The
Yield()function declaration correctly exposes the C scheduler yield function with appropriate crate-private visibility for use in the spinlock contention path.kernel/atomic/rust/src/spinlock.rs (2)
107-112: Well-designed yield strategy with contention mitigation.The replacement of busy-wait backoff with scheduler yield in the high-contention path is a solid optimization. The reset of
attemptsto 0 is particularly clever—it forces the thread to traverse the backoff path (64 attempts) before yielding again, preventing yield thrashing and giving the lock holder time to complete.
107-112: Safety validation complete—no issues found.Both concerns have been validated:
CFS no-op confirmed: Scheduler.c line 62 shows CFS Yield() returns immediately without action. This is inefficient under high contention but not incorrect.
Yield() is safe from interrupt-disabled context: MLFQYield() (MLFQ.c:995) performs busy-wait with pause instructions; EEVDFYield() (EEVDF.c:1045) sets a flag. Neither acquires locks, making reentrancy and deadlock risks non-existent. Scheduler initialization (Kernel.c:764) completes before any lock acquisition, ensuring proper state.
The code is correct as-is.
cmake/source.cmake (2)
28-33: Add kernel/sched/Scheduler.c: LGTM.The unified dispatcher is now part of the build.
187-189: Include dirs: LGTM.Adding arch/x86_64 and arch/x86_64/asm is consistent with new sources.
include/Scheduler.h (1)
13-25: No critical issue; risk is lower than suggested.The review correctly identifies that
PROC_FLAG_*macros lack definitions whenVF_CONFIG_SCHED_CFSis selected. However, verification reveals:
VF_CONFIG_SCHED_CFS is defined in the header but cannot currently be selected via CMake—the build system only accepts
MLFQorEEVDFand fails otherwise (seecmake/features.cmakelines 47–52).No external code actually uses
PROC_FLAG_*—all usages are confined tokernel/sched/MLFQ.c, which only compiles whenVF_CONFIG_SCHED_MLFQis active. The 18 files that includeScheduler.hdo not reference these macros.Adding the CFS definitions is a reasonable defensive practice for API consistency, but the practical risk of undefined behavior is minimal given the current architecture.
If CFS backend support is planned and will become selectable in the build system, apply the suggested diff. Otherwise, this can be deferred until CFS is actually integrated into the CMake configuration.
kernel/sched/MLFQ.c (1)
1149-1151: Good: use GDT selectors from Gdt.h instead of magic constants.This prevents drift if the GDT layout changes.
arch/x86_64/asm/bss.asm (1)
1-25: LGTM! Well-structured x86_64 boot memory layout.The BSS and data section definitions provide a clean foundation for paging setup and boot-time initialization. The alignment requirements are correctly specified, and the 64GB memory support (4 base + 60 extended PD tables) is properly implemented.
kernel/sched/EEVDF.c (5)
598-612: LGTM! Strong defense-in-depth security model.The separation of token checksum (intrinsic token integrity) and PCB hash (structural process integrity) provides robust validation. Hashing critical fields like
initial_entry_point,stack, andcapabilitiesprevents tampering with process structure.
858-878: LGTM! Correct initialization sequence.The idle process initialization properly follows the security model:
- Initialize all PCB fields (including
initial_entry_point)- Initialize token metadata
- Compute token checksum
- Compute PCB hash (after all fields finalized)
This ensures cryptographic validation covers the complete, finalized process state.
905-1019: Approve security enhancements; note parameter order concern.The refactored process creation properly implements the capability-based security model with immutable entry point tracking and dual-hash validation. Using
KERNEL_CODE_SELECTORandKERNEL_DATA_SELECTORfrom Gdt.h is clearer than magic numbers.However, the
privandcapabilitiesparameters could be accidentally swapped at call sites despite having different sizes (uint8_t vs uint64_t), as they're both integral types and adjacent.Based on static analysis hints
1054-1073: LGTM! Robust dual-validation with excellent diagnostics.The refactored
EEVDFValidateTokennow validates both token integrity (checksum) and process structural integrity (PCB hash), with clear diagnostic output showing expected vs. actual values. This dual-validation approach significantly strengthens the security posture.
1075-1174: LGTM! Consistent capability-based protection model.The preflight, postflight, and termination logic correctly uses the new capability-based model:
EEVDF_CAP_SUPERVISOR | EEVDF_CAP_CRITICAL | EEVDF_CAP_IMMUNEfor privilege checksEEVDF_CAP_IMMUNEto prevent terminationEEVDF_CAP_CRITICALto protect critical system processesAll token validation calls properly pass the PCB parameter for dual integrity checking.
| db 10011010b ; 0x9A - Present, DPL 0, Code, Executable, Read/Write | ||
| db 10101111b ; 0xAF - Granularity (4K), 64-bit Code (L-bit=1), Limit | ||
| db 0 ; base |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
GDT entries look correct for long mode; tiny style nits only.
- Code descriptor: L=1, D=0 via 0xAF is good.
- Data descriptor: 0xCF (D/B=1) is permitted but generally unnecessary in long mode; consider 0x8F (D/B=0) for tidiness. Optional.
- gdt64_pointer uses dq base; lgdt in 32-bit will read 6 bytes and ignore the extra. Fine, but a dd base (and optionally dd 0) is the more conventional form.
Also applies to: 15-16, 21-22
Scheduler changes
Summary by CodeRabbit
Release Notes
New Features
Improvements