Skip to content

Commit 9e427ce

Browse files
committed
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
1 parent 3f3ce1e commit 9e427ce

File tree

4 files changed

+110
-113
lines changed

4 files changed

+110
-113
lines changed

uefi-test-runner/src/boot/memory.rs

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -63,17 +63,8 @@ fn alloc_alignment() {
6363
fn memory_map(bt: &BootServices) {
6464
info!("Testing memory map functions");
6565

66-
// Get the memory descriptor size and an estimate of the memory map size
67-
let sizes = bt.memory_map_size();
68-
69-
// 2 extra descriptors should be enough.
70-
let buf_sz = sizes.map_size + 2 * sizes.desc_size;
71-
72-
// We will use vectors for convenience.
73-
let mut buffer = vec![0_u8; buf_sz];
74-
7566
let mut memory_map = bt
76-
.memory_map(&mut buffer)
67+
.memory_map(MemoryType::LOADER_DATA)
7768
.expect("Failed to retrieve UEFI memory map");
7869

7970
memory_map.sort();

uefi/CHANGELOG.md

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,22 @@
1616
- Added `ByteConversionError`.
1717
- Re-exported `CapsuleFlags`.
1818
- One can now specify in `TimeError` what fields of `Time` are outside its valid
19-
range. `Time::is_valid` has been updated accordingly.
19+
range. `Time::is_valid` has been updated accordingly.
2020

2121
## Changed
2222
- `SystemTable::exit_boot_services` is now `unsafe`. See that method's
2323
documentation for details of obligations for callers.
2424
- `BootServices::allocate_pool` now returns `NonZero<u8>` instead of
2525
`*mut u8`.
2626
- `helpers::system_table` is deprecated, use `table::system_table_boot` instead.
27+
- `BootServices::memory_map` changed its signature from \
28+
`pub fn memory_map<'buf>(&self, buffer: &'buf mut [u8]) -> Result<MemoryMap<'buf>> {` \
29+
to \
30+
`pub fn memory_map(&self, mt: MemoryType) -> Result<MemoryMap>`
31+
- Allocations now happen automatically internally on the UEFI heap. Also, the
32+
returned type is automatically freed on the UEFI heap, as long as boot
33+
services are not excited. By removing the need for that explicit buffer and
34+
the lifetime, the API is simpler.
2735

2836
## Removed
2937
- Removed the `panic-on-logger-errors` feature of the `uefi` crate. Logger

uefi/src/table/boot.rs

Lines changed: 91 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -217,37 +217,46 @@ impl BootServices {
217217
}
218218
}
219219

220-
/// Stores the current UEFI memory map in the provided buffer.
221-
///
222-
/// The allocated buffer must be at least aligned to a [`MemoryDescriptor`]
223-
/// and should be big enough to store the whole map. To estimating how big
224-
/// the map will be, you can call [`Self::memory_map_size`].
225-
///
226-
/// The memory map contains entries of type [`MemoryDescriptor`]. However,
227-
/// the relevant step size is always the reported `desc_size` but never
228-
/// `size_of::<MemoryDescriptor>()`.
229-
///
230-
/// The returned key is a unique identifier of the current configuration of
231-
/// memory. Any allocations or such will change the memory map's key.
232-
///
233-
/// If you want to store the resulting memory map without having to keep
234-
/// the buffer around, you can use `.copied().collect()` on the iterator.
235-
/// Note that this will change the current memory map again, if the UEFI
236-
/// allocator is used under the hood.
220+
/// Stores the current UEFI memory map in an UEFI-heap allocated buffer
221+
/// and returns a [`MemoryMap`].
237222
///
238223
/// # Errors
239224
///
240-
/// See section `EFI_BOOT_SERVICES.GetMemoryMap()` in the UEFI Specification for more details.
225+
/// See section `EFI_BOOT_SERVICES.GetMemoryMap()` in the UEFI Specification
226+
/// for more details.
241227
///
242228
/// * [`uefi::Status::BUFFER_TOO_SMALL`]
243229
/// * [`uefi::Status::INVALID_PARAMETER`]
244-
pub fn memory_map<'buf>(&self, buffer: &'buf mut [u8]) -> Result<MemoryMap<'buf>> {
245-
let mut map_size = buffer.len();
246-
MemoryDescriptor::assert_aligned(buffer);
247-
let map_buffer = buffer.as_mut_ptr().cast::<MemoryDescriptor>();
230+
pub fn memory_map(&self, mt: MemoryType) -> Result<MemoryMap> {
231+
let mut buffer = MemoryMapBackingMemory::new(mt)?;
232+
233+
let MemoryMapMeta {
234+
map_size,
235+
map_key,
236+
desc_size,
237+
desc_version,
238+
} = self.get_memory_map(buffer.as_mut_slice())?;
239+
240+
let len = map_size / desc_size;
241+
assert_eq!(map_size % desc_size, 0);
242+
assert_eq!(desc_version, MemoryDescriptor::VERSION);
243+
Ok(MemoryMap {
244+
key: map_key,
245+
buf: buffer,
246+
desc_size,
247+
len,
248+
})
249+
}
250+
251+
/// Calls the underlying `GetMemoryMap` function of UEFI. On success,
252+
/// the buffer is mutated and contains the map. The map might be shorter
253+
/// than the buffer, which is reflected by the return value.
254+
fn get_memory_map(&self, buf: &mut [u8]) -> Result<MemoryMapMeta> {
255+
let mut map_size = buf.len();
256+
let map_buffer = buf.as_mut_ptr().cast::<MemoryDescriptor>();
248257
let mut map_key = MemoryMapKey(0);
249258
let mut desc_size = 0;
250-
let mut entry_version = 0;
259+
let mut desc_version = 0;
251260

252261
assert_eq!(
253262
(map_buffer as usize) % mem::align_of::<MemoryDescriptor>(),
@@ -261,18 +270,14 @@ impl BootServices {
261270
map_buffer,
262271
&mut map_key.0,
263272
&mut desc_size,
264-
&mut entry_version,
273+
&mut desc_version,
265274
)
266275
}
267-
.to_result_with_val(move || {
268-
let len = map_size / desc_size;
269-
270-
MemoryMap {
271-
key: map_key,
272-
buf: buffer,
273-
desc_size,
274-
len,
275-
}
276+
.to_result_with_val(|| MemoryMapMeta {
277+
map_size,
278+
desc_size,
279+
map_key,
280+
desc_version,
276281
})
277282
}
278283

@@ -1685,6 +1690,14 @@ impl MemoryMapBackingMemory {
16851690
Self(slice)
16861691
}
16871692

1693+
/// Creates an instance from the provided memory, which is not necessarily
1694+
/// on the UEFI heap.
1695+
#[cfg(test)]
1696+
fn from_slice(buffer: &mut [u8]) -> Self {
1697+
let len = buffer.len();
1698+
unsafe { Self::from_raw(buffer.as_mut_ptr(), len) }
1699+
}
1700+
16881701
/// Returns a best-effort size hint of the memory map size. This is
16891702
/// especially created with exiting boot services in mind.
16901703
#[must_use]
@@ -1716,8 +1729,15 @@ impl MemoryMapBackingMemory {
17161729
map_size + extra_size
17171730
}
17181731

1719-
/// Returns the raw pointer to the beginning of the allocation.
1720-
pub fn as_ptr_mut(&mut self) -> *mut u8 {
1732+
/// Returns a raw pointer to the beginning of the allocation.
1733+
#[must_use]
1734+
pub fn as_ptr(&self) -> *const u8 {
1735+
self.0.as_ptr().cast()
1736+
}
1737+
1738+
/// Returns a mutable raw pointer to the beginning of the allocation.
1739+
#[must_use]
1740+
pub fn as_mut_ptr(&mut self) -> *mut u8 {
17211741
self.0.as_ptr().cast()
17221742
}
17231743

@@ -1786,31 +1806,27 @@ impl MemoryMapMeta {
17861806
///
17871807
/// [0]: https://github.com/tianocore/edk2/blob/7142e648416ff5d3eac6c6d607874805f5de0ca8/MdeModulePkg/Core/PiSmmCore/Page.c#L1059
17881808
#[derive(Debug)]
1789-
pub struct MemoryMap<'buf> {
1809+
pub struct MemoryMap {
1810+
/// Backing memory, properly initialized at this point.
1811+
buf: MemoryMapBackingMemory,
17901812
key: MemoryMapKey,
1791-
buf: &'buf mut [u8],
17921813
/// Usually bound to the size of a [`MemoryDescriptor`] but can indicate if
17931814
/// this field is ever extended by a new UEFI standard.
17941815
desc_size: usize,
17951816
len: usize,
17961817
}
17971818

1798-
impl<'buf> MemoryMap<'buf> {
1799-
/// Creates a [`MemoryMap`] from the given buffer and entry size.
1800-
/// The entry size is usually bound to the size of a [`MemoryDescriptor`]
1801-
/// but can indicate if this field is ever extended by a new UEFI standard.
1802-
///
1803-
/// This allows parsing a memory map provided by a kernel after boot
1804-
/// services have already exited.
1805-
pub fn from_raw(buf: &'buf mut [u8], desc_size: usize) -> Self {
1806-
assert!(!buf.is_empty());
1807-
assert_eq!(
1808-
buf.len() % desc_size,
1809-
0,
1810-
"The buffer length must be a multiple of the desc_size"
1811-
);
1819+
impl MemoryMap {
1820+
/// Creates a [`MemoryMap`] from the give initialized memory map behind
1821+
/// the buffer and the reported `desc_size` from UEFI.
1822+
pub(crate) fn from_initialized_mem(buf: MemoryMapBackingMemory, meta: MemoryMapMeta) -> Self {
1823+
let MemoryMapMeta {
1824+
map_size,
1825+
desc_size,
1826+
..
1827+
} = meta;
18121828
assert!(desc_size >= mem::size_of::<MemoryDescriptor>());
1813-
let len = buf.len() / desc_size;
1829+
let len = map_size / desc_size;
18141830
MemoryMap {
18151831
key: MemoryMapKey(0),
18161832
buf,
@@ -1819,6 +1835,20 @@ impl<'buf> MemoryMap<'buf> {
18191835
}
18201836
}
18211837

1838+
#[cfg(test)]
1839+
fn from_raw(buf: &mut [u8], desc_size: usize) -> Self {
1840+
let mem = MemoryMapBackingMemory::from_slice(buf);
1841+
Self::from_initialized_mem(
1842+
mem,
1843+
MemoryMapMeta {
1844+
map_size: buf.len(),
1845+
desc_size,
1846+
map_key: MemoryMapKey(0),
1847+
desc_version: MemoryDescriptor::VERSION,
1848+
},
1849+
)
1850+
}
1851+
18221852
#[must_use]
18231853
/// Returns the unique [`MemoryMapKey`] associated with the memory map.
18241854
pub fn key(&self) -> MemoryMapKey {
@@ -1913,7 +1943,7 @@ impl<'buf> MemoryMap<'buf> {
19131943

19141944
/// Returns a reference to the [`MemoryDescriptor`] at `index` or `None` if out of bounds.
19151945
#[must_use]
1916-
pub fn get(&self, index: usize) -> Option<&'buf MemoryDescriptor> {
1946+
pub fn get(&self, index: usize) -> Option<&MemoryDescriptor> {
19171947
if index >= self.len {
19181948
return None;
19191949
}
@@ -1931,7 +1961,7 @@ impl<'buf> MemoryMap<'buf> {
19311961

19321962
/// Returns a mut reference to the [`MemoryDescriptor`] at `index` or `None` if out of bounds.
19331963
#[must_use]
1934-
pub fn get_mut(&mut self, index: usize) -> Option<&'buf mut MemoryDescriptor> {
1964+
pub fn get_mut(&mut self, index: usize) -> Option<&mut MemoryDescriptor> {
19351965
if index >= self.len {
19361966
return None;
19371967
}
@@ -1948,15 +1978,15 @@ impl<'buf> MemoryMap<'buf> {
19481978
}
19491979
}
19501980

1951-
impl core::ops::Index<usize> for MemoryMap<'_> {
1981+
impl core::ops::Index<usize> for MemoryMap {
19521982
type Output = MemoryDescriptor;
19531983

19541984
fn index(&self, index: usize) -> &Self::Output {
19551985
self.get(index).unwrap()
19561986
}
19571987
}
19581988

1959-
impl core::ops::IndexMut<usize> for MemoryMap<'_> {
1989+
impl core::ops::IndexMut<usize> for MemoryMap {
19601990
fn index_mut(&mut self, index: usize) -> &mut Self::Output {
19611991
self.get_mut(index).unwrap()
19621992
}
@@ -1965,13 +1995,13 @@ impl core::ops::IndexMut<usize> for MemoryMap<'_> {
19651995
/// An iterator of [`MemoryDescriptor`]. The underlying memory map is always
19661996
/// associated with a unique [`MemoryMapKey`].
19671997
#[derive(Debug, Clone)]
1968-
pub struct MemoryMapIter<'buf> {
1969-
memory_map: &'buf MemoryMap<'buf>,
1998+
pub struct MemoryMapIter<'a> {
1999+
memory_map: &'a MemoryMap,
19702000
index: usize,
19712001
}
19722002

1973-
impl<'buf> Iterator for MemoryMapIter<'buf> {
1974-
type Item = &'buf MemoryDescriptor;
2003+
impl<'a> Iterator for MemoryMapIter<'a> {
2004+
type Item = &'a MemoryDescriptor;
19752005

19762006
fn size_hint(&self) -> (usize, Option<usize>) {
19772007
let sz = self.memory_map.len - self.index;
@@ -2222,7 +2252,7 @@ mod tests {
22222252
}
22232253

22242254
// Added for debug purposes on test failure
2225-
impl core::fmt::Display for MemoryMap<'_> {
2255+
impl core::fmt::Display for MemoryMap {
22262256
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
22272257
writeln!(f)?;
22282258
for desc in self.entries() {

0 commit comments

Comments
 (0)