Re: [PATCH pve-qemu-server-rs 2/9] pci: add machine abstraction and PCI bridge generation

["Elias Huhsovitz" <e.huhsovitz@proxmox.com>]

Comments inline.

On Tue Sep 22, 2026 at 12:55 PM CEST, Dominik Csapak wrote:
> Besides the address of a single device, callers also need the list of
> bridge devices a guest has to be started with. Which bridges those are
> depends on more than one property of the guest config, so pass them in
> as a `Machine` struct rather than as a growing list of parameters. It
> can be extended with the layout variant and other guest wide settings
> later on, without touching every call site again.
>
> The bridge list itself follows the Perl implementation: q35 machines
> already define the bridges up to three in their machine definition,
> pci.3 is only needed for virtio-scsi-single, and pci.4 only once a
> SCSI controller above index one exists or the machine version is new
> enough to always include it.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---

[snip]

> +/// Renders the bridge devices a guest needs, see
> +/// [`crate::get_pci_bridges`].
> +pub(crate) fn get_pci_bridges(machine: &Machine) -> Vec<String> {

nit: naming: If you remove the prefix from the print_ functions, i would
also remove the prefix here for consistency. i.e., `pci_bridges`.

> +    // at most four bridges, each contributing a flag and its value
> +    let mut res = Vec::with_capacity(8);
> +
> +    // some SCSI controllers can only have 7 disks each, so scsi14 and upwards
> +    // need scsihw2 and above, which live on pci.4
> +    let include_pci4 = machine.version_at_least(11, 1, 0) || machine.max_scsihw > 1;
> +
> +    for i in 1..=4 {
> +        // q35.cfg already includes the bridges up to three
> +        if i < 4 && machine.q35 {
> +            continue;
> +        }
> +
> +        if i == 3 && !machine.virtio_scsi_single() {
> +            continue;
> +        }
> +
> +        if i == 4 && !include_pci4 {
> +            continue;
> +        }
> +
> +        let id = if i == 2 && machine.legacy_igd {
> +            format!("pci.{i}-igd")
> +        } else {
> +            format!("pci.{i}")
> +        };
> +
> +        let addr = match print_pci_addr(&id, machine.arch) {
> +            Ok(addr) => addr,
> +            Err(err) => {
> +                log::warn!("could not find address for bridge {id}: {err}");
> +                continue;
> +            }

Consider `log::error!` instead of `log::warn!`. 

IMO a missing bridge is a cause for concern and should be reflected with
an error.

> +        };
> +        res.push("-device".to_string());
> +        res.push(format!("pci-bridge,id=pci.{i},chassis_nr={i}{addr}"));
> +    }
> +
> +    res
> +}
> +

[snip] 

