Add AcpiTable and simplify get_sdt, removing need for coerce_type

acpi/src/fadt.rs

@@ -1,10 +1,8 @@
 use crate::{
     sdt::{ExtendedField, SdtHeader, ACPI_VERSION_2_0},
     AcpiError,
-    AcpiHandler,
-    AmlTable,
+    AcpiTable,
     GenericAddress,
-    PhysicalMapping,
 };
 
 #[derive(Clone, Copy, PartialEq, Eq, Debug)]
@@ -92,6 +90,12 @@ pub struct Fadt {
     hypervisor_vendor_id: ExtendedField<u64, ACPI_VERSION_2_0>,
 }
 
+impl AcpiTable for Fadt {
+    fn header(&self) -> &SdtHeader {
+        &self.header
+    }
+}
+
 impl Fadt {
     pub fn validate(&self) -> Result<(), AcpiError> {
         self.header.validate(crate::sdt::Signature::FADT)

acpi/src/handler.rs

@@ -1,4 +1,4 @@
-use core::{mem, ops::Deref, ptr::NonNull};
+use core::{ops::Deref, ptr::NonNull};
 
 /// Describes a physical mapping created by `AcpiHandler::map_physical_region` and unmapped by
 /// `AcpiHandler::unmap_physical_region`. The region mapped must be at least `size_of::<T>()`
@@ -11,45 +11,7 @@ where
     pub virtual_start: NonNull<T>,
     pub region_length: usize, // Can be equal or larger than size_of::<T>()
     pub mapped_length: usize, // Differs from `region_length` if padding is added for alignment
-    /*
-     * NOTE: we store an `Option<H>` here to make the implementation of `coerce_type` easier - if we can find a
-     * better way, that would be better. Other than that, this should never be `None`, so is fine to unwrap.
-     */
-    handler: Option<H>,
-}
-
-impl<H, T> PhysicalMapping<H, T>
-where
-    H: AcpiHandler,
-{
-    pub fn new(
-        physical_start: usize,
-        virtual_start: NonNull<T>,
-        region_length: usize,
-        mapped_length: usize,
-        handler: H,
-    ) -> PhysicalMapping<H, T> {
-        PhysicalMapping { physical_start, virtual_start, region_length, mapped_length, handler: Some(handler) }
-    }
-
-    pub(crate) unsafe fn coerce_type<N>(mut self) -> PhysicalMapping<H, N> {
-        /*
-         * Ideally, we'd like to assert something like `self.region_length >= mem::size_of::<N>()` here, but we
-         * can't as some types are actually sometimes larger than their tables, and use mechanisms such as
-         * `ExtendedField` to mediate access.
-         */
-        assert!((self.virtual_start.as_ptr() as usize) % mem::align_of::<N>() == 0);
-
-        let result = PhysicalMapping {
-            physical_start: self.physical_start,
-            virtual_start: NonNull::new(self.virtual_start.as_ptr() as *mut N).unwrap(),
-            region_length: self.region_length,
-            mapped_length: self.mapped_length,
-            handler: mem::replace(&mut self.handler, None),
-        };
-        mem::forget(self);
-        result
-    }
+    pub handler: H,
 }
 
 impl<H, T> Deref for PhysicalMapping<H, T>
@@ -68,7 +30,7 @@ where
     H: AcpiHandler,
 {
     fn drop(&mut self) {
-        self.handler.as_ref().unwrap().unmap_physical_region(self)
+        self.handler.unmap_physical_region(self)
     }
 }
 

acpi/src/hpet.rs

@@ -1,4 +1,4 @@
-use crate::{sdt::SdtHeader, AcpiError, AcpiHandler, AcpiTables, GenericAddress, PhysicalMapping};
+use crate::{sdt::SdtHeader, AcpiError, AcpiHandler, AcpiTable, AcpiTables, GenericAddress};
 use bit_field::BitField;
 
 #[derive(Debug)]
@@ -61,3 +61,9 @@ pub(crate) struct HpetTable {
     clock_tick_unit: u16,
     page_protection_oem: u8,
 }
+
+impl AcpiTable for HpetTable {
+    fn header(&self) -> &SdtHeader {
+        &self.header
+    }
+}

acpi/src/lib.rs

@@ -222,21 +222,33 @@ where
         Ok(())
     }
 
