-
Notifications
You must be signed in to change notification settings - Fork 777
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
[rv_core_ibex, top_earlgrey] Enable SecureIbex for CW340 #25146
base: master
Are you sure you want to change the base?
Conversation
f426eb6
to
c3ed16d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nasahlpa for looking into this. Do you know by how much the resource utilization increases roughtly?
The changes look mostly good, but there are FPGA CI failures and we should make sure all tests pass before merging this.
% elif target["name"] == "cw310": | ||
.KmacEnMasking(0), | ||
.KmacSwKeyMasked(1), | ||
.KeymgrKmacEnMasking(0), | ||
.SecKmacCmdDelay(0), | ||
.SecKmacIdleAcceptSwMsg(1'b0), | ||
.RvCoreIbexSecureIbex(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably also should add that for the cw305.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks, done.
About that: I get the same error on a local CW340 with the new bitstream: |
c3ed16d
to
4369e7c
Compare
I've updated the PR description with area and timing numbers before and after this PR. |
Thanks for adding the area and timing numbers. I think the change is reasonable for the big board. About the SFDP header issue, I have no idea abou this. Maybe someone from the software team knows? |
That likely means bootstrapping failed at the very first step, and the chip gave no response to SPI. MISO was left high when the tester tried to read the SFDP table. For the prior PR state and its associated CI jobs, I found no evidence any software had run on CW340's Ibex. The ROM did not seem to print any identification messages. |
Thanks @a-will. Do you know how I can debug this? |
@nasahlpa what test failed? Can't find the older CI logs and still awaiting the run for the newer one. At a guess the bootloader has immediately fallen over because we get an instant alert from Ibex, maybe because the lockstep hasn't come out of reset properly? There's some prims involved that may have FPGA specific implementations that could explain the behaviour difference. |
All of them. Not a single test on CW340 passed. Here are the old results from CI. |
I investigated this and discovered the following: When enabling the When either setting the My assumption is that the In the long term, we should investigate this and fix it. As a temporary solution, we could bypass the error:
I tested both solutions and they work. WDYT @GregAC? Edit: ah, after discussing with @vogelpi about this I think the error is here: |
@nasahlpa nice work on diagnosing the issue. I'd favour just using the FF register file in FPGA, assuming the FPGA implementation still works. The reason to use the FPGA RF is efficient resource utilization I don't think switching to the FF version will have a meaningful effect given the full earl grey design is huge. We should work out why the FPGA version is broken in this instance. I'm happy to take a look at this. And having written that just seen the edit! If that's the quick fix then great go for it, otherwise switch to FF version would be my vote. |
We should use `WordZeroVal` instead of `0` for reads from register `x0` in the FPGA register file. This bug was discovered when enabling the `RegFileECC` parameter. When this is enabled, the core performs ECC checks, expecting that `WordZeroVal` is returned for `x0`. Else, we get a major alert. Fixes lowRISC/opentitan#25146 Signed-off-by: Pascal Nasahl <[email protected]>
The error was indeed the zero value for Thanks all for helping me debugging this issue. |
We should use `WordZeroVal` instead of `0` for reads from register `x0` in the FPGA register file. This bug was discovered when enabling the `RegFileECC` parameter. When this is enabled, the core performs ECC checks, expecting that `WordZeroVal` is returned for `x0`. Else, we get a major alert. Fixes lowRISC/opentitan#25146 Signed-off-by: Pascal Nasahl <[email protected]>
4369e7c
to
60e27e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
270a89d
to
4e32e45
Compare
As the `SecureIbex` configuration is the one we use for the Earl Grey ASIC, we also should enable it for the FPGA boards. Due to resource constraints, this is only possible for the CW340 but not the CW310. Resource utilization stays still will below 70%. Closes lowRISC#25137 Signed-off-by: Pascal Nasahl <[email protected]>
4e32e45
to
a020f8b
Compare
Closes lowRISC#20119. Signed-off-by: Pascal Nasahl <[email protected]>
a020f8b
to
5ad1420
Compare
localparam bit RegFileECC = SecureIbex; | ||
localparam bit RegFileECC = 1'b0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should be removed before merging this.
As the
SecureIbex
configuration is the one we use for the Earl Grey ASIC, we also should enable it for the FPGA boards.Due to resource constraints, this is only possible for the CW340 but not the CW310.
Report for CW340 with SecureIbex enabled:
Resource utilization stays still well below 70%:
And timing is met:
Report for CW340 with SecureIbex disabled:
Closes #25137
Furthermore, to test this feature, this PR also adds the CW340 exec. environment for the otbn_scramble_test to close #20119.