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 323B61FF0B3 for ; Wed, 11 Nov 2026 10:19:36 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 6C9EF21649; Wed, 11 Nov 2026 10:19:33 +0100 (CET) Message-ID: Date: Wed, 11 Nov 2026 10:19:23 +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> <0bc1ecb7-9b92-4bae-9353-c67b998ab48e@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794388768247 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.283 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: QXRSXJC3LQILO6GAZFC646DYHOKJLQXX X-Message-ID-Hash: QXRSXJC3LQILO6GAZFC646DYHOKJLQXX 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: Am 11.11.26 um 10:10 AM schrieb Dominik Csapak: > On 11/10/26 5:03 PM, Fiona Ebner wrote: >> Am 06.11.26 um 2:28 PM schrieb Dominik Csapak: >>> +    /// 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? Sounds like a plan :) >>> +    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? We could start enforcing it, but it's not currently, so such guest configurations already might exist (by accident, i.e. changing one property without adapting the other). Even if they can't be started, the generated commandline should not be different from what the config contains. Otherwise they suddenly might be able to start ;) (not with your series alone here, but still). Even if we did enforce it, I still think we should not assume stuff about another config property, but rather actually look at its value. >>> +        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 It's just an idea, so maybe it isn't worth the overhead.