openvmm: Add PCIe bus selection for virtio vsock - #4462
Anatol Belski (weltling) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The only noted gap is a non-blocking test-coverage nit.
Pull request overview
Adds pcie and pcie:PORT support for virtio-vsock bus selection.
Changes:
- Supports default and named PCIe root ports.
- Routes vsock devices through PCIe configuration.
- Documents the new syntax and adds parser tests.
File summaries
| File | Summary |
|---|---|
openvmm/openvmm_entry/src/lib.rs |
Maps PCIe vsock selections into device configuration. |
openvmm/openvmm_entry/src/cli_args.rs |
Adds parsing and tests for PCIe bus forms. |
Guide/src/reference/openvmm/management/cli.md |
Documents the new option syntax. |
Review details
Suppressed comments (1)
openvmm/openvmm_entry/src/lib.rs:1998
- The new behavior is covered only by parser assertions; no test exercises
vm_config_from_command_lineto verify thatpcie/pcie:PORTemits aPcieDeviceConfigfor the selected port (for either the Unix relay or Linux vhost backend). Add a focused configuration test so a regression cannot silently route vsock through the old bus path.
let (virtio_vsock_bus, virtio_vsock_pcie_port) = match &opt.virtio_vsock_bus {
None => (Some(VirtioBusCli::Auto), None),
Some(VirtioVsockBusCli::Mmio) => (Some(VirtioBusCli::Mmio), None),
Some(VirtioVsockBusCli::Pci) => (Some(VirtioBusCli::Pci), None),
Some(VirtioVsockBusCli::Pcie(port)) => (None, Some(port)),
- Files reviewed: 3/3 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.
c5b0990 to
fd57f82
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Reject extra colons in named PCIe ports and add a parser regression test.
Review details
Suppressed comments (1)
openvmm/openvmm_entry/src/cli_args.rs:1605
- This
split_onceaccepts values such aspcie:port:extraand storesport:extraas the port name.--pcie-root-portrejects names containing a second colon (viaPortNamePair), so this can never resolve and only fails later as a missing PCIe port; reject an additional:here (and add a parser regression test).
} else if let Some((bus, port)) = value.split_once(':')
&& bus.eq_ignore_ascii_case("pcie")
&& !port.is_empty()
{
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
fd57f82 to
b635801
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Add coverage for PCIe port mapping and address the documentation line-width nit.
Review details
Suppressed comments (2)
Guide/src/reference/openvmm/management/cli.md:242
- The new Guide line exceeds the repository's 80-character documentation width (
.github/instructions/guide-docs.instructions.md:16), which makes this option harder to scan in the CLI reference. Wrap the description beforecreated by.
* `--virtio-vsock-bus <mmio|pci|pcie[:PORT]>`: Select the bus for a virtio-vsock device
openvmm/openvmm_entry/src/lib.rs:1998
- The tests added here cover only parsing
VirtioVsockBusCli; the new mapping that places the resource inpcie_devicesis not exercised. Add a config-level or CLI integration test that requestspcie:<port>and asserts the resultingPcieDeviceConfiguses that port (including the vhost backend where supported), so a regression cannot silently return the device to the legacy virtio bus.
let (virtio_vsock_bus, virtio_vsock_pcie_port) = match &opt.virtio_vsock_bus {
None => (Some(VirtioBusCli::Auto), None),
Some(VirtioVsockBusCli::Mmio) => (Some(VirtioBusCli::Mmio), None),
Some(VirtioVsockBusCli::Pci) => (Some(VirtioBusCli::Pci), None),
Some(VirtioVsockBusCli::Pcie(port)) => (None, Some(port)),
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
b635801 to
3b019a2
Compare
3b019a2 to
a242a66
Compare
a242a66 to
91f837f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Document virtio-fs-shmem support and the shared pcie:PORT one-device restriction.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Guide/src/reference/openvmm/management/cli.md:238
--virtio-fs-busis also applied to--virtio-fs-shmeminvm_config_from_command_line, but this description only documents the per-devicepcie_portoverride for--virtio-fs. Mention--virtio-fs-shmemhere as well so users of the shared-memory form know that the same override is supported.
`auto`. A `pcie_port` prefix on `--virtio-fs` overrides this option.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
91f837f to
933cdd2
Compare
933cdd2 to
a5d9f09
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Reject duplicate PCIe port assignments across all routed devices and add focused fs/RNG routing tests.
Review details
Suppressed comments (2)
openvmm/openvmm_entry/src/lib.rs:1818
PcieDeviceConfigattaches only one endpoint per named PCIe port, so a second registration is rejected as an occupied port. This branch appends without checking existingpcie_devices; for example,--virtio-fs-bus pcie:footogether with--virtio-rng --virtio-rng-bus pcie:foo(or an explicitpcie_port=foo) produces two devices onfoo, while the precheck below only counts unprefixed filesystem devices. Track effective port names for all PCIe-routed devices and reject duplicates before building the config.
VirtioBusCli::Pcie(port_name) => pcie_devices.push(PcieDeviceConfig {
port_name,
resource: VirtioPciDeviceHandle(resource).into_resource(),
}),
openvmm/openvmm_entry/src/lib.rs:1880
- The new PCIe bus path is exercised only for vsock; the fs and RNG call sites are not covered by a config-building test. A regression that accidentally routes either
--virtio-fs-bus pcie:PORTor--virtio-rng-bus pcie:PORTthrough the legacy virtio list would therefore pass the current tests. Add a focused test (or table-driven case) that asserts these options producePcieDeviceConfigentries with the selected port.
add_virtio_device(opt.virtio_fs_bus.clone(), resource, &mut pcie_devices);
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
a5d9f09 to
dd0d076
Compare
dd0d076 to
4633833
Compare
Extend VirtioBusCli with named PCIe ports and route selected devices through PcieDeviceConfig. Apply shared bus selection to virtio fs and RNG. Reject one global PCIe port for multiple fs devices and document the accepted values. Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Linux direct boot can use a native PCIe topology without legacy PCI interrupt routing. Virtio vsock could only select generic PCI or MMIO, which left no way to attach it to a named PCIe root port. Extend --virtio-vsock-bus with pcie and pcie:PORT. Bare pcie selects root port vsock by default. The named form selects another root port. Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
4633833 to
f6bf554
Compare
|
I don't love the CLI options for this stuff. This doesn't compose well and we have too many options. But this seems like a reasonable extension to the mess we have right now. |
Chris Oo (chris-oo)
left a comment
There was a problem hiding this comment.
+1 to johns feedback but i think it's fine for now.
|
Thanks for the reviews! I was initially only thinking about exposing the vsock device over PCIe so it can be discovered more easily during direct kernel boot. However, a discussion with Daman showed there are others like virtio-rng that would benefit. Adding PCIe selection to them would bring a little more unity to the existing options, at least as long as they stay. And yep, I've also explicitly avoided targeting any new console options for exactly the same reason mentioned in the review :) |
Extend
VirtioBusCliwith named PCIe ports and route selected devices throughPcieDeviceConfig.Apply shared bus selection to virtio fs and RNG. Reject one global PCIe port for multiple fs devices and document the accepted values.
Linux direct boot can use a native PCIe topology without legacy PCI interrupt routing. Virtio vsock could only select generic PCI or MMIO, which left no way to attach it to a named PCIe root port.
Extend
--virtio-vsock-buswithpcieandpcie:PORT. Barepcieselects root portvsockby default. The named form selects another root port.