openvmm_entry: add explicit controller and relay CLI for storage#3607
Open
jstarks wants to merge 5 commits into
Open
openvmm_entry: add explicit controller and relay CLI for storage#3607jstarks wants to merge 5 commits into
jstarks wants to merge 5 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds explicit named storage controller management to openvmm_entry, replacing the previously implicit --disk/--nvme/uh/uh-nvme flows. New CLI flags --nvme-controller, --vmbus-scsi-controller, and --openhcl-controller register named controllers, and --disk gains controller=, nsid=, lun=, and relay= options to attach disks or relay them through OpenHCL. The OpenHCL relay path now looks up source/target controllers by name instead of using hardcoded instance IDs, and deterministic_guid is rewritten to use UUIDv8 with SHA-256.
Changes:
- New
StorageBuildercontroller maps (nvme_controllers,scsi_controllers,openhcl_controllers) plus aNamedDiskLocationandadd_relayflow. - CLI parsing for the three new controller types and the new
--diskoptions, with extensive parser unit tests. - Legacy
--disk/--nvme/uhpaths are kept but deprecated;build_underhillis renamed tobuild_openhcl_settingsand now consumes its LUN state viamem::take.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| openvmm/openvmm_entry/src/storage_builder.rs | Adds named controllers, relay path, UUIDv8 GUIDs; build_openhcl_settings now takes &mut self and consumes underhill LUN state. |
| openvmm/openvmm_entry/src/lib.rs | Registers named controllers before disk processing; routes --disk to Named/relay; emits deprecation warnings; pre-registers implicit PCIe controllers for legacy --nvme. |
| openvmm/openvmm_entry/src/cli_args.rs | New NvmeControllerCli/ScsiControllerCli/OpenhclControllerCli types, extended DiskCli fields, parser constraints, and tests. |
| openvmm/openvmm_entry/Cargo.toml | Adds sha2 workspace dependency for the new deterministic GUID derivation. |
| Cargo.lock | Records the new sha2 dependency edge. |
smalis-msft
reviewed
Jun 1, 2026
Contributor
smalis-msft
left a comment
smalis-msft
left a comment
There was a problem hiding this comment.
Overall looks fine by me, but copilot's still got some feedback
smalis-msft
approved these changes
Jun 1, 2026
The existing --disk and --nvme flags use implicit, unnamed controllers whose identity and VTL are determined by convention. This makes it impossible to precisely control controller placement, and the OpenHCL relay path (uh/uh-nvme) is tightly coupled to hardcoded instance IDs. Introduce explicit, named controller management: - --nvme-controller id=<name>,pcie_port=<port>|vpci[=<guid>][,vtl2] creates a named NVMe controller with an explicit transport and VTL. Accepts an optional pre-created mesh channel for runtime hot-plug. - --vmbus-scsi-controller id=<name>[,sub_channels=<N>][,vtl2] creates a named VMBus SCSI controller. - --openhcl-controller id=<name>,type=scsi|nvme[,guid=<guid>] registers an OpenHCL-managed controller as a relay target. - --disk gains controller=<name>, nsid=<N>, lun=<N>, and relay=<ctrl>[:<loc>] options. controller= attaches the disk to a named controller (NVMe or SCSI, resolved by type at add time). relay= routes the disk through OpenHCL to the named target controller, replacing the uh/uh-nvme mechanism for new usage. The storage builder is refactored to support these named controllers alongside the existing implicit paths. Named NVMe controllers carry VTL, transport, and an optional hot-plug channel. Named SCSI controllers carry VTL, instance ID, and sub-channel count. The relay path (add_relay) looks up the source controller to determine VTL and instance ID, adds the disk, then creates an OpenHCL LUN entry on the target controller. The deterministic_guid function is rewritten to use UUIDv8 (RFC 9562) with SHA-256 instead of DefaultHasher, which is not stable across Rust versions. --disk without controller=, --nvme, and uh/uh-nvme are deprecated with warnings pointing to the new flags. build_underhill is renamed to build_openhcl_settings.
smalis-msft
approved these changes
Jun 1, 2026
Comment on lines
202
to
+214
| pub fn has_vtl0_nvme(&self) -> bool { | ||
| !self.vtl0_nvme_namespaces.is_empty() || !self.underhill_nvme_luns.is_empty() | ||
| !self.vtl0_nvme_namespaces.is_empty() | ||
| || !self.underhill_nvme_luns.is_empty() | ||
| || self.controllers.values().any(|entry| { | ||
| matches!( | ||
| entry, | ||
| ControllerEntry::Nvme(nvme) | ||
| if nvme.vtl == DeviceVtl::Vtl0 | ||
| && matches!(nvme.transport, NvmeControllerTransport::Vpci(_)) | ||
| && !nvme.namespaces.is_empty() | ||
| ) | ||
| }) | ||
| } |
Comment on lines
+564
to
+584
| if !opt.nvme.is_empty() { | ||
| tracing::warn!("--nvme is deprecated; use --nvme-pci and --disk on=<name> instead"); | ||
|
|
||
| // Pre-register implicit PCIe controllers for unique port names. | ||
| let mut registered_ports = std::collections::BTreeSet::new(); | ||
| for disk in &opt.nvme { | ||
| if let Some(port) = &disk.pcie_port { | ||
| if registered_ports.insert(port.clone()) { | ||
| storage.add_nvme_controller( | ||
| port.clone(), | ||
| DeviceVtl::Vtl0, | ||
| storage_builder::NvmeControllerTransport::Pcie(port.clone()), | ||
| None, | ||
| ).with_context(|| format!( | ||
| "legacy --nvme flag conflicts with an explicit controller named '{port}'; \ | ||
| use --nvme-pci and --disk on=<name> instead" | ||
| ))?; | ||
| } | ||
| } | ||
| } | ||
| } |
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.
The existing
--diskand--nvmeflags use implicit, unnamed controllers whose identity and VTL are determined by convention. This makes it impossible to precisely control controller placement, and the OpenHCL relay path (uh/uh-nvme) is tightly coupled to hardcoded instance IDs.This PR introduces explicit, named controller management:
--nvme-pci id=<name>,pcie_port=<port>|vpci[=<guid>][,vtl2]creates a named NVMe controller with an explicit transport and VTL. Accepts an optional pre-created mesh channel for runtime hot-plug.--vmbus-scsi id=<name>[,sub_channels=<N>][,vtl2]creates a named VMBus SCSI controller.--openhcl-controller id=<name>,type=scsi|nvme[,guid=<guid>]registers an OpenHCL-managed controller as a relay target.--diskgainson=<name>,nsid=<N>,lun=<N>, andrelay=<ctrl>[:<loc>]options.on=attaches the disk to a named controller (NVMe or SCSI, resolved by type at add time).relay=routes the disk through OpenHCL to the named target controller, replacing theuh/uh-nvmemechanism for new usage.The storage builder is refactored to support these named controllers alongside the existing implicit paths.
--diskwithouton=,--nvme, anduh/uh-nvmeare deprecated with warnings pointing to the new flags.build_underhillis renamed tobuild_openhcl_settings.Guide documentation is updated throughout to use the new CLI syntax in examples.