Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork175
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
a97a62b
tod973341
CompareThis 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. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
8cccd2d
to42b1bef
CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
uefi/src/table/boot.rs Outdated
Ok(Self::from_raw(ptr, len)) | ||
} | ||
fn from_raw(ptr: *mut u8, len: usize) -> Self { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Marked as as unsafe.
I agree, changing |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
32d8dc8
to3f33fbb
Comparephip1611 commentedJun 21, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
13cef07
to7045074
CompareThis 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.
4ca4daa
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Follow-up of#1174 that improves the MemoryMap abstraction.
entry_size
->desc_size
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:
size_of)
between the allocation of a buffer and storing the memory map inside it
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
Checklist