petri: add NVMe emulator support#3378
Open
babayet2 wants to merge 17 commits intomicrosoft:mainfrom
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Hyper-V NVMe coverage in the Petri-based vmm_tests suite by provisioning NVMe devices via a closed-source Hyper-V “NVMe emulator” and wiring it through the Hyper-V Petri backend.
Changes:
- Adds an (unstable) Hyper-V OpenHCL Linux storage test that validates discovery + IO via an NVMe emulator device.
- Extends the Hyper-V Petri backend to translate
VmbusStorageType::Nvmeinto post-create NVMe emulator device attachment. - Adds PowerShell-side plumbing to attach NVMe emulator devices after
New-CustomVMcompletes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
vmm_tests/vmm_tests/tests/tests/x86_64/storage.rs |
Adds an unstable Hyper-V NVMe emulator storage test that exercises discovery and IO. |
petri/src/vm/hyperv/mod.rs |
Maps VmbusStorageType::Nvme controllers to NVMe emulator device configs for Hyper-V backend runs. |
petri/src/vm/hyperv/powershell.rs |
Adds nvme_emulator_devices to VM creation args and attaches them post-create via PowerShell. |
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
petri/src/vm/hyperv/mod.rs:389
ide_controllersis still computed above, but it is no longer passed intoHyperVNewCustomVMArgs(onlystorage_controllersis set). This will silently drop IDE drive configuration for Hyper-V VMs and also leaves an unusedide_controllerslocal. Include theide_controllersfield inhyperv_args(or remove the mapping if IDE is intentionally unsupported).
guest_state_path,
storage_controllers,
com_3: supports_com3,
imc_hiv,
management_vtl_settings,
..HyperVNewCustomVMArgs::from_config(&config, &properties)?
};
let vm = HyperVVM::new(hyperv_args, log_source.clone(), driver.clone()).await?;
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 27, 2026
tjones60
reviewed
Apr 28, 2026
tjones60
reviewed
Apr 28, 2026
tjones60
reviewed
Apr 28, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
petri/src/vm/hyperv/mod.rs:365
ide_controllersis constructed above but never passed intoHyperVNewCustomVMArgs(and the struct update default setside_controllersto empty). This looks like a functional regression: IDE disks/DVDs from the firmware config will no longer be attached on Hyper-V, and the localide_controllersnow becomes an unused variable. Passide_controllersintohyperv_args(or remove the mapping if IDE is intentionally unsupported here).
let hyperv_args = HyperVNewCustomVMArgs {
firmware_file: igvm_file.clone(),
firmware_parameters: openhcl_command_line,
guest_state_path,
storage_controllers,
com_3: supports_com3,
imc_hiv,
management_vtl_settings,
..HyperVNewCustomVMArgs::from_config(&config, &properties)?
};
| if ($NvmeControllers) { | ||
| foreach ($controller in $NvmeControllers.GetEnumerator()) { | ||
| foreach ($vhdPath in $controller.Value["Drives"]) { | ||
| icacls $vhdPath /grant "NT VIRTUAL MACHINE\${vmid}:(F)" | Out-Null |
There was a problem hiding this comment.
The icacls call's result is discarded (| Out-Null) and failures won't stop VM creation. If ACL granting fails, the NVMe emulator will likely fail later with a harder-to-diagnose error. Consider checking $LASTEXITCODE (or using -ErrorAction Stop equivalents) and throwing a clear error that includes the VHD path and VMID.
Suggested change
| icacls $vhdPath /grant "NT VIRTUAL MACHINE\${vmid}:(F)" | Out-Null | |
| $icaclsOutput = icacls $vhdPath /grant "NT VIRTUAL MACHINE\${vmid}:(F)" 2>&1 | |
| $icaclsExitCode = $LASTEXITCODE | |
| if ($icaclsExitCode -ne 0) { | |
| $icaclsOutputText = ($icaclsOutput | Out-String).Trim() | |
| throw ("Failed to grant NVMe VHD access with icacls for VHD path '{0}' and VMID '{1}' (exit code {2}). icacls output: {3}" -f ` | |
| $vhdPath, | |
| $vmid, | |
| $icaclsExitCode, | |
| $icaclsOutputText) | |
| } |
Comment on lines
+690
to
+703
| // Sort drives by namespace ID and validate they are exactly | ||
| // 1..N — the emulator assigns NSIDs sequentially by VHD | ||
| // argument order. | ||
| let mut sorted_drives: Vec<_> = drives.into_iter().collect(); | ||
| sorted_drives.sort_by_key(|(nsid, _)| *nsid); | ||
| let expected: Vec<u32> = (1..=sorted_drives.len() as u32).collect(); | ||
| let actual: Vec<u32> = sorted_drives.iter().map(|(nsid, _)| *nsid).collect(); | ||
| anyhow::ensure!( | ||
| actual == expected, | ||
| "NVMe namespace IDs must be 1..{}, got {:?}", | ||
| expected.len(), | ||
| actual | ||
| ); | ||
| nvme_entries.push(( |
There was a problem hiding this comment.
NVMe controllers with an empty drives map will currently serialize as Drives = @() and pass the actual == expected check (both empty). That likely results in a confusing failure from New-NvmeEmulatorRasd. Consider explicitly rejecting empty NVMe controllers with a clear error (including VSID).
Contributor
There was a problem hiding this comment.
What is the behavior if you have an empty NVMe controller? Is that valid?
tjones60
reviewed
May 1, 2026
tjones60
reviewed
May 1, 2026
tjones60
reviewed
May 1, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
petri/src/vm/hyperv/mod.rs:365
ide_controllersis still built above, but it is no longer passed intoHyperVNewCustomVMArgs(the struct update only setsstorage_controllers). This drops IDE controller configuration for the Hyper-V backend and likely breaks any VM config that relies on IDE (and also leaveside_controllersunused). Pass the computedide_controllersinto the args struct again.
let hyperv_args = HyperVNewCustomVMArgs {
firmware_file: igvm_file.clone(),
firmware_parameters: openhcl_command_line,
guest_state_path,
storage_controllers,
com_3: supports_com3,
imc_hiv,
management_vtl_settings,
..HyperVNewCustomVMArgs::from_config(&config, &properties)?
};
| .filter(|c| matches!(c.controller_type, crate::VmbusStorageType::Nvme)) | ||
| .flat_map(|c| c.drives.values()) | ||
| .filter_map(|drive| match &drive.disk { | ||
| Some(Disk::Persistent(path)) => Some(path), |
Comment on lines
+695
to
+699
| let expected: Vec<u32> = (1..=sorted_drives.len() as u32).collect(); | ||
| let actual: Vec<u32> = sorted_drives.iter().map(|(nsid, _)| *nsid).collect(); | ||
| anyhow::ensure!( | ||
| actual == expected, | ||
| "NVMe namespace IDs must be 1..{}, got {:?}", |
Comment on lines
+703
to
+714
| nvme_entries.push(( | ||
| format!("\"{vsid}\""), | ||
| ps::Value::new(ps::HashTable::new([ | ||
| ("Vtl", ps::Value::new(target_vtl as u32)), | ||
| ( | ||
| "Drives", | ||
| ps::Value::new(ps::Array::new(sorted_drives.into_iter().map( | ||
| |(_, HyperVDrive { disk, .. })| { | ||
| disk.expect("NVMe drives must have disk paths") | ||
| }, | ||
| ))), | ||
| ), |
| let scsi_instance = Guid::new_random(); | ||
| const NVME_DISK_SECTORS: u64 = 0x5_0000; | ||
| const SECTOR_SIZE: u64 = 512; | ||
| const EXPECTED_NVME_DISK_SIZE_BYTES: u64 = NVME_DISK_SECTORS * SECTOR_SIZE; |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
petri/src/vm/hyperv/mod.rs:365
ide_controllersis built from the firmware config above, but it isn’t passed intohyperv_argsanymore (it falls back to the default empty map via..from_config). This looks like a functional regression for any VM that relies on IDE attachments. Includeide_controllersinHyperVNewCustomVMArgsconstruction (or remove the mapping if IDE is intentionally unsupported).
let hyperv_args = HyperVNewCustomVMArgs {
firmware_file: igvm_file.clone(),
firmware_parameters: openhcl_command_line,
guest_state_path,
storage_controllers,
com_3: supports_com3,
imc_hiv,
management_vtl_settings,
..HyperVNewCustomVMArgs::from_config(&config, &properties)?
};
tjones60
reviewed
May 5, 2026
tjones60
reviewed
May 5, 2026
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.
Add coverage for HyperV NVMe devices leveraging a closed source emulator