From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id DE1361FF09B for ; Mon, 28 Sep 2026 14:54:06 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id AAE18216FA; Mon, 28 Sep 2026 14:53:59 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Mon, 28 Sep 2026 14:53:48 +0200 Message-Id: Subject: Re: [PATCH pve-qemu-server-rs 1/9] add pve-qemu-server-pci crate for guest PCI address generation From: "Elias Huhsovitz" To: "Dominik Csapak" , Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.20.0 References: <20260922105550.2084078-1-d.csapak@proxmox.com> <20260922105550.2084078-2-d.csapak@proxmox.com> In-Reply-To: <20260922105550.2084078-2-d.csapak@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790600028491 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.401 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_MAILER 2 Automated Mailer Tag Left in Email RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: F3PPIDCRYLQMXFZP4TYZDARTHJBQEDLA X-Message-ID-Hash: F3PPIDCRYLQMXFZP4TYZDARTHJBQEDLA X-MailFrom: e.huhsovitz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: I tested this locally and it seems to work fine so far. from a high level view:=20 * 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. =09 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 [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=3D/usr/bin/cargo > +export RUSTC=3D/usr/bin/rustc > + > +CRATE=3D$1 > +BUILDCMD=3D${BUILDCMD:-"dpkg-buildpackage -b -uc -us"} > +BUILDDIR=3D"${BUILDDIR:-"build"}" > +TEST_CMD=3D"${TEST_CMD:-"$CARGO test --all-features --all-targets --rele= ase"}" > + > +mkdir -p "${BUILDDIR}" > +echo system >"${BUILDDIR}"/rust-toolchain > +rm -rf ""${BUILDDIR}"/${CRATE}" nit: redundant quotes, should be rm -rf "${BUILDDIR}/${CRATE}" > + > +CONTROL=3D"$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" =3D '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 | se= d -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 debca= rgo assembles. > +[ "x$NOTEST" =3D "x" ] && ${TEST_CMD} nit: redundant parathesis around {TEST_CMD}. Can be simplified to $TEST_CMD. nit: why not just use [ -z ] here? > + > +[ "x$NOCONTROL" =3D "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/s= rc/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 slo= ts for. > +//! > +//! These mirror the limits enforced by the qemu-server configuration sc= hema. > + > +pub const MAX_NET_DEVICES: u8 =3D 32; > +pub const MAX_SCSI_DEVICES: u8 =3D 31; > +pub const MAX_VIRTIO_BLK_DEVICES: u8 =3D 16; > +pub const MAX_VIRTIOFS_DEVICES: u8 =3D 10; > +pub const MAX_HOSTPCI_DEVICES: u8 =3D 16; To my knowledge, all these constants are already defined in=20 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 bridg= e > +/// itself, so only [`USABLE_BRIDGE_SLOTS`] of them can hold a device. > +pub(crate) const BRIDGE_SLOT_NUM: usize =3D 32; nit: naming: Maybe something like SLOTS_PER_BUS is more accurate. But curre= nt naming here is fine. > +pub(crate) const USABLE_BRIDGE_SLOTS: u8 =3D BRIDGE_SLOT_NUM as u8 - 1; nit: naming: Same thing here, could be USEABLE_SLOTS_PER_BUS. > +pub(crate) const FUNCTIONS_NUM: usize =3D 8; nit: naming: could be FUNCTIONS_PER_SLOT. > diff --git a/pve-qemu-server-pci/src/error.rs b/pve-qemu-server-pci/src/e= rror.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 abou= t. > + 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 =3D match self { > + PciConfigError::InvalidConfigId =3D> "invalid configuration = id", > + PciConfigError::NoAddressFound =3D> "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-p= ci/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 { > + let new_id =3D map_legacy_pci_id(id); > + // aarch64 has no PCI host bridge, only a PCIe one > + let pcie =3D arch =3D=3D ClusterResourceHostArch::Aarch64; > + let layout =3D get_legacy_pci_layout(pcie, is_virtio_scsi_single_id(= new_id)); > + let addr =3D 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 { > + let (new_id, win7) =3D map_legacy_pcie_id(id); > + let layout =3D if win7 { > + LEGACY_PCIE_LAYOUT_WIN7 > + } else { > + LEGACY_PCIE_LAYOUT > + }; > + let addr =3D 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 { > + // root ports are numbered from one in the layout, hostpci devices f= rom zero > + let index =3D index.checked_add(1).ok_or(PciConfigError::NoAddressFo= und)?; > + let res =3D 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/s= rc/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 =3D 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) =3D> format!("pci.{i}"), > + Bus::Pcie(i) =3D> format!("pcie.{i}"), > + Bus::RootPort(i) =3D> format!("pve.root-port{i}"), > + Bus::Sys(i) =3D> format!("pve.sys{i}"), > + Bus::Pcidmi =3D> "pcidmi".to_string(), > + Bus::Pve { kind, index } =3D> { > + 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) =3D> format!("ich9-pcie-port-{i}"), other =3D> 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(_) =3D> None, > + Bus::Pcidmi =3D> Some("i82801b11-bridge"), > + Bus::Sys(_) | Bus::Pci(_) | Bus::Pve { .. } =3D> Some(match = parent { > + Bus::Pcie(_) | Bus::RootPort(_) =3D> "pcie-pci-bridge", > + _ =3D> "pci-bridge", > + }), > + } > + } > +} > + [snip] > diff --git a/pve-qemu-server-pci/src/types/device.rs b/pve-qemu-server-pc= i/src/types/device.rs > new file mode 100644 > index 0000000..f63bf39 > + [snip] > +type DeviceConstructor =3D fn(u8) -> Device; > + > +/// Parses the configuration id of a device, for example `net0` or `host= pci3`. > +impl FromStr for Device { > + type Err =3D PciConfigError; > + > + fn from_str(s: &str) -> Result { > + // NOTE: order matters, the longer prefixes have to come first s= o that > + // 'virtio' does not shadow 'virtioscsi' and 'hostpci' not 'host= pcie'. > + const PREFIXES: &[(&str, DeviceConstructor)] =3D &[ > + ("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".=20 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) =3D s.strip_prefix(prefix) { > + let index =3D index.parse().map_err(|_| PciConfigError::= InvalidConfigId)?; > + return Ok(c(index)); > + } > + } > + > + let class =3D match s { > + "viommu" =3D> Device::Viommu, > + "vga" =3D> Device::Vga(0), > + "vga1" =3D> Device::Vga(1), > + "vga2" =3D> Device::Vga(2), > + "vga3" =3D> Device::Vga(3), > + "xhci" =3D> Device::XhciController(0), > + "ahci0" =3D> Device::Ahci, > + "balloon0" =3D> Device::Balloon, > + "watchdog" =3D> Device::Watchdog, > + "qga0" =3D> Device::GuestAgent, > + "spice" =3D> Device::SpiceSerial, > + "rng0" =3D> Device::Rng, > + "audio0" =3D> Device::Audio, > + "ivshmem" =3D> Device::Ivshmem, > + // legacy rules > + "ehci" | "piix3" =3D> Device::Piix3Controller, > + "legacy-igd" =3D> Device::Vga(0), > + "pci.2-igd" =3D> Device::Bridge(Bus::Pci(99)), > + other =3D> { > + if let Ok(bridge) =3D 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-pc= i/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_SLO= TS}; > +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 examp= le by > + /// the machine type's own configuration file. > + Reserved, > + /// Free for future use. Kept explicit so that the surrounding entri= es 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. T= he > + /// 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.=20 > +} > + > +/// The complete slot assignment of a guest, starting at the root bus. > +/// > +/// A layout is a static description: it lists every address the crate c= an > +/// 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, nes= ted > + /// 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 { > + let pci_addr =3D self > + .find_root_port(index) > + .ok_or(PciConfigError::NoAddressFound)?; > + > + let id =3D 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 =3D Bus::RootPort(index).to_qemu_q35_id(); See the to_qemu_q35_id() function in the Bus implementation. > + > + let bus =3D format!("bus=3D{}", pci_addr.bus.to_id()); > + let addr =3D format!("addr=3D{:02x}.{}", pci_addr.device, pci_ad= dr.function); > + > + Ok(format!( > + "pcie-root-port,id=3D{id},{addr},x-speed=3D16,x-width=3D32,m= ultifunction=3Don,{bus},port=3D{index},chassis=3D{index}" Why is `multifunction=3Don` 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 { > + self.iter() > + .find_map(|used| (used.device =3D=3D *device).then_some(used= .addr)) > + .ok_or(PciConfigError::NoAddressFound) > + } > + > + fn find_root_port(&self, i: u8) -> Option { > + self.iter() > + .find_map(|used| (used.device =3D=3D Device::RootPort(i)).th= en_some(used.addr)) > + } > + > + /// Iterates over every address this layout hands out. > + /// > + /// Bridges are yielded before the devices behind them, so the resul= t 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 ar= e 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, > + bridge_slot_stack: Vec<(Slot<'a>, PciAddress)>, > +} > + > +impl<'a> UsedSlotIterator<'a> { > + pub(crate) fn new(layout: &DeviceLayout<'a>) -> Self { > + let bus_type =3D if layout.pcie { > + Bus::Pcie(0) > + } else { > + Bus::Pci(0) > + }; > + let mut stack: Vec<(Slot<'a>, PciAddress)> =3D Vec::new(); > + for (device, slot) in layout.root.iter().enumerate().rev() { > + let address =3D 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 p= ush > + /// 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 =3D> {} > + Function::Device(device) =3D> { > + self.resolved_slots.push_back(UsedSlot { > + addr, > + device, > + multifunction, > + }); > + } > + Function::RootPort(i, device) =3D> { > + 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) =3D> { > + self.resolved_slots.push_back(UsedSlot { > + addr, > + device: Device::Bridge(bus_type), > + multifunction, > + }); > + for (device, slot) in slots.iter().enumerate().rev() { > + let addr =3D 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 =3D UsedSlot; > + > + fn next(&mut self) -> Option { > + loop { > + if let Some(slot) =3D self.resolved_slots.pop_front() { > + return Some(slot); > + } > + if let Some((slot, mut addr)) =3D self.bridge_slot_stack.pop= () { > + match slot { > + Slot::Reserved | Slot::Unused =3D> {} > + Slot::Single(function) =3D> self.handle_function(add= r, function, false), > + Slot::Multi(functions) =3D> { > + for (function_nr, function) in functions.iter().= enumerate() { > + addr.function =3D function_nr as u8; > + self.handle_function(addr, *function, functi= on_nr =3D=3D 0); > + } > + } > + Slot::DynamicBridge { kind, max } =3D> { > + // the bridges share one slot, so there cannot b= e more > + // of them than the slot has function numbers > + let num_bridges =3D max.div_ceil(USABLE_BRIDGE_S= LOTS); > + debug_assert!(usize::from(num_bridges) <=3D FUNC= TIONS_NUM); > + > + for bridge_index in 0..num_bridges { > + addr.function =3D bridge_index; > + let bus =3D Bus::Pve { > + kind, > + index: bridge_index, > + }; > + self.resolved_slots.push_back(UsedSlot { > + addr, > + device: Device::Bridge(bus), > + multifunction: bridge_index =3D=3D 0 && = num_bridges > 1, > + }); > + for slot_nr in 1..=3DUSABLE_BRIDGE_SLOTS { > + // u16 so that a bigger 'max' cannot ove= rflow > + let index =3D u16::from(bridge_index) > + * u16::from(USABLE_BRIDGE_SLOTS) > + + u16::from(slot_nr) > + - 1; > + if index >=3D 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-serv= er-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 a= nd > +/// 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=3D...,addr=3D...` suffix of a Q= EMU > + /// `-device` argument, including the leading comma. > + /// > + /// The function number is only emitted when it is not zero, matchin= g what > + /// the Perl implementation produces. > + pub fn to_qemu_addr(&self) -> String { > + let bus =3D 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=20 let bus =3D self.bus.to_qemu_q35_id() See the bus implementation as a reference > + let function =3D if self.function > 0 { > + format!(".{}", self.function) > + } else { > + String::new() > + }; > + > + format!(",bus=3D{bus},addr=3D0x{:x}{function}", self.device) > + } > +} [snip]