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.

Sign up for GitHub

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

Jump to bottom

Size-aware memory types #1031

Merged
merged 30 commits into from
Sep 8, 2023
Merged

Size-aware memory types #1031

merged 30 commits into from
Sep 8, 2023

Conversation

NIMogen
Copy link
Contributor

@NIMogen NIMogen commented Aug 28, 2023

Draft PR for huge pages without changes to the mapper.

NIMogen and others added 21 commits July 20, 2023 13:17
@NIMogen
page allocator works; higher level pte always seems to be used
ee8ce6a
@NIMogen
Merge branch 'theseus-os:theseus_main' into theseus_main
a121eac
@NIMogen
added branches to unmap; higher level page table entries always used
7180c69
@NIMogen
Issues with memory access via as_* functions
4057902
@NIMogen
Added tests for 1gb pages; changed function names; docstrings and com…
c466d2c 
…ments added
@NIMogen
Merge branch 'theseus_main' into theseus_main
84f5b1e
@NIMogen
Chunk no longer holds enum; added todo! items; misc changes
6114abd
@NIMogen
small formatting changes
663506a
@NIMogen
restore some formatting
42114c7
@NIMogen
restoring whitespace
c417d40
@NIMogen
more restored whitespace
43efbe3
@NIMogen
misc formatting and comments
742528b
@NIMogen
added tests for memory_structs; restored mapper
7ec34c1
@NIMogen
checkout cargo.lock
0bdcc85
@NIMogen
Merge branch 'theseus_main' into size-aware-memory-types
35b4041
@NIMogen
restored files; whitespace changes
8927d46
@NIMogen
Update pte_flags_x86_64.rs
5149010
@NIMogen
Update lib.rs (whitespace removal)
a46424a
@NIMogen
Update page_allocator lib.rs (whitespace removal)
26c27cb
@NIMogen
merged generic impls; added TryFroms for Page/Frame types
69b495a
@NIMogen
Favor consts associated with PageSize trait
f295369
kevinaboos
kevinaboos requested changes Aug 29, 2023
View reviewed changes
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

For the TryFrom conversions, you have some of the cases covered, but not all. Specifically, if you think of it as a 3x3 table where you're converting a type on the left to the type on the top:

Page4K Page2M Page 1G
Page4K auto. fallible. fallible.
Page2M infallible auto fallible.
Page1G infallible infallible auto.

Notes:

  • "auto" means it's covered by Rust's built-in blanket implementation, where From is always implemented for converting a type to itself.
  • "fallible" means that the conversion may fail due to improper page number alignment, so you must implement that with TryFrom.
  • "infallible" means that the conversion is guaranteed to succeed, so you can implement it with From instead of TryFrom.

Aside from that, my main comment is to use generics everywhere. You're using them in about half of the places they could be used, which is good, but you shouldn't need to implement methods for specific Page<Page2MiB>/Page<Page1GiB> types hardly at all, except for the aforementioned conversion methods.

kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
@NIMogen NIMogen marked this pull request as ready for review September 5, 2023 18:45
kevinaboos
kevinaboos requested changes Sep 5, 2023
View reviewed changes
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

a few more comments

kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kevinaboos
kevinaboos requested changes Sep 6, 2023
View reviewed changes
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Left a few more comments with a couple tiny mistakes found.

For these cases, your tests ought to catch such mistakes. If you don't have test cases to cover these, please add them and then ensure they all pass.

kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kernel/memory_structs/src/lib.rs Outdated Show resolved Hide resolved
kevinaboos
kevinaboos approved these changes Sep 8, 2023
View reviewed changes
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Thanks very much Noah! Looking forward to seeing this in #1016!

@kevinaboos kevinaboos merged commit b521678 into theseus-os:theseus_main Sep 8, 2023
2 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 8, 2023
@kevinaboos
Add PageSize trait for defining 4K, 2M, and 1G huge pages (#1031)
05960af 
* All chunk-related types are now parameterized with a `P: PageSize`
  type parameter:
  * `Page` and `Frame`, which ensure that the underlying virtual
    or physical address is always properly aligned to the specified size.
  * `PageRange` and `FrameRange`, which ensure that the range of
    pages or frames are correctly aligned and only can be created
    in granular chunks of the given `PageSize`.
    * When iterating over a range of huge pages or frames, each step
      will be at the granularity of one huge page, which makes it easy
      to iterate of huge pages and huge frames in lockstep.
  * There are various implementations of the conversion traits
    `From` and `TryFrom` for normal and huge pages/frames and ranges.
  * The `PageSize` parameter can only be one of 3 marker structs:
    1. `Page4K`: a normal 4KiB page (P1-level), which is the default.
    2. `Page2M`: a P2-level huge page.
    3. `Page1G`: a P3-level huge page.

* This is only relevant to x86_64 at the moment, though we may add
  aarch64 huge page sizes in the future too.

Co-authored-by: Kevin Boos <kevinaboos@gmail.com> b521678
github-actions bot pushed a commit to tsoutsman/Theseus that referenced this pull request Sep 9, 2023
@tsoutsman
Add PageSize trait for defining 4K, 2M, and 1G huge pages (theseus-…
397c1ad 
…os#1031)

* All chunk-related types are now parameterized with a `P: PageSize`
  type parameter:
  * `Page` and `Frame`, which ensure that the underlying virtual
    or physical address is always properly aligned to the specified size.
  * `PageRange` and `FrameRange`, which ensure that the range of
    pages or frames are correctly aligned and only can be created
    in granular chunks of the given `PageSize`.
    * When iterating over a range of huge pages or frames, each step
      will be at the granularity of one huge page, which makes it easy
      to iterate of huge pages and huge frames in lockstep.
  * There are various implementations of the conversion traits
    `From` and `TryFrom` for normal and huge pages/frames and ranges.
  * The `PageSize` parameter can only be one of 3 marker structs:
    1. `Page4K`: a normal 4KiB page (P1-level), which is the default.
    2. `Page2M`: a P2-level huge page.
    3. `Page1G`: a P3-level huge page.

* This is only relevant to x86_64 at the moment, though we may add
  aarch64 huge page sizes in the future too.

Co-authored-by: Kevin Boos <kevinaboos@gmail.com> b521678
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinaboos kevinaboos kevinaboos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NIMogen @kevinaboos