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>]

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<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.

> +    /// 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.

> +    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<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?

> +        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)
> +    }
> +}