Add APOB messages to host_sp_comms #2006

mkeeter · 2025-02-07T15:45:23Z

Implements the state machine described in RFD 593

See https://github.com/oxidecomputer/stlouis/issues/707, https://github.com/oxidecomputer/rfd/pull/855 , oxidecomputer/amd-host-image-builder#222

jgallagher · 2025-02-07T16:47:24Z

task/host-sp-comms/src/main.rs

+            hf.bonus_sector_erase(offset)
+                .map_err(|err| APOBError::EraseFailed { offset, err })?;
+        } else {
+            // Read back the page and confirm that it's all empty


Could this fail if there's a blip in the IPCC path and the host resends an APOB request? (I'm not sure what the expectations are for the offsets the host is providing.)

This has changed a bunch since February; messages should now all be idempotent.

hawkw

Some mostly kind of annoying nitpicks.

lib/host-sp-messages/src/lib.rs

task/host-sp-comms/src/main.rs

rmustacc · 2025-08-29T20:10:35Z

task/host-sp-comms/src/main.rs

+    fn apob_write(
+        hf: &HostFlash,
+        mut offset: u64,
+        data: &[u8],
+    ) -> Result<(), ApobError> {


So we need something in the broader API to discover what the erase size granularity is or at least make sure that we're sending stuff that is page size aligned. This gets to what @jgallagher gets at below. But if the host sent things that wasn't page aligned then we'd erase the entire page because our API is not doing a read-modify-write.

This has all changed now – we ensure that the to-be-written page is erased when the state machine starts, and writes are idempotent.

labbott

Will probably do another pass later

drv/cosmo-hf/src/apob.rs

labbott · 2025-10-01T14:25:44Z

drv/cosmo-hf/src/apob.rs

+            checksum: 0, // dummy value
+        };
+        out.checksum = out.expected_checksum();
+        assert!(out.is_valid());


assert becomes panic, is that the behavior we want here?

I think so; the only way this should panic is if someone has broken the code in a dramatic way (e.g. editing the implementation of is_valid so that previously valid data is no longer valid).

But this is also copied from HfRawPersistentData, so I didn't think about it too much!

labbott · 2025-10-01T14:29:42Z

drv/cosmo-hf/src/apob.rs

+    /// Either 0 or 1; directly translatable to [`ApobSlot`]
+    pub slot_select: u32,


There are a couple of places where we have unreachable because of using the u32, maaaybe an enum would be cleaner and reduce a few checks? Or is the issue this needs to match exactly what the host is expecting?

This isn't seen at all by the host, but we're reading / writing this object directly to disk, so we need zerocopy-friendly types.

