vmm_test: MANA VF reset and link-state events - #4304
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the OpenVMM Petri integration tests for OpenHCL Linux-direct MANA by adding VF reset and vport link-state coverage, backed by new GDMA test infrastructure that can inject HWC EQEs into the MANA emulator.
Changes:
- Add new OpenHCL Linux-direct x86_64 MANA NIC tests for VF reset (with/without VTL0 revoke) and link up/down notifications.
- Refactor GDMA vport resolution for reuse and introduce a
gdma_testdevice/resolver for out-of-band EQE injection. - Wire the new test resolver into OpenVMM resource registration and Petri OpenVMM backend configuration.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vmm_tests/vmm_tests/tests/tests/x86_64/openhcl_linux_direct/mana_nic.rs | New MANA NIC integration tests including VF reset and link-state notifications. |
| vmm_tests/vmm_tests/tests/tests/x86_64/openhcl_linux_direct.rs | Factor MANA NIC tests into a dedicated module. |
| vm/devices/net/gdma/src/resolver.rs | Extract reusable resolve_vports helper for GDMA vport resolution. |
| vm/devices/net/gdma/src/queues.rs | Expose ID_OFFSET within the crate for test helper usage. |
| vm/devices/net/gdma/src/lib.rs | Add test_helpers feature/module (vport resolve + EQE injection helper). |
| vm/devices/net/gdma/Cargo.toml | Add test_helpers feature flag. |
| vm/devices/net/gdma_test/src/resolver.rs | New GDMA test resolver that injects HWC EQEs based on test requests. |
| vm/devices/net/gdma_test/src/lib.rs | New gdma_test crate entry point. |
| vm/devices/net/gdma_test/Cargo.toml | New crate manifest for gdma_test. |
| vm/devices/net/gdma_resources/src/lib.rs | Add GdmaTestDeviceHandle and GdmaTestRequest resource types. |
| petri/src/vm/openvmm/modify.rs | Add Petri backend wiring for paired VTL2 test NIC + VTL0 VF and a ManaTestControl RPC interface. |
| petri/src/vm/openvmm/mod.rs | Re-export ManaTestControl. |
| openvmm/openvmm_resources/src/lib.rs | Register the GdmaTestDeviceResolver. |
| openvmm/openvmm_resources/Cargo.toml | Add dependency on gdma_test. |
| Cargo.toml | Add workspace dependency path entry for gdma_test. |
| Cargo.lock | Lockfile updates for the new gdma_test crate dependency graph. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
vm/devices/net/gdma/src/lib.rs:143
hwc_eq_injectorforwards an arbitrary&[u8]toQueues::post_eq, which panics ifdata.len() > 12(the size ofgdma_defs::Eqe::data). Since this helper is explicitly cross-crate, add an explicit length check (or otherwise bound the payload) so misuse doesn't cause an unexpected panic.
/// Returns a function that injects EQEs into the hardware channel EQ.
pub fn hwc_eq_injector(device: &GdmaDevice) -> impl Fn(u8, &[u8]) + Send + Sync + 'static {
let queues = device.queues.clone();
move |ty, data| queues.post_eq(queues::ID_OFFSET as u32, ty, data)
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
vm/devices/net/gdma_test/src/resolver.rs:78
encode_requestusesassert!(vport <= 0x00ff_ffff, ...), which will panic the VMM process if a caller sends an out-of-range vport index. Even if this is primarily test-driven, it’s easy to trip accidentally and turns a bad request into a hard crash.
Consider validating the vport index and gracefully ignoring/rejecting the request (e.g., log + skip injection) instead of panicking.
assert!(
vport <= 0x00ff_ffff,
"vport index does not fit in the 24-bit EQE field"
);
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
4e76803 to
b306d76
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
vmm_tests/vmm_tests/tests/tests/x86_64/openhcl_linux_direct/mana_nic.rs:74
mana_nic_shared_poolis currently identical tomana_nic(same backend modifications and assertions). As written it doesn’t exercise a distinct “shared pool” path, so it adds runtime without increasing coverage and the name is misleading.
/// Test an OpenHCL Linux direct VM with a MANA nic assigned to VTL2 (backed by
/// the MANA emulator), and vmbus relay.
#[openvmm_test(openhcl_linux_direct_x64)]
async fn mana_nic_shared_pool(
config: PetriVmBuilder<OpenVmmPetriBackend>,
) -> Result<(), anyhow::Error> {
let (vm, agent) = config
.with_vmbus_redirect(true)
.modify_backend(|b| b.with_nic())
.run()
.await?;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
vmm_tests/vmm_tests/tests/tests/x86_64/openhcl_linux_direct/mana_nic.rs:74
mana_nic_shared_poolcurrently uses the same VM/backend configuration and assertions asmana_nic(both callmodify_backend(|b| b.with_nic())and thenvalidate_mana_nic(..., false)). If this test is intended to exercise a distinct “shared pool” DMA path, it needs a configuration difference that actually enables that path; otherwise it’s redundant and the name is misleading (consider removing it or renaming it to reflect what it validates).
/// Test an OpenHCL Linux direct VM with a MANA nic assigned to VTL2 (backed by
/// the MANA emulator), and vmbus relay.
#[openvmm_test(openhcl_linux_direct_x64)]
async fn mana_nic_shared_pool(
config: PetriVmBuilder<OpenVmmPetriBackend>,
) -> Result<(), anyhow::Error> {
let (vm, agent) = config
.with_vmbus_redirect(true)
.modify_backend(|b| b.with_nic())
.run()
.await?;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (1)
vmm_tests/vmm_tests/tests/tests/x86_64/openhcl_linux_direct/mana_nic.rs:74
mana_nic_shared_poolduplicates the exact same VM configuration and assertions asmana_nic(same backend modifier, same validate call, same teardown). If the intent is to exercise a distinct “shared pool” DMA path, the test should apply a configuration knob that actually selects that path; otherwise this is just redundant coverage/time in the integration suite.
/// Test an OpenHCL Linux direct VM with a MANA nic assigned to VTL2 (backed by
/// the MANA emulator), and vmbus relay.
#[openvmm_test(openhcl_linux_direct_x64)]
async fn mana_nic_shared_pool(
config: PetriVmBuilder<OpenVmmPetriBackend>,
) -> Result<(), anyhow::Error> {
let (vm, agent) = config
.with_vmbus_redirect(true)
.modify_backend(|b| b.with_nic())
.run()
.await?;
| gdb = ["openvmm_resources/gdb"] | ||
| vendored_crypto = ["crypto/vendored"] | ||
| tpm = ["openvmm_resources/tpm"] | ||
| test_utilities = ["openvmm_resources/test_utilities"] |
There was a problem hiding this comment.
what's the reason behind a new feature? it makes us also have to take a bunch of flowey changes
There was a problem hiding this comment.
is the feature we should be exposing instead whether or not we should expose the gdma device? I'm not sure we need to really gate that behind a feature either?
There was a problem hiding this comment.
I didn't want the GdmaTestDeviceResolver to be built into the product, but you're right that neither NvmeFaultControllerResolver nor CxlTestDeviceResolver bother with that. I'm removing the feature.
There was a problem hiding this comment.
🔵 Needs a closer look
The PR description claims GDMA test code is excluded from production builds, but the new gdma_test dependency/resolver registration appears to be unconditional, so either gating or the description needs to be corrected.
Review details
Suppressed comments (1)
openvmm/openvmm_resources/src/lib.rs:92
- The PR description says production builds should not see GDMA test code, but
openvmm_resourcesnow unconditionally depends on and registersgdma_test::resolver::GdmaTestDeviceResolver. Since theopenvmmbinary always linksopenvmm_resources, this makes the test-controllable GDMA device (and thegdmacrate’stest_helpersfeature viagdma_test) part of normal builds unless it’s gated elsewhere.
Consider gating the gdma_test dependency + resolver registration behind a non-default feature (or cfg(test)/test-only build feature used by vmm_tests), or update the PR description to reflect that this test resolver is available in production builds.
// PCI devices
cxl_spec::test::resolver::CxlTestDeviceResolver,
gdma::resolver::GdmaDeviceResolver,
gdma_test::resolver::GdmaTestDeviceResolver,
nvme::resolver::NvmeControllerResolver,
- Files reviewed: 17/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new VPCI device control handler and GDMA filter/vport indexing paths have correctness issues that can break existing flows or mishandle guest-provided values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
vm/devices/net/gdma/src/bnic.rs:437
req.vportinManaMoveFilterVTL2PrivilegedReqis au64, but it is cast directly tousizefor indexing. On 32-bit targets (or for large values) this truncates and could update/query the wrong vport instead of rejecting the request.
vm/devices/net/gdma/src/bnic.rs:452ManaQueryFilterStateReq.vportis au64, but it is cast directly tousizefor indexing. This can truncate on 32-bit targets (or for large values) and report the filter state for the wrong vport instead of erroring.
vm/devices/net/gdma/src/lib.rs:149
hwc_eq_injectorforwards an arbitrary&[u8]intoqueues.post_eq, which panics ifdata.len() > 12(EQE payload size). Since this is exposed cross-crate for tests, it’s easy for a caller to accidentally crash the VMM with an oversized payload.
/// Returns a function that injects EQEs into the hardware channel EQ.
pub fn hwc_eq_injector(device: &GdmaDevice) -> impl Fn(u8, &[u8]) + Send + Sync + 'static {
let queues = Arc::downgrade(&device.queues);
move |ty, data| {
if let Some(queues) = queues.upgrade() {
queues.post_eq(queues::ID_OFFSET as u32, ty, data)
}
}
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
| fn handle_vpci_device_control(&mut self, message_buf: &[u8]) -> Result<(), Error> { | ||
| let request = get_protocol::VpciDeviceControlRequest::read_from_prefix(message_buf) | ||
| .map_err(|_| Error::MessageTooSmall)? | ||
| .0; | ||
| let status = if request.code == get_protocol::VpciDeviceControlCode::OFFER { | ||
| get_protocol::VpciDeviceControlStatus::SUCCESS | ||
| } else { | ||
| get_protocol::VpciDeviceControlStatus::INVALID_REQUEST | ||
| }; | ||
| let response = get_protocol::VpciDeviceControlResponse::new(status); | ||
| self.channel | ||
| .try_send(response.as_bytes()) | ||
| .map_err(Error::Vmbus) | ||
| } |
Adding to the VMM Petri integration test suite with GDMA infrastructure and Petri coverage for the MANA VF reset and link-state change behavior.