TBOR Enablement#396
Open
vsonims wants to merge 3 commits into
Open
Conversation
Two cleanups to the DdiDev trait surface that together remove ~140
net lines and a long-standing foot-gun in the test helpers.
1) Remove set_device_kind; make device_kind() a trait getter
The kind is a static property of each backend, not a runtime
toggle: DdiMockDev is always Virtual, DdiNixDev/DdiWinDev/DdiEmuDev
are always Physical. The setter just rediscovered at runtime what
the backend already knew at construction. Tests had to remember to
call set_device_kind(&mut dev) after every open_dev or the codec
would silently use the wrong wire-format mode.
- Drop `fn set_device_kind` from the DdiDev trait.
- Add `fn device_kind(&self) -> DdiDeviceKind` to the trait
(replaces the per-backend inherent methods, which returned
Option<DdiDeviceKind>).
- Each backend hardcodes its kind in `open()`; nix/win/emu fields
tightened from Option<DdiDeviceKind> to DdiDeviceKind.
- DdiResTestDev loses its `device_kind` field entirely; just
delegates to the inner backend.
- api/lib: open_dev no longer performs the GetApiRev+GetDeviceInfo
probe-then-set dance; HsmDev::device_kind() returns the value
directly (no more Option/InternalError plumbing). The internal
`fn get_device_kind` helper is removed (dead code).
- ddi/lib/tests/integration/common.rs: drop `pub fn
set_device_kind`; keep `open_dev_and_set_device_kind` as a thin
pass-through so existing call sites compile unchanged.
- Removed ~26 bare `set_device_kind(&mut dev)` lines across the
integration test suite; `cargo fix` cleaned up the resulting
now-unused `mut` bindings.
2) Rename simulate_nssr_after_lm -> erase
The original name described a test-only mechanism (NVMe Subsystem
Reset after Live Migration); the actual contract is "erase keys
and cryptographic state, return the device to a clean state."
That semantic is what every caller relies on — fault injection's
TriggerReset, test cleanup, partition reset, and the live-
migration simulation tests.
- DdiDev trait method renamed; docstring updated to describe the
contract rather than one implementation strategy.
- Per-backend doc comments refreshed: nix/win still note the NSSR
IOCTL as the implementation detail; mock and emu describe their
own mechanisms.
- All 65 call sites updated across ddi/lib tests, api/lib,
api/tests, tools/resiliency_stress, and per-backend
implementations.
Validation
- cargo xtask nextest --features mock --package azihsm_ddi
-> 556 passed, 0 failed
- cargo xtask clippy: clean
- cargo +nightly xtask fmt --fix: clean
- cargo xtask copyright --fix: clean
Contributor
There was a problem hiding this comment.
Pull request overview
This PR simplifies the DdiDev trait surface by making device kind a fixed per-backend property (removing the set_device_kind foot-gun) and renaming the live-migration reset hook to a more semantic erase() operation. It also updates backends and call sites (including integration tests and resiliency wrappers) to use the new APIs.
Changes:
- Replaced
DdiDev::set_device_kindwithDdiDev::device_kind() -> DdiDeviceKind, and hardcoded kind per backend. - Renamed
simulate_nssr_after_lmtoeraseacross backends, wrappers, and tests, updating docs accordingly. - Removed device-kind probing/set logic from
api/libdevice open path and adjusted partition reset to callerase().
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ddi/win/src/dev.rs | Makes Windows backend device kind fixed to Physical; implements device_kind() and renames reset hook to erase(). |
| ddi/nix/src/dev.rs | Makes Linux backend device kind fixed to Physical; implements device_kind() and renames reset hook to erase(). |
| ddi/mock/src/dev.rs | Removes setter-based kind plumbing; reports Virtual via device_kind(); renames reset hook to erase(). |
| ddi/emu/src/dev.rs | Makes emulator backend kind fixed to Physical; implements device_kind() and renames reset hook to erase(); updates unit test accordingly. |
| ddi/interface/src/lib.rs | Updates the DdiDev trait to expose device_kind() and erase() with revised documentation. |
| ddi/res_test_dev/src/fault.rs | Updates resiliency test device docs to reference erase instead of simulate_nssr_after_lm. |
| ddi/res_test_dev/src/dev.rs | Delegates device_kind() to inner dev and switches reset action to call erase(). |
| api/lib/src/ddi/dev.rs | Removes open-time GetDeviceInfo probe/set for device kind; HsmDev::device_kind() now returns a concrete DdiDeviceKind. |
| api/lib/src/partition.rs | Uses the new concrete device_kind() and switches partition reset to call erase(). |
| ddi/lib/tests/integration/common.rs | Removes set_device_kind helper; keeps open_dev_and_set_device_kind as a pass-through and switches cleanup reset to erase(). |
| ddi/lib/tests/integration/reopen_session.rs | Updates migration/reset calls from simulate_nssr_after_lm to erase(). |
| ddi/lib/tests/integration/prov_part.rs | Updates reset calls to erase() and removes set_device_kind usage. |
| ddi/lib/tests/integration/open_session.rs | Removes set_device_kind usage in threaded helper/dev opens. |
| ddi/lib/tests/integration/masked_key_get_unwrapping.rs | Updates migration/reset call to erase(). |
| ddi/lib/tests/integration/masked_key_ecc_gen.rs | Removes set_device_kind usage when opening session-only device handle. |
| ddi/lib/tests/integration/masked_key_aes_gen.rs | Removes set_device_kind usage in threaded helper/dev opens. |
| ddi/lib/tests/integration/live_migration_sim.rs | Replaces all migration/reset invocations with erase() and removes set_device_kind calls. |
| ddi/lib/tests/integration/live_migration_expected_errors.rs | Updates migration/reset calls to erase() and removes set_device_kind usage. |
| ddi/lib/tests/integration/get_unwrapping_key.rs | Removes set_device_kind usage in thread helper. |
| ddi/lib/tests/integration/get_session_encryption_key.rs | Removes set_device_kind usage in thread helper. |
| ddi/lib/tests/integration/get_establish_cred_encryption_key.rs | Removes set_device_kind usage in thread helper. |
| ddi/lib/tests/integration/get_cert_chain.rs | Removes set_device_kind usage in multithread tests and uses immutable dev where possible. |
| ddi/lib/tests/integration/flush_session.rs | Removes set_device_kind usage when opening additional device handles. |
| ddi/lib/tests/integration/establish_credential.rs | Removes set_device_kind usage in thread helper; updates reset call to erase(). |
| ddi/lib/tests/integration/ecc_sign_stress.rs | Removes set_device_kind usage in thread helper. |
| ddi/lib/tests/integration/ecc_generate.rs | Removes set_device_kind usage when opening session-only device handles. |
| ddi/lib/tests/integration/close_session.rs | Updates reset call to erase(); continues using open_dev_and_set_device_kind helper. |
| ddi/lib/tests/integration/change_pin.rs | Removes set_device_kind usage when opening additional device handles. |
| ddi/lib/tests/integration/aes_xts_encrypt_decrypt.rs | Removes set_device_kind calls before opening sessions. |
| ddi/lib/tests/integration/aes_xts_bulk_stress.rs | Removes set_device_kind usage in thread helper. |
| ddi/lib/tests/integration/aes_gcm_bulk_stress.rs | Removes set_device_kind usage in thread helper. |
Refactor the host-side DDI crates to mirror the firmware layout
(fw/core/ddi/{mbor,tbor}/{codec,types,derive}) and lay the groundwork
for routing DDI commands through either MBOR or TBOR per request.
Crate moves and renames
- ddi/serde/derive -> ddi/mbor/derive (azihsm_ddi_derive -> azihsm_ddi_mbor_derive)
- ddi/serde/mbor -> ddi/mbor/codec (azihsm_ddi_mbor -> azihsm_ddi_mbor_codec)
- ddi/serde/types -> ddi/mbor/types (azihsm_ddi_types -> azihsm_ddi_mbor_types)
- ddi/serde/ removed.
- Derive proc-macro emit paths updated from `azihsm_ddi_mbor::*`
to `azihsm_ddi_mbor_codec::*` so existing `#[derive(Ddi)]` users
compile unchanged.
- New ddi/tbor/{codec,derive,types} skeleton, re-exporting the
firmware TBOR codec/derive crates. `azihsm_ddi_tbor_types`
declares a `TborOpReq` trait stub; concrete TBOR request types
will land as DDI commands are migrated (first: GetApiRev).
Tests
- ddi/lib/tests/{azihsm_ddi_tests.rs, integration/} relocated to
ddi/mbor/types/tests/. `azihsm_ddi_mbor_types` gained
dev-dependencies and feature forwarding (`mock`, `emu`,
`table-4`, `table-64`) to host the integration test binary.
- ddi/lib slimmed down: no longer carries dev-deps or test target.
- Five MBOR unit-test files now opt out of clippy::unwrap_used
(and implicit_clone for der_tests) to match the lint policy that
the old `ddi/serde/types` crate had been silently bypassing.
DdiDev trait
- `DdiDev::exec_op` renamed to `exec_op_mbor`. ~80 call sites
updated across api/lib, ddi/sim, ddi/res_test_dev,
ddi/test_helpers, and tools/resiliency_stress.
- New `exec_op_tbor<T: TborOpReq>` method with a default
implementation that returns the new
`DdiError::UnsupportedEncoding`. Backends opt into TBOR by
overriding the default once OP_TBOR is wired through the SQE
layer.
- TBOR is intentionally a method on `DdiDev` (not a sibling
trait) so callers don't need an extra `use` and backends don't
need a separate impl block until they actually support TBOR.
CI / tooling
- xtask/src/precheck.rs, xtask/src/nextest_report.rs and
.github/workflows/rust.yml updated to target
`azihsm_ddi_mbor_types` (was `azihsm_ddi`) so the mock+table
mock test runs continue to execute the 580 integration +
unit tests.
- xtask/src/integration_tests.rs: provider-integration-tests-capi
block commented out (with a TODO note) while the
api_native/OpenSSL build coupling is being reworked. cli and
nginx provider integration tests continue to run.
Validation
- cargo build --workspace: clean
- cargo xtask nextest --features mock --package azihsm_ddi_mbor_types:
580 passed (was 556 integration + 24 unit; numbers preserved)
- cargo xtask nextest --package azihsm_ddi_tbor_types:
1 placeholder test passes
- cargo xtask clippy: clean
- cargo +nightly xtask fmt --fix: clean
- cargo xtask copyright --fix: clean
Comment on lines
+34
to
+38
| if !self.force { | ||
| log::warn!( | ||
| "skipping provider integration tests: temporarily disabled \ | ||
| while OpenSSL provider build coupling with libazihsm_api_native \ | ||
| is reworked. Pass --force to run them anyway." |
Comment on lines
+43
to
+47
| anyhow::bail!( | ||
| "provider integration tests cannot currently be run: \ | ||
| the OpenSSL provider / libazihsm_api_native build coupling is \ | ||
| being reworked. Drop `--force` to skip, or re-enable the \ | ||
| individual provider-integration-tests-* nextest invocations \ |
Comment on lines
+125
to
+129
| impl From<azihsm_ddi_tbor_codec::EncodeError> for DdiError { | ||
| #[inline] | ||
| fn from(_: azihsm_ddi_tbor_codec::EncodeError) -> Self { | ||
| Self::MborError(MborError::EncodeError) | ||
| } |
Comment on lines
+132
to
+136
| impl From<azihsm_ddi_tbor_codec::DecodeError> for DdiError { | ||
| #[inline] | ||
| fn from(_: azihsm_ddi_tbor_codec::DecodeError) -> Self { | ||
| Self::MborError(MborError::DecodeError) | ||
| } |
Comment on lines
+20
to
+24
| /// Pass `--force` to opt back in once the rework lands and the test | ||
| /// passes are runnable again. | ||
| #[derive(Parser)] | ||
| #[clap(about = "Run Integration Tests")] | ||
| pub struct IntegrationTest {} | ||
| #[clap(about = "Run Integration Tests (currently disabled)")] | ||
| pub struct IntegrationTest { |
This change introduces TBOR as a second on-the-wire DDI codec alongside
MBOR, wires it end-to-end through firmware and the emu host backend,
and implements `GetApiRev` as the first TBOR-encoded command.
* Add `OP_TBOR = 2` SQE opcode; rename `OP_GENERIC` -> `OP_MBOR` for
symmetry. SQE validation helper renamed from `validate_generic_op`
to `validate_io_op`.
* `Hsm::handle_io` gains a third dispatch arm. `handle_generic_op`
splits into `handle_mbor_op` (existing behavior, unchanged) and
`handle_tbor_op` (new). Both share the same DMA-in / dispatch /
DMA-out shape. TBOR currently requires `SessionCtrl::NoSession`
(session-bearing TBOR commands will land separately).
* Restructure `fw/core/lib/src/ddi/` into `mbor/` + `tbor/` submodules.
All existing MBOR handlers move under `mbor/` unchanged (git mv);
`tbor/` is new and contains:
- `mod.rs` -- `tbor::dispatch(opcode, view, out)` matches on
single-byte opcodes; `encode_tbor_err` emits a status-bearing
error response.
- `get_api_rev.rs` -- decodes the shared schema request, encodes
`min/max_protocol_version = 1`.
* `fw/core/ddi/tbor/codec` (`azihsm_fw_ddi_tbor`) gains a dep on
`azihsm_fw_hsm_pal_traits` plus `impl From<{Encode,Decode}Error> for
HsmError`. Handlers and dispatch can now use `?` directly without
per-call `.map_err(...)` boilerplate.
* `fw/core/ddi/tbor/types` (`azihsm_fw_ddi_tbor_types`) becomes the
single source of truth for TBOR wire schemas. Adds the
`TborGetApiRevReq` / `TborGetApiRevResp` schema using the `#[tbor]`
derive; the request is a unit struct (body is empty).
* `#[tbor]` derive extended to support empty schemas. Unit structs
and `struct Foo {}` are now accepted; the generated encoder writes
a synthetic `None` TOC placeholder (the codec requires
`toc_count >= 1`) and the generated validator enforces that
placeholder on decode. Existing non-empty schemas are unaffected.
* `azihsm_ddi_tbor_types` adds new traits `TborOpReq` and `TborResp`
with owned-value `encode_request` / `decode_response` methods. This
lets `DdiDev::exec_op_tbor<T: TborOpReq>(...) -> DdiResult<T::OpResp>`
return decoded responses without lifetime gymnastics around the IO
scratch buffer.
* `TborGetApiRevReq` / `TborGetApiRevResp` host wrappers delegate to
the firmware schema's `encode` / `decode`, keeping host and firmware
speaking byte-identical wire content.
* `DdiError` gains `From<MborError>`, `From<TborEncodeError>`, and
`From<TborDecodeError>` impls. Backend `exec_op_tbor` overrides can
now use `?` directly throughout.
* Emu backend (`ddi/emu`) gains an `exec_op_tbor` override that
encodes the TBOR request, builds an SQE with `OP_TBOR`, submits via
the embedded `StdHsm` runtime, and decodes the typed response. The
existing `exec_op_mbor` now sets `OP_MBOR` explicitly (was relying
on the default opcode of 0).
* Other backends (mock, nix, win, res_test_dev) keep the trait's
default `exec_op_tbor` -> `DdiError::UnsupportedEncoding`.
Documented contract: backends opt in to TBOR.
* `DdiDev::erase` doc updated to match actual sim behavior (NSSR
preserves sealed BK3 + bk3_initialized in
`FunctionStateInner::reset`). The previous wording incorrectly
claimed sealed BK3 was always discarded.
* New crate `azihsm_ddi_tbor_test_helpers` mirrors
`azihsm_ddi_mbor_test_helpers`. First helper:
`helper_get_api_rev_tbor(dev)`.
* `ddi/tbor/types/tests/integration/get_api_rev.rs`:
- `round_trip_emu` (`--features emu`): full host -> emu -> fw
`handle_tbor_op` -> response decode round-trip, asserts
`min = max = 1`.
- `unsupported_on_mock` (`--features mock`): asserts mock returns
`DdiError::UnsupportedEncoding` (negative contract test).
Both gated under `#![cfg(any(feature = "emu", feature = "mock"))]`.
* fw `#[tbor]` derive compile_tests: `empty_struct.rs` moves from
`fail/` to `pass/` and now does a real encode -> decode round-trip
on the empty schema. `tuple_struct.stderr` updated to the new error
message wording.
* `xtask precheck --nextest` runs `azihsm_ddi_tbor_types` against the
emu backend under the new `ci-tbor-emu` nextest profile. Wired into
`.github/workflows/rust.yml`.
* Drop dead helper `open_dev_and_set_device_kind` (was a no-op
wrapper after the earlier `set_device_kind` removal); 8 call sites
inlined to `ddi.open_dev(path).unwrap()`.
* 580/580 azihsm_ddi_mbor_types tests pass under `--features mock`
(no regression from the OP_GENERIC -> OP_MBOR rename or the fw
ddi/ restructure).
* 2/2 azihsm_ddi_tbor_types tests pass under `--features emu`.
* 2/2 azihsm_ddi_tbor_types tests pass under `--features mock`
(incl. negative test).
* fw_ddi_tbor + fw_ddi_tbor_api unit + trybuild tests pass.
* clippy, fmt, copyright clean for both `--features emu` and
`--features mock`.
Comment on lines
+223
to
+229
| features: Some("emu".to_string()), | ||
| package: Some("azihsm_ddi_tbor_types".to_string()), | ||
| no_default_features: false, | ||
| filterset: None, | ||
| profile: self.profile.or(Some("ci-tbor-emu".to_string())), | ||
| exclude: self.exclude.clone(), | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.