Skip to content

refactor: 清理跨模块架构常量访问#254

Merged
ZIYAN137 merged 1 commit into
mainfrom
refactor/cleanup-cross-module-arch-constants
May 24, 2026
Merged

refactor: 清理跨模块架构常量访问#254
ZIYAN137 merged 1 commit into
mainfrom
refactor/cleanup-cross-module-arch-constants

Conversation

@ZIYAN137

Copy link
Copy Markdown
Contributor

变更内容

本 PR 处理 #250,主要收敛通用代码对架构常量的跨模块直接访问,减少 arch::constant 泄漏到 common code。

  • VirtualMemory 中新增架构无关的 USER_TOP 关联常量
  • config.rs 通过 VirtualMemory::USER_TOP 计算用户栈布局,并在 common config 中明确保留 guard page
  • user_buffer.rs 和用户拷贝校验逻辑改为通过 VirtualMemory::USER_TOP 做用户地址范围检查
  • PLIC 保留 SUPERVISOR_EXTERNAL 使用,但改为通过 crate::arch::SUPERVISOR_EXTERNAL 短路径访问
  • align_down 改为 const fn,用于编译期计算 sigreturn trampoline 地址

验证情况

  • cargo check --target riscv64gc-unknown-none-elf
  • cargo fmt --all -- --check
  • cargo clippy --target riscv64gc-unknown-none-elf
  • make run TEST=1

make run TEST=1 结果:Total: 510, Passed: 510, Failed: 0

Closes #250

@ZIYAN137 ZIYAN137 requested a review from miiyakumo May 24, 2026 10:36

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors architectural constants by integrating USER_TOP into the VirtualMemory trait and updating memory layout, user buffer validation, and interrupt management to use this abstracted constant. Review feedback identifies several critical improvements, including ensuring USER_STACK_TOP is page-aligned, fixing off-by-one errors in user space boundary checks, and preventing potential overflows in address calculations. Additionally, it is recommended to further abstract USER_BASE into the trait to maintain architectural consistency across the codebase.

Comment thread os/src/config.rs

pub const USER_STACK_TOP: usize = SV39_BOT_HALF_TOP - PAGE_SIZE; // leave another guard page
/// Leave one guard page below the top of the user address space.
pub const USER_STACK_TOP: usize = <ArchImpl as VirtualMemory>::USER_TOP - PAGE_SIZE;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current calculation USER_TOP - PAGE_SIZE results in a non-page-aligned address (e.g., 0x...EFFF if USER_TOP is 0x...FFFF). This is unconventional for a stack top and may lead to alignment issues or unexpected behavior in memory mapping. It should typically be page-aligned. If the intention is to place the stack immediately below the trampoline page, it should be aligned to the page boundary.

Suggested change
pub const USER_STACK_TOP: usize = <ArchImpl as VirtualMemory>::USER_TOP - PAGE_SIZE;
pub const USER_STACK_TOP: usize = align_down(<ArchImpl as VirtualMemory>::USER_TOP, PAGE_SIZE);

return false;
};
end <= USER_TOP
end <= <ArchImpl as VirtualMemory>::USER_TOP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The check end <= USER_TOP is too strict. Since USER_TOP is defined as the "highest accessible address" (inclusive), a buffer that ends exactly at USER_TOP + 1 (meaning its last byte is at USER_TOP) should be considered valid. The current logic excludes the last byte of the user address space.

        if self.len == 0 {
            return start <= <ArchImpl as VirtualMemory>::USER_TOP;
        }
        end - 1 <= <ArchImpl as VirtualMemory>::USER_TOP

Comment on lines +169 to +170
/// 用户地址空间最高可访问地址
const USER_TOP: usize;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To fully achieve the goal of converging architecture constants and reducing leakage of the constant module, consider adding USER_BASE to the VirtualMemory trait as well. Currently, constant::USER_BASE is still accessed directly in the impl_arch! macro. This ensures consistency with how other architectural constants like USER_TOP are handled.

References
  1. Prioritize consistency with existing architecture implementations when adding support for a new architecture.

Comment thread os/src/arch/arch_impl.rs
type KernelAddressSpace = $kernel_space;

const PAGE_OFFSET: usize = mm::VADDR_START;
const USER_TOP: usize = constant::USER_TOP;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If USER_BASE is added to the VirtualMemory trait, it should be implemented here in the macro to maintain consistency with USER_TOP and existing architecture implementations.

Suggested change
const USER_TOP: usize = constant::USER_TOP;
const USER_BASE: usize = constant::USER_BASE;
const USER_TOP: usize = constant::USER_TOP;
References
  1. Prioritize consistency with existing architecture implementations when adding support for a new architecture.

// 检查是否会溢出用户空间
if let Some(end_addr) = addr.checked_add(size) {
if end_addr > USER_TOP + 1 {
if end_addr > <ArchImpl as VirtualMemory>::USER_TOP + 1 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using ::USER_TOP + 1 can potentially overflow if USER_TOP is usize::MAX. While unlikely on current supported architectures, it is safer and more consistent with the logic in arch_impl.rs to use a subtraction-based check or handle the size > 0 case explicitly.

Suggested change
if end_addr > <ArchImpl as VirtualMemory>::USER_TOP + 1 {
if size > 0 && end_addr - 1 > <ArchImpl as VirtualMemory>::USER_TOP {

@ZIYAN137 ZIYAN137 merged commit 2251b4c into main May 24, 2026
1 check passed
@ZIYAN137 ZIYAN137 deleted the refactor/cleanup-cross-module-arch-constants branch May 24, 2026 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

跨模块架构常量访问清理

1 participant