Skip to content

Make PhysicalMapping safely Send / Sync #361

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.

Sign up for GitHub

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

Merged
IsaacWoods merged 3 commits into from
Oct 4, 2026
Merged

Make PhysicalMapping safely Send / Sync #361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d719a23
Refactor PhysicalMapping to its own submodule
martin-hughes Sep 19, 2026
fc32783
Disallow direct construction of mappings
martin-hughes Sep 19, 2026
3d8835d
Address review feedback
martin-hughes Oct 3, 2026
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
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ lock_api = "0.4.14"
parking_lot = "0.12.5"
pretty_env_logger = "0.5.0"
serial_test = "4.0.1"
static_assertions = "1.1.0"

[features]
default = ["alloc", "aml"]
Expand Down
37 changes: 17 additions & 20 deletions src/address.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,10 @@ where
AddressSpace::SystemMemory => {
let mapping = self.mapping.as_ref().unwrap();
match access_size_bits {
8 => Ok(unsafe { ptr::read_volatile(mapping.raw.virtual_start.as_ptr() as *const u8) as u64 }),
16 => {
Ok(unsafe { ptr::read_volatile(mapping.raw.virtual_start.as_ptr() as *const u16) as u64 })
}
32 => {
Ok(unsafe { ptr::read_volatile(mapping.raw.virtual_start.as_ptr() as *const u32) as u64 })
}
64 => Ok(unsafe { ptr::read_volatile(mapping.raw.virtual_start.as_ptr() as *const u64) }),
8 => Ok(unsafe { ptr::read_volatile(mapping.as_ptr()) as u64 }),
16 => Ok(unsafe { ptr::read_volatile(mapping.as_ptr() as *const u16) as u64 }),
32 => Ok(unsafe { ptr::read_volatile(mapping.as_ptr() as *const u32) as u64 }),
64 => Ok(unsafe { ptr::read_volatile(mapping.as_ptr() as *const u64) }),
_ => Err(AcpiError::InvalidGenericAddress),
}
}
Expand All @@ -191,17 +187,18 @@ where
match self.gas.address_space {
AddressSpace::SystemMemory => {
let mapping = self.mapping.as_ref().unwrap();
// Safety: The following writes are all to memory outside the Rust allocation system
match access_size_bits {
8 => unsafe {
ptr::write_volatile(mapping.raw.virtual_start.as_ptr(), value as u8);
ptr::write_volatile(mapping.as_mut_ptr(), value as u8);
},
16 => unsafe {
ptr::write_volatile(mapping.raw.virtual_start.as_ptr() as *mut u16, value as u16);
ptr::write_volatile(mapping.as_mut_ptr() as *mut u16, value as u16);
},
32 => unsafe {
ptr::write_volatile(mapping.raw.virtual_start.as_ptr() as *mut u32, value as u32);
ptr::write_volatile(mapping.as_mut_ptr() as *mut u32, value as u32);
},
64 => unsafe { ptr::write_volatile(mapping.raw.virtual_start.as_ptr() as *mut u64, value) },
64 => unsafe { ptr::write_volatile(mapping.as_mut_ptr() as *mut u64, value) },
_ => return Err(AcpiError::InvalidGenericAddress),
}
Ok(())
Expand All @@ -225,10 +222,10 @@ where
/// Ignores the GAS access size and does a 16-bit read.
pub fn read_u16(&self, byte_offset: u64) -> u16 {
match self.gas.address_space {
AddressSpace::SystemMemory => {
let addr = self.mapping.as_ref().unwrap().raw.virtual_start.cast::<u16>();
unsafe { addr.byte_offset(byte_offset as isize).read_unaligned() }
}
AddressSpace::SystemMemory => unsafe {
let addr = self.mapping.as_ref().unwrap().as_mut_ptr().cast::<u16>();
addr.byte_offset(byte_offset as isize).read_unaligned()
},
AddressSpace::SystemIo => self.handler.read_io_u16(self.gas.address as u16 + byte_offset as u16),
address_space => todo!("{address_space:?}"),
}
Expand All @@ -237,10 +234,10 @@ where
/// Ignores the GAS access size and does a 16-bit write.
pub fn write_u16(&self, byte_offset: u64, value: u16) {
match self.gas.address_space {
AddressSpace::SystemMemory => {
let addr = self.mapping.as_ref().unwrap().raw.virtual_start.cast::<u16>();
unsafe { addr.byte_offset(byte_offset as isize).write_unaligned(value) }
}
AddressSpace::SystemMemory => unsafe {
let addr = self.mapping.as_ref().unwrap().as_mut_ptr().cast::<u16>();
addr.byte_offset(byte_offset as isize).write_unaligned(value)
},
AddressSpace::SystemIo => {
self.handler.write_io_u16(self.gas.address as u16 + byte_offset as u16, value)
}
Expand Down
3 changes: 2 additions & 1 deletion src/aml/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ use alloc::{
use bit_field::BitField;
use core::{
mem,
ptr,
slice,
str::FromStr,
sync::atomic::{AtomicU64, Ordering},
Expand Down Expand Up @@ -204,7 +205,7 @@ where
};
let stream = unsafe {
slice::from_raw_parts(
mapping.raw.virtual_start.as_ptr().byte_add(mem::size_of::<SdtHeader>()) as *const u8,
ptr::from_ref(&*mapping).byte_add(size_of::<SdtHeader>()) as *const u8,

@IsaacWoods IsaacWoods Oct 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Applies throughout: I get why the pattern here works (Deref to create a reference to the underlying and then turn that into a pointer) but I wonder if unsafe helpers to create pointers directly as_ptr and as_mut would be clearer?

@martin-hughes martin-hughes Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perhaps as_mut_ptr to distinguish it from the existing as_mut from trait AsMut that takes &mut self and returns a mut ref?

(Agree otherwise though, seems sensible)

table.length as usize - mem::size_of::<SdtHeader>(),
)
};
Expand Down
138 changes: 9 additions & 129 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,23 +55,19 @@ extern crate alloc;
pub mod address;
#[cfg(feature = "aml")]
pub mod aml;
pub mod physical_mapping;
#[cfg(feature = "alloc")]
pub mod platform;
pub mod registers;
pub mod rsdp;
pub mod sdt;

pub use pci_types::PciAddress;
pub use physical_mapping::{PhysicalMapping, RawPhysicalMapping};
pub use sdt::{fadt::PowerProfile, hpet::HpetInfo, madt::MadtError};

use crate::sdt::{SdtHeader, Signature};
use core::{
fmt,
mem,
ops::{Deref, DerefMut},
pin::Pin,
ptr::NonNull,
};
use core::mem;
use log::warn;
use rsdp::Rsdp;

Expand Down Expand Up @@ -169,9 +165,8 @@ where
rsdt_entry_size: usize,
quirks: AcpiQuirks,
) -> Result<AcpiTables<H>, AcpiError> {
let rsdt_mapping = unsafe {
PhysicalMapping::<_, SdtHeader>::new(rsdt_address, mem::size_of::<SdtHeader>(), handler.clone())
};
let rsdt_mapping =
unsafe { PhysicalMapping::<_, SdtHeader>::new(rsdt_address, size_of::<SdtHeader>(), handler.clone()) };

let rsdt_length = rsdt_mapping.length;
let rsdt_mapping =
Expand All @@ -182,10 +177,10 @@ where
/// Iterate over the **physical** addresses of the SDTs.
pub fn table_entries(&self) -> impl Iterator<Item = usize> {
let mut table_entries_ptr =
unsafe { self.rsdt_mapping.raw.virtual_start.as_ptr().byte_add(mem::size_of::<SdtHeader>()) }
.cast::<u8>();
let mut num_entries = (self.rsdt_mapping.raw.region_length.saturating_sub(mem::size_of::<SdtHeader>()))
/ self.rsdt_entry_size;
unsafe { self.rsdt_mapping.as_ptr().byte_add(size_of::<SdtHeader>()) }.cast::<u8>();
let mut num_entries =
(self.rsdt_mapping.raw().region_length().saturating_sub(mem::size_of::<SdtHeader>()))
/ self.rsdt_entry_size;

core::iter::from_fn(move || {
if num_entries > 0 {
Expand Down Expand Up @@ -347,119 +342,6 @@ pub struct AcpiQuirks {
pub ignore_xsdt: bool,
}

/// Describes a physical mapping.
///
/// The region mapped must be at least `size_of::<T>()` bytes, but may be bigger.
#[derive(Debug)]
pub struct RawPhysicalMapping<T: ?Sized> {
/// The physical address of the mapped structure. The actual mapping may start at a lower address
/// if the requested physical address is not well-aligned.
pub physical_start: usize,
/// The virtual address of the mapped structure. It must be a valid, non-null pointer to the
/// start of the requested structure. The actual virtual mapping may start at a lower address
/// if the requested address is not well-aligned.
pub virtual_start: NonNull<T>,
/// The size of the requested region, in bytes. Can be equal or larger to `size_of::<T>()`. If a
/// larger region has been mapped, this should still be the requested size.
pub region_length: usize,
/// The total size of the produced mapping. This may be the same as `region_length`, or larger to
/// meet requirements of the mapping implementation.
pub mapped_length: usize,
}

impl<T: ?Sized> Clone for RawPhysicalMapping<T> {
fn clone(&self) -> Self {
*self
}
}

impl<T: ?Sized> Copy for RawPhysicalMapping<T> {}

/// Describes a physical mapping created by [`Handler::map_physical_region`] and unmapped by
/// [`Handler::unmap_physical_region`]. The region mapped must be at least `size_of::<T>()`
/// bytes, but may be bigger.
pub struct PhysicalMapping<H, T>
where
H: Handler,
{
pub raw: RawPhysicalMapping<T>,
/// The [`Handler`] that was used to produce the mapping. When this mapping is dropped, this
/// handler will be used to unmap the region.
pub handler: H,
}

impl<H, T> PhysicalMapping<H, T>
where
H: Handler,
{
/// Creates a new physical mapping from the given handler.
///
/// # Safety
///
/// - `physical_address` must point to a valid `T` in physical memory.
/// - `size` must be at least `size_of::<T>()`.
pub unsafe fn new(physical_address: usize, size: usize, handler: H) -> PhysicalMapping<H, T> {
let raw = unsafe { handler.map_physical_region(physical_address, size) };
PhysicalMapping { raw, handler }
}

/// Get a pinned reference to the inner `T`. This is generally only useful if `T` is `!Unpin`,
/// otherwise the mapping can simply be dereferenced to access the inner type.
pub fn get(&self) -> Pin<&T> {
unsafe { Pin::new_unchecked(self.raw.virtual_start.as_ref()) }
}
}

impl<H, T> fmt::Debug for PhysicalMapping<H, T>
where
H: Handler,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("PhysicalMapping")
.field("physical_start", &self.raw.physical_start)
.field("virtual_start", &self.raw.virtual_start)
.field("region_length", &self.raw.region_length)
.field("mapped_length", &self.raw.mapped_length)
.field("handler", &())
.finish()
}
}

unsafe impl<H: Handler + Send, T: Send> Send for PhysicalMapping<H, T> {}

impl<H, T> Deref for PhysicalMapping<H, T>
where
T: Unpin,
H: Handler,
{
type Target = T;

fn deref(&self) -> &T {
unsafe { self.raw.virtual_start.as_ref() }
}
}

impl<H, T> DerefMut for PhysicalMapping<H, T>
where
T: Unpin,
H: Handler,
{
fn deref_mut(&mut self) -> &mut T {
unsafe { self.raw.virtual_start.as_mut() }
}
}

impl<H, T> Drop for PhysicalMapping<H, T>
where
H: Handler,
{
fn drop(&mut self) {
unsafe {
self.handler.unmap_physical_region(self.raw);
}
}
}

/// A `Handle` is an opaque reference to an object that is managed by the host on behalf of this
/// library.
///
Expand Down Expand Up @@ -496,8 +378,6 @@ pub trait Handler: Clone {

/// Unmap the given physical mapping. This is called when a [`PhysicalMapping`] is dropped, you should **not** manually call this.
///
/// Note: A reference to the [`Handler`] used to construct `region` can be acquired from [`PhysicalMapping::handler`].
///
/// # Safety
///
/// `region` must denote a physical mapping _currently allocated_ via this handler.
Expand Down
Loading
Loading