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

[Fiona Ebner <f.ebner@proxmox.com>]

Note that I didn't go in-depth here yet when reviewing, just some things
I noted:

Am 06.11.26 um 2:27 PM schrieb Dominik Csapak:
> 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.

There currently are 9 (or 13 for pcie, or 9 for the standalone devices)
slots in the new layout's root port assigned already. So we can add at
most 23 (or 19 for pcie, or 23 for the standalone devices) more kinds of
devices, or? Which might seem a lot at first glance, but in the long run
might be a bit restrictive? Although I suppose some of them will be
standalone and if we really reach that point we can add a switch for new
layout again. Still, might be worth a bit of consideration if we can
increase those limits a bit without too much trouble? If it's not easy
to do so, don't mind me :)

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

---snip 8<---

> diff --git a/pve-qemu-server-pci/src/layout/grouped.rs b/pve-qemu-server-pci/src/layout/grouped.rs
> new file mode 100644
> index 0000000..7ee1458
> --- /dev/null
> +++ b/pve-qemu-server-pci/src/layout/grouped.rs
> @@ -0,0 +1,467 @@
> +use crate::constants::{
> +    MAX_HOSTPCI_DEVICES, MAX_NET_DEVICES, MAX_SCSI_DEVICES, MAX_VIRTIO_BLK_DEVICES,
> +    MAX_VIRTIOFS_DEVICES, SLOTS_PER_BUS,
> +};
> +use crate::layout::bridge_slots;
> +use crate::{
> +    Bus, Device, Function, HardwareConfig, Layout, LayoutError, PciAddress, PveBridge, Slot,
> +};
> +
> +/// The guest's built-in devices, which all live on a bridge of their own so
> +/// that the root bus stays free for the per-kind bridges.
> +///
> +/// NOTE: only append here, don't reorder. The order implies the address, which
> +/// has to stay stable.
> +const BUILTIN_DEVICES: [Slot; SLOTS_PER_BUS] = bridge_slots(&[
> +    single_device!(Ahci),
> +    single_device!(Balloon),
> +    single_device!(XhciController),
> +    single_device!(GuestAgent),
> +    single_device!(SpiceSerial),
> +    single_device!(Rng),
> +    single_device!(Audio),
> +    single_device!(Watchdog),
> +    single_device!(Ivshmem),

Are we sure we never want to have multiple instances for any of these?
Maybe for rng or audio at some point? At least their config keys are
named rng0 and audio0 in qemu-server ;)

---snip 8<---

> diff --git a/pve-qemu-server-pci/src/types/layout.rs b/pve-qemu-server-pci/src/types/layout.rs
> index 00a5a01..c08eacf 100644
> --- a/pve-qemu-server-pci/src/types/layout.rs
> +++ b/pve-qemu-server-pci/src/types/layout.rs
> @@ -1,7 +1,7 @@
>  use std::collections::{HashSet, VecDeque};
>  
> -use crate::constants::{FUNCTIONS_PER_DEVICE, SLOTS_PER_BUS};

Only noticing it now, but maybe MAX_FUNCTIONS_PER_DEVICE?

And the type Device is used for one function, so clashes a bit with the
naming,so MAX_FUNCTIONS_PER_DEVICE_SLOT or similar? Not sure.