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

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

On 9/28/26 2:53 PM, Elias Huhsovitz wrote:
> 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.
> 

yeah it took a few attempts before I landed on this, I think
this really shows the benefit of having a strongly typed language ;)

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

yeah this is up in the air for now. My motivation was that this
could at some point replace the 'qemu-server' git repository.

Outside of that we don't really have a need to model such pci layouts
though.

I don't have anything against putting it together with other parts
or pulling it out completely, but I had to start at something ;)

The only thing we probably don't want is to put them in the
`proxmox` workspace. IMO the code here does not belong there...
(But could be done ofc)

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

note, i copied the build env more or less verbatim
from the proxmox workspace, so all your comments
should apply there as well.

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


yep true, these should be used like:

pve_api_types::QemuConfigHostpciArray::MAX;

they are usize though, so some casting is required.>
>> +
>> +/// 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.

mhmm any idea? I wanted to keep the 'legacy' handling in this module,
but putting it into the bus would weaken that...

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

yeah i just started with these.

in my local version (not sent yet) I removed them alltogether in favor
of an 'addr' function that is exposed as a method of the `Machine` struct.

With that we can then call in perl (names pending):

$machine->addr('net0', 0); # second parameter is pci or pcie

> 
> [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().

as i said i wanted to keep the legacy stuff in one module,
but this can be fine too. If we want we can also keep that naming
in general, even though it's not true for e.g. arm/virt machines.
(it's not an 'ich9' port there ;) )

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

yeah this needs to be adapted either on the perl side too,
or we port the hostpci functions to rust.

the idea here was that currently the only way to differentiate
between hostpciX on a pci slot and hostpciX on a pcie slot
is which method is used 'print_pci_addr' or 'print_pcie_addr'

for some devices this is currently defined by which arch/machine
type we have (e.g. vga/ivshmem) and for hostpci it's defined
by its own config.

I did not really find a good way yet to encode that differently
besides using 'hostpcieX'.

> 
>> +            ("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.

yeah i started with DeviceSlot actually, but I found it too long too ;)

if you can come up with something better, please do tell!

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

Sure

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

yeah makes sense. Note that all the naming is rather rough currently,
just wanted to put something out at some point without bike-shedding
with myself^^

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

well for the 'v2' layout it will also be called so that'll not work then.

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

mostly legacy, but if we'd want to set it dynamically, we'd have to know
about the other root ports here as well (which we don't)
and it's harmless to set for the other functions

technically only the first in a slot needs it though, but since
we don't really know upfront which root ports are included, we
simply always add it.

>> +        ))
>> +    }
>> +
>> +    /// 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`.

yeah i like deviceplacement/assignment better

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

sure

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