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

[sw/opentitantool] Add otp alert-digest subcommand #15872

Merged
merged 3 commits into from
Nov 4, 2022
Merged

[sw/opentitantool] Add otp alert-digest subcommand #15872

merged 3 commits into from
Nov 4, 2022

Conversation

jon-flatley
Copy link
Contributor

Adds a new command/subcommand opentitantool otp alert-digest <alert-cfg> <lc-state> that takes an HJSON OTP image file that defines the OWNER_SW_CFG_ROM_ALERT_* OTP fields and prints the corresponding value for OWNER_SW_CFG_ROM_ALERT_DIGEST_* used by ROM to verify alert_handler initialization.

@jon-flatley jon-flatley requested review from cfrantz and alphan November 1, 2022 15:05
@jon-flatley jon-flatley requested a review from a team as a code owner November 1, 2022 15:05
@jon-flatley
Copy link
Contributor Author

I haven't hooked this up to serde-annotate yet. I made a small deserializer for OTP field values just to get this off the ground. I do ultimately intend to use lowRISC/serde-annotate#7 for these fields instead, thank you @cfrantz! Also note that the output will be in decimal unless JSON5 or YAML is specified as the output format.

@jon-flatley jon-flatley force-pushed the ottool_otp_alert branch 2 times, most recently from 13fd74d to e40dab9 Compare November 1, 2022 19:17
@jon-flatley
Copy link
Contributor Author

While trying to integrate this into the OTP bazel rules I found that having the digest value presented in stdout isn't ideal. Since the schema for otp_img_*.hjson files is already included, I found that it was easier to have opentitantool just write this file itself. Now, the tool will print JSON output that matches the otp_img_* schema which can be redirected to a file output with the --output switch. Optionally, the names for the partition and item name in the OTP image can be overwritten as well.

@jon-flatley jon-flatley force-pushed the ottool_otp_alert branch 2 times, most recently from dfa8fc2 to a58706b Compare November 2, 2022 20:35
@jon-flatley
Copy link
Contributor Author

Outputting the value fields as decimal was breaking the parser in the python tools. Had to jump through a few hoops to get these fields serializing as hex, thank you @cfrantz for your help on debugging this and for the quick fixes to serde-annotate.

The current state of this PR contains a workaround for a bug in serde-annotate to get hex serialization working, and this is now blocked on lowRISC/serde-annotate#10.

@jon-flatley jon-flatley mentioned this pull request Nov 2, 2022
Merged
alphan
alphan reviewed Nov 2, 2022
View reviewed changes
sw/host/opentitanlib/src/otp/otp_img.rs Outdated
Comment on lines 111 to 113
// This attribute shouldn't be here, it should be in `OtpImgValue`, but due to a bug in
// serde_annotate this is needed to get values serialized in hex.
#[annotate(format = hex)]
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need this? Asking because of the recent update to serde_annotate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've filed an issue for this, see cfrantz/serde-annotate#5

alphan
alphan approved these changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@alphan alphan left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jon-flatley! Just some minor comments.

sw/host/opentitanlib/src/util/num_de.rs Show resolved Hide resolved
sw/host/opentitantool/src/command/otp.rs Outdated Show resolved Hide resolved
sw/host/opentitantool/src/command/otp.rs Outdated Show resolved Hide resolved
@alphan
Copy link
Contributor

alphan commented Nov 3, 2022

@jon-flatley can you check CI logs? Also, it would be nice to modify this to print/update values for all lifecycle states at once.

@jon-flatley
Copy link
Contributor Author

@jon-flatley can you check CI logs? Also, it would be nice to modify this to print/update values for all lifecycle states at once.

I don't expect this to pass CI until we update to the most recent version of serde-annotate #15951.

I'll update this and #15890 to populate the OTP words for all lifecycle states.

@jon-flatley jon-flatley force-pushed the ottool_otp_alert branch 2 times, most recently from 0c9af49 to 96c919c Compare November 3, 2022 15:56
@alphan
Copy link
Contributor

alphan commented Nov 4, 2022

@jon-flatley can you check CI logs? Also, it would be nice to modify this to print/update values for all lifecycle states at once.

I don't expect this to pass CI until we update to the most recent version of serde-annotate #15951.

I'll update this and #15890 to populate the OTP words for all lifecycle states.

Ah, sounds good, thanks!

Jon Flatley added 3 commits November 4, 2022 13:29
[opentitanlib] Cleanup unused OTP features
f4dd469 
OTP support in opentitantool is moving away from end to end vmem
generation. Instead, opentitantool will serve as a front end utility for
generating some software values (such as for alert_handler
configuration) and for providing an way to specifying OTP values using
named constants.

Signed-off-by: Jon Flatley <jflat@google.com>
[opentitantool] Add alert_handler magic value generation
45a1ddd 
Adds a new command, `opentitantool otp alert-crc`, for generating the
magic value used to validate alert_handler register values during
shutdown_init in ROM.

Signed-off-by: Jon Flatley <jflat@google.com>
[opentitantool] Output OTP image overlays with hex values
5d25613 
Signed-off-by: Jon Flatley <jflat@google.com>
@jon-flatley jon-flatley merged commit e1d71da into lowRISC:master Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alphan alphan alphan approved these changes

@cfrantz cfrantz Awaiting requested review from cfrantz

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
@jon-flatley @alphan