Re: [PATCH pve-qemu-server-rs v2 4/9] pci: layout: add grouped PCI and PCIe layouts

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

Comments inline.

On Thu Oct 15, 2026 at 10:10 AM CEST, Dominik Csapak wrote:
> The legacy layout grew over many releases and it shows: devices of one
> kind are spread over several buses, the bus a device lands on depends on
> how many devices of other kinds exist, and the remaining free slots are
> scattered. That makes it hard to raise any of the per-kind limits
> without further scattering the types over the available addresses.
>
> Add a second set of layouts that gives every device kind its own set of
> bridges instead. A device's address then only depends on its own index,
> so raising a limit adds bridges at the end rather than shifting anything
> that exists, and the built-in devices move to a bridge of their own to
> keep the root bus free.
>
> The new `layout` field of `HardwareConfig` selects which set is used,
> so `pci_bridge_for_device` now takes the config too. Since the layout
> is part of the guest ABI, the grouped one is only intended for new
> guests.
>
> `pci_bus_devices` returns its bridges and the hostpcie root ports,
> with the same function 0 handling as for the legacy layout.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---

[snip]

> +
> +fn layout(pcie: bool) -> &'static Layout<'static> {
> +    if pcie { &PCIE_LAYOUT } else { &PCI_LAYOUT }
> +}
> +
> +pub(crate) fn address(hw: &HardwareConfig, id: &str) -> Result<PciAddress, LayoutError> {
> +    let layout = layout(hw.has_pcie());
> +    layout.find_device(&id.parse()?)
> +}
> +
> +pub(crate) fn pci_bridge_for_device(hw: &HardwareConfig, id: &str) -> Option<Bus> {
> +    let addr = address(hw, id).ok()?;
> +    Some(addr.bus)
> +}
> +
> +pub(crate) fn pci_bus_devices(hw: &HardwareConfig) -> Result<Vec<String>, LayoutError> {
> +    let mut bridges = Vec::new();
> +    let layout = layout(hw.has_pcie());
> +    let mut chassis_nr = 1;
> +    for bridge in layout
> +        .iter()
> +        .filter(|d| matches!(d.device, Device::Bridge(_)))
> +    {
> +        let addr = bridge.addr;
> +        let (model, id) = match bridge.device {
> +            Device::Bridge(bus) => (bus.bridge_model(addr.bus), bus),
> +            _ => continue,
> +        };
> +        let Some(model) = model else {
> +            continue;
> +        };
> +        let mf = if addr.function == 0 {
> +            ",multifunction=on"

Isn't the addr.function == 0 already encoded in AddressAssignment?

So we can just check for:

let mf = bridge.multifunction {
           ",multifunction=on"

> +        } else {
> +            ""
> +        };
> +
> +        let chassis = if model == "pci-bridge" {
> +            let text = format!(",chassis_nr={chassis_nr}");
> +            chassis_nr += 1;

We could consider deriving chassis_nr from something instead letting the
layout iteration assign a sequential ordering.

Similarly to how the legacy layout does it.

> +            text
> +        } else {
> +            String::new()
> +        };
> +        bridges.push("-device".to_string());
> +        bridges.push(format!(
> +            "{model},id={id}{chassis}{}{mf}",
> +            addr.to_qemu_addr()
> +        ));
> +    }
> +
> +    for port in layout.root_ports(&hw.hostpcie()) {
> +        bridges.push("-device".to_string());
> +        bridges.push(port.to_qemu_device());
> +    }
> +
> +    Ok(bridges)
> +}
> +
  
[snip]  

> diff --git a/pve-qemu-server-pci/src/layout/mod.rs b/pve-qemu-server-pci/src/layout/mod.rs
> index 5439c17..9a637ea 100644
> --- a/pve-qemu-server-pci/src/layout/mod.rs
> +++ b/pve-qemu-server-pci/src/layout/mod.rs
> @@ -13,6 +13,8 @@ macro_rules! single_device {
>  
>  pub(crate) mod legacy;
>  
> +pub mod grouped;
> +
>  /// Constructs a correctly sized list of bridge slots, starting at address 0x01
>  /// because 0x00 is always taken by the bridge itself.
>  ///
> diff --git a/pve-qemu-server-pci/src/lib.rs b/pve-qemu-server-pci/src/lib.rs
> index a5d82a4..5417fa5 100644
> --- a/pve-qemu-server-pci/src/lib.rs
> +++ b/pve-qemu-server-pci/src/lib.rs
> @@ -21,10 +21,45 @@ pub use error::LayoutError;
>  
>  pub mod layout;
>  
> +/// The set of layouts a guest's addresses are taken from.
> +#[derive(Default, Debug, Clone, Copy, PartialEq, Eq)]
> +pub enum PciLayout {
> +    /// The layouts reproducing the addresses of the Perl implementation.
> +    #[default]
> +    Legacy,
> +    /// The layouts giving every device kind its own set of bridges, see
> +    /// [`layout::grouped`].
> +    Grouped,
> +}
> +

IMHO the code for PciLayout enum should live inside layout module.
e.g. in layout/mod.rs

> +impl std::str::FromStr for PciLayout {
> +    type Err = LayoutError;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        match s {
> +            "legacy" => Ok(Self::Legacy),
> +            "grouped" => Ok(Self::Grouped),
> +            _ => Err(LayoutError::InvalidLayout),
> +        }
> +    }
> +}
> +
> +impl std::fmt::Display for PciLayout {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        f.write_str(match self {
> +            PciLayout::Legacy => "legacy",
> +            PciLayout::Grouped => "grouped",
> +        })
> +    }
> +}
> +
>  /// Returns the number of the PCI bridge `id` sits on, or `None` if it is on
>  /// the root bus or not a known device.
> -pub fn pci_bridge_for_device(id: &str) -> Option<Bus> {
> -    layout::legacy::pci_bridge_for_device(id)
> +pub fn pci_bridge_for_device(hw: &HardwareConfig, id: &str) -> Option<Bus> {
> +    match hw.layout {
> +        PciLayout::Legacy => layout::legacy::pci_bridge_for_device(id),
> +        PciLayout::Grouped => layout::grouped::pci_bridge_for_device(hw, id),
> +    }
>  }

nit naming: The function is called pci_bridge_for_device() but returns a Bus.
IMO the semantics here are a bit ambiguous.

What about renaming it to something like:
bridge_bus_for_device()

IMO we can also omit the "pci" prefix, since we are in the PCI crate.

>  
>  /// Returns the QEMU arguments for all devices that form the buses `hw`
> @@ -33,10 +68,16 @@ pub fn pci_bridge_for_device(id: &str) -> Option<Bus> {
>  /// They are ordered so that every bus is created before the devices on it,
>  /// and have to be passed before any other device that sits on these buses.
>  pub fn pci_bus_devices(hw: &HardwareConfig) -> Result<Vec<String>, LayoutError> {

nit naming: we could also consider dropping the pci prefix of the
function name to "bus_devices".

> -    layout::legacy::pci_bus_devices(hw)
> +    match hw.layout {
> +        PciLayout::Legacy => layout::legacy::pci_bus_devices(hw),
> +        PciLayout::Grouped => layout::grouped::pci_bus_devices(hw),
> +    }
>  }
>  
>  /// Returns the address of the device with the configuration id `id`.
>  pub fn address(hw: &HardwareConfig, id: &str) -> Result<PciAddress, LayoutError> {
> -    layout::legacy::address(hw, id)
> +    match hw.layout {
> +        PciLayout::Legacy => layout::legacy::address(hw, id),
> +        PciLayout::Grouped => layout::grouped::address(hw, id),
> +    }
>  }
> diff --git a/pve-qemu-server-pci/src/types/mod.rs b/pve-qemu-server-pci/src/types/mod.rs
> index 1605066..43b08c0 100644
> --- a/pve-qemu-server-pci/src/types/mod.rs
> +++ b/pve-qemu-server-pci/src/types/mod.rs
> @@ -3,6 +3,9 @@ pub use bus::{Bus, PveBridge};
>  
>  mod device;
>  pub use device::Device;
> +// only needed to enumerate all device kinds in tests
> +#[cfg(test)]
> +pub(crate) use device::DeviceDiscriminants;
>  
>  mod pci_address;
>  pub use pci_address::PciAddress;