[runtime] Start recovery flow in the runtime #1904

swenson · 2025-01-23T00:49:27Z

This stubs out the recovery flow in the runtime.

We also update the DMA peripheral so that it can be used as a
ureg::Mmio, which will allow us to use the autogenerated I3C recovery
register interface instead of hardcoding offsets.

We also had to modify ureg slightly to remove the unused impl Uint
for u64 (because we don't don't support 64-bit AXI transfers) and add
a from_u32() method for the (unused) u8 and u16 implementations.

This builds on #1867 and rewrites it to use the updated DMA interface.

swenson · 2025-01-23T00:52:25Z

drivers/src/dma.rs

@@ -62,7 +106,7 @@ pub struct DmaWriteTransaction {

 /// Dma Widget
 pub struct Dma {
-    dma: AxiDmaReg,
+    dma: Cell<Option<AxiDmaReg>>,


We have to switch to a Cell because the Mmio and MmioMut interfaces require that the methods use &self instead of &mut self, so we need interior mutability.

For safety, I used a with_dma() to ensure that the AxiDmaReg is only used by one function at a time, though this is probably overkill since Caliptra is single-threaded.

Suggest simplifying the code (if possible) with the single-threaded execution assumption.

This stubs out the recovery flow in the runtime. We also update the DMA peripheral so that it can be used as a `ureg::Mmio`, which will allow us to use the autogenerated I3C recovery register interface instead of hardcoding offsets. We also had to modify `ureg` slightly to remove the unused `impl Uint` for `u64` (because we don't don't support 64-bit AXI transfers) and add a `from_u32()` method for the (unused) `u8` and `u16` implementations. This builds on #1867 and rewrites it to use the updated DMA interface.

This reverts commit 9d899b1.

This reverts commit cab7e49.

mhatrevi · 2025-01-28T00:55:33Z

drivers/src/dma.rs

+            self.dma.set(Some(dma));
+            Ok(result)
+        } else {
+            Err(CaliptraError::RUNTIME_INTERNAL) // should never happen


Suggest renaming this to something like 'DRIVERS_INTERNAL' to disambiguate from a Runtime app error.

mhatrevi · 2025-01-28T01:16:50Z

runtime/src/recovery_flow.rs

+            .auth_manifest_image_metadata_col = image_metadata_collection;
+        // [TODO][CAP2]: capture measurement of Soc manifest?
+        // [TODO][CAP2]: this should be writing to MCU SRAM directly via AXI
+        let _mcu_size_bytes = dma_recovery.download_image_to_mbox(SOC_MANIFEST_INDEX)?;


This should use the MCU_FIRMWARE_INDEX.

mhatrevi

Please see the feedback.

jhand2 · 2025-01-28T19:23:19Z

runtime/src/drivers.rs

@@ -171,6 +176,9 @@ impl Drivers {
            ResetReason::ColdReset => {
                cfi_assert_eq(self.soc_ifc.reset_reason(), ResetReason::ColdReset);
                Self::initialize_dpe(self)?;
+                if self.soc_ifc.active_mode() {


I wonder if this should be a build-time flag since for a given integration it's either active or not. Otoh, it's probably easier to have a less complicated test matrix for RT.

Would we plan on building a separate active mode ROM then?

jhand2 · 2025-01-28T19:27:01Z

runtime/src/recovery_flow.rs

+        let _soc_size_bytes = dma_recovery.download_image_to_mbox(SOC_MANIFEST_INDEX)?;
+        let Some(preamble) = AuthManifestPreamble::read_from_prefix(drivers.mbox.raw_mailbox_contents()) else {
+            return Err(CaliptraError::IMAGE_VERIFIER_ERR_MANIFEST_SIZE_MISMATCH);
+        };
+        let Some(image_metadata_collection) = AuthManifestImageMetadataCollection::read_from_prefix(&drivers.mbox.raw_mailbox_contents()[AUTH_MANIFEST_PREAMBLE_SIZE..]) else {
+            return Err(CaliptraError::IMAGE_VERIFIER_ERR_MANIFEST_SIZE_MISMATCH);
+        };


nit: can probably simplify this by making a single stuct that contains AuthManifestPreamble and AuthManifestImageMetadataCollection and then doing a single read.

jhand2 · 2025-01-28T19:27:58Z

ureg/src/lib.rs

 }
 impl private::Sealed for u32 {}

-impl Uint for u64 {


Why is u64 removed here?

We never use 64-bit registers, and I don't know if we have the semantics for it super well-defined given our 32-bit memory bus. (Things like, which word is written first.) So, it seemed easier to just to remove 64-bit support altogether.

(Side note, I'd also be a fan of removing 8- and 16-bit register support as well, since it also is a little murky semantically, and refactoring out the Uint type altogether to just be u32.)

jhand2 · 2025-01-28T19:29:18Z

ureg/src/lib.rs

+    fn from_u32(val: u32) -> Self {
+        (val & 0xff) as u8
+    }


Is this needed for multiple types? I think I would prefer to the the truncation closer to the specific register that needs it so that we can review specific usages as not dropping any meaningful bytes.

It's not truly needed except on paper, as we don't have any 8- or 16-bit registers.

mhatrevi · 2025-01-29T01:30:13Z

runtime/src/recovery_flow.rs

+
+        // // download SoC manifest
+        let _soc_size_bytes = dma_recovery.download_image_to_mbox(SOC_MANIFEST_INDEX)?;
+        let Some(manifest) = AuthorizationManifest::read_from_prefix(drivers.mbox.raw_mailbox_contents()) else {


This is not efficient. AuthorizationManifest is of 14284 bytes; most of the time there will only be a handful of image digests in it. I would simply add a TODO here to call the existing set_manifest_code.

swenson added the Caliptra v2.0 Items to be considered for v2.0 Release label Jan 23, 2025

swenson requested review from jhand2, fdamato, rusty1968, bluegate010, mhatrevi, vsonims, ajisaxena, korran and JohnTraverAmd as code owners January 23, 2025 00:49

swenson commented Jan 23, 2025

View reviewed changes

swenson force-pushed the download-manifest-mcu-fw branch from 173817b to 9152e52 Compare January 23, 2025 21:53

swenson force-pushed the download-manifest-mcu-fw branch from 9152e52 to c9b226d Compare January 23, 2025 22:00

Fix hardware model offset (how did this get here?)

cab7e49

swenson requested a review from jlmahowa-amd as a code owner January 23, 2025 22:57

Fix the rest of the EXTERNAL_PERIPH_BASE offsets

9d899b1

swenson requested review from wmaroneAMD and nquarton as code owners January 24, 2025 00:01

swenson added 3 commits January 23, 2025 16:04

Revert "Fix the rest of the EXTERNAL_PERIPH_BASE offsets"

14e3fd8

This reverts commit 9d899b1.

Revert "Fix hardware model offset (how did this get here?)"

86a90e2

This reverts commit cab7e49.

Revert rtl change

af5b552

mhatrevi reviewed Jan 28, 2025

View reviewed changes

mhatrevi previously approved these changes Jan 28, 2025

View reviewed changes

Address feedback

e54babc

swenson dismissed mhatrevi’s stale review via e54babc January 28, 2025 17:52

Merge branch 'main-2.x' into download-manifest-mcu-fw

400e1d0

jhand2 reviewed Jan 28, 2025

View reviewed changes

Read the whole manifest in at once

df30ff0

mhatrevi reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[runtime] Start recovery flow in the runtime #1904

[runtime] Start recovery flow in the runtime #1904

swenson commented Jan 23, 2025

swenson Jan 23, 2025

mhatrevi Jan 28, 2025

mhatrevi Jan 28, 2025

mhatrevi Jan 28, 2025

mhatrevi left a comment

jhand2 Jan 28, 2025

swenson Jan 28, 2025

jhand2 Jan 28, 2025

jhand2 Jan 28, 2025

swenson Jan 28, 2025

jhand2 Jan 28, 2025

swenson Jan 28, 2025

mhatrevi Jan 29, 2025

[runtime] Start recovery flow in the runtime #1904

Are you sure you want to change the base?

[runtime] Start recovery flow in the runtime #1904

Conversation

swenson commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhatrevi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment