fixes to run on spike #335
Merged
Conversation
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
There was a problem hiding this comment.
Pull Request Overview
This PR makes modifications to enable salus to run on spike in addition to qemu by adjusting error handling and validation logic in several components.
- In src/main.rs, the UART probe now logs errors instead of propagating them, potentially affecting console initialization.
- In page-tracking, error variants are updated to include additional contextual information.
- In IMSIC-related code, new error variants are added and the MMIO region validations are adjusted for improved error reporting.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Changed UART probe error handling from propagating errors to logging the error. |
| page-tracking/src/page_tracker.rs | Updated PageMapNoSpace to include a u64 value. |
| page-tracking/src/page_info.rs | Updated error propagation to pass the page_map_size to the new PageMapNoSpace variant. |
| drivers/src/imsic/error.rs | Added new error variants with improved descriptions; note spelling correction needed. |
| drivers/src/imsic/core.rs | Modified MMIO region validation logic and error reporting for IMSIC configuration. |
Comments suppressed due to low confidence (3)
src/main.rs:481
- Switching from error propagation to merely logging the error during UART initialization may result in the system console not being properly set up; please confirm that this behavior is intentional.
if let Err(e) = UartDriver::probe_from(&hyp_dt, &mut mem_map) {
drivers/src/imsic/core.rs:366
- [nitpick] Review the updated condition for checking the MMIO region size; replacing a misalignment check with a minimum size check may not fully capture the intended validation.
if region_size < guests_per_hart as u64 * PageSize::Size4k as u64 {
drivers/src/imsic/core.rs:369
- [nitpick] Ensure that forcing a minimum count of 1 hart in the region using 'max' is safe and that it does not mask potential configuration issues with region sizes.
let harts_in_region = core::cmp::max(region_size / per_hart_size, 1);
Use a different error for different failure cases. Signed-off-by: Dylan Reid <[email protected]>
If m-mode interrupt files are interleaved with S and guest files, then the device tree may report holes in the reg map for the imsics, with each cpu's files separated by a missing page. Secondary reg regions will have non-zero hart ids, so remove the check that all regions are for hart 0. Similarly the per_hart_size is more of a stride and it's possible to have less than that size in each region. Signed-off-by: Dylan Reid <[email protected]>
Having the amount of space needed available makes debugging more convenient. Signed-off-by: Dylan Reid <[email protected]>
dgreid
force-pushed
the
topic/spike_errors
branch
from
937ec06 to
254ca52
Compare
atishp04
reviewed
Jun 13, 2025
Contributor
atishp04
left a comment
atishp04
left a comment
There was a problem hiding this comment.
What do you mean by this UART ? The default UART device in virt machine that salus parses from DT ? We rely only on DBCN to run things on spike/hw for now.
May be just clarify the commit message ?
The UART driver supports only the 16550a that is only available on QEMU. Print an error but continue so salus can run on spike and HW. Signed-off-by: Dylan Reid <[email protected]>
dgreid
force-pushed
the
topic/spike_errors
branch
from
254ca52 to
4a78ce6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
some useful changes that allow salus to run on spike in addition to qemu.