Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memory map: improvements to fight pitfalls (MemoryMap now owns the memory) #1175

Merged
merged 11 commits into from
Jun 23, 2024

Conversation

phip1611
Copy link
Contributor

@phip1611 phip1611 commented May 24, 2024

Follow-up of #1174 that improves the MemoryMap abstraction.

  • rename entry_size -> desc_size
  • better documentation
  • unify usage of common constructor with sane assertions
  • removed lifetime/reference, type now owns the memory (which makes much more sense IMHO)

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

Checklist

  • Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
  • Update the changelog (if necessary)

@phip1611 phip1611 marked this pull request as draft May 25, 2024 18:09
@phip1611 phip1611 force-pushed the mmap branch 2 times, most recently from a97a62b to d973341 Compare May 25, 2024 22:27
@phip1611 phip1611 changed the title memory map: improvements to fight pitfalls memory map: improvements to fight pitfalls (MemoryMap know owns the memory) May 25, 2024
@phip1611
Copy link
Contributor Author

This took me a while - it was much more complicated than anticipated. It is hard to come up with a nice and rusty solution due to the many conditions and limitations mentioned above (and in the commit messages). I think this is a clear improvement as we could get rid of the lifetime/slice in Memory Type.

@phip1611 phip1611 marked this pull request as ready for review May 25, 2024 22:45
uefi-raw/src/table/boot.rs Outdated Show resolved Hide resolved
uefi/src/table/mod.rs Outdated Show resolved Hide resolved
@phip1611 phip1611 force-pushed the mmap branch 2 times, most recently from 8cccd2d to 42b1bef Compare May 26, 2024 09:59
uefi-raw/src/table/boot.rs Outdated Show resolved Hide resolved
uefi-services/README.md Show resolved Hide resolved
uefi/src/table/boot.rs Show resolved Hide resolved
Ok(Self::from_raw(ptr, len))
}

fn from_raw(ptr: *mut u8, len: usize) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be unsafe, but I'm not sure if it's needed at all; can we just fold it into new?

Copy link
Contributor Author

@phip1611 phip1611 Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is needed. It is the common constructor for the unit test constructor and the runtime constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marked as as unsafe.

@nicholasbishop
Copy link
Member

I agree, changing MemoryMap to own the memory should make for a clearer API.

uefi-test-runner/src/boot/memory.rs Outdated Show resolved Hide resolved
uefi-test-runner/src/boot/memory.rs Outdated Show resolved Hide resolved
uefi/CHANGELOG.md Show resolved Hide resolved
uefi/src/table/mod.rs Outdated Show resolved Hide resolved
uefi/src/table/mod.rs Outdated Show resolved Hide resolved
@phip1611 phip1611 changed the title memory map: improvements to fight pitfalls (MemoryMap know owns the memory) memory map: improvements to fight pitfalls (MemoryMap now owns the memory) Jun 21, 2024
@phip1611 phip1611 force-pushed the mmap branch 2 times, most recently from 32d8dc8 to 3f33fbb Compare June 21, 2024 09:37
@phip1611
Copy link
Contributor Author

phip1611 commented Jun 21, 2024

Ready for the next review round. I know this is complex and hard to review, but I think it is definetely a step forward and an improvement. Let me know what you think @nicholasbishop - I hope I could address all open questions.

I recommend to review this commit by commit.

@phip1611 phip1611 force-pushed the mmap branch 2 times, most recently from 13cef07 to 7045074 Compare June 21, 2024 10:24
@phip1611 phip1611 marked this pull request as draft June 21, 2024 10:31
@phip1611 phip1611 marked this pull request as ready for review June 21, 2024 15:13
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.
This prepares the following changes.
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
This prepares the next commit.
This is now a benefit compared to the old API. This wasn't possible.
@nicholasbishop nicholasbishop added this pull request to the merge queue Jun 23, 2024
Merged via the queue into rust-osdev:main with commit 4ca4daa Jun 23, 2024
12 checks passed
@phip1611 phip1611 deleted the mmap branch July 14, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants