Skip to content

Guard the lower 1MB of memory #317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
10 changes: 10 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ members = [
"tests/test_kernels/higher_half",
"tests/test_kernels/pie",
"tests/test_kernels/lto",
"tests/test_kernels/ramdisk"
"tests/test_kernels/ramdisk",
"tests/test_kernels/lower_memory_free"
]
exclude = ["examples/basic", "examples/test_framework"]

Expand Down Expand Up @@ -57,6 +58,7 @@ test_kernel_higher_half = { path = "tests/test_kernels/higher_half", artifact =
test_kernel_map_phys_mem = { path = "tests/test_kernels/map_phys_mem", artifact = "bin", target = "x86_64-unknown-none" }
test_kernel_pie = { path = "tests/test_kernels/pie", artifact = "bin", target = "x86_64-unknown-none" }
test_kernel_ramdisk = { path = "tests/test_kernels/ramdisk", artifact = "bin", target = "x86_64-unknown-none" }
test_kernel_lower_memory_free = { path = "tests/test_kernels/lower_memory_free", artifact = "bin", target = "x86_64-unknown-none" }

[profile.dev]
panic = "abort"
Expand Down
4 changes: 2 additions & 2 deletions api/src/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,12 @@ impl MemoryRegion {
#[non_exhaustive]
#[repr(C)]
pub enum MemoryRegionKind {
/// Unused conventional memory, can be used by the kernel.
Usable,
/// Memory mappings created by the bootloader, including the page table and boot info mappings.
///
/// This memory should _not_ be used by the kernel.
Bootloader,
/// Unused conventional memory, can be used by the kernel.
Usable,
Copy link
Member

Choose a reason for hiding this comment

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

Reordering the variants changes their integer value, which is technically a breaking change. So we should undo this change if possible.

/// An unknown memory region reported by the UEFI firmware.
///
/// Contains the UEFI memory type tag.
Expand Down
29 changes: 24 additions & 5 deletions common/src/legacy_memory_region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,11 @@ pub struct LegacyFrameAllocator<I, D> {
memory_map: I,
current_descriptor: Option<D>,
next_frame: PhysFrame,
start_frame: PhysFrame,
Copy link
Member

Choose a reason for hiding this comment

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

We already use the name start_frame in multiple places, so we should name this field differently to avoid confusion. I think min_frame would be a fitting name:

Suggested change
start_frame: PhysFrame,
min_frame: PhysFrame,

}

const LOWER_MEMORY_END_PAGE: u64 = 0x10_0000;

impl<I, D> LegacyFrameAllocator<I, D>
where
I: ExactSizeIterator<Item = D> + Clone,
Expand All @@ -42,18 +45,32 @@ where
/// library assumes that references can never point to virtual address `0`.
pub fn new(memory_map: I) -> Self {
// skip frame 0 because the rust core library does not see 0 as a valid address
let start_frame = PhysFrame::containing_address(PhysAddr::new(0x1000));
Self::new_starting_at(start_frame, memory_map)
// also skip the first 1MB of frames, there are use cases that require lower conventional memory access. (Such as SMP SIPI)
let start_frame = PhysFrame::containing_address(PhysAddr::new(LOWER_MEMORY_END_PAGE));
unsafe { Self::unsafe_new_starting_at(start_frame, memory_map) }
Comment on lines +48 to +50
Copy link
Member

Choose a reason for hiding this comment

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

With the suggested changes below, we could keep the previous implementation:

Suggested change
// also skip the first 1MB of frames, there are use cases that require lower conventional memory access. (Such as SMP SIPI)
let start_frame = PhysFrame::containing_address(PhysAddr::new(LOWER_MEMORY_END_PAGE));
unsafe { Self::unsafe_new_starting_at(start_frame, memory_map) }
let start_frame = PhysFrame::containing_address(PhysAddr::new(0x1000));
Self::new_starting_at(start_frame, memory_map)

}

/// Creates a new frame allocator based on the given legacy memory regions. Skips any frames
/// before the given `frame`.
/// before the given `frame`, or 0x100000, whichever is higher.
pub fn new_starting_at(frame: PhysFrame, memory_map: I) -> Self {
if frame.start_address().as_u64() < LOWER_MEMORY_END_PAGE {
// Skip the first 1MB of frames, regardless of what was requested.
// there are use cases that require lower conventional memory access. (Such as SMP SIPI)
return Self::new(memory_map);
}

return unsafe { Self::unsafe_new_starting_at(frame, memory_map) };
}

/// Creates a new frame allocator based on the given legacy memory regions. Skips any frames
/// before the given `frame`, without attempting to protect the lower 1MB of memory.
pub unsafe fn unsafe_new_starting_at(frame: PhysFrame, memory_map: I) -> Self {
Comment on lines +56 to +67
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit complicated. How about we just use a max call in new_starting_at and keep calling it from the new function?

Suggested change
if frame.start_address().as_u64() < LOWER_MEMORY_END_PAGE {
// Skip the first 1MB of frames, regardless of what was requested.
// there are use cases that require lower conventional memory access. (Such as SMP SIPI)
return Self::new(memory_map);
}
return unsafe { Self::unsafe_new_starting_at(frame, memory_map) };
}
/// Creates a new frame allocator based on the given legacy memory regions. Skips any frames
/// before the given `frame`, without attempting to protect the lower 1MB of memory.
pub unsafe fn unsafe_new_starting_at(frame: PhysFrame, memory_map: I) -> Self {
// Skip the first 1MB of frames, regardless of what was requested.
// there are use cases that require lower conventional memory access. (Such as SMP SIPI)
let lower_mem_end = PhysFrame::containing_address(PhysAddr::new(LOWER_MEMORY_END_PAGE));
let frame = core::cmp::max(frame, lower_mem_end);

Self {
original: memory_map.clone(),
memory_map,
current_descriptor: None,
next_frame: frame,
start_frame: frame,
}
}

Expand Down Expand Up @@ -121,9 +138,11 @@ where
let next_free = self.next_frame.start_address();
let kind = match descriptor.kind() {
MemoryRegionKind::Usable => {
if end <= next_free {
if end <= next_free && end > self.start_frame.start_address() {
MemoryRegionKind::Bootloader
} else if descriptor.start() >= next_free {
} else if descriptor.start() >= next_free
|| end <= self.start_frame.start_address()
{
Comment on lines +143 to +145
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to split this condition so that we can add a comment:

Suggested change
} else if descriptor.start() >= next_free
|| end <= self.start_frame.start_address()
{
} else if descriptor.start() >= next_free {
MemoryRegionKind::Usable
} else if end <= self.start_frame.start_address() {
// treat regions that end before the minimum frame as usable too

MemoryRegionKind::Usable
} else {
// part of the region is used -> add it separately
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to update this else branch too, in case the descriptor goes beyond 0x100000.

Expand Down
7 changes: 7 additions & 0 deletions tests/lower_memory_free.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
use bootloader_test_runner::run_test_kernel;
#[test]
fn lower_memory_free() {
run_test_kernel(env!(
"CARGO_BIN_FILE_TEST_KERNEL_LOWER_MEMORY_FREE_lower_memory_free"
));
}
13 changes: 13 additions & 0 deletions tests/test_kernels/lower_memory_free/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
[package]
name = "test_kernel_lower_memory_free"
version = "0.1.0"
authors = ["Philipp Oppermann <[email protected]>"]
edition = "2021"

[dependencies]
bootloader_api = { path = "../../../api" }
x86_64 = { version = "0.14.7", default-features = false, features = [
"instructions",
"inline_asm",
] }
uart_16550 = "0.2.10"
44 changes: 44 additions & 0 deletions tests/test_kernels/lower_memory_free/src/bin/lower_memory_free.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#![no_std] // don't link the Rust standard library
#![no_main] // disable all Rust-level entry points

use bootloader_api::{entry_point, info::MemoryRegionKind, BootInfo};
use test_kernel_lower_memory_free::{exit_qemu, QemuExitCode};

const LOWER_MEMORY_END_PAGE: u64 = 0x0010_0000;

entry_point!(kernel_main);

fn kernel_main(boot_info: &'static mut BootInfo) -> ! {
use core::fmt::Write;
use test_kernel_lower_memory_free::serial;

let mut count = 0;
for region in boot_info.memory_regions.iter() {
writeln!(
serial(),
"Region: {:016x}-{:016x} - {:?}",
region.start,
region.end,
region.kind
)
.unwrap();
if region.end <= LOWER_MEMORY_END_PAGE && region.kind == MemoryRegionKind::Usable {
let pages = (region.end - region.start) / 4096;
count += pages;
}
Comment on lines +25 to +28
Copy link
Member

Choose a reason for hiding this comment

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

We should also account for regions that don't end at LOWER_MEMORY_END_PAGE. For example:

Suggested change
if region.end <= LOWER_MEMORY_END_PAGE && region.kind == MemoryRegionKind::Usable {
let pages = (region.end - region.start) / 4096;
count += pages;
}
if region.kind == MemoryRegionKind::Usable && region.start < LOWER_MEMORY_END_PAGE {
let end = core::cmp::min(region.end, LOWER_MEMORY_END_PAGE);
let pages = (end - region.start) / 4096;
count += pages;
}

}

writeln!(serial(), "Free lower memory page count: {}", count).unwrap();
assert!(count > 0x10); // 0x20 chosen arbirarily, we need _some_ free conventional memory, but not all of it. Some, especially on BIOS, may be reserved for hardware.
exit_qemu(QemuExitCode::Success);
}

/// This function is called on panic.
#[panic_handler]
#[cfg(not(test))]
fn panic(info: &core::panic::PanicInfo) -> ! {
use core::fmt::Write;

let _ = writeln!(test_kernel_lower_memory_free::serial(), "PANIC: {}", info);
exit_qemu(QemuExitCode::Failed);
}
29 changes: 29 additions & 0 deletions tests/test_kernels/lower_memory_free/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#![no_std]

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[repr(u32)]
pub enum QemuExitCode {
Success = 0x10,
Failed = 0x11,
}

pub static RAMDISK_CONTENTS: &[u8] = include_bytes!("../../../ramdisk.txt");

pub fn exit_qemu(exit_code: QemuExitCode) -> ! {
use x86_64::instructions::{nop, port::Port};

unsafe {
let mut port = Port::new(0xf4);
port.write(exit_code as u32);
}

loop {
nop();
}
}

pub fn serial() -> uart_16550::SerialPort {
let mut port = unsafe { uart_16550::SerialPort::new(0x3F8) };
port.init();
port
}