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

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

I tested this locally and it seems to work fine so far.

from a high level view: 

* I really like the idea of the layout tree. I currently cannot think
  of any better way to do this.

* I also like that this is handled in rust and not perl. handling this
 in perl might be easier short term, but overall i don't see much
 benefit.

* Will this repo be used only for the qemu-server? If we find some other
  uses, perhaps a better name would be something like `pve-pci-rs` or
  `pve-guest-pci-rs`. IMO this repo should provide clean PCI topology and
  functions and shouldnt be limited to only the qemu-server.
	
  My question extends to the crate name. So maybe `pve-pci-rs` for the repo
  name and `pve-guest-pci` for the crate. (IMO we should have some
  kind of poll for this).

See other comments inline.

On Tue Sep 22, 2026 at 12:55 PM 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, so both implementations can be used interchangeably.
>
> 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]

> +	mkfs.erofs extensions/proxmox-workspace.raw build/sysext/workspace
> diff --git a/build.sh b/build.sh
> new file mode 100755
> index 0000000..12444c1
> --- /dev/null
> +++ b/build.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +
> +set -e

nit: IMO should be set -eu

> +
> +export CARGO=/usr/bin/cargo
> +export RUSTC=/usr/bin/rustc
> +
> +CRATE=$1
> +BUILDCMD=${BUILDCMD:-"dpkg-buildpackage -b -uc -us"}
> +BUILDDIR="${BUILDDIR:-"build"}"
> +TEST_CMD="${TEST_CMD:-"$CARGO test --all-features --all-targets --release"}"
> +
> +mkdir -p "${BUILDDIR}"
> +echo system >"${BUILDDIR}"/rust-toolchain
> +rm -rf ""${BUILDDIR}"/${CRATE}"

nit: redundant quotes, should be

rm -rf "${BUILDDIR}/${CRATE}"

> +
> +CONTROL="$PWD/${CRATE}/debian/control"
> +
> +if [ -e "$CONTROL" ]; then
> +    # check but only warn, debcargo fails anyway if crates are missing
> +    dpkg-checkbuilddeps $PWD/${CRATE}/debian/control || true
> +    [ "x$NOCONTROL" = 'x' ] && rm -f "$PWD/${CRATE}/debian/control"
> +fi
> +
> +debcargo package \
> +    --config "$PWD/${CRATE}/debian/debcargo.toml" \
> +    --changelog-ready \
> +    --no-overlay-write-back \
> +    --directory "$PWD/"${BUILDDIR}"/${CRATE}" \
> +    "${CRATE}" \
> +    "$(dpkg-parsechangelog -l "${CRATE}/debian/changelog" -SVersion | sed -e 's/-.*//')"
> +
> +cd ""${BUILDDIR}"/${CRATE}"

nit: redundant quotes, should be

cd "${BUILDDIR}/${CRATE}"

> +rm -f debian/source/format.debcargo.hint
> +${BUILDCMD}
> +
> +# needs all crates build-dependencies, which can be more than what debcargo assembles.
> +[ "x$NOTEST" = "x" ] && ${TEST_CMD}

nit: redundant parathesis around {TEST_CMD}. Can be simplified to
$TEST_CMD.

nit: why not just use [ -z ] here?

> +
> +[ "x$NOCONTROL" = "x" ] && cp debian/control "$CONTROL"

nit: also [ -z ] possible here.

