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

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

On 11/10/26 6:08 PM, Fiona Ebner wrote:
> 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 :)


actually we could add up to 8 devices per slot, the only requirement is
that slot 0 is there. In the worst case we can expand by adding bridges

though i won't guess there won't be many slots necessary for upcoming 
device types, more like single devices and these can be put behind
new bridges too if we want/need.

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

i thought about that shortly, but honestly most of them don't make sense
to have multiple of or the use case is very niche in the first place
(ivshmem for example).

for usb controllers we can solve it for a long time with usb hubs 
instead (up to i think ~200 devices) so that should not be a problem

If anybody has a very niche use case for multiple of those one
can always use 'args' as an escape hatch.

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

yeah makes sense