feat: FwMgmt, Notify, and TPM service implementation + tests by dymk · Pull Request #58 · OpenDevicePartnership/odp-secure-services

dymk · 2026-04-15T18:07:45Z

Summary

Implement FwMgmt error handling, Notify state-tracking (add/remove/assign/unassign),
and TPM extended handlers (get_feature_info, register, unregister, finish, deinit).
Adds comprehensive unit tests for all three services plus MessageHandler dispatch
tests. Fixes panic on invalid MessageID and shift overflow in notify service.

Changes

FwMgmt: error handling + process_indirect NotSupported
Notify: Add/Remove/Assign/Unassign state-tracking
TPM: get_feature_info, register, unregister, finish + deinit
Add FwMgmt and Notify unit test modules
Add MessageHandler dispatch unit tests
Add TPM extended handler tests + cargo fmt
Fix panic on invalid MessageID and shift overflow in notify
Add TPM2_Startup test helper for E2E command execution coverage

Files changed

ec-service-lib/src/services/fw_mgmt.rs
ec-service-lib/src/services/notify.rs
ec-service-lib/src/services/tpm.rs
ec-service-lib/src/message_handler.rs

Copilot

Pull request overview

Implements extended behaviors and unit tests for the FwMgmt, Notify, TPM services, plus MessageHandler dispatch tests, and tightens dev workflow checks.

Changes:

Add TPM extended handlers (feature info + notification register/unregister/finish) and deinit state reset, with expanded tests and a TPM2_Startup CRB test helper.
Implement Notify Add/Remove/Assign/Unassign state tracking, safer message-id parsing, shift-safe bitmap handling, and add Notify unit tests.
Improve FwMgmt error propagation (remove unwraps, make indirect unsupported) and add FwMgmt unit tests; add MessageHandler routing tests and strengthen pre-commit checks.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`ec-service-lib/src/services/tpm.rs`	Adds TPM notification registration state, extended handlers, deinit reset, and expanded unit tests/helpers.
`ec-service-lib/src/services/notify.rs`	Adds Add/Remove/Assign/Unassign handling, safer message-id parsing, bitmap guard helper, and extensive unit tests.
`ec-service-lib/src/services/fw_mgmt.rs`	Converts several handlers to return `Result`, propagates FF-A errors, marks indirect unsupported, and adds unit tests.
`ec-service-lib/src/message_handler.rs`	Adds unit tests validating UUID-based routing and unknown-UUID error behavior.
`.husky/pre-commit`	Enforces clippy warnings as errors and runs tests on the host target explicitly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cargo test without --target fails locally when rust-toolchain.toml installs aarch64-unknown-none-softfloat, because no_std-incompatible transitive deps (futures-timer, slab) are resolved for that target. Using the host target from rustc -vV ensures portability across Linux, macOS, and Windows.

- process_indirect returns Err("process_indirect not supported") instead of hardcoded 0x0 - map_share uses ? operator on MemRetrieveReq::exec() instead of .unwrap() - test_notify uses ? operator on NotificationSet::exec() instead of .unwrap() - Dispatch match arms updated to use ? propagation - Removed old commented-out FfaNotify code block

- nfy_add: find-or-create entry, register mappings (mirrors nfy_setup logic) - nfy_remove: find entry, unregister mappings (mirrors nfy_destroy logic) - nfy_assign: update id/ntype on existing cookie-matched mappings with bitmap conflict check - nfy_unassign: clear id/ntype on cookie-matched mappings, keep slot reserved - Replace catch-all NotSupported arm with four individual dispatch arms

- Add notification_registered field to TpmService struct - get_feature_info_handler returns capability flags for notification feature - register_handler tracks notification registration state (Already on double-register) - unregister_handler validates registration (Denied if not registered) - finish_handler validates registration (Denied if not registered) - deinit resets all state (current_state, active_locality, locality_states, notification_registered) - init also resets notification_registered - Update 3 test assertions: register→Ok, unregister→Denied, finish→Denied - Apply cargo fmt fixup for fw_mgmt.rs

- FwMgmt: 5 tests covering get_fw_state, get_svc_list, get_bid, process_indirect error, unknown command - Notify: 12 tests covering Setup, Destroy, Add, Remove, Assign, Unassign + error cases - UUID round-trip via to_u128_le() for correct NotifyReq deserialization

- 6 tests: UUID routing to MockServiceA/B, unknown UUID error, empty handler, chain ordering - Mock services use echo pattern with distinct markers (0xAA, 0xBB) - Verifies LIFO linked-list dispatch and Error::Other("Unknown UUID") terminal

- 9 new TPM tests: get_feature_info (notification/unknown), register/unregister state transitions, double register -> Already, finish with/without registration, deinit state reset, register after deinit - cargo fmt fixups for all test modules (alignment/line-length) - Total: 94 tests passing (62 existing + 32 new), zero regressions

