Implementation Review Summary

Intent

Add an opt-in Intel Level Zero backend behind a default-off level_zero Cargo feature that augments the existing sysfs (Linux) and WMI (Windows) Intel GPU readers with engine activity (including the XMX COMPUTE_SINGLE class) and energy-counter-derived power, while leaving the default build's binary footprint and runtime behaviour unchanged.

Findings Addressed

• CRITICAL — ZES_STRUCTURE_TYPE_* constants did not match the Sysman spec. The hand-written FFI declared ZES_STRUCTURE_TYPE_PCI_PROPERTIES = 0x1 and ZES_STRUCTURE_TYPE_ENGINE_PROPERTIES = 0xa. Per oneapi-src/level-zero/include/zes_api.h the correct values are 0x2 and 0x5. The values 0x1 and 0xa correspond to DEVICE_PROPERTIES and LED_PROPERTIES respectively, so every refresh wrote the wrong stype into zes_pci_properties_t / zes_engine_properties_t. Intel's current loader does not strictly validate top-level stype, but strict validators and future-proofing both require the correct values. The structure_type_constants_match_spec test also locked the wrong values. Fixed in 2d39a77.
• HIGH — CI Test Suite failed on cargo fmt --check. Seven Level Zero files (and the Linux / Windows glue) had formatting violations that the Test Suite workflow's Check formatting step rejected. Applied cargo fmt cleanly with no semantic changes; all 30 L0 tests + 19 Linux tests still pass under both default and --features level_zero builds. Fixed in a4c956a.

Verified Correct (no fix needed)

• zes_engine_group_t v1-tracked values match the upstream zes_api.h exactly (COMPUTE_SINGLE=4, RENDER_SINGLE=5, MEDIA_DECODE_SINGLE=6, MEDIA_ENCODE_SINGLE=7, COPY_SINGLE=8). The mapping cited in the review prompt was off-by-one against the actual spec; the code is correct.
• Energy-counter math: (delta_energy_uj as f64) / (delta_timestamp_us as f64) produces watts directly because µJ/µs = J/s = W. The power_watts_correct test asserts 30 J over 1 s → 30 W. Comment at refresh.rs:264 documents the unit relationship.
• PCI BDF formatting: format_pci_bdf produces lowercase "{:04x}:{:02x}:{:02x}.{:x}" matching the sysfs symlink target basename (e.g. 0000:03:00.0); normalise_pci_bdf lowercases the sysfs string before lookup so the map hit is case-insensitive.
• ZES_ENABLE_SYSMAN=1 is set inside Once::call_once strictly before the first zeInit call (lines 208–219 precede line 241 in loader.rs); unsafe is correctly scoped per Rust 2024 rules; existing operator override is preserved (if var_os().is_none()).
• Linux augmentation adds Engine: <label> (L0) / Power (L0) detail entries and flips Metrics Source to "sysfs + Level Zero" but does NOT overwrite GpuInfo.utilization or GpuInfo.power_consumption. Test: apply_to_gpu_info_linux_does_not_overwrite_utilization.
• Windows augmentation DOES overwrite utilization (with max(render, compute (XMX))) and power_consumption, and flips Metrics Source to "WMI + Level Zero". Test: apply_to_gpu_info_windows_overwrites_zero_fields.
• Metrics Source baseline strings are emitted before L0 augmentation runs; without an L0 runtime present the baseline survives unchanged ("sysfs (engine counters)" / "WMI").
• Graceful degradation: try_load_library_returns_none_for_nonexistent_path, enumerated_pci_bdfs_empty_when_runtime_absent, and refresh_returns_none_without_runtime cover the no-L0-runtime path. No panic! / unwrap() in production code (the one best.unwrap() in refresh_power is guarded by short-circuit is_none() ||).
• Module gating: pub mod intel_gpu_level_zero in mod.rs is gated on #[cfg(all(any(target_os = "linux", target_os = "windows"), feature = "level_zero"))]. All non-mod.rs references in the per-OS readers (level_zero_state field, crate::device::readers::intel_gpu_level_zero::* paths) are #[cfg(feature = "level_zero")]-gated.
• Default build: cargo build --bin all-smi produces a binary with zero zeInit / libze_loader / ZES_ENABLE_SYSMAN strings (verified locally via strings). ldd shows no L0 library linked even with --features level_zero (dynamic via libloading, as intended).
• File-size budget: all six new files under 500 lines (ffi 246, loader 351, refresh 293, level_zero 342, tests 476, level_zero_glue 37; intel_gpu_linux.rs 497 — at the budget but compliant).
• Commit messages: conventional-commits prefix, English-only, no co-authorship, no AI attribution.
• PR body: explicit deferred surface listed, hardware ACs left unchecked with (awaits hardware verification by maintainer), no new Cargo dependencies, ends with Closes #248.

