Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions os/src/arch/arch_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ macro_rules! impl_arch {
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.


fn kern_address_space() -> &'static SpinLock<Self::KernelAddressSpace> {
&KERN_ADDR_SPACE
Expand Down Expand Up @@ -85,7 +86,7 @@ macro_rules! impl_arch {
max_len: usize,
) -> Result<usize, ()> {
let src = src.as_usize();
if src < constant::USER_BASE || src > constant::USER_TOP {
if !(constant::USER_BASE..=<$arch as VirtualMemory>::USER_TOP).contains(&src) {
return Err(());
}
if max_len != 0 && dst.is_null() {
Expand Down Expand Up @@ -143,12 +144,12 @@ macro_rules! impl_arch {
if len == 0 {
return Ok(());
}
if start < constant::USER_BASE || start > constant::USER_TOP {
if !(constant::USER_BASE..=<$arch as VirtualMemory>::USER_TOP).contains(&start) {
return Err(());
}
let end = start.checked_add(len).ok_or(())?;
let last = end.checked_sub(1).ok_or(())?;
if last > constant::USER_TOP {
if last > <$arch as VirtualMemory>::USER_TOP {
return Err(());
}

Expand Down
1 change: 1 addition & 0 deletions os/src/arch/mock/arch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ impl VirtualMemory for MockArch {
type ProcessAddressSpace = MockAddressSpace;
type KernelAddressSpace = MockAddressSpace;
const PAGE_OFFSET: usize = 0xffff_ffc0_0000_0000;
const USER_TOP: usize = super::constant::USER_TOP;

fn kern_address_space() -> &'static SpinLock<Self::KernelAddressSpace> {
static KERN_SPACE: SpinLock<MockAddressSpace> = SpinLock::new(MockAddressSpace::new());
Expand Down
2 changes: 2 additions & 0 deletions os/src/arch/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ mod mock;
#[cfg(not(any(target_arch = "riscv64", target_arch = "loongarch64")))]
pub use mock::*;

pub use constant::SUPERVISOR_EXTERNAL;

// ---- ArchImpl 类型别名 ----
// 内核其余部分通过 ArchImpl 访问架构特定功能,无需关心具体架构。
#[cfg(target_arch = "riscv64")]
Expand Down
3 changes: 3 additions & 0 deletions os/src/arch/virtual_memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ pub trait VirtualMemory: CpuOps + Sized {
/// 对于直接映射,`paddr + PAGE_OFFSET = vaddr`。
const PAGE_OFFSET: usize;

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

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.


/// 获取全局内核地址空间
fn kern_address_space() -> &'static SpinLock<Self::KernelAddressSpace>;
}
13 changes: 8 additions & 5 deletions os/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ pub const USER_STACK_SIZE: usize = 4 * 1024 * 1024; // 4MB

pub const MAX_ARGV: usize = 256;

use crate::arch::{ArchImpl, virtual_memory::VirtualMemory};
use crate::util::address::align_down;

// User space memory layout constants
// Memory layout (from high to low address):
// [USER_STACK] <--
Expand All @@ -20,13 +23,15 @@ pub const MAX_ARGV: usize = 256;
// [USER_DATA]
// [USER_TEXT]

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);


/// Userspace rt_sigreturn trampoline address (one RX page).
///
/// Must not overlap with the user stack mapping. With the current (non page-aligned) `USER_STACK_TOP`,
/// the stack mapping ends at `align_down(SV39_BOT_HALF_TOP, PAGE_SIZE)`, so we place the trampoline there.
pub const USER_SIGRETURN_TRAMPOLINE: usize = SV39_BOT_HALF_TOP & !(PAGE_SIZE - 1);
/// the stack mapping ends at `align_down(USER_TOP, PAGE_SIZE)`, so we place the trampoline there.
pub const USER_SIGRETURN_TRAMPOLINE: usize =
align_down(<ArchImpl as VirtualMemory>::USER_TOP, PAGE_SIZE);

/// Maximum heap size (prevent OOM)
pub const MAX_USER_HEAP_SIZE: usize = 64 * 1024 * 1024; // 64MB
Expand All @@ -40,5 +45,3 @@ pub const EXT4_BLOCK_SIZE: usize = 4096;
pub const VIRTIO_BLK_SECTOR_SIZE: usize = 512;
/// 文件系统镜像大小 (与 qemu-run.sh 中的 fs.img 大小一致)
pub const FS_IMAGE_SIZE: usize = 1024 * 1024 * 1024; // 1 GB

use crate::arch::constant::SV39_BOT_HALF_TOP;
2 changes: 1 addition & 1 deletion os/src/device/irq/plic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! PLIC 提供对外设中断的集中管理,支持优先级和中断分发功能。

use super::{super::DRIVERS, IrqManager};
use crate::arch::constant::SUPERVISOR_EXTERNAL;
use crate::arch::SUPERVISOR_EXTERNAL;
use crate::device::device_tree::{DEVICE_TREE_INTC, DEVICE_TREE_REGISTRY};
use crate::device::irq::IntcDriver;
use crate::device::{DeviceType, Driver, IRQ_MANAGER};
Expand Down
2 changes: 1 addition & 1 deletion os/src/util/address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

/// 这是一个安全且常见的 align_down 实现
/// T 必须是整数类型
pub fn align_down(addr: usize, align: usize) -> usize {
pub const fn align_down(addr: usize, align: usize) -> usize {
// 对齐值必须是 2 的幂,否则行为可能不正确
debug_assert!(align.is_power_of_two());

Expand Down
9 changes: 4 additions & 5 deletions os/src/util/user_buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@
use alloc::vec::Vec;
use core::mem::MaybeUninit;

use crate::arch::constant::USER_TOP;
use crate::arch::{Arch, address::UA};
use crate::arch::{Arch, ArchImpl, address::UA, virtual_memory::VirtualMemory};

/// 向用户空间写入数据
/// # 参数
Expand Down Expand Up @@ -104,7 +103,7 @@ impl UserBuffer {
let Some(end) = start.checked_add(self.len) else {
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

}

/// 返回用户缓冲区长度
Expand Down Expand Up @@ -147,13 +146,13 @@ pub fn validate_user_ptr<T>(ptr: *const T) -> bool {

// 检查起始地址是否在用户空间范围内
// USER_BASE 为 0,所以只需检查上界
if addr > USER_TOP {
if addr > <ArchImpl as VirtualMemory>::USER_TOP {
return false;
}

// 检查是否会溢出用户空间
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 {

return false;
}
} else {
Expand Down
Loading