tdisp_openhcl: Implement necessary infrastructure for TDISP under VPCI Relay

[Michael Frohlich (mfrohlich-msft)]

This PR includes all feature work and changes to the VPCI stack to support relayed TDISP devices over VPCI.

This includes:

• Attestation infrastructure for TDISP presented over VPCI including protocol negotiation, report retrieval, state management, and teardown.
• Command register and BAR emulation to drive TDISP block/unblocks when VPCI receives cfg writes to TDISP-capable devices.
• Traits to implement platform specific functionality and firmware calls (these will be implemented in future stacked PRs for TDX and SNP).
• The new RB version extension of VPCI, which includes an implementation of QueryIsolatedResources to report block/unblocked resources to the guest over VPCI.
• Unit tests to drive tdisp functionality and relayed vpci functionality locally and confirm expected behavior.

Command register accesses in TDISP under the VPCI Relay

This is especially tricky. For the first time, VPCI command register writes, specifically to the command register, drive attestation cycles that could take a significant amount of time on the VP. The VPCI relay handles this by making the TDISP operations async with the command register write. While these operations are happening, other VPs can execute cfg accesses. These accesses are queued in the order recieved and processed as soon as the device completes its TDISP work. This ensures that TDISP attestation flows are serialized and the guest cannot footgun itself with a config space access during the attestation flows.

Testing end-to-end flows with OpenVMM

This PR contains an extension to the OpenVMM end-to-end TDISP test. In a previous PR, the NVMe fault controller emulator was repurposed for basic TDISP testing. In this PR, it's extended to support a full end-to-end acceptance flow including emulation of device BAR behavior on block/unblock.

The test plan is now as follows:

• An emulated TDISP device is created and relayed through the VPCI bus. The device
  requires attestation and the acceptance of its BAR0 range before it answers to any
  BAR requests.
• The device creates and advertises a TDISP capability and synthesized TDI
  report to the guest.
• The guest successfully attests the device over VPCI and accepts its
  register BAR range.
• Once attested and the BAR range accepted, the device's registers become
  accessible.
• Lastly, the TDI is unbound and the device's registers are expected to
  become inaccessible again.

Note: This PR does not include the infrastructure to actually attest a device, it just drives the device to a functional state.

[Copilot]

🟡 Changes recommended

Critical TDISP validation, cleanup, state-handling, and relay issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds TDISP MMIO-range management and VPCI isolated-resource querying for TDX Connect and newer VPCI infrastructure.

Changes:

• Adds ModifyMmioRange support and related serialization/state-machine coverage.
• Adds VPCI RB protocol support and BAR/DMA isolation reporting.
• Integrates TDISP lifecycle handling, validation, relay support, and tests.

File summaries

File	Description
vm/devices/tdisp/src/tests/statemachine_tests.rs	Adds device-ID and state-machine tests.
vm/devices/tdisp/src/tests/serialize_tests.rs	Tests MMIO command serialization.
vm/devices/tdisp/src/tests/mocks.rs	Extends host mocks.
vm/devices/tdisp/src/tests/endtoend_tests.rs	Adds MMIO lifecycle tests.
vm/devices/tdisp/src/test_helpers.rs	Updates test interfaces.
vm/devices/tdisp/src/serialize_proto.rs	Validates MMIO actions.
vm/devices/tdisp/src/lib.rs	Adds MMIO and isolation APIs.
vm/devices/tdisp/src/devicereport.rs	Updates device-report documentation.
vm/devices/tdisp_proto/src/tdisp.proto	Defines new TDISP protocol messages.
vm/devices/tdisp_proto/src/lib.rs	Adds protocol helpers and state formatting.
vm/devices/pci/vpci/src/device.rs	Handles isolated-resource queries.
vm/devices/pci/vpci_relay/src/lib.rs	Integrates relay and asynchronous config handling.
vm/devices/pci/vpci_relay/Cargo.toml	Updates dependencies.
vm/devices/pci/vpci_protocol/src/lib.rs	Adds RB protocol and isolation structures.
vm/devices/pci/vpci_client/src/tests.rs	Adds BAR-decoding tests.
vm/devices/pci/vpci_client/src/tdisp.rs	Implements TDISP lifecycle and resource classification.
vm/devices/pci/vpci_client/src/lib.rs	Integrates client lifecycle and negotiation.
vm/devices/pci/vpci_client/Cargo.toml	Updates dependencies.
vm/chipset_device/src/lib.rs	Adds isolation-report support.
vm/chipset_device_resources/src/lib.rs	Forwards the isolation-report capability.
openhcl/underhill_core/src/worker.rs	Wires resource validation.
openhcl/underhill_core/Cargo.toml	Updates dependencies.
openhcl/openhcl_tdisp/src/noop.rs	Implements no-op validation hooks.
openhcl/openhcl_tdisp/src/lib.rs	Adds TDISP interfaces and command builders.
openhcl/openhcl_tdisp/Cargo.toml	Updates dependencies.
Cargo.lock	Updates locked dependencies.

Review details

Suppressed comments (10)

vm/devices/pci/vpci_client/src/lib.rs:734