Remaining Items

• Hardware-gated ACs (XMX activity visible on Arc / Battlemage / Lunar Lake / Meteor Lake; Windows utilization / power non-zero with L0 runtime present; Linux sysfs vs. L0 agreement) — explicitly deferred to maintainer hardware verification per the issue body. Not a finding.
• LOW — stale Note on Windows after L0 succeeds. The WMI baseline writes detail["Note"] = "Detailed metrics require Level Zero / xpu-smi"; once L0 augmentation flips Metrics Source to "WMI + Level Zero", this Note is left in place and now contradicts the real state. The source comment acknowledges the legacy key is kept for backward compatibility. Cosmetic; not blocking.
• LOW — best.unwrap() in refresh.rs:214. Guarded by best.is_none() || short-circuit so it is safe, but best.is_none_or(|b| b < w) would be idiomatic. Cosmetic; clippy passes.
• MEDIUM (acknowledged in PR) — Windows ordinal-paired BDF matching. Pairing WMI controllers with L0 devices by sorted-BDF ordinal is correct for the common 1-Intel-GPU case but can misalign on multi-Intel-GPU Windows hosts. Explicitly flagged in augment_with_level_zero's doc comment with the follow-up path (Win32_PnPEntity.LocationInformation parsing). Not a regression from the WMI-only baseline; acceptable scope-bounded compromise.

Verification

• All stated requirements implemented
• No placeholder/mock code remaining (no TODOs, no todo!/unimplemented!)
• Integrated into project code flow (Linux: level_zero_glue::augment called after baseline GpuInfo push; Windows: augment_with_level_zero called after query_intel_gpus)
• Project conventions followed (Rust 2024 edition, cargo fmt clean, cargo clippy -- -D warnings clean under both feature settings, conventional-commits)
• Existing modules reused where applicable (libloading pattern from tpu_pjrt.rs; Mutex<EngineState> pattern for per-card delta tracking; intel_gpu_names::classify_intel_architecture shared with both OS readers; DeviceStaticInfo cached once)
• No unintended structural changes (only added the level_zero_state field on IntelGpuCard and IntelWindowsGpuReader, plus the module tree under intel_gpu_level_zero/)
• Tests pass (30/30 L0; 19/19 Linux under both feature settings; 20/20 intel_gpu_engine; 23/23 intel_gpu_fdinfo; cargo fmt --check clean; cargo clippy --lib --tests -- -D warnings clean both modes; cargo clippy --lib --tests --features level_zero -- -D warnings clean)

Verdict

Ready for security/perf review pending CI green on the latest push. The one CRITICAL finding (wrong Sysman stype constants) and one HIGH finding (CI red on cargo fmt) have been fixed on the branch in 2d39a77 and a4c956a respectively. All v1-scoped functional checks pass; deferred items match what the PR body and issue #248 explicitly enumerate.

PR Security & Performance Review

Re-review after the two earlier auto-fixes (commits 2d39a77 and a4c956a). I exercised cargo check / cargo clippy / the targeted test suites under both default and --features level_zero builds — every command passes (30/30 L0 tests, 19/19 Linux tests, both with and without the feature; no clippy warnings; default binary contains zero L0 symbols per nm -D).

The remaining findings are all in the new Level Zero FFI surface, in descending severity:

HIGH — Unbounded driver-reported handle counts in vec allocation

src/device/readers/intel_gpu_level_zero/loader.rs:275-276, 294-295 and src/device/readers/intel_gpu_level_zero/refresh.rs:91-92, 139-140 all do vec![std::ptr::null_mut::<c_void>(); count as usize] where count: u32 comes verbatim from the L0 driver via the count-only first pass. A buggy or malicious driver that writes u32::MAX into count would trigger an allocation of ~32 GiB (8 bytes per pointer × 4 billion). The process either OOMs the host or panics on allocation failure — both are denial-of-service surfaces. Cap each enumeration with a guard similar to MAX_DEVICES = 256 used by discover_cards (e.g. let count = count.min(MAX_L0_HANDLES); before the vec![...; count]). Driver / power-domain / engine-group counts in practice are < 64, so a 256 cap is generous and matches the existing parity convention.

MEDIUM — ze_bool_t is uint8_t, not u32

