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

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

Good changes/additions! I think this version much more coherent than the
previous one.

Some comments inline.

On Thu Oct 15, 2026 at 10:10 AM CEST, Dominik Csapak wrote:
> Since the hardware layout, including PCI addresses, are part of the
> guest's ABI, the assignment has to stay stable for the whole lifetime
> of a VM. The Perl implementation in PVE::QemuServer::PCI encodes this as
> two flat hashes of hardcoded addresses, which is hard to extend without
> accidentally shifting an existing entry.
>
> Describe the assignment as a layout instead: a tree of buses, slots and
> functions that says what may sit where, which addresses are reserved for
> something outside of our control, and which are still free. Looking up a
> device then means walking that tree, and adding a new device means
> filling in a free slot rather than picking a number by hand.
>
> The layouts added here reproduce the current Perl address maps exactly,
> including the quirks for legacy IGD passthrough, virtio-scsi-single and
> the Windows 7 PCIe workaround. The tests cover every id the Perl maps
> contain (except win7's hostpciXbus0, since that will change in perl
> anyway).
>
> Lookups take a `HardwareConfig`, which only contains the parts of the
> guest config that influence the layout and its quirks, so callers can
> build it from whatever config representation they have.
>
> Also adds the cargo workspace and the build tooling the other Proxmox
> Rust repositories use to build Debian packages.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

[snip]

> diff --git a/Cargo.toml b/Cargo.toml
> new file mode 100644
> index 0000000..71bc8e5
> --- /dev/null
> +++ b/Cargo.toml
> @@ -0,0 +1,29 @@
> +[workspace]
> +members = [
> +    "pve-qemu-server-pci"
> +]
> +exclude = [
> +    "build",
> +]
> +resolver = "3"
> +
> +[workspace.package]
> +authors = ["Proxmox Support Team <support@proxmox.com>"]
> +edition = "2024"
> +license = "AGPL-3"
> +repository = "https://git.proxmox.com/?p=pve-qemu-server-rs.git"
> +homepage = "https://proxmox.com"
> +exclude = [ "debian" ]
> +rust-version = "1.94"
> +
> +[workspace.dependencies]
> +# any features enabled here are enabled on all members using 'workspace = true'!
> +
> +# external dependencies
> +anyhow = "1.0"

nit: anyhow crate is currently unused.

I am not sure if we even need it, since this repo will primarly consist
of libraires, as far as i can tell.

> +log = "0.4"
> +strum = { version = "0.26", features = ["derive"] }
> +

[snip]

> diff --git a/pve-qemu-server-pci/src/constants.rs b/pve-qemu-server-pci/src/constants.rs
> new file mode 100644
> index 0000000..ea6db21
> --- /dev/null
> +++ b/pve-qemu-server-pci/src/constants.rs
> @@ -0,0 +1,7 @@
> +//! Limits the layouts are built around.
> +
> +/// Number of slots on a PCI bus. Slot 0x00 is always taken by the bridge
> +/// itself, so only [`USABLE_SLOTS_PER_BUS`] of them can hold a device.
> +pub(crate) const SLOTS_PER_BUS: usize = 32;
> +pub(crate) const USABLE_SLOTS_PER_BUS: u8 = SLOTS_PER_BUS as u8 - 1;
> +pub(crate) const FUNCTIONS_PER_DEVICE: usize = 8;
> diff --git a/pve-qemu-server-pci/src/error.rs b/pve-qemu-server-pci/src/error.rs
> new file mode 100644
> index 0000000..5c4f7de
> --- /dev/null
> +++ b/pve-qemu-server-pci/src/error.rs
> @@ -0,0 +1,24 @@
> +use std::{error::Error, fmt::Display};
> +

nit: We could consider using the 'thiserror' crate here.
(unless minimizing dependencies is desired)

