Arch refactor#255
Conversation
- Replace earlyprintln! macro with emergency_println! for better error handling in trap handlers. - Update console_putchar to accept u8 instead of usize for consistency across architectures. - Modify RISC-V boot and trap handlers to use println! instead of earlyprintln!. - Introduce a BootConsoleBuffer to temporarily store console output during early boot before switching to runtime console. - Enhance the Console trait to include a write_bytes method for raw byte output. - Update various modules to utilize the new println! macro for logging and error messages. - Remove deprecated print and println macros from stdio.rs to streamline output handling. - Ensure that panic messages are printed using the new emergency_print method.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request unifies the hardware trap frame interface across architectures by introducing the HwTrapFrame trait, refactors early boot console logging to use a replayable buffer, and cleans up unused imports and macros. The code review highlights critical concurrency issues in the new console logging mechanism: the fallback logic in with_console_lock_or_fallback and write_fmt bypasses mutual exclusion on lock failure, which can lead to interleaved outputs, and the use of UnsafeCell<u8> in BootConsoleBuffer introduces data races that should be resolved by using AtomicU8. Additionally, since HwTrapFrame requires Copy, the trait methods for cloning and forking trap frames can be refactored to be entirely safe by replacing raw pointer copies with safe struct assignments.
| fn write_fmt(&mut self, args: fmt::Arguments) -> fmt::Result { | ||
| // 重写 write_fmt 以确保整个格式化输出在一个锁内完成 | ||
| // 这样可以防止多个 CPU 的日志输出交错 | ||
| let _guard = CONSOLE_LOCK.lock(); | ||
|
|
||
| // 创建一个临时的 writer，它使用 putchar_unlocked（不加锁） | ||
| struct UnlockedWriter; | ||
| impl Write for UnlockedWriter { | ||
| fn write_str(&mut self, s: &str) -> fmt::Result { | ||
| write_str_unlocked(s); | ||
| Ok(()) | ||
| } | ||
| if let Some(_guard) = CONSOLE_LOCK.try_lock() { | ||
| UnlockedWriter.write_fmt(args) | ||
| } else { | ||
| UnlockedWriter.write_fmt(args) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Similarly to with_console_lock_or_fallback, write_fmt currently falls back to printing without holding the lock if try_lock fails. This allows multiple CPUs to interleave their log messages, violating the guarantee stated in the comment. It should block on CONSOLE_LOCK to ensure formatted outputs are printed atomically.
fn write_fmt(&mut self, args: fmt::Arguments) -> fmt::Result {
// 重写 write_fmt 以确保整个格式化输出在一个锁内完成
// 这样可以防止多个 CPU 的日志输出交错
let _guard = CONSOLE_LOCK.lock();
UnlockedWriter.write_fmt(args)
}
}| struct BootConsoleBuffer { | ||
| write_seq: AtomicUsize, | ||
| replayed_seq: AtomicUsize, | ||
| bytes: [UnsafeCell<u8>; BOOT_CONSOLE_BUFFER_SIZE], | ||
| } |
There was a problem hiding this comment.
Using UnsafeCell<u8> in a multi-threaded/multi-core context without synchronization can lead to data races (e.g., when push writes to a cell while replay_to_runtime_console reads from it), which is Undefined Behavior in Rust. We can make this completely safe and lock-free by using AtomicU8 instead of UnsafeCell<u8>.
| struct BootConsoleBuffer { | |
| write_seq: AtomicUsize, | |
| replayed_seq: AtomicUsize, | |
| bytes: [UnsafeCell<u8>; BOOT_CONSOLE_BUFFER_SIZE], | |
| } | |
| struct BootConsoleBuffer { | |
| write_seq: AtomicUsize, | |
| replayed_seq: AtomicUsize, | |
| bytes: [core::sync::atomic::AtomicU8; BOOT_CONSOLE_BUFFER_SIZE], | |
| } |
| fn push(&self, byte: u8) { | ||
| let seq = self.write_seq.fetch_add(1, Ordering::Relaxed); | ||
| unsafe { | ||
| *self.bytes[seq % BOOT_CONSOLE_BUFFER_SIZE].get() = byte; | ||
| } | ||
| } | ||
|
|
||
| fn replay_to_runtime_console(&self) { | ||
| #[cfg(feature = "device")] | ||
| if let Some(console) = crate::device::console::MAIN_CONSOLE.read().as_ref() { | ||
| let write_seq = self.write_seq.load(Ordering::Acquire); | ||
| let oldest = write_seq.saturating_sub(BOOT_CONSOLE_BUFFER_SIZE); | ||
| let start = self.replayed_seq.load(Ordering::Acquire).max(oldest); | ||
|
|
||
| for seq in start..write_seq { | ||
| let byte = unsafe { *self.bytes[seq % BOOT_CONSOLE_BUFFER_SIZE].get() }; | ||
| console.write_bytes(&[byte]); | ||
| } | ||
|
|
||
| self.replayed_seq.store(write_seq, Ordering::Release); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Update push and replay_to_runtime_console to use AtomicU8 operations, which removes the need for unsafe blocks and guarantees data-race-free concurrency.
fn push(&self, byte: u8) {
let seq = self.write_seq.fetch_add(1, Ordering::Relaxed);
self.bytes[seq % BOOT_CONSOLE_BUFFER_SIZE].store(byte, Ordering::Release);
}
fn replay_to_runtime_console(&self) {
#[cfg(feature = "device")]
if let Some(console) = crate::device::console::MAIN_CONSOLE.read().as_ref() {
let write_seq = self.write_seq.load(Ordering::Acquire);
let oldest = write_seq.saturating_sub(BOOT_CONSOLE_BUFFER_SIZE);
let start = self.replayed_seq.load(Ordering::Acquire).max(oldest);
for seq in start..write_seq {
let byte = self.bytes[seq % BOOT_CONSOLE_BUFFER_SIZE].load(Ordering::Acquire);
console.write_bytes(&[byte]);
}
self.replayed_seq.store(write_seq, Ordering::Release);
}
}| unsafe fn set_clone_trap_frame( | ||
| &mut self, | ||
| parent_frame: &Self, | ||
| kernel_sp: usize, | ||
| user_sp: usize, | ||
| ); | ||
| unsafe fn set_fork_trap_frame(&mut self, parent_frame: &Self); |
There was a problem hiding this comment.
Since HwTrapFrame requires Copy (as specified by pub trait HwTrapFrame: Copy + ...), any implementing type is guaranteed to be Copy. Therefore, copying the trap frame can be done safely using simple struct assignment (e.g., *self = *parent_frame;) instead of core::ptr::copy_nonoverlapping. This allows us to remove the unsafe keyword from these trait methods entirely, improving safety and maintainability.
| unsafe fn set_clone_trap_frame( | |
| &mut self, | |
| parent_frame: &Self, | |
| kernel_sp: usize, | |
| user_sp: usize, | |
| ); | |
| unsafe fn set_fork_trap_frame(&mut self, parent_frame: &Self); | |
| fn set_clone_trap_frame( | |
| &mut self, | |
| parent_frame: &Self, | |
| kernel_sp: usize, | |
| user_sp: usize, | |
| ); | |
| fn set_fork_trap_frame(&mut self, parent_frame: &Self); |
| unsafe fn set_clone_trap_frame( | ||
| &mut self, | ||
| parent_frame: &Self, | ||
| kernel_sp: usize, | ||
| user_sp: usize, | ||
| ) { | ||
| unsafe { TrapFrame::set_clone_trap_frame(self, parent_frame, kernel_sp, user_sp) } | ||
| } | ||
|
|
||
| unsafe fn set_fork_trap_frame(&mut self, parent_frame: &Self) { | ||
| unsafe { TrapFrame::set_fork_trap_frame(self, parent_frame) } | ||
| } |
There was a problem hiding this comment.
Implement the safe version of set_clone_trap_frame and set_fork_trap_frame for TrapFrame under the HwTrapFrame trait. Note that the underlying inherent methods can also be refactored to be safe by replacing copy_nonoverlapping with simple struct assignment *self = *parent_frame;.
| unsafe fn set_clone_trap_frame( | |
| &mut self, | |
| parent_frame: &Self, | |
| kernel_sp: usize, | |
| user_sp: usize, | |
| ) { | |
| unsafe { TrapFrame::set_clone_trap_frame(self, parent_frame, kernel_sp, user_sp) } | |
| } | |
| unsafe fn set_fork_trap_frame(&mut self, parent_frame: &Self) { | |
| unsafe { TrapFrame::set_fork_trap_frame(self, parent_frame) } | |
| } | |
| fn set_clone_trap_frame( | |
| &mut self, | |
| parent_frame: &Self, | |
| kernel_sp: usize, | |
| user_sp: usize, | |
| ) { | |
| unsafe { TrapFrame::set_clone_trap_frame(self, parent_frame, kernel_sp, user_sp) } | |
| } | |
| fn set_fork_trap_frame(&mut self, parent_frame: &Self) { | |
| unsafe { TrapFrame::set_fork_trap_frame(self, parent_frame) } | |
| } |
References
- Prioritize consistency with existing architecture implementations (like RISC-V) when adding support for a new architecture (like LoongArch).
| unsafe fn set_clone_trap_frame( | ||
| &mut self, | ||
| parent_frame: &Self, | ||
| kernel_sp: usize, | ||
| user_sp: usize, | ||
| ) { | ||
| unsafe { TrapFrame::set_clone_trap_frame(self, parent_frame, kernel_sp, user_sp) } | ||
| } | ||
|
|
||
| unsafe fn set_fork_trap_frame(&mut self, parent_frame: &Self) { | ||
| unsafe { TrapFrame::set_fork_trap_frame(self, parent_frame) } | ||
| } |
There was a problem hiding this comment.
Implement the safe version of set_clone_trap_frame and set_fork_trap_frame for TrapFrame under the HwTrapFrame trait.
| unsafe fn set_clone_trap_frame( | |
| &mut self, | |
| parent_frame: &Self, | |
| kernel_sp: usize, | |
| user_sp: usize, | |
| ) { | |
| unsafe { TrapFrame::set_clone_trap_frame(self, parent_frame, kernel_sp, user_sp) } | |
| } | |
| unsafe fn set_fork_trap_frame(&mut self, parent_frame: &Self) { | |
| unsafe { TrapFrame::set_fork_trap_frame(self, parent_frame) } | |
| } | |
| fn set_clone_trap_frame( | |
| &mut self, | |
| parent_frame: &Self, | |
| kernel_sp: usize, | |
| user_sp: usize, | |
| ) { | |
| unsafe { TrapFrame::set_clone_trap_frame(self, parent_frame, kernel_sp, user_sp) } | |
| } | |
| fn set_fork_trap_frame(&mut self, parent_frame: &Self) { | |
| unsafe { TrapFrame::set_fork_trap_frame(self, parent_frame) } | |
| } |
| unsafe fn set_clone_trap_frame( | ||
| &mut self, | ||
| parent_frame: &Self, | ||
| kernel_sp: usize, | ||
| user_sp: usize, | ||
| ) { | ||
| unsafe { TrapFrame::set_clone_trap_frame(self, parent_frame, kernel_sp, user_sp) } | ||
| } | ||
|
|
||
| unsafe fn set_fork_trap_frame(&mut self, parent_frame: &Self) { | ||
| unsafe { TrapFrame::set_fork_trap_frame(self, parent_frame) } | ||
| } |
There was a problem hiding this comment.
Implement the safe version of set_clone_trap_frame and set_fork_trap_frame for TrapFrame under the HwTrapFrame trait.
| unsafe fn set_clone_trap_frame( | |
| &mut self, | |
| parent_frame: &Self, | |
| kernel_sp: usize, | |
| user_sp: usize, | |
| ) { | |
| unsafe { TrapFrame::set_clone_trap_frame(self, parent_frame, kernel_sp, user_sp) } | |
| } | |
| unsafe fn set_fork_trap_frame(&mut self, parent_frame: &Self) { | |
| unsafe { TrapFrame::set_fork_trap_frame(self, parent_frame) } | |
| } | |
| fn set_clone_trap_frame( | |
| &mut self, | |
| parent_frame: &Self, | |
| kernel_sp: usize, | |
| user_sp: usize, | |
| ) { | |
| unsafe { TrapFrame::set_clone_trap_frame(self, parent_frame, kernel_sp, user_sp) } | |
| } | |
| fn set_fork_trap_frame(&mut self, parent_frame: &Self) { | |
| unsafe { TrapFrame::set_fork_trap_frame(self, parent_frame) } | |
| } |
1 participant
This pull request introduces a new unified hardware trap frame abstraction (
HwTrapFrame) for the architecture layer, and refactors related code to use this trait. It also updates the console output and interrupt handling interfaces for better type safety and consistency, and adds various utility functions to thearchmodule. Additionally, the patch improves platform-specific code by using architecture constants and modernizes logging macros.The most important changes are:
Architecture Abstraction
HwTrapFrametrait inarch.rsto provide a unified and extensible interface for hardware trap frames across architectures. TheArchtrait now includes an associatedTrapFrametype implementingHwTrapFrame. Implementations for both LoongArch and mock architectures are provided. [1] [2] [3] [4] [5]TrapFrameand re-exports for architecture modules inarch/mod.rs, simplifying cross-module usage. [1] [2]Platform and Console Output
console_putcharto accept au8(byte) argument instead ofusizefor type safety, and updated all platform implementations accordingly. [1] [2] [3] [4]DMW0_BASEconstant for LoongArch, improving maintainability and clarity. [1] [2] [3] [4]Interrupt and Trap Handling Utilities
archmodule for interrupt state management and trap frame operations, such asread_and_disable_interrupts,restore_interrupts,are_interrupts_enabled,restore_trap_frame,init_kernel_trap_frame, and others. [1] [2]Logging and Output
earlyprintln!with the standardprintln!macro or a newemergency_println!macro for consistent logging, and replaced direct calls in trap handlers and boot code. [1] [2] [3] [4] [5] [6] [7]Code Cleanups
These changes improve the modularity, maintainability, and type safety of the architecture layer, and lay groundwork for future architecture support.
closes #242 #243 #247