FWIW, zerocopy::TryFromBytes can be derived for enums (though I'm not sure how annoying that would be to actually use here)

I think it would be usable, but the docs seem to imply that you shouldn't it when round-tripping through bytes (?!).

I've opened google/zerocopy#2722 to ask for clarification

drv/cosmo-hf/src/hf.rs

labbott · 2025-10-01T14:37:22Z

drv/cosmo-hf/src/main.rs

 fn fail(err: drv_hf_api::HfError) {
    let mut buffer = [0; hf::idl::INCOMING_SIZE];
-    let mut server = hf::idl::FailServer::new(err);
+    let mut server = hf::FailServer(err);


Hmmm, do we lose the idl generation of this?

Yeah – I switched to finer-grained error types for a few methods, which means that the IDL generator can't make a FailServer (which assumes a single error type).

labbott · 2025-10-01T14:38:48Z

drv/cosmo-hf/src/apob.rs

+    pub(crate) fn write(
+        &mut self,
+        drv: &mut FlashDriver,
+        offset: u64,


Do we need the u64 everywhere? It seems like everything gets converted/checked against u32 anyway

The initial u64 is sent by the host, but I pushed the u32 conversion upstream into host_sp_comms.

citrus-it · 2025-10-02T15:12:55Z

@mkeeter - here are the IPCC messages which are expected to be seen before the host is finished with APOB. Anything else incoming from the host should trigger the lockdown.

humility: ring buffer task_host_sp_comms::__RINGBUF in host_sp_comms:
   TOTAL VARIANT
      99 Request(ApobRead)
      96 Request(ApobData)
       2 Request(KeyLookup)
       1 Request(GetBootStorageUnit)
       1 Request(GetIdentity)
       1 Request(GetStatus)
       1 Request(AckSpStart)
       1 Request(ApobBegin)
       1 Request(ApobCommit)

mkeeter · 2025-10-02T20:37:20Z

@citrus-it Great, thanks! I've pushed this list to Hubris and to RFD 593.

hawkw · 2025-10-08T20:04:24Z

app/gimlet/base.toml

 interrupts = {"uart7.irq" = "usart-irq"}
 priority = 8
-max-sizes = {flash = 65952, ram = 65536}
+max-sizes = {flash = 67648, ram = 65536}


Huh, feels a little unfortunate that this change impacts Gimlet at all, given that (IIUC) the APOB stuff is ~SP5-specific. Is this just due to the addition of new IPCC messages?

hawkw · 2025-10-08T20:05:48Z

drv/cosmo-hf/src/apob.rs

+        actual: u32,
+    },
+}
+ringbuf!(Trace, 16, Trace::None);


16 entries is a pretty small ringbuf, might we want to also generate counters for the various operations like slot erase/sector erase/persistent data write, and/or the total number of errors?

hawkw · 2025-10-08T20:07:10Z

drv/cosmo-hf/src/apob.rs

+}
+
+#[derive(Copy, Clone, PartialEq)]
+pub(crate) enum ApobState {


nitpicky, feel free to ignore: it might be nice to have a doc comment on this referencing the state diagram in https://rfd.shared.oxide.computer/rfd/593?expand=1#_production_strength_implementation ?

hawkw · 2025-10-08T22:56:18Z

drv/cosmo-hf/src/apob.rs

+        // We do a CRC32 of everything except the checksum, which is positioned
+        // at the end of the struct and is a `u32`
+        let size = core::mem::size_of::<ApobRawPersistentData>()
+            - core::mem::size_of::<u32>();
+        c.update(&self.as_bytes()[..size]);


turbo nit, feel free to ignore: it occurs to me that this could also be achieved by having two #[repr(C)] #[derive(IntoBytes)] structs and nesting the "used to calculate CRC32" parts inside the outer struct, like:

#[derive( Copy, Clone, Eq, PartialEq, IntoBytes, FromBytes, Immutable, KnownLayout, )] #[repr(C)] pub struct ApobRawPersistentData { pub inner: ApobRawPersistentDataInner, checksum: u32, } #[derive( Copy, Clone, Eq, PartialEq, IntoBytes, FromBytes, Immutable, KnownLayout, )] #[repr(C)] pub struct ApobRawPersistentDataInner { oxide_magic: u32, header_version: u32, pub monotonic_counter: u64, pub slot_select: u32, } impl ApobRawPersistentData { // ... fn expected_checksum(&self) -> u32 { static CRC: crc::Crc<u32> = crc::Crc::<u32>::new(&crc::CRC_32_ISCSI); let mut c = CRC.digest(); // We do a CRC32 of everything except the checksum, which is positioned // at the end of the struct and is a `u32` c.update(&self.inner.as_bytes()); c.finalize() } // ... }

is this worth it? well...perhaps not!

hawkw · 2025-10-08T23:00:40Z

drv/cosmo-hf/src/apob.rs

+    /// Searches for an active slot in the metadata regions, updating the offset
+    /// in the FPGA driver if found, and erases unused or invalid slots.


where does the erasure happen in this function? i don't think that happens in set_apob_offset, right?

Update: OH it's get_slot that does that. this naming was shocking and disturbing to me, i would not expect get_thing to also delete all the other things.

hawkw · 2025-10-08T23:42:55Z

drv/cosmo-hf/src/hf.rs

    pub dev: HfDevSelect,
    hash: HashData,
+
+    // Used in the `apob` module for implementation


for implementation?

hawkw · 2025-10-08T23:43:30Z

drv/hf-api/src/lib.rs

+)]
+pub enum ApobBeginError {
+    /// APOB is not implemented on this hardware
+    NotImplemented = 1,


ImAGimlet :)

