From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id A389A1FF0AA for ; Tue, 22 Sep 2026 12:56:22 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 167D821602; Tue, 22 Sep 2026 12:55:57 +0200 (CEST) From: Dominik Csapak To: pve-devel@lists.proxmox.com Subject: [PATCH pve-qemu-server-rs 2/9] pci: add machine abstraction and PCI bridge generation Date: Tue, 22 Sep 2026 12:55:33 +0200 Message-ID: <20260922105550.2084078-3-d.csapak@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260922105550.2084078-1-d.csapak@proxmox.com> References: <20260922105550.2084078-1-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.543 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_MAILER 2 Automated Mailer Tag Left in Email RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: UVM5UIA7WGIFVMM7RXUFY4LXE6XNP3T7 X-Message-ID-Hash: UVM5UIA7WGIFVMM7RXUFY4LXE6XNP3T7 X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Besides the address of a single device, callers also need the list of bridge devices a guest has to be started with. Which bridges those are depends on more than one property of the guest config, so pass them in as a `Machine` struct rather than as a growing list of parameters. It can be extended with the layout variant and other guest wide settings later on, without touching every call site again. The bridge list itself follows the Perl implementation: q35 machines already define the bridges up to three in their machine definition, pci.3 is only needed for virtio-scsi-single, and pci.4 only once a SCSI controller above index one exists or the machine version is new enough to always include it. Signed-off-by: Dominik Csapak --- pve-qemu-server-pci/Cargo.toml | 1 + pve-qemu-server-pci/src/layout/legacy.rs | 141 ++++++++++++++++++++++- pve-qemu-server-pci/src/lib.rs | 18 +++ pve-qemu-server-pci/src/machine.rs | 99 ++++++++++++++++ 4 files changed, 255 insertions(+), 4 deletions(-) create mode 100644 pve-qemu-server-pci/src/machine.rs diff --git a/pve-qemu-server-pci/Cargo.toml b/pve-qemu-server-pci/Cargo.toml index d3f4401..60c4ed1 100644 --- a/pve-qemu-server-pci/Cargo.toml +++ b/pve-qemu-server-pci/Cargo.toml @@ -10,6 +10,7 @@ exclude.workspace = true rust-version.workspace = true [dependencies] +log.workspace = true strum.workspace = true pve-api-types.workspace = true diff --git a/pve-qemu-server-pci/src/layout/legacy.rs b/pve-qemu-server-pci/src/layout/legacy.rs index 40b3265..3f544bc 100644 --- a/pve-qemu-server-pci/src/layout/legacy.rs +++ b/pve-qemu-server-pci/src/layout/legacy.rs @@ -2,7 +2,7 @@ use pve_api_types::ClusterResourceHostArch; use crate::constants::BRIDGE_SLOT_NUM; use crate::layout::bridge_slots; -use crate::{Bus, Device, DeviceLayout, Function, PciConfigError, Slot}; +use crate::{Bus, Device, DeviceLayout, Function, Machine, PciConfigError, Slot}; const PCI_1: [Slot; BRIDGE_SLOT_NUM] = bridge_slots(&[ single_device!(Net(6)), @@ -369,6 +369,65 @@ pub(crate) fn print_pcie_root_port(index: u8) -> Result Ok(res) } +/// Looks up the PCI bridge a device sits on, see +/// [`crate::get_pci_bridge_for_device`]. +pub(crate) fn get_pci_bridge_for_device(id: &str) -> Option { + let new_id = map_legacy_pci_id(id); + // the root bus is never a bridge we have to add ourselves, so it does not + // matter whether it is PCI or PCIe here + let layout = get_legacy_pci_layout(false, is_virtio_scsi_single_id(new_id)); + let device = new_id.parse().ok()?; + let addr = layout.find_device(&device).ok()?; + match addr.bus { + Bus::Pci(i) => Some(i), + _ => None, + } +} + +/// Renders the bridge devices a guest needs, see +/// [`crate::get_pci_bridges`]. +pub(crate) fn get_pci_bridges(machine: &Machine) -> Vec { + // at most four bridges, each contributing a flag and its value + let mut res = Vec::with_capacity(8); + + // some SCSI controllers can only have 7 disks each, so scsi14 and upwards + // need scsihw2 and above, which live on pci.4 + let include_pci4 = machine.version_at_least(11, 1, 0) || machine.max_scsihw > 1; + + for i in 1..=4 { + // q35.cfg already includes the bridges up to three + if i < 4 && machine.q35 { + continue; + } + + if i == 3 && !machine.virtio_scsi_single() { + continue; + } + + if i == 4 && !include_pci4 { + continue; + } + + let id = if i == 2 && machine.legacy_igd { + format!("pci.{i}-igd") + } else { + format!("pci.{i}") + }; + + let addr = match print_pci_addr(&id, machine.arch) { + Ok(addr) => addr, + Err(err) => { + log::warn!("could not find address for bridge {id}: {err}"); + continue; + } + }; + res.push("-device".to_string()); + res.push(format!("pci-bridge,id=pci.{i},chassis_nr={i}{addr}")); + } + + res +} + #[cfg(test)] pub(crate) mod test { use pve_api_types::ClusterResourceHostArch; @@ -376,10 +435,10 @@ pub(crate) mod test { use std::collections::HashSet; use super::{ - LEGACY_PCIE_LAYOUT, get_legacy_pci_layout, print_pci_addr, print_pcie_addr, - print_pcie_root_port, + LEGACY_PCIE_LAYOUT, get_legacy_pci_layout, get_pci_bridge_for_device, get_pci_bridges, + print_pci_addr, print_pcie_addr, print_pcie_root_port, }; - use crate::{Bus, Device, DeviceLayout, PciAddress}; + use crate::{Bus, Device, DeviceLayout, Machine, PciAddress}; use crate::constants::MAX_HOSTPCI_DEVICES; @@ -652,4 +711,78 @@ pub(crate) mod test { assert_buses_before_devices(&get_legacy_pci_layout(false, true)); assert_buses_before_devices(&get_legacy_pci_layout(true, true)); } + + #[test] + /// The bus of a device has to agree with the address it is given. Note + /// that the root bus is reported as bus zero, not as no bus at all. + fn test_pci_bridge_for_device() { + for (input, bus, _) in LEGACY_ADDRS { + assert_eq!(get_pci_bridge_for_device(input), Some(bus), "for {input}"); + } + + assert_eq!(get_pci_bridge_for_device("no-such-device"), None); + } + + fn machine(q35: bool, scsihw: Option<&str>, max_scsihw: u8, legacy_igd: bool) -> Machine { + Machine::new( + "x86_64", + q35, + scsihw.map(str::to_string), + max_scsihw, + legacy_igd, + None, + 9, + 2, + None, + ) + } + + fn bridge(nr: u8, addr: &str) -> [String; 2] { + [ + "-device".to_string(), + format!("pci-bridge,id=pci.{nr},chassis_nr={nr}{addr}"), + ] + } + + #[test] + fn test_pci_bridges() { + let pci1 = bridge(1, ",bus=pci.0,addr=0x1e"); + let pci2 = bridge(2, ",bus=pci.0,addr=0x1f"); + let pci3 = bridge(3, ",bus=pci.0,addr=0x5"); + let pci4 = bridge(4, ",bus=pci.1,addr=0x1c"); + + let test = |machine: Machine, expected: &[&[String; 2]]| { + let expected: Vec = expected.iter().flat_map(|b| b.iter().cloned()).collect(); + assert_eq!(get_pci_bridges(&machine), expected); + }; + + test(machine(false, None, 0, false), &[&pci1, &pci2]); + + // q35 already brings the bridges up to three along + test(machine(true, None, 0, false), &[]); + + // every SCSI disk gets its own controller on pci.3 + test( + machine(false, Some("virtio-scsi-single"), 0, false), + &[&pci1, &pci2, &pci3], + ); + + // scsihw2 and above live on pci.4 + test(machine(false, Some("lsi"), 1, false), &[&pci1, &pci2]); + test( + machine(false, Some("lsi"), 2, false), + &[&pci1, &pci2, &pci4], + ); + test(machine(true, Some("lsi"), 2, false), &[&pci4]); + + // pci.4 is always added from machine version 11.1 on + let mut new_machine = machine(false, None, 0, false); + new_machine.version = (11, 1, None); + test(new_machine, &[&pci1, &pci2, &pci4]); + + // with legacy IGD passthrough pci.2 moves behind pci.1 + let mut expected_igd = pci2.clone(); + expected_igd[1] = "pci-bridge,id=pci.2,chassis_nr=2,bus=pci.1,addr=0x1e".to_string(); + test(machine(false, None, 0, true), &[&pci1, &expected_igd]); + } } diff --git a/pve-qemu-server-pci/src/lib.rs b/pve-qemu-server-pci/src/lib.rs index 5e8b8a7..7d0bd92 100644 --- a/pve-qemu-server-pci/src/lib.rs +++ b/pve-qemu-server-pci/src/lib.rs @@ -10,6 +10,9 @@ pub mod constants; +mod machine; +pub use machine::Machine; + mod types; use pve_api_types::ClusterResourceHostArch as Arch; pub use types::*; @@ -42,3 +45,18 @@ pub fn print_pcie_addr(id: &str) -> Result { pub fn print_pcie_root_port(index: u8) -> Result { layout::legacy::print_pcie_root_port(index) } + +/// Returns the number of the PCI bridge `id` sits on, or `None` if it is on +/// the root bus or not a known device. +pub fn get_pci_bridge_for_device(id: &str) -> Option { + layout::legacy::get_pci_bridge_for_device(id) +} + +/// Returns the QEMU arguments for all PCI bridges `machine` needs, as +/// alternating `-device` flags and their values. +/// +/// Bridges whose address cannot be determined are skipped with a warning +/// rather than failing the whole guest. +pub fn get_pci_bridges(machine: &Machine) -> Vec { + layout::legacy::get_pci_bridges(machine) +} diff --git a/pve-qemu-server-pci/src/machine.rs b/pve-qemu-server-pci/src/machine.rs new file mode 100644 index 0000000..10ee9db --- /dev/null +++ b/pve-qemu-server-pci/src/machine.rs @@ -0,0 +1,99 @@ +use pve_api_types::{ClusterResourceHostArch as Arch, QemuConfigOstype, QemuConfigScsihw}; + +/// The parts of a guest configuration that influence which PCI layout is used +/// and which quirks have to be applied to it. +/// +/// This is deliberately not the full guest config: it only carries what the +/// address generation needs, so that callers can build it from whatever +/// configuration representation they have. +#[derive(Clone, Debug)] +pub struct Machine { + pub arch: Arch, + pub q35: bool, + pub scsihw: Option, + /// Highest `scsihw` controller index in use. Controllers from index 2 on + /// live on `pci.4`, so this decides whether that bridge is needed. + pub max_scsihw: u8, + pub legacy_igd: bool, + pub ostype: Option, + /// QEMU machine version as `(major, minor, pve)`, where the PVE specific + /// revision is optional. + pub version: (u16, u16, Option), +} + +impl Machine { + /// Builds a [`Machine`] from the raw configuration values. + /// + /// Unparsable `arch`, `scsihw` and `ostype` values are not an error here: + /// `arch` falls back to x86_64 and the other two to `None`, which is the + /// same defaulting the Perl implementation does. + #[allow(clippy::too_many_arguments)] + pub fn new( + arch: &str, + q35: bool, + scsihw: Option, + max_scsihw: u8, + legacy_igd: bool, + ostype: Option, + major: u16, + minor: u16, + pve: Option, + ) -> Self { + let arch = arch.parse().unwrap_or(Arch::X8664); + let scsihw = scsihw.and_then(|hw| hw.parse().ok()); + let ostype = ostype.and_then(|ostype| ostype.parse().ok()); + Self { + arch, + q35, + scsihw, + max_scsihw, + legacy_igd, + ostype, + version: (major, minor, pve), + } + } + + /// Whether the guest's root bus is PCIe. True for q35 machines and for + /// aarch64, which only has a PCIe host bridge. + pub fn is_pcie(&self) -> bool { + self.q35 || self.arch == Arch::Aarch64 + } + + /// Whether each SCSI disk gets its own controller, which needs the extra + /// `pci.3` bridge. + pub fn virtio_scsi_single(&self) -> bool { + self.scsihw == Some(QemuConfigScsihw::VirtioScsiSingle) + } + + /// Whether the machine version is at least `major.minor.pve`. + /// + /// A machine without a PVE revision counts as being at least any requested + /// one, since the plain QEMU version already implies all PVE changes made + /// for it. + pub fn version_at_least(&self, major: u16, minor: u16, pve: u16) -> bool { + if self.version.0 > major { + return true; + } + if self.version.0 < major { + return false; + } + + if self.version.1 > minor { + return true; + } + if self.version.1 < minor { + return false; + } + + if let Some(our_pve) = self.version.2 { + if our_pve > pve { + return true; + } + if our_pve < pve { + return false; + } + } + + true + } +} -- 2.47.3