From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 941BB1FF0B3 for ; Wed, 11 Nov 2026 10:58:33 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D09C921625; Wed, 11 Nov 2026 10:58:32 +0100 (CET) Message-ID: Date: Wed, 11 Nov 2026 10:58:22 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH pve-qemu-server-rs v3 4/9] pci: layout: add grouped PCI and PCIe layouts To: Fiona Ebner , pve-devel@lists.proxmox.com References: <20261106132604.2556840-1-d.csapak@proxmox.com> <20261106132604.2556840-5-d.csapak@proxmox.com> <3e0667f1-b7c2-4988-8ce8-3c6a462c206d@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <3e0667f1-b7c2-4988-8ce8-3c6a462c206d@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794391102938 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.311 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) 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: WS47CNBCGMCGXSJZ6N2H7CL6SEQV4SPU X-Message-ID-Hash: WS47CNBCGMCGXSJZ6N2H7CL6SEQV4SPU X-MailFrom: d.csapak@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: 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