hawkw · 2025-10-08T23:44:07Z

drv/hf-api/src/lib.rs

+    IdolError,
+    counters::Count,
+)]
+pub enum ApobCommitError {


thank you for having separate error types for each operation <3

hawkw · 2025-10-08T23:44:28Z

drv/spartan7-loader/cosmo-seq/README.md

-FPGA images and collateral are generated from:
-[this sha](https://github.com/oxidecomputer/quartz/commit/bdc5fb31e1905a1b66c19647fe2d156dd1b97b7b)
-[release](https://api.github.com/repos/oxidecomputer/quartz/releases/242278756)
+FPGA images and collateral are generated from some hw guy's local build


i assume this is going to be changed before merging this? :)

hawkw · 2025-10-08T23:47:46Z

task/host-sp-comms/src/main.rs

+                if let Ok(d) = data.try_into() {
+                    let hash = ApobHash::Sha256(d);
+                    match hf.apob_begin(length, hash) {
+                        Ok(()) => ApobBeginResult::Ok,
+                        Err(ApobBeginError::NotImplemented) => {
+                            ApobBeginResult::NotImplemented
+                        }
+                        Err(ApobBeginError::InvalidState) => {
+                            ApobBeginResult::InvalidState
+                        }
+                        Err(ApobBeginError::BadDataLength) => {
+                            ApobBeginResult::BadDataLength
+                        }
+                    }
+                } else {
+                    ApobBeginResult::BadHashLength
+                }


take it or leave it, might be stupid, but we could make this less rightward-drift-y

Suggested change

if let Ok(d) = data.try_into() {

let hash = ApobHash::Sha256(d);

match hf.apob_begin(length, hash) {

Ok(()) => ApobBeginResult::Ok,

Err(ApobBeginError::NotImplemented) => {

ApobBeginResult::NotImplemented

}

Err(ApobBeginError::InvalidState) => {

ApobBeginResult::InvalidState

}

Err(ApobBeginError::BadDataLength) => {

ApobBeginResult::BadDataLength

}

}

} else {

ApobBeginResult::BadHashLength

}

let Ok(d) = data.try_into() else {

return ApobBeginResult::BadHashLength;

};

let hash = ApobHash::Sha256(d);

match hf.apob_begin(length, hash) {

Ok(()) => ApobBeginResult::Ok,

Err(ApobBeginError::NotImplemented) => {

ApobBeginResult::NotImplemented

}

Err(ApobBeginError::InvalidState) => {

ApobBeginResult::InvalidState

}

Err(ApobBeginError::BadDataLength) => {

ApobBeginResult::BadDataLength

}

}

mkeeter requested review from jgallagher, hawkw and citrus-it February 7, 2025 15:45

jgallagher reviewed Feb 7, 2025

View reviewed changes

mkeeter marked this pull request as draft February 7, 2025 17:08

hawkw reviewed Feb 7, 2025

View reviewed changes

lib/host-sp-messages/src/lib.rs Outdated Show resolved Hide resolved

task/host-sp-comms/src/main.rs Outdated Show resolved Hide resolved

task/host-sp-comms/src/main.rs Outdated Show resolved Hide resolved

task/host-sp-comms/src/main.rs Outdated Show resolved Hide resolved

Aaron-Hartwig force-pushed the mkeeter/ipcc-apob branch from 3c34974 to 593980a Compare February 24, 2025 18:19

mkeeter force-pushed the mkeeter/ipcc-apob branch from 26109b2 to 65a65da Compare May 5, 2025 18:55

citrus-it force-pushed the mkeeter/ipcc-apob branch from 65a65da to ebcfe39 Compare July 1, 2025 14:48

citrus-it force-pushed the mkeeter/ipcc-apob branch from ded20cd to 7846e44 Compare August 28, 2025 12:14

rmustacc reviewed Aug 29, 2025

View reviewed changes

mkeeter force-pushed the mkeeter/ipcc-apob branch 3 times, most recently from 14165d3 to b0acdc3 Compare September 26, 2025 15:37

mkeeter force-pushed the mkeeter/ipcc-apob branch from f7e8314 to 4a07f2b Compare October 1, 2025 14:00

mkeeter marked this pull request as ready for review October 1, 2025 14:13

mkeeter force-pushed the mkeeter/ipcc-apob branch from ded1fa1 to ad93477 Compare October 1, 2025 14:27

labbott reviewed Oct 1, 2025

View reviewed changes

mkeeter force-pushed the mkeeter/ipcc-apob branch 2 times, most recently from 49afe3d to 695c7d5 Compare October 2, 2025 14:30

mkeeter force-pushed the mkeeter/ipcc-apob branch from efc7039 to 8d35455 Compare October 2, 2025 20:35

mkeeter force-pushed the mkeeter/ipcc-apob branch from 8d35455 to 58e761d Compare October 2, 2025 20:40

mkeeter added 6 commits October 3, 2025 10:44

Add APOB message to host_sp_comms

350bfd6

s/APOB/Apob

065b31e

Thanks, clippy

698dae6

s/offset/page_offset

6ee0313

Add ApobRead

d1eeebd

Clippy fixes

d4e28d3

mkeeter and others added 27 commits October 3, 2025 10:44

Working on new APOB messages

dcfeeb1

Get gimlet_hf working (requires custom FailServer, sadly)

205ae09

Get cosmo-hf building

9d75162

Implement apob_write

7fbff49

Implement apob_read

c5fac41

Move functionality into apob_state

a2c253b

Remove dead code

a932483

Remove redundant apob_state.apob_* names

6bafe94

Implement apob_commit

f35f98b

Post-rebase fixes

2978941

Reinitialize the APOB when muxing to the host

2fe4f2c

Reset the APOB offset under all circumstances

0506062

Fix apob_read call

4cdca32

update grapefruit with the ApobFlashOffset register

d1810bd

Get stuff working with faux-ipcc

3c66b99

Clippy fix

d191501

Changes to match new HSS messages

ec74ca7

Check APOB shape

4a7914c

Erase data if there's a validation error

8a421aa

Small APOB tweaks

d35e4ce

Fix read offset

cac99c2

Fix build

003c154

Push u64 -> u32 conversion upstream

e638119

More memory fixes

a0994ff

clippyyyyyy

2f91e98

Lock APOB state machine on unapproved messages

31e6828

Only erase written region on validation failure

f883776

mkeeter force-pushed the mkeeter/ipcc-apob branch from 58e761d to f883776 Compare October 3, 2025 15:15

labbott approved these changes Oct 8, 2025

View reviewed changes

hawkw reviewed Oct 9, 2025

View reviewed changes

		/// Either 0 or 1; directly translatable to [`ApobSlot`]
		pub slot_select: u32,

		/// Searches for an active slot in the metadata regions, updating the offset
		/// in the FPGA driver if found, and erases unused or invalid slots.

Add APOB messages to host_sp_comms #2006

Are you sure you want to change the base?

Add APOB messages to host_sp_comms #2006

Uh oh!

Conversation

mkeeter commented Feb 7, 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!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labbott left a comment

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!

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!

citrus-it commented Oct 2, 2025

Uh oh!

mkeeter commented Oct 2, 2025

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!

Uh oh!

mkeeter commented Feb 7, 2025 •

edited

Loading