Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicholasbishop merged 11 commits intorust-osdev:mainfromphip1611:mmap
Jun 23, 2024

Conversation

phip1611
Copy link
Member

@phip1611phip1611 commentedMay 24, 2024
edited
Loading

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

  • renameentry_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 theRewriting History guide for help.
  • Update the changelog (if necessary)

@phip1611phip1611 marked this pull request as draftMay 25, 2024 18:09
@phip1611phip1611force-pushed themmap branch 2 times, most recently froma97a62b tod973341CompareMay 25, 2024 22:27
@phip1611phip1611 changed the titlememory map: improvements to fight pitfallsmemory map: improvements to fight pitfalls (MemoryMap know owns the memory)May 25, 2024
@phip1611
Copy link
MemberAuthor

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.

@phip1611phip1611 marked this pull request as ready for reviewMay 25, 2024 22:45
@phip1611phip1611force-pushed themmap branch 2 times, most recently from8cccd2d to42b1befCompareMay 26, 2024 09:59
Ok(Self::from_raw(ptr, len))
}

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

Choose a reason for hiding this comment

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

This should beunsafe, but I'm not sure if it's needed at all; can we just fold it intonew?

Copy link
MemberAuthor

@phip1611phip1611Jun 21, 2024
edited
Loading

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
MemberAuthor

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, changingMemoryMap to own the memory should make for a clearer API.

@phip1611phip1611 changed the titlememory map: improvements to fight pitfalls (MemoryMap know owns the memory)memory map: improvements to fight pitfalls (MemoryMap now owns the memory)Jun 21, 2024
@phip1611phip1611force-pushed themmap branch 2 times, most recently from32d8dc8 to3f33fbbCompareJune 21, 2024 09:37
@phip1611
Copy link
MemberAuthor

phip1611 commentedJun 21, 2024
edited
Loading

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.

@phip1611phip1611force-pushed themmap branch 2 times, most recently from13cef07 to7045074CompareJune 21, 2024 10:24
@phip1611phip1611 marked this pull request as draftJune 21, 2024 10:31
@phip1611phip1611 marked this pull request as ready for reviewJune 21, 2024 15:13
This helps to prevent confusions. MemoryDescriptors and their reported size arealready a pitfall. So at least we should refrain from using non-standard namesfor them.
This prepares the following changes.
This is an attempt to simplify the overall complex handling of obtaining the UEFI memorymap. We have the following pre-requisites and use-cases all to keep in mind whendesigning 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 loopThis makes it hard to come up with an ergonomic solution such as using a Boxor any other high-level Rust type.The main simplification of my design is that the MemoryMap type now doesn'thas a reference to memory anymore but actually owns it. This also models thereal world use case where one typically obtains the memory map once whenboot services are exited. A &'static [u8] on the MemoryMap just creates moreconfusion that it brings any benefit.The MemoryMap now knows whether boot services are still active and freesthat memory, or it doesn't if the boot services are exited. This meansless 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.
@nicholasbishopnicholasbishop added this pull request to themerge queueJun 23, 2024
Merged via the queue intorust-osdev:main with commit4ca4daaJun 23, 2024
12 checks passed
@phip1611phip1611 deleted the mmap branchJuly 14, 2024 06:59
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicholasbishopnicholasbishopnicholasbishop approved these changes

@GabrielMajeriGabrielMajeriAwaiting requested review from GabrielMajeri

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@phip1611@nicholasbishop

[8]ページ先頭

©2009-2025 Movatter.jp