Skip to content

sp-sim: update reported version info across updates #8060

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Apr 28, 2025

This PR does two main things:

  • After a software update is performed, sp-sim reports caboose and RoT information that reflects the update.
  • the Nexus tests for SP and RoT update verify this behavior. Note that these are tests for the old update facilities that are eventually going to be ripped out. But they're the only tests we have for this right now and it seemed worth verifying here. Eventually, these will be replaced by tests that use the new update facilities.

While doing this, I found a bunch of places that were self-inconsistent: the state operation and rot-boot-info operation would return different FWIDs for slots A and B; Gimlet reported setting the active slot for stage0 but Sidecar didn't; the comments about which components support which operations were inconsistent with each other, the code, and the real implementation; etc. I'm not positive all of my fixes are right. I welcome corrections. But I think it's a step in the right direction regardless.

Notes for reviewers

I believe this is basically ready for review, with one kind of big caveat: after discussion, if I were doing this again, I'd implement it very differently. Instead of storing caboose values and updating these during various parts of the update process, I would instead have the SimSpUpdate directly store data for all of the firmware slots that it simulates and compute caboose contents on-demand. I'd also have rot_reset recalculate the FWIDs on startup. I don't think this would have any functional change but should be more faithful in cases we haven't thought about and easier to reason about. But I don't think this is the most useful next step.

Here's what I'm thinking instead:

  • if this PR looks broadly good, then land it
  • write some tests for MgsUpdateDriver that use this new behavior so that we have some useful test coverage of that tool
  • then if it still makes sense: revisit the implementation here

Please let me know if y'all feel like that's wrong.

Fixes #7913.

@davepacheco davepacheco marked this pull request as ready for review April 29, 2025 23:02
Copy link
Contributor

@labbott labbott left a comment

Choose a reason for hiding this comment

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

I think this looks broadly okay. I'd like to do another pass to double check some of the existing update behavior but if everyone else thinks it's okay and wants to merge go ahead and do so.

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.

sp-sim could more faithfully simulate updates and cabooses
2 participants