`ot_otp`: unify implementations #272

rivos-eblot · 2025-10-30T17:59:30Z

This PR generalizes OTP implementation, so that code is no longer duplicated for each top.

Most of the code is now implemented in a new component OtOTPEngineState (ot_otp_engine.c) which contains over 4/5 of the duplicated code that used to exist in ot_otp_eg.c and ot_otp_dj.c files.

Those two latter files now only contain what is specific to each Top:

register definitions (IO regs and SW CFG regs) + their reset values
partition definitions
key seed definition
HW cfg exposed to other "HW" clients
LC controller tokens
some lightweight glue logic

Some note about the interfaces/classes:

OtOTPIf(Class) interface exposes the public interface of the OTP, i.e. the API that other OpenTitan "HW" devices can use to communicate with the OTP
OtOTPImplIf(Class) interface exposes the semi- private interface that a top-specific OTP implementation exposes to the OtOTPEngineState
OtOtpBeIf(Class) interface exposes the public interface of an OTP backend, which describes the characteristics of a specific backend. Note that ideally, this component should manage the QEMU OTP image file, which is not (yet) the case.
OtOTPEngineState/Class implements the whole OTP emulation. It is an abstract class whose concrete implementation is Top-specific.
OtOTPEgState/Class and OtOTPDjState/Class are two concrete implementations of OtOTPEngineState/Class. Each implements the implements the OtOTPIf interface which is exposed to the OTP clients, and implement the OtOTPImplIf to provide the features required by their abstract class OtOTPEngineState/Class.

loiclefort · 2025-10-31T09:42:22Z

hw/opentitan/ot_keymgr_dpe.c

+    (void)OBJECT_CHECK(OtOTPIf, s->otp_ctrl, TYPE_OT_OTP_IF);
+


Is this check required? Doesn't DEFINE_PROP_LINK already add a type check?

"Interfaces" property are assigned to generic device, the top-most level kind, DeviceState *. QEMU checks on property assignment that the object is a DeviceState, but not the interfaces the device supports. This is very specific to interfaces

loiclefort · 2025-10-31T09:47:53Z

include/hw/opentitan/ot_otp_impl_if.h

+    void (*update_status_error)(OtOTPImplIf *dev, OtOTPStatus error, bool set);
+
+    /*
+     * Signal the power management as triggered the OTP initialization sequence.


"Signal that power management has triggered"

loiclefort · 2025-10-31T10:01:55Z

hw/opentitan/ot_otp_dj.c


+// @otod add _ADDR or _OFFSET


typo (@todo)

loiclefort · 2025-10-31T10:09:41Z

hw/opentitan/ot_otp_engine.c

+         * "Note that if errors occur, we aggregate the error code but still
+         *  attempt to program all remaining words. This is done to ensure that
+         *  a life cycle state with ECC correctable errors in some words can
+         * still be scrapped."


missing a space before still

loiclefort · 2025-10-31T10:11:42Z

hw/opentitan/ot_otp_engine.c

+     * IP reset, which are exercised automatically from the SoC, since all the
+     * OT SysBysDevice IPs are connected to the private system bus of the Ibex.
+     * This is by-design in QEMU. The reset management is already far too
+     * complex to create a special case for the OTP. Kind in mind that the OTP


Kind in mind -> Keep in mind

loiclefort · 2025-10-31T10:12:52Z

hw/opentitan/ot_otp_engine.c

+         ERR_CODE_NAMES[(_err_)] : \
+         "?")
+
+/* todo: add assertion to validate those */


loiclefort · 2025-10-31T10:12:58Z

hw/opentitan/ot_otp_engine.c

