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
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 12, 2026 06:43
fb366e2 to
b6535e4
Compare
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 enabled (the default;hotplug_off = false) fails lite ACPI verification. A captured attestation from an 8×B200 CVM (QEMU 10.2.1,dstack-0.6.0-rc3) failsverifier-v0.6.0-rc3with:Root cause: once a root port's secondary bus is ACPI-hotplug capable, QEMU gets a BSEL for it (
acpi_set_bsel) and adds hotplug AML to the DSDT. The generator emitted none of it:BSEL, aScope (S00_)withASUN/_DSM/_SUN/_EJ0, and aDVNTmethod (build_pci_bridge_aml→build_append_pcihp_slots);\_SB.PCI0.PCNTtree that selects each bus and notifies it (build_append_notification_callback);_GPE._E01that calls\_SB.PCI0.PCNTunderBLCKwhenever a PCNT exists.Fix
notify.rs: when hotplug is on, emit the hotplug AML for eachpcie.0root port, plus thePCNTscope. BSEL values run in reverse slot order because QEMU inserts each secondary bus at the head of the parent's child list (QLIST_INSERT_HEADinpci_bridge_initfn) and assigns BSEL depth-first.gpe.rs:_E01acquiresBLCK, callsPCNT, and releasesBLCKwhen a PCNT exists. Otherwise it stays empty, as before.QEMU version coverage
I extracted every QEMU function involved from the upstream tags and compared them after stripping comments and whitespace:
build_append_pcihp_slots,build_append_pcihp_notify_entry,is_devfn_ignored_{generic,hotplug}build_pci_bridge_aml,build_append_pci_bus_devicesaml_pci_device_dsmhw/acpi/pcihp.cin 10.1, body identicalbuild_append_notification_callback(PCNT)_E01/has_pcntblock inbuild_dsdtacpi_set_bsel,pci_for_each_bus_depth_first, child-busQLIST_INSERT_HEAD,pcie_cap_slot_initacpi-pci-hotplug-with-bridge-supportdefault for current q35So the new AML is version-independent across the supported range and needs no compatibility-profile gating.
Verification
verifier-v0.6.0-rc3plus this change. Only one other input differs from the capture:pci_hole64_sizeis set to the 64-bit window the guest actually saw (see the note below). With that, all four checks pass: quote, event log, OS image hash, and ACPI (is_valid: true,tcb_status: UpToDate).tdx_lite_acpi_matches_captured_eight_gpu_qemu_10_2_vmpins all three RTMR0 ACPI digests (loader, RSDP, tables) from that CVM's event log. The loader digest alone only proves table sizes and offsets; the tables digest proves the AML bytes.cargo test -p qemu-acpi(46 passed),cargo test -p dstack-verifier tdx_lite_acpi(5 passed), clippy clean.Not covered: no QEMU-generated fixture with root ports and endpoints exists yet. The byte-level evidence for versions other than 10.2.1 is the source comparison above. A follow-up should add a root-port + endpoint topology to
scripts/differential-upstream.sh.Known gap (follow-up, not in this PR)
The DSDT
_CRS64-bit window ismax(firmware-assigned BAR range, start + pci-hole64-size)(q35_host_get_pci_hole64_end). With the defaultpci_hole64_size = 0, QEMU uses a 32 GiB hole, but eight B200s need roughly 3.75 TiB of BAR space. The real window (0x380000000000-0x3bc006013fff) therefore cannot be derived fromvm_config, and the tables digest still mismatches in production. Possible fixes:qemu_pci_hole64_sizelarger than the BAR span;num_gpus > 0 && pci_hole64_size == 0with a clear error.Separately, the generator ignores QEMU's 1 GiB round-up of
start + pci_hole64_size.Co-authored with @Leechael, who wrote the qemu-acpi change.