Single-task dumping #1284
Merged
Single-task dumping #1284
Conversation
This file contains 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
cbiffle
reviewed
Apr 14, 2023
| @@ -22,7 +22,6 @@ enum Trace { | |||
| DeserializeHeaderError(hubpack::Error), | |||
| SendError(SendError), | |||
| WrongVersion(u8), | |||
| Hi(u8, usize), | |||
task/jefe/src/dump.rs
Outdated
| // | ||
| // We need to claim a dump area. Once it's claimed, we have committed | ||
| // to dumping into it: any failure will result in a partial or otherwise | ||
| // corrupted dump. | ||
| // | ||
| let area = humpty::claim_dump_area( | ||
| base, | ||
| DumpContents::SingleTask, | ||
| if full { | ||
| DumpContents::SingleTask |
There was a problem hiding this comment.
I admit a mild preference for exposing this enum in place of bool in the function signature, as I think it'd make it clearer.
There was a problem hiding this comment.
I'll do you one better and make a new type, since DumpContents has other variants that aren't relevant here.
Done in c61c063
| let mem = start..start + length; | ||
| let mut okay = false; | ||
| loop { | ||
| // This is Accidentally Quadratic; see the note in `dump_task` |
There was a problem hiding this comment.
Either having the kernel return an error for an out-of-range region (as we discussed in chat), or exposing a "validate region" entry point, would be good ways of eliminating this
There was a problem hiding this comment.
Agreed, that's going on my list for later cleanups
task/jefe/src/main.rs
Outdated
| if task_index == 0 { | ||
| // Can't dump the supervisor | ||
| return Err(DumpAgentError::NotSupported.into()); | ||
| } else if task_index as usize > self.task_states.len() { |
There was a problem hiding this comment.
It seems like all of these comparisons should be >=
There was a problem hiding this comment.
Yes, fixed in bc20ab5
| dump::dump_task(self.dump_areas, task_index as usize) | ||
| .map_err(|e| e.into()) | ||
| } | ||
| fn dump_task_region( |
There was a problem hiding this comment.
(would like blank lines between functions, ideally)
There was a problem hiding this comment.
Me too, not sure where it went!
Fixed in d359897
mkeeter
force-pushed
the
single-task-dump
branch
3 times, most recently
from
90d0bb6
to
b473b04
Compare
Add the ability to dump a subsection of a task's memory Bump humpty + version Switch to returning dump area indices Add API to reinitialize dump regions
mkeeter
force-pushed
the
single-task-dump
branch
from
b473b04
to
4477a9b
Compare
mkeeter
added a commit
to oxidecomputer/humility
that referenced
this pull request
Apr 17, 2023
This PR follows - oxidecomputer/humpty#5 - oxidecomputer/hubris#1284 It implements a few things: - Single-task dumping using the new APIs from the PRs above - Letting the `NetCore` read RAM using the task region dump API Things spiraled a _little_ out of control as I attempted to tame the Humility dependency tree. This mostly involved splitting modules into crates of their own, to ease the blockage at `humility` and `humility_cmd`: - Moved `NetCore` from `humility` to a separate crate `humility-net-core` - Moved `hiffy`, `idol`, and `doppel` out of `humility_cmd` into standalone crates - Added a new `humility-dump-agent` crate, which is used in both `humility_cmd_dump` and `NetCore` - Fixed everything that these changes broke The reorganization isn't done – it's still too easy to trigger a rebuild of every single command – but it's a step in the right direction.
bcantrill
pushed a commit
to oxidecomputer/humility
that referenced
this pull request
Jun 22, 2023
This PR follows - oxidecomputer/humpty#5 - oxidecomputer/hubris#1284 It implements a few things: - Single-task dumping using the new APIs from the PRs above - Letting the `NetCore` read RAM using the task region dump API Things spiraled a _little_ out of control as I attempted to tame the Humility dependency tree. This mostly involved splitting modules into crates of their own, to ease the blockage at `humility` and `humility_cmd`: - Moved `NetCore` from `humility` to a separate crate `humility-net-core` - Moved `hiffy`, `idol`, and `doppel` out of `humility_cmd` into standalone crates - Added a new `humility-dump-agent` crate, which is used in both `humility_cmd_dump` and `NetCore` - Fixed everything that these changes broke The reorganization isn't done – it's still too easy to trigger a rebuild of every single command – but it's a step in the right direction.
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 PR implements single-task dumping in Hubris, mediated by the supervisor task. It includes three new APIs:
(see oxidecomputer/humpty#5 for the associated UDP types)
Single-task dumping is between the supervisor and the kernel, and doesn't require the RoT!
Most of this PR is plumbing of various kinds:
dump-agent→ Idol messages to Jefedump-agent-> Idol messages to JefeWithin
jefe::dump,dump_tasksplits into four functions:dump_task_setup)dump_task_run)dump_task, which now uses the abovedump_task_regionTo be very clear – even though it may go without saying – tasks talking to
jefethrough these APIs should not be able to crash the supervisor.The most suspicion is directed towards
dump_task_region, because it specifies an arbitrary address + length. To make sure the region is legal,jefechecks this region against the kernel's report of valid dump regions for the task (kipc::get_task_dump_region) and bails out early if it's invalid.