Skip to content

[keymgr_dpe, rtl] Introduce registers to read slot metadata - #31228

Merged
andreaskurth merged 3 commits into
lowRISC:masterfrom
rroth-lowrisc:keymgr_dpe_feat_read_metadata
Oct 5, 2026
Merged

andreaskurth merged 3 commits into
lowRISC:masterfrom
rroth-lowrisc:keymgr_dpe_feat_read_metadata

Conversation

@rroth-lowrisc

@rroth-lowrisc rroth-lowrisc commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

This PR introduces HW register which allows sw to read the metadata for each hw slot. It was aggreed in this RFC. The sw side will be implemented in a follow up PR.

This PR depends on:

Only the last 5 3 commits are relevant for this PR.

edit: prior to merge the commit marked [SQUASH_ME] must be squashed for a clean history.

@rroth-lowrisc

Copy link
Copy Markdown
Contributor Author

Note: I will need the function extract_metadata_from_slot(...) later on in another PR too. To avoid code duplication I wrote a function (even if its called only once for now)

@rroth-lowrisc
rroth-lowrisc marked this pull request as ready for review September 23, 2026 08:13
@rroth-lowrisc
rroth-lowrisc requested review from a team as code owners September 23, 2026 08:13
@rroth-lowrisc
rroth-lowrisc requested review from a team, hcallahan-lowrisc, jwnrt and pamaury and removed request for a team, hcallahan-lowrisc, jwnrt and pamaury September 23, 2026 08:13

@gautschimi gautschimi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this PR. It is mostly good. Just a few comments:

  1. Instead of METADATA_LOW/HIGH we could call the registers SLOT_KEY_VERSION, SLOT_METADATA. But no strong opinion about it. What do you think?

  2. are there any security concerns of exposing this information? If yes, do we need an option to disable this output? E.g. a regwen that just ties the info to 0.

  3. I think at the moment we add around 300 registers for this block. half of them might get optimized away because they are constant 0. But I believe we can get rid of all of them by defining the registers to be external (in the slot). Would be worth to synthesize this.

  4. Can we create a sw-issue to update the dif with this new functionality? (Maybe it already exists. in that case just link it)

Comment thread hw/ip/keymgr_dpe/data/keymgr_dpe.hjson Outdated
Comment thread hw/ip/keymgr_dpe/data/keymgr_dpe.hjson
Comment thread hw/ip/keymgr_dpe/data/keymgr_dpe.hjson
Comment thread hw/ip/keymgr_dpe/data/keymgr_dpe.hjson Outdated
Comment thread hw/ip/keymgr_dpe/rtl/keymgr_dpe.sv Outdated
Comment thread hw/ip/keymgr_dpe/rtl/keymgr_dpe.sv Outdated
Comment thread hw/ip/keymgr_dpe/rtl/keymgr_dpe.sv
@rroth-lowrisc

Copy link
Copy Markdown
Contributor Author

Thanks @gautschimi for your review, I will take a look.

  1. I prefer the solution with _HIGH / _LOW. SLOT_KEY_VERSION sounds like it doesn't belong to the metadata.
  2. I talked about this with @nasahlpa. AFAIR reading from these register requires root access and if you manage to get root access you can do far worst than reading these register. But not having one and finding out afterwards that you would have needed one fells also a bit wrong. @nasahlpa What do you think about this?
  3. Yes, I didn't know the hwext: "true" option
  4. [keymgr_dpe, sw] Support metadata register in various driver #31356 introduces the feature in all drivers while [keymgr_dpe, rom/rom_ext] Verify boot_stage of UDS / CDI_0 / CDI_1 DPE context #31357 uses the feature in ROM

@gautschimi

gautschimi commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Thanks @gautschimi for your review, I will take a look.

1. I prefer the solution with `_HIGH` / `_LOW`. `SLOT_KEY_VERSION` sounds like it doesn't belong to the metadata.

2. I talked about this with @nasahlpa. AFAIR reading from these register requires root access and if you manage to get root access you can do far worst than reading these register. But not having one and finding out afterwards that you would have needed one fells also a bit wrong. @nasahlpa What do you think about this?

3. Yes, I didn't know the `hwext: "true"` option

