igvmfilegen: make generated ACPI tables for SNP guests use a single NUMA node - #4466
Conversation
Assign every VP to node zero in FixedGuestLayout to match its single RAM node. Verify the generated SRAT CPU and memory affinities, APIC IDs, and checksum, including x2APIC entries. Document matching OpenVMM memory, processor, socket, and SMT arguments and the requirement to regenerate existing images. Verified 2, 4, and 8 VP boots with SMT enabled and no topology warnings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 309b650d-fb0e-44da-8054-30ece8ab7e01
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review issues were identified.
Pull request overview
Updates generated SNP Linux-direct ACPI topology so all VPs and memory reside in NUMA node 0.
Changes:
- Assigns all VPs to NUMA node 0.
- Adds SRAT validation tests and an ACPI parsing test dependency.
- Documents fixed-profile NUMA and launch requirements.
File summaries
| File | Summary |
|---|---|
vm/loader/manifests/README.md |
Documents the fixed topology and launch configuration. |
vm/loader/igvmfilegen/src/snp_linux_direct.rs |
Assigns VPs to node 0 and validates SRAT output. |
vm/loader/igvmfilegen/Cargo.toml |
Adds the ACPI parsing test dependency. |
Guide/src/reference/openvmm/management/cli.md |
Documents SNP topology requirements. |
Cargo.lock |
Records the dependency update. |
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let mut processors = TopologyBuilder::new_x86() | ||
| .build(processor_count) | ||
| .context("building processor topology")?; | ||
| // Match the single RAM node instead of inheriting per-socket vnodes. |
There was a problem hiding this comment.
is this yet another instance of a GPT model documenting something that no longer exists? it loves to do that
There was a problem hiding this comment.
actually i guess not, because the default behavior is to inherit. why is that the default behavior?
There was a problem hiding this comment.
the fact that the default behavior is one cpu per socket? no idea...
There was a problem hiding this comment.
Should we change that default behavior? It always seemed weird to me that the default was one cpu per socket...
There was a problem hiding this comment.
Or at least something that is a TopologyBuilder method.
There was a problem hiding this comment.
we could but seems out of scope of this PR. that probably changed when john added support for "auto" to match host topology
| the IGVM after changing the manifest or updating the generator's topology | ||
| logic; existing images retain their old tables and launch measurements. | ||
|
|
||
| Use one memory node and match both the VP count and memory size to the image: |
There was a problem hiding this comment.
More of a general question, but why does the VP count and memory have to match? This isn't the case with HCL-based SNP VMs right? Seems inconvenient if you want to change your VM size.
There was a problem hiding this comment.
yeah it's because we hardcode the acpi tables & such in the IGVM file and make it part of your launch measurement. I have some changes that instead parse DT and generate acpi tables which we should probably take instead.
Place every VP and memory for a generated SNP direct boot guest inside NUMA node 0. This fixes a previous issue where we put VPs in their own NUMA node, which is fairly strange for a boot configuration.
A future change may instead regenerate all ACPI tables from device tree provided by the host, which is needed to support PCIe devices.