[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..a96c519
> --- /dev/null
> +++ b/pve-qemu-server-pci/src/constants.rs
> @@ -0,0 +1,15 @@
> +//! Upper bounds for the per-guest device counts the layouts reserve slots for.
> +//!
> +//! These mirror the limits enforced by the qemu-server configuration schema.
> +
> +pub const MAX_NET_DEVICES: u8 = 32;
> +pub const MAX_SCSI_DEVICES: u8 = 31;
> +pub const MAX_VIRTIO_BLK_DEVICES: u8 = 16;
> +pub const MAX_VIRTIOFS_DEVICES: u8 = 10;
> +pub const MAX_HOSTPCI_DEVICES: u8 = 16;

To my knowledge, all these constants are already defined in 
proxmox/pve-api-types

Perhaps we can import this somehow, I would like to avoid multiple
sources of truth.

> +
> +/// Number of slots on a PCI bus. Slot 0x00 is always taken by the bridge
> +/// itself, so only [`USABLE_BRIDGE_SLOTS`] of them can hold a device.
> +pub(crate) const BRIDGE_SLOT_NUM: usize = 32;

nit: naming: Maybe something like SLOTS_PER_BUS is more accurate. But current
naming here is fine.

> +pub(crate) const USABLE_BRIDGE_SLOTS: u8 = BRIDGE_SLOT_NUM as u8 - 1;

nit: naming: Same thing here, could be USEABLE_SLOTS_PER_BUS.

> +pub(crate) const FUNCTIONS_NUM: usize = 8;

nit: naming: could be FUNCTIONS_PER_SLOT.

> diff --git a/pve-qemu-server-pci/src/error.rs b/pve-qemu-server-pci/src/error.rs
> new file mode 100644
> index 0000000..6cd5abd
> --- /dev/null
> +++ b/pve-qemu-server-pci/src/error.rs
> @@ -0,0 +1,23 @@
> +use std::{error::Error, fmt::Display};
> +
> +/// Errors that can occur while looking up a PCI address.
> +#[derive(Clone, Copy, Debug, PartialEq, Eq)]
> +#[non_exhaustive]
> +pub enum PciConfigError {

nit: naming: The current list of error are more about the layout lookup,
so we could call this something like `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,
> +}
> +
> +impl Display for PciConfigError {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        let text = match self {
> +            PciConfigError::InvalidConfigId => "invalid configuration id",
> +            PciConfigError::NoAddressFound => "no such address found",
> +        };
> +        f.write_str(text)
> +    }
> +}
> +
> +impl Error for PciConfigError {}
> diff --git a/pve-qemu-server-pci/src/layout/legacy.rs b/pve-qemu-server-pci/src/layout/legacy.rs
> new file mode 100644
> index 0000000..40b3265
> --- /dev/null

[snip]

> +/// Looks up the PCI address of a device, see [`crate::print_pci_addr`].
> +pub(crate) fn print_pci_addr(
> +    id: &str,
> +    arch: ClusterResourceHostArch,
> +) -> Result<String, PciConfigError> {
> +    let new_id = map_legacy_pci_id(id);
> +    // aarch64 has no PCI host bridge, only a PCIe one
> +    let pcie = arch == ClusterResourceHostArch::Aarch64;
> +    let layout = get_legacy_pci_layout(pcie, is_virtio_scsi_single_id(new_id));
> +    let addr = layout.find_device(&new_id.parse()?)?;
> +    Ok(addr.to_qemu_addr())
> +}
> +
> +/// Looks up the PCIe address of a device, see [`crate::print_pcie_addr`].
> +pub(crate) fn print_pcie_addr(id: &str) -> Result<String, PciConfigError> {
> +    let (new_id, win7) = map_legacy_pcie_id(id);
> +    let layout = if win7 {
> +        LEGACY_PCIE_LAYOUT_WIN7
> +    } else {
> +        LEGACY_PCIE_LAYOUT
> +    };
> +    let addr = layout.find_device(&new_id.parse()?)?;
> +    Ok(addr.to_qemu_addr())
> +}
> +
> +/// Renders a PCIe root port device, see [`crate::print_pcie_root_port`].
> +pub(crate) fn print_pcie_root_port(index: u8) -> Result<String, PciConfigError> {
> +    // root ports are numbered from one in the layout, hostpci devices from zero
> +    let index = index.checked_add(1).ok_or(PciConfigError::NoAddressFound)?;
> +    let res = LEGACY_PCIE_LAYOUT
> +        .print_root_port(index)?
> +        .replace("pve.root-port", "ich9-pcie-port-");

I feel like the Bus should just provide the correct getter here, instead
of calling replace.

> +
> +    Ok(res)
> +}
> +

I don't like the naming of these functions:
- print_pcie_addr
- print_pcie_root_port

The functions return values, and should be named as such. So either i
would just drop the prefix to:

- pcie_addr
- pcie_root_port

Or describe that they are "getting" the respective value:

- get_pcie_addr
- get_pcie_root_port

Since we are also calling `to_qemu_addr`, we could include the
formatting in the name:

- format_pcie_addr
- format_pcie_root_port

(I know that they mirror the legacy naming scheme from the perl code,
but I belive we shouldnt continue with this style here).

[snip]

> diff --git a/pve-qemu-server-pci/src/types/bus.rs b/pve-qemu-server-pci/src/types/bus.rs
> new file mode 100644
> index 0000000..37ad57d

[snip]

> +pub enum Bus {
> +    /// A PCI bus. Index 0 is the root bus of a non-PCIe machine.
> +    Pci(u8),
> +    /// A PCIe bus. Index 0 is the root bus of a PCIe machine.
> +    Pcie(u8),
> +    /// The bus behind a PCIe root port, which can hold a single device.
> +    RootPort(u8),
> +    /// The PCI bus behind the q35 DMI-to-PCI bridge.
> +    Pcidmi,
> +    /// A bridge for the guest's built-in devices.
> +    Sys(u8),
> +    /// One of the bridges PVE adds per device kind, see [`PveBridge`].
> +    Pve { kind: PveBridge, index: u8 },
> +}
> +
> +type BusConstructor = fn(u8) -> Bus;
> +
> +impl Bus {
> +    /// Renders the bus as the id QEMU refers to it by.
> +    ///
> +    /// This is the inverse of the [`std::str::FromStr`] implementation.
> +    pub fn to_id(self) -> String {
> +        match self {
> +            Bus::Pci(i) => format!("pci.{i}"),
> +            Bus::Pcie(i) => format!("pcie.{i}"),
> +            Bus::RootPort(i) => format!("pve.root-port{i}"),
> +            Bus::Sys(i) => format!("pve.sys{i}"),
> +            Bus::Pcidmi => "pcidmi".to_string(),
> +            Bus::Pve { kind, index } => {
> +                format!("pve.{kind}{index}")
> +            }
> +        }
> +    }

IMO Should have a function like this for the `to_qemu_addr` call:
pub fn to_qemu_q35_id(self) -> String {
    match self {
        Bus::RootPort(i) => format!("ich9-pcie-port-{i}"),
        other => other.to_id(),
    }
}

This way we don't have to manually re-format the returned id after
calling to_id().

> +
> +    /// The QEMU device model needed to create this bus below `parent`, or
> +    /// `None` if the bus is not created by a bridge device.
> +    pub fn bridge_model(self, parent: Bus) -> Option<&'static str> {
> +        match self {
> +            Bus::Pcie(_) | Bus::RootPort(_) => None,
> +            Bus::Pcidmi => Some("i82801b11-bridge"),
> +            Bus::Sys(_) | Bus::Pci(_) | Bus::Pve { .. } => Some(match parent {
> +                Bus::Pcie(_) | Bus::RootPort(_) => "pcie-pci-bridge",
> +                _ => "pci-bridge",
> +            }),
> +        }
> +    }
> +}
> +

[snip]

> diff --git a/pve-qemu-server-pci/src/types/device.rs b/pve-qemu-server-pci/src/types/device.rs
> new file mode 100644
> index 0000000..f63bf39
> +

[snip]

> +type DeviceConstructor = fn(u8) -> Device;
> +
> +/// Parses the configuration id of a device, for example `net0` or `hostpci3`.
> +impl FromStr for Device {
> +    type Err = PciConfigError;
> +
> +    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),

AFAIK, the "hostpcie" is never used in the perl code. We always default
to "hostpci". 

This might be an issue with the new v2 layout (See Patch 3 for
reference).

Will this be changed in the phase 2 patches, or am misreading
something here?

> +            ("hostpci", Device::Hostpci),
> +        ];
> +
> +        for (prefix, c) in PREFIXES {
> +            if let Some(index) = s.strip_prefix(prefix) {
> +                let index = index.parse().map_err(|_| PciConfigError::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,
> +            "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(PciConfigError::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..18977af
> --- /dev/null
> +++ b/pve-qemu-server-pci/src/types/layout.rs
> @@ -0,0 +1,345 @@
> +use std::collections::VecDeque;
> +
> +use crate::constants::{BRIDGE_SLOT_NUM, FUNCTIONS_NUM, USABLE_BRIDGE_SLOTS};
> +use crate::{Bus, Device, PciAddress, PciConfigError, 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>; BRIDGE_SLOT_NUM]),
> +    /// A PCIe root port with the single device attached to it.
> +    RootPort(u8, Device),
> +}
> +
> +/// What sits at one slot of a bus.
> +#[derive(Clone, Copy, Debug, PartialEq, Eq)]
> +pub enum Slot<'a> {

nit naming: I am kind of torn about the naming here. The code has
Slot::Multi which reads like a multi-function slot, instead of a
multi-function device. I thought about re-naming it to something like
`DeviceSlot`, but this seems too verbose.

But I am unable to come up with a better name.

> +    /// 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 holding up to [`FUNCTIONS_NUM`] entries.
> +    Multi(&'a [Function<'a>; FUNCTIONS_NUM]),
> +    /// As many bridges of `kind` as are needed to hold `max` devices. The
> +    /// bridges share this slot by using one function number each.
> +    DynamicBridge { kind: PveBridge, max: u8 },

nit: name: IMO name it DynamicBridges (plural) since there can be
multiple. 

> +}
> +
> +/// 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 DeviceLayout<'a> {

IMO: Could just be called `Layout` since we are inside of the PCI crate.
Or we have other layouts in the future (.e.g, CPU, NUMA, USB, machine,
etc) then perhaps something like `PCILayout` | `PciLayout` might be
better.


> +    /// Whether the root bus is PCIe. Only the root bus is affected, nested
> +    /// buses stay plain PCI.
> +    pub pcie: bool,
> +    pub root: [Slot<'a>; BRIDGE_SLOT_NUM],
> +}
> +
> +impl DeviceLayout<'_> {
> +    /// Renders the QEMU `-device` arguments for the `index`th PCIe root port
> +    /// of this layout.
> +    pub fn print_root_port(&self, index: u8) -> Result<String, PciConfigError> {
> +        let pci_addr = self
> +            .find_root_port(index)
> +            .ok_or(PciConfigError::NoAddressFound)?;
> +
> +        let id = Bus::RootPort(index).to_id();

print_root_port is always used in a q35 machine context, e.g. in
legacy.rs::print_pcie_root_port. So what about just calling the correct
id here? e.g.,

let id = Bus::RootPort(index).to_qemu_q35_id();

See the to_qemu_q35_id() function in the Bus implementation.

> +
> +        let bus = format!("bus={}", pci_addr.bus.to_id());
> +        let addr = format!("addr={:02x}.{}", pci_addr.device, pci_addr.function);
> +
> +        Ok(format!(
> +            "pcie-root-port,id={id},{addr},x-speed=16,x-width=32,multifunction=on,{bus},port={index},chassis={index}"

Why is `multifunction=on` hardcoded here? Is there any specific reason
or is this just to match the legacy perl code?

> +        ))
> +    }
> +
> +    /// 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, PciConfigError> {
> +        self.iter()
> +            .find_map(|used| (used.device == *device).then_some(used.addr))
> +            .ok_or(PciConfigError::NoAddressFound)
> +    }
> +
> +    fn find_root_port(&self, i: u8) -> Option<PciAddress> {
> +        self.iter()
> +            .find_map(|used| (used.device == Device::RootPort(i)).then_some(used.addr))
> +    }
> +
> +    /// 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) -> UsedSlotIterator<'_> {
> +        UsedSlotIterator::new(self)
> +    }
> +}
> +
> +/// A slot of a layout that holds a device, together with its address.
> +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
> +pub struct UsedSlot {

nit: name: `UsedSlot` seems like it is trying to describe a state to me.
What about something like `DevicePlacement` | `SlotAssignment`.

> +    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 used slots of a [`DeviceLayout`], see
> +/// [`DeviceLayout::iter`].
> +#[derive(Debug)]
> +pub struct UsedSlotIterator<'a> {
> +    resolved_slots: VecDeque<UsedSlot>,
> +    bridge_slot_stack: Vec<(Slot<'a>, PciAddress)>,
> +}
> +
> +impl<'a> UsedSlotIterator<'a> {
> +    pub(crate) fn new(layout: &DeviceLayout<'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_slots: VecDeque::new(),
> +        }
> +    }
> +
> +    /// Resolves one function of a slot into the queue of used slots.
> +    ///
> +    /// 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_slots.push_back(UsedSlot {
> +                    addr,
> +                    device,
> +                    multifunction,
> +                });
> +            }
> +            Function::RootPort(i, device) => {
> +                self.resolved_slots.push_back(UsedSlot {
> +                    addr,
> +                    device: Device::RootPort(i),
> +                    multifunction,
> +                });
> +                self.resolved_slots.push_back(UsedSlot {
> +                    addr: PciAddress::new(Bus::RootPort(i), 0, 0),
> +                    device,
> +                    multifunction: false,
> +                });
> +            }
> +            Function::FixedBridge(bus_type, slots) => {
> +                self.resolved_slots.push_back(UsedSlot {
> +                    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));
> +                }
> +            }
> +        }
> +    }
> +}
> +

nit: Iterator Indentation is a bit too deep IMO. I recommend factoring
out the logic for each Slot type.

> +impl Iterator for UsedSlotIterator<'_> {
> +    type Item = UsedSlot;
> +
> +    fn next(&mut self) -> Option<Self::Item> {
> +        loop {
> +            if let Some(slot) = self.resolved_slots.pop_front() {
> +                return Some(slot);
> +            }
> +            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::DynamicBridge { kind, max } => {
> +                        // 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_BRIDGE_SLOTS);
> +                        debug_assert!(usize::from(num_bridges) <= FUNCTIONS_NUM);
> +
> +                        for bridge_index in 0..num_bridges {
> +                            addr.function = bridge_index;
> +                            let bus = Bus::Pve {
> +                                kind,
> +                                index: bridge_index,
> +                            };
> +                            self.resolved_slots.push_back(UsedSlot {
> +                                addr,
> +                                device: Device::Bridge(bus),
> +                                multifunction: bridge_index == 0 && num_bridges > 1,
> +                            });
> +                            for slot_nr in 1..=USABLE_BRIDGE_SLOTS {
> +                                // u16 so that a bigger 'max' cannot overflow
> +                                let index = u16::from(bridge_index)
> +                                    * u16::from(USABLE_BRIDGE_SLOTS)
> +                                    + u16::from(slot_nr)
> +                                    - 1;
> +                                if index >= u16::from(max) {
> +                                    break;
> +                                }
> +                                self.resolved_slots.push_back(UsedSlot {
> +                                    addr: PciAddress::new(bus, slot_nr, 0),
> +                                    device: kind.device(index as u8),
> +                                    multifunction: false,
> +                                });
> +                            }
> +                        }
> +                    }
> +                }
> +            }
> +
> +            if self.resolved_slots.is_empty() && self.bridge_slot_stack.is_empty() {
> +                return None;
> +            }
> +        }
> +    }
> +}
> +

[snip]

> 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..5be601e
> --- /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 {
> +        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.to_id().replace("pve.root-port", "ich9-pcie-port-");

Same thing here as in `print_pcie_root_port`. I feel like the call
should just be something like 

let bus = self.bus.to_qemu_q35_id()

See the bus implementation as a reference

> +        let function = if self.function > 0 {
> +            format!(".{}", self.function)
> +        } else {
> +            String::new()
> +        };
> +
> +        format!(",bus={bus},addr=0x{:x}{function}", self.device)
> +    }
> +}

[snip]