Switch to software hashing for async host flash hashing (and some cleanup) by labbott · Pull Request #2682 · oxidecomputer/hubris

labbott · 2026-09-11T15:17:36Z

No description provided.

jamesmunns · 2026-09-11T16:47:17Z

                    self.hash.state = HashState::Done;
-                    match self.hash.task.finalize_sha256() {
-                        Ok(v) => match dev {
+                    let mut out: [u8; 32] = [0; 32];


Is this drv_hash_api::SHA256_SZ?

Yes. I was initially trying to see if I could move away from that since we don't have the separate task anymore but the constant is still handy. I'll fix it up.

jamesmunns · 2026-09-11T16:48:12Z

-            .task
-            .finalize_sha256()
-            .map_err(|_| HfError::HashError.into())
+        if self.hash.task.finalize_sha256(&mut out).is_err() {


Do we want to log this error somehow? Right now we just discard it.

This error is not actually likely to be useful in practice. The only way we would get an error is if we forgot to call init or called finalize twice in a row.

jamesmunns · 2026-09-11T16:50:41Z

+            }
+        };
+
+        if self.nvalid > 0 {


Any worries that we could end up changing the state, then these checks cause us to remain in that state even though our current state or the input was invalid?

I'm not following this comment. Can you clarify? You might be asking about interactions between update and update_exact and how they might interact?

jamesmunns · 2026-09-11T16:57:02Z

+                // away is too costly so just busy wait.
+            }
+            for w in block.chunks(SIZEOF_U32) {
+                let data = u32::from_le_bytes(w.try_into().unwrap());


Would it be worth forcing the alignment of this buffer so we don't have to stack-copy through? Not sure if it matters much if we end up spinning on self.is_busy() anyway.

It could be nicer! I've been debating the value of accepting arbitrary [u8] vs requiring [u32] everywhere in this driver. We would be limiting what we could accept and wouldn't work on some NIST test vectors. But the entire point of this PR is that it turns out we don't need a generic hashing mechanism in hubris: software hashing is Good Enough(tm) for most cases except for the 32 MB of QSPI.

labbott · 2026-09-11T18:28:23Z

-        while offset + SIZEOF_U32 <= data.len() {
-            self.write_word(
-                u32::from_le_bytes(
-                    (&data[offset..offset + SIZEOF_U32]).try_into().unwrap(),
-                ),
-                SIZEOF_U32,
-            );
-            offset += SIZEOF_U32;
-        }


For the record this was some dead code I saw floating around (increase the context to see this can never be called)

hawkw · 2026-09-14T17:16:49Z

+        if !data.len().is_multiple_of(BLOCK_LEN_BYTES) {
+            return Err(HashError::InvalidState);
+        }


this feels more like an invalid input than an invalid state; i wonder if a different error would be more appropriate here?

I was very lazy about adding errors and you are correct

hawkw · 2026-09-14T17:17:48Z

+                // If the block is busy a write to `DATAIN` will stall the
+                // AHB bus. Past experience has shown that context switching
+                // away is too costly so just busy wait.


nit, sorry --- just passing through with the comma shaker:

Suggested change

// If the block is busy a write to `DATAIN` will stall the

// AHB bus. Past experience has shown that context switching

// away is too costly so just busy wait.

// If the block is busy, a write to `DATAIN` will stall the

// AHB bus. Past experience has shown that context switching

// away is too costly, so just busy wait.

The hash of the host flash was originally designed to be calculated asynchronously because it was too slow to do in one shot. We used the existing hardware hash block infrastructure because it was there. We now have a use case for wanting to actually hash the entire host flash at once (measured boot). Rather than trying to be concerned about the interaction between async hashing and measurement just make the async hashing software based. It also turns out having the hardware hashing in a separate task was pretty slow. Sample numbers from just moving the hardware hashing into the same task Before: laura@cadbury ~ $ time pfexec humility -t c71 hiffy -c HostFlash.hash -aaddress=0,len=33554432 --timeout 1000000 humility: attached to 0483:3754:001100184741500820383733 via ST-Link V3 HostFlash.hash() => [ 0x9c, 0x41, 0xaf, 0x46, 0x10, 0xb8, 0x55, 0x1f, 0x28, 0x5a, 0xf6, 0xbe, 0xd7, 0x84, 0x2c, 0xab, 0xc6, 0xa4, 0x2a, 0xf4, 0x90, 0x93, 0x88, 0x8a, 0xc9, 0x61, 0x39, 0xf3, 0xde, 0xee, 0x86, 0x1d ] real 0m10.115s user 0m3.457s sys 0m0.663s After: laura@cadbury ~ $ time pfexec humility -t c71 -a ~/build-gimlet-c-lab-image-default.zip hiffy -c HostFlash.hash -aaddress=0,len=33554432 --timeout 1000000 humility: WARNING: archive on command-line overriding archive in environment file humility: attached to 0483:3754:001100184741500820383733 via ST-Link V3 HostFlash.hash() => [ 0x9c, 0x41, 0xaf, 0x46, 0x10, 0xb8, 0x55, 0x1f, 0x28, 0x5a, 0xf6, 0xbe, 0xd7, 0x84, 0x2c, 0xab, 0xc6, 0xa4, 0x2a, 0xf4, 0x90, 0x93, 0x88, 0x8a, 0xc9, 0x61, 0x39, 0xf3, 0xde, 0xee, 0x86, 0x1d ] real 0m7.355s user 0m3.319s sys 0m0.560s This change also removes the hiffy interfaces for hashing. While this is technically a breaking change we've never really made use of the hash interface except for testing and profiling.

labbott · 2026-09-16T19:23:52Z

+uses = ["quadspi", "hash"]
+interrupts = {"quadspi.irq" = "qspi-irq", "hash.irq" = "hash-irq"}
+task-slots = ["sys"]
+notifications = ["qspi-irq", "timer", "hash-irq"]


I guess I could move some of this out of here but it gets used in the follow on work

labbott · 2026-09-16T19:25:13Z

+#[derive(Debug, Clone, Copy, PartialEq)]
+enum Trace {
+    None,
+
+    AsyncTime(u64),
+}
+
+ringbuf::ringbuf!(Trace, 8, Trace::None);


I threw this in because it was nice for profiling/quick checks. I can also remove it (but might be nice to keep in when measuring)

fine with me if you think this will be useful in the future! i suppose we could probably scrape off a few bytes if we got rid of the enum here and zero-initialized it, like:

Suggested change

#[derive(Debug, Clone, Copy, PartialEq)]

enum Trace {

None,

AsyncTime(u64),

}

ringbuf::ringbuf!(Trace, 8, Trace::None);

ringbuf::ringbuf!(u64, 8, 0);

but that might be more confusing to a future reader (who would just see all zeros when we haven't logged stuff), and perhaps we will want to record things other than timings eventually? so maybe leave it as is...

labbott requested review from hawkw and jamesmunns September 11, 2026 15:18

jamesmunns reviewed Sep 11, 2026

View reviewed changes

labbott commented Sep 11, 2026

View reviewed changes

hawkw reviewed Sep 14, 2026

View reviewed changes

labbott force-pushed the labbott/more_hash_improve branch from ce4963b to 19c8e07 Compare September 16, 2026 15:22

labbott changed the title ~~Hash performance improvements~~ Switch to software hashing for async host flash hashing (and some cleanup) Sep 16, 2026

labbott force-pushed the labbott/more_hash_improve branch from 19c8e07 to ee562f5 Compare September 16, 2026 15:27

labbott added 4 commits September 16, 2026 13:34

missed

95e79bb

okay clippy

d21c598

more clippy

a108801

guess what

ccc1ca6

labbott commented Sep 16, 2026

View reviewed changes

Conversation

labbott commented Sep 11, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants