Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved multi-PXB mapping and production BAR-window issues can cause ACPI measurement divergence.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds QEMU-compatible ACPI PCI hotplug AML for root-port buses to fix TDX lite ACPI measurements.
Changes:
- Generates root-port hotplug AML, BSEL mappings, and PCNT notifications.
- Updates
_GPE._E01handling. - Adds golden and eight-GPU regression tests.
File summaries
| File | Summary | Final review comments |
|---|---|---|
dstack/verifier/src/verification.rs |
Adds captured eight-GPU ACPI regression coverage. | Moderate, 1 vote: Test supplies a realized BAR-window size and masks the production default failure. |
dstack/crates/qemu-acpi/src/golden_tests.rs |
Updates hotplug-related golden tests and hashes. | No final comments. |
dstack/crates/qemu-acpi/src/dsdt/notify.rs |
Generates root-port hotplug and PCNT AML. | Critical, 1 vote: Multi-PXB topologies can map PCNT scopes to incorrect devfns. Moderate, 2 votes: PXB handling lacks QEMU-generated coverage. |
dstack/crates/qemu-acpi/src/dsdt/mod.rs |
Integrates hotplug AML and PCNT generation. | No final comments. |
dstack/crates/qemu-acpi/src/dsdt/gpe.rs |
Adds conditional PCI hotplug scanning through _E01. |
No final comments. |
Review details
Suppressed comments (1)
dstack/verifier/src/verification.rs:2482
- This regression test supplies the realized GPU BAR-window length instead of the production default (
pci_hole64_size = 0/None). With the default,crs::buildstill emits the fixed 32 GiB window while QEMU derives_CRSfrom the firmware-assigned GPU BARs, so the real eight-B200 configuration continues to fail the tables digest; this test masks that failure. Please wire the realized window into measurement or reject GPU configurations without an explicit size before treating the verifier path as fixed.
"pci_hole64_size": 0x3c0_0601_4000u64,
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+121
to
+125
| pub(crate) fn pcnt( | ||
| slot_count: u32, | ||
| root_port_count: u32, | ||
| pxb_devfn: Option<u8>, | ||
| ) -> Option<Vec<u8>> { |
Comment on lines
+164
to
+168
| fn root_ports(slot_count: u32, count: u32, pxb_devfn: Option<u8>) -> Vec<(u8, u32)> { | ||
| let mut ports = Vec::with_capacity(count as usize); | ||
| let mut slot = slot_count; | ||
| for index in 0..count { | ||
| if pxb_devfn == Some((slot * 8) as u8) { |
kvinwang
marked this pull request as draft
September 11, 2026 17:02
kvinwang
force-pushed
the
fix/qemu-acpi-root-port-hotplug
branch
from
September 12, 2026 06:43
fb366e2 to
b6535e4
Compare
Co-authored-by: Leechael Yim <yanleech@gmail.com>
The oracle dumped the ACPI tables built inside acpi_setup(), a machine_init_done notifier. QEMU assigns the ACPI hotplug BSEL property to PCI bridge secondary buses in acpi_set_pci_info(), reached from the ICH9 PM reset handler through acpi_pcihp_reset(), and machine reset runs after that notifier. QEMU then rebuilds the tables via acpi_build_update(), so the blob a guest reads -- and measures into RTMR0 -- is the rebuilt one. Sampling before reset produced a DSDT in which no bus had a BSEL, and so none of the root-port hotplug AML this branch adds. The oracle agreed with any generator that omitted those terms: 17 of its 61 cases could not fail, namely every case with bridge hotplug enabled and a root port on pcie.0. That is why this branch's differential job reported the generator as wrong when it was the reference that was wrong. The fix lives in the QEMU compatibility fork, which now performs the one reset side effect that changes the generated AML before dumping, and folds the directory output mode in so both consumers sample at the same point. Bump to that revision, drop the patch it replaces, and repin the image. Also cross the axes that had only ever run GPU-less. Every per-version and large-CPU fixed case ran with no GPUs, so no version tier and no CPU-count boundary was pinned against a topology with root ports. Add the host sharing mode as an explicit axis too: dstack-mr accepts "9p", "vvfat" and "vhd" and models none of them, on the theory that each contributes exactly one PCI slot and the DSDT records only _ADR; QEMU confirms all three are byte-identical. Fixed cases go from 29 to 40. Document the two measured inputs the oracle structurally cannot reach, so a green run is not read as covering them: the 64-bit PCI window when pci_hole64_size is 0, which QEMU derives from BARs the guest firmware has assigned and no dump-and-exit oracle can observe, and multi-PXB topologies, which MachineConfig cannot express.
kvinwang
force-pushed
the
fix/qemu-acpi-root-port-hotplug
branch
from
September 24, 2026 09:04
b6535e4 to
79296be
Compare
Collaborator
Author
|
Superseded by #1387: instead of modeling ACPI hotplug AML for root ports, hotplug is now required off for GPU passthrough and off by default. |
source-c
pushed a commit
to silicon-foundation/dstack
that referenced
this pull request
Oct 4, 2026
…passthrough QEMU emits ACPI hotplug AML for pcie-root-port buses when PCI hotplug is on, and qemu-acpi does not model it, so a TDX CVM with GPUs or NVSwitches on root ports failed lite verification with an ACPI digest mismatch. Rather than modeling that AML (Dstack-TEE#1203), require hotplug off for such topologies: MachineConfig::validate rejects them with a named error, and the VMM config, dstackup template, tutorial and e2e config default qemu_hotplug_off to true. The GPU goldens move to hotplug off, with hashes checked against the pinned QEMU reference image.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A TDX CVM with GPUs behind
pcie-root-ports and ACPI PCI hotplug on (the default,hotplug_off = false) fails lite ACPI verification. A captured 8×B200 CVM (QEMU 10.2.1,dstack-0.6.0-rc3) failsverifier-v0.6.0-rc3with:Once a root port's secondary bus is hotplug capable, QEMU gives it a BSEL and adds hotplug AML to the DSDT (per-port
BSEL/S00_scope/DVNT, a\_SB.PCI0.PCNTtree, and a non-empty_GPE._E01). The generator emitted none of it.Fix
notify.rs: with hotplug on, emit that AML for eachpcie.0root port plus thePCNTscope. BSEL runs in reverse slot order (QEMU inserts child buses at the list head and numbers depth-first). The helper documents the VMM device order this relies on.gpe.rs:_E01acquiresBLCK, callsPCNT, releasesBLCKwhen a PCNT exists; otherwise stays empty.The involved QEMU functions are unchanged (modulo moves/renames) from v8.2.0 to v11.1.0, so no compatibility-profile gating is needed.
Verification
pci_hole64_sizethe guest actually saw (see gap below).tdx_lite_acpi_matches_captured_eight_gpu_qemu_10_2_vmpins all three RTMR0 ACPI digests from that CVM's event log.cargo test -p qemu-acpi -p dstack-verifier, fmt, clippy clean; differential job green.Known gaps (follow-up)
pci_hole64_size = 0, QEMU sizes the DSDT_CRS64-bit window from the firmware-assigned GPU BARs (~3.75 TiB for 8×B200), whichvm_configcannot express, so the tables digest still mismatches on such hosts. Options: GPU hosts set an explicit hole size, the VMM sizes it, or the verifier rejectsnum_gpus > 0 && pci_hole64_size == 0.qemu-acpiat all; this PR does not change that.Co-authored with @Leechael, who wrote the qemu-acpi change.