From 793af58ea47e1398bcdf2e6fd4c3725a92630065 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Thu, 23 May 2024 11:18:40 +0200 Subject: [PATCH 01/11] doc: fix typos --- uefi-services/README.md | 2 +- uefi/src/table/boot.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/uefi-services/README.md b/uefi-services/README.md index ddb053248..f240f5870 100644 --- a/uefi-services/README.md +++ b/uefi-services/README.md @@ -1,4 +1,4 @@ # uefi-services WARNING: `uefi-services` is deprecated. Functionality was moved to -`uefi::helpers::init` in `uefi` ´v0.28.0`. +`uefi::helpers::init` in `uefi@v0.28.0`. diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs index 1a5e44876..436d57d56 100644 --- a/uefi/src/table/boot.rs +++ b/uefi/src/table/boot.rs @@ -216,8 +216,8 @@ impl BootServices { /// /// The buffer must be aligned like a `MemoryDescriptor`. /// - /// The returned key is a unique identifier of the current configuration of memory. - /// Any allocations or such will change the memory map's key. + /// The returned key is a unique identifier of the current configuration of + /// memory. Any allocations or such will change the memory map's key. /// /// If you want to store the resulting memory map without having to keep /// the buffer around, you can use `.copied().collect()` on the iterator. @@ -1628,7 +1628,7 @@ pub struct MemoryMapSize { /// map, you manually have to call [`MemoryMap::sort`] first. /// /// ## UEFI pitfalls -/// **Please note that when working with memory maps, the `entry_size` is +/// **Please note** that when working with memory maps, the `entry_size` is /// usually larger than `size_of:: Date: Sat, 25 May 2024 12:55:13 +0200 Subject: [PATCH 02/11] doc: more clarity in uefi/mem.rs --- uefi/src/mem.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/uefi/src/mem.rs b/uefi/src/mem.rs index bd6d3794a..1d0d66d41 100644 --- a/uefi/src/mem.rs +++ b/uefi/src/mem.rs @@ -23,10 +23,14 @@ use {core::alloc::Allocator, core::ptr::NonNull}; /// success. /// /// # Feature `unstable` / `allocator_api` -/// By default, this function works with Rust's default allocation mechanism. If you activate the -/// `unstable`-feature, it uses the `allocator_api` instead. In that case, the function takes an -/// additional parameter describing the specific [`Allocator`]. You can use [`alloc::alloc::Global`] -/// as default. +/// By default, this function works with the allocator that is set as +/// `#[global_allocator]`. This might be UEFI allocator but depends on your +/// use case and how you set up the environment. +/// +/// If you activate the `unstable`-feature, all allocations uses the provided +/// allocator (via `allocator_api`) instead. In that case, the function takes an +/// additional parameter describing the specific [`Allocator`]. You can use +/// [`alloc::alloc::Global`] which defaults to the `#[global_allocator]`. /// /// [`Allocator`]: https://doc.rust-lang.org/alloc/alloc/trait.Allocator.html /// [`alloc::alloc::Global`]: https://doc.rust-lang.org/alloc/alloc/struct.Global.html From 1b7a9bd7a322cedbe081fc6c9896f4b2e33d4f58 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Thu, 23 May 2024 11:21:02 +0200 Subject: [PATCH 03/11] treewide: entry_size -> desc_size This helps to prevent confusions. MemoryDescriptors and their reported size are already a pitfall. So at least we should refrain from using non-standard names for them. --- uefi-test-runner/src/boot/memory.rs | 2 +- uefi/src/table/boot.rs | 63 ++++++++++++++++++----------- uefi/src/table/system.rs | 14 +++++-- 3 files changed, 52 insertions(+), 27 deletions(-) diff --git a/uefi-test-runner/src/boot/memory.rs b/uefi-test-runner/src/boot/memory.rs index 078548991..1855d1fc6 100644 --- a/uefi-test-runner/src/boot/memory.rs +++ b/uefi-test-runner/src/boot/memory.rs @@ -67,7 +67,7 @@ fn memory_map(bt: &BootServices) { let sizes = bt.memory_map_size(); // 2 extra descriptors should be enough. - let buf_sz = sizes.map_size + 2 * sizes.entry_size; + let buf_sz = sizes.map_size + 2 * sizes.desc_size; // We will use vectors for convenience. let mut buffer = vec![0_u8; buf_sz]; diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs index 436d57d56..469ea0f1d 100644 --- a/uefi/src/table/boot.rs +++ b/uefi/src/table/boot.rs @@ -189,7 +189,7 @@ impl BootServices { pub fn memory_map_size(&self) -> MemoryMapSize { let mut map_size = 0; let mut map_key = MemoryMapKey(0); - let mut entry_size = 0; + let mut desc_size = 0; let mut entry_version = 0; let status = unsafe { @@ -197,30 +197,41 @@ impl BootServices { &mut map_size, ptr::null_mut(), &mut map_key.0, - &mut entry_size, + &mut desc_size, &mut entry_version, ) }; assert_eq!(status, Status::BUFFER_TOO_SMALL); + assert_eq!( + map_size % desc_size, + 0, + "Memory map must be a multiple of the reported descriptor size." + ); + MemoryMapSize { - entry_size, + desc_size, map_size, } } - /// Retrieves the current memory map. + /// Stores the current UEFI memory map in the provided buffer. /// - /// The allocated buffer should be big enough to contain the memory map, - /// and a way of estimating how big it should be is by calling `memory_map_size`. + /// The allocated buffer must be at least aligned to a [`MemoryDescriptor`] + /// and should be big enough to store the whole map. To estimating how big + /// the map will be, you can call [`Self::memory_map_size`]. /// - /// The buffer must be aligned like a `MemoryDescriptor`. + /// The memory map contains entries of type [`MemoryDescriptor`]. However, + /// the relevant step size is always the reported `desc_size` but never + /// `size_of::()`. /// /// The returned key is a unique identifier of the current configuration of /// memory. Any allocations or such will change the memory map's key. /// /// If you want to store the resulting memory map without having to keep /// the buffer around, you can use `.copied().collect()` on the iterator. + /// Note that this will change the current memory map again, if the UEFI + /// allocator is used under the hood. /// /// # Errors /// @@ -233,7 +244,7 @@ impl BootServices { MemoryDescriptor::assert_aligned(buffer); let map_buffer = buffer.as_mut_ptr().cast::(); let mut map_key = MemoryMapKey(0); - let mut entry_size = 0; + let mut desc_size = 0; let mut entry_version = 0; assert_eq!( @@ -247,17 +258,17 @@ impl BootServices { &mut map_size, map_buffer, &mut map_key.0, - &mut entry_size, + &mut desc_size, &mut entry_version, ) } .to_result_with_val(move || { - let len = map_size / entry_size; + let len = map_size / desc_size; MemoryMap { key: map_key, buf: buffer, - entry_size, + desc_size, len, } }) @@ -1613,7 +1624,7 @@ pub struct MemoryMapKey(usize); #[derive(Debug)] pub struct MemoryMapSize { /// Size of a single memory descriptor in bytes - pub entry_size: usize, + pub desc_size: usize, /// Size of the entire memory map in bytes pub map_size: usize, } @@ -1642,7 +1653,7 @@ pub struct MemoryMap<'buf> { buf: &'buf mut [u8], /// Usually bound to the size of a [`MemoryDescriptor`] but can indicate if /// this field is ever extended by a new UEFI standard. - entry_size: usize, + desc_size: usize, len: usize, } @@ -1653,13 +1664,19 @@ impl<'buf> MemoryMap<'buf> { /// /// This allows parsing a memory map provided by a kernel after boot /// services have already exited. - pub fn from_raw(buf: &'buf mut [u8], entry_size: usize) -> Self { - assert!(entry_size >= mem::size_of::()); - let len = buf.len() / entry_size; + pub fn from_raw(buf: &'buf mut [u8], desc_size: usize) -> Self { + assert!(!buf.is_empty()); + assert_eq!( + buf.len() % desc_size, + 0, + "The buffer length must be a multiple of the desc_size" + ); + assert!(desc_size >= mem::size_of::()); + let len = buf.len() / desc_size; MemoryMap { key: MemoryMapKey(0), buf, - entry_size, + desc_size, len, } } @@ -1727,15 +1744,15 @@ impl<'buf> MemoryMap<'buf> { unsafe { ptr::swap_nonoverlapping( - base.add(index1 * self.entry_size), - base.add(index2 * self.entry_size), - self.entry_size, + base.add(index1 * self.desc_size), + base.add(index2 * self.desc_size), + self.desc_size, ); } } fn get_element_phys_addr(&self, index: usize) -> PhysicalAddress { - let offset = index.checked_mul(self.entry_size).unwrap(); + let offset = index.checked_mul(self.desc_size).unwrap(); let elem = unsafe { &*self.buf.as_ptr().add(offset).cast::() }; elem.phys_start } @@ -1767,7 +1784,7 @@ impl<'buf> MemoryMap<'buf> { &*self .buf .as_ptr() - .add(self.entry_size * index) + .add(self.desc_size * index) .cast::() }; @@ -1785,7 +1802,7 @@ impl<'buf> MemoryMap<'buf> { &mut *self .buf .as_mut_ptr() - .add(self.entry_size * index) + .add(self.desc_size * index) .cast::() }; diff --git a/uefi/src/table/system.rs b/uefi/src/table/system.rs index 87ed22a55..e27082a2a 100644 --- a/uefi/src/table/system.rs +++ b/uefi/src/table/system.rs @@ -165,7 +165,7 @@ impl SystemTable { let extra_entries = 8; let memory_map_size = self.boot_services().memory_map_size(); - let extra_size = memory_map_size.entry_size.checked_mul(extra_entries)?; + let extra_size = memory_map_size.desc_size.checked_mul(extra_entries)?; memory_map_size.map_size.checked_add(extra_size) } @@ -253,7 +253,12 @@ impl SystemTable { let boot_services = self.boot_services(); // Reboot the device. - let reset = |status| -> ! { self.runtime_services().reset(ResetType::COLD, status, None) }; + let reset = |status| -> ! { + { + log::warn!("Resetting the machine"); + self.runtime_services().reset(ResetType::COLD, status, None) + } + }; // Get the size of the buffer to allocate. If that calculation // overflows treat it as an unrecoverable error. @@ -284,7 +289,10 @@ impl SystemTable { }; return (st, memory_map); } - Err(err) => status = err.status(), + Err(err) => { + log::error!("Error retrieving the memory map for exiting the boot services"); + status = err.status() + } } } From 82178a2d1834f32f4f6ae3696788e86be889e8c3 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sun, 26 May 2024 00:05:20 +0200 Subject: [PATCH 04/11] uefi: MemoryMapSize -> MemoryMapMeta This prepares the following changes. --- uefi/src/table/boot.rs | 52 +++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs index 469ea0f1d..cca8a662d 100644 --- a/uefi/src/table/boot.rs +++ b/uefi/src/table/boot.rs @@ -179,18 +179,18 @@ impl BootServices { unsafe { (self.0.free_pages)(addr, count) }.to_result() } - /// Returns struct which contains the size of a single memory descriptor - /// as well as the size of the current memory map. + /// Queries the `get_memory_map` function of UEFI to retrieve the current + /// size of the map. Returns a [`MemoryMapMeta`]. /// - /// Note that the size of the memory map can increase any time an allocation happens, - /// so when creating a buffer to put the memory map into, it's recommended to allocate a few extra - /// elements worth of space above the size of the current memory map. + /// It is recommended to add a few more bytes for a subsequent allocation + /// for the memory map, as the memory map itself also needs heap memory, + /// and other allocations might occur before that call. #[must_use] - pub fn memory_map_size(&self) -> MemoryMapSize { + pub fn memory_map_size(&self) -> MemoryMapMeta { let mut map_size = 0; let mut map_key = MemoryMapKey(0); let mut desc_size = 0; - let mut entry_version = 0; + let mut desc_version = 0; let status = unsafe { (self.0.get_memory_map)( @@ -198,7 +198,7 @@ impl BootServices { ptr::null_mut(), &mut map_key.0, &mut desc_size, - &mut entry_version, + &mut desc_version, ) }; assert_eq!(status, Status::BUFFER_TOO_SMALL); @@ -209,9 +209,11 @@ impl BootServices { "Memory map must be a multiple of the reported descriptor size." ); - MemoryMapSize { + MemoryMapMeta { desc_size, map_size, + map_key, + desc_version, } } @@ -1619,14 +1621,30 @@ impl Align for MemoryDescriptor { #[repr(C)] pub struct MemoryMapKey(usize); -/// A structure containing the size of a memory descriptor and the size of the -/// memory map. -#[derive(Debug)] -pub struct MemoryMapSize { - /// Size of a single memory descriptor in bytes - pub desc_size: usize, - /// Size of the entire memory map in bytes +/// A structure containing the meta attributes associated with a call to +/// `GetMemoryMap` of UEFI. Note that all values refer to the time this was +/// called. All following invocations (hidden, subtle, and asynchronous ones) +/// will likely invalidate this. +#[derive(Copy, Clone, Debug)] +pub struct MemoryMapMeta { + /// The actual size of the map. pub map_size: usize, + /// The reported memory descriptor size. Note that this is the reference + /// and never `size_of::()`! + pub desc_size: usize, + /// A unique memory key bound to a specific memory map version/state. + pub map_key: MemoryMapKey, + /// The version of the descriptor struct. + pub desc_version: u32, +} + +impl MemoryMapMeta { + /// Returns the amount of entries in the map. + #[must_use] + pub fn entry_count(&self) -> usize { + assert_eq!(self.map_size % self.desc_size, 0); + self.map_size / self.desc_size + } } /// An accessory to the memory map that can be either iterated or @@ -1644,8 +1662,6 @@ pub struct MemoryMapSize { /// always use `entry_size` as step-size when interfacing with the memory map on /// a low level. /// -/// -/// /// [0]: https://github.com/tianocore/edk2/blob/7142e648416ff5d3eac6c6d607874805f5de0ca8/MdeModulePkg/Core/PiSmmCore/Page.c#L1059 #[derive(Debug)] pub struct MemoryMap<'buf> { From 98215a287979994e5c4fc89879822adbf48d5377 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 25 May 2024 20:24:58 +0200 Subject: [PATCH 05/11] uefi: introduce MemoryMapBackingMemory helper type --- uefi/src/table/boot.rs | 132 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 128 insertions(+), 4 deletions(-) diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs index cca8a662d..f5bc295b2 100644 --- a/uefi/src/table/boot.rs +++ b/uefi/src/table/boot.rs @@ -1,6 +1,6 @@ //! UEFI services available during boot. -use super::Revision; +use super::{system_table_boot, Revision}; use crate::data_types::{Align, PhysicalAddress}; use crate::proto::device_path::DevicePath; use crate::proto::loaded_image::LoadedImage; @@ -186,7 +186,7 @@ impl BootServices { /// for the memory map, as the memory map itself also needs heap memory, /// and other allocations might occur before that call. #[must_use] - pub fn memory_map_size(&self) -> MemoryMapMeta { + fn memory_map_size(&self) -> MemoryMapMeta { let mut map_size = 0; let mut map_key = MemoryMapKey(0); let mut desc_size = 0; @@ -209,12 +209,16 @@ impl BootServices { "Memory map must be a multiple of the reported descriptor size." ); - MemoryMapMeta { + let mmm = MemoryMapMeta { desc_size, map_size, map_key, desc_version, - } + }; + + mmm.assert_sanity_checks(); + + mmm } /// Stores the current UEFI memory map in the provided buffer. @@ -1621,6 +1625,111 @@ impl Align for MemoryDescriptor { #[repr(C)] pub struct MemoryMapKey(usize); +/// The backing memory for the UEFI memory app on the UEFI heap, allocated using +/// the UEFI boot services allocator. This occupied memory will also be +/// reflected in the memory map itself. +/// +/// Although untyped, it is similar to the `Box` type in terms of heap +/// allocation and deallocation, as well as ownership of the corresponding +/// memory. Apart from that, this type only has the semantics of a buffer. +/// +/// The memory is untyped, which is necessary due to the nature of the UEFI +/// spec. It still ensures a correct alignment to hold [`MemoryDescriptor`]. The +/// size of the buffer is sufficient to hold the memory map at the point in time +/// where this is created. Note that due to (not obvious or asynchronous) +/// allocations/deallocations in your environment, this might be outdated at the +/// time you store the memory map in it. +/// +/// Note that due to the nature of the UEFI memory app, this buffer might +/// hold (a few) bytes more than necessary. The `map_size` reported by +/// `get_memory_map` tells the actual size. +/// +/// When this type is dropped and boot services are not exited yet, the memory +/// is freed. +/// +/// # Usage +/// The type is intended to be used like this: +/// 1. create it using [`MemoryMapBackingMemory::new`] +/// 2. pass it to [`BootServices::get_memory_map`] +/// 3. construct a [`MemoryMap`] from it +#[derive(Debug)] +#[allow(clippy::len_without_is_empty)] // this type is never empty +pub(crate) struct MemoryMapBackingMemory(NonNull<[u8]>); + +impl MemoryMapBackingMemory { + /// Constructs a new [`MemoryMapBackingMemory`]. + /// + /// # Parameters + /// - `memory_type`: The memory type for the memory map allocation. + /// Typically, [`MemoryType::LOADER_DATA`] for regular UEFI applications. + pub(crate) fn new(memory_type: MemoryType) -> Result { + let st = system_table_boot().expect("Should have boot services activated"); + let bs = st.boot_services(); + + let memory_map_meta = bs.memory_map_size(); + let len = Self::safe_allocation_size_hint(memory_map_meta); + let ptr = bs.allocate_pool(memory_type, len)?.as_ptr(); + + // Should be fine as UEFI always has allocations with a guaranteed + // alignment of 8 bytes. + assert_eq!(ptr.align_offset(mem::align_of::()), 0); + + // If this panics, the UEFI implementation is broken. + assert_eq!(memory_map_meta.map_size % memory_map_meta.desc_size, 0); + + unsafe { Ok(Self::from_raw(ptr, len)) } + } + + unsafe fn from_raw(ptr: *mut u8, len: usize) -> Self { + assert_eq!(ptr.align_offset(mem::align_of::()), 0); + + let ptr = NonNull::new(ptr).expect("UEFI should never return a null ptr. An error should have been reflected via an Err earlier."); + let slice = NonNull::slice_from_raw_parts(ptr, len); + + Self(slice) + } + + /// Returns a "safe" best-effort size hint for the memory map size with + /// some additional bytes in buffer compared to the [`MemoryMapMeta`]. + /// This helps + #[must_use] + fn safe_allocation_size_hint(mmm: MemoryMapMeta) -> usize { + // Allocate space for extra entries beyond the current size of the + // memory map. The value of 8 matches the value in the Linux kernel: + // https://github.com/torvalds/linux/blob/e544a07438/drivers/firmware/efi/libstub/efistub.h#L173 + const EXTRA_ENTRIES: usize = 8; + + let extra_size = mmm.desc_size * EXTRA_ENTRIES; + mmm.map_size + extra_size + } + + /// Returns the raw pointer to the beginning of the allocation. + pub fn as_ptr_mut(&mut self) -> *mut u8 { + self.0.as_ptr().cast() + } + + /// Returns a mutable slice to the underlying memory. + #[must_use] + pub fn as_mut_slice(&mut self) -> &mut [u8] { + unsafe { self.0.as_mut() } + } +} + +// Don't drop when we use this in unit tests. +#[cfg(not(test))] +impl Drop for MemoryMapBackingMemory { + fn drop(&mut self) { + if let Some(bs) = system_table_boot() { + let res = unsafe { bs.boot_services().free_pool(self.0.as_ptr().cast()) }; + if let Err(e) = res { + log::error!("Failed to deallocate memory map: {e:?}"); + } + } else { + log::debug!("Boot services are excited. Memory map won't be freed using the UEFI boot services allocator."); + } + } +} + /// A structure containing the meta attributes associated with a call to /// `GetMemoryMap` of UEFI. Note that all values refer to the time this was /// called. All following invocations (hidden, subtle, and asynchronous ones) @@ -1645,6 +1754,21 @@ impl MemoryMapMeta { assert_eq!(self.map_size % self.desc_size, 0); self.map_size / self.desc_size } + + /// Runs some sanity assertions. + pub fn assert_sanity_checks(&self) { + assert!(self.desc_size > 0); + // Although very unlikely, this might fail if the memory descriptor is + // extended by a future UEFI revision by a significant amount, we + // update the struct, but an old UEFI implementation reports a small + // size. + assert!(self.desc_size >= mem::size_of::()); + assert!(self.map_size > 0); + + // Ensure the mmap size is (somehow) sane. + const ONE_GB: usize = 1024 * 1024 * 1024; + assert!(self.map_size <= ONE_GB); + } } /// An accessory to the memory map that can be either iterated or From 098fb7724f1182f89ec51a925caa989b5fcc5bf6 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 25 May 2024 23:48:36 +0200 Subject: [PATCH 06/11] uefi: MemoryMap now owns its memory This is an attempt to simplify the overall complex handling of obtaining the UEFI memory map. We have the following pre-requisites and use-cases all to keep in mind when designing the functions and associated helper types: - the memory map itself needs memory; typically on the UEFI heap - acquiring that memory and storing the memory map inside it are two distinct steps - the memory map is not just a slice of [MemoryDescriptor] (desc_size is bigger than size_of) - the required map size can be obtained by a call to a boot service function - the needed memory might change due to hidden or asynchronous allocations between the allocation of a buffer and storing the memory map inside it - when boot services are excited, best practise has shown (looking at linux code) that one should use the same buffer (with some extra capacity) and call exit_boot_services with that buffer at most two times in a loop This makes it hard to come up with an ergonomic solution such as using a Box or any other high-level Rust type. The main simplification of my design is that the MemoryMap type now doesn't has a reference to memory anymore but actually owns it. This also models the real world use case where one typically obtains the memory map once when boot services are exited. A &'static [u8] on the MemoryMap just creates more confusion that it brings any benefit. The MemoryMap now knows whether boot services are still active and frees that memory, or it doesn't if the boot services are exited. This means less fiddling with life-times and less cognitive overhead when - reading the code - calling BootService::memory_map independently of exit_boot_services --- uefi-test-runner/src/boot/memory.rs | 11 +- uefi/CHANGELOG.md | 10 +- uefi/src/table/boot.rs | 152 +++++++++++++++++----------- uefi/src/table/system.rs | 50 ++------- 4 files changed, 110 insertions(+), 113 deletions(-) diff --git a/uefi-test-runner/src/boot/memory.rs b/uefi-test-runner/src/boot/memory.rs index 1855d1fc6..b23b38248 100644 --- a/uefi-test-runner/src/boot/memory.rs +++ b/uefi-test-runner/src/boot/memory.rs @@ -63,17 +63,8 @@ fn alloc_alignment() { fn memory_map(bt: &BootServices) { info!("Testing memory map functions"); - // Get the memory descriptor size and an estimate of the memory map size - let sizes = bt.memory_map_size(); - - // 2 extra descriptors should be enough. - let buf_sz = sizes.map_size + 2 * sizes.desc_size; - - // We will use vectors for convenience. - let mut buffer = vec![0_u8; buf_sz]; - let mut memory_map = bt - .memory_map(&mut buffer) + .memory_map(MemoryType::LOADER_DATA) .expect("Failed to retrieve UEFI memory map"); memory_map.sort(); diff --git a/uefi/CHANGELOG.md b/uefi/CHANGELOG.md index 4e780f6c4..344766e47 100644 --- a/uefi/CHANGELOG.md +++ b/uefi/CHANGELOG.md @@ -16,7 +16,7 @@ - Added `ByteConversionError`. - Re-exported `CapsuleFlags`. - One can now specify in `TimeError` what fields of `Time` are outside its valid - range. `Time::is_valid` has been updated accordingly. + range. `Time::is_valid` has been updated accordingly. ## Changed - `SystemTable::exit_boot_services` is now `unsafe`. See that method's @@ -24,6 +24,14 @@ - `BootServices::allocate_pool` now returns `NonZero` instead of `*mut u8`. - `helpers::system_table` is deprecated, use `table::system_table_boot` instead. +- `BootServices::memory_map` changed its signature from \ + `pub fn memory_map<'buf>(&self, buffer: &'buf mut [u8]) -> Result> {` \ + to \ + `pub fn memory_map(&self, mt: MemoryType) -> Result` + - Allocations now happen automatically internally on the UEFI heap. Also, the + returned type is automatically freed on the UEFI heap, as long as boot + services are not excited. By removing the need for that explicit buffer and + the lifetime, the API is simpler. ## Removed - Removed the `panic-on-logger-errors` feature of the `uefi` crate. Logger diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs index f5bc295b2..006b83a12 100644 --- a/uefi/src/table/boot.rs +++ b/uefi/src/table/boot.rs @@ -221,37 +221,46 @@ impl BootServices { mmm } - /// Stores the current UEFI memory map in the provided buffer. - /// - /// The allocated buffer must be at least aligned to a [`MemoryDescriptor`] - /// and should be big enough to store the whole map. To estimating how big - /// the map will be, you can call [`Self::memory_map_size`]. - /// - /// The memory map contains entries of type [`MemoryDescriptor`]. However, - /// the relevant step size is always the reported `desc_size` but never - /// `size_of::()`. - /// - /// The returned key is a unique identifier of the current configuration of - /// memory. Any allocations or such will change the memory map's key. - /// - /// If you want to store the resulting memory map without having to keep - /// the buffer around, you can use `.copied().collect()` on the iterator. - /// Note that this will change the current memory map again, if the UEFI - /// allocator is used under the hood. + /// Stores the current UEFI memory map in an UEFI-heap allocated buffer + /// and returns a [`MemoryMap`]. /// /// # Errors /// - /// See section `EFI_BOOT_SERVICES.GetMemoryMap()` in the UEFI Specification for more details. + /// See section `EFI_BOOT_SERVICES.GetMemoryMap()` in the UEFI Specification + /// for more details. /// /// * [`uefi::Status::BUFFER_TOO_SMALL`] /// * [`uefi::Status::INVALID_PARAMETER`] - pub fn memory_map<'buf>(&self, buffer: &'buf mut [u8]) -> Result> { - let mut map_size = buffer.len(); - MemoryDescriptor::assert_aligned(buffer); - let map_buffer = buffer.as_mut_ptr().cast::(); + pub fn memory_map(&self, mt: MemoryType) -> Result { + let mut buffer = MemoryMapBackingMemory::new(mt)?; + + let MemoryMapMeta { + map_size, + map_key, + desc_size, + desc_version, + } = self.get_memory_map(buffer.as_mut_slice())?; + + let len = map_size / desc_size; + assert_eq!(map_size % desc_size, 0); + assert_eq!(desc_version, MemoryDescriptor::VERSION); + Ok(MemoryMap { + key: map_key, + buf: buffer, + desc_size, + len, + }) + } + + /// Calls the underlying `GetMemoryMap` function of UEFI. On success, + /// the buffer is mutated and contains the map. The map might be shorter + /// than the buffer, which is reflected by the return value. + pub(crate) fn get_memory_map(&self, buf: &mut [u8]) -> Result { + let mut map_size = buf.len(); + let map_buffer = buf.as_mut_ptr().cast::(); let mut map_key = MemoryMapKey(0); let mut desc_size = 0; - let mut entry_version = 0; + let mut desc_version = 0; assert_eq!( (map_buffer as usize) % mem::align_of::(), @@ -265,18 +274,14 @@ impl BootServices { map_buffer, &mut map_key.0, &mut desc_size, - &mut entry_version, + &mut desc_version, ) } - .to_result_with_val(move || { - let len = map_size / desc_size; - - MemoryMap { - key: map_key, - buf: buffer, - desc_size, - len, - } + .to_result_with_val(|| MemoryMapMeta { + map_size, + desc_size, + map_key, + desc_version, }) } @@ -1689,6 +1694,14 @@ impl MemoryMapBackingMemory { Self(slice) } + /// Creates an instance from the provided memory, which is not necessarily + /// on the UEFI heap. + #[cfg(test)] + fn from_slice(buffer: &mut [u8]) -> Self { + let len = buffer.len(); + unsafe { Self::from_raw(buffer.as_mut_ptr(), len) } + } + /// Returns a "safe" best-effort size hint for the memory map size with /// some additional bytes in buffer compared to the [`MemoryMapMeta`]. /// This helps @@ -1703,8 +1716,15 @@ impl MemoryMapBackingMemory { mmm.map_size + extra_size } - /// Returns the raw pointer to the beginning of the allocation. - pub fn as_ptr_mut(&mut self) -> *mut u8 { + /// Returns a raw pointer to the beginning of the allocation. + #[must_use] + pub fn as_ptr(&self) -> *const u8 { + self.0.as_ptr().cast() + } + + /// Returns a mutable raw pointer to the beginning of the allocation. + #[must_use] + pub fn as_mut_ptr(&mut self) -> *mut u8 { self.0.as_ptr().cast() } @@ -1788,31 +1808,27 @@ impl MemoryMapMeta { /// /// [0]: https://github.com/tianocore/edk2/blob/7142e648416ff5d3eac6c6d607874805f5de0ca8/MdeModulePkg/Core/PiSmmCore/Page.c#L1059 #[derive(Debug)] -pub struct MemoryMap<'buf> { +pub struct MemoryMap { + /// Backing memory, properly initialized at this point. + buf: MemoryMapBackingMemory, key: MemoryMapKey, - buf: &'buf mut [u8], /// Usually bound to the size of a [`MemoryDescriptor`] but can indicate if /// this field is ever extended by a new UEFI standard. desc_size: usize, len: usize, } -impl<'buf> MemoryMap<'buf> { - /// Creates a [`MemoryMap`] from the given buffer and entry size. - /// The entry size is usually bound to the size of a [`MemoryDescriptor`] - /// but can indicate if this field is ever extended by a new UEFI standard. - /// - /// This allows parsing a memory map provided by a kernel after boot - /// services have already exited. - pub fn from_raw(buf: &'buf mut [u8], desc_size: usize) -> Self { - assert!(!buf.is_empty()); - assert_eq!( - buf.len() % desc_size, - 0, - "The buffer length must be a multiple of the desc_size" - ); +impl MemoryMap { + /// Creates a [`MemoryMap`] from the give initialized memory map behind + /// the buffer and the reported `desc_size` from UEFI. + pub(crate) fn from_initialized_mem(buf: MemoryMapBackingMemory, meta: MemoryMapMeta) -> Self { + let MemoryMapMeta { + map_size, + desc_size, + .. + } = meta; assert!(desc_size >= mem::size_of::()); - let len = buf.len() / desc_size; + let len = map_size / desc_size; MemoryMap { key: MemoryMapKey(0), buf, @@ -1821,6 +1837,20 @@ impl<'buf> MemoryMap<'buf> { } } + #[cfg(test)] + fn from_raw(buf: &mut [u8], desc_size: usize) -> Self { + let mem = MemoryMapBackingMemory::from_slice(buf); + Self::from_initialized_mem( + mem, + MemoryMapMeta { + map_size: buf.len(), + desc_size, + map_key: MemoryMapKey(0), + desc_version: MemoryDescriptor::VERSION, + }, + ) + } + #[must_use] /// Returns the unique [`MemoryMapKey`] associated with the memory map. pub fn key(&self) -> MemoryMapKey { @@ -1915,7 +1945,7 @@ impl<'buf> MemoryMap<'buf> { /// Returns a reference to the [`MemoryDescriptor`] at `index` or `None` if out of bounds. #[must_use] - pub fn get(&self, index: usize) -> Option<&'buf MemoryDescriptor> { + pub fn get(&self, index: usize) -> Option<&MemoryDescriptor> { if index >= self.len { return None; } @@ -1933,7 +1963,7 @@ impl<'buf> MemoryMap<'buf> { /// Returns a mut reference to the [`MemoryDescriptor`] at `index` or `None` if out of bounds. #[must_use] - pub fn get_mut(&mut self, index: usize) -> Option<&'buf mut MemoryDescriptor> { + pub fn get_mut(&mut self, index: usize) -> Option<&mut MemoryDescriptor> { if index >= self.len { return None; } @@ -1950,7 +1980,7 @@ impl<'buf> MemoryMap<'buf> { } } -impl core::ops::Index for MemoryMap<'_> { +impl core::ops::Index for MemoryMap { type Output = MemoryDescriptor; fn index(&self, index: usize) -> &Self::Output { @@ -1958,7 +1988,7 @@ impl core::ops::Index for MemoryMap<'_> { } } -impl core::ops::IndexMut for MemoryMap<'_> { +impl core::ops::IndexMut for MemoryMap { fn index_mut(&mut self, index: usize) -> &mut Self::Output { self.get_mut(index).unwrap() } @@ -1967,13 +1997,13 @@ impl core::ops::IndexMut for MemoryMap<'_> { /// An iterator of [`MemoryDescriptor`]. The underlying memory map is always /// associated with a unique [`MemoryMapKey`]. #[derive(Debug, Clone)] -pub struct MemoryMapIter<'buf> { - memory_map: &'buf MemoryMap<'buf>, +pub struct MemoryMapIter<'a> { + memory_map: &'a MemoryMap, index: usize, } -impl<'buf> Iterator for MemoryMapIter<'buf> { - type Item = &'buf MemoryDescriptor; +impl<'a> Iterator for MemoryMapIter<'a> { + type Item = &'a MemoryDescriptor; fn size_hint(&self) -> (usize, Option) { let sz = self.memory_map.len - self.index; @@ -2224,7 +2254,7 @@ mod tests { } // Added for debug purposes on test failure - impl core::fmt::Display for MemoryMap<'_> { + impl core::fmt::Display for MemoryMap { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { writeln!(f)?; for desc in self.entries() { diff --git a/uefi/src/table/system.rs b/uefi/src/table/system.rs index e27082a2a..014169110 100644 --- a/uefi/src/table/system.rs +++ b/uefi/src/table/system.rs @@ -2,6 +2,7 @@ use core::ffi::c_void; use core::marker::PhantomData; use core::ptr::NonNull; use core::slice; +use uefi::table::boot::{MemoryMapBackingMemory, MemoryMapMeta}; use crate::proto::console::text; use crate::{CStr16, Result, Status, StatusExt}; @@ -151,33 +152,15 @@ impl SystemTable { unsafe { &*(*self.table).boot_services.cast_const().cast() } } - /// Get the size in bytes of the buffer to allocate for storing the memory - /// map in `exit_boot_services`. - /// - /// This map contains some extra room to avoid needing to allocate more than - /// once. - /// - /// Returns `None` on overflow. - fn memory_map_size_for_exit_boot_services(&self) -> Option { - // Allocate space for extra entries beyond the current size of the - // memory map. The value of 8 matches the value in the Linux kernel: - // https://github.com/torvalds/linux/blob/e544a07438/drivers/firmware/efi/libstub/efistub.h#L173 - let extra_entries = 8; - - let memory_map_size = self.boot_services().memory_map_size(); - let extra_size = memory_map_size.desc_size.checked_mul(extra_entries)?; - memory_map_size.map_size.checked_add(extra_size) - } - /// Get the current memory map and exit boot services. unsafe fn get_memory_map_and_exit_boot_services( &self, - buf: &'static mut [u8], - ) -> Result> { + buf: &mut [u8], + ) -> Result { let boot_services = self.boot_services(); // Get the memory map. - let memory_map = boot_services.memory_map(buf)?; + let memory_map = boot_services.get_memory_map(buf)?; // Try to exit boot services using the memory map key. Note that after // the first call to `exit_boot_services`, there are restrictions on @@ -185,7 +168,7 @@ impl SystemTable { // only `get_memory_map` and `exit_boot_services` are allowed. Starting // in UEFI 2.9 other memory allocation functions may also be called. boot_services - .exit_boot_services(boot_services.image_handle(), memory_map.key()) + .exit_boot_services(boot_services.image_handle(), memory_map.map_key) .map(move |()| memory_map) } @@ -247,11 +230,9 @@ impl SystemTable { pub unsafe fn exit_boot_services( self, memory_type: MemoryType, - ) -> (SystemTable, MemoryMap<'static>) { + ) -> (SystemTable, MemoryMap) { crate::helpers::exit(); - let boot_services = self.boot_services(); - // Reboot the device. let reset = |status| -> ! { { @@ -260,19 +241,7 @@ impl SystemTable { } }; - // Get the size of the buffer to allocate. If that calculation - // overflows treat it as an unrecoverable error. - let buf_size = match self.memory_map_size_for_exit_boot_services() { - Some(buf_size) => buf_size, - None => reset(Status::ABORTED), - }; - - // Allocate a byte slice to hold the memory map. If the - // allocation fails treat it as an unrecoverable error. - let buf: *mut u8 = match boot_services.allocate_pool(memory_type, buf_size) { - Ok(buf) => buf.as_ptr(), - Err(err) => reset(err.status()), - }; + let mut buf = MemoryMapBackingMemory::new(memory_type).expect("Failed to allocate memory"); // Calling `exit_boot_services` can fail if the memory map key is not // current. Retry a second time if that occurs. This matches the @@ -280,14 +249,13 @@ impl SystemTable { // https://github.com/torvalds/linux/blob/e544a0743/drivers/firmware/efi/libstub/efi-stub-helper.c#L375 let mut status = Status::ABORTED; for _ in 0..2 { - let buf: &mut [u8] = unsafe { slice::from_raw_parts_mut(buf, buf_size) }; - match unsafe { self.get_memory_map_and_exit_boot_services(buf) } { + match unsafe { self.get_memory_map_and_exit_boot_services(buf.as_mut_slice()) } { Ok(memory_map) => { let st = SystemTable { table: self.table, _marker: PhantomData, }; - return (st, memory_map); + return (st, MemoryMap::from_initialized_mem(buf, memory_map)); } Err(err) => { log::error!("Error retrieving the memory map for exiting the boot services"); From d864ce7a96aeff65bf8c986c20304d17b084a326 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Fri, 21 Jun 2024 10:50:30 +0200 Subject: [PATCH 07/11] uefi: use MemoryMapMeta in MemoryMap This prepares the next commit. --- uefi/src/table/boot.rs | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs index 006b83a12..e230fb0ee 100644 --- a/uefi/src/table/boot.rs +++ b/uefi/src/table/boot.rs @@ -234,12 +234,13 @@ impl BootServices { pub fn memory_map(&self, mt: MemoryType) -> Result { let mut buffer = MemoryMapBackingMemory::new(mt)?; + let meta = self.get_memory_map(buffer.as_mut_slice())?; let MemoryMapMeta { map_size, map_key, desc_size, desc_version, - } = self.get_memory_map(buffer.as_mut_slice())?; + } = meta; let len = map_size / desc_size; assert_eq!(map_size % desc_size, 0); @@ -247,7 +248,7 @@ impl BootServices { Ok(MemoryMap { key: map_key, buf: buffer, - desc_size, + meta, len, }) } @@ -1812,9 +1813,7 @@ pub struct MemoryMap { /// Backing memory, properly initialized at this point. buf: MemoryMapBackingMemory, key: MemoryMapKey, - /// Usually bound to the size of a [`MemoryDescriptor`] but can indicate if - /// this field is ever extended by a new UEFI standard. - desc_size: usize, + meta: MemoryMapMeta, len: usize, } @@ -1822,17 +1821,12 @@ impl MemoryMap { /// Creates a [`MemoryMap`] from the give initialized memory map behind /// the buffer and the reported `desc_size` from UEFI. pub(crate) fn from_initialized_mem(buf: MemoryMapBackingMemory, meta: MemoryMapMeta) -> Self { - let MemoryMapMeta { - map_size, - desc_size, - .. - } = meta; - assert!(desc_size >= mem::size_of::()); - let len = map_size / desc_size; + assert!(meta.desc_size >= mem::size_of::()); + let len = meta.entry_count(); MemoryMap { key: MemoryMapKey(0), buf, - desc_size, + meta, len, } } @@ -1914,15 +1908,15 @@ impl MemoryMap { unsafe { ptr::swap_nonoverlapping( - base.add(index1 * self.desc_size), - base.add(index2 * self.desc_size), - self.desc_size, + base.add(index1 * self.meta.desc_size), + base.add(index2 * self.meta.desc_size), + self.meta.desc_size, ); } } fn get_element_phys_addr(&self, index: usize) -> PhysicalAddress { - let offset = index.checked_mul(self.desc_size).unwrap(); + let offset = index.checked_mul(self.meta.desc_size).unwrap(); let elem = unsafe { &*self.buf.as_ptr().add(offset).cast::() }; elem.phys_start } @@ -1954,7 +1948,7 @@ impl MemoryMap { &*self .buf .as_ptr() - .add(self.desc_size * index) + .add(self.meta.desc_size * index) .cast::() }; @@ -1972,7 +1966,7 @@ impl MemoryMap { &mut *self .buf .as_mut_ptr() - .add(self.desc_size * index) + .add(self.meta.desc_size * index) .cast::() }; From c511c52d552d04eb88aa5ce40ec116701b15d93c Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Fri, 21 Jun 2024 10:50:37 +0200 Subject: [PATCH 08/11] uefi: add MemoryMap::raw --- uefi/CHANGELOG.md | 3 +++ uefi/src/table/boot.rs | 15 +++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/uefi/CHANGELOG.md b/uefi/CHANGELOG.md index 344766e47..975680c24 100644 --- a/uefi/CHANGELOG.md +++ b/uefi/CHANGELOG.md @@ -17,6 +17,9 @@ - Re-exported `CapsuleFlags`. - One can now specify in `TimeError` what fields of `Time` are outside its valid range. `Time::is_valid` has been updated accordingly. +- `MemoryMap::as_raw` which provides raw access to the memory map. This is for + example useful if you create your own Multiboot2 bootloader that embeds the + EFI mmap in a Multiboot2 boot information structure. ## Changed - `SystemTable::exit_boot_services` is now `unsafe`. See that method's diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs index e230fb0ee..a279aa022 100644 --- a/uefi/src/table/boot.rs +++ b/uefi/src/table/boot.rs @@ -1729,6 +1729,12 @@ impl MemoryMapBackingMemory { self.0.as_ptr().cast() } + /// Returns a slice to the underlying memory. + #[must_use] + pub fn as_slice(&self) -> &[u8] { + unsafe { self.0.as_ref() } + } + /// Returns a mutable slice to the underlying memory. #[must_use] pub fn as_mut_slice(&mut self) -> &mut [u8] { @@ -1972,6 +1978,15 @@ impl MemoryMap { Some(desc) } + + /// Provides access to the raw memory map. + /// + /// This is for example useful if you want to embed the memory map into + /// another data structure, such as a Multiboot2 boot information. + #[must_use] + pub fn as_raw(&self) -> (&[u8], MemoryMapMeta) { + (self.buf.as_slice(), self.meta) + } } impl core::ops::Index for MemoryMap { From 55a0ba67406e75d1d923adcb4a49ce383a2ff4fc Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sun, 26 May 2024 11:58:37 +0200 Subject: [PATCH 09/11] uefi: add memory map unit test based on real-world data --- uefi/src/table/boot.rs | 137 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 136 insertions(+), 1 deletion(-) diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs index a279aa022..3b59c4784 100644 --- a/uefi/src/table/boot.rs +++ b/uefi/src/table/boot.rs @@ -2161,7 +2161,7 @@ impl<'a> HandleBuffer<'a> { pub struct ProtocolSearchKey(NonNull); #[cfg(test)] -mod tests { +mod tests_mmap_artificial { use core::mem::{size_of, size_of_val}; use crate::table::boot::{MemoryAttribute, MemoryMap, MemoryType}; @@ -2292,3 +2292,138 @@ mod tests { true } } + +#[cfg(test)] +mod tests_mmap_real { + use super::*; + use core::mem::size_of; + + const MMAP_META: MemoryMapMeta = MemoryMapMeta { + map_size: MMAP_RAW.len() * size_of::(), + desc_size: 48, + map_key: MemoryMapKey(0), + desc_version: 1, + }; + /// Sample with 10 entries of a real UEFI memory map extracted from our + /// UEFI test runner. + const MMAP_RAW: [u64; 60] = [ + 3, 0, 0, 1, 15, 0, 7, 4096, 0, 134, 15, 0, 4, 552960, 0, 1, 15, 0, 7, 557056, 0, 24, 15, 0, + 7, 1048576, 0, 1792, 15, 0, 10, 8388608, 0, 8, 15, 0, 7, 8421376, 0, 3, 15, 0, 10, 8433664, + 0, 1, 15, 0, 7, 8437760, 0, 4, 15, 0, 10, 8454144, 0, 240, 15, 0, + ]; + extern crate std; + #[test] + fn basic_functionality() { + let mut buf = MMAP_RAW; + let buf = + unsafe { slice::from_raw_parts_mut(buf.as_mut_ptr().cast::(), MMAP_META.map_size) }; + let mut mmap = MemoryMap::from_raw(buf, MMAP_META.desc_size); + mmap.sort(); + + let entries = mmap.entries().copied().collect::>(); + + let expected = [ + MemoryDescriptor { + ty: MemoryType::BOOT_SERVICES_CODE, + phys_start: 0x0, + virt_start: 0x0, + page_count: 0x1, + att: MemoryAttribute::UNCACHEABLE + | MemoryAttribute::WRITE_COMBINE + | MemoryAttribute::WRITE_THROUGH + | MemoryAttribute::WRITE_BACK, + }, + MemoryDescriptor { + ty: MemoryType::CONVENTIONAL, + phys_start: 0x1000, + virt_start: 0x0, + page_count: 0x86, + att: MemoryAttribute::UNCACHEABLE + | MemoryAttribute::WRITE_COMBINE + | MemoryAttribute::WRITE_THROUGH + | MemoryAttribute::WRITE_BACK, + }, + MemoryDescriptor { + ty: MemoryType::BOOT_SERVICES_DATA, + phys_start: 0x87000, + virt_start: 0x0, + page_count: 0x1, + att: MemoryAttribute::UNCACHEABLE + | MemoryAttribute::WRITE_COMBINE + | MemoryAttribute::WRITE_THROUGH + | MemoryAttribute::WRITE_BACK, + }, + MemoryDescriptor { + ty: MemoryType::CONVENTIONAL, + phys_start: 0x88000, + virt_start: 0x0, + page_count: 0x18, + att: MemoryAttribute::UNCACHEABLE + | MemoryAttribute::WRITE_COMBINE + | MemoryAttribute::WRITE_THROUGH + | MemoryAttribute::WRITE_BACK, + }, + MemoryDescriptor { + ty: MemoryType::CONVENTIONAL, + phys_start: 0x100000, + virt_start: 0x0, + page_count: 0x700, + att: MemoryAttribute::UNCACHEABLE + | MemoryAttribute::WRITE_COMBINE + | MemoryAttribute::WRITE_THROUGH + | MemoryAttribute::WRITE_BACK, + }, + MemoryDescriptor { + ty: MemoryType::ACPI_NON_VOLATILE, + phys_start: 0x800000, + virt_start: 0x0, + page_count: 0x8, + att: MemoryAttribute::UNCACHEABLE + | MemoryAttribute::WRITE_COMBINE + | MemoryAttribute::WRITE_THROUGH + | MemoryAttribute::WRITE_BACK, + }, + MemoryDescriptor { + ty: MemoryType::CONVENTIONAL, + phys_start: 0x808000, + virt_start: 0x0, + page_count: 0x3, + att: MemoryAttribute::UNCACHEABLE + | MemoryAttribute::WRITE_COMBINE + | MemoryAttribute::WRITE_THROUGH + | MemoryAttribute::WRITE_BACK, + }, + MemoryDescriptor { + ty: MemoryType::ACPI_NON_VOLATILE, + phys_start: 0x80b000, + virt_start: 0x0, + page_count: 0x1, + att: MemoryAttribute::UNCACHEABLE + | MemoryAttribute::WRITE_COMBINE + | MemoryAttribute::WRITE_THROUGH + | MemoryAttribute::WRITE_BACK, + }, + MemoryDescriptor { + ty: MemoryType::CONVENTIONAL, + phys_start: 0x80c000, + virt_start: 0x0, + page_count: 0x4, + att: MemoryAttribute::UNCACHEABLE + | MemoryAttribute::WRITE_COMBINE + | MemoryAttribute::WRITE_THROUGH + | MemoryAttribute::WRITE_BACK, + }, + MemoryDescriptor { + ty: MemoryType::ACPI_NON_VOLATILE, + phys_start: 0x810000, + virt_start: 0x0, + page_count: 0xf0, + att: MemoryAttribute::UNCACHEABLE + | MemoryAttribute::WRITE_COMBINE + | MemoryAttribute::WRITE_THROUGH + | MemoryAttribute::WRITE_BACK, + }, + ]; + assert_eq!(entries.as_slice(), &expected); + } +} From b4d70beffb627fb380b52eeca934bb31db13a8af Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Fri, 21 Jun 2024 11:28:37 +0200 Subject: [PATCH 10/11] uefi: improve doc --- uefi/src/lib.rs | 3 ++- uefi/src/table/boot.rs | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/uefi/src/lib.rs b/uefi/src/lib.rs index eef044920..fb658d7c3 100644 --- a/uefi/src/lib.rs +++ b/uefi/src/lib.rs @@ -48,7 +48,8 @@ //! the Rust standard library. For example, methods that return a //! `Vec` rather than filling a statically-sized array. This requires //! a global allocator; you can use the `global_allocator` feature or -//! provide your own. +//! provide your own. This is independent of internal direct usages of the +//! UEFI boot service allocator which may happen anyway, where necessary. //! - `global_allocator`: Set [`allocator::Allocator`] as the global Rust //! allocator. This is a simple allocator that relies on the UEFI pool //! allocator. You can choose to provide your own allocator instead of diff --git a/uefi/src/table/boot.rs b/uefi/src/table/boot.rs index 3b59c4784..1a27de37c 100644 --- a/uefi/src/table/boot.rs +++ b/uefi/src/table/boot.rs @@ -224,6 +224,12 @@ impl BootServices { /// Stores the current UEFI memory map in an UEFI-heap allocated buffer /// and returns a [`MemoryMap`]. /// + /// # Parameters + /// + /// - `mt`: The memory type for the backing memory on the UEFI heap. + /// Usually, this is [`MemoryType::LOADER_DATA`]. You can also use a + /// custom type. + /// /// # Errors /// /// See section `EFI_BOOT_SERVICES.GetMemoryMap()` in the UEFI Specification From e47e36c1fca1597e84095d1332c203678f192043 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sun, 26 May 2024 00:19:18 +0200 Subject: [PATCH 11/11] uefi-test-runner: test that BootService::memory_map memory is freed This is now a benefit compared to the old API. This wasn't possible. --- uefi-test-runner/src/boot/memory.rs | 51 ++++++++++++++++------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/uefi-test-runner/src/boot/memory.rs b/uefi-test-runner/src/boot/memory.rs index b23b38248..133110b43 100644 --- a/uefi-test-runner/src/boot/memory.rs +++ b/uefi-test-runner/src/boot/memory.rs @@ -63,36 +63,41 @@ fn alloc_alignment() { fn memory_map(bt: &BootServices) { info!("Testing memory map functions"); - let mut memory_map = bt - .memory_map(MemoryType::LOADER_DATA) - .expect("Failed to retrieve UEFI memory map"); + // Ensure that the memory map is freed after each iteration (on drop). + // Otherwise, we will have an OOM. + for _ in 0..200000 { + let mut memory_map = bt + .memory_map(MemoryType::LOADER_DATA) + .expect("Failed to retrieve UEFI memory map"); - memory_map.sort(); + memory_map.sort(); - // Collect the descriptors into a vector - let descriptors = memory_map.entries().copied().collect::>(); + // Collect the descriptors into a vector + let descriptors = memory_map.entries().copied().collect::>(); - // Ensured we have at least one entry. - // Real memory maps usually have dozens of entries. - assert!(!descriptors.is_empty(), "Memory map is empty"); + // Ensured we have at least one entry. + // Real memory maps usually have dozens of entries. + assert!(!descriptors.is_empty(), "Memory map is empty"); - let mut curr_value = descriptors[0]; + let mut curr_value = descriptors[0]; - for value in descriptors.iter().skip(1) { - if value.phys_start <= curr_value.phys_start { - panic!("memory map sorting failed"); + for value in descriptors.iter().skip(1) { + if value.phys_start <= curr_value.phys_start { + panic!("memory map sorting failed"); + } + curr_value = *value; } - curr_value = *value; - } - // This is pretty much a sanity test to ensure returned memory isn't filled with random values. - let first_desc = descriptors[0]; + // This is pretty much a basic sanity test to ensure returned memory + // isn't filled with random values. + let first_desc = descriptors[0]; - #[cfg(target_arch = "x86_64")] - { - let phys_start = first_desc.phys_start; - assert_eq!(phys_start, 0, "Memory does not start at address 0"); + #[cfg(target_arch = "x86_64")] + { + let phys_start = first_desc.phys_start; + assert_eq!(phys_start, 0, "Memory does not start at address 0"); + } + let page_count = first_desc.page_count; + assert!(page_count != 0, "Memory map entry has size zero"); } - let page_count = first_desc.page_count; - assert!(page_count != 0, "Memory map entry has zero size"); }