>  }
> diff --git a/pve-qemu-server-pci/src/lib.rs b/pve-qemu-server-pci/src/lib.rs
> index 5e8b8a7..7d0bd92 100644
> --- a/pve-qemu-server-pci/src/lib.rs
> +++ b/pve-qemu-server-pci/src/lib.rs
> @@ -10,6 +10,9 @@
>  
>  pub mod constants;
>  
> +mod machine;
> +pub use machine::Machine;
> +
>  mod types;
>  use pve_api_types::ClusterResourceHostArch as Arch;
>  pub use types::*;
> @@ -42,3 +45,18 @@ pub fn print_pcie_addr(id: &str) -> Result<String, PciConfigError> {
>  pub fn print_pcie_root_port(index: u8) -> Result<String, PciConfigError> {
>      layout::legacy::print_pcie_root_port(index)
>  }
> +
> +/// Returns the number of the PCI bridge `id` sits on, or `None` if it is on
> +/// the root bus or not a known device.
> +pub fn get_pci_bridge_for_device(id: &str) -> Option<u8> {
> +    layout::legacy::get_pci_bridge_for_device(id)
> +}
> +
> +/// Returns the QEMU arguments for all PCI bridges `machine` needs, as
> +/// alternating `-device` flags and their values.
> +///
> +/// Bridges whose address cannot be determined are skipped with a warning
> +/// rather than failing the whole guest.
> +pub fn get_pci_bridges(machine: &Machine) -> Vec<String> {
> +    layout::legacy::get_pci_bridges(machine)
> +}
> diff --git a/pve-qemu-server-pci/src/machine.rs b/pve-qemu-server-pci/src/machine.rs
> new file mode 100644
> index 0000000..10ee9db
> --- /dev/null
> +++ b/pve-qemu-server-pci/src/machine.rs
> @@ -0,0 +1,99 @@
> +use pve_api_types::{ClusterResourceHostArch as Arch, QemuConfigOstype, QemuConfigScsihw};
> +
> +/// 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(Clone, Debug)]
> +pub struct Machine {

nit naming: Machine seems a bit strange to me here, since we a passing
the relevant guest config. So why not just call it `LayoutContext`, or 
`GuestConfig | `GuestLayoutConfig`.

> +    pub arch: Arch,
> +    pub q35: bool,

Since q35 is a boolean, it is possible to configure:
Machine { arch: Arch::Aarch64, q35: true }

What about making the chipset part of the Arch enum like this:

pub enum X86Chipset {
    I440fx,
    Q35,
}

pub enum Arch {
    X8664(X86Chipset),
    Aarch64,
}

This would allow us to remove the q35 boolean value.

Or if you want to keep a field here, what about a replacing q35 with an
enum `machine_type` with 3 staes (Q35, I440fx, Virt)

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

nit: naming: Consequently we should rename `version` -> `machine_version`

> +}
> +
> +impl Machine {
> +    /// Builds a [`Machine`] 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.
> +    #[allow(clippy::too_many_arguments)]
> +    pub fn new(
> +        arch: &str,
> +        q35: bool,
> +        scsihw: Option<String>,
> +        max_scsihw: u8,
> +        legacy_igd: bool,
> +        ostype: Option<String>,
> +        major: u16,
> +        minor: u16,
> +        pve: Option<u16>,
> +    ) -> Self {
> +        let arch = arch.parse().unwrap_or(Arch::X8664);
> +        let scsihw = scsihw.and_then(|hw| hw.parse().ok());
> +        let ostype = ostype.and_then(|ostype| ostype.parse().ok());
> +        Self {
> +            arch,
> +            q35,
> +            scsihw,
> +            max_scsihw,
> +            legacy_igd,
> +            ostype,
> +            version: (major, minor, pve),
> +        }
> +    }
> +
> +    /// Whether the guest's root bus is PCIe. True for q35 machines and for
> +    /// aarch64, which only has a PCIe host bridge.
> +    pub fn is_pcie(&self) -> bool {
> +        self.q35 || self.arch == Arch::Aarch64
> +    }
> +
> +    /// Whether each SCSI disk gets its own controller, which needs the extra
> +    /// `pci.3` bridge.
> +    pub fn virtio_scsi_single(&self) -> bool {
> +        self.scsihw == Some(QemuConfigScsihw::VirtioScsiSingle)
> +    }
> +
> +    /// Whether the machine version is at least `major.minor.pve`.
> +    ///
> +    /// A machine without a PVE revision counts as being at least any requested
> +    /// one, since the plain QEMU version already implies all PVE changes made
> +    /// for it.
> +    pub fn version_at_least(&self, major: u16, minor: u16, pve: u16) -> bool {
> +        if self.version.0 > major {
> +            return true;
> +        }
> +        if self.version.0 < major {
> +            return false;
> +        }
> +
> +        if self.version.1 > minor {
> +            return true;
> +        }
> +        if self.version.1 < minor {
> +            return false;
> +        }
> +
> +        if let Some(our_pve) = self.version.2 {
> +            if our_pve > pve {
> +                return true;
> +            }
> +            if our_pve < pve {
> +                return false;
> +            }
> +        }
> +
> +        true
> +    }

IMO we can just let rusts tuple compare logic handle this:

pub fn version_at_least(&self, major: u16, minor: u16, pve: u16) -> bool {
    let (our_major, our_minor, our_pve) = self.version;
	(our_major, our_minor, our_pve.unwrap_or(u16::MAX)) >= (major, minor, pve)
}

> +}