Merge hw tests with existing emu - #592
Conversation
There was a problem hiding this comment.
Copilot encountered an error: Your billing is not configured or you have Copilot licenses from multiple standalone organizations or enterprises. To use premium requests, select a billing entity via the GitHub site, under Settings > Copilot > Features.
|
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Copilot encountered an error: Your billing is not configured or you have Copilot licenses from multiple standalone organizations or enterprises. To use premium requests, select a billing entity via the GitHub site, under Settings > Copilot > Features.
There was a problem hiding this comment.
Copilot encountered an error: Your billing is not configured or you have Copilot licenses from multiple standalone organizations or enterprises. To use premium requests, select a billing entity via the GitHub site, under Settings > Copilot > Features.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Copilot encountered an error: Your billing is not configured or you have Copilot licenses from multiple standalone organizations or enterprises. To use premium requests, select a billing entity via the GitHub site, under Settings > Copilot > Features.
There was a problem hiding this comment.
Copilot encountered an error: Your billing is not configured or you have Copilot licenses from multiple standalone organizations or enterprises. To use premium requests, select a billing entity via the GitHub site, under Settings > Copilot > Features.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
ddi/tbor/types/tests/harness/mod.rs:36
- This module doc says in-session tests are gated with
cfg(not(any(feature = "mock", feature = "sock"))), but the updated command tests (e.g.open_session.rs,psk_change.rs) are no longer gated off undersockand will run there. The documentation should match the actual cfg regime (likely only excludingmock).
//! on-silicon test runs; the in-session command tests run under
//! this backend too (they are gated
//! `#![cfg(not(any(feature = "mock", feature = "sock")))]`, which
//! admits both `emu` and the no-feature native OS build). The
ddi/tbor/types/tests/commands/open_session.rs:705
- This diagnostic message still references the old test name (
open_session_multi_threaded_all_should_open). If the test is renamed to reflect its real assertions, the log prefix should be updated too to avoid confusion when reading CI output.
eprintln!(
"open_session_multi_threaded_all_should_open: FW accepted all {MULTI_THREADED_TOTAL} \
concurrent sessions without emitting any table-full rejection; race between success \
and rejection branches not exercised at this thread count",
);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
ddi/tbor/types/tests/harness/fixture.rs:98
- The
open_devdoc comment claimssockbuilds gate the harness out at the crate root, buttests/azihsm_ddi_tbor_tests.rsincludesharnessfor allnot(feature = "mock")builds (sosockincludes it). This comment is now misleading.
/// Acquire the test lock, open the configured backend device, and
/// factory-reset it before use. Only reachable under `--features emu`
/// or the no-feature (hw) build — mock/sock builds gate the whole
/// harness out at the crate root.
ddi/tbor/types/tests/harness/ctx.rs:114
- This doc comment says the harness is gated out for
sock, but the test crate includesharnessandcommandsfor anynot(feature = "mock")build, including--features sock. Update the comment so it matches the actual cfg behavior.
/// Factory-reset the partition. Used by the
/// `commands::part_init` determinism test for cold-restart
/// iterations. Available on every backend the harness compiles
/// under (emu + hw); `mock`/`sock` gate the whole harness out at
/// the crate root.
| // Recovery: drop every guard (each guard's `Drop` sends an | ||
| // explicit SessionClose), then drop the extra fds. A fresh open | ||
| // on the primary ctx must succeed. | ||
| drop(guards); |
There was a problem hiding this comment.
dropping just one should be sufficient.
| matches!(err, azihsm_ddi_interface::DdiError::TborStatus(_)), | ||
| "expected FW-side TborStatus rejection, got {err:?}", | ||
| ); | ||
| drop(ctx_second); |
There was a problem hiding this comment.
shouldnt need to drop second.
| handles.push(s.spawn( | ||
| || -> Result<(TestCtx, u16), azihsm_ddi_interface::DdiError> { | ||
| let ctx_extra = TestCtx::new_with_path(path); | ||
| let handshake = ctx_extra.open_session_raw(CU, SessionType::PlainText)?; |
There was a problem hiding this comment.
why not open_session instead of raw?
| // Drop winners' ctxs before asserting so a failing assert never | ||
| // leaves the session table dirty — each ctx's fd close reclaims | ||
| // its slot in FW. | ||
| drop(winners); |
There was a problem hiding this comment.
Is this true? Because if you open session properly then it should clean up.
| /// [`TestCtx`] fd — hw requires one fd per concurrent session. | ||
| #[cfg(not(feature = "emu"))] | ||
| #[test] | ||
| fn open_session_multi_threaded_single_winner() { |
There was a problem hiding this comment.
This is not a single winner test, I had left that comment earlier as well, but cannot find it here.
| /// | ||
| /// Caller must ensure the primary [`TestDev`] outlives every | ||
| /// secondary, otherwise the lock guard drops mid-test. | ||
| pub(crate) fn open_dev_secondary(path: &str) -> TestDev { |
There was a problem hiding this comment.
Is this still used or needed?
There was a problem hiding this comment.
Yes this is still used. This is used during the new_with_path. I limited to the (crate) so that outside wont see it.
| mac_fin: [u8; 48], | ||
| ) -> DdiResult<SessionHandshake> { | ||
| session_open_finish_with_mac_helper(&self.dev, pending, mac_fin) | ||
| session_open_finish_with_mac(&self.dev, pending, mac_fin) |
There was a problem hiding this comment.
I am not sure of the reason of the rename but the new names are not consistent. For example, you have session_open_finish_on_dev and session_open_finish_with_mac.
Old names were session_open_finish_helper and session_open_finish_with_mac_helper. Werent the older names clearer?
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[test] | ||
| fn open_session_fills_co_slot_then_recovers() { |
There was a problem hiding this comment.
I think we should have a test to exahust both CO + CU limit in single test to make sure whole table is full.
There was a problem hiding this comment.
Not sure if we have both CO exahustion and CU exahustion, why we need to have another test to run them together. The exahustion is based on the role.
There was a problem hiding this comment.
To make sure there is no such bug.
| .open_session(CU, SessionType::PlainText) | ||
| .expect_err("open_session past CU limit must be rejected"); | ||
| assert!( | ||
| matches!(err, azihsm_ddi_interface::DdiError::TborStatus(_)), |
There was a problem hiding this comment.
Checking for blanket error could mask a different issue. Can we check for specific error like SessionLimitReached?
| .open_session(CO, SessionType::Authenticated) | ||
| .expect_err("second concurrent CO open must be rejected"); | ||
| assert!( | ||
| matches!(err, azihsm_ddi_interface::DdiError::TborStatus(_)), |
There was a problem hiding this comment.
Checking for blanket error could mask a different issue. Can we check for specific error like SessionLimitReached?
| ); | ||
| for err in &rejections { | ||
| assert!( | ||
| matches!(err, azihsm_ddi_interface::DdiError::TborStatus(_)), |
There was a problem hiding this comment.
Checking for blanket error could mask a different issue. Can we check for specific error like SessionLimitReached?
| .open_session(CU, SessionType::PlainText) | ||
| .expect_err("second open on the same fd must be rejected"); | ||
| assert!( | ||
| matches!(err, azihsm_ddi_interface::DdiError::TborStatus(_)), |
There was a problem hiding this comment.
Checking for blanket error could mask a different issue. Can we check for specific error like SessionLimitReached?
| ctx.psk_change(bootstrap.handshake(), &ROTATED_CO_PSK) | ||
| .expect("rotate CO PSK"); | ||
| let _ = ctx.session_close(bootstrap.session_id); | ||
| let _ = bootstrap.close(); |
There was a problem hiding this comment.
ignores error? and we could use bootstrap_rotated_co
| /// Named with an `_on_dev` suffix to disambiguate from the | ||
| /// [`TestCtx::session_close`](crate::harness::TestCtx::session_close) | ||
| /// method which acts on the ctx's implicit fd. | ||
| pub fn session_close_on_dev( |
There was a problem hiding this comment.
I see that session_close_on_dev called from ctx::session_close only, we should be using pub(crate)
| pub use session::psk_change; | ||
| pub use session::session_close; | ||
| pub use session::session_open_finish; | ||
| pub use session::session_close_on_dev; |
| /// [`TestCtx::open_session`](crate::harness::TestCtx::open_session) | ||
| /// method which acts on the ctx's implicit fd and returns a | ||
| /// [`SessionGuard`](crate::harness::SessionGuard). | ||
| pub fn open_session_on_dev( |
There was a problem hiding this comment.
Not consistent, as session open is here and session close is part of session/session_close. Please move both open and close to single mod under session.
There was a problem hiding this comment.
also lot of unused re-exports in the file.
| // in-tree Rust FW that emu runs. Emu accepts the rotation. | ||
| // =========================================================================== | ||
|
|
||
| #[cfg(not(feature = "emu"))] |
There was a problem hiding this comment.
please open a task to track changes on emu side. We should remove this emu specific gate.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
ddi/tbor/types/tests/harness/fixture.rs:111
open_dev()now unconditionally callsdev.erase(), but the socket backend implementsDdiDev::eraseasUnsupportedCmd(seeddi/sock/src/dev.rs:301-303). This makes--features sockruns panic during fixture setup, before any tests execute. Consider skipping the erase step for the sock backend (or implementing erase support in the socket transport) so sock tests can run.
let path = info.path.clone();
let dev = ddi.open_dev(&path).expect("open test backend device");
dev.erase()
.expect("open_dev: factory-reset backend before test");
ddi/tbor/types/tests/harness/fixture.rs:86
unsafe impl Send for TestDevrelies on a non-enforced invariant ("only secondaries are sent across threads"). If a future test accidentally moves a primaryTestDev(with_guard: Some(...)) to another thread, dropping it there may violate the mutex’s thread-affinity requirements and turn into UB. A safer pattern is to make the primary/secondary distinction explicit in the type system (e.g., separatePrimaryTestDev/SecondaryTestDevtypes, or an enum where only the secondary variant isSend) so the compiler enforces the invariant.
#[allow(unsafe_code)]
unsafe impl Send for TestDev {}
ddi/tbor/types/tests/commands/part_init/happy_path.rs:317
- Same issue as
part_init_determinism: this test callsctx.erase()and will fail under--features sockbecause the socket backend does not supportDdiDev::erase(UnsupportedCmd). Gate it off for sock (or implement erase in the socket transport).
#[test]
fn part_init_pta_pub_differs_when_include_fmc_cdi_flag_toggled() {
ddi/tbor/types/Cargo.toml:39
- With the
hw-testsopt-in feature removed and no default feature set,cargo test -p azihsm_ddi_tbor_typeswill now select the native OS backend (real hardware) by default. Combined with the updated fixture doing a factory reset, this can cause surprising/destructive behavior on developer machines and will panic on hosts without a device. Consider restoring an explicit opt-in for hardware runs (or setting an explicit default test backend) so the defaultcargo testpath is non-destructive and does not require silicon.
[features]
# Forward backend selection to the azihsm_ddi facade so the integration
# test binary picks up the right backend. The same tests run across every
# transport — choose one with `--features emu` / `mock` / `sock`.
emu = ["azihsm_ddi/emu"]
| #[test] | ||
| fn part_init_determinism_emu() { | ||
| fn part_init_determinism() { |
There was a problem hiding this comment.
dont worry about sock, do erase as needed, Wenbo.
| /// would break long-term binding assumed by higher-layer protocols. | ||
| #[test] | ||
| fn partition_pk_hsm_stable_across_handshakes() { | ||
| let ctx = TestCtx::new(); |
There was a problem hiding this comment.
Shouldnt most new tests call erase before starting the test?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
ddi/win/src/dev.rs:682
- Same issue as above:
NoValidSessionIdis currently remapped toTborStatus::SessionNotFound, but for TBOR callers this should preserve the driver-layer scoping signal usingTborStatus::FileHandleNoExistingSession(numeric encoding matches theDdiStatusvariant).
ddi/tbor/types/tests/harness/fixture.rs:98 - Doc comment is now inaccurate: the harness is gated out only for
--features mock, but this comment says "mock/sock builds gate the whole harness out". Also,open_devis reachable under--features sockas well.
/// Acquire the test lock, open the configured backend device, and
/// factory-reset it before use. Only reachable under `--features emu`
/// or the no-feature (hw) build — mock/sock builds gate the whole
/// harness out at the crate root.
ddi/tbor/types/tests/harness/fixture.rs:86
unsafe impl Send for TestDevmakes the primary (lock-holding)TestDevmovable across threads even though the implementation relies on it staying on the lock-acquiring thread. This invariant isn’t enforced by the type system, so it’s easy for future refactors to accidentally introduce UB. Consider splitting the types (e.g.,PrimaryTestDevholding theMutexGuardandSecondaryTestDevwithout it) so only the secondary handle isSendwithoutunsafe.
// secondaries (with `_guard: None`) are sent into `thread::scope`
// worker threads, and rustc cannot prove that at compile time
// because both variants share the same type.
#[allow(unsafe_code)]
unsafe impl Send for TestDev {}
ddi/win/src/dev.rs:666
map_ioctl_status_tbormaps the driver’sNoExistingSession(per-file-handle session scoping) toTborStatus::SessionNotFound. This loses the driver-vs-FW distinction and contradicts the doc comment that this helper is a pureFileHandle*type-level remap (andTborStatusdoes defineFileHandleNoExistingSession).
This issue also appears on line 680 of the same file.
ddi/nix/src/dev.rs:723
map_ioctl_status_tborcurrently mapsNoExistingSessiontoTborStatus::SessionNotFound, butTborStatusprovidesFileHandleNoExistingSessionfor this driver-layer scoping rejection. Using theFileHandle*status keeps TBOR error typing consistent with theDdiStatusmapping inmap_ioctl_status.
Ok(McrCpGenericIoctlErrorKind::NoExistingSession) => {
return Err(DdiError::TborStatus(TborStatus::SessionNotFound));
}
| pub(crate) use part_init::encrypt_mach_seed_envelope; | ||
| pub(crate) use part_init::part_init; | ||
| pub(crate) use psk_change::encrypt_psk_envelope; | ||
| pub(crate) use psk_change::psk_change; |
There was a problem hiding this comment.
this is a module file, I would rename it to ddi/tbor/types/tests/harness/session/mod.rs
| /// secondary, otherwise the lock guard drops mid-test. | ||
| pub fn new_with_path(path: &str) -> Self { | ||
| Self { | ||
| dev: open_dev_secondary(path), |
There was a problem hiding this comment.
open_dev_secondary panics on failed to open dev. Should we make this try_new_With_path to give more control to the caller for the tests that need to assert/handle open failures explicitely?
This PR merges all the existing HW tests with emu tests. Most of the tests are working both on emu and HW.
There are quite a few HW only tests that need emu support. This should be fixed in the follow-up PR.