Skip to content

UEFI Allocator: add PAGE_SIZE shortcut #1611

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

UEFI Allocator: add PAGE_SIZE shortcut #1611

wants to merge 6 commits into from

Conversation

phip1611
Copy link
Member

@phip1611 phip1611 commented Apr 7, 2025

Please review this commit-by-commit.

Steps to Un-Draft

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 requested a review from nicholasbishop April 7, 2025 08:43
phip1611 added 3 commits April 7, 2025 10:45
The refactoring to a match {} block simplifies the following
improvements.
This is a prerequisite for the next commit.
phip1611 added 2 commits April 7, 2025 10:49
Allocating page-aligned memory via the global allocator is not
uncommon for UEFI OS loaders. Therefore, it is feasible to use a
shortcut in the allocator, and directly use boot::allocate_pages()
rather than boot::allocate_pool().

## Testing

We can look at the TRACE messages of `cargo xtask run` to
verify that the shortcut is taken.
We have not been requested yet to do this, but this gives downstream
users more flexibility.
`allocator_api` is a `core::alloc` feature, not a feature
of the `alloc` crate. This fixes build issues.
@phip1611 phip1611 self-assigned this Apr 7, 2025
@phip1611 phip1611 mentioned this pull request Apr 7, 2025
13 tasks
aligned_ptr
// Allocating pages is actually very expected in UEFI OS loaders, so
// it makes sense to provide this optimization.
// In Rust, each allocation's size must be at least the alignment,
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of automatically using allocate_pages in some cases. Would it make sense to branch on the size rather than just alignment? E.g. if (size % PAGE_SIZE) == 0 && align <= PAGE_SIZE { ... }? Then there's never a concern about over-allocating memory.

@nicholasbishop
Copy link
Member

General note: there are a lot of changes going on in this PR. I know it's broken down more clearly by commit, but especially when I'm just "allocating" (😉) 5 or 10 minutes of review time to a PR it helps to have less stuff in each one. E.g. the implementation of alloc_api could go in a separate PR from the PAGE_SIZE allocation changes.

@phip1611 phip1611 changed the title uefi: (global) Allocator: add PAGE_SIZE shortcut + implement allocator_api UEFI Allocator: Implement cores allocator_api: add PAGE_SIZE shortcut Apr 14, 2025
@phip1611 phip1611 changed the title UEFI Allocator: Implement cores allocator_api: add PAGE_SIZE shortcut UEFI Allocator: add PAGE_SIZE shortcut Apr 14, 2025
@phip1611 phip1611 marked this pull request as draft April 14, 2025 20:28
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