[keymgr_dpe, sw] Support metadata register in various driver by rroth-lowrisc · Pull Request #31356 · lowRISC/opentitan

rroth-lowrisc · 2026-09-17T06:20:14Z

This PR introduces the required driver modification for #31228. It implements the read metadata functionality in silicon_creator / cryptolib / dif libraries. To test the feature the keymgr_dpe_functest is extended to verify the boot_stage of the derived DPE context.

This PR depends on:

Only the last 5 commits are relevant for this PR.

gautschimi

Only minor comments. What is the reason for having two versions of keymgr_dpe.c/h?

gautschimi

Thanks for this PR. Looks mostly good. only minor comments:

unittests

There are no unit tests, and some new code has no callers.

dif_keymgr_dpe_unittest.cc and silicon_creator/lib/drivers/keymgr_dpe_unittest.cc both exist, but neither gets a test for the new functions.

keymgr_dpe_testutils_check_metadata() and the cryptolib keymgr_dpe_get_metadata() aren't called anywhere in this PR. Is there already a followup PR that uses them? Even if not, it's mostly debug functionality. so probably not required.

bazel deps

keymgr_dpe_functest may be missing a Bazel dependency. It now includes silicon_creator/lib/manifest.h and manifest_def.h, but the PR doesn't touch sw/device/silicon_creator/lib/drivers/BUILD.

It probably builds because ottf_main pulls in test_framework_manifest_def indirectly.
Adding a direct dependency on //sw/device/silicon_creator/lib:manifest would be cleaner.

Nit: the types differ between layers.

The DIF uses uint32_t for boot_stage and valid.
The cryptolib uses a bool plus its own enum.
silicon_creator uses its own enum with uint32_t valid.

Could we just define this boot_stage enum once? Aligns with an earlier comment of mine.

I don't fully understand why we have two versions of keymgr_dpe.c/h @nasahlpa is that intended?

rroth-lowrisc · 2026-10-06T08:06:55Z

Thanks for this PR. Looks mostly good. only minor comments:
1. unittests
There are no unit tests, and some new code has no callers.

dif_keymgr_dpe_unittest.cc and silicon_creator/lib/drivers/keymgr_dpe_unittest.cc both exist, but neither gets a test for the new functions.

keymgr_dpe_testutils_check_metadata() and the cryptolib keymgr_dpe_get_metadata() aren't called anywhere in this PR. Is there already a followup PR that uses them? Even if not, it's mostly debug functionality. so probably not required.
2. bazel deps
keymgr_dpe_functest may be missing a Bazel dependency. It now includes silicon_creator/lib/manifest.h and manifest_def.h, but the PR doesn't touch sw/device/silicon_creator/lib/drivers/BUILD.

It probably builds because ottf_main pulls in test_framework_manifest_def indirectly. Adding a direct dependency on //sw/device/silicon_creator/lib:manifest would be cleaner.
3. Nit: the types differ between layers.
The DIF uses uint32_t for boot_stage and valid. The cryptolib uses a bool plus its own enum. silicon_creator uses its own enum with uint32_t valid.

Could we just define this boot_stage enum once? Aligns with an earlier comment of mine.

I don't fully understand why we have two versions of keymgr_dpe.c/h @nasahlpa is that intended?

Thanks for the review @gautschimi

I extended silicon_creator/lib/drivers/keymgr_dpe_unittest.cc to check the new functions. Currently the dif_keymgr_dpe_unittest.cc is empty, so this will be fixed in a follow-up PR. keymgr_dpe_testutils_check_metadata() and the cryptolib keymgr_dpe_get_metadata() will be used in [keymgr_dpe, rom/rom_ext] Verify boot_stage of UDS / CDI_0 / CDI_1 DPE context #31357
Fixed the missing dependency
I had an offline disscussion with @nasahlpa: silicon_creator is fixed for ROM during tape-out and will not be changed afterwards. The cryptolib will evolve after tape-out, so nothing can be shared between the two.
I will change the silicon driver to use bool for the valid too.

gautschimi

Cool! Thanks for the additional unittests, clarifications and the enum alignment.