Upstream ze_api.h: typedef uint8_t ze_bool_t;. The PR declares the four ze_bool_t fields as u32 in src/device/readers/intel_gpu_level_zero/ffi.rs:

• zes_engine_properties_t::on_subdevice (line 162)
• zes_pci_properties_t::have_bandwidth_counters (line 132)
• zes_pci_properties_t::have_packet_counters (line 133)
• zes_pci_properties_t::have_replay_counters (line 134)

For zes_engine_properties_t, the next field subdeviceId is uint32_t so the C compiler pads onSubdevice up to a 4-byte slot. Reading on_subdevice as u32 happens to work because Rust's Default zero-initialises the padding bytes before the driver writes its one byte (and subdeviceId lands at the same offset in both layouts). Functionally correct in practice, but the type does not match the spec.

For zes_pci_properties_t the divergence is larger. The C struct ends at byte 55 (three trailing uint8_t bool fields + 5 bytes of struct-end padding to align to the 8-byte void* alignment of the struct). The Rust struct as written is 60 bytes (three u32s, no end padding). When the driver writes 56 bytes per the C ABI, the trailing four Rust bytes (offsets 56-59) are never touched; Default zero-init makes them safe to read but they are not driver data. More importantly, the three Rust u32 fields read across the boundary of the three C u8 bools and the C padding — have_bandwidth_counters ends up reading bytes for all three bools mashed together. The PR never reads these fields (only .address), so today this is dormant. But it is spec-non-compliant, will silently corrupt the values if a future maintainer reads them, and risks compatibility issues with strict-validation drivers or the optional ZE_ENABLE_VALIDATION_LAYER=1 path.

Fix: change all four field types to u8. Re-derive the Rust struct size against the spec C layout while adding the change.

MEDIUM — Engine-group enum constants 9-12 do not match the spec

