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 4419D1FF0B3 for ; Wed, 11 Nov 2026 10:11:06 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 4450E2162C; Wed, 11 Nov 2026 10:11:02 +0100 (CET) Message-ID: Date: Wed, 11 Nov 2026 10:10:57 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH pve-qemu-server-rs v3 1/9] add pve-qemu-server-pci crate for guest PCI address generation To: Fiona Ebner , pve-devel@lists.proxmox.com References: <20261106132604.2556840-1-d.csapak@proxmox.com> <20261106132604.2556840-2-d.csapak@proxmox.com> <0bc1ecb7-9b92-4bae-9353-c67b998ab48e@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <0bc1ecb7-9b92-4bae-9353-c67b998ab48e@proxmox.com> 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: 1794388257934 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.312 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: RHO6UWRPZTZ5UJ66A2QK4UYRZTZVOWII X-Message-ID-Hash: RHO6UWRPZTZ5UJ66A2QK4UYRZTZVOWII 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 11/10/26 5:03 PM, Fiona Ebner wrote: > 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. nope not yet, but even if that test is big, i guess it would be a nice test regardless, i'll send it for qemu-server (actually 3, once for i440x, q35 and virt) > > 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. so basically a 'machine' struct? yeah that makes sense. > >> + /// 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. true. Would it be better if we expect explicit values here and not fall back to anything (so let perl handle the defaults for now) and raise an error otherwise? > >> + 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 also true, but this can't work anyway ? no virt on x86 and no q35/i440fx on aarch64, so this should really be enforced somewhere in our stack before trying to start qemu? > >> + }; >> + 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? yeah the comment should be introduced later ;) > >> + 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? mhmm i'll see. I tried to have most of the code free of Result's and do most of it statically, but can still make sense > > >> + 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? same here as above, i'll try > >> + 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) >> + } >> +}