All my comments have been addressed. Approving this PR.

andrea-caforio

Thanks @rroth-lowrisc. Some questions and remarks from my side.

andrea-caforio · 2026-10-06T12:09:22Z

+  ptrdiff_t offset_low = (ptrdiff_t)(KEYMGR_DPE_METADATA_LOW_0_REG_OFFSET +
+                                     slot * sizeof(uint32_t));
+  ptrdiff_t offset_high = (ptrdiff_t)(KEYMGR_DPE_METADATA_HIGH_0_REG_OFFSET +
+                                      slot * sizeof(uint32_t));
+
+  metadata->max_key_version =
+      mmio_region_read32(keymgr_dpe->base_addr, offset_low);


Why can't you do it like in the driver function to get the register addresses?

I removed the intermediate variables offset_low and offset_high. However, the dif doesn't have access to the abs_mmio_read32() function which uses an absolute uint32_t address.

andrea-caforio · 2026-10-06T12:12:23Z

+ * Checks that a keymgr_dpe HW slot holds a valid DPE context with the expected
+ * maximum key version, boot stage and slot policy.
+ */
+rom_error_t sc_keymgr_dpe_check_metadata(


Bit of a redundant function to be honest. If you really need a test then do the verification step by in the test itself.

This function is actually the main feature. The goal of this feature is to verify if a derivation was properly done by checking all the metadata after the derivation.

IMO it doesn't make sense to implement this function multiple times.

One example for this function can be seen in #31357. I'm verifying if the derivation in the ROM code was successfully (bcf6bca).

…tadata Read the max key version, validity, boot stage and policy of one slot. Signed-off-by: Raphael Roth <rroth@lowrisc.org>

Check that a slot holds a valid DPE context with the expected max key version, boot stage and policy, to confirm a derivation happened. Signed-off-by: Raphael Roth <rroth@lowrisc.org>

Add `sc_keymgr_dpe_get_metadata()` and `sc_keymgr_dpe_check_metadata()`, both hardened against FI on the slot index and the checked fields. Signed-off-by: Raphael Roth <rroth@lowrisc.org>

Verify the DPE context after each advance operation with `sc_keymgr_dpe_boot_stage_check()`. This ensures that the derivation was done correctly. Signed-off-by: Raphael Roth <rroth@lowrisc.org>

Read the max key version, validity, boot stage and policy of one slot. Out-of-range slots return `OTCRYPTO_BAD_ARGS` behind a hardened check. Signed-off-by: Raphael Roth <rroth@lowrisc.org>

rroth-lowrisc · 2026-10-06T14:13:35Z

Thanks @andrea-caforio for the review

I changed the code accordingly and answered your questions

andrea-caforio

Thanks @rroth-lowrisc.

rroth-lowrisc self-assigned this Sep 17, 2026

rroth-lowrisc added the IP:keymgr_dpe label Sep 17, 2026

rroth-lowrisc requested a review from gautschimi September 17, 2026 06:21

This was referenced Sep 17, 2026

[keymgr_dpe, testenv.] Verify boot_stage_counter for DPE context #30759

Open

[keymgr_dpe, rom/rom_ext] Verify boot_stage of UDS / CDI_0 / CDI_1 DPE context #31357

Draft

rroth-lowrisc force-pushed the keymgr_dpe_feat_read_metadata_sw branch from ba60964 to fad7df1 Compare September 23, 2026 10:52

rroth-lowrisc mentioned this pull request Sep 25, 2026

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

Merged

gautschimi reviewed Sep 25, 2026

View reviewed changes

Comment thread sw/device/lib/crypto/drivers/keymgr_dpe.h Outdated

Comment thread sw/device/lib/crypto/drivers/keymgr_dpe.h

rroth-lowrisc force-pushed the keymgr_dpe_feat_read_metadata_sw branch from fad7df1 to fd10c1b Compare September 29, 2026 09:54

rroth-lowrisc marked this pull request as ready for review October 5, 2026 07:47

rroth-lowrisc requested review from a team as code owners October 5, 2026 07:47

rroth-lowrisc requested review from a team, KinzaQamar, jwnrt and moidx and removed request for a team, KinzaQamar, jwnrt and moidx October 5, 2026 07:47

rroth-lowrisc force-pushed the keymgr_dpe_feat_read_metadata_sw branch 2 times, most recently from 75f56fa to 77f45d8 Compare October 5, 2026 15:46

gautschimi reviewed Oct 5, 2026

View reviewed changes

Comment thread sw/device/lib/crypto/drivers/keymgr_dpe.c

rroth-lowrisc requested review from andrea-caforio and nasahlpa October 6, 2026 08:06

nasahlpa requested a review from siemen11 October 6, 2026 08:11

rroth-lowrisc force-pushed the keymgr_dpe_feat_read_metadata_sw branch from 77f45d8 to 8486540 Compare October 6, 2026 08:24

gautschimi approved these changes Oct 6, 2026

View reviewed changes

rroth-lowrisc force-pushed the keymgr_dpe_feat_read_metadata_sw branch from 8486540 to 4cf558e Compare October 6, 2026 08:31

andrea-caforio reviewed Oct 6, 2026

View reviewed changes

rroth-lowrisc added 5 commits October 6, 2026 15:58

[keymgr_dpe, sw] Add dif_keymgr_dpe_get_metadata to read HW slot me…

6ff6a43

…tadata Read the max key version, validity, boot stage and policy of one slot. Signed-off-by: Raphael Roth <rroth@lowrisc.org>

[keymgr_dpe, sw] Add keymgr_dpe_testutils_check_metadata to testutils

5f18d23

Check that a slot holds a valid DPE context with the expected max key version, boot stage and policy, to confirm a derivation happened. Signed-off-by: Raphael Roth <rroth@lowrisc.org>

[keymgr_dpe, sw] Add sc_keymgr_dpe_get_metadata driver function

96699d9

Add `sc_keymgr_dpe_get_metadata()` and `sc_keymgr_dpe_check_metadata()`, both hardened against FI on the slot index and the checked fields. Signed-off-by: Raphael Roth <rroth@lowrisc.org>

[keymgr_dpe, sw] Verify boot_stage in keymgr_dpe_functest

1ad570c

Verify the DPE context after each advance operation with `sc_keymgr_dpe_boot_stage_check()`. This ensures that the derivation was done correctly. Signed-off-by: Raphael Roth <rroth@lowrisc.org>

[keymgr_dpe, sw] Add keymgr_dpe_get_metadata to cryptolib driver

adfbe42

Read the max key version, validity, boot stage and policy of one slot. Out-of-range slots return `OTCRYPTO_BAD_ARGS` behind a hardened check. Signed-off-by: Raphael Roth <rroth@lowrisc.org>

rroth-lowrisc force-pushed the keymgr_dpe_feat_read_metadata_sw branch from 4cf558e to adfbe42 Compare October 6, 2026 14:00

rroth-lowrisc requested a review from a team as a code owner October 6, 2026 14:00

rroth-lowrisc requested review from sasdf and removed request for a team and sasdf October 6, 2026 14:00

andrea-caforio approved these changes Oct 6, 2026

View reviewed changes

rroth-lowrisc added the CI:Rerun Rerun failed CI jobs label Oct 7, 2026

github-actions Bot removed the CI:Rerun Rerun failed CI jobs label Oct 7, 2026

rroth-lowrisc added the CI:Rerun Rerun failed CI jobs label Oct 8, 2026

github-actions Bot removed the CI:Rerun Rerun failed CI jobs label Oct 8, 2026

Conversation

rroth-lowrisc commented Sep 17, 2026

Uh oh!

gautschimi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gautschimi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rroth-lowrisc commented Oct 6, 2026 • edited by nasahlpa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gautschimi left a comment

Choose a reason for hiding this comment

Uh oh!

andrea-caforio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrea-caforio Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

rroth-lowrisc Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

andrea-caforio Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

rroth-lowrisc Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

rroth-lowrisc commented Oct 6, 2026

Uh oh!

andrea-caforio left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rroth-lowrisc commented Oct 6, 2026 •

edited by nasahlpa

Loading