> +use crate::Device;
> +
> +/// Errors that can occur while looking up a PCI address.
> +#[derive(Clone, Copy, Debug, PartialEq, Eq)]
> +#[non_exhaustive]
> +pub enum LayoutError {
> +    /// The given configuration id is not a device this crate knows about.
> +    InvalidConfigId,
> +    /// The device is not part of the layout it was looked up in.
> +    NoAddressFound(Device),
> +}
> +
> +impl Display for LayoutError {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        match self {
> +            LayoutError::InvalidConfigId => write!(f, "invalid configuration id"),
> +            LayoutError::NoAddressFound(device) => write!(f, "address '{device}' not found"),
> +        }
> +    }
> +}
> +
> +impl Error for LayoutError {}
> diff --git a/pve-qemu-server-pci/src/hardware_config.rs b/pve-qemu-server-pci/src/hardware_config.rs
> new file mode 100644
> index 0000000..a348353
> --- /dev/null
> +++ b/pve-qemu-server-pci/src/hardware_config.rs
> @@ -0,0 +1,156 @@
> +use std::sync::Arc;
> +use std::sync::atomic::AtomicU16;
> +
> +use pve_api_types::{ClusterResourceHostArch as Arch, QemuConfigOstype, QemuConfigScsihw};
> +
> +#[derive(Default, Clone, Debug, PartialEq)]
> +pub enum Mainboard {
> +    #[default]
> +    I440fx,
> +    Q35,
> +    Virt,
> +}
> +
> +impl std::fmt::Display for Mainboard {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        let text = match self {
> +            Mainboard::I440fx => "i440fx",
> +            Mainboard::Q35 => "q35",
> +            Mainboard::Virt => "virt",
> +        };
> +        f.write_str(text)
> +    }
> +}
> +
> +impl Mainboard {
> +    pub fn has_pcie(&self) -> bool {
> +        match self {
> +            Mainboard::I440fx => false,
> +            Mainboard::Q35 | Mainboard::Virt => true,
> +        }
> +    }
> +}
> +
> +/// 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,
> +    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>),
> +    /// 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.
> +    #[allow(clippy::too_many_arguments)]
> +    pub fn new(
> +        arch: &str,
> +        machine_type: String,
> +        scsihw: Option<String>,
> +        max_scsihw: u8,
> +        legacy_igd: bool,
> +        ostype: Option<String>,
> +        major: u16,
> +        minor: u16,
> +        pve: Option<u16>,
> +    ) -> Self {
> +        let arch = match arch.parse().unwrap_or(Arch::X8664) {
> +            Arch::UnknownEnumValue(_) => Arch::X8664,
> +            other => other,
> +        };


> +        let scsihw = scsihw.and_then(|hw| hw.parse().ok());
> +        let ostype = ostype.and_then(|ostype| ostype.parse().ok());
> +
> +        let mainboard = match arch {
> +            Arch::Aarch64 => Mainboard::Virt,
> +            Arch::X8664 if machine_type.contains("q35") => Mainboard::Q35,
> +            Arch::X8664 | Arch::UnknownEnumValue(_) => Mainboard::I440fx,
> +        };

I understand that since we are using the pve_api_types we are stuck with
UnknownEnumValue, but this creates the problem that we always have to check
for Arch::UnknownEnumValue in each match statement.

What about creating an internal `pub enum Arch` and storing this as a
field inside of HardwareConfig? e.g. we just use the 
ClusterResourceHostArch for parsing at the API level and then switch an
internal Arch enum in order to guarantee that a valid state is stored in
the arch field.

> +        Self {
> +            arch,
> +            mainboard,
> +            scsihw,
> +            max_scsihw,
> +            legacy_igd,
> +            ostype,
> +            machine_version: (major, minor, pve),
> +            required_pve_version: Arc::new(AtomicU16::new(0)),
> +        }
> +    }
> +
> +    /// Whether the guest's root bus is PCIe. True for q35 machines and for
> +    /// aarch64, which only has a PCIe host bridge.
> +    pub fn has_pcie(&self) -> bool {
> +        self.mainboard.has_pcie()
> +    }
> +
> +    /// Returns if the mainboard is a Q35 variant.
> +    pub fn is_q35(&self) -> bool {
> +        self.mainboard == Mainboard::Q35
> +    }
> +
> +    /// 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 '+pve0'.
> +    pub fn version_at_least(&self, major: u16, minor: u16, pve: Option<u16>) -> bool {
> +        match (self.machine_version.0, self.machine_version.1).cmp(&(major, minor)) {
> +            std::cmp::Ordering::Less => false,
> +            std::cmp::Ordering::Greater => true,
> +            std::cmp::Ordering::Equal => {
> +                let ours = self.machine_version.2.unwrap_or(0);
> +                let pve = pve.unwrap_or(0);
> +                let ok = ours >= pve;
> +                if ok && pve > 0 {
> +                    self.required_pve_version
> +                        .fetch_max(pve, std::sync::atomic::Ordering::SeqCst);
> +                }

The .fetch_max() makes it a bit ambigous. When I am calling the function
from perl, I wouldnt suspect it to modify the unnderlying 
required_pve_version.

IMO this behaviour needs to be documented somehow (e.g. by changing the
function name, or metioning this in the function doc). Alternatively
the mutation part could be split out into another function.

> +                ok
> +            }
> +        }
> +    }
> +
> +    /// Returns the version number for Windows OS types or 0 for other.
> +    ///
> +    /// Windows XP era OS types are represented by `5`, and Vista era ones by `6`.

