Development #151
Development #151
Conversation
WalkthroughAdds a Rust synchronization library with C FFI, integrates its build into CMake, updates C spinlock internals to use rdtsc and __atomic, replaces many C spinlocks with Rust-backed locks across drivers, memory, IPC, and compositor, and introduces low-level IO/IRQ helpers and headers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Kernel code (drivers/subsys)
participant C as C runtime function
participant FFI as SpinlockRust.h (FFI)
participant R as Rust lib (voidframe-spinlock)
Caller->>C: enter API (e.g., Read/Alloc/Print)
C->>FFI: if lock == NULL ? rust_spinlock_new() : (noop)
alt lock allocated
C->>FFI: rust_spinlock_lock(_or_lock_irqsave_)
FFI->>R: acquire Rust spinlock (internal)
R-->>FFI: acquired
FFI-->>C: return
C->>C: perform critical section
C->>FFI: rust_spinlock_unlock(_or_unlock_irqrestore_)
FFI->>R: release Rust spinlock
R-->>FFI: released
FFI-->>C: return
C-->>Caller: return result
else allocation failed
C-->>Caller: print error / fallback or return error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 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 (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/Ide.c (1)
228-271: Per-call spinlock allocation breaks mutual exclusionAllocating a fresh RustSpinLock inside every read/write call means each caller locks a different object. Two threads will therefore breeze through the “locked” region at the same time, reintroducing the register/data races that the old global
ide_lockprevented. Please restore a shared lock (per controller/channel or global) that is created once during init and reused without being freed on every operation, and hold it across the whole transfer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
kernel/atomic/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
CMakeLists.txt(2 hunks)cmake/dependencies.cmake(1 hunks)drivers/Ide.c(10 hunks)kernel/atomic/Spinlock.h(2 hunks)kernel/atomic/SpinlockRust.h(1 hunks)kernel/atomic/rust/.cargo/config.toml(1 hunks)kernel/atomic/rust/CMakeLists.txt(1 hunks)kernel/atomic/rust/Cargo.toml(1 hunks)kernel/atomic/rust/README.md(1 hunks)kernel/atomic/rust/src/ffi.rs(1 hunks)kernel/atomic/rust/src/lib.rs(1 hunks)kernel/atomic/rust/src/mcs.rs(1 hunks)kernel/atomic/rust/src/rwlock.rs(1 hunks)kernel/atomic/rust/src/spinlock.rs(1 hunks)kernel/core/Compositor.c(1 hunks)mm/rust/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
kernel/atomic/rust/src/mcs.rs (2)
kernel/atomic/rust/src/rwlock.rs (1)
new(12-19)kernel/atomic/rust/src/spinlock.rs (3)
new(31-35)lock(38-77)unlock(80-82)
kernel/atomic/Spinlock.h (1)
kernel/atomic/rust/src/spinlock.rs (3)
backoff_delay(18-23)rdtsc(8-10)lock(38-77)
kernel/atomic/rust/src/spinlock.rs (1)
kernel/atomic/Spinlock.h (2)
backoff_delay(12-17)SpinLock(20-54)
kernel/atomic/rust/src/rwlock.rs (2)
kernel/atomic/rust/src/mcs.rs (2)
new(11-16)new(25-29)kernel/atomic/rust/src/spinlock.rs (1)
new(31-35)
drivers/Ide.c (2)
kernel/atomic/rust/src/spinlock.rs (1)
lock(38-77)kernel/atomic/rust/src/ffi.rs (4)
rust_spinlock_new(21-31)rust_spinlock_lock(48-52)rust_spinlock_unlock(55-59)rust_spinlock_free(34-45)
kernel/atomic/SpinlockRust.h (1)
kernel/atomic/rust/src/ffi.rs (19)
rust_spinlock_new(21-31)rust_spinlock_free(34-45)rust_spinlock_lock(48-52)rust_spinlock_unlock(55-59)rust_spinlock_try_lock(62-68)rust_spinlock_lock_irqsave(205-216)rust_spinlock_unlock_irqrestore(219-226)rust_mcs_lock_new(77-87)rust_mcs_lock_free(90-101)rust_mcs_node_new(104-114)rust_mcs_node_free(117-128)rust_mcs_lock(131-135)rust_mcs_unlock(138-142)rust_rwlock_new(149-159)rust_rwlock_free(162-173)rust_rwlock_read_lock(176-180)rust_rwlock_read_unlock(183-187)rust_rwlock_write_lock(190-194)rust_rwlock_write_unlock(197-201)
kernel/atomic/rust/src/ffi.rs (3)
kernel/atomic/rust/src/mcs.rs (3)
new(11-16)new(25-29)lock(32-46)kernel/atomic/rust/src/rwlock.rs (1)
new(12-19)kernel/atomic/rust/src/spinlock.rs (2)
new(31-35)lock(38-77)
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 (2)
kernel/etc/Console.c (2)
83-110: Per-call lock breaks console serialization; avoid recursive PrintKernel on failure; use a persistent lock with irqsaveAllocating a new lock per call does not serialize concurrent callers; ClearScreen and PrintKernel must share the same lock. Also, calling PrintKernel on lock allocation failure recurses into itself.
Use a static console_lock allocated once (e.g., in ConsoleInit), use irqsave, and fall back to SerialWrite if the lock is unavailable.
Apply this diff within ClearScreen:
- RustSpinLock* lock = rust_spinlock_new(); - if (!lock) { - PrintKernel("Console: Failed to allocate spinlock\n"); - return; - } - rust_spinlock_lock(lock); + uint64_t __flags = 0; + if (console_lock) { + __flags = rust_spinlock_lock_irqsave(console_lock); + } else { + SerialWrite("Console: console_lock not initialized; clearing without lock\n"); + } @@ - rust_spinlock_unlock(lock); - rust_spinlock_free(lock); + if (console_lock) { + rust_spinlock_unlock_irqrestore(console_lock, __flags); + }Add outside this hunk:
// at file-scope static RustSpinLock* console_lock = NULL; // in ConsoleInit(), early: console_lock = rust_spinlock_new(); if (!console_lock) { SerialWrite("Console: Failed to allocate console_lock\n"); }
181-198: Do not allocate a lock per PrintKernel; avoid recursion on failure; use shared console_lock with irqsavePer-call locks don’t coordinate across threads; and printing “Failed to allocate spinlock” via PrintKernel recurses.
Switch to the shared console_lock and irqsave; fall back to SerialWrite if unavailable.
- RustSpinLock* lock = rust_spinlock_new(); - if (!lock) { - PrintKernel("Console: Failed to allocate spinlock\n"); - return; - } - rust_spinlock_lock(lock); + uint64_t __flags = 0; + if (console_lock) { + __flags = rust_spinlock_lock_irqsave(console_lock); + } else { + SerialWrite("Console: console_lock not initialized; printing without lock\n"); + } @@ - rust_spinlock_unlock(lock); - rust_spinlock_free(lock); + if (console_lock) { + rust_spinlock_unlock_irqrestore(console_lock, __flags); + }
♻️ Duplicate comments (2)
kernel/atomic/rust/src/spinlock.rs (1)
71-112: Restore deadlock watchdog state after timeout.After the deadlock timeout triggers at line 88 and
handle_potential_deadlock()completes, the loop continues without resettingstart,attempts, orbackoff. Sincerdtsc()keeps increasing, every subsequent iteration immediately re-enters the timeout path, keeping the lock in permanent maximum backoff even after contention clears.Apply this diff to reset the watchdog state after timeout:
// Deadlock detection with owner tracking if rdtsc() - start > DEADLOCK_TIMEOUT_CYCLES { self.handle_potential_deadlock(cpu_id, start); + start = rdtsc(); + attempts = 0; + backoff = 1; continue; }drivers/Ide.c (1)
374-383: Lock released before ATAPI I/O (still an issue)You unlock/free before IdeSelectDrive, command submission, and data reads, so controller access isn’t serialized. Keep the channel lock held until the operation completes; release it on every exit path after I/O finishes.
- rust_spinlock_unlock(lock); - rust_spinlock_free(lock); + /* keep lock held; unlock after the entire ATAPI operation completes */
🧹 Nitpick comments (1)
kernel/atomic/rust/src/spinlock.rs (1)
231-242: Document safety invariants for UnsafeCell access.The
unsafedereferences ofself.data.get()at lines 233 and 238 are correct because theSpinLockensures mutual exclusion, but this safety justification should be documented.Add safety comments:
pub fn lock(&'_ self) -> SpinLockGuard<'_, T> { self.lock.lock(); + // SAFETY: SpinLock ensures exclusive access; only one guard exists at a time SpinLockGuard::new(&self.lock, unsafe { &mut *self.data.get() }) } pub fn try_lock(&'_ self) -> Option<SpinLockGuard<'_, T>> { if self.lock.try_lock() { + // SAFETY: SpinLock ensures exclusive access; only one guard exists at a time Some(SpinLockGuard::new(&self.lock, unsafe { &mut *self.data.get() })) } else { None } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmake/source.cmake(1 hunks)drivers/Ide.c(10 hunks)include/Io.c(1 hunks)include/Io.h(2 hunks)kernel/atomic/rust/src/spinlock.rs(1 hunks)kernel/etc/Console.c(5 hunks)kernel/ipc/Ipc.c(0 hunks)mm/PMem.c(7 hunks)mm/VMem.c(19 hunks)
💤 Files with no reviewable changes (1)
- kernel/ipc/Ipc.c
✅ Files skipped from review due to trivial changes (1)
- mm/VMem.c
🧰 Additional context used
🧠 Learnings (1)
📚 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: VoidFrame's interrupt handlers in arch/x86_64/interrupts/Interrupts.asm only save basic GPRs and segment registers, missing critical registers for deep fault tracing: control registers (CR0/CR2/CR3/CR4), SIMD state (XMM/YMM/ZMM), debug registers (DR0-DR7), and extended FPU state. The CPU initialization code enables SSE/AVX/XSAVE features but interrupt handlers don't preserve this extended state. Enhanced fault tracing requires selective extended state saving for critical exceptions (ISR 6, 8, 13, 14) while keeping lightweight handling for regular interrupts.
Applied to files:
include/Io.c
🧬 Code graph analysis (5)
drivers/Ide.c (2)
kernel/atomic/rust/src/ffi.rs (4)
rust_spinlock_new(21-31)rust_spinlock_lock(48-52)rust_spinlock_unlock(55-59)rust_spinlock_free(34-45)kernel/etc/Console.c (1)
PrintKernel(175-199)
mm/PMem.c (2)
kernel/atomic/rust/src/ffi.rs (4)
rust_spinlock_new(21-31)rust_spinlock_lock_irqsave(205-216)rust_spinlock_unlock_irqrestore(219-226)rust_spinlock_free(34-45)kernel/etc/Console.c (1)
PrintKernel(175-199)
kernel/etc/Console.c (2)
kernel/atomic/rust/src/spinlock.rs (2)
lock(71-112)lock(231-234)kernel/atomic/rust/src/ffi.rs (4)
rust_spinlock_new(21-31)rust_spinlock_lock(48-52)rust_spinlock_unlock(55-59)rust_spinlock_free(34-45)
kernel/atomic/rust/src/spinlock.rs (3)
kernel/atomic/Spinlock.h (2)
backoff_delay(12-17)SpinLock(20-54)kernel/atomic/rust/src/mcs.rs (4)
new(11-16)new(25-29)lock(32-46)unlock(49-73)kernel/atomic/rust/src/rwlock.rs (1)
new(12-19)
include/Io.h (1)
include/Io.c (7)
cli(4-12)sti(14-22)save_irq_flags(44-48)restore_irq_flags(50-52)cpuid(25-29)rdmsr(32-36)wrmsr(38-42)
⏰ 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 (9)
include/Io.h (1)
88-102: LGTM!The new public API declarations for interrupt control, CPUID, MSR access, and IRQ flag management are well-structured and align with the implementations provided in
include/Io.c.include/Io.c (3)
4-22: LGTM!The
cli()andsti()implementations correctly use inline assembly with memory barriers and platform-specific memory protection wrappers.
24-42: LGTM!The
cpuid(),rdmsr(), andwrmsr()implementations are correct and follow standard x86_64 conventions for these privileged operations.
44-52: Verify that memory protection wrappers are not needed for flag operations.
save_irq_flags()andrestore_irq_flags()don't use the_full_mem_prot_start()/_full_mem_prot_end()wrappers thatcli()andsti()use. Whilepushfq/popfqthemselves don't modify interrupt state directly,restore_irq_flags()can re-enable interrupts if the saved flags had IF=1.Confirm whether the memory protection wrappers should be added for consistency and correctness, especially for
restore_irq_flags().Based on learnings: VoidFrame has specific memory protection patterns for privileged operations that should be consistently applied.
cmake/source.cmake (1)
106-106: LGTM!The addition of
include/Io.cto theINCLUDE_SOURCESlist correctly integrates the new I/O implementation file into the build system.kernel/atomic/rust/src/spinlock.rs (4)
1-33: LGTM! Helper functions correctly mirror C implementation.The constants, intrinsic wrappers, and backoff logic match the C implementation in
kernel/atomic/Spinlock.hand are appropriate for kernel-level spinlocks.
35-58: LGTM! Struct definitions and trait implementations are correct.The unsafe
SyncandSendimplementations forSpinMutex<T>are sound:
Syncis justified because theUnsafeCellis protected by theSpinLock, ensuring only one thread can access&mut Tat a timeSendis justified because transferring ownership of the lock and its atomics between threads is safe whenT: Send
153-190: LGTM! Lock release and query methods are correct.The
unlock()method properly clears ownership tracking before releasing the lock withReleaseordering, and the contention decay provides good adaptive behavior. Thetry_lock()and query methods are straightforward and correct.
193-220: LGTM! RAII guard implementation is correct.The guard correctly implements:
- Automatic unlock on drop
- Deref/DerefMut for data access
- !Send + !Sync via
PhantomDatato prevent inter-thread transfer- Lifetime binding to prevent use-after-free
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 (2)
drivers/Ide.c (1)
366-411: Hold the IDE lock for the entire ATAPI read
IdeReadLBA2048acquireside_lock, immediately releases it, and then performs drive selection and packet transfer without any lock held. This reopens the race we just closed elsewhere: concurrent callers can interleave ATAPI commands on the same controller and corrupt the transaction. Keepide_lockacquired until the operation completes (including all error paths), matching the block/character read/write paths.- rust_spinlock_lock(ide_lock); - uint16_t base_port = channels[channel].base_port; - rust_spinlock_unlock(ide_lock); + rust_spinlock_lock(ide_lock); + uint16_t base_port = channels[channel].base_port; int result; result = IdeSelectDrive(base_port, drive_num, 0); // LBA is in the packet if (result != IDE_OK) { - return result; + rust_spinlock_unlock(ide_lock); + return result; } @@ - result = IdeWaitData(base_port); - if (result != IDE_OK) { - return result; - } + result = IdeWaitData(base_port); + if (result != IDE_OK) { + rust_spinlock_unlock(ide_lock); + return result; + } @@ - result = IdeWaitData(base_port); - if (result != IDE_OK) { - return result; - } + result = IdeWaitData(base_port); + if (result != IDE_OK) { + rust_spinlock_unlock(ide_lock); + return result; + } @@ - return IDE_OK; + rust_spinlock_unlock(ide_lock); + return IDE_OK;(Ensure every early return also unlocks.)
kernel/ipc/Ipc.c (1)
145-250: Guard all IPC queue lock acquisitions
Several paths (IpcReceiveMessageType,IpcHasMessageType,IpcFlushQueue, etc.) callrust_spinlock_lock(queue->lock)without first ensuringqueue->lockis initialized. Because the Rust FFI treats NULL as “do nothing”, these become unlocked critical sections until some other code happens to callIpcSendMessage/IpcReceiveMessage. Please add the same lazy-initialization guard (if (!queue->lock) queue->lock = rust_spinlock_new();with failure handling) before every lock/unlock pair so the queue is always protected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmake/variable.cmake(1 hunks)drivers/Ide.c(11 hunks)drivers/virtio/VirtioBlk.c(7 hunks)include/Scheduler.h(1 hunks)kernel/atomic/rust/src/ffi.rs(1 hunks)kernel/core/Compositor.c(7 hunks)kernel/etc/Console.c(6 hunks)kernel/ipc/Ipc.c(11 hunks)kernel/ipc/Ipc.h(2 hunks)kernel/sched/EEVDF.c(16 hunks)mm/PMem.c(9 hunks)mm/security/Cerberus.c(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-17T08:16:43.539Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#93
File: kernel/memory/VMem.c:72-79
Timestamp: 2025-08-17T08:16:43.539Z
Learning: In VoidFrame kernel, VMemGetPhysAddr and related page table walking functions were causing panics because they assumed identity mapping existed for physical addresses < IDENTITY_MAP_SIZE (4GB), but the bootloader doesn't provide this 1:1 mapping. The fix is to remove all identity mapping assumptions and always use PHYS_TO_VIRT() for page table access in functions like VMemGetPageTablePhys, VMemMap, VMemMapHuge, VMemGetPhysAddr, VMemUnmap, VMemMapMMIO, and VMemUnmapMMIO.
Applied to files:
mm/PMem.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel's VBEConsoleClear() function, the color is hardcoded to 0x08 (gray on black) instead of using the current console color state, causing color washing issues. The function should use console.color or the current color state to maintain color consistency across screen clears.
Applied to files:
kernel/etc/Console.c
🧬 Code graph analysis (10)
include/Scheduler.h (2)
kernel/sched/MLFQ.c (1)
MLFQGetSystemTicks(162-165)kernel/sched/EEVDF.c (1)
EEVDFGetSystemTicks(120-122)
drivers/virtio/VirtioBlk.c (2)
kernel/atomic/rust/src/ffi.rs (3)
rust_spinlock_new(26-36)rust_spinlock_lock(53-57)rust_spinlock_unlock(60-64)kernel/etc/Console.c (2)
PrintKernelError(215-220)PrintKernel(175-194)
mm/PMem.c (2)
kernel/atomic/rust/src/ffi.rs (3)
rust_spinlock_new(26-36)rust_spinlock_lock_irqsave(210-221)rust_spinlock_unlock_irqrestore(224-231)kernel/etc/Console.c (3)
PrintKernelWarning(222-227)PrintKernelError(215-220)PrintKernel(175-194)
kernel/ipc/Ipc.c (1)
kernel/atomic/rust/src/ffi.rs (3)
rust_spinlock_new(26-36)rust_spinlock_lock(53-57)rust_spinlock_unlock(60-64)
kernel/sched/EEVDF.c (1)
kernel/atomic/rust/src/ffi.rs (5)
rust_spinlock_lock_irqsave(210-221)rust_spinlock_unlock_irqrestore(224-231)rust_spinlock_new(26-36)rust_spinlock_lock(53-57)rust_spinlock_unlock(60-64)
kernel/etc/Console.c (1)
kernel/atomic/rust/src/ffi.rs (3)
rust_spinlock_new(26-36)rust_spinlock_lock(53-57)rust_spinlock_unlock(60-64)
kernel/atomic/rust/src/ffi.rs (3)
kernel/atomic/rust/src/mcs.rs (3)
new(11-16)new(25-29)lock(32-46)kernel/atomic/rust/src/spinlock.rs (5)
new(61-68)new(194-200)new(224-229)lock(71-112)lock(231-234)kernel/atomic/rust/src/rwlock.rs (1)
new(12-19)
drivers/Ide.c (2)
kernel/atomic/rust/src/ffi.rs (3)
rust_spinlock_new(26-36)rust_spinlock_lock(53-57)rust_spinlock_unlock(60-64)kernel/etc/Console.c (1)
PrintKernelError(215-220)
mm/security/Cerberus.c (2)
kernel/atomic/rust/src/ffi.rs (3)
rust_spinlock_lock(53-57)rust_spinlock_unlock(60-64)rust_spinlock_new(26-36)kernel/etc/Console.c (1)
PrintKernelError(215-220)
kernel/core/Compositor.c (4)
kernel/atomic/rust/src/ffi.rs (3)
rust_spinlock_lock_irqsave(210-221)rust_spinlock_unlock_irqrestore(224-231)rust_spinlock_new(26-36)kernel/etc/StringOps.c (1)
FastStrCmp(43-53)mm/MemOps.c (1)
FastMemset(23-210)kernel/etc/Console.c (1)
PrintKernelError(215-220)
⏰ 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
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores