acpi: Reduce duplicate code to iterate over root table entries

This also fixes `SsdtIterator`, which was not iterating reliably.

Author: rcerc

acpi/Cargo.toml

@@ -11,6 +11,7 @@ edition = "2018"
 
 [dependencies]
 bit_field = "0.10"
+log = "0.4"
 rsdp = { version = "2", path = "../rsdp" }
 
 [features]

acpi/src/lib.rs

@@ -236,43 +236,52 @@ where
         self.revision
     }
 
-    /// Searches through the ACPI table headers and attempts to locate the table with a matching `T::SIGNATURE`.
-    pub fn find_table<T: AcpiTable>(&self) -> AcpiResult<PhysicalMapping<H, T>> {
-        use core::mem::size_of;
+    /// Constructs a [`TablesPhysPtrsIter`] over this table.
+    fn tables_phys_ptrs(&self) -> TablesPhysPtrsIter<'_> {
+        // SAFETY: The virtual address of the array of pointers follows the virtual address of the table in memory.
+        let ptrs_virt_start = unsafe { self.mapping.virtual_start().as_ptr().add(1).cast::<u8>() };
+        let ptrs_bytes_len = self.mapping.region_length() - mem::size_of::<SdtHeader>();
+        // SAFETY: `ptrs_virt_start` points to an array of `ptrs_bytes_len` bytes that lives as long as `self`.
+        let ptrs_bytes = unsafe { core::slice::from_raw_parts(ptrs_virt_start, ptrs_bytes_len) };
+        let ptr_size = if self.revision == 0 {
+            4 // RSDT entry size
+        } else {
+            8 // XSDT entry size
+        };
 
-        if self.revision == 0 {
-            let num_tables = ((self.mapping.length as usize) - size_of::<SdtHeader>()) / size_of::<u32>();
-            // Safety: Table pointer is known-good for these offsets and types.
-            let tables_base = unsafe { self.mapping.virtual_start().as_ptr().add(1).cast::<u32>() };
+        ptrs_bytes.chunks(ptr_size).map(|ptr_bytes_src| {
+            // Construct a native pointer using as many bytes as required from `ptr_bytes_src` (note that ACPI is
+            // little-endian)
 
-            for offset in 0..num_tables {
-                // Safety: Table pointer is known-good for these offsets and types.
-                let sdt_header_address = unsafe { tables_base.add(offset).read_unaligned() } as usize;
+            let mut ptr_bytes_dst = [0; mem::size_of::<usize>()];
+            let common_ptr_size = usize::min(mem::size_of::<usize>(), ptr_bytes_src.len());
+            ptr_bytes_dst[..common_ptr_size].copy_from_slice(&ptr_bytes_src[..common_ptr_size]);
 
-                // Safety: `RSDT` guarantees its contained addresses to be valid.
-                let table_result = unsafe { read_table(self.handler.clone(), sdt_header_address) };
-                if table_result.is_ok() {
-                    return table_result;
-                }
-            }
-        } else {
-            let num_tables = ((self.mapping.length as usize) - size_of::<SdtHeader>()) / size_of::<u64>();
-            // Safety: Table pointer is known-good for these offsets and types.
-            let tables_base = unsafe { self.mapping.virtual_start().as_ptr().add(1).cast::<u64>() };
-
-            for offset in 0..num_tables {
-                // Safety: Table pointer is known-good for these offsets and types.
-                let sdt_header_address = unsafe { tables_base.add(offset).read_unaligned() } as usize;
-
-                // Safety: `XSDT` guarantees its contained addresses to be valid.
-                let table_result = unsafe { read_table(self.handler.clone(), sdt_header_address) };
-                if table_result.is_ok() {
-                    return table_result;
-                }
-            }
-        }
+            usize::from_le_bytes(ptr_bytes_dst) as *const SdtHeader
+        })
+    }
 
-        Err(AcpiError::TableMissing(T::SIGNATURE))
+    /// Searches through the ACPI table headers and attempts to locate the table with a matching `T::SIGNATURE`.
+    pub fn find_table<T: AcpiTable>(&self) -> AcpiResult<PhysicalMapping<H, T>> {
+        self.tables_phys_ptrs()
+            .find_map(|table_phys_ptr| {
+                // SAFETY: Table guarantees its contained addresses to be valid.
+                match unsafe { read_table(self.handler.clone(), table_phys_ptr as usize) } {
+                    Ok(table_mapping) => Some(table_mapping),
+                    Err(AcpiError::SdtInvalidSignature(_)) => None,
+                    Err(e) => {
+                        log::warn!(
+                            "Found invalid {} table at physical address {:p}: {:?}",
+                            T::SIGNATURE,
+                            table_phys_ptr,
+                            e
+                        );
+
+                        None
+                    }
+                }
+            })
+            .ok_or(AcpiError::TableMissing(T::SIGNATURE))
     }
 
     /// Finds and returns the DSDT AML table, if it exists.
@@ -298,17 +307,7 @@ where
 
     /// Iterates through all of the SSDT tables.
     pub fn ssdts(&self) -> SsdtIterator<H> {
-        let header_ptrs_base_ptr =
-            unsafe { self.mapping.virtual_start().as_ptr().add(1).cast::<*const SdtHeader>() };
-        let header_ptr_count = ((self.mapping.length as usize) - mem::size_of::<SdtHeader>())
-            / core::mem::size_of::<*const *const SdtHeader>();
-
-        SsdtIterator {
-            current_sdt_ptr: header_ptrs_base_ptr,
-            remaining: header_ptr_count,
-            handler: self.handler.clone(),
-            marker: core::marker::PhantomData,
-        }
+        SsdtIterator { tables_phys_ptrs: self.tables_phys_ptrs(), handler: self.handler.clone() }
     }
 
     /// Convenience method for contructing a [`PlatformInfo`](crate::platform::PlatformInfo). This is one of the
@@ -333,6 +332,9 @@ pub struct Sdt {
     pub validated: bool,
 }
 
+/// An iterator over the physical table addresses in an RSDT or XSDT.
+type TablesPhysPtrsIter<'t> = core::iter::Map<core::slice::Chunks<'t, u8>, fn(&[u8]) -> *const SdtHeader>;
+
 #[derive(Debug)]
 pub struct AmlTable {
     /// Physical address of the start of the AML stream (excluding the table header).
@@ -391,55 +393,50 @@ unsafe fn read_table<H: AcpiHandler, T: AcpiTable>(
 }
 
 /// Iterator that steps through all of the tables, and returns only the SSDTs as `AmlTable`s.
-pub struct SsdtIterator<'a, H>
+pub struct SsdtIterator<'t, H>
 where
     H: AcpiHandler,
 {
-    current_sdt_ptr: *const *const SdtHeader,
-    remaining: usize,
+    tables_phys_ptrs: TablesPhysPtrsIter<'t>,
     handler: H,
-    marker: core::marker::PhantomData<&'a ()>,
 }
 
-impl<'a, H> Iterator for SsdtIterator<'a, H>
+impl<'t, H> Iterator for SsdtIterator<'t, H>
 where
     H: AcpiHandler,
 {
     type Item = AmlTable;
 
     fn next(&mut self) -> Option<Self::Item> {
-        struct Ssdt;
-        // Safety: Implementation properly represents a valid SSDT.
+        #[repr(transparent)]
+        struct Ssdt {
+            header: SdtHeader,
+        }
+
+        // SAFETY: Implementation properly represents a valid SSDT.
         unsafe impl AcpiTable for Ssdt {
             const SIGNATURE: Signature = Signature::SSDT;
 
-            fn header(&self) -> &sdt::SdtHeader {
-                // Safety: DSDT will always be valid for an SdtHeader at its `self` pointer.
-                unsafe { &*(self as *const Self as *const sdt::SdtHeader) }
+            fn header(&self) -> &SdtHeader {
+                &self.header
             }
         }
 
-        if self.remaining == 0 {
-            None
-        } else {
-            loop {
-                // Attempt to peek at the SDT header to correctly enumerate the entire table.
-                // Safety: `address` needs to be valid for the size of `SdtHeader`, or the ACPI tables are malformed (not a software issue).
-                let sdt_header = unsafe {
-                    self.handler.map_physical_region::<SdtHeader>(
-                        self.current_sdt_ptr.read() as usize,
-                        core::mem::size_of::<SdtHeader>(),
-                    )
-                };
+        // Borrow single field for closure to avoid immutable reference to `self` that inhibits `find_map`
+        let handler = &self.handler;
 
-                self.remaining -= 1;
+        // Consume iterator until next valid SSDT and return the latter
+        self.tables_phys_ptrs.find_map(|table_phys_ptr| {
+            // SAFETY: Table guarantees its contained addresses to be valid.
+            match unsafe { read_table::<_, Ssdt>(handler.clone(), table_phys_ptr as usize) } {
+                Ok(ssdt_mapping) => Some(AmlTable::new(ssdt_mapping.physical_start(), ssdt_mapping.header.length)),
+                Err(AcpiError::SdtInvalidSignature(_)) => None,
+                Err(e) => {
+                    log::warn!("Found invalid SSDT at physical address {:p}: {:?}", table_phys_ptr, e);
 
-                if sdt_header.validate(Ssdt::SIGNATURE).is_err() {
-                    continue;
-                } else {
-                    return Some(AmlTable { address: sdt_header.physical_start(), length: sdt_header.length });
+                    None
                 }
             }
-        }
+        })
     }
 }