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

[tlgen] Do not enforce a minimum region size #15192

Merged
merged 3 commits into from
Sep 29, 2022

Conversation

msfschaffner
Copy link
Contributor

@msfschaffner msfschaffner commented Sep 29, 2022

This addresses #15182. I ran the chip_tl_errors test with 200 different seeds and did not see any failures.

Note that only the last two commits are relevant. Others will be rebased away.

Fix #15182 #14200

@msfschaffner msfschaffner force-pushed the xbar-addrs branch 2 times, most recently from d28b0be to 6257d63 Compare September 29, 2022 01:06
@msfschaffner msfschaffner marked this pull request as ready for review September 29, 2022 01:09
@msfschaffner msfschaffner requested review from jdonjdon, jadephilipoom, sriyerg, a-will and weicaiyang and removed request for a team, jdonjdon and jadephilipoom September 29, 2022 01:09
@@ -833,7 +833,7 @@ ast_reg_top u_reg (
.reg2hw ( reg2hw ),
.hw2reg ( hw2reg ),
.intg_err_o ( intg_err ),
.devmode_i ( 1'b0 )
.devmode_i ( 1'b1 )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tjaychen @arnonsha @Jacob-Levy FYI.
With this set to 0 we get many errors in the chip_tl_errors test since the CSR node does not error back.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that all other modules indeed have devmode=1, which means they report errors when illegal transaction occurs. From security point of view, isn't this what we're looking for ? I don't see the value of devmode = 0, can you explain why when masked errors can/should be used ?

Copy link
Contributor

@Jacob-Levy Jacob-Levy Sep 29, 2022

Choose a reason for hiding this comment

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

@msfschaffner The value or .devmode_i ( 1'b0 ) was given by Google in the past for the AST... what changed?
Should the change apply to the close source as well?
The ast.sv is an A U T O - G E N E R A T E D C O D E !! file!

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me explain the history of devmode=0. The feature is not fully baked into OT design yet. But the intention is to not disclose which address space is unmapped or mapped but all 0 for security sensitive modules. So, for the security sensitive modules should have devmode tied to 0 regardless of LC states. Other peripheral should have devmode controlled by life cycle state. @tjaychen may comment more in detail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @eunchan
This makes sense, however all modules that I can see are connected to 1'b1, including keymgr, kmac, lc and otbn.

Choose a reason for hiding this comment

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

this was indeed the original intent. However we decided (somewhere along the line) that debuggability trumps security, at least for this first generation.

So we have tied everyone to 1'b1 for now. In the future we may tie it back to 1'b0.

This whole thing though requires a higher level discussion, because there are a lot of architectural inconsistencies in how we include the process in the security envelope.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense for the 1st gen. Thanks @tjaychen

Copy link
Contributor

@Jacob-Levy Jacob-Levy Sep 29, 2022

Choose a reason for hiding this comment

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

@eunchan Thanks for explaining!
@msfschaffner is asking to set devmode=1 that is why I asked "what changed"..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference, this has been discussed in these issues: #14063, #10311

@vogelpi
Copy link
Contributor

vogelpi commented Sep 29, 2022

Thanks for giving this a try @msfschaffner. Do you know if this has an impact on timing?

@tjaychen
Copy link

this looks good to me. I raised the question in the issue also, but do we expect a timing issue? Because it looks like you're still rounding up to the closest power of 2, so it's not going we're going to a bounds comparison right? it's still mostly address bits. Just fewer of them.

@msfschaffner
Copy link
Contributor Author

msfschaffner commented Sep 29, 2022

Thanks for giving this a try @msfschaffner. Do you know if this has an impact on timing?

I haven't run this through ASIC synthesis, but as @tjaychen points out, the address routing is still done by masking off the relevant bits and does not require full comparisons.

@msfschaffner msfschaffner force-pushed the xbar-addrs branch 4 times, most recently from d2ead91 to a81aeb1 Compare September 29, 2022 19:18
@msfschaffner msfschaffner force-pushed the xbar-addrs branch 2 times, most recently from 797ee8f to 761045b Compare September 29, 2022 19:58
@msfschaffner
Copy link
Contributor Author

I have added an additional address alignment / overlap check that uses 0x1000 as before so that we can still enforce this at the system level (see last commit). The check does not alter any of the existing config, but just goes over the base addresses and throws an error if anything is amiss.

Copy link
Contributor

@weicaiyang weicaiyang left a comment

Choose a reason for hiding this comment

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

LGTM

This enhances the address alignment and overlap checks
so that a minimum spacing can be enforced at the system
level.

Signed-off-by: Michael Schaffner <[email protected]>
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.

[tlul] How to properly generate errors in address space "holes"
7 participants