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

[Dominik Csapak <d.csapak@proxmox.com>]

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<QemuConfigScsihw>,
>> +    /// 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<QemuConfigOstype>,
>> +    /// QEMU machine version as `(major, minor, pve)`, where the PVE specific
>> +    /// revision is optional.
>> +    pub machine_version: (u16, u16, Option<u16>),
> 
> 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<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?


> 
>> +    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<Self, Self::Err> {
>> +        // 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)
>> +    }
>> +}