+            if (level) {
+                DAI_CHANGE_STATE(s, OTP_DAI_ERROR);
+                LCI_CHANGE_STATE(s, OTP_LCI_ERROR);
+                /* TODO: manage other FSMs */


loiclefort · 2025-10-31T10:13:04Z

hw/opentitan/ot_otp_engine.c

+        pctrl->failed = true;
+        /* this is a fatal error */
+        ot_otp_engine_set_error(s, part_ix, OTP_CHECK_FAIL_ERROR);
+        /* TODO: revert buffered part to default */


loiclefort · 2025-10-31T10:13:13Z

hw/opentitan/ot_otp_engine.c

+    ibex_irq_set(&s->pwc_otp_rsp, 0);
+
+    for (unsigned part_ix = 0; part_ix < s->part_count; part_ix++) {
+        /* TODO: initialize with actual default partition data once known */


AlexJones0

Partial review (reviewed up to the "merge OTP implementation" commit, and I only looked at the ot_otp_engine.h and ot_otp_{dj,eg}_parts.c files in that commit so far).

AlexJones0 · 2025-10-31T14:26:29Z

hw/opentitan/ot_otp_dj.c

+            secret_addr = (unsigned)A_SECRET0_TEST_EXIT_TOKEN;
            break;
        case OTP_TOKEN_RMA:
            partition = OTP_PART_SECRET2;
-            secret_addr = A_SECRET2_RMA_TOKEN;
+            secret_addr = (unsigned)A_SECRET2_RMA_TOKEN;


Casts should be on the partition, not the secret_addr.

AlexJones0 · 2025-10-31T15:01:57Z

hw/opentitan/ot_entropy_src.c

@@ -1,5 +1,5 @@
 /*
- * QEMU OpenTitan Earlgrey 1.0.0 Entropy Source device
+ * QEMU OpenTitan Earlgrey Entropy Source device


This shouldn't even be "Earlgrey" anymore now these are unified, right?
The header file is correct ("QEMU OpenTitan Entropy Source device").

AlexJones0 · 2025-10-31T15:05:14Z

hw/opentitan/ot_otp_dj.c

 }

-static inline unsigned ot_otp_dj_part_data_offset(const OtOTPDjState *s, unsigned part_ix)
+static inline unsigned


Small nit: it's not very important (especially if it ends up being a lot of conflicts to fix), but really this formatter run should be on the earlier commit preparing the OTP for unification, not on the commit adding the interface. It makes it harder to follow the history by blaming (and harder to review!)

I have reformatted all the commits on this branch. Is that ok?

AlexJones0 · 2025-10-31T15:13:55Z

python/qemu/ot/otp/descriptor.py

-        print('#define OTP_PART_COUNT ARRAY_SIZE(OtOTPPartDescs)', file=cfp)
+        print('#define OTP_PART_COUNT ARRAY_SIZE(OT_OTP_PART_DESCS)', file=cfp)
        print(file=cfp)
+        key_seeds: list[str, str, int, int] = []


Typing nit - should be:

Suggested change

key_seeds: list[str, str, int, int] = []

key_seeds: list[tuple[str, str, int, int]] = []

?

AlexJones0 · 2025-10-31T15:22:39Z

hw/opentitan/ot_otp_dj.c


+// @todo add _ADDR or _OFFSET


Nit: QEMU comment style

I added this special comment not to forget to do it before pushing the PR. It seems it did not work as expected :-)

Even if it doesn't stop you pushing it, it gets pointed out in review at least :)

AlexJones0 · 2025-10-31T15:24:07Z

include/hw/opentitan/ot_otp_engine.h

@@ -0,0 +1,423 @@
+/*
+ * QEMU OpenTitan EarlGrey One Time Programmable (OTP) memory controller


Suggested change

* QEMU OpenTitan EarlGrey One Time Programmable (OTP) memory controller

* QEMU OpenTitan One Time Programmable (OTP) memory controller

AlexJones0 · 2025-10-31T15:30:09Z

hw/opentitan/ot_otp_engine.h

+
+    if (s->part_descs[part_ix].hw_digest || s->part_descs[part_ix].sw_digest) {
+        size -= sizeof(uint32_t) * NUM_DIGEST_WORDS;
+    }


Should also account for zeroizable words in zeroizable partitions?

Good catch.
I originally selected the DJ model to do the port, then I moved to EG as DJ does not have flash ... and I forgot about the zeroification altogether.

AlexJones0 · 2025-10-31T15:32:14Z

hw/opentitan/ot_otp_engine.h

+FIELD(DIRECT_ACCESS_CMD, RD, 0u, 1u)
+FIELD(DIRECT_ACCESS_CMD, WR, 1u, 1u)
+FIELD(DIRECT_ACCESS_CMD, DIGEST, 2u, 1u)
+FIELD(DIRECT_ACCESS_CMD, ZEROIZE, 3u, 1u)


Nit: the ZEROIZE field is not "common" in the Earlgrey 1.0.0 version (have not looked where this is used yet, maybe it does not end up being a problem).

I've updated the command, and added a flag in OtOTPImplIfClass that tracks support for this feature.

Note that this feature is not yet implemented (I mean in Darjeeling), only what's needed to use the partitions when such feature is defined. I've added a @todo.

AlexJones0 · 2025-10-31T15:33:56Z

hw/opentitan/ot_otp_dj_parts.c

+#define OTP_PART_COUNT ARRAY_SIZE(OT_OTP_PART_DESCS)
+
+static const OtOTPKeySeed OT_OTP_KEY_SEEDS[OTP_KEY_COUNT] = {
+    [OTP_KEY_OTBN] = {


(same for EG) Maybe put a comment here similar to the previous comment explaining the overlap with the SRAM key:

/* The OTBN scrambling key is derived from the SRAM scrambling key */

(could even mention a reference to OpenTitan's rtl/otp_ctrl_kdi.sv?

I think this is commented where those values are used. Is it required to add this here as well (more to fix if it evolves, as it needs to be in Python and in C)?

(@ line 1822, in ot_otp_engine_get_otp_key function)

It's probably ok, this is just the type of code that I would find when debugging and think "surely this must be a copy/paste error, why are the SRAM and OTBN key set to the same offset/size?" and then only find out after an hour of reading through the RTL that they are intentionally the same. I went through a similar experience to this going through the original RTL to implement these scrambling keys...

But the other comment is probably sufficient (I just hadn't got to reviewing that file yet - the downsides of doing partial reviews).

Leaving it open for now, if you think it is better to have it in Python, I'll add it.

Disable all LLVM rules as they hard-code paths from LLVM project. llvm-header-guard therefore suggests to replace header guard path with the full absolute path of the current local check out directory path! It appears that llvm-* rules are only to be used for the LLVM project itself, but they are nevertheless automatically added with the default `*` wildcard... Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

…erator Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

- move partition names to the OtOTPPartDesc array - allocate arrays of string properties and their matching byte values at runtime, - rename partition-related variables, as several new variables are added - replace statically defined constants with dynamic variables - clean up Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

…erface. This is the second step towards unification of OtOTP* implementations. It is better to expose the OtOTPState feature as an interface, so that OtOTPState can later implement the common feature for all OtOTP Top-specialized implementation. Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

…TP devices Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

… file Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

… Darjeeling - merge and adapt common code into OtOTPEngine - keep only OTP top-specific implementation in ot_otp_eg.c and ot_otp_dj.c, which implement the OtOTPImplIf interface (specialization for Tops) - regenerate OTP partition descriptions and key seed definition with otptool.py Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

- use Top defined constants - simplify stack of macro calls - remove definitions from ot_otp_engine.h Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

…ctory This is needed to implement other OTP top-specific implementations. Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

…rties - secret_scrambling_keys: the exact number of such properties is created. this avoid allocated useless memory, and ensure QEMU property subsystem may report any attempt to define values for keys that do not exist - secret_scrambling_key and inv_default_data values are now tied to the partition controller, rather than to be allocated as separate arrays Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

Should start with OT_OTP, OtOtp, ot_otp to prevent from unexpected conflicts Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

…d data Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

rivos-eblot mentioned this pull request Oct 30, 2025

ot_otp: prepare unification of OTP implementations #264

Closed

rivos-eblot force-pushed the dev/ebl/otp_unify branch from 32de450 to a5cad1d Compare October 30, 2025 18:24

rivos-eblot requested a review from loiclefort October 30, 2025 18:41

rivos-eblot force-pushed the dev/ebl/otp_unify branch 2 times, most recently from e6edb49 to 4eb19dc Compare October 31, 2025 09:06

rivos-eblot requested review from AlexJones0 and jwnrt October 31, 2025 09:11

loiclefort reviewed Oct 31, 2025

View reviewed changes

rivos-eblot force-pushed the dev/ebl/otp_unify branch 2 times, most recently from 8494dad to 56b1a5e Compare October 31, 2025 14:24

AlexJones0 reviewed Oct 31, 2025

View reviewed changes

rivos-eblot force-pushed the dev/ebl/otp_unify branch 2 times, most recently from b588e1b to 5e0c13a Compare October 31, 2025 17:11

rivos-eblot requested a review from loiclefort October 31, 2025 17:29

rivos-eblot added 12 commits October 31, 2025 18:31

[ot] python/qemu: ot.otp.descriptor: add name string to partition gen…

2a4e894

…erator Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

[ot] hw/opentitan: ot_otp_impl_if: define a new OtOTPImplIf API for O…

cb7b7a9

…TP devices Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

[ot] python/qemu: ot.otp: add key seed extraction and generation to C…

79c2f2f

… file Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

[ot] hw/opentitan: ot_otp: rework key management

b501c3c

- use Top defined constants - simplify stack of macro calls - remove definitions from ot_otp_engine.h Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

[ot] hw/opentitan: ot_otp: move ot_otp_engine.h to public header dire…

1f2af3c

…ctory This is needed to implement other OTP top-specific implementations. Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

[ot] hw/opentitan: ot_otp: make OTP definitions use an OT_OTP prefix

d7583e8

Should start with OT_OTP, OtOtp, ot_otp to prevent from unexpected conflicts Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

rivos-eblot force-pushed the dev/ebl/otp_unify branch from 5e0c13a to fbe0c39 Compare October 31, 2025 17:32

rivos-eblot marked this pull request as ready for review October 31, 2025 17:36

rivos-eblot requested a review from ziuziakowska October 31, 2025 17:36

rivos-eblot force-pushed the dev/ebl/otp_unify branch from fbe0c39 to 1aadd5c Compare October 31, 2025 18:08

[ot] hw/opentitan: ot_otp: retrieve HW config parameters from buffere…

d7b4ee4

…d data Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>

rivos-eblot force-pushed the dev/ebl/otp_unify branch from 1aadd5c to d7b4ee4 Compare October 31, 2025 18:09

	key_seeds: list[str, str, int, int] = []
	key_seeds: list[tuple[str, str, int, int]] = []

		@@ -0,0 +1,423 @@
		/*
		* QEMU OpenTitan EarlGrey One Time Programmable (OTP) memory controller

	* QEMU OpenTitan EarlGrey One Time Programmable (OTP) memory controller
	* QEMU OpenTitan One Time Programmable (OTP) memory controller

ot_otp: unify implementations #272

Are you sure you want to change the base?

ot_otp: unify implementations #272

Uh oh!

Conversation

rivos-eblot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexJones0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexJones0 Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rivos-eblot Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexJones0 Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

`ot_otp`: unify implementations #272

`ot_otp`: unify implementations #272

rivos-eblot commented Oct 30, 2025 •

edited

Loading

AlexJones0 Oct 31, 2025 •

edited

Loading

rivos-eblot Oct 31, 2025 •

edited

Loading

AlexJones0 Oct 31, 2025 •

edited

Loading