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 37B2F1FF0B4 for ; Tue, 10 Nov 2026 17:03:34 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 77DF521615; Tue, 10 Nov 2026 17:03:32 +0100 (CET) Message-ID: <0bc1ecb7-9b92-4bae-9353-c67b998ab48e@proxmox.com> Date: Tue, 10 Nov 2026 17:03:27 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-qemu-server-rs v3 1/9] add pve-qemu-server-pci crate for guest PCI address generation To: Dominik Csapak , pve-devel@lists.proxmox.com References: <20261106132604.2556840-1-d.csapak@proxmox.com> <20261106132604.2556840-2-d.csapak@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20261106132604.2556840-2-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794326607301 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.285 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) 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: 6EDFK24FVM5TAMCXMW7ZH5WD2XZ4KYFH X-Message-ID-Hash: 6EDFK24FVM5TAMCXMW7ZH5WD2XZ4KYFH X-MailFrom: f.ebner@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: Nice work! Just wondering: did you also test the changes with maximal configs, i.e. where all devices (except conflicting ones) are configured at the same time? The resulting config is likely too big to be worth committing in the cfg2cmd tests, but still might be nice to check. Am 06.11.26 um 2:28 PM schrieb Dominik Csapak: > +/// 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(Default, Clone, Debug)] > +pub struct HardwareConfig { > + pub arch: Arch, > + pub mainboard: Mainboard, > + 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, The two SCSI-related options could be grouped in a struct. > + pub legacy_igd: bool, > + pub ostype: Option, > + /// QEMU machine version as `(major, minor, pve)`, where the PVE specific > + /// revision is optional. > + pub machine_version: (u16, u16, Option), I would prefer a dedicated struct rather than a tuple here. And maybe it could also include the mainboard and required_pve_version? Or maybe that bigger one could be a second-level struct with the version struct inside of it? Not sure what is nicer in practice. > + /// The recorded required `+pveX` version > + pub required_pve_version: Arc, > +} > + > +impl HardwareConfig { > + /// Builds a [`HardwareConfig`] 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. Arch falls back to the host architecture, not x86_64. > + pub fn new(params: HardwareConfigParams) -> Self { > + let arch = params > + .arch > + .parse() > + .unwrap_or(ClusterResourceHostArch::X8664) > + .into(); > + let scsihw = params.scsihw.and_then(|hw| hw.parse().ok()); > + let ostype = params.ostype.and_then(|ostype| ostype.parse().ok()); > + > + let mainboard = match arch { > + Arch::Aarch64 => Mainboard::Virt, > + Arch::X8664 if params.machine_type.contains("q35") => Mainboard::Q35, > + Arch::X8664 => Mainboard::I440fx, You must not rely on the default for the arch here, since we don't enforce that a machine type matches the arch currently: [I] root@pve9a1 ~# qm create 100 --machine virt --arch x86_64 [I] root@pve9a1 ~# cat /etc/pve/qemu-server/100.conf arch: x86_64 boot: machine: virt meta: creation-qemu=11.1.3,ctime=1794318573 smbios1: uuid=e325ddd6-f74b-4a08-be42-b618f2924a9e vmgenid: ff99f0bf-96ef-44fe-9861-481c506c65ff > + }; > + Self { > + arch, > + mainboard, > + scsihw, > + max_scsihw: params.max_scsihw, > + legacy_igd: params.legacy_igd, > + ostype, > + machine_version: (params.major, params.minor, params.pve), > + required_pve_version: Arc::new(AtomicU16::new(0)), > + } > + } > + ---snip 8<--- > +impl std::str::FromStr for Bus { > + type Err = LayoutError; > + > + fn from_str(s: &str) -> Result { > + // NOTE: the prefixes here must be checked before the generic 'pve.' > + // one below, which would otherwise swallow them. Where is the "pve." prefix? Is this still a left-over comment? > + const PREFIXES: &[(&str, BusConstructor)] = &[ > + ("pci.", Bus::Pci), > + ("pcie.", Bus::Pcie), > + #[expect(deprecated)] > + ("ich9-pcie-port-", Bus::LegacyRootPort), > + ]; > + > + if s == "pcidmi" { > + return Ok(Bus::Pcidmi); > + } > + > + for (prefix, c) in PREFIXES { > + if let Some(rest) = s.strip_prefix(prefix) { > + let index: u8 = rest.parse().map_err(|_| LayoutError::InvalidConfigId)?; > + return Ok(c(index)); > + } > + } > + > + Err(LayoutError::InvalidConfigId) > + } > +} ---snip 8<--- > +/// The address a layout assigns to a device. > +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] > +pub struct AddressAssignment { > + pub addr: PciAddress, > + pub device: Device, > + /// Whether QEMU has to be told that other functions of this slot are in > + /// use. Only ever set on function 0. Should there be a check for this with a new() function? Or would that complicate things too much? > + pub multifunction: bool, > +} ---snip 8<--- > diff --git a/pve-qemu-server-pci/src/types/pci_address.rs b/pve-qemu-server-pci/src/types/pci_address.rs > new file mode 100644 > index 0000000..dcef298 > --- /dev/null > +++ b/pve-qemu-server-pci/src/types/pci_address.rs > @@ -0,0 +1,36 @@ > +use crate::Bus; > + > +/// The complete address of a device: the bus it sits on plus the slot and > +/// function number within that bus. > +#[derive(Hash, Debug, Clone, Copy, PartialEq, Eq)] > +pub struct PciAddress { > + pub bus: Bus, > + pub device: u8, > + pub function: u8, > +} > + > +impl PciAddress { > + pub const fn new(bus: Bus, device: u8, function: u8) -> Self { Should there be a check for constraints, e.g. that function < FUNCTIONS_PER_DEVICE? Or would that complicate things too much? > + Self { > + bus, > + device, > + function, > + } > + } > + > + /// Renders the address as the `,bus=...,addr=...` suffix of a QEMU > + /// `-device` argument, including the leading comma. > + /// > + /// The function number is only emitted when it is not zero, matching what > + /// the Perl implementation produces. > + pub fn to_qemu_addr(&self) -> String { > + let bus = self.bus; > + let function = if self.function > 0 { > + format!(".{}", self.function) > + } else { > + String::new() > + }; > + > + format!(",bus={bus},addr=0x{:x}{function}", self.device) > + } > +}