4. [[keymgr_dpe, sw] Support metadata register in various driver #31356](https://github.com/lowRISC/opentitan/pull/31356) introduces the feature in all drivers while [[keymgr_dpe, rom/rom_ext] Verify `boot_stage` of UDS / CDI_0 / CDI_1 DPE context #31357](https://github.com/lowRISC/opentitan/pull/31357) uses the feature in `ROM`

The register names (1) are ok, let's leave them as they are.
Good that (2) was already discussed with @nasahlpa. For me either solution is fine.
Thanks, I've just reviewed the software PR (4):)

@andreaskurth andreaskurth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @rroth-lowrisc for this PR! Exposing the slot metadata lets SW see each slot's boot stage again, and the change is small and in line with the RFC. Beyond the points @gautschimi already raised, I only have minor comments.

Am I right that nothing verifies the new registers yet? No sequence reads METADATA_*, the scoreboard handles them in its default branch ("isn't handled", no read check), and the testplan has no entry for them. The CSR tests pass locally, but they only cover reset values and RO behaviour. Could you open an issue to track this (e.g., linked to #30753)?

Comment thread hw/ip/keymgr_dpe/data/keymgr_dpe.hjson Outdated
Comment thread hw/ip/keymgr_dpe/data/keymgr_dpe.hjson Outdated
Comment thread hw/ip/keymgr_dpe/data/keymgr_dpe.hjson
Comment thread hw/ip/keymgr_dpe/data/keymgr_dpe.hjson Outdated
These registers enable the metadata for each DPE context stored
inside the HW slot to be read. For any uninstantiated hw slot the
register will return `0` for all entries.

Signed-off-by: Raphael Roth <rroth@lowrisc.org>
Signed-off-by: Raphael Roth <rroth@lowrisc.org>
Signed-off-by: Raphael Roth <rroth@lowrisc.org>
@rroth-lowrisc
rroth-lowrisc force-pushed the keymgr_dpe_feat_read_metadata branch from 508ccd4 to 7b8893b Compare September 28, 2026 12:54
@rroth-lowrisc

Copy link
Copy Markdown
Contributor Author

Thanks @rroth-lowrisc for this PR! Exposing the slot metadata lets SW see each slot's boot stage again, and the change is small and in line with the RFC. Beyond the points @gautschimi already raised, I only have minor comments.

Am I right that nothing verifies the new registers yet? No sequence reads METADATA_*, the scoreboard handles them in its default branch ("isn't handled", no read check), and the testplan has no entry for them. The CSR tests pass locally, but they only cover reset values and RO behaviour. Could you open an issue to track this (e.g., linked to #30753)?

Thanks for the review @andreaskurth - I implemented your suggestions and squashed the SQUASH_ME commits.
I opened issue #31512 to track the verification of these register.
PR #31357 uses this feature to verify the boot_stage for context derived in ROM / ROM_EXT

@andreaskurth andreaskurth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @rroth-lowrisc!

@andreaskurth

Copy link
Copy Markdown
Contributor

CHANGE AUTHORIZED: hw/top_earlgrey/ip/xbar_main/rtl/autogen/tl_main_pkg.sv
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey_pkg.sv

The new METADATA_LOW/METADATA_HIGH registers double the keymgr_dpe register space from 0x100 to 0x200, so topgen regenerated the crossbar address mask and the peripheral size. The base address is unchanged and the new range fits the existing 64 KiB peripheral slot.

@vogelpi

vogelpi commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

CHANGE AUTHORIZED: hw/top_earlgrey/ip/xbar_main/rtl/autogen/tl_main_pkg.sv
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey_pkg.sv

The new METADATA_LOW/METADATA_HIGH registers double the keymgr_dpe register space from 0x100 to 0x200, so topgen regenerated the crossbar address mask and the peripheral size. The base address is unchanged and the new range fits the existing 64 KiB peripheral slot.

@vogelpi vogelpi added the CI:Rerun Rerun failed CI jobs label Oct 2, 2026
@github-actions github-actions Bot removed the CI:Rerun Rerun failed CI jobs label Oct 2, 2026
@andreaskurth
andreaskurth added this pull request to the merge queue Oct 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 2, 2026
@andreaskurth
andreaskurth added this pull request to the merge queue Oct 5, 2026
Merged via the queue into lowRISC:master with commit 38b1fdd Oct 5, 2026
57 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants