diff --git a/Cargo.toml b/Cargo.toml index 4bb93377..3c7bb871 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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"] diff --git a/src/address.rs b/src/address.rs index 25d3efed..57d3cefb 100644 --- a/src/address.rs +++ b/src/address.rs @@ -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), } } @@ -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(()) @@ -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::(); - unsafe { addr.byte_offset(byte_offset as isize).read_unaligned() } - } + AddressSpace::SystemMemory => unsafe { + let addr = self.mapping.as_ref().unwrap().as_mut_ptr().cast::(); + 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:?}"), } @@ -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::(); - 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::(); + 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) } diff --git a/src/aml/mod.rs b/src/aml/mod.rs index 24b74815..5aa53066 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -43,6 +43,7 @@ use alloc::{ use bit_field::BitField; use core::{ mem, + ptr, slice, str::FromStr, sync::atomic::{AtomicU64, Ordering}, @@ -204,7 +205,7 @@ where }; let stream = unsafe { slice::from_raw_parts( - mapping.raw.virtual_start.as_ptr().byte_add(mem::size_of::()) as *const u8, + ptr::from_ref(&*mapping).byte_add(size_of::()) as *const u8, table.length as usize - mem::size_of::(), ) }; diff --git a/src/lib.rs b/src/lib.rs index fdb9582b..e4b11ab7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -55,6 +55,7 @@ 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; @@ -62,16 +63,11 @@ 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; @@ -169,9 +165,8 @@ where rsdt_entry_size: usize, quirks: AcpiQuirks, ) -> Result, AcpiError> { - let rsdt_mapping = unsafe { - PhysicalMapping::<_, SdtHeader>::new(rsdt_address, mem::size_of::(), handler.clone()) - }; + let rsdt_mapping = + unsafe { PhysicalMapping::<_, SdtHeader>::new(rsdt_address, size_of::(), handler.clone()) }; let rsdt_length = rsdt_mapping.length; let rsdt_mapping = @@ -182,10 +177,10 @@ where /// Iterate over the **physical** addresses of the SDTs. pub fn table_entries(&self) -> impl Iterator { let mut table_entries_ptr = - unsafe { self.rsdt_mapping.raw.virtual_start.as_ptr().byte_add(mem::size_of::()) } - .cast::(); - let mut num_entries = (self.rsdt_mapping.raw.region_length.saturating_sub(mem::size_of::())) - / self.rsdt_entry_size; + unsafe { self.rsdt_mapping.as_ptr().byte_add(size_of::()) }.cast::(); + let mut num_entries = + (self.rsdt_mapping.raw().region_length().saturating_sub(mem::size_of::())) + / self.rsdt_entry_size; core::iter::from_fn(move || { if num_entries > 0 { @@ -347,119 +342,6 @@ pub struct AcpiQuirks { pub ignore_xsdt: bool, } -/// Describes a physical mapping. -/// -/// The region mapped must be at least `size_of::()` bytes, but may be bigger. -#[derive(Debug)] -pub struct RawPhysicalMapping { - /// 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, - /// The size of the requested region, in bytes. Can be equal or larger to `size_of::()`. 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 Clone for RawPhysicalMapping { - fn clone(&self) -> Self { - *self - } -} - -impl Copy for RawPhysicalMapping {} - -/// 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::()` -/// bytes, but may be bigger. -pub struct PhysicalMapping -where - H: Handler, -{ - pub raw: RawPhysicalMapping, - /// 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 PhysicalMapping -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::()`. - pub unsafe fn new(physical_address: usize, size: usize, handler: H) -> PhysicalMapping { - 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 fmt::Debug for PhysicalMapping -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 Send for PhysicalMapping {} - -impl Deref for PhysicalMapping -where - T: Unpin, - H: Handler, -{ - type Target = T; - - fn deref(&self) -> &T { - unsafe { self.raw.virtual_start.as_ref() } - } -} - -impl DerefMut for PhysicalMapping -where - T: Unpin, - H: Handler, -{ - fn deref_mut(&mut self) -> &mut T { - unsafe { self.raw.virtual_start.as_mut() } - } -} - -impl Drop for PhysicalMapping -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. /// @@ -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. diff --git a/src/physical_mapping.rs b/src/physical_mapping.rs new file mode 100644 index 00000000..e1b9e833 --- /dev/null +++ b/src/physical_mapping.rs @@ -0,0 +1,218 @@ +use crate::Handler; +use core::{ + fmt, + ops::{Deref, DerefMut}, + pin::Pin, + ptr::NonNull, +}; + +/// Describes a physical mapping. +/// +/// The region mapped must be at least `size_of::()` bytes, but may be bigger. +#[derive(Debug)] +pub struct RawPhysicalMapping { + /// 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. + 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. + virtual_start: NonNull, + /// The size of the requested region, in bytes. Can be equal or larger to `size_of::()`. If a + /// larger region has been mapped, this should still be the requested size. + 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. + mapped_length: usize, +} + +impl RawPhysicalMapping { + /// Construct a new RawPhysicalMapping. + /// + /// # Safety + /// + /// `virtual_start` must be unique amongst all `RawPhysicalMappings` that are created, + /// and `virtual_start + sizeof` must not alias other `RawPhysicalMappings`. + /// + /// If this `RawPhysicalMapping` will ever be unmapped, then all arguments provided must be + /// consistent to allow that unmapping to take place. + /// + /// If this `RawPhysicalMapping` will be used as part of a `MappedGas`, then `virtual_start` + /// must not point to memory within the Rust allocation system (as described in + /// [`core::ptr::write_volatile`]) + pub unsafe fn new( + physical_start: usize, + virtual_start: NonNull, + region_length: usize, + mapped_length: usize, + ) -> Self { + Self { physical_start, virtual_start, region_length, mapped_length } + } + + pub fn physical_start(&self) -> usize { + self.physical_start + } + + /// Get the underlying virtual pointer for this mapping + /// + /// # Safety + /// + /// The returned pointer must not be used to create duplicate references to the same address, + /// or any other action that would violate the Rust aliasing rules. Always try to access the + /// stored data by dereferencing a PhysicalMapping. + pub unsafe fn virtual_start(&self) -> NonNull { + self.virtual_start + } + + pub fn region_length(&self) -> usize { + self.region_length + } + + pub fn mapped_length(&self) -> usize { + self.mapped_length + } +} + +impl Clone for RawPhysicalMapping { + fn clone(&self) -> Self { + *self + } +} + +impl Copy for RawPhysicalMapping {} + +/// 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::()` +/// bytes, but may be bigger. +pub struct PhysicalMapping +where + H: Handler, +{ + raw: RawPhysicalMapping, + + // See the notes adjacent to the `unsafe impl Sync` line if you are planning to add new methods + // that access this member. + /// The [`Handler`] that was used to produce the mapping. When this mapping is dropped, this + /// handler will be used to unmap the region. + handler: H, +} + +impl PhysicalMapping +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::()`. + pub unsafe fn new(physical_address: usize, size: usize, handler: H) -> PhysicalMapping { + let raw = unsafe { handler.map_physical_region(physical_address, size) }; + PhysicalMapping { raw, handler } + } + + /// Creates a new physical mapping from a previously constructed [`RawPhysicalMapping`]. + /// + /// # Safety + /// + /// This must not be used to create duplicate physical mappings for the same + /// [`RawPhysicalMapping`]. + /// + /// It is unlikely that this function will be useful outside of tests. + pub unsafe fn new_unchecked(raw: RawPhysicalMapping, handler: H) -> PhysicalMapping { + 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()) } + } + + pub fn raw(&self) -> RawPhysicalMapping { + self.raw + } + + /// Return the underlying pointer. + /// + /// # Safety + /// + /// The returned pointer must not be aliased with any references to `&mut self` or pointers + /// returned by [`Self::as_mut_ptr`] - that is, normal Rust pointer rules apply. + pub unsafe fn as_ptr(&self) -> *const T { + self.raw.virtual_start.as_ptr() + } + + /// Return the underlying pointer, for use in a `mut` context. + /// + /// # Safety + /// + /// **This `mut` pointer is not protected against aliasing by being taken from an `&mut self`** + /// \- it would be very easy to create multiple `mut` pointers, or a `mut` pointer alongside + /// non-`mut` references. The caller must not create or use such pointers. + /// + /// Alternatively, users calling write_volatile may rely on the idea that writes to memory + /// outside the Rust allocation system are allowed to avoid the above rule - see the docs for + /// [`core::ptr::write_volatile`] for details on that. + pub unsafe fn as_mut_ptr(&self) -> *mut T { + self.raw.virtual_start.as_ptr() + } +} + +impl fmt::Debug for PhysicalMapping +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 Send for PhysicalMapping {} + +// Safety: H need not be sync as PhysicalMapping owns a copy of Handler, and only ever uses it in +// the context of `&mut self` - therefore, usage of the Handler is implicitly synchronised. +// +// HOWEVER: If `handler` is ever called in a PhysicalMapping member that takes only `&self`, this +// will become incorrect, and `H: Handler` below should become `H: Handler + Sync`. +unsafe impl Sync for PhysicalMapping {} + +impl Deref for PhysicalMapping +where + T: Unpin, + H: Handler, +{ + type Target = T; + + fn deref(&self) -> &T { + unsafe { self.raw.virtual_start.as_ref() } + } +} + +impl DerefMut for PhysicalMapping +where + T: Unpin, + H: Handler, +{ + fn deref_mut(&mut self) -> &mut T { + unsafe { self.raw.virtual_start.as_mut() } + } +} + +impl Drop for PhysicalMapping +where + H: Handler, +{ + fn drop(&mut self) { + unsafe { + self.handler.unmap_physical_region(self.raw); + } + } +} diff --git a/src/rsdp.rs b/src/rsdp.rs index 95d3efc5..f168acd2 100644 --- a/src/rsdp.rs +++ b/src/rsdp.rs @@ -64,15 +64,17 @@ impl Rsdp { }; let extended_area_bytes = - unsafe { slice::from_raw_parts(mapping.raw.virtual_start.as_ptr(), mapping.raw.region_length) }; + // Safety: We aren't casting from non-mut to mut. Also, we have not created any + // other reference to the extended data area. This means reading the extended data + // area is protected from overlapping writes by the Rust referencing rules. + unsafe { slice::from_raw_parts(mapping.as_ptr(), mapping.raw().region_length()) }; // Search `Rsdp`-sized windows at 16-byte boundaries relative to the base of the area (which is also // aligned to 16 bytes due to the implementation of `find_search_areas`) extended_area_bytes.windows(mem::size_of::()).step_by(16).find_map(|maybe_rsdp_bytes_slice| { let maybe_rsdp_virt_ptr = maybe_rsdp_bytes_slice.as_ptr().cast::(); - let maybe_rsdp_phys_start = maybe_rsdp_virt_ptr as usize - - mapping.raw.virtual_start.as_ptr() as usize - + mapping.raw.physical_start; + let maybe_rsdp_phys_start = maybe_rsdp_virt_ptr as usize - unsafe { mapping.as_ptr() } as usize + + mapping.raw().physical_start(); // SAFETY: `maybe_rsdp_virt_ptr` points to an aligned, readable `Rsdp`-sized value, and the `Rsdp` // struct's fields are always initialized. let maybe_rsdp = unsafe { &*maybe_rsdp_virt_ptr }; diff --git a/tests/static_assertions.rs b/tests/static_assertions.rs new file mode 100644 index 00000000..24b66715 --- /dev/null +++ b/tests/static_assertions.rs @@ -0,0 +1,26 @@ +#![feature(negative_impls)] + +use acpi::{PhysicalMapping, aml::Interpreter, registers::FixedRegisters}; +use aml_test_tools::handlers::null_handler::NullHandler; +use static_assertions::{assert_impl_all, assert_not_impl_any}; + +struct NoSend {} +impl !Send for NoSend {} +impl !Sync for NoSend {} + +struct SendOnly {} +unsafe impl Send for SendOnly {} +impl !Sync for SendOnly {} + +// If all other types are correctly labelled with Send and/or Sync, then Interpreter should +// naturally become Send/Sync. +assert_impl_all!(Interpreter: Send, Sync); + +// The Send/Sync-ness of PhysicalMapping depend directly and purely on the wrapped type. +assert_impl_all!(PhysicalMapping: Send, Sync); +assert_not_impl_any!(PhysicalMapping: Send, Sync); +assert_impl_all!(PhysicalMapping: Send); +assert_not_impl_any!(PhysicalMapping: Sync); + +// This was explicitly requested in [#324](https://github.com/rust-osdev/acpi/issues/324) +assert_impl_all!(FixedRegisters: Send, Sync); diff --git a/tools/aml-test-tools/src/handlers/check_cmd_handler.rs b/tools/aml-test-tools/src/handlers/check_cmd_handler.rs index 57618af4..93926ba7 100644 --- a/tools/aml-test-tools/src/handlers/check_cmd_handler.rs +++ b/tools/aml-test-tools/src/handlers/check_cmd_handler.rs @@ -108,7 +108,7 @@ where // Also stop checking if we're at or past the end of the command list. This stops any // confusion about whether we're in Drop or not. if !std::thread::panicking() && self.commands.len() > self.next_command_idx.load(Relaxed) { - self.check_command(AcpiCommands::UnmapPhysicalRegion(region.physical_start)); + self.check_command(AcpiCommands::UnmapPhysicalRegion(region.physical_start())); } unsafe { diff --git a/tools/aml-test-tools/src/handlers/logging_handler.rs b/tools/aml-test-tools/src/handlers/logging_handler.rs index 8248354b..fdb30d21 100644 --- a/tools/aml-test-tools/src/handlers/logging_handler.rs +++ b/tools/aml-test-tools/src/handlers/logging_handler.rs @@ -32,7 +32,7 @@ where } unsafe fn unmap_physical_region(&self, region: RawPhysicalMapping) { - info!("unmap_physical_region(physical_start={:#x})", region.physical_start); + info!("unmap_physical_region(physical_start={:#x})", region.physical_start()); unsafe { self.next_handler.unmap_physical_region(region); diff --git a/tools/aml-test-tools/src/lib.rs b/tools/aml-test-tools/src/lib.rs index 6cad0af9..8b1ff8ac 100644 --- a/tools/aml-test-tools/src/lib.rs +++ b/tools/aml-test-tools/src/lib.rs @@ -284,14 +284,11 @@ where // This PhysicalMapping is dropped when the interpreter is dropped, and if you use logging in // the handler object you'll see a call to Handler::unmap_physical_region without any // corresponding call to Interpreter::map_physical_region. - let fake_facs_mapping = PhysicalMapping { - raw: RawPhysicalMapping { - physical_start: 0x0, - virtual_start: NonNull::new(fake_facs_ptr).unwrap(), - region_length: 32, - mapped_length: 32, - }, - handler: handler.clone(), + let fake_facs_mapping = unsafe { + PhysicalMapping::new_unchecked( + RawPhysicalMapping::new(0x0, NonNull::new(fake_facs_ptr).unwrap(), 32, 32), + handler.clone(), + ) }; Interpreter::new(handler, 2, fake_registers, Some(fake_facs_mapping)) }