CR-01: MessageInfo::message_id() now returns Option<MessageID> instead of panicking via .expect(). Invalid bit patterns (6, 7) return an error response rather than crashing the secure partition. CR-02: All 1<<id bitmap operations now use nfy_bitmask() helper that returns None for id >= 64, preventing shift overflow panics in debug and silent bitmap corruption in release builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…erage Add test_write_crb operation 5 that prepares a TPM2_Startup(CLEAR) command in the CRB data buffer with correct command_size. This enables E2E tests to exercise the full sst.start() pipeline (copy_command_data, start_command, copy_response_data) through the emulated TPM CRB interface. Only available when test-bypass-locality-check feature is enabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nfy_assign: pre-validate all tuples (cookie exists, id in range, no bitmap conflicts) using a simulated bitmap before applying any mutations. On error, no state is modified. nfy_unassign: pre-validate all cookies exist before clearing any mappings. Bitmap is updated atomically after all mutations. test_setup_overflow_count: NotifyReq::from() clamps count with .min(7), so sending count=8 never reached the >= 8 guard. Replace with two tests: one verifying clamped count=8 succeeds with 7 valid tuples, another exercising count=7 directly as the effective maximum. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The merge-base changed after approval.

kurtjd

Is just doing a git rebase causing the approvals to be dismissed? If so, that might be an issue with this repo's settings, our other repos don't seem to have that problem.

The merge-base changed after approval.

philgweber · 2026-06-15T15:52:35Z

+    use odp_ffa::{DirectMessagePayload, HasRegisterPayload};
+    use uuid::uuid;
+
+    const NOTIFY_UUID: Uuid = uuid!("e474d87e-5731-4044-a727-cb3e8cf3c8df");


Isn't this defined somewhere externally as this is defined in the FFA standard we can maybe move it here.

philgweber · 2026-06-15T15:53:58Z

@@ -236,6 +237,7 @@ impl<S: TpmSstOps> TpmService<S> {
            interface_id_default: PtpCrbInterfaceIdentifier::new(),
            locality_states: [TpmLocalityState::Closed; NUM_LOCALITIES as usize],
            tpm_internal_crb_address: 0x10000200000,


This will probably conflict with the other change the TPM address has changed.

The merge-base changed after approval.

dymk force-pushed the dymk/fwmgmt-notify-tpm-impl branch 4 times, most recently from 99657fb to 4352dac Compare April 15, 2026 18:41

dymk marked this pull request as ready for review April 15, 2026 19:01

dymk requested a review from a team as a code owner April 15, 2026 19:01

dymk requested review from kat-perez, maxperga-msft, philgweber and rogurr April 15, 2026 19:01

This was referenced Apr 15, 2026

feat: fwmgmt and notify E2E test binaries OpenDevicePartnership/odp-platform-qemu-sbsa#62

Closed

feat: battery platform stub + TPM E2E test updates OpenDevicePartnership/odp-platform-qemu-sbsa#64

Closed

dymk requested a review from Copilot April 15, 2026 19:11

Copilot started reviewing on behalf of dymk April 15, 2026 19:12 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Comment thread ec-service-lib/src/services/notify.rs Outdated

Comment thread ec-service-lib/src/services/notify.rs

Comment thread ec-service-lib/src/services/notify.rs Outdated

Comment thread ec-service-lib/src/services/notify.rs Outdated

Comment thread ec-service-lib/src/services/fw_mgmt.rs

dymk and others added 10 commits April 15, 2026 12:27

dymk force-pushed the dymk/fwmgmt-notify-tpm-impl branch from 4352dac to fff1bd2 Compare April 15, 2026 19:34

philgweber previously approved these changes Apr 15, 2026

View reviewed changes

kurtjd previously approved these changes Apr 15, 2026

View reviewed changes

philgweber previously approved these changes Apr 15, 2026

View reviewed changes

jerrysxie added this to ODP Stack for QEMU SBSA (Arm64) Apr 15, 2026

github-project-automation Bot moved this to In progress in ODP Stack for QEMU SBSA (Arm64) Apr 15, 2026

jerrysxie assigned dymk Apr 15, 2026

kurtjd previously approved these changes Apr 15, 2026

View reviewed changes

philgweber previously approved these changes Apr 16, 2026

View reviewed changes

kurtjd previously approved these changes Apr 16, 2026

View reviewed changes

This was referenced Apr 16, 2026

Complete EC service stub implementations + unit tests #60

Open

Implement FwMgmt service error handling #63

Open

Implement notify service state-tracking #64

Open

Implement TPM extended handlers #65

Open

rogurr previously approved these changes Apr 17, 2026

View reviewed changes

philgweber reviewed Jun 15, 2026

View reviewed changes

philgweber previously approved these changes Jun 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: FwMgmt, Notify, and TPM service implementation + tests#58

feat: FwMgmt, Notify, and TPM service implementation + tests#58
dymk wants to merge 10 commits into
OpenDevicePartnership:mainfrom
dymk:dymk/fwmgmt-notify-tpm-impl

dymk commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kurtjd left a comment

Uh oh!

philgweber Jun 15, 2026

Uh oh!

philgweber Jun 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

dymk commented Apr 15, 2026

Summary

Changes

Files changed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kurtjd left a comment

Choose a reason for hiding this comment

Uh oh!

philgweber Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

philgweber Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants