multiboot2: Fix EFI Memory Map 'last_area' calculation #119
Conversation
multiboot2/src/memory_map.rs
Outdated
| @@ -140,7 +140,7 @@ impl EFIMemoryMapTag { | |||
| let start_area = (&self.first_desc) as *const EFIMemoryDesc; | |||
| EFIMemoryAreaIter { | |||
| current_area: start_area as u64, | |||
| last_area: (self_ptr as u64 + self.size as u64), | |||
| last_area: (self_ptr as u64 + (self.size - self.desc_size) as u64), | |||
There was a problem hiding this comment.
I think that the root problem is that size_of::<EFIMemoryMapTag>() contains already the size of the first entry. I think a nicer solution would be to type the EFIMemoryMapTag as
pub struct EFIMemoryMapTag {
typ: TagType,
size: u32,
desc_size: u32,
desc_version: u32,
first_desc: [EFIMemoryDesc; 0],
}instead. Depending on your skill and time you'd like to invest, feel free to look into it. Otherwise, I can take care of it.
There was a problem hiding this comment.
Thanks for the suggestion. I'm a bit new to Rust, but the zero length arrays seem much more elegant.
I've updated both the E820 and EFI code to use them.
|
Hi and thanks a lot for your contribution! There are unit-test failures, but it might also be the case, that the unit test itself is wrong. I can't look into it right now. If you'd like to invest more time into it, I'd appreciate it. Otherwise, I try to look into it within the next 10 days. |
Correct EFI 'last_area' calculation so it correctly describes the greatest possible starting address of the last record. (Previously, iterating the EFI Memory map resulted in a superfluous entry as it ran over the next tag) This revision also cleans up the E820/EFI parsing to use 0-length arrays for the records and to faciliate a more natural 'sizeof' operation on the Multiboot2 tag.
| EFIMemoryAreaIter { | ||
| current_area: start_area as u64, | ||
| last_area: (self_ptr as u64 + self.size as u64), | ||
| // NOTE: `last_area` is only a bound, it doesn't necessarily point exactly to the last element |
There was a problem hiding this comment.
An oddly formed multiboot tag could have a combination of "tag size" and "EFI descriptor size" that represents a non-integer multiple of descriptors. Thus, I wanted to make it clear that "last_area" should only be used as a bound and not a direct pointer to the last descriptor.
|
Thanks very much for the contribution. I merge it as it and fix the CI issue myself. |
Merge #119 + necessary fixed into main + multiboot2-0.14.1
|
thanks! the bug fix is live on crates.io! |
Correct EFI 'last_area' calculation and align it to be more similar the E820 version.
(Previously, iterating the EFI Memory map resulted in a superfluous entry as it ran over the next tag)