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 2D2021FF09B for ; Mon, 28 Sep 2026 16:06:03 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 48BAB2170E; Mon, 28 Sep 2026 16:06:01 +0200 (CEST) Message-ID: <1003d008-f949-4fbf-a8de-6df87376329c@proxmox.com> Date: Mon, 28 Sep 2026 16:05:56 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH pve-qemu-server-rs 3/9] pci: layout: add v2 PCI and PCIe layouts To: Elias Huhsovitz , pve-devel@lists.proxmox.com References: <20260922105550.2084078-1-d.csapak@proxmox.com> <20260922105550.2084078-4-d.csapak@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: 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: 1790604357020 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.427 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: MKVP5AWDGJLHMDUYZQTVQ74QIAKBV3BG X-Message-ID-Hash: MKVP5AWDGJLHMDUYZQTVQ74QIAKBV3BG 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 9/28/26 2:55 PM, Elias Huhsovitz wrote: > Since the `v2` name is just a development placeholder, I will start with > some suggestions: > > * `grouped` - describing the property of the new layout. > > * `per_device` | `per_kind` - since each device gets its own bridges. > > * `default` - since it is the new default (might age badly if a new > default is introduced). > > On Tue Sep 22, 2026 at 12:55 PM 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. >> >> These are not wired up to the entry points yet: they are meant for new >> guests only, and the machine property selecting them still has to be >> added on the qemu-server side. >> >> Signed-off-by: Dominik Csapak >> --- >> pve-qemu-server-pci/src/layout/mod.rs | 2 + >> pve-qemu-server-pci/src/layout/v2.rs | 344 ++++++++++++++++++++++++++ >> pve-qemu-server-pci/src/types/mod.rs | 3 + >> 3 files changed, 349 insertions(+) >> create mode 100644 pve-qemu-server-pci/src/layout/v2.rs >> >> diff --git a/pve-qemu-server-pci/src/layout/mod.rs b/pve-qemu-server-pci/src/layout/mod.rs >> index c81d730..0b72abd 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 v2; >> + >> /// 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/layout/v2.rs b/pve-qemu-server-pci/src/layout/v2.rs >> new file mode 100644 >> index 0000000..9525492 >> --- /dev/null >> +++ b/pve-qemu-server-pci/src/layout/v2.rs >> @@ -0,0 +1,344 @@ >> +use crate::constants::{ >> + BRIDGE_SLOT_NUM, MAX_HOSTPCI_DEVICES, MAX_NET_DEVICES, MAX_SCSI_DEVICES, >> + MAX_VIRTIO_BLK_DEVICES, MAX_VIRTIOFS_DEVICES, >> +}; >> +use crate::layout::bridge_slots; >> +use crate::{Bus, Device, DeviceLayout, Function, 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 SYS_BRIDGE_SLOTS: [Slot; BRIDGE_SLOT_NUM] = bridge_slots(&[ > > nit: name: I would prefer `BUILTIN_BRIDGE_SLOTS` or > `BUILTIN_DEVICE_SLOTS`. Since devies are usually in slots, we can also > just shorten it to `BUILTIN_DEVICES`. > >> + single_device!(Ahci), >> + single_device!(Balloon), >> + single_device!(XhciController(0)), >> + single_device!(GuestAgent), >> + single_device!(SpiceSerial), >> + single_device!(Rng), >> + single_device!(Audio), >> + single_device!(Watchdog), >> + single_device!(Ivshmem), >> +]); >> + >> +/// The layout for guests with a PCI root bus. >> +pub static PCI_LAYOUT: DeviceLayout = DeviceLayout { >> + pcie: false, >> + root: bridge_slots(&[ >> + Slot::Reserved, >> + Slot::Multi(&[ >> + Function::Device(Device::Vga(0)), >> + Function::Device(Device::Vga(1)), >> + Function::Device(Device::Vga(2)), >> + Function::Device(Device::Vga(3)), >> + Function::Unused, >> + Function::Unused, >> + Function::Unused, >> + Function::Unused, >> + ]), >> + Slot::Single(Function::Device(Device::Viommu)), >> + Slot::Single(Function::FixedBridge(Bus::Sys(0), &SYS_BRIDGE_SLOTS)), >> + Slot::DynamicBridge { >> + kind: PveBridge::VirtioFs, >> + max: MAX_VIRTIOFS_DEVICES, >> + }, >> + Slot::DynamicBridge { >> + kind: PveBridge::Net, >> + max: MAX_NET_DEVICES, >> + }, >> + Slot::Unused, >> + Slot::DynamicBridge { >> + kind: PveBridge::Scsi, >> + max: MAX_SCSI_DEVICES, >> + }, >> + Slot::Unused, >> + Slot::DynamicBridge { >> + kind: PveBridge::VirtioBlk, >> + max: MAX_VIRTIO_BLK_DEVICES, >> + }, >> + Slot::Unused, >> + Slot::Unused, >> + Slot::Unused, >> + Slot::DynamicBridge { >> + kind: PveBridge::Hostpci, >> + max: MAX_HOSTPCI_DEVICES, >> + }, >> + ]), >> +}; >> + >> +/// The layout for guests with a PCIe root bus. >> +/// >> +/// Identical to [`PCI_LAYOUT`] except for the root bus type and the root ports >> +/// for passed through PCIe devices, which a PCI machine cannot have. >> +pub static PCIE_LAYOUT: DeviceLayout = DeviceLayout { >> + pcie: true, >> + root: bridge_slots(&[ >> + Slot::Reserved, >> + Slot::Multi(&[ >> + Function::Device(Device::Vga(0)), >> + Function::Device(Device::Vga(1)), >> + Function::Device(Device::Vga(2)), >> + Function::Device(Device::Vga(3)), >> + Function::Unused, >> + Function::Unused, >> + Function::Unused, >> + Function::Unused, >> + ]), >> + Slot::Single(Function::Device(Device::Viommu)), >> + Slot::Single(Function::FixedBridge(Bus::Sys(0), &SYS_BRIDGE_SLOTS)), >> + Slot::DynamicBridge { >> + kind: PveBridge::VirtioFs, >> + max: MAX_VIRTIOFS_DEVICES, >> + }, >> + Slot::DynamicBridge { >> + kind: PveBridge::Net, >> + max: MAX_NET_DEVICES, >> + }, >> + Slot::Unused, >> + Slot::DynamicBridge { >> + kind: PveBridge::Scsi, >> + max: MAX_SCSI_DEVICES, >> + }, >> + Slot::Unused, >> + Slot::DynamicBridge { >> + kind: PveBridge::VirtioBlk, >> + max: MAX_VIRTIO_BLK_DEVICES, >> + }, >> + Slot::Unused, >> + Slot::Unused, >> + Slot::Unused, >> + Slot::DynamicBridge { >> + kind: PveBridge::Hostpci, >> + max: MAX_HOSTPCI_DEVICES, >> + }, >> + Slot::Unused, >> + Slot::Multi(&[ >> + Function::RootPort(0, Device::Hostpcie(0)), >> + Function::RootPort(1, Device::Hostpcie(1)), >> + Function::RootPort(2, Device::Hostpcie(2)), >> + Function::RootPort(3, Device::Hostpcie(3)), >> + Function::RootPort(4, Device::Hostpcie(4)), >> + Function::RootPort(5, Device::Hostpcie(5)), >> + Function::RootPort(6, Device::Hostpcie(6)), >> + Function::RootPort(7, Device::Hostpcie(7)), >> + ]), >> + Slot::Multi(&[ >> + Function::RootPort(8, Device::Hostpcie(8)), >> + Function::RootPort(9, Device::Hostpcie(9)), >> + Function::RootPort(10, Device::Hostpcie(10)), >> + Function::RootPort(11, Device::Hostpcie(11)), >> + Function::RootPort(12, Device::Hostpcie(12)), >> + Function::RootPort(13, Device::Hostpcie(13)), >> + Function::RootPort(14, Device::Hostpcie(14)), >> + Function::RootPort(15, Device::Hostpcie(15)), >> + ]), > > We have 16 device slots via the dynmic bridge and 16 devices slots via > root ports = 32 passthrough slots. But the config schema only allows > for 16 total `hostpciN` devices. > > IMO we need to add `hostpcieN` as a config key in perl and add a new > constant. > > pub const MAX_HOSTPCIE_DEVICES: u8 = 16; > > OR what about removing the explicit `Device::Hostpcie` and folding in the > placement into the enum, like this: > > pub enum Device { > Hostpci(u8, HostpciPlacement), > } > > pub enum HostpciPlacement { > Bridge, > RootPort, > } > > Since, if I understand this correctly, we primarily care about the placement. i tried that, but that didn't quite workout with the FromStr implementations. The issue here is that a hostpciX device can be either placed in a pci or pcie slot, depending on its own config. but see my reply to patch 1 > >> + ]), >> +}; > > [snip]