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 2DFFD1FF09B for ; Mon, 28 Sep 2026 16:04:34 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 25AA1216E5; Mon, 28 Sep 2026 16:04:32 +0200 (CEST) Message-ID: Date: Mon, 28 Sep 2026 16:04:26 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH pve-qemu-server-rs 2/9] pci: add machine abstraction and PCI bridge generation To: Elias Huhsovitz , pve-devel@lists.proxmox.com References: <20260922105550.2084078-1-d.csapak@proxmox.com> <20260922105550.2084078-3-d.csapak@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790604266948 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.575 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: UGYKWYVUO6KCOONMBT6ZJCCL3VDJFLMU X-Message-ID-Hash: UGYKWYVUO6KCOONMBT6ZJCCL3VDJFLMU 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: On 9/28/26 2:55 PM, Elias Huhsovitz wrote: > 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 = 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; >> + } > > Consider `log::error!` instead of `log::warn!`. > > IMO a missing bridge is a cause for concern and should be reflected with > an error. i think returning an error here would even be better. we already have to touch the perl side, so an additional eval { } shouldn't hurt (or we simply die there; it shouldn't be possible anyway?) > >> + }; >> + res.push("-device".to_string()); >> + res.push(format!("pci-bridge,id=pci.{i},chassis_nr={i}{addr}")); >> + } >> + >> + res >> +} >> + > > [snip] > >> } >> 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 { > > 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 > `GuestConfig | `GuestLayoutConfig`. > yeah, machine was not a very well chosen name :P I'll think about what to name it, thanks for the suggestions! >> + 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) since i want to match (and at some point use) the pve config fields here having an enum for the 3 machine types makes most sense IMO > >> + 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), > > 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 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 >> + } > > 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) = self.version; > (our_major, our_minor, our_pve.unwrap_or(u16::MAX)) >= (major, minor, pve) > } true. This is a thing that should probably live in a helper crate somewhere. I guess well reuse this kind of version comparison more than once. > >> +} >