From e17f7321a1a031fafbf8e9db1501e0c13ce377f8 Mon Sep 17 00:00:00 2001 From: Dylan Knutson Date: Fri, 24 Apr 2026 14:11:46 -0700 Subject: [PATCH 1/5] build(toolchain): pin workspace MSRV to Rust 1.92.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add explicit `channel = "1.92.0"` to the workspace rust-toolchain.toml and `rust-version = "1.92"` to [workspace.package]. Wire each member crate's [package] to opt in via `rust-version.workspace = true` (Cargo's [workspace.package] does NOT auto-inherit; explicit per-member opt-in is required, mirroring `version.workspace = true` / `edition.workspace = true`). Members opted in: dev-hooks, ec-service-lib, espi-device, espi-device-stub, platform/ihv1-sp, platform/qemu-sp, aarch64-haf. odp-ffa is left untouched — it pre-pins `rust-version = "1.75"` directly; that pre-existing decision is preserved and noted in the phase SUMMARY. Devcontainer + CI already run 1.92.0; this locks that floor declaratively against silent regression below mctp-rs's edition-2024 1.85 requirement. Per planning Phase 13 D-1, D-2, D-3. Closes/absorbs backlog 999.1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Cargo.toml | 1 + aarch64-haf/Cargo.toml | 1 + dev-hooks/Cargo.toml | 1 + ec-service-lib/Cargo.toml | 1 + espi-device-stub/Cargo.toml | 1 + espi-device/Cargo.toml | 1 + platform/ihv1-sp/Cargo.toml | 1 + platform/qemu-sp/Cargo.toml | 1 + rust-toolchain.toml | 1 + 9 files changed, 9 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 167af94..3d7cf4d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,6 +14,7 @@ members = [ [workspace.package] version = "0.1.0" edition = "2021" +rust-version = "1.92" [workspace.dependencies] aarch64-cpu = "10.0.0" diff --git a/aarch64-haf/Cargo.toml b/aarch64-haf/Cargo.toml index 17f51b8..7ea7fd5 100644 --- a/aarch64-haf/Cargo.toml +++ b/aarch64-haf/Cargo.toml @@ -2,6 +2,7 @@ name = "aarch64-haf" version = "0.1.0" edition = "2024" +rust-version.workspace = true license = "MIT" authors = ["Dylan Knutson "] description = "AArch64 HAL for Hafnium" diff --git a/dev-hooks/Cargo.toml b/dev-hooks/Cargo.toml index b531e5d..6b39772 100644 --- a/dev-hooks/Cargo.toml +++ b/dev-hooks/Cargo.toml @@ -2,6 +2,7 @@ name = "dev-hooks" version = "0.1.0" edition = "2021" +rust-version.workspace = true publish = false description = "Git hooks setup (husky-rs); not published" license = "MIT" diff --git a/ec-service-lib/Cargo.toml b/ec-service-lib/Cargo.toml index fcb60fa..724fd41 100644 --- a/ec-service-lib/Cargo.toml +++ b/ec-service-lib/Cargo.toml @@ -3,6 +3,7 @@ name = "ec-service-lib" categories = ["embedded", "no-std"] version = "0.1.0" edition = "2021" +rust-version.workspace = true license = "MIT" description = "Provides standard EC services to OS" readme = "README.md" diff --git a/espi-device-stub/Cargo.toml b/espi-device-stub/Cargo.toml index 5c23675..1bcd757 100644 --- a/espi-device-stub/Cargo.toml +++ b/espi-device-stub/Cargo.toml @@ -2,6 +2,7 @@ name = "espi-device-stub" version = "0.1.0" edition = "2021" +rust-version.workspace = true authors = ["Dylan Knutson "] license = "MIT" description = "Stub implementation of the eSPI driver" diff --git a/espi-device/Cargo.toml b/espi-device/Cargo.toml index 899c0e8..3f0bf2e 100644 --- a/espi-device/Cargo.toml +++ b/espi-device/Cargo.toml @@ -2,6 +2,7 @@ name = "espi-device" version = "0.1.0" edition = "2021" +rust-version.workspace = true description = "eSPI device driver interface" license = "MIT" authors = ["Dylan Knutson "] diff --git a/platform/qemu-sp/Cargo.toml b/platform/qemu-sp/Cargo.toml index 48735cd..7c17034 100644 --- a/platform/qemu-sp/Cargo.toml +++ b/platform/qemu-sp/Cargo.toml @@ -2,6 +2,7 @@ name = "qemu-ec-sp" version = "0.1.0" edition = "2021" +rust-version.workspace = true license = "MIT OR Apache-2.0" description = "QEMU Embedded Controller Secure Partition Service" authors = ["Phil Weber "] diff --git a/rust-toolchain.toml b/rust-toolchain.toml index e097ffd..3a8621a 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,3 +1,4 @@ [toolchain] +channel = "1.92.0" components = ["rust-src", "llvm-tools-preview", "rustfmt", "rust-analyzer"] targets = ["aarch64-unknown-none-softfloat"] From 2814ce84c9ea108c3c6334298c672ae6497987df Mon Sep 17 00:00:00 2001 From: Dylan Knutson Date: Fri, 24 Apr 2026 16:44:47 -0700 Subject: [PATCH 2/5] =?UTF-8?q?build(deps):=20add=20mctp-rs=20to=20workspa?= =?UTF-8?q?ce=20+=20bump=20num=5Fenum=200.7.3=20=E2=86=92=200.7.6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds mctp-rs at the EC-matching git pin (rev 3d941ba5205ca7781bf37e3dc7c5dfdc99a082d6) to [workspace.dependencies] for SP↔EC MCTP framing interop. Pin is byte-for-byte identical to EC's mod/ec/platform/dev-qemu/Cargo.lock (verified by Phase 12 spike, see .planning/phases/12-.../12-VERDICT.md section (a)). Bumps num_enum floor from 0.7.3 to 0.7.4 in workspace deps; resolver locks 0.7.6 (latest matching ^0.7.4) — same minor mctp-rs requires transitively. Direct workspace consumers via num_enum.workspace = true (ec-service-lib, espi-device, odp-ffa) all rebuild cleanly for aarch64-unknown-none-softfloat. Devcontainer `make all` end-to-end green. Note: mctp-rs is declared in [workspace.dependencies] but does not yet appear in Cargo.lock — cargo only adds workspace deps to the lockfile once a member consumes them. The lockfile entry materializes in the next commit (planning Plan 15-02) when sp-mctp-framer declares mctp-rs.workspace = true. Per planning Phase 15 D-1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 9 +++++---- Cargo.toml | 3 ++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 00f478d..c9509e2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -353,18 +353,19 @@ dependencies = [ [[package]] name = "num_enum" -version = "0.7.3" +version = "0.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4e613fc340b2220f734a8595782c551f1250e969d87d3be1ae0579e8d4065179" +checksum = "5d0bca838442ec211fa11de3a8b0e0e8f3a4522575b5c4c06ed722e005036f26" dependencies = [ "num_enum_derive", + "rustversion", ] [[package]] name = "num_enum_derive" -version = "0.7.3" +version = "0.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "af1844ef2428cc3e1cb900be36181049ef3d3193c63e43026cfe202983b27a56" +checksum = "680998035259dcfcafe653688bf2aa6d3e2dc05e98be6ab46afb089dc84f1df8" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index 3d7cf4d..7805bb9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,8 +34,9 @@ espi-device-stub = { path = "espi-device-stub" } odp-ffa = { path = "odp-ffa" } hafnium = { path = "hafnium" } log = { version = "0.4", default-features = false } +mctp-rs = { git = "https://github.com/dymk/mctp-rs", rev = "3d941ba5205ca7781bf37e3dc7c5dfdc99a082d6", default-features = false } mockall = "0.13.1" -num_enum = { version = "0.7.3", default-features = false } +num_enum = { version = "0.7.4", default-features = false } num-traits = { version = "0.2.19", default-features = false } subenum = { version = "1.1.2", default-features = false } uuid = { version = "1.0", default-features = false, features = ["v1"] } From ed58aef02c997a54bb074a9aa03be4782048b446 Mon Sep 17 00:00:00 2001 From: Dylan Knutson Date: Fri, 24 Apr 2026 16:56:21 -0700 Subject: [PATCH 3/5] feat(sp-mctp-framer): new crate with SmbusEspi+MCTP encoder/decoder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds sp-mctp-framer as a workspace member, consuming the mctp-rs workspace dep added in the previous commit. The crate is no_std with no heap on the runtime path (Box/Vec/String absent from src/lib.rs); only test code uses pretty_assertions for diff readability. Public API: - encode_battery_request(buf, battery_id) -> Result - decode_battery_response(buf) -> Result Validated by host-side cargo test golden-bytes round-trip against the Phase 12 spike captures (TX 18B + RX 13B). Encoder produces the captured 18-byte sequence byte-for-byte; decoder parses the captured 13-byte response into structured fields. See .planning/phases/12-spike-mctp-uart-ping/12-VERDICT.md §(c) for provenance. Source-confirmed constants (the planning artifact had three swap- direction errors; all caught and corrected against upstream source): 1. SMBUS slave addresses: dst=1, src=0 (matching uart-service:75-76). mctp-rs SmbusEspiMedium::serialize swaps the reply-context fields, so reply_ctx.{dst,src}=(1,0) lands as (0,1) on wire — combined with the wire byte-order convention this produces the captured `00 0f 0e 03` SmbusEspi header (where 0x0f is the MCTP command code, NOT a slave address as the plan mistakenly assumed). 2. MCTP endpoint IDs: source=0x08(BATTERY), destination=0x80(SP). mctp-rs serialize.rs:62-63 swaps these too — to put dst_eid=0x08 on wire we set source=0x08. 3. ODP relay-header bit-extraction (from embedded-services::relay::mctp bitfield!): is_request: raw bit 25 service_id: raw bits 23..16 is_error: raw bit 15 message_id: raw bits 14..0 (15-bit field, widened from u8 to u16) Decoder appends a placeholder PEC byte before delegating to mctp-rs::SmbusEspiMedium::deserialize (which requires but does not validate the trailing PEC). Encoder drops the trailing PEC from mctp-rs's output to match uart-service's on-wire form. Both documented inline. Closes Phase 15 MF-01, MF-02, MF-03. Per planning Phase 15 D-1, D-2. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 64 ++++++- Cargo.toml | 1 + sp-mctp-framer/Cargo.toml | 17 ++ sp-mctp-framer/src/lib.rs | 372 ++++++++++++++++++++++++++++++++++++ sp-mctp-framer/src/tests.rs | 83 ++++++++ 5 files changed, 533 insertions(+), 4 deletions(-) create mode 100644 sp-mctp-framer/Cargo.toml create mode 100644 sp-mctp-framer/src/lib.rs create mode 100644 sp-mctp-framer/src/tests.rs diff --git a/Cargo.lock b/Cargo.lock index c9509e2..e49c304 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -150,6 +150,12 @@ dependencies = [ "husky-rs", ] +[[package]] +name = "diff" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8" + [[package]] name = "ec-service-lib" version = "0.1.0" @@ -161,6 +167,12 @@ dependencies = [ "uuid", ] +[[package]] +name = "embedded-crc-macros" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4f1c75747a43b086df1a87fb2a889590bc0725e0abf54bba6d0c4bf7bd9e762c" + [[package]] name = "equivalent" version = "1.0.2" @@ -336,6 +348,17 @@ version = "0.4.27" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "13dc2df351e3202783a1fe0d44375f7295ffb4049267b0f3018346dc122a1d94" +[[package]] +name = "mctp-rs" +version = "0.1.0" +source = "git+https://github.com/dymk/mctp-rs?rev=3d941ba5205ca7781bf37e3dc7c5dfdc99a082d6#3d941ba5205ca7781bf37e3dc7c5dfdc99a082d6" +dependencies = [ + "bit-register", + "num_enum", + "smbus-pec", + "thiserror", +] + [[package]] name = "memchr" version = "2.7.4" @@ -400,6 +423,16 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" +[[package]] +name = "pretty_assertions" +version = "1.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ae130e2f271fbc2ac3a40fb1d07180839cdbbe443c7a27e1e3c13c5cac0116d" +dependencies = [ + "diff", + "yansi", +] + [[package]] name = "proc-macro-crate" version = "3.3.0" @@ -543,6 +576,23 @@ dependencies = [ "autocfg", ] +[[package]] +name = "smbus-pec" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ca0763a680cd5d72b28f7bfc8a054c117d8841380a6ad4f72f05bd2a34217d3e" +dependencies = [ + "embedded-crc-macros", +] + +[[package]] +name = "sp-mctp-framer" +version = "0.1.0" +dependencies = [ + "mctp-rs", + "pretty_assertions", +] + [[package]] name = "static_assertions" version = "1.1.0" @@ -585,18 +635,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "2.0.12" +version = "2.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "567b8a2dae586314f7be2a752ec7474332959c6460e02bde30d702a66d488708" +checksum = "4288b5bcbc7920c07a1149a35cf9590a2aa808e0bc1eafaade0b80947865fbc4" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "2.0.12" +version = "2.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7f7cf42b4507d8ea322120659672cf1b9dbb93f8f2d4ecfd6e51350ff5b17a1d" +checksum = "ebc4ee7f67670e9b64d05fa4253e753e016c6c95ff35b89b7941d6b856dec1d5" dependencies = [ "proc-macro2", "quote", @@ -766,3 +816,9 @@ checksum = "c06928c8748d81b05c9be96aad92e1b6ff01833332f281e8cfca3be4b35fc9ec" dependencies = [ "memchr", ] + +[[package]] +name = "yansi" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cfe53a6657fd280eaa890a3bc59152892ffa3e30101319d168b781ed6529b049" diff --git a/Cargo.toml b/Cargo.toml index 7805bb9..0910f90 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,7 @@ members = [ "platform/ihv1-sp", "platform/qemu-sp", "aarch64-haf", + "sp-mctp-framer", ] [workspace.package] diff --git a/sp-mctp-framer/Cargo.toml b/sp-mctp-framer/Cargo.toml new file mode 100644 index 0000000..0863c92 --- /dev/null +++ b/sp-mctp-framer/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "sp-mctp-framer" +categories = ["embedded", "no-std"] +version = "0.1.0" +edition = "2021" +rust-version.workspace = true +license = "MIT" +description = "SP-side MCTP framer over SmbusEspiMedium for SP↔EC interop" + +[dependencies] +mctp-rs.workspace = true + +[dev-dependencies] +pretty_assertions = "1.4.1" + +[lints] +workspace = true diff --git a/sp-mctp-framer/src/lib.rs b/sp-mctp-framer/src/lib.rs new file mode 100644 index 0000000..dd4ce76 --- /dev/null +++ b/sp-mctp-framer/src/lib.rs @@ -0,0 +1,372 @@ +//! SP-side MCTP framer over `SmbusEspiMedium` for SP↔EC interop. +//! +//! Mirrors EC's `embedded-services::uart-service` framing byte-for-byte +//! (verified by the Phase 12 spike capture; see +//! `.planning/phases/12-spike-mctp-uart-ping/12-VERDICT.md`). +//! +//! Runtime path uses only stack/static buffers — no heap (MF-03). + +#![no_std] + +use core::fmt; + +use mctp_rs::smbus_espi::{SmbusEspiMedium, SmbusEspiReplyContext}; +use mctp_rs::{ + EndpointId, MctpMessageHeaderTrait, MctpMessageTrait, MctpPacketContext, MctpPacketError, + MctpPacketResult, MctpReplyContext, MctpSequenceNumber, MctpMessageTag, +}; + +// --------------------------------------------------------------------------- +// Wire constants +// --------------------------------------------------------------------------- +// +// All on-wire constants below are derived from the Phase 12 spike capture +// (12-VERDICT.md §(c)) cross-checked against: +// +// * EC's `embedded-services::uart-service::process_response` +// (uart-service/src/lib.rs:65-95) — slave-address values + endpoint-id / +// message-tag conventions. +// * `mctp-rs`'s `SmbusEspiMedium::serialize` +// (medium/smbus_espi.rs:58-102) — confirms reply_context source/dest +// slave addresses are SWAPPED on serialize, and the bit_register layout +// puts `destination_slave_address` in bits 25..31 of the LE u32 that is +// then written `to_be_bytes()` (so it lands in wire byte 0). +// * `mctp-rs`'s `serialize_packet` / `SerializePacketState::next` +// (mctp_packet_context.rs:155-187, serialize.rs:25-85) — confirms the +// transport-header source_endpoint_id / destination_endpoint_id are +// SWAPPED on serialize (line 62-63 of serialize.rs), and that +// `packet_sequence_number` is `.inc()`'d before being serialized (so the +// initial value 0 yields wire seq=1 → matches captured byte 0xd3). +// * `embedded-services::relay::mctp` ODP relay header `bitfield!` +// (embedded-service/src/relay/mod.rs:285-303) — formal bit layout for +// the ODP relay header u32 (big-endian on wire): +// bit 25 : is_request (1 bit) +// bits 23..16: service_id (8 bits) +// bit 15 : is_error (1 bit) +// bits 14..0 : message_id (15 bits) +// +// Names below mirror `uart-service/src/lib.rs:69-78` so the SP↔EC interop +// surface is auditable side-by-side. + +/// SP-side MCTP endpoint ID (matches `uart-service` source_endpoint_id 0x80). +const SP_EID: u8 = 0x80; +/// EC battery-service MCTP endpoint ID (matches `BatteryService` discriminant). +const BATTERY_SVC_EID: u8 = 0x08; +/// MCTP message tag — `uart-service` hard-codes 3. +const MCTP_MSG_TAG: u8 = 3; +/// SmbusEspi destination slave (semantic — wire byte ordering is swapped by +/// `SmbusEspiMedium::serialize`). Matches `uart-service` line 75: `destination_slave_address: 1`. +const SMBUS_DST_SLAVE: u8 = 1; +/// SmbusEspi source slave. Matches `uart-service` line 76: `source_slave_address: 0`. +const SMBUS_SRC_SLAVE: u8 = 0; +/// Battery::GetSta message id (verdict §(c) byte 12 = 0x0f = 15). +const BATTERY_GETSTA_MSG_ID: u16 = 15; +/// Battery service id used by the ODP relay header (= BATTERY_SVC_EID). +const BATTERY_SVC_ID: u8 = 0x08; + +// --------------------------------------------------------------------------- +// Public API +// --------------------------------------------------------------------------- + +/// Errors surfaced by the framer. No allocator required. +#[derive(Debug, PartialEq, Eq)] +pub enum FramerError { + /// Output / scratch buffer too small for the requested operation. + BufTooSmall, + /// Input buffer too short to contain the expected wire framing. + Truncated, + /// Underlying mctp-rs decoder/encoder rejected the input. + DecodeFailed, + /// Underlying mctp-rs decoder/encoder rejected the encode. + EncodeFailed, +} + +impl fmt::Display for FramerError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + FramerError::BufTooSmall => f.write_str("output buffer too small"), + FramerError::Truncated => f.write_str("input truncated"), + FramerError::DecodeFailed => f.write_str("decode failed"), + FramerError::EncodeFailed => f.write_str("encode failed"), + } + } +} + +/// Structured view of a decoded EC→SP Battery response. +/// +/// The `body` slice borrows from the caller's input buffer and contains the +/// per-service result payload (i.e. everything after the 4-byte ODP relay +/// header). +#[derive(Debug, PartialEq, Eq)] +pub struct BatteryResponse<'a> { + pub is_request: bool, + pub service_id: u8, + pub is_error: bool, + pub message_id: u16, + pub body: &'a [u8], +} + +/// Encode a Battery::GetSta request for `battery_id` into `buf` over the +/// SmbusEspiMedium framing used by EC's `uart-service`. +/// +/// Returns the number of bytes written. The output is the on-wire byte +/// sequence WITHOUT the trailing PEC byte — `uart-service` strips PEC before +/// writing to UART (uart-service/src/lib.rs:88), and the Phase 12 capture is +/// the post-strip form. Phase 16 (the actual TX wiring) decides whether to +/// re-add PEC if the underlying transport requires it. +pub fn encode_battery_request(buf: &mut [u8], battery_id: u8) -> Result { + // Cheap up-front guard so callers with obviously-too-small buffers fail + // fast (Test 3 — `encode_short_buf_errors`). + const MIN_OUTPUT: usize = 18; + if buf.len() < MIN_OUTPUT { + return Err(FramerError::BufTooSmall); + } + + // Scratch buffer for mctp-rs's packet assembly. Sized for the worst + // single-packet SmbusEspi frame (max body 32 + 4 header + 1 PEC = 37); + // we also need room for the prefix that `serialize_packet` carves off + // (msg_type 1 + odp header 4 + odp body 5 = 10). Round to 64. + let mut assembly_buf = [0u8; 64]; + let mut ctx = + MctpPacketContext::::new(SmbusEspiMedium, &mut assembly_buf); + + let reply_ctx = MctpReplyContext:: { + // Note the swap convention (see file-header comment): `source_*` + // here becomes the on-wire DESTINATION after `SmbusEspiMedium::serialize` + // / `SerializePacketState::next` swap them. So to put EC (0x08) on + // wire-dst and SP (0x80) on wire-src, we set source=EC, dest=SP. + source_endpoint_id: EndpointId::Id(BATTERY_SVC_EID), + destination_endpoint_id: EndpointId::Id(SP_EID), + packet_sequence_number: MctpSequenceNumber::new(0), + message_tag: MctpMessageTag::try_from(MCTP_MSG_TAG) + .map_err(|_| FramerError::EncodeFailed)?, + medium_context: SmbusEspiReplyContext { + destination_slave_address: SMBUS_DST_SLAVE, + source_slave_address: SMBUS_SRC_SLAVE, + }, + }; + + // ODP relay header for an outbound Battery::GetSta request: + // is_request=1, service_id=0x08, is_error=0, message_id=15 + let odp_header = OdpRelayHeader { + is_request: true, + service_id: BATTERY_SVC_ID, + is_error: false, + message_id: BATTERY_GETSTA_MSG_ID, + }; + + // ODP body for Battery::GetSta(battery_id) — verdict §(c) shows the + // payload is the relay-header re-echo + a single battery_id byte. + let body = OdpRelayBody([ + 0x02, // is_request=1 (high nibble) + BATTERY_SVC_ID, + 0x00, + BATTERY_GETSTA_MSG_ID as u8, + battery_id, + ]); + + let mut packet_state = ctx + .serialize_packet(reply_ctx, (odp_header, body)) + .map_err(|_| FramerError::EncodeFailed)?; + + // The framer is single-packet (Battery::GetSta easily fits in one MCTP + // body), so we expect exactly one `next()`. + let packet = packet_state + .next() + .ok_or(FramerError::EncodeFailed)? + .map_err(|_| FramerError::EncodeFailed)?; + + // Drop trailing PEC byte to match the on-wire form `uart-service` + // produces (and the Phase 12 capture). + let stripped = packet + .get(..packet.len().saturating_sub(1)) + .ok_or(FramerError::EncodeFailed)?; + + if buf.len() < stripped.len() { + return Err(FramerError::BufTooSmall); + } + buf[..stripped.len()].copy_from_slice(stripped); + Ok(stripped.len()) +} + +/// Decode an EC-originated SmbusEspi+MCTP framed Battery response from `buf`. +/// +/// `buf` is the on-wire byte sequence as produced by `uart-service` +/// (i.e. PEC byte already stripped). +pub fn decode_battery_response(buf: &[u8]) -> Result, FramerError> { + // mctp-rs's `SmbusEspiMedium::deserialize` requires a trailing PEC byte + // (medium/smbus_espi.rs:46-55). The on-wire form `uart-service` produces + // does NOT include PEC. Re-attach a placeholder PEC byte so mctp-rs can + // parse — the deserializer reads but does not validate the PEC value. + if buf.len() < 4 { + return Err(FramerError::Truncated); + } + let byte_count = buf[2] as usize; + let needed = 4 + byte_count; + if buf.len() < needed { + return Err(FramerError::Truncated); + } + + let mut padded = [0u8; 64]; + if needed + 1 > padded.len() { + return Err(FramerError::DecodeFailed); + } + padded[..needed].copy_from_slice(&buf[..needed]); + padded[needed] = 0; // dummy PEC + + let mut assembly_buf = [0u8; 64]; + let mut ctx = + MctpPacketContext::::new(SmbusEspiMedium, &mut assembly_buf); + + let message = ctx + .deserialize_packet(&padded[..needed + 1]) + .map_err(|_| FramerError::DecodeFailed)? + .ok_or(FramerError::DecodeFailed)?; + + let (header, body) = message + .parse_as::() + .map_err(|_| FramerError::DecodeFailed)?; + + // `body.0` is the per-service result payload (everything past the 4-byte + // ODP header). It borrows from `assembly_buf` which lives only for this + // call; copy the few-byte borrow into a slice that borrows from the + // caller's input instead. We do this by re-slicing `buf` directly. + // + // Layout of `buf` (PEC stripped): + // [0..4) SmbusEspi header + // [4..8) MCTP transport header + // [8] MCTP message type byte (0x7d) + // [9..13) ODP relay header + // [13..) per-service result body + let _ = body; + let result_body = buf.get(13..needed).unwrap_or(&[]); + + Ok(BatteryResponse { + is_request: header.is_request, + service_id: header.service_id, + is_error: header.is_error, + message_id: header.message_id, + body: result_body, + }) +} + +// --------------------------------------------------------------------------- +// ODP relay header + body — minimal local impls of mctp-rs's +// `MctpMessageHeaderTrait` / `MctpMessageTrait` so we can drive +// `serialize_packet` / `parse_as` with the ODP message type (0x7d). +// --------------------------------------------------------------------------- + +/// MCTP message type byte that ODP uses for relay traffic (verdict §(c) byte 8). +const ODP_MSG_TYPE: u8 = 0x7d; + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +struct OdpRelayHeader { + is_request: bool, + service_id: u8, + is_error: bool, + message_id: u16, +} + +impl OdpRelayHeader { + /// Bit-extraction layout per `embedded-services::relay::mctp` `bitfield!` + /// (embedded-service/src/relay/mod.rs:285-303), wire bytes are + /// big-endian: + /// bit 25 : is_request (1) + /// bits 23..16: service_id (8) + /// bit 15 : is_error (1) + /// bits 14..0 : message_id (15) + fn from_u32(raw: u32) -> Self { + Self { + is_request: ((raw >> 25) & 1) != 0, + service_id: ((raw >> 16) & 0xff) as u8, + is_error: ((raw >> 15) & 1) != 0, + message_id: (raw & 0x7fff) as u16, + } + } + + fn to_u32(self) -> u32 { + ((self.is_request as u32) << 25) + | ((self.service_id as u32) << 16) + | ((self.is_error as u32) << 15) + | ((self.message_id as u32) & 0x7fff) + } +} + +impl MctpMessageHeaderTrait for OdpRelayHeader { + fn serialize(self, buffer: &mut [u8]) -> MctpPacketResult { + if buffer.len() < 4 { + return Err(MctpPacketError::SerializeError( + "buffer too small for odp relay header", + )); + } + buffer[..4].copy_from_slice(&self.to_u32().to_be_bytes()); + Ok(4) + } + + fn deserialize( + buffer: &[u8], + ) -> MctpPacketResult<(Self, &[u8]), M> { + if buffer.len() < 4 { + return Err(MctpPacketError::HeaderParseError( + "buffer too small for odp relay header", + )); + } + let raw = u32::from_be_bytes([buffer[0], buffer[1], buffer[2], buffer[3]]); + Ok((OdpRelayHeader::from_u32(raw), &buffer[4..])) + } +} + +/// Outbound (encode-side) ODP body — owns its 5-byte payload by value so it +/// satisfies the lifetime gymnastics of `MctpMessageTrait<'buf>` without +/// needing a borrow into the caller's stack. +struct OdpRelayBody([u8; 5]); + +impl<'buf> MctpMessageTrait<'buf> for OdpRelayBody { + type Header = OdpRelayHeader; + const MESSAGE_TYPE: u8 = ODP_MSG_TYPE; + + fn serialize(self, buffer: &mut [u8]) -> MctpPacketResult { + if buffer.len() < self.0.len() { + return Err(MctpPacketError::SerializeError( + "buffer too small for odp relay body", + )); + } + buffer[..self.0.len()].copy_from_slice(&self.0); + Ok(self.0.len()) + } + + fn deserialize( + _: &Self::Header, + _buffer: &'buf [u8], + ) -> MctpPacketResult { + // Encode-side type — should never be used on the decode path. + Err(MctpPacketError::HeaderParseError( + "OdpRelayBody is encode-only", + )) + } +} + +/// Decode-side ODP body wrapper — owns no data; we re-slice the caller's +/// input buffer in `decode_battery_response` to produce the borrow. +struct OdpRelayBodyOwned; + +impl<'buf> MctpMessageTrait<'buf> for OdpRelayBodyOwned { + type Header = OdpRelayHeader; + const MESSAGE_TYPE: u8 = ODP_MSG_TYPE; + + fn serialize(self, _: &mut [u8]) -> MctpPacketResult { + Err(MctpPacketError::SerializeError( + "OdpRelayBodyOwned is decode-only", + )) + } + + fn deserialize( + _: &Self::Header, + _buffer: &'buf [u8], + ) -> MctpPacketResult { + Ok(OdpRelayBodyOwned) + } +} + +#[cfg(test)] +mod tests; diff --git a/sp-mctp-framer/src/tests.rs b/sp-mctp-framer/src/tests.rs new file mode 100644 index 0000000..3be11d1 --- /dev/null +++ b/sp-mctp-framer/src/tests.rs @@ -0,0 +1,83 @@ +//! Golden-bytes round-trip tests against Phase 12 spike captures. +//! +//! See `.planning/phases/12-spike-mctp-uart-ping/12-VERDICT.md` §(c) for the +//! provenance of the byte sequences asserted below. These bytes were captured +//! verbatim from a successful SP→EC→SP round-trip on the QEMU SBSA secure +//! UART (`0x60030000`) and represent the canonical interop contract this +//! framer must preserve. + +#![cfg(test)] + +use crate::{decode_battery_response, encode_battery_request, FramerError}; +use pretty_assertions::assert_eq; + +/// Phase 12 spike-captured TX (SP→EC, Battery::GetSta request, battery_id=0). +/// +/// Byte breakdown (verdict §(c)): +/// 00 0f 0e 03 SmbusEspi header (dst_slave=0, src_slave=1, byte_count=14, cmd=0x0f MCTP) +/// 01 08 80 d3 MCTP transport header (ver=1, dst_eid=0x08 EC, src_eid=0x80 SP, SOM/EOM/seq=1/tag=3) +/// 7d MCTP message type byte (ODP relay) +/// 02 08 00 0f ODP relay header (is_request=1, service_id=0x08, msg_id=15) +/// 02 08 00 0f 00 ODP body: relay-header re-echo + battery_id=0 +const GOLDEN_TX_18B: &[u8] = &[ + 0x00, 0x0f, 0x0e, 0x03, + 0x01, 0x08, 0x80, 0xd3, + 0x7d, + 0x02, 0x08, 0x00, 0x0f, + 0x02, 0x08, 0x00, 0x0f, 0x00, +]; + +/// Phase 12 spike-captured RX (EC→SP, Battery error response, message_id=1). +/// +/// Byte breakdown (verdict §(c)): +/// 00 0f 09 03 SmbusEspi header (byte_count=0x09) +/// 01 80 08 d3 MCTP transport header (dst_eid=0x80 SP, src_eid=0x08 EC battery) +/// 7d MCTP message type byte (ODP relay) +/// 00 08 80 01 ODP relay header (is_request=0, service_id=0x08, is_error=1, msg_id=1) +const GOLDEN_RX_13B: &[u8] = &[ + 0x00, 0x0f, 0x09, 0x03, + 0x01, 0x80, 0x08, 0xd3, + 0x7d, + 0x00, 0x08, 0x80, 0x01, +]; + +#[test] +fn encode_battery_request_matches_phase12_capture() { + let mut buf = [0u8; 64]; + let n = encode_battery_request(&mut buf, /*battery_id=*/ 0).expect("encode ok"); + assert_eq!(n, GOLDEN_TX_18B.len(), "encoded length mismatch"); + assert_eq!(&buf[..n], GOLDEN_TX_18B, "encoded bytes diverge from Phase 12 capture"); +} + +#[test] +fn decode_battery_response_parses_phase12_rx() { + let resp = decode_battery_response(GOLDEN_RX_13B).expect("decode ok"); + assert!(!resp.is_request, "expected response (is_request=false)"); + assert_eq!(resp.service_id, 0x08, "expected Battery service"); + assert!(resp.is_error, "expected is_error=1 from spike capture"); + assert_eq!(resp.message_id, 1, "expected message_id=1"); +} + +#[test] +fn encode_short_buf_errors() { + let mut buf = [0u8; 4]; // intentionally too small + let result = encode_battery_request(&mut buf, 0); + assert!( + matches!(result, Err(FramerError::BufTooSmall)), + "expected BufTooSmall, got {:?}", + result + ); +} + +#[test] +fn decode_truncated_errors() { + let result = decode_battery_response(&[0x00, 0x0f]); // truncated SmbusEspi header + assert!( + matches!( + result, + Err(FramerError::Truncated) | Err(FramerError::DecodeFailed) + ), + "expected Truncated or DecodeFailed, got {:?}", + result + ); +} From 07462b27f092c0b3434f6d76b3a983409dcae1be Mon Sep 17 00:00:00 2001 From: Copilot Date: Fri, 24 Apr 2026 23:10:21 -0700 Subject: [PATCH 4/5] fix(sp-mctp-framer): close v1.3 review findings + inline tests + test-fixtures feature Address 9 v1.3 review findings on the framer crate: - M2: decode_battery_response now hard-errors (Truncated) on a body slice that under-runs the SmbusEspi-claimed byte_count, instead of silently returning an empty body. - F-3: oversized byte_count beyond the input buffer is mapped to BufTooSmall rather than the (more confusing) Truncated. - L-doc-3: body header bytes are now derived from odp_header.to_u32().to_be_bytes() at runtime instead of a hard-coded literal, so the encode path can never drift from the header struct. - M-doc-2: rename OdpRelayBodyOwned -> OdpRelayBodyDecode (the type is not actually owned; it borrows from the caller's RX buffer). - M5: introduce a 'test-fixtures' Cargo feature that re-exports the Phase 12 captured wire bytes (PHASE_12_TX_18B / PHASE_12_RX_13B) as pub constants. Replaces the bit-identical duplicate previously living in the OUTER mctp_ping.rs. - T-1, T-2, T-7, T-10, F-3 tests: inline 'mod tests' into lib.rs and add coverage for the truncation, oversize, partial-body, encode/ decode roundtrip, and 17/18-byte boundary cases. Also rewrite Cargo.toml to use the workspace pretty_assertions, declare peer-crate metadata (repository / readme / keywords), and gate the new fixtures behind the test-fixtures feature. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Cargo.toml | 1 + sp-mctp-framer/Cargo.toml | 16 ++- sp-mctp-framer/README.md | 23 ++++ sp-mctp-framer/src/lib.rs | 243 ++++++++++++++++++++++++++++++++---- sp-mctp-framer/src/tests.rs | 83 ------------ 5 files changed, 255 insertions(+), 111 deletions(-) create mode 100644 sp-mctp-framer/README.md delete mode 100644 sp-mctp-framer/src/tests.rs diff --git a/Cargo.toml b/Cargo.toml index 0910f90..4e99c1f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,6 +42,7 @@ num-traits = { version = "0.2.19", default-features = false } subenum = { version = "1.1.2", default-features = false } uuid = { version = "1.0", default-features = false, features = ["v1"] } rstest = "0.26.1" +pretty_assertions = "1.4.1" [workspace.lints.clippy] suspicious = "deny" diff --git a/sp-mctp-framer/Cargo.toml b/sp-mctp-framer/Cargo.toml index 0863c92..2869ffd 100644 --- a/sp-mctp-framer/Cargo.toml +++ b/sp-mctp-framer/Cargo.toml @@ -1,17 +1,29 @@ [package] name = "sp-mctp-framer" -categories = ["embedded", "no-std"] version = "0.1.0" edition = "2021" rust-version.workspace = true license = "MIT" description = "SP-side MCTP framer over SmbusEspiMedium for SP↔EC interop" +repository = "https://github.com/OpenDevicePartnership/odp-secure-services" +readme = "README.md" +keywords = ["mctp", "smbus", "no-std", "embedded", "sp"] +categories = ["embedded", "no-std"] + +[features] +# Re-export Phase 12 golden capture fixtures for downstream test suites +# (e.g. `mod/secure-services/platform/src/mctp_ping.rs` host tests). Keeps +# the wire-format goldens in a single place — see lib.rs::test_fixtures. +test-fixtures = [] [dependencies] mctp-rs.workspace = true [dev-dependencies] -pretty_assertions = "1.4.1" +pretty_assertions.workspace = true [lints] workspace = true + +[package.metadata.docs.rs] +all-features = true diff --git a/sp-mctp-framer/README.md b/sp-mctp-framer/README.md new file mode 100644 index 0000000..364bb3b --- /dev/null +++ b/sp-mctp-framer/README.md @@ -0,0 +1,23 @@ +# sp-mctp-framer + +SP-side MCTP framer over `SmbusEspiMedium`. Encodes Battery::GetSta requests +and decodes EC→SP Battery responses byte-for-byte against EC's +`embedded-services::uart-service` framing. + +## Usage + +```rust +use sp_mctp_framer::{encode_battery_request, decode_battery_response}; + +let mut tx = [0u8; 32]; +let n = encode_battery_request(&mut tx, /* battery_id = */ 0)?; +// transmit tx[..n] over the wire ... +let resp = decode_battery_response(&rx[..])?; +``` + +Runtime path uses only stack/static buffers (no heap). + +## Features + +- `test-fixtures` — re-export captured ping-pong wire bytes + (`PHASE_12_TX_18B`, `PHASE_12_RX_13B`) for downstream test crates. diff --git a/sp-mctp-framer/src/lib.rs b/sp-mctp-framer/src/lib.rs index dd4ce76..c54dc41 100644 --- a/sp-mctp-framer/src/lib.rs +++ b/sp-mctp-framer/src/lib.rs @@ -1,10 +1,10 @@ //! SP-side MCTP framer over `SmbusEspiMedium` for SP↔EC interop. //! //! Mirrors EC's `embedded-services::uart-service` framing byte-for-byte -//! (verified by the Phase 12 spike capture; see -//! `.planning/phases/12-spike-mctp-uart-ping/12-VERDICT.md`). +//! (verified against captured wire bytes from a successful SP→EC→SP +//! ping-pong on the QEMU SBSA secure UART at `0x60030000`). //! -//! Runtime path uses only stack/static buffers — no heap (MF-03). +//! Runtime path uses only stack/static buffers — no heap. #![no_std] @@ -20,8 +20,8 @@ use mctp_rs::{ // Wire constants // --------------------------------------------------------------------------- // -// All on-wire constants below are derived from the Phase 12 spike capture -// (12-VERDICT.md §(c)) cross-checked against: +// All on-wire constants below were derived from a captured SP→EC ping-pong +// session and cross-checked against: // // * EC's `embedded-services::uart-service::process_response` // (uart-service/src/lib.rs:65-95) — slave-address values + endpoint-id / @@ -59,7 +59,7 @@ const MCTP_MSG_TAG: u8 = 3; const SMBUS_DST_SLAVE: u8 = 1; /// SmbusEspi source slave. Matches `uart-service` line 76: `source_slave_address: 0`. const SMBUS_SRC_SLAVE: u8 = 0; -/// Battery::GetSta message id (verdict §(c) byte 12 = 0x0f = 15). +/// Battery::GetSta message id (captured wire byte 12 = 0x0f = 15). const BATTERY_GETSTA_MSG_ID: u16 = 15; /// Battery service id used by the ODP relay header (= BATTERY_SVC_EID). const BATTERY_SVC_ID: u8 = 0x08; @@ -155,13 +155,17 @@ pub fn encode_battery_request(buf: &mut [u8], battery_id: u8) -> Result Result, Framer let mut padded = [0u8; 64]; if needed + 1 > padded.len() { - return Err(FramerError::DecodeFailed); + // Wire-claimed size exceeds our internal padded scratch. Surface as + // BufTooSmall (the requested operation cannot fit our buffer) rather + // than DecodeFailed (which implies semantic rejection). + return Err(FramerError::BufTooSmall); } padded[..needed].copy_from_slice(&buf[..needed]); padded[needed] = 0; // dummy PEC @@ -224,7 +231,7 @@ pub fn decode_battery_response(buf: &[u8]) -> Result, Framer .ok_or(FramerError::DecodeFailed)?; let (header, body) = message - .parse_as::() + .parse_as::() .map_err(|_| FramerError::DecodeFailed)?; // `body.0` is the per-service result payload (everything past the 4-byte @@ -238,8 +245,14 @@ pub fn decode_battery_response(buf: &[u8]) -> Result, Framer // [8] MCTP message type byte (0x7d) // [9..13) ODP relay header // [13..) per-service result body + // + // Defense-in-depth: if `byte_count` is small enough that the header alone + // doesn't fit (`needed < 13`), `buf.get(13..needed)` is an inverted range. + // Treat that as truncation rather than silently substituting `&[]`, since + // a downstream caller that gates only on `service_id` could otherwise + // accept a malformed-but-mctp-rs-parsed frame. let _ = body; - let result_body = buf.get(13..needed).unwrap_or(&[]); + let result_body = buf.get(13..needed).ok_or(FramerError::Truncated)?; Ok(BatteryResponse { is_request: header.is_request, @@ -256,7 +269,7 @@ pub fn decode_battery_response(buf: &[u8]) -> Result, Framer // `serialize_packet` / `parse_as` with the ODP message type (0x7d). // --------------------------------------------------------------------------- -/// MCTP message type byte that ODP uses for relay traffic (verdict §(c) byte 8). +/// MCTP message type byte that ODP uses for relay traffic (captured wire byte 8). const ODP_MSG_TYPE: u8 = 0x7d; #[derive(Copy, Clone, Debug, PartialEq, Eq)] @@ -316,9 +329,10 @@ impl MctpMessageHeaderTrait for OdpRelayHeader { } } -/// Outbound (encode-side) ODP body — owns its 5-byte payload by value so it -/// satisfies the lifetime gymnastics of `MctpMessageTrait<'buf>` without -/// needing a borrow into the caller's stack. +/// Encode-side ODP body — owns its 5-byte payload by value so it satisfies +/// the lifetime gymnastics of `MctpMessageTrait<'buf>` without borrowing +/// from the caller's stack. (Encode-only: see `OdpRelayBodyDecode` for the +/// decode-side counterpart.) struct OdpRelayBody([u8; 5]); impl<'buf> MctpMessageTrait<'buf> for OdpRelayBody { @@ -346,17 +360,19 @@ impl<'buf> MctpMessageTrait<'buf> for OdpRelayBody { } } -/// Decode-side ODP body wrapper — owns no data; we re-slice the caller's -/// input buffer in `decode_battery_response` to produce the borrow. -struct OdpRelayBodyOwned; +/// Decode-side marker type. Carries no payload — the body bytes are +/// re-sliced from the caller's input buffer in `decode_battery_response` +/// (so they outlive `mctp-rs`'s transient assembly buffer). Renamed from +/// the misleading `OdpRelayBodyOwned`: it owns nothing. +struct OdpRelayBodyDecode; -impl<'buf> MctpMessageTrait<'buf> for OdpRelayBodyOwned { +impl<'buf> MctpMessageTrait<'buf> for OdpRelayBodyDecode { type Header = OdpRelayHeader; const MESSAGE_TYPE: u8 = ODP_MSG_TYPE; fn serialize(self, _: &mut [u8]) -> MctpPacketResult { Err(MctpPacketError::SerializeError( - "OdpRelayBodyOwned is decode-only", + "OdpRelayBodyDecode is decode-only", )) } @@ -364,9 +380,184 @@ impl<'buf> MctpMessageTrait<'buf> for OdpRelayBodyOwned { _: &Self::Header, _buffer: &'buf [u8], ) -> MctpPacketResult { - Ok(OdpRelayBodyOwned) + Ok(OdpRelayBodyDecode) } } +// --------------------------------------------------------------------------- +// Test fixtures (re-exported for downstream test crates under the +// `test-fixtures` feature). Kept here as the single source of truth for the +// captured ping-pong wire bytes — duplicating them across submodule +// boundaries (e.g. `mctp_ping.rs`'s host tests) silently masks wire-format +// drift if one copy is updated and the other is not. +// --------------------------------------------------------------------------- + +#[cfg(any(test, feature = "test-fixtures"))] +pub mod test_fixtures { + /// Captured TX (SP→EC, Battery::GetSta request, battery_id=0). + /// + /// Byte breakdown: + /// 00 0f 0e 03 SmbusEspi header (dst_slave=0, src_slave=1, byte_count=14, cmd=0x0f MCTP) + /// 01 08 80 d3 MCTP transport header (ver=1, dst_eid=0x08 EC, src_eid=0x80 SP, SOM/EOM/seq=1/tag=3) + /// 7d MCTP message type byte (ODP relay) + /// 02 08 00 0f ODP relay header (is_request=1, service_id=0x08, msg_id=15) + /// 02 08 00 0f 00 ODP body: relay-header re-echo + battery_id=0 + pub const PHASE_12_TX_18B: [u8; 18] = [ + 0x00, 0x0f, 0x0e, 0x03, + 0x01, 0x08, 0x80, 0xd3, + 0x7d, + 0x02, 0x08, 0x00, 0x0f, + 0x02, 0x08, 0x00, 0x0f, 0x00, + ]; + + /// Captured RX (EC→SP, Battery error response, message_id=1). + /// + /// Byte breakdown: + /// 00 0f 09 03 SmbusEspi header (byte_count=0x09) + /// 01 80 08 d3 MCTP transport header (dst_eid=0x80 SP, src_eid=0x08 EC battery) + /// 7d MCTP message type byte (ODP relay) + /// 00 08 80 01 ODP relay header (is_request=0, service_id=0x08, is_error=1, msg_id=1) + pub const PHASE_12_RX_13B: [u8; 13] = [ + 0x00, 0x0f, 0x09, 0x03, + 0x01, 0x80, 0x08, 0xd3, + 0x7d, + 0x00, 0x08, 0x80, 0x01, + ]; +} + #[cfg(test)] -mod tests; +mod tests { + use super::test_fixtures::{PHASE_12_RX_13B, PHASE_12_TX_18B}; + use super::{decode_battery_response, encode_battery_request, FramerError}; + use pretty_assertions::assert_eq; + + #[test] + fn encode_battery_request_matches_phase12_capture() { + let mut buf = [0u8; 64]; + let n = encode_battery_request(&mut buf, /*battery_id=*/ 0).expect("encode ok"); + assert_eq!(n, PHASE_12_TX_18B.len(), "encoded length mismatch"); + assert_eq!(&buf[..n], &PHASE_12_TX_18B[..], "encoded bytes diverge from captured wire"); + } + + #[test] + fn decode_battery_response_parses_phase12_rx() { + let resp = decode_battery_response(&PHASE_12_RX_13B).expect("decode ok"); + assert!(!resp.is_request, "expected response (is_request=false)"); + assert_eq!(resp.service_id, 0x08, "expected Battery service"); + assert!(resp.is_error, "expected is_error=1 from spike capture"); + assert_eq!(resp.message_id, 1, "expected message_id=1"); + // Captured RX has byte_count=9 → needed=13, so the per-service body + // is empty (just an ODP relay header echo). Pin that explicitly so a + // future regression to the body slice offset is caught. + assert_eq!(resp.body, &[][..], "Phase 12 RX has zero body bytes"); + } + + #[test] + fn decode_battery_response_extracts_body_bytes() { + // Synthetic frame with a non-empty per-service body: header byte_count + // bumped to 0x0d so needed=17, then 4 trailing bytes (0xDE 0xAD 0xBE + // 0xEF) appended after the 13-byte header. Validates that + // `decode_battery_response` returns the correct body slice (catches + // any future regression to the `13..needed` re-slice offset). + let mut frame = [0u8; 17]; + frame[..PHASE_12_RX_13B.len()].copy_from_slice(&PHASE_12_RX_13B); + frame[2] = 0x0d; // byte_count = 4 (header) + 9 (transport+ODP) → needed=17 + frame[13] = 0xDE; + frame[14] = 0xAD; + frame[15] = 0xBE; + frame[16] = 0xEF; + let resp = decode_battery_response(&frame).expect("decode ok"); + assert_eq!(resp.body, &[0xDE, 0xAD, 0xBE, 0xEF][..]); + } + + /// Roundtrip: encode each interesting battery_id, decode the result, + /// assert header fields are preserved. Catches encoder regressions where + /// the literal byte happens to match the captured fixture but the + /// semantic field has drifted. + #[test] + fn encode_decode_roundtrip_preserves_header_fields() { + for &battery_id in &[0u8, 1, 0x7D, 0x7E, 0xFF] { + let mut tx = [0u8; 64]; + let n = encode_battery_request(&mut tx, battery_id) + .unwrap_or_else(|e| panic!("encode failed for battery_id={battery_id}: {e:?}")); + // The encoder produces a REQUEST frame; `decode_battery_response` + // is permissive about request vs response and just decodes the + // ODP relay header. We assert the fields the SP cares about. + let resp = decode_battery_response(&tx[..n]) + .unwrap_or_else(|e| panic!("decode failed for battery_id={battery_id}: {e:?}")); + assert!(resp.is_request, "encoded frame must mark is_request=1"); + assert_eq!(resp.service_id, 0x08, "service_id must round-trip"); + assert!(!resp.is_error, "request frames have is_error=0"); + assert_eq!(resp.message_id, 15, "GetSta message_id must round-trip"); + assert_eq!( + resp.body.last().copied(), + Some(battery_id), + "battery_id selector must appear at end of body" + ); + } + } + + #[test] + fn encode_short_buf_errors() { + // Pin the BufTooSmall boundary at MIN_OUTPUT=18 — 17 bytes too small, + // 18 bytes sufficient. + let mut too_small = [0u8; 17]; + let result = encode_battery_request(&mut too_small, 0); + assert!( + matches!(result, Err(FramerError::BufTooSmall)), + "expected BufTooSmall at 17 bytes, got {result:?}" + ); + + let mut just_enough = [0u8; 18]; + let result = encode_battery_request(&mut just_enough, 0); + assert!( + result.is_ok(), + "expected Ok at 18 bytes, got {result:?}" + ); + } + + #[test] + fn decode_truncated_header_errors() { + let result = decode_battery_response(&[0x00, 0x0f]); // truncated SmbusEspi header + assert!( + matches!( + result, + Err(FramerError::Truncated) | Err(FramerError::DecodeFailed) + ), + "expected Truncated or DecodeFailed, got {result:?}" + ); + } + + #[test] + fn decode_partial_body_errors() { + // Header says byte_count=9 → needed=13, but only 6 bytes supplied. + // Exercises the second truncation guard (`buf.len() < needed`), + // distinct from the 4-byte header guard above. + let frame = [0x00u8, 0x0f, 0x09, 0x03, 0x01, 0x80]; + let result = decode_battery_response(&frame); + assert_eq!( + result, + Err(FramerError::Truncated), + "partial body must surface as Truncated" + ); + } + + #[test] + fn decode_oversize_byte_count_errors_as_buf_too_small() { + // byte_count=200 → needed=204, exceeding the internal 64-byte + // padded scratch. F-3 / review-correctness regression test — this + // path used to return DecodeFailed. + let mut frame = [0u8; 13]; + frame[2] = 200; + // pad needed bytes with zeros so the truncation guard doesn't fire + // first: we need buf.len() >= needed, so build a 204-byte buffer. + let mut big = [0u8; 204]; + big[2] = 200; + let result = decode_battery_response(&big); + assert_eq!( + result, + Err(FramerError::BufTooSmall), + "wire-claimed oversize must surface as BufTooSmall (not DecodeFailed)" + ); + } +} diff --git a/sp-mctp-framer/src/tests.rs b/sp-mctp-framer/src/tests.rs deleted file mode 100644 index 3be11d1..0000000 --- a/sp-mctp-framer/src/tests.rs +++ /dev/null @@ -1,83 +0,0 @@ -//! Golden-bytes round-trip tests against Phase 12 spike captures. -//! -//! See `.planning/phases/12-spike-mctp-uart-ping/12-VERDICT.md` §(c) for the -//! provenance of the byte sequences asserted below. These bytes were captured -//! verbatim from a successful SP→EC→SP round-trip on the QEMU SBSA secure -//! UART (`0x60030000`) and represent the canonical interop contract this -//! framer must preserve. - -#![cfg(test)] - -use crate::{decode_battery_response, encode_battery_request, FramerError}; -use pretty_assertions::assert_eq; - -/// Phase 12 spike-captured TX (SP→EC, Battery::GetSta request, battery_id=0). -/// -/// Byte breakdown (verdict §(c)): -/// 00 0f 0e 03 SmbusEspi header (dst_slave=0, src_slave=1, byte_count=14, cmd=0x0f MCTP) -/// 01 08 80 d3 MCTP transport header (ver=1, dst_eid=0x08 EC, src_eid=0x80 SP, SOM/EOM/seq=1/tag=3) -/// 7d MCTP message type byte (ODP relay) -/// 02 08 00 0f ODP relay header (is_request=1, service_id=0x08, msg_id=15) -/// 02 08 00 0f 00 ODP body: relay-header re-echo + battery_id=0 -const GOLDEN_TX_18B: &[u8] = &[ - 0x00, 0x0f, 0x0e, 0x03, - 0x01, 0x08, 0x80, 0xd3, - 0x7d, - 0x02, 0x08, 0x00, 0x0f, - 0x02, 0x08, 0x00, 0x0f, 0x00, -]; - -/// Phase 12 spike-captured RX (EC→SP, Battery error response, message_id=1). -/// -/// Byte breakdown (verdict §(c)): -/// 00 0f 09 03 SmbusEspi header (byte_count=0x09) -/// 01 80 08 d3 MCTP transport header (dst_eid=0x80 SP, src_eid=0x08 EC battery) -/// 7d MCTP message type byte (ODP relay) -/// 00 08 80 01 ODP relay header (is_request=0, service_id=0x08, is_error=1, msg_id=1) -const GOLDEN_RX_13B: &[u8] = &[ - 0x00, 0x0f, 0x09, 0x03, - 0x01, 0x80, 0x08, 0xd3, - 0x7d, - 0x00, 0x08, 0x80, 0x01, -]; - -#[test] -fn encode_battery_request_matches_phase12_capture() { - let mut buf = [0u8; 64]; - let n = encode_battery_request(&mut buf, /*battery_id=*/ 0).expect("encode ok"); - assert_eq!(n, GOLDEN_TX_18B.len(), "encoded length mismatch"); - assert_eq!(&buf[..n], GOLDEN_TX_18B, "encoded bytes diverge from Phase 12 capture"); -} - -#[test] -fn decode_battery_response_parses_phase12_rx() { - let resp = decode_battery_response(GOLDEN_RX_13B).expect("decode ok"); - assert!(!resp.is_request, "expected response (is_request=false)"); - assert_eq!(resp.service_id, 0x08, "expected Battery service"); - assert!(resp.is_error, "expected is_error=1 from spike capture"); - assert_eq!(resp.message_id, 1, "expected message_id=1"); -} - -#[test] -fn encode_short_buf_errors() { - let mut buf = [0u8; 4]; // intentionally too small - let result = encode_battery_request(&mut buf, 0); - assert!( - matches!(result, Err(FramerError::BufTooSmall)), - "expected BufTooSmall, got {:?}", - result - ); -} - -#[test] -fn decode_truncated_errors() { - let result = decode_battery_response(&[0x00, 0x0f]); // truncated SmbusEspi header - assert!( - matches!( - result, - Err(FramerError::Truncated) | Err(FramerError::DecodeFailed) - ), - "expected Truncated or DecodeFailed, got {:?}", - result - ); -} From 878bd3229f268537d94f6dddd4d72dcbeb982926 Mon Sep 17 00:00:00 2001 From: dymk Date: Sat, 25 Apr 2026 11:34:48 -0700 Subject: [PATCH 5/5] refactor(sp-mctp-framer): make encode/decode service-agnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename and generalize the public API so the framer is no longer hard-coded to Battery::GetSta: encode_battery_request(buf, battery_id) → encode_request(buf, service_id, message_id, payload) decode_battery_response(buf) -> BatteryResponse → decode_response(buf) -> OdpRelayResponse The framer now takes per-service identity from its caller (service_id and message_id) and an arbitrary-length payload slice, and prepends the 4-byte ODP relay header echo internally. The previously-hard-coded constants (BATTERY_SVC_EID, BATTERY_GETSTA_MSG_ID, BATTERY_SVC_ID) are removed; battery-specific values now live in the consumer (`mctp_ping.rs` in the platform crate). Implementation notes: * Internal `OdpRelayBody` is now `&'a [u8]` instead of `[u8; 5]`, so payload length is dynamic. Caller-side payload bound is `MAX_PAYLOAD_LEN` (publicly exposed); over-bound payloads surface as `BufTooSmall` (configuration error) not `EncodeFailed` (mctp-rs rejection) — new `encode_oversize_payload_errors_as_buf_too_small` test pins this. * Min output buffer size is now derived per-payload (was const 18). * SCRATCH_SIZE / FRAME_OVERHEAD constants extracted so the buffer-sizing math is auditable in one place. * `test_fixtures::PHASE_12_TX_18B` / `PHASE_12_RX_13B` retain their battery-specific names — they ARE captured battery traffic and serve as the wire-format reference for both encode and decode validation. * Inline tests parameterize on local `BATTERY_SVC_ID` / `BATTERY_GETSTA_MSG_ID` constants so they continue to validate against the captured fixtures while exercising the generic API surface. Test results: cargo test -p sp-mctp-framer --target x86_64-unknown-linux-gnu → 9 passed (was 8; new test covers oversize payload path) --- sp-mctp-framer/README.md | 21 ++-- sp-mctp-framer/src/lib.rs | 230 +++++++++++++++++++++++--------------- 2 files changed, 152 insertions(+), 99 deletions(-) diff --git a/sp-mctp-framer/README.md b/sp-mctp-framer/README.md index 364bb3b..b59d592 100644 --- a/sp-mctp-framer/README.md +++ b/sp-mctp-framer/README.md @@ -1,23 +1,30 @@ # sp-mctp-framer -SP-side MCTP framer over `SmbusEspiMedium`. Encodes Battery::GetSta requests -and decodes EC→SP Battery responses byte-for-byte against EC's -`embedded-services::uart-service` framing. +SP-side MCTP framer over `SmbusEspiMedium`. Encodes ODP-relay requests and +decodes ODP-relay responses byte-for-byte against EC's +`embedded-services::uart-service` framing. Service-agnostic — caller +supplies `service_id`, `message_id`, and the per-service payload bytes. ## Usage ```rust -use sp_mctp_framer::{encode_battery_request, decode_battery_response}; +use sp_mctp_framer::{encode_request, decode_response}; + +const BATTERY_SVC_ID: u8 = 0x08; +const BATTERY_GETSTA_MSG_ID: u16 = 15; let mut tx = [0u8; 32]; -let n = encode_battery_request(&mut tx, /* battery_id = */ 0)?; +let n = encode_request(&mut tx, BATTERY_SVC_ID, BATTERY_GETSTA_MSG_ID, &[/* battery_id */ 0])?; // transmit tx[..n] over the wire ... -let resp = decode_battery_response(&rx[..])?; +let resp = decode_response(&rx[..])?; ``` -Runtime path uses only stack/static buffers (no heap). +Runtime path uses only stack/static buffers (no heap). Maximum per-service +payload is `MAX_PAYLOAD_LEN` bytes — larger payloads return `BufTooSmall`. ## Features - `test-fixtures` — re-export captured ping-pong wire bytes (`PHASE_12_TX_18B`, `PHASE_12_RX_13B`) for downstream test crates. + These are the captured Battery::GetSta ping-pong from QEMU SBSA spike-up + and serve as the reference for both encode and decode validation. diff --git a/sp-mctp-framer/src/lib.rs b/sp-mctp-framer/src/lib.rs index c54dc41..afa24d4 100644 --- a/sp-mctp-framer/src/lib.rs +++ b/sp-mctp-framer/src/lib.rs @@ -1,8 +1,11 @@ //! SP-side MCTP framer over `SmbusEspiMedium` for SP↔EC interop. //! -//! Mirrors EC's `embedded-services::uart-service` framing byte-for-byte -//! (verified against captured wire bytes from a successful SP→EC→SP -//! ping-pong on the QEMU SBSA secure UART at `0x60030000`). +//! Service-agnostic: the public API takes `service_id`, `message_id`, and +//! a per-service payload, and produces / parses the SmbusEspi+MCTP+ODP-relay +//! wire framing that EC's `embedded-services::uart-service` expects. +//! Mirrors that crate byte-for-byte (verified against captured wire bytes +//! from a successful Battery::GetSta SP→EC→SP ping-pong on the QEMU SBSA +//! secure UART at `0x60030000`). //! //! Runtime path uses only stack/static buffers — no heap. @@ -50,8 +53,6 @@ use mctp_rs::{ /// SP-side MCTP endpoint ID (matches `uart-service` source_endpoint_id 0x80). const SP_EID: u8 = 0x80; -/// EC battery-service MCTP endpoint ID (matches `BatteryService` discriminant). -const BATTERY_SVC_EID: u8 = 0x08; /// MCTP message tag — `uart-service` hard-codes 3. const MCTP_MSG_TAG: u8 = 3; /// SmbusEspi destination slave (semantic — wire byte ordering is swapped by @@ -59,10 +60,18 @@ const MCTP_MSG_TAG: u8 = 3; const SMBUS_DST_SLAVE: u8 = 1; /// SmbusEspi source slave. Matches `uart-service` line 76: `source_slave_address: 0`. const SMBUS_SRC_SLAVE: u8 = 0; -/// Battery::GetSta message id (captured wire byte 12 = 0x0f = 15). -const BATTERY_GETSTA_MSG_ID: u16 = 15; -/// Battery service id used by the ODP relay header (= BATTERY_SVC_EID). -const BATTERY_SVC_ID: u8 = 0x08; +/// Number of leading wire bytes consumed by everything in front of the +/// per-service payload: 4-byte SmbusEspi header + 4-byte MCTP transport +/// header + 1-byte MCTP message-type + 4-byte ODP relay header. +const FRAME_OVERHEAD: usize = 4 + 4 + 1 + 4; +/// Internal mctp-rs scratch buffer size. Sized for a single-packet +/// SmbusEspi frame (max body 32 + 4 header + 1 PEC = 37) plus the prefix +/// `serialize_packet` carves off (msg_type 1 + odp header 4 = 5). Round to 64. +const SCRATCH_SIZE: usize = 64; +/// Maximum per-service payload the framer can encode/decode in one call. +/// Bounded by `SCRATCH_SIZE` minus `FRAME_OVERHEAD` minus the 1-byte PEC slot +/// the deserializer expects. Callers passing larger payloads get `BufTooSmall`. +pub const MAX_PAYLOAD_LEN: usize = SCRATCH_SIZE - FRAME_OVERHEAD - 1; // --------------------------------------------------------------------------- // Public API @@ -92,13 +101,13 @@ impl fmt::Display for FramerError { } } -/// Structured view of a decoded EC→SP Battery response. +/// Structured view of a decoded EC→SP ODP-relay response. /// /// The `body` slice borrows from the caller's input buffer and contains the /// per-service result payload (i.e. everything after the 4-byte ODP relay -/// header). +/// header echo). #[derive(Debug, PartialEq, Eq)] -pub struct BatteryResponse<'a> { +pub struct OdpRelayResponse<'a> { pub is_request: bool, pub service_id: u8, pub is_error: bool, @@ -106,36 +115,46 @@ pub struct BatteryResponse<'a> { pub body: &'a [u8], } -/// Encode a Battery::GetSta request for `battery_id` into `buf` over the -/// SmbusEspiMedium framing used by EC's `uart-service`. +/// Encode a generic ODP-relay request into `buf` over the SmbusEspiMedium +/// framing used by EC's `uart-service`. +/// +/// The framer prepends the 4-byte ODP relay header echo to `payload` before +/// handing it to mctp-rs, so callers pass only the per-service payload bytes +/// (e.g. `&[battery_id]` for `Battery::GetSta`). +/// +/// `service_id` is used both as the MCTP destination endpoint ID on the wire +/// and as the `service_id` field of the ODP relay header — these are the same +/// value by ODP convention (each service is its own MCTP endpoint). /// /// Returns the number of bytes written. The output is the on-wire byte /// sequence WITHOUT the trailing PEC byte — `uart-service` strips PEC before -/// writing to UART (uart-service/src/lib.rs:88), and the Phase 12 capture is -/// the post-strip form. Phase 16 (the actual TX wiring) decides whether to -/// re-add PEC if the underlying transport requires it. -pub fn encode_battery_request(buf: &mut [u8], battery_id: u8) -> Result { +/// writing to UART (uart-service/src/lib.rs:88). +pub fn encode_request( + buf: &mut [u8], + service_id: u8, + message_id: u16, + payload: &[u8], +) -> Result { + if payload.len() > MAX_PAYLOAD_LEN { + return Err(FramerError::BufTooSmall); + } // Cheap up-front guard so callers with obviously-too-small buffers fail - // fast (Test 3 — `encode_short_buf_errors`). - const MIN_OUTPUT: usize = 18; - if buf.len() < MIN_OUTPUT { + // fast. + let min_output = FRAME_OVERHEAD + payload.len(); + if buf.len() < min_output { return Err(FramerError::BufTooSmall); } - // Scratch buffer for mctp-rs's packet assembly. Sized for the worst - // single-packet SmbusEspi frame (max body 32 + 4 header + 1 PEC = 37); - // we also need room for the prefix that `serialize_packet` carves off - // (msg_type 1 + odp header 4 + odp body 5 = 10). Round to 64. - let mut assembly_buf = [0u8; 64]; + let mut assembly_buf = [0u8; SCRATCH_SIZE]; let mut ctx = MctpPacketContext::::new(SmbusEspiMedium, &mut assembly_buf); let reply_ctx = MctpReplyContext:: { // Note the swap convention (see file-header comment): `source_*` // here becomes the on-wire DESTINATION after `SmbusEspiMedium::serialize` - // / `SerializePacketState::next` swap them. So to put EC (0x08) on - // wire-dst and SP (0x80) on wire-src, we set source=EC, dest=SP. - source_endpoint_id: EndpointId::Id(BATTERY_SVC_EID), + // / `SerializePacketState::next` swap them. So to put the target + // service on wire-dst and SP on wire-src, we set source=service, dest=SP. + source_endpoint_id: EndpointId::Id(service_id), destination_endpoint_id: EndpointId::Id(SP_EID), packet_sequence_number: MctpSequenceNumber::new(0), message_tag: MctpMessageTag::try_from(MCTP_MSG_TAG) @@ -146,34 +165,32 @@ pub fn encode_battery_request(buf: &mut [u8], battery_id: u8) -> Result Result Result Result, FramerError> { +/// (i.e. PEC byte already stripped). The returned `body` slice borrows from +/// `buf` and contains the per-service result payload (everything past the +/// 4-byte ODP relay header echo). +/// +/// Callers gate on `service_id` themselves — a successful decode does NOT +/// imply the response targets any particular service. +pub fn decode_response(buf: &[u8]) -> Result, FramerError> { // mctp-rs's `SmbusEspiMedium::deserialize` requires a trailing PEC byte // (medium/smbus_espi.rs:46-55). The on-wire form `uart-service` produces // does NOT include PEC. Re-attach a placeholder PEC byte so mctp-rs can @@ -211,7 +233,7 @@ pub fn decode_battery_response(buf: &[u8]) -> Result, Framer return Err(FramerError::Truncated); } - let mut padded = [0u8; 64]; + let mut padded = [0u8; SCRATCH_SIZE]; if needed + 1 > padded.len() { // Wire-claimed size exceeds our internal padded scratch. Surface as // BufTooSmall (the requested operation cannot fit our buffer) rather @@ -221,7 +243,7 @@ pub fn decode_battery_response(buf: &[u8]) -> Result, Framer padded[..needed].copy_from_slice(&buf[..needed]); padded[needed] = 0; // dummy PEC - let mut assembly_buf = [0u8; 64]; + let mut assembly_buf = [0u8; SCRATCH_SIZE]; let mut ctx = MctpPacketContext::::new(SmbusEspiMedium, &mut assembly_buf); @@ -254,7 +276,7 @@ pub fn decode_battery_response(buf: &[u8]) -> Result, Framer let _ = body; let result_body = buf.get(13..needed).ok_or(FramerError::Truncated)?; - Ok(BatteryResponse { + Ok(OdpRelayResponse { is_request: header.is_request, service_id: header.service_id, is_error: header.is_error, @@ -329,24 +351,26 @@ impl MctpMessageHeaderTrait for OdpRelayHeader { } } -/// Encode-side ODP body — owns its 5-byte payload by value so it satisfies -/// the lifetime gymnastics of `MctpMessageTrait<'buf>` without borrowing -/// from the caller's stack. (Encode-only: see `OdpRelayBodyDecode` for the -/// decode-side counterpart.) -struct OdpRelayBody([u8; 5]); +/// Encode-side ODP body — borrows the assembled (4-byte header echo + +/// per-service payload) byte sequence so the framer can support variable-length +/// per-service payloads without a const generic. Encode-only — see +/// `OdpRelayBodyDecode` for the decode-side counterpart. +struct OdpRelayBody<'a> { + bytes: &'a [u8], +} -impl<'buf> MctpMessageTrait<'buf> for OdpRelayBody { +impl<'buf, 'a> MctpMessageTrait<'buf> for OdpRelayBody<'a> { type Header = OdpRelayHeader; const MESSAGE_TYPE: u8 = ODP_MSG_TYPE; fn serialize(self, buffer: &mut [u8]) -> MctpPacketResult { - if buffer.len() < self.0.len() { + if buffer.len() < self.bytes.len() { return Err(MctpPacketError::SerializeError( "buffer too small for odp relay body", )); } - buffer[..self.0.len()].copy_from_slice(&self.0); - Ok(self.0.len()) + buffer[..self.bytes.len()].copy_from_slice(self.bytes); + Ok(self.bytes.len()) } fn deserialize( @@ -428,20 +452,32 @@ pub mod test_fixtures { #[cfg(test)] mod tests { use super::test_fixtures::{PHASE_12_RX_13B, PHASE_12_TX_18B}; - use super::{decode_battery_response, encode_battery_request, FramerError}; + use super::{decode_response, encode_request, FramerError}; use pretty_assertions::assert_eq; + // Concrete service constants used to drive the captured Battery::GetSta + // ping-pong fixture. The framer itself is service-agnostic — these live + // in the tests because the fixtures they validate are battery traffic. + const BATTERY_SVC_ID: u8 = 0x08; + const BATTERY_GETSTA_MSG_ID: u16 = 15; + #[test] - fn encode_battery_request_matches_phase12_capture() { + fn encode_request_matches_phase12_battery_capture() { let mut buf = [0u8; 64]; - let n = encode_battery_request(&mut buf, /*battery_id=*/ 0).expect("encode ok"); + let n = encode_request( + &mut buf, + BATTERY_SVC_ID, + BATTERY_GETSTA_MSG_ID, + &[/*battery_id=*/ 0u8], + ) + .expect("encode ok"); assert_eq!(n, PHASE_12_TX_18B.len(), "encoded length mismatch"); assert_eq!(&buf[..n], &PHASE_12_TX_18B[..], "encoded bytes diverge from captured wire"); } #[test] - fn decode_battery_response_parses_phase12_rx() { - let resp = decode_battery_response(&PHASE_12_RX_13B).expect("decode ok"); + fn decode_response_parses_phase12_battery_rx() { + let resp = decode_response(&PHASE_12_RX_13B).expect("decode ok"); assert!(!resp.is_request, "expected response (is_request=false)"); assert_eq!(resp.service_id, 0x08, "expected Battery service"); assert!(resp.is_error, "expected is_error=1 from spike capture"); @@ -453,12 +489,12 @@ mod tests { } #[test] - fn decode_battery_response_extracts_body_bytes() { + fn decode_response_extracts_body_bytes() { // Synthetic frame with a non-empty per-service body: header byte_count // bumped to 0x0d so needed=17, then 4 trailing bytes (0xDE 0xAD 0xBE // 0xEF) appended after the 13-byte header. Validates that - // `decode_battery_response` returns the correct body slice (catches - // any future regression to the `13..needed` re-slice offset). + // `decode_response` returns the correct body slice (catches any future + // regression to the `13..needed` re-slice offset). let mut frame = [0u8; 17]; frame[..PHASE_12_RX_13B.len()].copy_from_slice(&PHASE_12_RX_13B); frame[2] = 0x0d; // byte_count = 4 (header) + 9 (transport+ODP) → needed=17 @@ -466,7 +502,7 @@ mod tests { frame[14] = 0xAD; frame[15] = 0xBE; frame[16] = 0xEF; - let resp = decode_battery_response(&frame).expect("decode ok"); + let resp = decode_response(&frame).expect("decode ok"); assert_eq!(resp.body, &[0xDE, 0xAD, 0xBE, 0xEF][..]); } @@ -478,47 +514,61 @@ mod tests { fn encode_decode_roundtrip_preserves_header_fields() { for &battery_id in &[0u8, 1, 0x7D, 0x7E, 0xFF] { let mut tx = [0u8; 64]; - let n = encode_battery_request(&mut tx, battery_id) + let n = encode_request(&mut tx, BATTERY_SVC_ID, BATTERY_GETSTA_MSG_ID, &[battery_id]) .unwrap_or_else(|e| panic!("encode failed for battery_id={battery_id}: {e:?}")); - // The encoder produces a REQUEST frame; `decode_battery_response` - // is permissive about request vs response and just decodes the - // ODP relay header. We assert the fields the SP cares about. - let resp = decode_battery_response(&tx[..n]) + // The encoder produces a REQUEST frame; `decode_response` is + // permissive about request vs response and just decodes the ODP + // relay header. Assert the fields the SP cares about. + let resp = decode_response(&tx[..n]) .unwrap_or_else(|e| panic!("decode failed for battery_id={battery_id}: {e:?}")); assert!(resp.is_request, "encoded frame must mark is_request=1"); - assert_eq!(resp.service_id, 0x08, "service_id must round-trip"); + assert_eq!(resp.service_id, BATTERY_SVC_ID, "service_id must round-trip"); assert!(!resp.is_error, "request frames have is_error=0"); - assert_eq!(resp.message_id, 15, "GetSta message_id must round-trip"); + assert_eq!(resp.message_id, BATTERY_GETSTA_MSG_ID, "message_id must round-trip"); assert_eq!( resp.body.last().copied(), Some(battery_id), - "battery_id selector must appear at end of body" + "payload byte must appear at end of body" ); } } #[test] fn encode_short_buf_errors() { - // Pin the BufTooSmall boundary at MIN_OUTPUT=18 — 17 bytes too small, - // 18 bytes sufficient. + // Pin the BufTooSmall boundary for a 1-byte payload at 18 bytes + // (FRAME_OVERHEAD=13 + 1 payload byte → 14? no — the header echo + // adds 4 bytes inside the ODP body, so total = 13 + 4 + 1 = 18). let mut too_small = [0u8; 17]; - let result = encode_battery_request(&mut too_small, 0); + let result = encode_request(&mut too_small, BATTERY_SVC_ID, BATTERY_GETSTA_MSG_ID, &[0]); assert!( matches!(result, Err(FramerError::BufTooSmall)), "expected BufTooSmall at 17 bytes, got {result:?}" ); let mut just_enough = [0u8; 18]; - let result = encode_battery_request(&mut just_enough, 0); + let result = encode_request(&mut just_enough, BATTERY_SVC_ID, BATTERY_GETSTA_MSG_ID, &[0]); assert!( result.is_ok(), "expected Ok at 18 bytes, got {result:?}" ); } + #[test] + fn encode_oversize_payload_errors_as_buf_too_small() { + // Payload exceeds MAX_PAYLOAD_LEN — must surface as BufTooSmall + // (configuration error) not EncodeFailed (mctp-rs rejection). + let mut buf = [0u8; 256]; + let oversize = [0u8; super::MAX_PAYLOAD_LEN + 1]; + let result = encode_request(&mut buf, 0x10, 0, &oversize); + assert!( + matches!(result, Err(FramerError::BufTooSmall)), + "expected BufTooSmall for oversized payload, got {result:?}" + ); + } + #[test] fn decode_truncated_header_errors() { - let result = decode_battery_response(&[0x00, 0x0f]); // truncated SmbusEspi header + let result = decode_response(&[0x00, 0x0f]); // truncated SmbusEspi header assert!( matches!( result, @@ -534,7 +584,7 @@ mod tests { // Exercises the second truncation guard (`buf.len() < needed`), // distinct from the 4-byte header guard above. let frame = [0x00u8, 0x0f, 0x09, 0x03, 0x01, 0x80]; - let result = decode_battery_response(&frame); + let result = decode_response(&frame); assert_eq!( result, Err(FramerError::Truncated), @@ -545,15 +595,11 @@ mod tests { #[test] fn decode_oversize_byte_count_errors_as_buf_too_small() { // byte_count=200 → needed=204, exceeding the internal 64-byte - // padded scratch. F-3 / review-correctness regression test — this - // path used to return DecodeFailed. - let mut frame = [0u8; 13]; - frame[2] = 200; - // pad needed bytes with zeros so the truncation guard doesn't fire - // first: we need buf.len() >= needed, so build a 204-byte buffer. + // padded scratch. Regression test for the oversize-claimed-length + // path — must surface as BufTooSmall (not DecodeFailed). let mut big = [0u8; 204]; big[2] = 200; - let result = decode_battery_response(&big); + let result = decode_response(&big); assert_eq!( result, Err(FramerError::BufTooSmall),