RFC: allow command line to specify controller + lun/nsid by mattkur · Pull Request #2399 · microsoft/openvmm

mattkur · 2025-11-12T17:49:40Z

I was surprised that adding additional uh-nvme disks didn't add multiple controllers, so add that capability.

I lost steam when trying to hook this up to the openvmm storage builder, but sending this out as an
RFC on the command line options format.

…llows manual repro of many-controllers)

github-actions · 2025-11-12T17:50:07Z

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Copilot

Pull Request Overview

This RFC adds support for specifying controller and lun/nsid values via command line when configuring disks for OpenVMM. The PR introduces a new NvmeControllerType enum to distinguish between VPCI and PCIe controllers, and updates the UnderhillDiskSource enum to include controller information. However, the implementation is incomplete as noted by the author, with broken code in the storage builder.

Key changes:

New command line syntax for uh and uh-nvme options supporting lun/nsid and controller specification
Introduction of NvmeControllerType enum to represent VPCI vs PCIe controllers
Refactored UnderhillDiskSource to include lun/nsid and controller information

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.

File	Description
openvmm/openvmm_entry/src/cli_args.rs	Adds parsing logic for new `uh` and `uh-nvme` syntax with controller/lun/nsid support, includes comprehensive test coverage
openvmm/openvmm_entry/src/storage_builder.rs	Introduces `NvmeControllerType` enum and updates storage handling; contains incomplete/broken implementation
openvmm/openvmm_entry/src/lib.rs	Updates to use new types and pass controller information through

Copilot · 2025-11-12T17:54:25Z

+                let def = (NamespaceDefinition {
                    nsid,
                    disk,
                    read_only,
                });


The tuple syntax is incorrect. Remove the wrapping parenthesis around NamespaceDefinition - it should be let def = NamespaceDefinition { ... }; instead of let def = (NamespaceDefinition { ... });

Suggested change

let def = (NamespaceDefinition {

nsid,

disk,

read_only,

});

let def = NamespaceDefinition {

nsid,

disk,

read_only,

};

Copilot · 2025-11-12T17:54:26Z

+                match (vtl, controller) {
+                    // VPCI
+                    (DeviceVtl::Vtl0, NvmeControllerType::Vpci(_)) => {
+                        &mut self.vtl0_nvme_namespaces
+                    }
+                    (DeviceVtl::Vtl1, NvmeControllerType::Vpci(_)) => {
+                        anyhow::bail!("vtl1 vpci unsupported")
+                    }
+                    (DeviceVtl::Vtl2, NvmeControllerType::Vpci(_)) => {
+                        &mut self.vtl2_nvme_namespaces
+                    }
+                    // PCIe
+                    (DeviceVtl::Vtl0, NvmeControllerType::Pcie(port)) => {
+                        self.pcie_nvme_controllers.entry(port).or_default()
+                    }
+                    (DeviceVtl::Vtl1, NvmeControllerType::Pcie(_)) => {
+                        anyhow::bail!("vtl1 pcie unsupported")
+                    }
+                    (DeviceVtl::Vtl2, NvmeControllerType::Pcie(_)) => {
+                        anyhow::bail!("vtl2 pcie unsupported")
+                    }
+
+                }
+
+                let namespaces = match (vtl, controller) {
+                };
+                let nsid = nsid.unwrap_or(namespaces.len() as u32 + 1);
+                namespaces.push);


This code is incomplete and syntactically broken. Lines 235-257 perform a match that returns namespace references but the result is not assigned. Lines 259-260 contain an empty match statement. Line 262 has malformed push syntax. The logic should assign the first match to let namespaces = ... and use def (not the tuple version) with the proper push syntax: namespaces.push((controller_vsid, def)); where controller_vsid is extracted from the NvmeControllerType.

Copilot · 2025-11-12T17:54:26Z

