Hal refactor#240
Merged
Merged
Conversation
…and RISC-V - Added `cpu_ops.rs` for LoongArch64 architecture, implementing `hal::CpuOps` trait with methods for CPU ID retrieval, interrupt management, and CPU halting. - Added `cpu_ops.rs` for RISC-V architecture, implementing `hal::CpuOps` trait with similar functionalities. - Introduced `address.rs` for type-safe address abstraction, supporting physical, virtual, and user addresses with associated methods. - Created `arch.rs` to define the top-level architecture abstraction, combining CPU operations and virtual memory management. - Established `cpu_ops.rs` as the foundational trait for CPU operations, ensuring minimal dependencies for architecture portability. - Developed `mock.rs` to provide mock implementations for testing, facilitating architecture-agnostic code testing. - Added `virtual_memory.rs` to define traits for virtual memory management, including user and kernel address space operations.
- Introduced a new module `inner.rs` defining the `PageTableInner` trait for page table operations. - Updated `mod.rs` to use the new `inner` module instead of a self-referential structure. fix(sync): specify types for PerCpu, RwLock, and TicketLock - Explicitly defined types for `PerCpu`, `RwLock`, and `TicketLock` instances in test cases to improve type safety and clarity.
… LoongArch architectures - Added `SyscallFrame` trait to unify syscall argument access across architectures. - Implemented `dispatch_syscall` function to handle syscall dispatching in an architecture-agnostic manner. - Updated `TrapFrame` to implement `SyscallFrame` for RISC-V. - Created a new module for syscall dispatching and updated syscall handling in the kernel. - Introduced early printing functions and macros for kernel initialization. - Added LoongArch platform support with MMIO for console I/O and power management. - Implemented pure Rust versions of `memcpy`, `memmove`, and `memset` for LoongArch due to incomplete compiler support. - Updated various syscall handling functions to utilize the new dispatch mechanism.
…ory access - Removed reliance on SumGuard for user space access in syscalls, replacing it with Arch trait methods for copying data to and from user space. - Updated syslog implementation to utilize `copy_to_user` and `copy_from_user` for safer memory operations. - Refactored task management syscalls to eliminate SumGuard usage, enhancing clarity and safety. - Improved user buffer handling in utility functions, ensuring proper memory access without architecture-specific guards. - Adjusted socket and ioctl implementations to follow the new user space access patterns, ensuring consistency across the codebase. - Added error handling for user space memory operations to prevent potential crashes or undefined behavior.
…oongArch and RISC-V
- Updated `exec_loader.rs` to conditionally set `value` for non-RISC-V architectures. - Modified `space.rs` and `mod.rs` to import `MEMORY_END` from the new mock platform module. - Enhanced `intr_guard.rs` to utilize CPU-specific interrupt status checks. - Updated synchronization module to conditionally compile mutex features. - Removed unused test module `test.rs`. - Introduced `mock_stubs.rs` to provide mock implementations for architecture-specific functions, enabling compilation and testing on non-RISC-V platforms.
…t methods - Implemented `on_task_switch`, `get_ticks`, `get_time`, `get_time_ms`, `clock_freq`, and `send_reschedule_ipi` methods in the Arch trait for both LoongArch and RISC-V architectures. - Updated the architecture-specific modules to utilize the new methods, ensuring better abstraction and code reuse. - Refactored existing code to replace direct calls to architecture-specific functions with the new trait methods, improving maintainability and readability.
- Updated `Cargo.toml` to reorganize dependencies into mandatory and optional categories, enhancing modularity. - Modified `build.rs` to conditionally compile user programs based on target architecture (RISC-V and LoongArch). - Enhanced console output functions to support device features conditionally. - Cleaned up CPU task management code by removing redundant comments. - Introduced feature gates for process-related modules in the task management system. - Updated main module to reflect multi-architecture support and reorganized module imports. - Moved frame allocator implementation to a new file for better organization and clarity. - Removed the global allocator module and replaced it with a new `talc_alloc` module for dynamic heap memory allocation. - Ensured that heap initialization is only called when the allocation feature is enabled.
… initialization, and syscall definitions - Implement SBI calls for console I/O, timer management, and system shutdown in `lib.rs`. - Create memory management traits for user and kernel address spaces in `memory.rs`. - Initialize Virt platform devices in `platform.rs`. - Define RISC-V syscall numbers in `syscall.rs` following Linux conventions.
- Removed unused imports from `mapping_area.rs`, `space.rs`, and `mod.rs`. - Introduced a new `arch_impl.rs` file to provide common architecture implementations for `VirtualMemory` and `Arch`. - Added `memory_impl.rs` to define `UserAddressSpace` and `KernAddressSpace` implementations for different architectures. - Created mock architecture files under `mock/` directory to facilitate testing when not targeting RISC-V or LoongArch. - Implemented basic mock functions for boot, interrupts, timers, and traps to support architecture-agnostic code compilation. - Defined Linux syscall numbers in `syscall.rs` for consistent ABI across architectures.
- Introduced `Arch` trait as a top-level architecture abstraction combining `CpuOps` and `VirtualMemory`. - Created `CpuOps` trait for low-level CPU operations, facilitating architecture portability. - Defined `VirtualMemory`, `UserAddressSpace`, and `KernAddressSpace` traits for memory management. - Updated all architecture implementations (RISC-V, LoongArch) to use the new trait structure. - Implemented a mock architecture for testing purposes, ensuring compatibility with non-target architectures. - Refactored imports across the codebase to align with the new architecture module structure. - Removed the old HAL module structure, consolidating architecture-related traits into the `arch` module.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces multi-architecture support by abstracting CPU operations and memory management through new traits (Arch, CpuOps, VirtualMemory). It replaces architecture-specific SumGuard mechanisms with a unified copy_from_user/copy_to_user interface. However, the review identified critical issues where the return values of these copy operations are ignored, leading to potential undefined behavior or silent failures when accessing user-space memory. These issues must be addressed to ensure memory safety.
Comment on lines
18
to
25
| unsafe { | ||
| ptr::write_volatile(user_ptr, value); | ||
| crate::arch::ArchImpl::copy_to_user( | ||
| (&value) as *const T as *const u8, | ||
| user_ptr as usize, | ||
| size, | ||
| ) | ||
| .ok(); | ||
| } |
Contributor
There was a problem hiding this comment.
The result of copy_to_user is ignored in write_to_user. If the copy fails, the function should return an error, but it currently silently fails.
Comment on lines
+36
to
+44
| unsafe { | ||
| crate::arch::ArchImpl::copy_from_user( | ||
| user_ptr as usize, | ||
| val.as_mut_ptr() as *mut u8, | ||
| size, | ||
| ) | ||
| .ok(); | ||
| val.assume_init() | ||
| } |
Contributor
There was a problem hiding this comment.
The read_from_user function ignores the result of copy_from_user and calls val.assume_init() on uninitialized memory if the copy fails, which is undefined behavior. The function should return a Result<T, ()> and callers should handle the error.
Comment on lines
+21
to
+28
| unsafe { | ||
| crate::arch::ArchImpl::copy_from_user( | ||
| buf as usize, | ||
| kernel_buf.as_mut_ptr(), | ||
| count, | ||
| ) | ||
| .ok(); | ||
| } |
Contributor
There was a problem hiding this comment.
The result of copy_from_user is ignored in the write function. If the copy fails, the kernel proceeds with a buffer of zeros, which is incorrect. The function should return EFAULT if the copy fails.
Comment on lines
+66
to
+73
| unsafe { | ||
| crate::arch::ArchImpl::copy_to_user( | ||
| kernel_buf.as_ptr(), | ||
| buf as usize, | ||
| n, | ||
| ) | ||
| .ok(); | ||
| } |
Contributor
There was a problem hiding this comment.
The result of copy_to_user is ignored in the read function. If the copy fails, the kernel should return EFAULT instead of proceeding.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a significant refactor and modularization of the architecture abstraction layer in the kernel. It establishes a more type-safe, extensible, and portable foundation for supporting multiple CPU architectures. The changes include the introduction of new traits for CPU operations and address management, a macro for implementing architecture-specific methods, and a more modular dependency and feature structure.
The most important changes are:
Architecture Abstraction and Address Safety:
arch/address.rsmodule that introduces a type-safe, generic address abstraction (Address<K, T>) with sealed traits for memory kinds (Physical, Virtual, User), preventing address space confusion at compile time and providing safe/unsafe access methods tailored to each address type. Also introduces anAddressTranslatortrait for controlled address space conversions.arch/arch.rstrait, which unifies CPU operations, virtual memory, process management, user/kernel memory copying, system information, and power management into a single, extensible interface for architecture-specific logic.arch/cpu_ops.rstrait, abstracting the minimal set of CPU operations (interrupt control, halt, core ID) to maximize portability and testability of core modules.arch/arch_impl.rs) to generate common implementations ofVirtualMemoryandArchfor different architectures, reducing code duplication and simplifying the process of adding new architectures.Build System and Dependency Management:
os/Cargo.tomlto make most dependencies optional and feature-gated, enabling a layered, modular build. The default feature set now includes common kernel subsystems, and dependencies are grouped by functionality (e.g., device, filesystem, networking).build.rs) to only build user programs and runtime images for supported target architectures, improving cross-compilation support and clarity of build warnings. [1] [2] [3].cargo/config.toml, clarifying known issues and proper usage for developers running tests on x86_64 hosts.Cargo.toml, aligning panic strategy across build modes.Miscellaneous:
These changes collectively lay a robust foundation for multi-architecture support, safer memory management, and a more maintainable kernel codebase.