Spec (verified against https://github.com/oneapi-src/level-zero/blob/master/include/zes_api.h and https://oneapi-src.github.io/level-zero-spec/level-zero/latest/sysman/api.html#zes-engine-group-t):

is_tracked_engine only matches values 4-8 (COMPUTE_SINGLE, RENDER_SINGLE, MEDIA_DECODE_SINGLE, MEDIA_ENCODE_SINGLE, COPY_SINGLE), so the runtime classification of tracked engines is correct on real Arc / Battlemage hardware. The bug is twofold:

1. The lock-in test engine_group_enum_values_match_spec asserts these wrong values "match the spec" — the safety net the PR description praises is fictional from value 9 onwards. A future spec drift in the 4-8 range would still be caught, but the test reads as if the entire enum is locked.
2. Future maintainers adding tracked engines (e.g. wiring RENDER_ALL to capture the unified render-engine aggregate the spec recommends for Arc) would write ffi::ZES_ENGINE_GROUP_RENDER_ALL expecting value 12, but the constant resolves to MEDIA_ENHANCEMENT_SINGLE (9 in spec). Silent misclassification at the source.

Fix: renumber 9-12 to the spec values, add RENDER_ALL = 12 and MEDIA_CODEC_SINGLE = 14, drop or rename the deprecated 3D_SINGLE = 10 / 3D_RENDER_COMPUTE_ALL = 11 slots, update the test to assert against the spec, and update the doc comment that references the spec URL.

LOW — std::env::set_var SAFETY contract violation

src/device/readers/intel_gpu_level_zero/loader.rs:206-219. The SAFETY comment claims "no L0 thread has begun yet (every L0 caller goes through ensure_runtime, which is what we are inside of)" — but the Rust 2024 unsafety of set_var is about any thread reading the environment, not specifically L0 threads. IntelGpuReader::new() is called inside run_collection_loop after the tokio runtime is up and after the axum server (and tracing, and reqwest, and ssh transport) threads are spawned. set_var therefore runs while many threads are alive and may be calling getenv indirectly via glibc internals (locale, time zone, libssl, etc.). This is the exact scenario Rust 2024 marked unsafe.

The window is small (one set_var call early in the first collection iteration) and ZES_ENABLE_SYSMAN itself is read only by the L0 loader, so the real-world chance of UB manifesting is very low. But the SAFETY comment is misleading and the contract is violated.

Either (a) move the env-var injection to a hook called by main before the tokio runtime spawns workers (cleanest, matches the Rust 2024 guidance), or (b) update the SAFETY comment to accurately describe the race window and document why the team accepts it for v1. I would prefer (a) but (b) is acceptable if the orchestration cost is too high.

LOW — level_zero_state map grows monotonically on Windows

src/device/readers/intel_gpu_windows.rs:286-300 inserts into level_zero_state: Mutex<HashMap<String, LevelZeroState>> keyed by gpu.uuid (derived from WMI PNPDeviceID) and never removes. In practice this is bounded by the small set of Intel GPUs in the machine, but on a system where PNPDeviceID is unstable across reboots (rare but possible after driver reinstall) the map would accumulate dead entries. Not worth fixing in v1; just worth noting in the module doc.

Items that are fine

• FFI memory safety. Function pointers are extracted by *ze_init (deref to copy out a Copy fn pointer) and the underlying Library is held in LzRuntime inside a OnceCell for process lifetime — no dangling symbols. All unsafe { (api.fn)(...) } call sites have driver-side null-buffer / count-only semantics and check the return code before reading the out-parameter. Opaque handles (zes_device_handle_t, zes_engine_handle_t, zes_pwr_handle_t) are never freed, never Box::from_raw'd — they live for runtime lifetime, matching the spec's process-scoped opacity.
• Library handle leak via OnceCell matches the tpu_pjrt pattern; no Drop impl on LzRuntime accidentally unloads.
• Library search paths are static const strings — no user-controlled input reaches Library::new.
• Energy and active-time counter deltas use saturating_sub (refresh.rs:234, 238, 259, 263). Backwards-clock, zero-delta, energy-reset, overrun-clamp, and seeding cases all have explicit tests that assert the right behaviour (return 0.0 / return None / clamp to 100).
• The Metrics Source baseline / augmented strings flip exactly once and only when fresh data arrives.
• PCI BDF lowercase formatting ({:04x}:{:02x}:{:02x}.{:x}) is locked in by pci_bdf_format_matches_sysfs and matches the /sys/class/drm/cardN/device symlink-target basename layout.
• Per-card Mutex<LevelZeroState> provides correct serialisation; lock order (per-card → global LZ_RUNTIME) is consistent across Linux and Windows.
• Library-not-found / runtime-absent paths return None cleanly with debug! logs only — verified by try_load_library_returns_none_for_nonexistent_path, enumerated_pci_bdfs_empty_when_runtime_absent, refresh_returns_none_without_runtime.
• Default-feature binary contains zero L0 references (nm -D | grep -ciE 'zes_|ze_loader|level_zero' = 0).

Recommendation

Three changes recommended before merge:

• HIGH: cap the driver-supplied count in all four vec![ptr; count as usize] sites.
• MEDIUM: fix the ze_bool_t type (u8, not u32) for all four boolean struct fields.
• MEDIUM: correct enum values 9-12 (and add RENDER_ALL=12, MEDIA_CODEC_SINGLE=14) so the lock-in test is honest.

The LOW items (set_var SAFETY comment, Windows state-map growth) can ship as-is for v1 with a note for a follow-up.

PR Finalization Complete

Tests

Added 4 unit tests for the cap_handle_count DoS guard in src/device/readers/intel_gpu_level_zero/tests.rs:

• cap_handle_count_passes_through_when_under_cap — normal hardware count (6) passes through unchanged
• cap_handle_count_clamps_when_over_cap — u32::MAX is clamped to MAX_L0_HANDLES (256)
• cap_handle_count_at_exact_boundary — exactly MAX_L0_HANDLES passes through, is not clamped
• cap_handle_count_one_over_boundary_is_clamped — MAX_L0_HANDLES + 1 is clamped

The previous test suite exercised this path only through enumerate_devices, which requires a real L0 runtime. These tests are runtime-free and run in CI.

Total unit tests in the L0 backend suite: 40 (was 36).

Documentation

• Manpage (docs/man/all-smi.1): Added a note to the Intel Arc SUPPORTED PLATFORMS entry describing the opt-in --features level_zero build flag, the runtime library names (libze_loader.so.1 / ze_loader.dll), and the graceful degradation contract. README.md and ARCHITECTURE.md already carried this information; the manpage had none.
• README.md, ARCHITECTURE.md, Cargo.toml: Verified accurate — no changes needed.

Lint / Format

cargo fmt --all, cargo clippy --lib --tests -- -D warnings, cargo clippy --lib --tests --features level_zero -- -D warnings, and the narrow bin-target passes all pass clean. No drift found.

Verification

• All tests pass without --features level_zero (no regression)
• All tests pass with --features level_zero (40 L0 tests, all OK)

Commit

7f3bd83 — chore(intel-gpu-level-zero): add MAX_L0_HANDLES cap tests and manpage feature note