Add APOB messages to host_sp_comms #2006
Conversation
| hf.bonus_sector_erase(offset) | ||
| .map_err(|err| APOBError::EraseFailed { offset, err })?; | ||
| } else { | ||
| // Read back the page and confirm that it's all empty |
There was a problem hiding this comment.
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.)
| // APOB is followed by a binary data blob, which should be written to flash | ||
| APOB { |
There was a problem hiding this comment.
suuuper nitpicky, feel free to disregard me: I think the recommended Rust naming conventions for casing is that acronyms should be treated as a "word" in camel-case, and only the first letter should be uppercased, so this would be:
| // APOB is followed by a binary data blob, which should be written to flash | |
| APOB { | |
| // APOB is followed by a binary data blob, which should be written to flash | |
| Apob { |
There was a problem hiding this comment.
Note that other variants of this enum follow this convention (e.g. GetMacIdentity rather than GetMACIdentity; RotRequest rather than ROTRequest, and so on).
| @@ -134,6 +134,11 @@ enum Trace { | |||
| #[count(children)] | |||
| message: SpToHost, | |||
| }, | |||
| APOBWriteError { | |||
There was a problem hiding this comment.
similarly, i think this would be
| APOBWriteError { | |
| ApobWriteError { |
| @@ -160,6 +165,31 @@ enum Timers { | |||
| TxPeriodicZeroByte, | |||
| } | |||
|
|
|||
| #[derive(Copy, Clone, Debug, Eq, PartialEq, counters::Count)] | |||
| enum APOBError { | |||
There was a problem hiding this comment.
aaaaand my stupid capitalization thing again:
| enum APOBError { | |
| enum ApobError { |
| offset: u64, | ||
| #[count(children)] | ||
| err: APOBError, |
There was a problem hiding this comment.
It might be worth leaving a comment here indicating that the offset field in the ringbuf entry is the initial offset of the write, while the offset fields in the APOBError variant are the offset of the page we were writing when the error actually occurred? Having two fields with the same names but potentially differing values could be confusing.
Or, perhaps we should call the APOBError field "page_offset" or something, to distinguish these?
See https://github.com/oxidecomputer/stlouis/issues/707, https://github.com/oxidecomputer/rfd/pull/855 , https://github.com/oxidecomputer/amd-host-image-builder/pull/222