Lets hope the new Windows will be Windows 12+ ;)

> +    pub fn windows_version(&self) -> u8 {
> +        match self.ostype.unwrap_or(QemuConfigOstype::Other) {
> +            QemuConfigOstype::Wxp | QemuConfigOstype::W2k | QemuConfigOstype::W2k3 => 5,
> +            QemuConfigOstype::W2k8 | QemuConfigOstype::Wvista => 6,
> +            QemuConfigOstype::Win7 => 7,
> +            QemuConfigOstype::Win8 => 8,
> +            QemuConfigOstype::Win10 => 10,
> +            QemuConfigOstype::Win11 => 11,
> +            _ => 0,
> +        }
> +    }
> +
> +    /// Returns the recorded required `+pveX` version.
> +    pub fn required_pve_version(&self) -> u16 {
> +        self.required_pve_version
> +            .load(std::sync::atomic::Ordering::Relaxed)
> +    }
> +}

[snip]

> diff --git a/pve-qemu-server-pci/src/layout/mod.rs b/pve-qemu-server-pci/src/layout/mod.rs
> new file mode 100644
> index 0000000..5439c17
> --- /dev/null
> +++ b/pve-qemu-server-pci/src/layout/mod.rs
> @@ -0,0 +1,48 @@
> +use crate::Slot;
> +use crate::constants::SLOTS_PER_BUS;
> +
> +// local helper macro to make the lines shorter
> +macro_rules! single_device {
> +    ($i:ident ( $e:expr )) => {
> +        $crate::Slot::Single($crate::Function::Device($crate::Device::$i($e)))
> +    };
> +    ($i:ident) => {
> +        $crate::Slot::Single($crate::Function::Device($crate::Device::$i))
> +    };
> +}

nit: IMO this could be a const fn instead. Obviously a macro here is
not a problem, i am not not a macro-fan :).

> +
> +pub(crate) mod legacy;
> +
> +/// Constructs a correctly sized list of bridge slots, starting at address 0x01
> +/// because 0x00 is always taken by the bridge itself.
> +///
> +/// Panics if more slots than fit are passed in. As this is a `const fn` used
> +/// from constants only, that turns into a compile time error.
> +pub(crate) const fn bridge_slots<'a>(slots: &'a [Slot<'a>]) -> [Slot<'a>; SLOTS_PER_BUS] {
> +    const PLACEHOLDER: Slot = Slot::Unused;
> +    let mut out = [PLACEHOLDER; SLOTS_PER_BUS];
> +    if slots.len() > SLOTS_PER_BUS - 1 {
> +        panic!("too many devices on bridge");
> +    }
> +    out[0] = Slot::Reserved;
> +    let mut i = 1;
> +    while i < SLOTS_PER_BUS && i - 1 < slots.len() {
> +        out[i] = slots[i - 1];
> +        i += 1;
> +    }
> +    out
> +}
> +

[snip]

> +++ b/pve-qemu-server-pci/src/types/device.rs
> @@ -0,0 +1,173 @@
> +use std::{fmt::Display, str::FromStr};
> +use strum::{EnumDiscriminants, VariantArray};
> +
> +use crate::{Bus, LayoutError};
> +
> +/// A device that needs a PCI address.
> +///
> +/// Variants carrying an index map to a configuration key of the same name plus
> +/// that index, for example `Net(3)` is the guest's `net3`.
> +#[derive(Hash, Debug, Clone, Copy, PartialEq, Eq, EnumDiscriminants)]
> +#[strum_discriminants(derive(VariantArray))]
> +pub enum Device {
> +    Ahci,
> +    Balloon,
> +    Watchdog,
> +    GuestAgent,
> +    SpiceSerial,
> +    Rng,
> +    Audio,
> +    Ivshmem,
> +    Bridge(Bus),
> +    RootPort(Bus),
> +    Vga(u8),
> +    Viommu,
> +    VirtioFs(u8),
> +    Net(u8),
> +    ScsiController(u8),
> +    VirtioBlk(u8),
> +    Piix3Controller,
> +    XhciController(u8),
> +    Hostpci(u8),
> +    Hostpcie(u8),
> +    /// The EHCI USB controllers of the ICH9 chipset, numbered from one like
> +    /// their QEMU models.
> +    Ich9Ehci(u8),
> +    /// The UHCI USB controllers of the ICH9 chipset, numbered from one like
> +    /// their QEMU models.
> +    Ich9Uhci(u8),
> +    /// The HDA controller legacy q35 guests always have, independent of a
> +    /// configured `audio0`.
> +    Ich9Audio,
> +}
> +
> +impl Display for Device {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        let text = match self {
> +            Device::Ahci => "ahci",
> +            Device::Balloon => "balloon",
> +            Device::Watchdog => "watchdog",
> +            Device::GuestAgent => "guest-agent",
> +            Device::SpiceSerial => "spice-serial",
> +            Device::Rng => "rng",
> +            Device::Audio => "audio",
> +            Device::Ivshmem => "ivshmem",
> +            Device::Viommu => "viommu",
> +            Device::Piix3Controller => "Piix3Controller",
> +            Device::Ich9Audio => "ich9-audio",
> +            _ => "",
> +        };
> +        if !text.is_empty() {
> +            return f.write_str(text);
> +        }
> +        let text = match self {
> +            Device::Bridge(bus) => bus.to_string(),
> +            Device::RootPort(bus) => bus.to_string(),
> +            Device::Vga(i) => format!("vga{i}"),
> +            Device::VirtioFs(i) => format!("virtiofs{i}"),
> +            Device::Net(i) => format!("net{i}"),
> +            Device::ScsiController(i) => format!("scsihw{i}"),
> +            Device::VirtioBlk(i) => format!("virtioblk{i}"),
> +            Device::XhciController(i) => format!("xhci{i}"),
> +            Device::Hostpci(i) => format!("hostpci{i}"),
> +            Device::Hostpcie(i) => format!("hostpcie{i}"),
> +            Device::Ich9Ehci(i) => format!("ich9-ehci{i}"),
> +            Device::Ich9Uhci(i) => format!("ich9-uhci{i}"),
> +            _ => String::new(),
> +        };
> +        write!(f, "{text}")
> +    }

I don't fully understand the point of splitting the match function into
2 parts here. What is the downside of just doing:

match self {
    Device::Ahci => f.write_str("ahci"),
    Device::Balloon => f.write_str("balloon"),
    //...
    Device::Bridge(bus) => write!(f, "{bus}"),
    Device::RootPort(bus) => write!(f, "{bus}"),
    //.. 
}

This way we don't have a default wildcard '_' and have an exhaustive
matching pattern for Device.