+            ("uh-nvme", UnderhillDiskSource::Nvme(None, None)),
+            ("uh-nvme=nsid:0", UnderhillDiskSource::Nvme(Some(0), None)),
+            ("uh-nvme=nsid:5", UnderhillDiskSource::Nvme(Some(5), None)),
+            (
+                "uh-nvme=nsid:205",
+                UnderhillDiskSource::Nvme(Some(205), None),
+            ),
+            (
+                "uh-nvme=controller:abcd",
+                UnderhillDiskSource::Nvme(None, Some("abcd".to_string())),
+            ),
+            (
+                "uh-nvme=nsid:5;controller:abcd",
+                UnderhillDiskSource::Nvme(Some(5), Some("abcd".to_string())),


Test expectations are incorrect. UnderhillDiskSource::Nvme expects (Option<u32>, NvmeControllerType) but these test cases use None and Some(String) for the second parameter. They should use NvmeControllerType::Vpci(None) and NvmeControllerType::Vpci(Some(\"abcd\".to_string())) respectively.

Suggested change

("uh-nvme", UnderhillDiskSource::Nvme(None, None)),

("uh-nvme=nsid:0", UnderhillDiskSource::Nvme(Some(0), None)),

("uh-nvme=nsid:5", UnderhillDiskSource::Nvme(Some(5), None)),

(

"uh-nvme=nsid:205",

UnderhillDiskSource::Nvme(Some(205), None),

),

(

"uh-nvme=controller:abcd",

UnderhillDiskSource::Nvme(None, Some("abcd".to_string())),

),

(

"uh-nvme=nsid:5;controller:abcd",

UnderhillDiskSource::Nvme(Some(5), Some("abcd".to_string())),

("uh-nvme", UnderhillDiskSource::Nvme(None, NvmeControllerType::Vpci(None))),

("uh-nvme=nsid:0", UnderhillDiskSource::Nvme(Some(0), NvmeControllerType::Vpci(None))),

("uh-nvme=nsid:5", UnderhillDiskSource::Nvme(Some(5), NvmeControllerType::Vpci(None))),

(

"uh-nvme=nsid:205",

UnderhillDiskSource::Nvme(Some(205), NvmeControllerType::Vpci(None)),

),

(

"uh-nvme=controller:abcd",

UnderhillDiskSource::Nvme(None, NvmeControllerType::Vpci(Some("abcd".to_string()))),

),

(

"uh-nvme=nsid:5;controller:abcd",

UnderhillDiskSource::Nvme(Some(5), NvmeControllerType::Vpci(Some("abcd".to_string()))),

Copilot · 2025-11-12T17:54:26Z

+            ),
+            (
+                "uh-nvme=controller:abcd",
+                UnderhillDiskSource::Nvme(None, Some("abcd".to_string())),


Test expectations are incorrect. UnderhillDiskSource::Nvme expects (Option<u32>, NvmeControllerType) but these test cases use None and Some(String) for the second parameter. They should use NvmeControllerType::Vpci(None) and NvmeControllerType::Vpci(Some(\"abcd\".to_string())) respectively.

Copilot · 2025-11-12T17:54:27Z

+            ),
+            (
+                "uh-nvme=nsid:5;controller:abcd",
+                UnderhillDiskSource::Nvme(Some(5), Some("abcd".to_string())),


Test expectations are incorrect. UnderhillDiskSource::Nvme expects (Option<u32>, NvmeControllerType) but these test cases use None and Some(String) for the second parameter. They should use NvmeControllerType::Vpci(None) and NvmeControllerType::Vpci(Some(\"abcd\".to_string())) respectively.

Copilot · 2025-11-12T17:54:27Z

+            DiskCli::from_str("file:disk.vhd,uh-nvme")
+                .unwrap()
+                .underhill,
+            Some(UnderhillDiskSource::Nvme(None, None))


Type mismatch: second parameter should be NvmeControllerType::Vpci(None) instead of None.

Copilot · 2025-11-12T17:54:27Z

+            Some(UnderhillDiskSource::Nvme(
+                Some(10),
+                Some("ctrl1".to_string())
+            ))


Type mismatch: second parameter should be NvmeControllerType::Vpci(Some(\"ctrl1\".to_string())) instead of Some(\"ctrl1\".to_string()).

Copilot · 2025-11-12T17:54:28Z

+            Some(UnderhillDiskSource::Nvme(
+                Some(10),
+                Some("ctrl1".to_string())
+            ))


Type mismatch: second parameter should be NvmeControllerType::Vpci(Some(\"ctrl1\".to_string())) instead of Some(\"ctrl1\".to_string()).

Copilot · 2025-11-12T17:54:28Z

-            underhill,
-            storage_builder::DiskLocation::Nvme(None, pcie_port.clone()),
+            underhill.clone(),
+            storage_builder::DiskLocation::Nvme(None, NvmeControllerType::Pcie(pcie_port.clone())),


Type error: pcie_port is Option<String>, but NvmeControllerType::Pcie expects String. This needs to handle the None case, either by unwrapping with a default value or by changing the logic to use NvmeControllerType::Vpci when pcie_port is None.

Suggested change

storage_builder::DiskLocation::Nvme(None, NvmeControllerType::Pcie(pcie_port.clone())),

storage_builder::DiskLocation::Nvme(

None,

match pcie_port {

Some(ref port) => NvmeControllerType::Pcie(port.clone()),

None => NvmeControllerType::Vpci,

},

),

Copilot · 2025-11-12T17:54:28Z

+                Ok(UnderhillDiskSource::Scsi(lun, controller_id))
+            }
+            (_, _) => {
+                anyhow::bail!("invalid underhill disk source '{option}', '{value:?}' found")


Error message is unclear - the format suggests both option and value are always present, but value is an Option. Consider: anyhow::bail!(\"invalid underhill disk source: '{option}'\" + value.map_or(String::new(), |v| format!(\" with value '{v}'\")))

Suggested change

anyhow::bail!("invalid underhill disk source '{option}', '{value:?}' found")

anyhow::bail!(

"invalid underhill disk source: '{option}'{}",

value.map_or(String::new(), |v| format!(" with value '{v}'"))

)

mattkur · 2026-03-12T13:44:36Z

I'm not going to finish this anytime soon.

openvmm_entry: allow command line to specify controller + lun/nsid (a…

4020f68

…llows manual repro of many-controllers)

mattkur requested a review from a team as a code owner November 12, 2025 17:49

mattkur requested review from a team and Copilot November 12, 2025 17:49

mattkur marked this pull request as draft November 12, 2025 17:49

github-actions Bot added the unsafe Related to unsafe code label Nov 12, 2025

Merge branch 'main' into controllers-via-cmdline

c560c24

Copilot started reviewing on behalf of mattkur November 12, 2025 17:50 View session

Copilot finished reviewing on behalf of mattkur November 12, 2025 17:51

Copilot AI reviewed Nov 12, 2025

View reviewed changes

mattkur closed this Mar 12, 2026

-            storage_builder::DiskLocation::Nvme(None, NvmeControllerType::Pcie(pcie_port.clone())),
+            storage_builder::DiskLocation::Nvme(
+                None,
+                match pcie_port {
+                    Some(ref port) => NvmeControllerType::Pcie(port.clone()),
+                    None => NvmeControllerType::Vpci,
+                },
+            ),

Conversation

mattkur commented Nov 12, 2025

Uh oh!

github-actions Bot commented Nov 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

mattkur commented Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants