Add Sorted Iterator for the UEFI Memory Map (Issue#661) #662

JarlEvanson · 2023-02-18T15:03:03Z

Added a sorted iterator and a method to convert a normal iterator to it.

phip1611 · 2023-02-18T17:20:39Z

@nicholasbishop I think we want a test for this. Do you think a unit-test with a "fake memory map" is the way to go, an integration test, or both?

I'd prefer a minimal unit test at least and an integration test in addition.

nicholasbishop · 2023-02-18T18:37:48Z

I have an alternative suggestion for how to implement this. While I agree with #661 (comment) that usually there shouldn't be a large number of entries so the speed of the sort shouldn't matter that much, I think we can avoid having to make that assumption while also just making the API nicer to use in general.

General outline:
Add a new type for the return value of BootServices::memory_map, something like:

pub struct MemoryMap<'a> {
    key: MemoryMapKey,
    buf: &'a mut [u8],
    entry_size: usize,
    len: usize,
}

impl<'a> MemoryMap<'a> {
    pub fn key(&self) -> MemoryMapKey { ... }
    pub fn sort(&mut self) { ... }
    pub fn entries(&self) -> MemoryMapIter { ... }
}

The caller can then decide if they care about sorting the map before iterating, and the sort can operate in-place on the buffer rather than the slower method of sorting within the iterator.

Does that seem reasonable? I haven't actually tried implementing this, but I think it should work.

JarlEvanson · 2023-02-18T18:48:00Z

That seems doable. I'll try it and get back to you.

phip1611 · 2023-02-22T12:04:01Z

uefi/src/table/boot.rs

+        }
+    }
+
+    /// Hoare partition scheme for quicksort


Suggested change

/// Hoare partition scheme for quicksort

/// Hoare partition scheme for quicksort.

phip1611 · 2023-02-22T12:04:16Z

uefi/src/table/boot.rs

+    }
+
+    /// Hoare partition scheme for quicksort
+    /// Must be called with `low` and `high` being indices within bounds


Suggested change

/// Must be called with `low` and `high` being indices within bounds

/// Must be called with `low` and `high` being indices within bounds.

phip1611 · 2023-02-22T12:06:21Z

uefi/src/table/boot.rs

+    /// Sorts the memory map in place
+    pub fn sort(&mut self) {
+        unsafe {
+            self.qsort(0, self.len - 1);


Why can't we use some built-in basic sorting functionality? It seems overkill to implement quick sort, doesn't it? What do you think, @nicholasbishop ?

I thought about a basic bubble sort as a UEFI memory map is unlikely to have thousands of entries.

I don't think the built-in sorting functions allow an unspecified constant spacing between elements in a slice, which would complicate using any of the built in basic sorting functions

What do you think about pulling in a dep on the index-sort crate? It's a small crate with a very short commit history, but it seems like exactly what we need for this use case. I would prefer that to implementing any of our own sorts, as that seems somewhat outside the scope of a uefi crate.

(I was a little surprised that I couldn't find more crates related to sorting non-slice data structures, but I guess it is kind of a niche need.)

Oh oops, just realized that crate isn't no-std. I might put up a PR for that.

OK, I put up a PR in that project to make it no-std compatible.

I don't want to block this PR on it though. I do feel a little uncomfortable about adding a somewhat complicated sort implementation here, but I'm OK with it for now because we can hopefully replace it in the future with a dependency on an external crate.

I think most of the time it would probably be fine to use a simpler and slower sort algorithm (insertion sort can be implemented in very few lines for example), but we don't have any way to guarantee that the number of entries is small. I could imagine for example that some ill-behaved driver accidentally creates a bunch of allocations; it's the kind of thing that might not be noticed in testing since the time that boot services are active is usually pretty short.

For the record, this is the PR nicholas set up: arlencox/index-sort#1

phip1611 · 2023-02-22T12:08:37Z

Thanks for working on this. LGTM and I think we are close to merging this.

Why would a user want an unsorted map? Should we sort the map automatically at any time?

nicholasbishop · 2023-02-23T02:33:11Z

uefi/src/table/boot.rs

+}
+
+impl<'buf> MemoryMap<'buf> {
+    #[allow(clippy::must_use_candidate)]


Rather than adding an allow for must_use_candidate here and elsewhere, add #[must_use].

nicholasbishop · 2023-02-23T02:45:32Z

uefi/src/table/boot.rs

+        self.key
+    }
+
+    /// Sorts the memory map in place


Should say here exactly what the memory map is sorted by (physical address? virtual address?)

nicholasbishop · 2023-02-23T02:45:51Z

uefi/src/table/boot.rs

+
+impl<'buf> MemoryMap<'buf> {
+    #[allow(clippy::must_use_candidate)]
+    /// Returns the unique [`MemoryMapKey`] associated with the memory map


Docstrings and comments should end with a period.

nicholasbishop · 2023-02-23T02:56:14Z

uefi/src/table/boot.rs

+
+    /// Hoare partition scheme for quicksort
+    /// Must be called with `low` and `high` being indices within bounds
+    unsafe fn qsort(&mut self, low: usize, high: usize) {


The qsort, partition, and swap functions should be made safe; they may do some unsafe operations internally but calling them shouldn't be unsafe.

nicholasbishop · 2023-02-23T03:02:24Z

uefi/src/table/boot.rs

+    }
+
+    unsafe fn partition(&mut self, low: usize, high: usize) -> usize {
+        let pivot = (self.buf.as_mut_ptr() as usize + (low + (high - low) / 2) * self.entry_size)


Add a function like this:

fn get_elem_phys_addr(&self, index: usize) -> PhysicalAddress { let offset = index.checked_mul(self.entry_size).unwrap(); let elem = unsafe { &*self.buf.as_ptr().add(offset).cast::<MemoryDescriptor>() }; elem.phys_start }

Then the three places where you do pointer arithmetic in this function can be replaced with a much simpler call to this function.

nicholasbishop · 2023-02-24T21:16:08Z

uefi/src/table/boot.rs

+                (base + index1 * self.entry_size) as *mut u8,
+                (base + index2 * self.entry_size) as *mut u8,
+                self.entry_size,
+            );


Casting from pointer -> integer -> pointer can be avoided here like this:

let base = self.buf.as_mut_ptr(); unsafe { ptr::swap_nonoverlapping( base.add(index1 * self.entry_size), base.add(index2 * self.entry_size), self.entry_size, ); }

(I noticed this from some Miri warnings, see also #666 which will make these warnings into hard errors.)

phip1611 · 2023-03-19T19:03:55Z

LGTM! I clean up the git history (remove merging main into this branch) and merge it then. Thanks for the contribution!

This adds a MemoryMap type that iterates over UEFI memory descriptors. The map is only sorted when one manually calls the sort method, as it may take up a few seconds, if the memory map contains thousands of entries, which may be the case on large systems.

phip1611 changed the title ~~Issue#661~~ Add Sorted Iterator for the UEFI Memory Map (Issue#661) Feb 18, 2023

phip1611 reviewed Feb 22, 2023

View reviewed changes

nicholasbishop reviewed Feb 23, 2023

View reviewed changes

nicholasbishop mentioned this pull request Feb 24, 2023

xtask: Enable strict provenance checks in Miri #666

Merged

2 tasks

nicholasbishop reviewed Feb 24, 2023

View reviewed changes

phil-opp mentioned this pull request Mar 12, 2023

fix issue #315 rust-osdev/bootloader#344

Closed

Jarl Evanson and others added 2 commits March 19, 2023 20:20

mem/doc: minor documentation adjustments

bbc030a

phip1611 merged commit 6b730d5 into rust-osdev:main Mar 19, 2023

JarlEvanson deleted the issue#661 branch October 21, 2023 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sorted Iterator for the UEFI Memory Map (Issue#661) #662

Add Sorted Iterator for the UEFI Memory Map (Issue#661) #662

JarlEvanson commented Feb 18, 2023

phip1611 commented Feb 18, 2023

nicholasbishop commented Feb 18, 2023

JarlEvanson commented Feb 18, 2023

phip1611 Feb 22, 2023

phip1611 Feb 22, 2023

phip1611 Feb 22, 2023

JarlEvanson Feb 22, 2023

nicholasbishop Feb 23, 2023

nicholasbishop Feb 23, 2023

nicholasbishop Feb 23, 2023 •

edited

Loading

phip1611 Mar 19, 2023

phip1611 commented Feb 22, 2023

nicholasbishop Feb 23, 2023

nicholasbishop Feb 23, 2023

nicholasbishop Feb 23, 2023

nicholasbishop Feb 23, 2023

nicholasbishop Feb 23, 2023

nicholasbishop Feb 24, 2023

phip1611 commented Mar 19, 2023

	/// Hoare partition scheme for quicksort
	/// Hoare partition scheme for quicksort.

	/// Must be called with `low` and `high` being indices within bounds
	/// Must be called with `low` and `high` being indices within bounds.

Add Sorted Iterator for the UEFI Memory Map (Issue#661) #662

Add Sorted Iterator for the UEFI Memory Map (Issue#661) #662

Conversation

JarlEvanson commented Feb 18, 2023

phip1611 commented Feb 18, 2023

nicholasbishop commented Feb 18, 2023

JarlEvanson commented Feb 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholasbishop Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phip1611 commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phip1611 commented Mar 19, 2023

nicholasbishop Feb 23, 2023 •

edited

Loading