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

Add uefi::boot module #1255

Merged
merged 1 commit into from
Jul 27, 2024
Merged

Conversation

nicholasbishop
Copy link
Contributor

The initial version contains allocate_pages/allocate_pool and the corresponding free functions.

Checklist

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

uefi-test-runner/src/boot/memory.rs Outdated Show resolved Hide resolved
uefi/src/boot.rs Outdated
ty: AllocateType,
mem_ty: MemoryType,
count: usize,
) -> Result<PhysicalAddress> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like an API inconsistency with allocate_pool, which returns Result<NonNull<u8>>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true. This inconsistency is present in our BootServices methods and in the UEFI spec; AllocatePool produces a void* pointer, while AllocatePages produces a EFI_PHYSICAL_ADDRESS. Notably, EFI_PHYSICAL_ADDRESS is always a UINT64 rather than being pointer sized, so there is a real difference in behavior. The docstring notes this in the context of ia32 PAE (I copied it from the BootServices method, the text was originally added in this commit).

That said, I think in almost all cases the first thing a caller is going to do with the EFI_PHYSICAL_ADDRESS is cast it to a pointer, and I can't currently think of any real-world counter example. So, while I don't feel very strongly in either direction, I don't mind converting this to return NonNull<u8> instead of PhysicalAddress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. For full consistency, I also changed both free functions to use NonNull<u8> as well.

The initial version contains allocate_pages/allocate_pool and the corresponding
`free` functions.

Unlike the `BootServices` methods, these functions all use `NonNull<u8>` for
consistency.
@phip1611 phip1611 enabled auto-merge July 27, 2024 10:25
@phip1611 phip1611 added this pull request to the merge queue Jul 27, 2024
Merged via the queue into rust-osdev:main with commit 64c26a4 Jul 27, 2024
12 checks passed
@nicholasbishop nicholasbishop deleted the bishop-add-boot-mod branch July 27, 2024 13:04
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