> +}
> +
> +/// Rebuilds the single possible [`Device`] of a discriminant, which only works
> +/// for the variants that carry no index.
> +impl TryFrom<DeviceDiscriminants> for Device {
> +    type Error = LayoutError;
> +
> +    fn try_from(value: DeviceDiscriminants) -> Result<Self, Self::Error> {
> +        let res = match value {
> +            DeviceDiscriminants::Ahci => Device::Ahci,
> +            DeviceDiscriminants::Balloon => Device::Balloon,
> +            DeviceDiscriminants::Watchdog => Device::Watchdog,
> +            DeviceDiscriminants::GuestAgent => Device::GuestAgent,
> +            DeviceDiscriminants::SpiceSerial => Device::SpiceSerial,
> +            DeviceDiscriminants::Rng => Device::Rng,
> +            DeviceDiscriminants::Audio => Device::Audio,
> +            DeviceDiscriminants::Ivshmem => Device::Ivshmem,
> +            DeviceDiscriminants::Piix3Controller => Device::Piix3Controller,
> +            DeviceDiscriminants::Ich9Audio => Device::Ich9Audio,
> +            DeviceDiscriminants::Bridge
> +            | DeviceDiscriminants::RootPort
> +            | DeviceDiscriminants::Vga
> +            | DeviceDiscriminants::Viommu

Why is Vimmo here? It also doesn't need an index.

> +            | DeviceDiscriminants::VirtioFs
> +            | DeviceDiscriminants::Net
> +            | DeviceDiscriminants::ScsiController
> +            | DeviceDiscriminants::VirtioBlk
> +            | DeviceDiscriminants::XhciController
> +            | DeviceDiscriminants::Hostpci
> +            | DeviceDiscriminants::Hostpcie
> +            | DeviceDiscriminants::Ich9Ehci
> +            | DeviceDiscriminants::Ich9Uhci => return Err(LayoutError::InvalidConfigId),
> +        };
> +        Ok(res)
> +    }
> +}
> +
> +type DeviceConstructor = fn(u8) -> Device;
> +
> +/// Parses the configuration id of a device, for example `net0` or `hostpci3`.
> +impl FromStr for Device {
> +    type Err = LayoutError;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        // NOTE: order matters, the longer prefixes have to come first so that
> +        // 'virtio' does not shadow 'virtioscsi' and 'hostpci' not 'hostpcie'.
> +        const PREFIXES: &[(&str, DeviceConstructor)] = &[
> +            ("net", Device::Net),
> +            ("scsihw", Device::ScsiController),
> +            ("virtioscsi", Device::ScsiController),
> +            ("virtiofs", Device::VirtioFs),
> +            ("virtio", Device::VirtioBlk),
> +            ("hostpcie", Device::Hostpcie),
> +            ("hostpci", Device::Hostpci),
> +        ];
> +
> +        for (prefix, c) in PREFIXES {
> +            if let Some(index) = s.strip_prefix(prefix) {
> +                let index = index.parse().map_err(|_| LayoutError::InvalidConfigId)?;
> +                return Ok(c(index));
> +            }
> +        }
> +
> +        let class = match s {
> +            "viommu" => Device::Viommu,
> +            "vga" => Device::Vga(0),
> +            "vga1" => Device::Vga(1),
> +            "vga2" => Device::Vga(2),
> +            "vga3" => Device::Vga(3),
> +            "xhci" => Device::XhciController(0),
> +            "ahci0" => Device::Ahci,

nit: IMO This is a bit confusing. When parsing a string we expect "ahci0".
But in in the Display implementation for Device:

Device::Ahci => "ahci"

The 0-suffix is dropped. 

Same goes for GustAgent

For "xhci", the issue exists in the opposite direction.
We parse from "xhci", but Display returns "xhci0"

Not a big deal, but I was wondering if this is intentional.

> +            "balloon0" => Device::Balloon,
> +            "watchdog" => Device::Watchdog,
> +            "qga0" => Device::GuestAgent,
> +            "spice" => Device::SpiceSerial,
> +            "rng0" => Device::Rng,
> +            "audio0" => Device::Audio,
> +            "ivshmem" => Device::Ivshmem,
> +            // legacy rules
> +            "ehci" | "piix3" => Device::Piix3Controller,
> +            "legacy-igd" => Device::Vga(0),
> +            "pci.2-igd" => Device::Bridge(Bus::Pci(99)),
> +            other => {
> +                if let Ok(bridge) = other.parse() {
> +                    return Ok(Device::Bridge(bridge));
> +                }
> +
> +                return Err(LayoutError::InvalidConfigId);
> +            }
> +        };
> +
> +        Ok(class)
> +    }
> +}
> diff --git a/pve-qemu-server-pci/src/types/layout.rs b/pve-qemu-server-pci/src/types/layout.rs
> new file mode 100644
> index 0000000..889b961
> --- /dev/null
> +++ b/pve-qemu-server-pci/src/types/layout.rs
> @@ -0,0 +1,329 @@
> +use std::collections::VecDeque;
> +
> +use crate::constants::{FUNCTIONS_PER_DEVICE, SLOTS_PER_BUS, USABLE_SLOTS_PER_BUS};
> +use crate::{Bus, Device, LayoutError, PciAddress, PveBridge};
> +
> +/// What sits at one function number of a slot.
> +#[derive(Clone, Copy, Debug, PartialEq, Eq)]
> +pub enum Function<'a> {
> +    /// Occupied by something outside of this crate's control, for example by
> +    /// the machine type's own configuration file.
> +    Reserved,
> +    /// Free for future use. Kept explicit so that the surrounding entries keep
> +    /// their address when something is added later.
> +    Unused,
> +    Device(Device),
> +    /// A bridge with a statically known set of slots behind it.
> +    FixedBridge(Bus, &'a [Slot<'a>; SLOTS_PER_BUS]),
> +    /// A PCIe root port with the single device attached to it.
> +    RootPort(Bus, Device),
> +}
> +
> +/// What sits at one slot of a bus.
> +#[derive(Clone, Copy, Debug, PartialEq, Eq)]
> +pub enum Slot<'a> {
> +    /// Occupied by something outside of this crate's control.
> +    Reserved,
> +    /// Free for future use, see [`Function::Unused`].
> +    Unused,
> +    /// A single device on function 0.
> +    Single(Function<'a>),
> +    /// A multifunction slot with one entry per function number.
> +    Multi(&'a [Function<'a>; FUNCTIONS_PER_DEVICE]),
> +    /// As many bridges of `kind` as are needed to hold `max` devices. The
> +    /// bridges share this slot by using one function number each.
> +    DynamicBridges { kind: PveBridge, max: u8 },
> +}
> +
> +/// The complete slot assignment of a guest, starting at the root bus.
> +///
> +/// A layout is a static description: it lists every address the crate can
> +/// hand out, regardless of which devices a concrete guest configures.
> +#[derive(Clone, Copy, Debug, PartialEq)]
> +pub struct Layout<'a> {
> +    /// Whether the root bus is PCIe. Only the root bus is affected, nested
> +    /// buses stay plain PCI.
> +    pub pcie: bool,
> +    pub root: [Slot<'a>; SLOTS_PER_BUS],
> +}
> +

