From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 5DA341FF0BC for ; Thu, 29 Oct 2026 14:38:36 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 2DE8921413; Thu, 29 Oct 2026 14:38:32 +0100 (CET) Content-Type: text/plain; charset=UTF-8 Date: Thu, 29 Oct 2026 14:31:39 +0100 Message-Id: Subject: Re: [PATCH pve-qemu-server-rs v2 4/9] pci: layout: add grouped PCI and PCIe layouts From: "Elias Huhsovitz" To: "Dominik Csapak" , Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.20.0 References: <20261015081041.743422-1-d.csapak@proxmox.com> <20261015081041.743422-5-d.csapak@proxmox.com> In-Reply-To: <20261015081041.743422-5-d.csapak@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793280699514 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.518 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) KAM_MAILER 2 Automated Mailer Tag Left in Email 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: R72YHFFHBCTVBXLKCS73HQTIPZARNL2Z X-Message-ID-Hash: R72YHFFHBCTVBXLKCS73HQTIPZARNL2Z X-MailFrom: e.huhsovitz@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: 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 > --- [snip] > + > +fn layout(pcie: bool) -> &'static Layout<'static> { > + if pcie { &PCIE_LAYOUT } else { &PCI_LAYOUT } > +} > + > +pub(crate) fn address(hw: &HardwareConfig, id: &str) -> Result { > + let layout =3D layout(hw.has_pcie()); > + layout.find_device(&id.parse()?) > +} > + > +pub(crate) fn pci_bridge_for_device(hw: &HardwareConfig, id: &str) -> Op= tion { > + let addr =3D address(hw, id).ok()?; > + Some(addr.bus) > +} > + > +pub(crate) fn pci_bus_devices(hw: &HardwareConfig) -> Result= , LayoutError> { > + let mut bridges =3D Vec::new(); > + let layout =3D layout(hw.has_pcie()); > + let mut chassis_nr =3D 1; > + for bridge in layout > + .iter() > + .filter(|d| matches!(d.device, Device::Bridge(_))) > + { > + let addr =3D bridge.addr; > + let (model, id) =3D match bridge.device { > + Device::Bridge(bus) =3D> (bus.bridge_model(addr.bus), bus), > + _ =3D> continue, > + }; > + let Some(model) =3D model else { > + continue; > + }; > + let mf =3D if addr.function =3D=3D 0 { > + ",multifunction=3Don" Isn't the addr.function =3D=3D 0 already encoded in AddressAssignment? So we can just check for: let mf =3D bridge.multifunction { ",multifunction=3Don" > + } else { > + "" > + }; > + > + let chassis =3D if model =3D=3D "pci-bridge" { > + let text =3D format!(",chassis_nr=3D{chassis_nr}"); > + chassis_nr +=3D 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=3D{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) > +} > + =20 [snip] =20 > 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 { > =20 > pub(crate) mod legacy; > =20 > +pub mod grouped; > + > /// Constructs a correctly sized list of bridge slots, starting at addre= ss 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; > =20 > pub mod layout; > =20 > +/// 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 =3D LayoutError; > + > + fn from_str(s: &str) -> Result { > + match s { > + "legacy" =3D> Ok(Self::Legacy), > + "grouped" =3D> Ok(Self::Grouped), > + _ =3D> 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 =3D> "legacy", > + PciLayout::Grouped =3D> "grouped", > + }) > + } > +} > + > /// Returns the number of the PCI bridge `id` sits on, or `None` if it i= s on > /// the root bus or not a known device. > -pub fn pci_bridge_for_device(id: &str) -> Option { > - layout::legacy::pci_bridge_for_device(id) > +pub fn pci_bridge_for_device(hw: &HardwareConfig, id: &str) -> Option { > + match hw.layout { > + PciLayout::Legacy =3D> layout::legacy::pci_bridge_for_device(id)= , > + PciLayout::Grouped =3D> layout::grouped::pci_bridge_for_device(h= w, id), > + } > } nit naming: The function is called pci_bridge_for_device() but returns a Bu= s. 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. > =20 > /// 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= { > /// 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 bus= es. > pub fn pci_bus_devices(hw: &HardwareConfig) -> Result, Layou= tError> { 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 =3D> layout::legacy::pci_bus_devices(hw), > + PciLayout::Grouped =3D> layout::grouped::pci_bus_devices(hw), > + } > } > =20 > /// Returns the address of the device with the configuration id `id`. > pub fn address(hw: &HardwareConfig, id: &str) -> Result { > - layout::legacy::address(hw, id) > + match hw.layout { > + PciLayout::Legacy =3D> layout::legacy::address(hw, id), > + PciLayout::Grouped =3D> layout::grouped::address(hw, id), > + } > } > diff --git a/pve-qemu-server-pci/src/types/mod.rs b/pve-qemu-server-pci/s= rc/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}; > =20 > mod device; > pub use device::Device; > +// only needed to enumerate all device kinds in tests > +#[cfg(test)] > +pub(crate) use device::DeviceDiscriminants; > =20 > mod pci_address; > pub use pci_address::PciAddress;