From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id EEDF11FF09B for ; Mon, 28 Sep 2026 14:55:17 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 6BC1A21616; Mon, 28 Sep 2026 14:55:15 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Mon, 28 Sep 2026 14:55:10 +0200 Message-Id: Subject: Re: [PATCH pve-qemu-server-rs 2/9] pci: add machine abstraction and PCI bridge generation From: "Elias Huhsovitz" To: "Dominik Csapak" , Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.20.0 References: <20260922105550.2084078-1-d.csapak@proxmox.com> <20260922105550.2084078-3-d.csapak@proxmox.com> In-Reply-To: <20260922105550.2084078-3-d.csapak@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790600110283 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.398 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: FY7Y3UEJOXKDK55YGDNVBCWKQZ3PEU5F X-Message-ID-Hash: FY7Y3UEJOXKDK55YGDNVBCWKQZ3PEU5F X-MailFrom: e.huhsovitz@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: Comments inline. On Tue Sep 22, 2026 at 12:55 PM CEST, Dominik Csapak wrote: > 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 > --- [snip] > +/// Renders the bridge devices a guest needs, see > +/// [`crate::get_pci_bridges`]. > +pub(crate) fn get_pci_bridges(machine: &Machine) -> Vec { nit: naming: If you remove the prefix from the print_ functions, i would also remove the prefix here for consistency. i.e., `pci_bridges`. > + // at most four bridges, each contributing a flag and its value > + let mut res =3D Vec::with_capacity(8); > + > + // some SCSI controllers can only have 7 disks each, so scsi14 and u= pwards > + // need scsihw2 and above, which live on pci.4 > + let include_pci4 =3D machine.version_at_least(11, 1, 0) || machine.m= ax_scsihw > 1; > + > + for i in 1..=3D4 { > + // q35.cfg already includes the bridges up to three > + if i < 4 && machine.q35 { > + continue; > + } > + > + if i =3D=3D 3 && !machine.virtio_scsi_single() { > + continue; > + } > + > + if i =3D=3D 4 && !include_pci4 { > + continue; > + } > + > + let id =3D if i =3D=3D 2 && machine.legacy_igd { > + format!("pci.{i}-igd") > + } else { > + format!("pci.{i}") > + }; > + > + let addr =3D match print_pci_addr(&id, machine.arch) { > + Ok(addr) =3D> addr, > + Err(err) =3D> { > + log::warn!("could not find address for bridge {id}: {err= }"); > + continue; > + } Consider `log::error!` instead of `log::warn!`.=20 IMO a missing bridge is a cause for concern and should be reflected with an error. > + }; > + res.push("-device".to_string()); > + res.push(format!("pci-bridge,id=3Dpci.{i},chassis_nr=3D{i}{addr}= ")); > + } > + > + res > +} > + [snip]=20 > } > 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 @@ > =20 > pub mod constants; > =20 > +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 i= s 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 warnin= g > +/// 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, Q= emuConfigScsihw}; > + > +/// The parts of a guest configuration that influence which PCI layout i= s 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 { nit naming: Machine seems a bit strange to me here, since we a passing the relevant guest config. So why not just call it `LayoutContext`, or=20 `GuestConfig | `GuestLayoutConfig`. > + pub arch: Arch, > + pub q35: bool, Since q35 is a boolean, it is possible to configure: Machine { arch: Arch::Aarch64, q35: true } What about making the chipset part of the Arch enum like this: pub enum X86Chipset { I440fx, Q35, } pub enum Arch { X8664(X86Chipset), Aarch64, } This would allow us to remove the q35 boolean value. Or if you want to keep a field here, what about a replacing q35 with an enum `machine_type` with 3 staes (Q35, I440fx, Virt) > + 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 spe= cific > + /// revision is optional. > + pub version: (u16, u16, Option), nit: naming: Consequently we should rename `version` -> `machine_version` > +} > + > +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 i= s 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 =3D arch.parse().unwrap_or(Arch::X8664); > + let scsihw =3D scsihw.and_then(|hw| hw.parse().ok()); > + let ostype =3D 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 =3D=3D 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 =3D=3D 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 re= quested > + /// one, since the plain QEMU version already implies all PVE change= s 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) =3D self.version.2 { > + if our_pve > pve { > + return true; > + } > + if our_pve < pve { > + return false; > + } > + } > + > + true > + } IMO we can just let rusts tuple compare logic handle this: pub fn version_at_least(&self, major: u16, minor: u16, pve: u16) -> bool { let (our_major, our_minor, our_pve) =3D self.version; (our_major, our_minor, our_pve.unwrap_or(u16::MAX)) >=3D (major, minor, pv= e) } > +}