I think the general Layout is quite sound, which is important because
chaning this later on might be quite painful.

I didn't notice any problems so far.

> +impl Layout<'_> {
> +    /// Looks up the address of a device in this layout.
> +    ///
> +    /// The first match wins, so the order of the layout is significant: a
> +    /// device may legitimately appear in two places, for example a SCSI
> +    /// controller that moves to its own bridge with `virtio-scsi-single`.
> +    pub fn find_device(&self, device: &Device) -> Result<PciAddress, LayoutError> {
> +        self.iter()
> +            .find_map(|used| (used.device == *device).then_some(used.addr))
> +            .ok_or(LayoutError::NoAddressFound(*device))
> +    }
> +
> +    /// Iterates over every address this layout hands out.
> +    ///
> +    /// Bridges are yielded before the devices behind them, so the result can
> +    /// be turned into a command line in order.
> +    pub fn iter(&self) -> AddressAssignments<'_> {
> +        AddressAssignments::new(self)
> +    }
> +}
> +
> +/// 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.
> +    pub multifunction: bool,
> +}
> +
> +/// Iterator over the address assignments of a [`Layout`], see
> +/// [`Layout::iter`].
> +#[derive(Debug)]
> +pub struct AddressAssignments<'a> {
> +    resolved: VecDeque<AddressAssignment>,
> +    bridge_slot_stack: Vec<(Slot<'a>, PciAddress)>,
> +}
> +
> +impl<'a> AddressAssignments<'a> {
> +    pub(crate) fn new(layout: &Layout<'a>) -> Self {
> +        let bus_type = if layout.pcie {
> +            Bus::Pcie(0)
> +        } else {
> +            Bus::Pci(0)
> +        };
> +        let mut stack: Vec<(Slot<'a>, PciAddress)> = Vec::new();
> +        for (device, slot) in layout.root.iter().enumerate().rev() {
> +            let address = PciAddress::new(bus_type, device as u8, 0);
> +            stack.push((*slot, address));
> +        }
> +        Self {
> +            bridge_slot_stack: stack,
> +            resolved: VecDeque::new(),
> +        }
> +    }
> +
> +    /// Resolves one function of a slot into the queue of address assignments.
> +    ///
> +    /// A bridge is queued before the devices behind it: fixed bridges push
> +    /// their slots onto the stack, which is only drained once the queue is
> +    /// empty again.
> +    fn handle_function(&mut self, addr: PciAddress, function: Function<'a>, multifunction: bool) {
> +        match function {
> +            Function::Reserved | Function::Unused => {}
> +            Function::Device(device) => {
> +                self.resolved.push_back(AddressAssignment {
> +                    addr,
> +                    device,
> +                    multifunction,
> +                });
> +            }
> +            Function::RootPort(bus, device) => {
> +                self.resolved.push_back(AddressAssignment {
> +                    addr,
> +                    device: Device::RootPort(bus),
> +                    multifunction,
> +                });
> +                self.resolved.push_back(AddressAssignment {
> +                    addr: PciAddress::new(bus, 0, 0),
> +                    device,
> +                    multifunction: false,
> +                });
> +            }
> +            Function::FixedBridge(bus_type, slots) => {
> +                self.resolved.push_back(AddressAssignment {
> +                    addr,
> +                    device: Device::Bridge(bus_type),
> +                    multifunction,
> +                });
> +                for (device, slot) in slots.iter().enumerate().rev() {
> +                    let addr = PciAddress::new(bus_type, device as u8, 0);
> +                    self.bridge_slot_stack.push((*slot, addr));
> +                }
> +            }
> +        }
> +    }
> +
> +    /// Resolves a slot of dynamic bridges into the queue of address
> +    /// assignments.
> +    ///
> +    /// Unlike fixed bridges, the devices behind them are queued right away,
> +    /// as they are generated from the bridge kind rather than listed in the
> +    /// layout.
> +    fn handle_dynamic_bridges(&mut self, mut addr: PciAddress, kind: PveBridge, max: u8) {
> +        // the bridges share one slot, so there cannot be more of them than the
> +        // slot has function numbers
> +        let num_bridges = max.div_ceil(USABLE_SLOTS_PER_BUS);
> +        debug_assert!(usize::from(num_bridges) <= FUNCTIONS_PER_DEVICE);

Why is this only asserted in debug builds?

> +
> +        for bridge_index in 0..num_bridges {
> +            addr.function = bridge_index;
> +            let bus = kind.new_bus(bridge_index);
> +            self.resolved.push_back(AddressAssignment {
> +                addr,
> +                device: Device::Bridge(bus),
> +                multifunction: bridge_index == 0 && num_bridges > 1,
> +            });
> +            for slot_nr in 1..=USABLE_SLOTS_PER_BUS {
> +                // u16 so that a bigger 'max' cannot overflow
> +                let index = u16::from(bridge_index) * u16::from(USABLE_SLOTS_PER_BUS)
> +                    + u16::from(slot_nr)
> +                    - 1;
> +                if index >= u16::from(max) {
> +                    break;
> +                }
> +                self.resolved.push_back(AddressAssignment {
> +                    addr: PciAddress::new(bus, slot_nr, 0),
> +                    device: kind.device(index as u8),
> +                    multifunction: false,
> +                });
> +            }
> +        }
> +    }
> +}
> +
> +impl Iterator for AddressAssignments<'_> {
> +    type Item = AddressAssignment;
> +
> +    fn next(&mut self) -> Option<Self::Item> {
> +        loop {
> +            if let Some(assignment) = self.resolved.pop_front() {
> +                return Some(assignment);
> +            }
> +            if let Some((slot, mut addr)) = self.bridge_slot_stack.pop() {
> +                match slot {
> +                    Slot::Reserved | Slot::Unused => {}
> +                    Slot::Single(function) => self.handle_function(addr, function, false),
> +                    Slot::Multi(functions) => {
> +                        for (function_nr, function) in functions.iter().enumerate() {
> +                            addr.function = function_nr as u8;
> +                            self.handle_function(addr, *function, function_nr == 0);
> +                        }
> +                    }
> +                    Slot::DynamicBridges { kind, max } => {
> +                        self.handle_dynamic_bridges(addr, kind, max)
> +                    }
> +                }
> +            }
> +
> +            if self.resolved.is_empty() && self.bridge_slot_stack.is_empty() {
> +                return None;
> +            }
> +        }
> +    }
> +}
> +

[snip]