[hw] Integrate KMAC for ROM checking#590
Conversation
ba3a986 to
51d0c97
Compare
engdoreis
left a comment
There was a problem hiding this comment.
The Software bits all looks good to me.
marnovandermaas
left a comment
There was a problem hiding this comment.
Great work! Some initial comments from my end.
| {from: "hw/ip/edn", to: "ip/edn"}, // EDN (KMAC dependency). | ||
| {from: "hw/ip/keymgr", to: "ip/keymgr"}, // Key Manager (KMAC dependency). | ||
| {from: "hw/ip/csrng", to: "ip/csrng"}, // CSRNG (KMAC dependency). |
There was a problem hiding this comment.
Is there any way that we can avoid pulling these IP blocks in. Maybe we can just patch out the need for key manager, EDN and CSRNG?
There was a problem hiding this comment.
A quick search for package names in the vendored KMAC files show that these would need to be patched: kmac.sv (5 lines), kmac_entropy.sv (5 lines), kmac_app.sv (3 lines).
There was a problem hiding this comment.
Would you mind attempting this patch so we avoid pulling in these extra three dependencies?
There was a problem hiding this comment.
Achieved by patching the few bits required into kmac_pkg.sv
| localparam int unsigned SPIHostIrqs = 2; | ||
| localparam int unsigned EntropySrcIrqs = 4; | ||
| localparam int unsigned KmacNumAppIntf = 1; | ||
| localparam int unsigned KmacNumAppIntf = 3; // system only needs 1 (rom_ctrl), but KMAC expects 3 |
There was a problem hiding this comment.
How baked in is this assumption? How many lines would need to change to just have 1?
There was a problem hiding this comment.
From what I remember, there was at least one primitive that would either need to be patched itself to fix or be patched out of KMAC, so not trivial. I haven't fully explored how many other things may need to be changed.
There was a problem hiding this comment.
Ok, then this may just not be worth the hassle. It can be an improvement we do later if we need to save area. Maybe we can open an issue to track this?
| COMMAND srec_cat "$<TARGET_FILE:${NAME}>.bin" -binary -byte-swap 4 | ||
| -o "$<TARGET_FILE:${NAME}>.vmem" -vmem 32 |
There was a problem hiding this comment.
Can we remove this file so that we don't accidentally use the non-scrambled version?
There was a problem hiding this comment.
@engdoreis Do we still need unscrambled bootrom.vmem files to be built once we switch to scrambled ROM in HW?
There was a problem hiding this comment.
We don't need it for anything else, so I don't mind removing it.
There was a problem hiding this comment.
Thanks, I will remove the srec_cat call in the upcoming change bundle
There was a problem hiding this comment.
To answer my own question the DV tests are failing:
Open failed on file "bootrom_scrambled.vmem". No such file or directory
UVM_FATAL @ *: (mem_bkdr_util.sv:658) [mem_bkdr_util[ChipMemROM]] file /home/mvdmaas/mocha/scratch/kmac/top_chip_system.sim.xcelium/*.pwr mgr_smoke/latest/bootrom_scrambled.vmem could not be opened for r modehas 1 failure
I'm marking this as request changes just to avoid us merging this, but still very much want this PR to be merged.
Yes this has been my main priority, but I hadn't got to the bottom of it yet. I evidently hadn't run a top-level UVM test as I thought I had. Fixing the filename issue reveals that alert assertions are firing in KMAC, which I'm attempting to investigate. The alerts are disconnected, so I'm guessing this is a case of something unexpectedly causing an alert to fire. |
|
Got it. I'll include the fixes in my update to this PR later today, once I've hopefully got some other changes in to avoid churn. |
Rather than vendoring dependencies of KMAC not already in Mocha (keymgr, edn, csrng), patch kmac_pkg.sv to include the minor bits required from the keymgr and edn packages. Mocha doesn't need these modules in full as we are only using KMAC to check the contents of the scrambled ROM. Also, fix the ROM image scrambling script import paths for Mocha.
Integrate KMAC hardware block for use with the ROM checker only. No other KMAC interfaces are connected. Modify the SW build system to generate a scrambled boot-ROM image. Attempting to run an un-scrambled image will be blocked by the in-hardware ROM checker. Add minimal HJSON files required for ROM image scrambling script. These use the OT testing secrets for now.
|
marnovandermaas
left a comment
There was a problem hiding this comment.
Very cool, just one more comment from my end. I'll need to re-run the DV as well.
| // Unused Interfaces | ||
| .tl_i (tlul_pkg::TL_H2D_DEFAULT), | ||
| .tl_o ( ), | ||
| .alert_rx_i ({prim_alert_pkg::ALERT_RX_DEFAULT, prim_alert_pkg::ALERT_RX_DEFAULT}), |
There was a problem hiding this comment.
Instead of repeating can we use something like:
'{default: prim_alert_pkg::ALERT_RX_DEFAULT}
We should probably replace that in all the other places as well because alert_rx_i is an array in all OpenTitan blocks, even if most of them only have one alert.
There was a problem hiding this comment.
Funny you should suggest that, because that's exactly what I did in my first set of changes. Unfortunately, that resulted in the top-level UVM tests failing due to an alert assertion firing in KMAC due to incorrectly assigned alert_rx signals (both _n and _p high). I still don't understand why the '{default: ...} structure causes problems for kmac but not e.g. rstmgr despite using the exact same interface. I'll continue investigating now I know this duplication is something you want to avoid.
There was a problem hiding this comment.
@marnovandermaas It seems using a type-key rather than the default-key works:
.alert_rx_i ('{prim_alert_pkg::alert_rx_t: prim_alert_pkg::ALERT_RX_DEFAULT}),
This avoids duplication, while being more specific about the members to assign.
Would this be an acceptable solution for you?
There was a problem hiding this comment.
Nevermind, Verilator doesn't like it
Top-level DV is now passing with the latest changes.
|
How clever I thought myself, to use a nifty SystemVerilog feature. |
ziuziakowska
left a comment
There was a problem hiding this comment.
I'm happy with the SW side.
Add KMAC to Mocha and use it to enable ROM scrambling.
See individual commits for details.