+    /// Create a mapping to a SDT, given its signature. This validates the SDT if it has not already been
+    /// validated.
+    ///
+    /// ### Safety
+    /// The table's memory is naively interpreted as a `T`, and so you must be careful in providing a type that
+    /// correctly represents the table's structure. Regardless of the provided type's size, the region mapped will
+    /// be the size specified in the SDT's header. Providing a `T` that is larger than this, *may* lead to
+    /// page-faults, aliasing references, or derefencing uninitialized memory (the latter two of which are UB).
+    /// This isn't forbidden, however, because some tables rely on `T` being larger than a provided SDT in some
+    /// versions of ACPI (the [`ExtendedField`](crate::sdt::ExtendedField) type will be useful if you need to do
+    /// this. See our [`Fadt`](crate::fadt::Fadt) type for an example of this).
     pub unsafe fn get_sdt<T>(&self, signature: sdt::Signature) -> Result<Option<PhysicalMapping<H, T>>, AcpiError>
     where
         T: AcpiTable,
+    {
         let sdt = match self.sdts.get(&signature) {
             Some(sdt) => sdt,
             None => return Ok(None),
         };
-        let mapping =
-            unsafe { self.handler.map_physical_region::<SdtHeader>(sdt.physical_address, sdt.length as usize) };
+        let mapping = unsafe { self.handler.map_physical_region::<T>(sdt.physical_address, sdt.length as usize) };
 
         if !sdt.validated {
-            mapping.validate(signature)?;
+            mapping.header().validate(signature)?;
         }
 
-        Ok(Some(mapping.coerce_type()))
+        Ok(Some(mapping))
+    }
     }
 }
 
@@ -249,6 +261,11 @@ pub struct Sdt {
     pub validated: bool,
 }
 
+/// All types representing ACPI tables should implement this trait.
+pub trait AcpiTable {
+    fn header(&self) -> &sdt::SdtHeader;
+}
+
 #[derive(Debug)]
 pub struct AmlTable {
     /// Physical address of the start of the AML stream (excluding the table header).

acpi/src/madt.rs

@@ -16,8 +16,7 @@ use crate::{
     },
     sdt::SdtHeader,
     AcpiError,
-    AcpiHandler,
-    PhysicalMapping,
+    AcpiTable,
 };
 use alloc::vec::Vec;
 use bit_field::BitField;
@@ -47,19 +46,13 @@ pub struct Madt {
     flags: u32,
 }
 
-impl Madt {
-    pub fn entries(&self) -> MadtEntryIter {
-        MadtEntryIter {
-            pointer: unsafe { (self as *const Madt as *const u8).offset(mem::size_of::<Madt>() as isize) },
-            remaining_length: self.header.length - mem::size_of::<Madt>() as u32,
-            _phantom: PhantomData,
-        }
-    }
-
-    pub fn supports_8259(&self) -> bool {
-        unsafe { self.flags.get_bit(0) }
+impl AcpiTable for Madt {
+    fn header(&self) -> &SdtHeader {
+        &self.header
     }
+}
 
+impl Madt {
     pub fn parse_interrupt_model(&self) -> Result<(InterruptModel, Option<ProcessorInfo>), AcpiError> {
         /*
          * We first do a pass through the MADT to determine which interrupt model is being used.
@@ -224,6 +217,18 @@ impl Madt {
             Some(ProcessorInfo { boot_processor: boot_processor.unwrap(), application_processors }),
         ))
     }
+
+    pub fn entries(&self) -> MadtEntryIter {
+        MadtEntryIter {
+            pointer: unsafe { (self as *const Madt as *const u8).offset(mem::size_of::<Madt>() as isize) },
+            remaining_length: self.header.length - mem::size_of::<Madt>() as u32,
+            _phantom: PhantomData,
+        }
+    }
+
+    pub fn supports_8259(&self) -> bool {
+        unsafe { self.flags.get_bit(0) }
+    }
 }
 
 pub struct MadtEntryIter<'a> {

acpi/src/mcfg.rs

@@ -1,4 +1,4 @@
-use crate::{handler::PhysicalMapping, sdt::SdtHeader, AcpiError, AcpiHandler, AcpiTables};
+use crate::{sdt::SdtHeader, AcpiError, AcpiHandler, AcpiTable, AcpiTables};
 use alloc::vec::Vec;
 use core::{mem, slice};
 
@@ -52,6 +52,12 @@ pub(crate) struct Mcfg {
     // Followed by `n` entries with format `McfgEntry`
 }
 
+impl AcpiTable for Mcfg {
+    fn header(&self) -> &SdtHeader {
+        &self.header
+    }
+}
+
 impl Mcfg {
     fn entries(&self) -> &[McfgEntry] {
         let length = self.header.length as usize - mem::size_of::<Mcfg>();