Re: [PATCH pve-qemu-server-rs v3 1/9] add pve-qemu-server-pci crate for guest PCI address generation

[Fiona Ebner <f.ebner@proxmox.com>]

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<AtomicU16>,
>>> +}
>>> +
>>> +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.