• The method documents this path as a no-op unless the TDI is in Run, but it unconditionally sends tdisp_unbind for Unlocked and Locked as well. That can issue an invalid/unnecessary host unbind and clears per-attest state on a path that should do nothing; guard the unbind (and its warning) with the observed Run state.

        if let Err(err) = self.tdisp_unbind(TdispGuestUnbindReason::Graceful).await {

vm/devices/pci/vpci_client/src/tdisp.rs:1012

• This comment has a duplicated word: “the the device”.

            // If any BAR is classified as PRIVATE, the the device should also have PRIVATE DMA.

vm/devices/pci/vpci_client/src/tdisp.rs:1369

• These failures are directly retriggerable by the guest’s isolation query, so the bare error event can be used to flood logs. Use the repository’s rate-limited error macro for this guest-triggered path.

                    tracing::error!(

vm/devices/pci/vpci_protocol/src/lib.rs:112

• The protocol definition marks VPCI_TDISP_COMMAND as valid only for ProtocolVersion::RB and newer, but the server's DeviceRequest::TdispCommand handler does not check self.vpci_version before dispatching it. A guest that negotiated VB can therefore use a packet the negotiated protocol does not advertise; reject it with NOT_SUPPORTED on downlevel protocols, consistently with QueryIsolatedResources.

        /// TDISP command from guest to host.
        ///
        /// Only valid on protocol version >= `ProtocolVersion::RB`.
        VPCI_TDISP_COMMAND = 0x4249001D,

vm/devices/pci/vpci_relay/src/lib.rs:681

• PciBusCfgAccessHandler can queue multiple deferred config writes, but this debug_assert! is removed in release builds. If a second MMIO-enable write arrives while pending is set, the assignment below drops the first DeferredWrite, so its caller receives IoError::NoResponse. Queue or reject overlapping activations in release rather than relying on a debug-only invariant.

        debug_assert!(
            self.pending.is_none(),
            "config space write deferred while another deferred write is in flight"

vm/devices/pci/vpci_relay/src/lib.rs:389

• The mock branch successfully runs the TDISP flow but leaves tdisp_capable set to false. The resulting relay bypasses TDISP config interception and returns NotTdispCapable for QueryIsolatedResources, so the dedicated TDISP test path does not exercise the new behavior. Set the flag after the mock flow succeeds.

        if self.options.test_tdisp_flow {
            Self::tdisp_test_mock_flow(vpci_device.clone())
                .await
                .expect("failed to exercise TDISP flow test");

vm/devices/pci/vpci_relay/src/lib.rs:475

• The new VpciClientTdispState::new initializes tdi_state to Unlocked, so this assertion fails before the mock flow runs. Assert Unlocked here, or make the client use Uninitialized consistently and adjust the attestation and teardown paths that currently treat Unlocked as the entry state.

        assert_eq!(device.tdisp_tdi_state().await, TdispTdiState::Uninitialized);

vm/devices/tdisp/src/lib.rs:166

• Ready is constructed with Invalid for unimplemented BAR slots, and the VPCI wire reply intentionally preserves those INVALID entries on SUCCESS. This public guarantee is therefore false and can cause consumers to reject a valid successful report; document INVALID BAR slots as allowed while keeping DMA limited to SHARED/PRIVATE.

    /// The TDI is in Run and resources have been unblocked. The inner
    /// arrays give the six per-BAR classifications and the DMA
    /// classification. Guaranteed to contain only `Shared` / `Private`.

vm/devices/tdisp/src/lib.rs:186

• This contract says the fresh attestation cycle ends back in Unlocked, but VpciDevice::tdisp_isolation_snapshot calls attest and returns with the TDI in Run. That state is significant because later MMIO activation expects Run; either the implementation or this public lifecycle description must be corrected so callers do not rely on a false postcondition.

    /// To retrieve the report, this may need to drive a fresh attestation cycle
    /// (Unlocked -> Locked -> Run -> cached report -> Unlocked) before
    /// answering. To avoid forcing callers to hold a sync device guard across

vm/devices/tdisp/src/tests/endtoend_tests.rs:554

• dispatch_roundtrip serializes and then calls deserialize_command(...).unwrap(), while validate_command rejects TdispMmioRangeAction::Invalid before the emulator is invoked. This test therefore panics in its helper instead of producing the expected response. Assert the wire-level deserialization error here, or call the emulator directly if the state-machine branch is what this test intends to cover.

    let resp = dispatch_roundtrip(
        &mut mock.emulator,
        modify_mmio_range_cmd(
            DEVICE_ID,
            TdispMmioRangeAction::Invalid,
            1,
            0xd000_0000,
            0x1000,
        ),

• Files reviewed: 25/26 changed files
• Comments generated: 15
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Critical and moderate protocol, lifecycle, cleanup, and validation issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (19)

vm/devices/pci/vpci/src/device.rs:1085

• The protocol comments now say VPCI_TDISP_COMMAND is valid only for ProtocolVersion::RB or newer, but this handler never checks self.vpci_version; a VB fallback channel still deserializes and executes TDISP commands. Gate this arm before handing the command to the chipset device, as the adjacent QueryIsolatedResources arm does.

                        let mut locked_dev = dev.device.lock();
                        if let Some(tdisp) = locked_dev.supports_tdisp_host() {

vm/devices/pci/vpci_client/src/tdisp.rs:168

• This state is the sentinel used by RelayedDevice::remove to distinguish devices that never accepted TDISP; initializing it to Unlocked makes every freshly initialized non-TDISP device look like it needs an Unbind, and the mock flow's first assertion fails. Start in Uninitialized; the first successful interface-info response will move it to Unlocked.

                tdi_state: TdispTdiState::Unlocked,

vm/devices/pci/vpci_client/src/tdisp.rs:677

• The DMA cleanup has the same VTL mismatch as the MMIO cleanup: activation unblocks with self.target_vtl, but this path re-blocks with VTL2. That can leave the actual target VTL's DMA access enabled after unbind. Use the configured target VTL.

                if let Err(e) = validator.tdisp_block_dma(Vtl::Vtl2, raw_device_id) {

vm/devices/pci/vpci_client/src/tdisp.rs:672

• If the platform re-block succeeds but tdisp_host_block_mmio_range fails, this unconditional removal forgets the range anyway. A later unbind or re-attestation then has no record with which to retry the host-side block, so the host can remain unblocked while the TDI state is cleared. Retain failed host cleanup (or track it separately) until both sides have succeeded.

                    self.mutable_state.validated_mmio_bars.remove(&bar_id);

vm/devices/pci/vpci_client/src/tdisp.rs:1149

• The host unblock completes before the platform validator call, but a validator error returns before this range is inserted into validated_mmio_bars. The subsequent cleanup therefore cannot send the compensating host block, leaving the host's range state ahead of the failed platform operation. Roll back the host unblock on validator failure or record the range before the call with explicit partial-cleanup handling.

        self.resource_validator
            .tdisp_unblock_mmio(self.target_vtl, device_id, base_address, 0, length, bar_id)
            .await
            .context("tdisp_on_mmio_reconfigured: failed to unblock MMIO")?;

vm/devices/pci/vpci_client/src/tdisp.rs:967

• A TDISP report may contain multiple MMIO entries sharing one range_id, but .find() classifies only the first entry. A later subrange can therefore be omitted from the BAR's isolation decision (and from the corresponding unblock work), making the reported classification disagree with the report. Handle all entries in the group and preserve their offsets/subranges when performing validation.

        let Some(range) = report
            .mmio_interface_info
            .iter()
            .find(|r| r.range_id == bar_id)

vm/devices/pci/vpci_client/src/tdisp.rs:1372

• The relay has already marked this device TDISP-capable after a successful probe, so a later capability-query failure is an internal/host error, not evidence that the device is non-TDISP. Returning NotTdispCapable maps to a successful all-SHARED VPCI reply, allowing a failed isolation query to be treated as a safe shared device. Return the error classification instead.

                    return TdispIsolationReport::NotTdispCapable;

vm/devices/pci/vpci_client/src/tdisp.rs:1012

• This comment repeats “the”; remove the duplicate.

            // If any BAR is classified as PRIVATE, the the device should also have PRIVATE DMA.

vm/devices/pci/vpci_client/src/tdisp.rs:697

• These per-attestation fields are cleared before the host Unbind command and the final state check complete. If Unbind fails, a retry no longer has the guest device ID, report, or resource records needed to clean up platform state, leaving the TDI irrecoverable. Defer clearing them until Unbind succeeds and the state is confirmed.

        // Clear every per-attest field. All of these will be fetched cleanly on
        // the next re-attest cycle.
        self.mutable_state.tdi_report = None;
        self.mutable_state.guest_device_id = TdispDeviceId::Invalid;
        self.mutable_state.intercepted_bars.clear();

vm/devices/pci/vpci_client/src/tdisp.rs:227

• cached is derived from the host's TDISP response, so an unexpected root value can reach this branch. Panicking here lets untrusted host input terminate OpenHCL; convert a state mismatch into an error and perform teardown instead of asserting on the protocol response.

        if cached != expected {
            panic!(
                "TDI {device_id:?} must be in state {expected}, but the host reports \
                 {cached} (firmware reports {firmware:?})"
            );

vm/devices/pci/vpci_relay/src/lib.rs:622

• BAR writes take this synchronous path even after TDISP activation. VpciDevice::write_cfg only updates the BAR shadow, and this branch never invokes tdisp_on_mmio_reconfigured, so a guest BAR change can leave the host mapping and platform validation at the old GPA while config reads expose the new one. Reject reprogramming while the TDI is active or perform old-range teardown and new-range validation.

        if !self.tdisp_capable || HeaderType00(offset) != HeaderType00::STATUS_COMMAND {
            self.device.write_cfg(offset, value);
            return IoResult::Ok;

vm/devices/pci/vpci_relay/src/lib.rs:392

• VPCI_TDISP_COMMAND is documented as valid only from protocol RB, but the relay probes every host VPCI device without knowing the negotiated host version. On a host that negotiated VB, this sends an invalid message and relies on the resulting failure being treated as ordinary non-TDISP. Propagate the negotiated version and skip the probe when it is below RB.

            // Probe TDISP capability without attesting.
            match vpci_device.tdisp_query_capabilities().await {

vm/devices/pci/vpci_relay/src/lib.rs:648

• This branch silently discards the guest's MMIO-disable edge, but tdisp_on_device_deactivate is never invoked by this path. A guest can therefore leave the TDI in Run with its resources unbound while the command shadow remains enabled. Defer the write until deactivation/unbind completes (or otherwise invoke the deactivation path) instead of dropping the configuration update.

            (true, false) => {
                // Once MMIO is on the TDI is bound and its ranges have been
                // unblocked and accepted into the guest. Drop the write rather
                // than letting the guest walk that back: the disable edge would
                // unbind the device and re-block every range.

vm/devices/pci/vpci_relay/src/lib.rs:405

• All capability-probe errors are treated as if the device simply lacks TDISP, including host or transport failures. That sets tdisp_capable false and later causes QueryIsolatedResources to return SUCCESS with all resources SHARED, hiding an operational failure. Distinguish an explicit unsupported response from a failed probe and surface the latter as an error.

                    tracing::info!(
                        %instance_id,
                        failure_reason = ?e,
                        "TDISP not supported or failed to query capabilities"
                    );

vm/devices/tdisp/src/lib.rs:846

• This context string repeats “to”, producing an awkward error message. Use “failed to modify MMIO range” so callers get a clear failure description.

            .context("failed to call to modify MMIO range");

vm/devices/tdisp/src/lib.rs:134

• The referenced wire type is named VpciQueryIsolatedResources in vpci_protocol, not VpciMsgQueryIsolatedResources; this makes the documentation point at a nonexistent API name.

/// `VpciMsgQueryIsolatedResources`, but is defined here so that

vm/devices/tdisp/src/lib.rs:162

• This says NotReady includes the Run state before resources are unblocked, but isolation_snapshot returns Ready as soon as an interface report is cached, and the existing empty-report test exercises that behavior before any resource is unblocked. Align this public contract with the implementation, or make the snapshot track resource activation.

    /// The TDI is not in the Run state, or is in Run but no resource has
    /// been unblocked yet. The paravisor should answer with an error
    /// status; the guest may retry later.

vm/devices/tdisp/src/lib.rs:153

• This PR adds a guest-visible VPCI_QUERY_ISOLATED_RESOURCES contract and TDISP BAR/DMA isolation semantics, but the existing OpenHCL VPCI relay Guide page has no section describing the RB protocol gate, reply statuses, or classification rules. Update the Guide (and its code-to-Guide mapping if needed) so this structural guest-facing behavior is documented.

/// Classification of a device's BAR and DMA isolation for the VPCI
/// `QueryIsolatedResources` message, reported by the guest-facing VPCI
/// server.

workers/chipset_device_worker/src/worker.rs:149

• The remote worker/proxy has no capability or request forwarding for supports_tdisp_relay—DeviceInit only carries MMIO/PIO/PCI, and the proxy's relay support is absent. Checking only supports_tdisp_host now lets a remote device with the new relay trait cross this boundary while silently losing its TDISP relay behavior. Reject the relay capability here as unsupported too, or add an explicit forwarding path.

            || device.supports_tdisp_host().is_some()

• Files reviewed: 27/28 changed files
• Comments generated: 6
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect TDISP isolation, lifecycle cleanup, and protocol behavior.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (12)

openhcl/openhcl_tdisp/src/lib.rs:304

• new_resource_validator is wired into every production VPCI relay, but every non-test isolation type still receives TdispNoopResourceValidator, whose MMIO/DMA methods only record and return success. A TDISP device can therefore be reported PRIVATE and activated without any platform memory/IOMMU transition, breaking the isolation guarantee. Do not enable TDISP for unsupported production isolation types until real validators are selected, or wire those validators here.

    // TODO: Add platform-specific resource validators based on the isolation type.
    // This will follow in subsequent PRs.

    Ok(Arc::new(noop::TdispNoopResourceValidator::new()))

vm/devices/pci/vpci_client/src/tdisp.rs:158

• VpciClientTdispState is the client-side pre-command state, and the relay's mock flow explicitly expects it to be Uninitialized before the first VPCI TDISP command. Starting at Unlocked makes that assertion fail and loses the sentinel needed to distinguish an untouched channel from one that has negotiated successfully. Initialize this field to Uninitialized; the first host response will transition it to Unlocked.

                tdi_state: TdispTdiState::Unlocked,

vm/devices/pci/vpci_client/src/tdisp.rs:668

• The DMA teardown path also hard-codes Vtl2, while the corresponding activation call uses self.target_vtl. If the configured target differs, DMA is re-blocked for the wrong VTL and the attested device's actual DMA mapping is not restored. Pass self.target_vtl here.

                if let Err(e) = validator.tdisp_block_dma(Vtl::Vtl2, raw_device_id) {

vm/devices/pci/vpci_client/src/tdisp.rs:1107

• This host-side unblock is sent before the platform call, but if tdisp_unblock_mmio fails afterward, the function returns before recording validated_mmio_bars. tdisp_unbind_resources then has no entry to send a compensating host block, leaving the host and platform views inconsistent during failed activation. Roll back the host notification on validator failure or track the range as pending until both operations succeed.

        self.tdisp_host_unblock_mmio_range(bar_id, base_address, length)
            .await
            .context("tdisp_on_mmio_reconfigured: failed to unblock MMIO on the host")?;

vm/devices/pci/vpci_client/src/tdisp.rs:663

• The entry is removed whenever tdisp_block_mmio succeeds, even if tdisp_host_block_mmio_range just failed. That discards the only local record needed for a retry and lets subsequent teardown checks treat the host-side range as restored when it was not. Remove the entry only after both platform and host block succeed, or abort teardown while retaining it.

                    // Successful re-block, remove the bar from the validated list.
                    self.mutable_state.validated_mmio_bars.remove(&bar_id);

vm/devices/pci/vpci_client/src/tdisp.rs:945

• range_id in the TDI report is device-specific and may group multiple MMIO subranges; it is not guaranteed to equal a PCI BAR index. Matching it directly to bar_id can misclassify valid BARs and skip their private-memory handling. Keep an explicit BAR-to-report-range mapping, including any report-relative subrange information needed by the platform validator.

        let Some(range) = report
            .mmio_interface_info
            .iter()
            .find(|r| r.range_id == bar_id)

vm/devices/pci/vpci_client/src/tdisp.rs:837

• The attestation contract says failures leave the device Unlocked with no retained state, but this and the later fallible steps return directly after a successful bind. For example, a failed on_pre_start leaves the host in Locked and no caller of attest is required to clean it up. Add rollback/unbind-on-error around the post-bind portion of the flow.

        self.resource_validator
            .on_pre_start(self.target_vtl, guest_device_id_u16)
            .context("tdisp_attest_device: pre-start validation failed")?;

vm/devices/pci/vpci_client/src/tdisp.rs:1322

• Once the client uses Uninitialized as its pre-command sentinel, this condition will skip auto-attestation for a brand-new device and return NotReady from QueryIsolatedResources. Treat both Uninitialized and Unlocked as the bootstrap states that may need capability probing and attestation.

        if guard.tdi_state() == TdispTdiState::Unlocked {

vm/devices/pci/vpci_client/src/tdisp.rs:980

• This comment contains a duplicated word: the the device.

            // If any BAR is classified as PRIVATE, the the device should also have PRIVATE DMA.

vm/devices/pci/vpci_protocol/src/lib.rs:112

• The protocol contract now declares VPCI_TDISP_COMMAND RB-only, but the ready-state handler still dispatches it under VB, and the client still probes TDISP after the RB-to-VB fallback. A downlevel peer can receive an operation the negotiated protocol says is unsupported (or close the channel). Gate TDISP command probing/dispatch on RB, or keep the command documented as valid for VB as before.

        /// TDISP command from guest to host.
        ///
        /// Only valid on protocol version >= `ProtocolVersion::RB`.
        VPCI_TDISP_COMMAND = 0x4249001D,

vm/devices/pci/vpci_relay/src/lib.rs:472

• This assertion cannot hold with the client state initialization: VpciClientTdispState::new starts at Unlocked, so the mock flow panics before sending its first command. Use the pre-command Uninitialized sentinel consistently (and handle that sentinel consistently during teardown) before relying on this flow.

        assert_eq!(device.tdisp_tdi_state().await, TdispTdiState::Uninitialized);

vm/devices/tdisp/src/lib.rs:160

• Ready is returned by tdisp_isolation_snapshot immediately after attest, but attest explicitly leaves resources inaccessible until MMIO activation and has not unblocked DMA either. This variant therefore claims a readiness condition the implementation does not meet; either report the attested classification separately or delay Ready until resource unblocks complete.

    /// The TDI is in Run and resources have been unblocked. The inner
    /// arrays give the six per-BAR classifications and the DMA
    /// classification. Guaranteed to contain only `Shared` / `Private`.

• Files reviewed: 27/28 changed files
• Comments generated: 7
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved state, cleanup, VTL, BAR-reporting, protocol-gating, and resource-tracking issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (14)

openhcl/openhcl_tdisp/src/noop.rs:183

• The resource validator is shared by all relayed VPCI devices, but this removes every recorded range with the same BAR index, regardless of device_id or VTL. Unbinding one device can therefore erase another device's still-unblocked range; retain records only when the target VTL, device ID, and range ID all differ from the block request.

            self.unblocked_mmio_ranges
                .lock()
                .retain(|r| r.range_id != range_id);

openhcl/openhcl_tdisp/src/noop.rs:47

• This single shared boolean is not a per-device DMA record: the worker constructs one validator and passes it to every relayed device, so unbinding any one TDI sets the flag false even if another TDI remains unblocked. Track DMA state by at least (target_vtl, device_id), or make the accessor explicitly aggregate all active devices.

    dma_unblocked: Mutex<bool>,

vm/devices/pci/vpci/src/device.rs:1178

• For a non-TDISP relayed device, this SUCCESS reply marks all six BAR slots as SHARED, including unimplemented slots. The protocol defines INVALID for a slot outside the device's BAR ID set, and the client already tracks present_bars; carry that mask into the non-TDISP report or emit INVALID for absent BARs instead of claiming they are host-visible resources.

        Some(TdispIsolationReport::NotTdispCapable) => protocol::VpciIsolatedResourcesReply {
            status: protocol::Status::SUCCESS,
            bar_isolation: [ResourceIsolation::SHARED; 6],
            dma_isolation: ResourceIsolation::SHARED,

vm/devices/pci/vpci_client/src/tdisp.rs:158

• tdi_state is the cache of the last host response, so before the first command it must be Uninitialized. The relay's mock flow explicitly asserts that state at vpci_relay/src/lib.rs:472-475; initializing it to Unlocked makes that test fail and loses the pre-command distinction needed by teardown. Initialize this field to TdispTdiState::Uninitialized.

                tdi_state: TdispTdiState::Unlocked,

vm/devices/pci/vpci_client/src/tdisp.rs:668

• DMA is unblocked with self.target_vtl on the activation path, but cleanup re-blocks it with hard-coded Vtl2. For the relay's Vtl0 target this leaves DMA enabled for the actual target after unbind. Pass self.target_vtl here as well.

                if let Err(e) = validator.tdisp_block_dma(Vtl::Vtl2, raw_device_id) {

vm/devices/pci/vpci_client/src/tdisp.rs:1329

• Once the relay has marked a device tdisp_capable, a later capability-query failure means the isolation state could not be read, not that the device is non-TDISP. Returning NotTdispCapable maps to a successful all-SHARED reply in build_isolation_reply, allowing the guest to proceed after a host error. Return Error (or otherwise preserve an error state) here.

                Err(err) => {
                    tracing::error!(
                        "tdisp_isolation_snapshot: query_capabilities failed (tdisp not supported or host errored out): {err}"
                    );
                    return TdispIsolationReport::NotTdispCapable;

vm/devices/pci/vpci_client/src/tdisp.rs:827

• After tdisp_bind_interface succeeds, each later fallible step in attest returns directly, leaving the host TDI in Locked or Run and retaining platform state. This violates the public tdisp_attest_device contract that an error leaves the device Unlocked with no attestation state; the outer activation wrapper happens to clean up, but direct trait callers do not. Add rollback in attest or revise the contract.

        self.tdisp_bind_interface()
            .await
            .context("tdisp_attest_device: failed to bind device interface")?;

vm/devices/pci/vpci_client/src/tdisp.rs:945

• The interface-report contract permits multiple MMIO entries to share one range_id (vm/devices/tdisp/src/devicereport.rs:65-67), but find inspects only the first entry. A grouped range with differing subrange attributes is therefore classified according to report order, which can make the whole BAR appear PRIVATE or SHARED incorrectly and skip or perform the wrong unblock operation. Aggregate or otherwise explicitly handle all entries for the BAR.

        let Some(range) = report
            .mmio_interface_info
            .iter()
            .find(|r| r.range_id == bar_id)

vm/devices/pci/vpci_protocol/src/lib.rs:112

• VPCI_TDISP_COMMAND is now documented as valid only for RB, but the VPCI ReadyState still accepts and forwards this message without checking self.vpci_version (unlike the new QueryIsolatedResources path). A VB guest can therefore use an interface the negotiated protocol says is unavailable; enforce the version gate in the command/probe path or remove the RB-only contract.

        /// TDISP command from guest to host.
        ///
        /// Only valid on protocol version >= `ProtocolVersion::RB`.
        VPCI_TDISP_COMMAND = 0x4249001D,

vm/devices/pci/vpci_relay/src/lib.rs:141

• The pre-command sentinel is Uninitialized, not Unlocked: this condition sends Unbind for a channel that has never received a TDISP response. Because request_unbind requires a negotiated protocol, removal can fail before negotiation; skip unbind only when the state is Uninitialized.

        if self.vpci_device.tdisp_tdi_state().await != TdispTdiState::Unlocked {

vm/devices/pci/vpci_relay/src/lib.rs:401

• This error branch leaves tdisp_capable false for both 'not supported' and a capability-probe failure. The reporter then returns NotTdispCapable, so VPCI answers SUCCESS with all resources SHARED even when the host query failed. Preserve a separate error state and fail the isolation query unless unsupported was positively established.

                Err(e) => {
                    tracing::info!(
                        %instance_id,
                        failure_reason = ?e,
                        "TDISP not supported or failed to query capabilities"

vm/devices/pci/vpci_relay/src/lib.rs:658

• If this invariant is violated, assigning a new future below abandons the first DeferredWrite, so its caller can wait forever. debug_assert! removes that protection in release builds; use a release assertion or handle the second request explicitly.

        debug_assert!(
            self.pending.is_none(),
            "config space write deferred while another deferred write is in flight"
        );

vm/devices/pci/vpci_relay/src/lib.rs:386

• After the mock flow succeeds, tdisp_capable is never set to true. The resulting relay therefore bypasses TDISP command-register activation and reports NotTdispCapable for QueryIsolatedResources, so this test path does not expose the device as the TDISP-capable relay it just exercised.

            Self::tdisp_test_mock_flow(vpci_device.clone())
                .await
                .expect("failed to exercise TDISP flow test");

vm/devices/tdisp/src/lib.rs:160

• Ready is documented as requiring resources to have been unblocked and its arrays to contain only Shared/Private, but isolation_snapshot returns Ready immediately after attestation (before MMIO/DMA unblocking) and intentionally emits Invalid for unimplemented BARs; the tests exercise both. Update this contract so consumers do not treat the report as current accessibility or assume Invalid cannot occur.

    /// The TDI is in Run and resources have been unblocked. The inner
    /// arrays give the six per-BAR classifications and the DMA
    /// classification. Guaranteed to contain only `Shared` / `Private`.

• Files reviewed: 27/28 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved correctness and security-sensitive lifecycle issues remain, including a critical panic path.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (16)

openhcl/openhcl_tdisp/src/noop.rs:47

• This validator is constructed once and cloned into every relayed VPCI device, but the tracking is global: blocking range N removes every device's range N, and one device's DMA block clears the flag for all other devices. Key MMIO and DMA state by at least (target_vtl, device_id, range_id) (and the corresponding device/VTL identity for DMA).

pub struct TdispNoopResourceValidator {
    unblocked_mmio_ranges: Mutex<Vec<UnblockedMmioRange>>,
    dma_unblocked: Mutex<bool>,

vm/devices/pci/vpci/src/device.rs:1066

• VPCI_TDISP_COMMAND is documented as valid only for protocol version RB, and the query branch above enforces that check, but this branch dispatches the command regardless of self.vpci_version. A VB/DT guest can therefore send a TDISP command and receive a successful response. Return NOT_SUPPORTED before deserializing or dispatching when the negotiated version is below RB.

                    DeviceRequest::TdispCommand { data } => {
                        let command = match tdisp::serialize_proto::deserialize_command(&data) {

vm/devices/pci/vpci/src/device.rs:1178

• NotTdispCapable is returned for ordinary devices, but this reply marks all six BAR slots SHARED with SUCCESS. The wire contract says INVALID identifies slots not in the device's BAR ID set, and most PCI devices do not implement all six BARs, so the authoritative reply advertises nonexistent resources. Preserve BAR presence in the report or return a response that does not claim all slots are shared.

        Some(TdispIsolationReport::NotTdispCapable) => protocol::VpciIsolatedResourcesReply {
            status: protocol::Status::SUCCESS,
            bar_isolation: [ResourceIsolation::SHARED; 6],
            dma_isolation: ResourceIsolation::SHARED,

vm/devices/pci/vpci_client/src/tdisp.rs:1069

• isolation_snapshot marks DMA PRIVATE as soon as the report contains any TEE MMIO, but attestation deliberately leaves resources inaccessible until tdisp_on_mmio_reconfigured performs the platform unblock (and dma_unblocked is set). VPCI_QUERY_ISOLATED_RESOURCES can therefore return PRIVATE immediately after auto-attestation while the host still sees the ranges as shared. Base this report on the actual validated/unblocked state or keep it NotReady until resources are accepted.

            // If any BAR is classified as PRIVATE, the the device should also have PRIVATE DMA.
            if bars.contains(&TdispResourceIsolation::Private) {
                // TDISP devices with private MMIO always have private DMA, even
                // if at this moment the device's DMA isn't unblocked.
                TdispResourceIsolation::Private

vm/devices/pci/vpci_client/src/tdisp.rs:1417

• Once this method is called, RelayedVpciDevice has already classified the device as TDISP-capable. A later capability-query failure is therefore a host/error condition, not evidence that the device is non-TDISP; returning NotTdispCapable maps it to a successful all-SHARED reply and can let the guest proceed without attestation. Return Error so VPCI reports UNSUCCESSFUL.

                    return TdispIsolationReport::NotTdispCapable;

vm/devices/pci/vpci_client/src/tdisp.rs:850

• setup_and_attest passes StartupFailure into tdisp_unbind, but TdispHostStateMachine::request_unbind accepts only Graceful, DeviceTeardown, and ResourceSetupFailure; it records this path as InvalidGuestUnbindReason and discards the actual paravisor failure reason. The new proto comments define startup/attestation failures as paravisor unbind reasons, so accept those reasons in the state machine or translate them before sending the command.

                reason: TdispGuestUnbindReason::StartupFailure,

vm/devices/pci/vpci_client/src/tdisp.rs:1197

• If the platform-side unblock fails after the host notification succeeds, this returns before recording the BAR in validated_mmio_bars. Activation then invokes tdisp_unbind, which has no record to drive tdisp_host_block_mmio_range, leaving host bookkeeping unblocked while the platform cleanup failed. Record a pending range before the platform call or explicitly compensate with a host-side block on this error path.

        self.resource_validator
            .tdisp_unblock_mmio(self.target_vtl, device_id, base_address, 0, length, bar_id)
            .await
            .context("tdisp_on_mmio_reconfigured: failed to unblock MMIO")?;

vm/devices/pci/vpci_protocol/src/lib.rs:112

• The protocol now declares VPCI_TDISP_COMMAND valid only for RB and newer, but the VPCI server dispatches DeviceRequest::TdispCommand without checking self.vpci_version (unlike QueryIsolatedResources). A VB/downlevel channel can therefore still execute this RB-only command; add the same version gate before deserializing or forwarding it.

        /// Only valid on protocol version >= `ProtocolVersion::RB`.
        VPCI_TDISP_COMMAND = 0x4249001D,

vm/devices/pci/vpci_relay/src/lib.rs:643

• prev != next is true for both MMIO enable and MMIO disable, but this closure always calls tdisp_on_device_activate. On a disable edge it can re-attest and leave the TDI in Run with stale attestation/resource state instead of unbinding, so later isolation queries can describe a device whose MMIO is disabled. Dispatch tdisp_on_device_deactivate when next is false.

        let fut = Box::pin(async move {
            // Attest while the command register is still off, then turn it on
            // after it succeeds.
            if !device.tdisp_on_device_activate(value).await {
                // The command register is left off if attestation failed.
                // Otherwise, command register is enabled.
                tracing::warn!("TDISP attestation failed. Not enabling STATUS_COMMAND.");
            }
        });

vm/devices/pci/vpci_relay/src/lib.rs:378

• In the mock-flow branch, successful attestation is followed by leaving tdisp_capable false. RelayedVpciDevice::pci_cfg_write then bypasses all TDISP activation/deactivation, and tdisp_isolation_report returns NotTdispCapable even though this device is in Run with an attestation report. Mark the device capable after the mock flow succeeds.

        if self.options.test_tdisp_flow {
            Self::tdisp_test_mock_flow(vpci_device.clone())
                .await
                .expect("failed to exercise TDISP flow test");

vm/devices/pci/vpci_relay/src/lib.rs:393

• Every capability-probe error is collapsed into tdisp_capable = false, which later maps to NotTdispCapable and a successful all-SHARED isolation report. A host/protocol failure is not evidence that the device is non-TDISP; this can expose a device as unprotected and make the guest bypass TDISP. Preserve a failure state (or distinguish explicit unsupported from probe errors) and return an error report for the latter.

                Err(e) => {
                    tracing::info!(
                        %instance_id,
                        failure_reason = ?e,
                        "TDISP not supported or failed to query capabilities"

vm/devices/pci/vpci_relay/src/lib.rs:650

• This is a correctness invariant, not a debug-only diagnostic: in a release build a second deferred write would overwrite self.pending, leaving the first deferred token without completion and stalling config processing. Use an unconditional assertion (or reject the second write) so the invariant is enforced in production.

        debug_assert!(
            self.pending.is_none(),
            "config space write deferred while another deferred write is in flight"
        );

vm/devices/pci/vpci_relay/src/lib.rs:641

• This warning is directly triggerable by repeated guest command-register writes. The VPCI request handler rate-limits comparable guest-triggered events (vm/devices/pci/vpci/src/device.rs:1023-1027 and 1069-1076), so use the rate-limited warning macro here to avoid log amplification during repeated failed activations.

                tracing::warn!("TDISP attestation failed. Not enabling STATUS_COMMAND.");

vm/devices/tdisp/src/lib.rs:915

• setup_and_attest maps failures (including failures after bind/start) to StartupFailure and attest sends that reason through tdisp_unbind, but this match treats StartupFailure and AttestationFailure as invalid. The host therefore records InvalidGuestUnbindReason instead of the failure reason that these new enum values are intended to carry; accept both paravisor-generated reasons here.

            TdispGuestUnbindReason::Graceful
            | TdispGuestUnbindReason::DeviceTeardown
            | TdispGuestUnbindReason::ResourceSetupFailure => {
                TdispUnbindReason::GuestInitiated(reason)

vm/devices/tdisp/src/lib.rs:160

• Ready is documented as containing only Shared/Private, but isolation_snapshot and the new VPCI reply intentionally emit Invalid for unimplemented or report-absent BAR slots. Consumers following this contract could reject or mis-handle valid replies; document Invalid as allowed for those slots.

    /// The TDI is in Run and resources have been unblocked. The inner
    /// arrays give the six per-BAR classifications and the DMA
    /// classification. Guaranteed to contain only `Shared` / `Private`.

vm/devices/tdisp_proto/src/tdisp.proto:96

• These values are documented as paravisor-generated unbind reasons, and setup_and_attest uses StartupFailure, but TdispHostStateMachine::request_unbind accepts only Graceful, DeviceTeardown, and ResourceSetupFailure. The cleanup command is consequently recorded as InvalidGuestUnbindReason instead of preserving the startup/attestation failure reason; separate internal unbind handling from guest validation or allow the paravisor path explicitly.

  // The paravisor failed during initial setup/bind of the TDI.
  TDISP_GUEST_UNBIND_REASON_STARTUP_FAILURE = 5;

• Files reviewed: 27/28 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate issues affect BAR classification, error handling, cleanup, logging, VTL selection, and TDISP operation serialization.

Review details

Suppressed comments (9)

vm/devices/pci/vpci/src/device.rs:1195

• This maps a non-TDISP device to SHARED for all six BAR slots, including BARs that do not exist or are the upper half of a 64-bit BAR. The wire contract explicitly uses INVALID for entries that are not applicable, and the Ready path already preserves that distinction. Pass the device's implemented-BAR information into this conversion (or include it in the report) so a SUCCESS reply does not advertise nonexistent resources.

        Some(TdispIsolationReport::NotTdispCapable) => protocol::VpciIsolatedResourcesReply {
            status: protocol::Status::SUCCESS,
            bar_isolation: [ResourceIsolation::SHARED; 6],
            dma_isolation: ResourceIsolation::SHARED,

vm/devices/pci/vpci_client/src/tdisp.rs:1409

• This path is reached only after the relay has already classified the device as TDISP-capable. If the re-query fails because of a transient, malformed, or otherwise unexpected host response, returning NotTdispCapable makes the VPCI reply SUCCESS with all resources SHARED, which fails open. Return Error (or another non-success result) for this failure instead of downgrading a known TDISP device.

            let info = match guard.query_capabilities().await {
                Ok(info) => info,
                Err(err) => {
                    tracing::error!(
                        "tdisp_isolation_snapshot: query_capabilities failed (tdisp not supported or host errored out): {err}"

vm/devices/pci/vpci_client/src/tdisp.rs:1064

• The comment contains a duplicated word (the the), which makes the explanation unclear.

            // If any BAR is classified as PRIVATE, the the device should also have PRIVATE DMA.

vm/devices/pci/vpci_client/src/tdisp.rs:1196

• If the host accepts the unblock but tdisp_unblock_mmio fails, this function returns before inserting validated_mmio_bars. The subsequent unbind therefore has no record with which to send ModifyMmioRange(BlockMmioRange), leaving the host's range state unrolled back while the platform reports failure. Keep a pending cleanup record or explicitly roll back the host notification on this error path.

        self.resource_validator
            .tdisp_unblock_mmio(self.target_vtl, device_id, base_address, 0, length, bar_id)
            .await
            .context("tdisp_on_mmio_reconfigured: failed to unblock MMIO")?;

vm/devices/pci/vpci_client/src/tdisp.rs:454

• The full host-supplied report is emitted at info on every attestation, and a guest can repeatedly trigger attest/unbind cycles by toggling MMIO. Reports can be large, so this can flood production logs and expose report contents; keep raw bytes at debug/trace or emit only a bounded digest/length with rate limiting.

        tracing::info!(
            vpci_device_id = self.vpci_device_id,
            len = buffer.len(),
            raw = format_args!("{buffer:02x?}"),
            "tdisp_get_tdi_report: raw TDI interface report from the host"

vm/devices/pci/vpci_relay/src/lib.rs:405

• The initial capability probe conflates an unsupported device with every probe failure, including VPCI transport errors and invalid host responses. That leaves tdisp_capable false, so the relay bypasses TDISP on config writes and later reports NotTdispCapable (which maps to a successful all-SHARED reply). In an isolated VM this fails open; only an explicit unsupported result should take this path, while other failures should fail attachment or be reported as an error.

                Err(e) => {
                    tracing::info!(
                        %instance_id,
                        failure_reason = ?e,
                        "TDISP not supported or failed to query capabilities"

vm/devices/pci/vpci_relay/src/lib.rs:744

• This future can call tdisp_isolation_snapshot, which performs a full bind/start/report attestation when the TDI is Unlocked, but it does not mark RelayedVpciDevice::pending. If a guest sends this query before any config-triggered operation and another VP writes config space during the await, pci_cfg_write sees no pending operation and passes the write through instead of queueing it. Coordinate this attestation with the relay's pending/queue gate so config accesses remain serialized for the whole TDISP operation.

            if !tdisp_capable {
                return TdispIsolationReport::NotTdispCapable;
            }

            device.tdisp_isolation_snapshot().await

vm/devices/pci/vpci_relay/src/lib.rs:376

• The client is initialized with a hard-coded Vtl::Vtl0, but all resource validation and cleanup later use the client's stored target_vtl. This bypasses the configured target VTL for any relay assignment that is not VTL0, so MMIO/DMA protections can be applied to the wrong guest VTL. Pass the relay's configured target VTL through instead of hard-coding it here.

        let (vpci_device, removed) = vpci_device
            .init(
                resource_validator,
                self.isolation_type,
                self.vtom.unwrap_or(0),
                hvdef::Vtl::Vtl0,
            )

vm/devices/tdisp/src/lib.rs:160

• TdispIsolationReport::Ready can currently contain Invalid BAR entries: isolation_snapshot initializes unimplemented/report-absent slots to Invalid, and the VPCI mapper forwards those values. The Shared/Private guarantee here is therefore false and can cause consumers to treat every BAR slot as classified; document Invalid as a possible BAR value.

    /// The TDI has attested and parsed its report successfully. The inner
    /// arrays give the six per-BAR classifications and the DMA classification.
    /// Guaranteed to contain only `Shared` / `Private`.

• Files reviewed: 27/28 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

6 Petri tests failed (0 unstable)

[Copilot]

🟡 Changes recommended

Unresolved critical VPCI protocol and teardown issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (10)

Previously missed (1) — in code that hasn't changed since the last review.

vm/devices/pci/vpci_client/src/tdisp.rs:594

• The documentation says cleanup is “best-effort” and that failures do not abort, but the implementation below explicitly panics when MMIO or DMA re-blocking fails. This contradiction matters to callers because tdisp_unbind is used during teardown; document the failure as fatal (or change the implementation to match the best-effort contract).

openhcl/openhcl_tdisp/src/noop.rs:48

• The validator's state is global to the validator even though every resource API call is scoped by target_vtl and device_id. If one validator is reused for two TDIs, unbinding one TDI's range_id removes the other's record, and the single DMA flag is cleared for the other device; key MMIO and DMA state by (target_vtl, device_id) (plus range ID for MMIO), or make the validator non-shareable.

pub struct TdispNoopResourceValidator {
    unblocked_mmio_ranges: Mutex<Vec<UnblockedMmioRange>>,
    dma_unblocked: Mutex<bool>,
}

vm/devices/pci/vpci/src/device.rs:2689

• The test implementation refers to Future without importing std::future::Future; that trait is not in the prelude, so this new impl fails to compile. Use the fully qualified trait (or add the missing import).

        ) -> std::pin::Pin<Box<dyn Future<Output = tdisp::TdispIsolationReport> + Send + 'static>>

vm/devices/pci/vpci/src/device.rs:1195

• NotTdispCapable is serialized as SHARED for all six BAR slots, including unimplemented slots. The wire contract documents INVALID as the authoritative value for a successful reply when a slot is not part of the device's BAR ID set, so this misclassifies non-existent BARs. Carry the implemented-BAR mask into this report or emit INVALID for those slots.

        Some(TdispIsolationReport::NotTdispCapable) => protocol::VpciIsolatedResourcesReply {
            status: protocol::Status::SUCCESS,
            bar_isolation: [ResourceIsolation::SHARED; 6],
            dma_isolation: ResourceIsolation::SHARED,

vm/devices/pci/vpci_client/src/tdisp.rs:1196

• The host-side unblock completes before the platform validator call, but the validated_mmio_bars entry is inserted only after both calls succeed. If the platform unblock fails, teardown has no record of this range and will not send the compensating host block, leaving host and platform resource state out of sync. Track the pending range or compensate the host before returning the error.

        // Tell the host before the platform unblocks. Depending on the platform, the host
        // may need to perform bookkeeping operations before the guest can unblock the range.
        self.tdisp_host_unblock_mmio_range(bar_id, base_address, length)
            .await
            .context("tdisp_on_mmio_reconfigured: failed to unblock MMIO on the host")?;

        self.resource_validator
            .tdisp_unblock_mmio(self.target_vtl, device_id, base_address, 0, length, bar_id)
            .await
            .context("tdisp_on_mmio_reconfigured: failed to unblock MMIO")?;

vm/devices/pci/vpci_client/src/tdisp.rs:1064

• This comment contains a duplicated article (the the), which makes the explanation harder to read. Remove the duplicate.

            // If any BAR is classified as PRIVATE, the the device should also have PRIVATE DMA.

vm/devices/pci/vpci_client/src/tdisp.rs:1411

• A capability-query failure is being reported as NotTdispCapable, which build_isolation_reply converts to SUCCESS with every BAR and DMA marked SHARED. For a device already classified as TDISP-capable, this is an inability to determine isolation, not proof that the device is non-TDISP; returning SHARED can let the guest treat resources as host-visible after an error. Return TdispIsolationReport::Error here so the wire status is unsuccessful/invalid instead.

                    return TdispIsolationReport::NotTdispCapable;

vm/devices/pci/vpci_relay/src/lib.rs:376

• The relay now passes a target_vtl into VpciDeviceDescription::init, but this production path hardcodes Vtl0 for every device. That discards the target-VTL configuration and can apply TDISP firmware resource operations to the wrong VTL once the relay serves another target; thread the configured target VTL through the relay instead.

        let (vpci_device, removed) = vpci_device
            .init(
                resource_validator,
                self.isolation_type,
                self.vtom.unwrap_or(0),
                hvdef::Vtl::Vtl0,
            )

vm/devices/pci/vpci_relay/src/lib.rs:405

• This Err path leaves tdisp_capable false, and the relay later maps that state to NotTdispCapable/SUCCESS with all resources SHARED. A transport or host failure during the probe is not evidence that the device is non-TDISP; preserving an error/unknown state is necessary so VPCI_QUERY_ISOLATED_RESOURCES cannot grant a shared classification after an inconclusive probe.

                Err(e) => {
                    tracing::info!(
                        %instance_id,
                        failure_reason = ?e,
                        "TDISP not supported or failed to query capabilities"

vm/devices/tdisp/src/lib.rs:160

• Ready is constructed with Invalid entries for unimplemented or report-absent BAR slots (and the tests assert that shape), so the public contract here is false. Document that the arrays may contain Invalid; otherwise consumers may treat a valid response as guaranteed to classify every slot.

    /// The TDI has attested and parsed its report successfully. The inner
    /// arrays give the six per-BAR classifications and the DMA classification.
    /// Guaranteed to contain only `Shared` / `Private`.

• Files reviewed: 27/28 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved issues affect error handling, isolation reporting, and resource state consistency.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

openhcl/openhcl_tdisp/src/lib.rs:298

• For non-test runs this factory returns TdispNoopResourceValidator even for SNP and TDX, while query_capabilities accepts those isolation types and the relay exposes the device as TDISP-capable. A real CVM can therefore classify host-reported TEE MMIO as PRIVATE and drive resource activation without any firmware/page-state or DMA validation. Fail closed until a platform validator is selected, rather than enabling this path with a no-op validator.

    Ok(Arc::new(noop::TdispNoopResourceValidator::new()))

openhcl/openhcl_tdisp/src/noop.rs:183

• The no-op validator can be supplied through the shared Arc<dyn TdispResourceValidationInterface> dependency, but this cleanup matches only range_id. If one validator services multiple TDI IDs or VTLs, unbinding one device removes another device's MMIO record as well, making later cleanup/bookkeeping incorrect. Include target_vtl and device_id in the key.

            self.unblocked_mmio_ranges
                .lock()
                .retain(|r| r.range_id != range_id);

vm/devices/pci/vpci_client/src/tdisp.rs:1196

• If the platform-side unblock fails, this returns before inserting the range into validated_mmio_bars, even though the host-side unblock above has already succeeded. The subsequent unbind therefore has no record with which to send a compensating host block, leaving the host's TDI state out of sync. Roll back the host operation on platform failure or retain the range until both sides succeed.

        self.resource_validator
            .tdisp_unblock_mmio(self.target_vtl, device_id, base_address, 0, length, bar_id)
            .await
            .context("tdisp_on_mmio_reconfigured: failed to unblock MMIO")?;

vm/devices/pci/vpci_client/src/tdisp.rs:1067

• DMA isolation is inferred solely from whether any BAR is private, but the TDI report exposes generate_dma_without_pasid and generate_dma_with_pasid to indicate whether a DMA path exists. A device with private MMIO and neither flag set is reported as having PRIVATE DMA despite having no DMA resource; use the report's DMA capability flags before assigning this field.

        let dma: TdispResourceIsolation = {
            // If any BAR is classified as PRIVATE, the the device should also have PRIVATE DMA.
            if bars.contains(&TdispResourceIsolation::Private) {
                // TDISP devices with private MMIO always have private DMA, even
                // if at this moment the device's DMA isn't unblocked.

vm/devices/pci/vpci_relay/src/lib.rs:367

• Every relayed device is initialized with Vtl0, and VpciClientTdispState passes this value to all platform resource-validation and cleanup calls. VpciRelay::new has no target-VTL input, so a relay serving another configured VTL would unblock and re-block resources in the wrong context. Thread the configured target VTL through the relay and pass it here instead of hard-coding Vtl0.

            .init(
                resource_validator,
                self.isolation_type,
                self.vtom.unwrap_or(0),
                hvdef::Vtl::Vtl0,

vm/devices/tdisp/src/lib.rs:160

• This contract is inconsistent with the implementation: isolation_snapshot returns Ready with Invalid entries for unimplemented or report-absent BAR slots, and the VPCI mapping preserves those entries. Document that Ready may contain Invalid BAR classifications so callers do not assume every entry is Shared or Private.

    /// The TDI has attested and parsed its report successfully. The inner
    /// arrays give the six per-BAR classifications and the DMA classification.
    /// Guaranteed to contain only `Shared` / `Private`.

• Files reviewed: 28/29 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings include a compile failure, unsafe relay error handling, and incorrect resource bookkeeping and logging behavior.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (7)

openhcl/openhcl_tdisp/src/noop.rs:47

• This validator is stored behind Arc and receives target_vtl/device_id, but its state is global: MMIO cleanup later filters only by range_id, and DMA is one shared boolean. If a validator is reused for two TDIs or VTLs, unbinding BAR 0 or DMA for one device removes/clears the bookkeeping for the other, so its resources may not be cleaned up. Key the tracked state by at least (target_vtl, device_id, range_id) and key DMA state the same way.

    unblocked_mmio_ranges: Mutex<Vec<UnblockedMmioRange>>,
    dma_unblocked: Mutex<bool>,

vm/devices/pci/vpci_relay/src/lib.rs:451

• This collapses every capability-probe error into NotTdispCapable, but that variant is serialized as SUCCESS with every BAR and DMA resource marked SHARED by build_isolation_reply. A transient host/channel failure can therefore be reported as an authoritative shared classification; preserve unsupported versus probe-error state and return an error/UNSUCCESSFUL result for the latter.

                    tracing::info!(
                        %instance_id,
                        failure_reason = ?e,
                        "TDISP not supported or failed to query capabilities"
                    );

vm/devices/pci/vpci_relay/src/lib.rs:559

• There is a stray period before The, which makes this new documentation read incorrectly.

    /// The write was deferred by cfg handling .The future carries out async

vm/devices/tdisp/src/lib.rs:160

• Ready is also produced when the report omits BARs or when a slot is the upper half of a 64-bit BAR; those entries are intentionally TdispResourceIsolation::Invalid (see isolation_snapshot). This guarantee contradicts the public API's actual output and can cause consumers to assume every slot is classified.

    /// Guaranteed to contain only `Shared` / `Private`.

vm/devices/tdisp/src/lib.rs:329

• This error log is directly reachable from an untrusted ModifyMmioRange command, so a guest can submit invalid actions repeatedly and flood the paravisor's logs. Use the repository's rate-limited error logging for this guest-triggered path.

                        tracing::error!(
                            action = cmd.action,
                            "ModifyMmioRange action is not a valid TdispMmioRangeAction"

vm/devices/tdisp/src/lib.rs:338

• This error log is also directly reachable from an untrusted ModifyMmioRange command, so a guest can repeatedly send an oversized range_id to flood the paravisor's logs. Use the repository's rate-limited error logging for this guest-triggered path.

                        tracing::error!(
                            range_id = cmd.range_id,
                            "ModifyMmioRange range_id does not fit in a u16"
                        );
                        error = TdispGuestOperationError::InvalidGuestCommandId;

vm/devices/tdisp/src/lib.rs:816

• A guest can send the valid Invalid enum value repeatedly, so this bare error event is guest-controlled and unbounded. Use a rate-limited error event here (and apply the same treatment to the adjacent invalid-state log) to prevent log flooding.

        if action == TdispMmioRangeAction::Invalid {
            tracing::error!("ModifyMmioRange requested with an invalid action.");
            return Err(TdispGuestOperationError::InvalidGuestCommandId);

• Files reviewed: 38/39 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

One critical and seven moderate findings remain unresolved, affecting error propagation, per-device isolation, command handling, and test compilation.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (7)

openhcl/openhcl_tdisp/src/noop.rs:183

• The validator API includes target_vtl and device_id, and this validator can be shared through its Arc interface, but blocking matches only range_id. Blocking one TDI can therefore remove another TDI's recorded range when IDs or VTLs overlap; include the target VTL and device ID in the predicate.

            self.unblocked_mmio_ranges
                .lock()
                .retain(|r| r.range_id != range_id);

openhcl/openhcl_tdisp/src/noop.rs:47

• DMA tracking is a single global boolean even though every DMA operation is scoped by target_vtl and TDI device_id. With a shared validator, unblocking one device makes dma_unblocked() true for all devices, and blocking any device clears the others' state; track the active DMA identities instead.

    dma_unblocked: Mutex<bool>,

vm/devices/pci/vpci_relay/src/lib.rs:450

• All failures from the initial capability probe are collapsed into tdisp_capable = false. supports_tdisp_relay then returns NotTdispCapable, which the VPCI server maps to SUCCESS with all resources SHARED; a transient host/RPC failure is therefore reported as a valid shared device instead of an error. Preserve unsupported and failed-probe states separately.

                Err(e) => {
                    tracing::info!(
                        %instance_id,
                        failure_reason = ?e,
                        "TDISP not supported or failed to query capabilities"

vm/devices/pci/vpci_relay/src/lib.rs:430

• reserve_mock_bar_mmio deliberately returns None when the relay range is too small, but this path immediately calls expect on that result. Enabling the test flow with a one-device-sized relay MMIO range therefore panics during device arrival instead of reporting the configuration error.

            let bar_mmio = self
                .mock_bar_mmio
                .expect("the mocked TDISP flow needs a reserved MMIO window");

vm/devices/pci/vpci_relay/src/lib.rs:620

• On the MMIO-enabled-to-disabled edge, the original command-register write is never applied: this future receives no value, and clear_command_register starts from the previous shadow before clearing only MMIO and bus-master. A write that disables MMIO while changing another command bit therefore silently loses that change; merge the incoming command value first, then force the safety bits off.

            (true, false) => {
                let device = self.device.clone();
                CfgWriteOutcome::Started(Box::pin(async move {
                    device.tdisp_on_device_deactivate().await;
                }))

vm/devices/tdisp/src/lib.rs:160

• Ready is also used for classifications containing Invalid: isolation_snapshot initializes all BARs as Invalid and returns it for unimplemented or report-absent slots, and the VPCI reply maps that value explicitly. The public contract should not claim that Ready contains only Shared and Private.

    /// Guaranteed to contain only `Shared` / `Private`.

vm/devices/tdisp/src/tests/devicereport_tests.rs:74

• This test uses size_of without importing or qualifying it; imports in the parent tests module are not visible as unqualified names here, so the crate fails to compile with an unresolved name. Qualify the call (or add a local import).

• Files reviewed: 38/39 changed files
• Comments generated: 1
• Review effort level: Lite

[github-actions]

2 Petri tests failed (1 unstable)

[Chris Oo (chris-oo)]

no where near done reviewing, but wanted to leave some initial thoughts after looking at vpci_relay

[Chris Oo (chris-oo)]

this is only for testing?

[Chris Oo (chris-oo)]

do we intend for this test code to be available in the prod code? i'm wondering if we should cfg gate this, or we need it for testing it end-to-end?

[Chris Oo (chris-oo)]

At least, i'm trying to understand how we can make it more clear what's test / mock code vs what's not, because sprinkling different control flow thru main code seems pretty unreadable/unextendable...

[Chris Oo (chris-oo)]

are we supposed to infer if we have deffered writes by the lenth of queue below?

[Chris Oo (chris-oo)]

do we ever keep track what vps are pending on the bar write?

[Chris Oo (chris-oo)]

maybe this should be inspect by iter? and then implement inspect on QueuedWrite?

[Chris Oo (chris-oo)]

we talked about this offline, but do we think we're going to need to support more of these queued writes in the future? or just for relayed tdisp devices?

[Chris Oo (chris-oo)]

how is it possible we get down this path? is it because we blanket implement this trait for all relayed devices?

[Chris Oo (chris-oo)]

is there a way to enforce that we called this with no operations in flight?