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

page allocator: support allocating pages within an address range #970

Conversation

kevinaboos
Copy link
Member

@kevinaboos kevinaboos commented Jun 8, 2023

This is only needed on aarch64 for a workaround to allow #940 to work.

Currently, based on the needs of aarch64, we reserve 128MiB of virtual address space for executable text sections. This region is contiguous with the base kernel image's .text section.

aarch64: reserve 128MiB of virtual address space for executable text
sections that is contiguous with the base kernel image's .text section.
@kevinaboos kevinaboos marked this pull request as draft June 8, 2023 06:24
Only issue is that deallocation isn't behaving as expected?
for ex, an app crate's text sections should be freed after the app is
finished executing and its crate has been unloaded, but that doesn't
seem to be happening.
Running `hello` twice in a row should use the same .text pages for both,
but the second invocation is not re-using the .text pages from the
first invocation.
@kevinaboos kevinaboos changed the title page allocator: allow allocating N pages within a range of addresses page allocator: support allocating pages within an address range Jun 9, 2023
@kevinaboos
Copy link
Member Author

@tsoutsman @NathanRoyer this isn't yet ready to merge (still has extra log statements and such), but the logic should be complete. I've tested it every way I can think of, but am still looking for reviews just to check the logic/math since it's such a core subsystem. Always helps to have an extra set of eyes on things, thanks!

@kevinaboos kevinaboos marked this pull request as ready for review June 9, 2023 06:53
Copy link
Member

@NathanRoyer NathanRoyer left a comment

Choose a reason for hiding this comment

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

I can confirm nothing seems weird here (I spent extra time in find_any_chunk to make sure that function was good).

Things I would change but you don't have to:

  • this file seems to use hard tabs for some reason
  • explicit imports were replaced with wildcard imports :(
  • some warn! calls look like debugging statements, dunno if you want to keep them

kernel/page_allocator/src/lib.rs Outdated Show resolved Hide resolved
@kevinaboos
Copy link
Member Author

kevinaboos commented Jun 11, 2023

I can confirm nothing seems weird here (I spent extra time in find_any_chunk to make sure that function was good).

Much appreciated!

Things I would change but you don't have to:

  • this file seems to use hard tabs for some reason

Oh weird, I'll look at that in another PR.

  • explicit imports were replaced with wildcard imports :(

Yeah, I can do this. That's just me being lazy temporarily.

  • some warn! calls look like debugging statements, dunno if you want to keep them

Indeed, will remove before merging.

Copy link
Member

@tsoutsman tsoutsman left a comment

Choose a reason for hiding this comment

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

A potential way to simplify this in the future would be to move UPCOMING_PAGE_TABLE_RECURSIVE_P4_INDEX and RECURSIVE_P4_INDEX to the last entries in the page table so we can just chop off the last two pages rather than allocating around them.

kernel/page_allocator/src/lib.rs Show resolved Hide resolved
kernel/page_allocator/src/lib.rs Outdated Show resolved Hide resolved
kernel/page_allocator/src/lib.rs Outdated Show resolved Hide resolved
@kevinaboos
Copy link
Member Author

A potential way to simplify this in the future would be to move UPCOMING_PAGE_TABLE_RECURSIVE_P4_INDEX and RECURSIVE_P4_INDEX to the last entries in the page table so we can just chop off the last two pages rather than allocating around them.

yes indeed, i plan to do something similar to this once we refactor the heap a bit. We can't actually use entry 511, but we can use two contiguous entries like 509 and 510 instead of 508 and 510 like we have now.

@kevinaboos kevinaboos merged commit e2806aa into theseus-os:theseus_main Jun 13, 2023
github-actions bot pushed a commit that referenced this pull request Jun 13, 2023
* Currently this is only used for allocating pages for new
  executable .text sections on aarch64, which itself is a
  workaround to enable runtime loading of crates (see #940).
  * Based on the limitations of aarch64's ISA (branch instructions),
    we reserve 128MiB of virtual address space for this purpose.
  * This 128MiB region is for executable .text sections only,
    and is contiguous with the base kernel image's .text section.

* This is available but not used by default on x86_64 yet. e2806aa
Ramla-I pushed a commit to Ramla-I/Theseus that referenced this pull request Jun 14, 2023
…seus-os#970)

* Currently this is only used for allocating pages for new
  executable .text sections on aarch64, which itself is a
  workaround to enable runtime loading of crates (see theseus-os#940).
  * Based on the limitations of aarch64's ISA (branch instructions),
    we reserve 128MiB of virtual address space for this purpose.
  * This 128MiB region is for executable .text sections only,
    and is contiguous with the base kernel image's .text section.

* This is available but not used by default on x86_64 yet.
github-actions bot pushed a commit to Ramla-I/Theseus that referenced this pull request Jun 14, 2023
…seus-os#970)

* Currently this is only used for allocating pages for new
  executable .text sections on aarch64, which itself is a
  workaround to enable runtime loading of crates (see theseus-os#940).
  * Based on the limitations of aarch64's ISA (branch instructions),
    we reserve 128MiB of virtual address space for this purpose.
  * This 128MiB region is for executable .text sections only,
    and is contiguous with the base kernel image's .text section.

* This is available but not used by default on x86_64 yet. e2806aa
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.

3 participants