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.

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

[regtool/rtl] Add missing CM annotations #21315

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

msfschaffner
Copy link
Contributor

@msfschaffner msfschaffner commented Feb 12, 2024

This fixes #20887.

Note that #10071 can only be closed once all autogen'd IPs have been transitioned to IPGen, since the annotation checker does not look in the right RTL locations otherwise. I.e., there are some IPs like pinmux which are correctly annotated, but will fail since the top-specific Hjson resides in a different subtree than the associated RTL (which will be fixes once transitioned to IPGen).

Fixes part of lowRISC#20887

Signed-off-by: Michael Schaffner <[email protected]>
Fixes part of lowRISC#20887

Signed-off-by: Michael Schaffner <[email protected]>
Copy link
Contributor

@ballifatih ballifatih left a comment

Choose a reason for hiding this comment

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

LGTM for KMAC and Keymgr. I am not very familiar with the flow that checks these comment lines though.

Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

One query about the last commit, but this looks sensible to me.

@@ -193,7 +193,7 @@ module rom_ctrl
.ErrOnWrite(1),
.CmdIntgCheck(1),
.EnableRspIntgGen(1),
.EnableDataIntgGen(SecDisableScrambling),
.EnableDataIntgGen(SecDisableScrambling), // SEC_CM: CTRL.MEM.INTEGRITY
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is quite what we want? This flag tells the module to generate integrity bits on the fly when scrambling is disabled. I don't think this is quite the right place to put the annotation?

Copy link
Contributor Author

@msfschaffner msfschaffner Feb 13, 2024

Choose a reason for hiding this comment

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

Any suggestion where else we could put it?

I put it here since this enables end-to-end integrity passthrough in case scrambling is enabled, and there is not really a specific datapath block that implements end-to-end integrity.

Copy link
Contributor Author

@msfschaffner msfschaffner Feb 13, 2024

Choose a reason for hiding this comment

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

I moved this label to the definition of the DataWidth parameter, where this is explained a bit better in the comment - PTAL. The issue is a bit that the bus integrity feature is only implicitly visible here, e.g. in the data width that is 32+7 bits.

Fixes part of lowRISC#20887

Signed-off-by: Michael Schaffner <[email protected]>
Fixes part of lowRISC#20887

Signed-off-by: Michael Schaffner <[email protected]>
@msfschaffner msfschaffner changed the title [regtool/rtl] Add missing CM annotations and promote annotation warning to error [regtool/rtl] Add missing CM annotations Feb 14, 2024
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

That makes lots of sense, thanks!

@msfschaffner msfschaffner merged commit dc05d4c into lowRISC:master Feb 14, 2024
32 checks passed
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.

[dv] Add countermeasure annotations in RTL
4 participants