Draft
Conversation
It currently panics because the latest bootloader maps some stuff on huge pages which we don't expect...
This was referenced Jan 4, 2025
Closed
There was a problem hiding this comment.
Pull Request Overview
This PR is an initial work-in-progress to add hugepage support and update error types across multiple modules. Key changes include:
- Renaming error types from AllocErr to AllocError and updating error handling with thiserror.
- Modifying page mapping and translation functions to handle hugepage cases.
- Updating toolchain and dependency versions.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/arch/x86_64/tests.rs | Refactored test function signatures for updated error types. |
| src/arch/x86_64/oops.rs | Simplified panic message handling in panic contexts. |
| src/allocator.rs | Updated error type in allocator API. |
| rust-toolchain.toml | Upgraded nightly toolchain version. |
| pci/src/error.rs | Adjusted error implementation to use core::error::Error. |
| inoculate/Cargo.toml | Bootloader dependency version update. |
| hal-x86_64/src/time/pit.rs | Enhanced Pit error types with thiserror annotations. |
| hal-x86_64/src/time.rs | Updated InvalidDuration error handling with thiserror. |
| hal-x86_64/src/segment.rs | Modified label usage in assembly with updated comments. |
| hal-x86_64/src/mm.rs | Extensive modifications to page mapping APIs for hugepage support. |
| hal-x86_64/src/interrupt/apic/local.rs | Added thiserror attributes to LocalApicError. |
| hal-x86_64/src/interrupt/apic/ioapic.rs | Adjusted try_new signature for improved mapping. |
| hal-x86_64/src/interrupt.rs | Updated periodic timer error handling. |
| hal-x86_64/Cargo.toml | Updated rust version and added dependency on thiserror. |
| hal-core/src/mem/page.rs | Renamed allocator error return types to AllocError. |
| hal-core/src/interrupt.rs | Enhanced error types with thiserror attributes. |
| hal-core/src/addr.rs | Removed custom Display impl in favor of thiserror. |
| hal-core/Cargo.toml | Updated rust version and added thiserror dependency. |
| alloc/src/buddy.rs | Updated error handling in buddy allocator to use AllocError. |
| Cargo.toml | Added workspace dependency on thiserror. |
Comments suppressed due to low confidence (1)
src/arch/x86_64/oops.rs:113
- The new implementation directly writes panic.message() without handling the None case, which may lead to unexpected output. Consider preserving the original match to provide an appropriate fallback error message if panic.message() returns None.
match panic.message() {
| let pdpt = pml4 | ||
| .create_next_table(virt, frame_alloc) | ||
| .expect("PML4 shouldn't point directly to a hugepage lol"); | ||
| let pd = match pdpt.create_next_table(virt, alloc) { |
There was a problem hiding this comment.
The variable 'alloc' is used here but it is not defined in the function; it should likely be 'frame_alloc' as used earlier.
Suggested change
| let pd = match pdpt.create_next_table(virt, alloc) { | |
| let pd = match pdpt.create_next_table(virt, frame_alloc) { |
| idx, | ||
| entry | ||
| ); | ||
| tracing::trace!("page is hudge"); |
There was a problem hiding this comment.
There is a spelling error in the log message: "hudge" should be corrected to "huge".
Suggested change
| tracing::trace!("page is hudge"); | |
| tracing::trace!("page is huge"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this is gonna take a lot more work and sucks and does nt compile