Wire accepted RequestUpdate to the orchestrator's UpdateRequest event The PLDM firmware-device loop now notifies an UpdateEventSink once per accepted RequestUpdate, detected as the FD's only Idle -> non-Idle transition, after the success response is sent. Rejected requests (already in update mode, bad transfer size) leave the state unchanged and never notify. The sink trait stays PLDM-flavored so this crate never depends on the orchestrator stack. The mapping to Event::UpdateRequest lives in the new orchestrator-pldm-adapter crate as UpdateRequestLatch, following the same rule that keeps HAL adapters out of orchestrator-capabilities. The latch is a bool, not a counter: the FD rejects a second RequestUpdate while one is in progress, and an undrained latch across update cycles coalesces into the single UpdateRequest the state machine would act on anyway. The firmware-update host test drives the latch end to end: the accepted RequestUpdate latches exactly one Event::UpdateRequest, the duplicate is rejected with AlreadyInUpdateMode and latches nothing, and no later command in the flow latches anything. Signed-off-by: Christina Quast <christina.quast@9elements.com>
diff --git a/services/orchestrator/pldm-adapter/BUILD.bazel b/services/orchestrator/pldm-adapter/BUILD.bazel new file mode 100644 index 0000000..ce7c90b --- /dev/null +++ b/services/orchestrator/pldm-adapter/BUILD.bazel
@@ -0,0 +1,24 @@ +# Licensed under the Apache-2.0 license +# SPDX-License-Identifier: Apache-2.0 + +load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") + +rust_library( + name = "orchestrator_pldm_adapter", + srcs = [ + "src/lib.rs", + ], + crate_name = "openprot_orchestrator_pldm_adapter", + edition = "2024", + visibility = ["//visibility:public"], + deps = [ + "//services/orchestrator/sm:orchestrator_sm", + "//services/pldm:pldm_service", + ], +) + +# Host tests: build on the host platform, no kernel/QEMU. +rust_test( + name = "orchestrator_pldm_adapter_test", + crate = ":orchestrator_pldm_adapter", +)
diff --git a/services/orchestrator/pldm-adapter/src/lib.rs b/services/orchestrator/pldm-adapter/src/lib.rs new file mode 100644 index 0000000..bc9bd05 --- /dev/null +++ b/services/orchestrator/pldm-adapter/src/lib.rs
@@ -0,0 +1,84 @@ +// Licensed under the Apache-2.0 license +// SPDX-License-Identifier: Apache-2.0 + +//! PLDM-backed adapter for the Boot Orchestrator's update-request input. +//! +//! [`UpdateRequestLatch`] binds the PLDM firmware-device service's +//! [`UpdateEventSink`] seam to the orchestrator's +//! [`Event::UpdateRequest`]: the PLDM run loop notifies the latch when the +//! Update Agent's `RequestUpdate` is accepted, and the orchestrator run loop +//! drains it with [`take`](UpdateRequestLatch::take). This crate depends on +//! both stacks by design — the PLDM service stays orchestrator-free and the +//! orchestrator stays transport-free, the same rule that keeps HAL adapters +//! out of `orchestrator-capabilities`. + +#![cfg_attr(not(test), no_std)] +#![forbid(unsafe_code)] +#![warn(missing_docs)] + +use openprot_orchestrator_sm::Event; +use openprot_pldm_service::firmware_device::UpdateEventSink; + +/// Latches an accepted PLDM `RequestUpdate` until the orchestrator run loop +/// drains it as [`Event::UpdateRequest`]. +/// +/// A `bool` latch, not a counter: the FD rejects a second `RequestUpdate` +/// while an update is in progress (`ALREADY_IN_UPDATE_MODE`), so at most one +/// accepted request can be outstanding per update cycle. Should a completed +/// or cancelled cycle admit a new `RequestUpdate` before the previous latch +/// is drained, the two coalesce into one [`Event::UpdateRequest`] — which is +/// what the state machine would do anyway (an update already being handled +/// defers further requests). +#[derive(Default)] +pub struct UpdateRequestLatch { + pending: bool, +} + +impl UpdateRequestLatch { + /// A latch with nothing pending. + pub const fn new() -> Self { + Self { pending: false } + } + + /// Drain the latch: [`Event::UpdateRequest`] if a `RequestUpdate` was + /// accepted since the last call, else `None`. + pub fn take(&mut self) -> Option<Event> { + self.pending.then(|| { + self.pending = false; + Event::UpdateRequest + }) + } +} + +impl UpdateEventSink for UpdateRequestLatch { + fn update_requested(&mut self) { + self.pending = true; + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn empty_latch_yields_nothing() { + assert_eq!(UpdateRequestLatch::new().take(), None); + } + + #[test] + fn accepted_request_yields_one_event() { + let mut latch = UpdateRequestLatch::new(); + latch.update_requested(); + assert_eq!(latch.take(), Some(Event::UpdateRequest)); + assert_eq!(latch.take(), None, "a drained latch must not re-fire"); + } + + #[test] + fn undrained_notifications_coalesce() { + let mut latch = UpdateRequestLatch::new(); + latch.update_requested(); + latch.update_requested(); + assert_eq!(latch.take(), Some(Event::UpdateRequest)); + assert_eq!(latch.take(), None); + } +}
diff --git a/services/pldm/BUILD.bazel b/services/pldm/BUILD.bazel index 6f9ddbc..b0fb31c 100644 --- a/services/pldm/BUILD.bazel +++ b/services/pldm/BUILD.bazel
@@ -55,6 +55,8 @@ ":pldm_service", "//services/mctp/api:mctp_api", "//services/mctp/server:mctp_server_lib", + "//services/orchestrator/pldm-adapter:orchestrator_pldm_adapter", + "//services/orchestrator/sm:orchestrator_sm", "@rust_crates//:mctp", "@rust_crates//:mctp-lib", "@rust_crates//:pldm-common",
diff --git a/services/pldm/src/firmware_device.rs b/services/pldm/src/firmware_device.rs index 4246b3c..5605c10 100644 --- a/services/pldm/src/firmware_device.rs +++ b/services/pldm/src/firmware_device.rs
@@ -62,6 +62,29 @@ /// within this window is expected and is not treated as an error. const RESPONDER_POLL_TIMEOUT_MILLIS: u32 = 1; +/// Receiver for update-lifecycle notifications out of the PLDM FD state +/// machine. +/// +/// [`FirmwareDevice::run_terminus`] owns the PLDM state machine but has no +/// knowledge of the platform's update orchestration; this trait is the seam +/// between the two. It is deliberately PLDM-flavored (no orchestrator types) +/// so that depending on this crate never pulls in the orchestrator stack — +/// the mapping to an orchestrator event lives in an adapter crate, following +/// the same rule as the orchestrator's HAL adapters. +pub trait UpdateEventSink { + /// The Update Agent's `RequestUpdate` was accepted: the FD moved out of + /// `Idle` (into `LearnComponents`) and the success response has already + /// been sent. Called exactly once per accepted `RequestUpdate`; rejected + /// ones (`ALREADY_IN_UPDATE_MODE`, bad transfer size) never reach here + /// because they leave the FD state unchanged. + fn update_requested(&mut self); +} + +/// Drop update notifications, for callers with no orchestration to notify. +impl UpdateEventSink for () { + fn update_requested(&mut self) {} +} + /// Outcome of [`FirmwareDevice::run_terminus`]. pub enum RunTerminusResult { /// The loop exited normally (currently unreachable: `run_terminus` only @@ -151,6 +174,11 @@ /// responder path to stay live even during a stalled FD-initiated /// request should pass a bounded value instead. /// + /// `sink` receives [`UpdateEventSink::update_requested`] once per + /// accepted `RequestUpdate` (the FD's only `Idle` → non-`Idle` + /// transition), after the success response has been sent. Callers with + /// nothing to notify pass `&mut ()`. + /// /// [`should_start_initiator_mode`]: pldm_interface::firmware_device::fd_context::FirmwareDeviceContext pub fn run_terminus( &mut self, @@ -158,8 +186,15 @@ buf: &mut [u8], timeout_millis: u32, requester_timeout_millis: u32, + sink: &mut impl UpdateEventSink, ) -> RunTerminusResult { - match self.run_terminus_inner(remote_eid, buf, timeout_millis, requester_timeout_millis) { + match self.run_terminus_inner( + remote_eid, + buf, + timeout_millis, + requester_timeout_millis, + sink, + ) { Ok(()) => RunTerminusResult::Completed, Err(e) => RunTerminusResult::StoppedByError(e), } @@ -171,6 +206,7 @@ buf: &mut [u8], timeout_millis: u32, requester_timeout_millis: u32, + sink: &mut impl UpdateEventSink, ) -> Result<(), PldmServiceError> { let mut responder_listener = self .responder_transport @@ -220,6 +256,11 @@ timeout_millis }; responder_listener.set_timeout(poll_timeout); + // Sampled around the responder poll: `RequestUpdate` is the only + // command that takes the FD out of `Idle`, so the false→true edge + // of `is_update_mode()` identifies exactly one accepted + // `RequestUpdate` (the initiator phase above never leaves `Idle`). + let was_update_mode = self.cmd_interface.fd_ctx.is_update_mode(); match self.responder_transport.respond_once( &mut responder_listener, buf, @@ -234,7 +275,11 @@ .map_err(PldmServiceError::MsgHandler) }, ) { - Ok(()) => {} + Ok(()) => { + if !was_update_mode && self.cmd_interface.fd_ctx.is_update_mode() { + sink.update_requested(); + } + } // A short poll timeout while an initiator request is active // just means no UA command arrived in that window; keep // looping so the transfer can continue.
diff --git a/services/pldm/src/lib.rs b/services/pldm/src/lib.rs index bbc5fe4..fcb999a 100644 --- a/services/pldm/src/lib.rs +++ b/services/pldm/src/lib.rs
@@ -60,8 +60,10 @@ //! // `run_terminus` loops forever, interleaving inbound UA commands with any //! // FD-initiated requests (e.g. RequestFirmwareData) once an update begins. //! // It returns only on error; a `timeout_millis`/`requester_timeout_millis` -//! // of `0` blocks indefinitely while idle. -//! if let Err(e) = fd.run_terminus(UA_EID, &mut buf, 0, 0) { +//! // of `0` blocks indefinitely while idle. The final argument is an +//! // `UpdateEventSink` notified once per accepted RequestUpdate; `&mut ()` +//! // drops the notifications. +//! if let Err(e) = fd.run_terminus(UA_EID, &mut buf, 0, 0, &mut ()) { //! // handle or log error //! } //! ```
diff --git a/services/pldm/tests/base_host.rs b/services/pldm/tests/base_host.rs index e71c8b2..29e1fc9 100644 --- a/services/pldm/tests/base_host.rs +++ b/services/pldm/tests/base_host.rs
@@ -207,7 +207,7 @@ // which point it returns Mctp(TimedOut); that terminating timeout means // "done", not a failure. let mut run_fd_once = - || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS) { + || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS, &mut ()) { RunTerminusResult::Completed => {} RunTerminusResult::StoppedByError(PldmServiceError::Mctp(e)) if e.is_timeout() => {} RunTerminusResult::StoppedByError(e) => panic!("firmware device failed: {e:?}"),
diff --git a/services/pldm/tests/firmware_update_host.rs b/services/pldm/tests/firmware_update_host.rs index 0ebef4f..1e8d875 100644 --- a/services/pldm/tests/firmware_update_host.rs +++ b/services/pldm/tests/firmware_update_host.rs
@@ -26,6 +26,8 @@ use mctp_lib::Sender; use openprot_mctp_api::Handle; use openprot_mctp_server::Server; +use openprot_orchestrator_pldm_adapter::UpdateRequestLatch; +use openprot_orchestrator_sm::Event; use openprot_pldm_service::firmware_device::{FirmwareDevice, RunTerminusResult}; use openprot_pldm_service::{MctpPldmTransport, PldmServiceError}; use pldm_common::codec::{PldmCodec, PldmCodecWithLifetime}; @@ -51,7 +53,8 @@ }; use pldm_common::protocol::firmware_update::{ ComponentClassification, ComponentResponseCode, Descriptor, FirmwareDeviceState, FwUpdateCmd, - PldmFirmwareString, UpdateOptionFlags, VersionStringType, PLDM_FWUP_IMAGE_SET_VER_STR_MAX_LEN, + FwUpdateCompletionCode, PldmFirmwareString, UpdateOptionFlags, VersionStringType, + PLDM_FWUP_IMAGE_SET_VER_STR_MAX_LEN, }; use pldm_common::util::fw_component::FirmwareComponent; use pldm_interface::firmware_device::fd_ops::{ComponentOperation, FdOps, FdOpsError}; @@ -312,6 +315,10 @@ )); let fd_buf = RefCell::new([0u8; 1024]); + // Orchestrator-facing latch: `run_terminus` marks it on each accepted + // RequestUpdate; the assertions below drain it as `Event::UpdateRequest`. + let update_events = RefCell::new(UpdateRequestLatch::new()); + // Run one full UA->FD->UA command round-trip and return the PLDM response // payload (without the MCTP framing byte). let ua_transact = |req_pldm: &[u8]| -> Vec<u8> { @@ -332,6 +339,7 @@ &mut fd_buf.borrow_mut()[..], TIMEOUT_MILLIS, TIMEOUT_MILLIS, + &mut *update_events.borrow_mut(), ) { RunTerminusResult::Completed => {} RunTerminusResult::StoppedByError(PldmServiceError::Mctp(e)) if e.is_timeout() => {} @@ -371,6 +379,42 @@ resp[3], 0, "RequestUpdate completion code should be success" ); + assert_eq!( + update_events.borrow_mut().take(), + Some(Event::UpdateRequest), + "accepted RequestUpdate should latch exactly one orchestrator event" + ); + assert_eq!( + update_events.borrow_mut().take(), + None, + "the latch must not re-fire once drained" + ); + + // ---- Duplicate RequestUpdate: rejected, must not latch an event ---- + instance_id += 1; + let dup_update = RequestUpdateRequest::new( + instance_id, + PldmMsgType::Request, + IMAGE_SIZE, + 1, + 1, + 0, + &comp_ver, + ); + let len = dup_update + .encode(&mut buf) + .expect("encode duplicate RequestUpdate"); + let resp = ua_transact(&buf[..len]); + assert_eq!( + resp[3], + FwUpdateCompletionCode::AlreadyInUpdateMode as u8, + "second RequestUpdate should be rejected while in update mode" + ); + assert_eq!( + update_events.borrow_mut().take(), + None, + "a rejected RequestUpdate must not latch an orchestrator event" + ); // ---- PassComponentTable (Start+End): move to ReadyXfer ---- instance_id += 1; @@ -451,6 +495,11 @@ ); assert!(fd_ops.verified.get(), "firmware should have been verified"); assert!(fd_ops.applied.get(), "firmware should have been applied"); + assert_eq!( + update_events.borrow_mut().take(), + None, + "no command after the accepted RequestUpdate should latch an event" + ); println!( "Firmware update host test completed: downloaded {} bytes, verified={}, applied={}",
diff --git a/services/pldm/tests/unexpected_eid_fw_host.rs b/services/pldm/tests/unexpected_eid_fw_host.rs index fef0682..055b974 100644 --- a/services/pldm/tests/unexpected_eid_fw_host.rs +++ b/services/pldm/tests/unexpected_eid_fw_host.rs
@@ -228,7 +228,7 @@ // "done", not a failure. `UA_EID` is the only EID `run_terminus` is told // to serve, so commands from `ATTACKER_EID` must be ignored below. let mut run_fd_once = - || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS) { + || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS, &mut ()) { RunTerminusResult::Completed => {} RunTerminusResult::StoppedByError(PldmServiceError::Mctp(e)) if e.is_timeout() => {} RunTerminusResult::StoppedByError(e) => panic!("firmware device failed: {e:?}"),
diff --git a/services/pldm/tests/unexpected_eid_host.rs b/services/pldm/tests/unexpected_eid_host.rs index b3d0302..c074f25 100644 --- a/services/pldm/tests/unexpected_eid_host.rs +++ b/services/pldm/tests/unexpected_eid_host.rs
@@ -202,7 +202,7 @@ // "done", not a failure. `UA_EID` is the only EID `run_terminus` is told // to serve, so commands from `ATTACKER_EID` must be ignored below. let mut run_fd_once = - || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS) { + || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS, &mut ()) { RunTerminusResult::Completed => {} RunTerminusResult::StoppedByError(PldmServiceError::Mctp(e)) if e.is_timeout() => {} RunTerminusResult::StoppedByError(e) => panic!("firmware device failed: {e:?}"),