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 F3D721FF0B4 for ; Tue, 08 Sep 2026 11:59:46 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 2F3CE21432; Tue, 08 Sep 2026 11:59:40 +0200 (CEST) Message-ID: <2675ebfa-393b-43fc-bad2-fa15987bb775@proxmox.com> Date: Tue, 8 Sep 2026 11:59:32 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server v2 4/4] pci: bridges: add bridges to devices up front To: Dominik Csapak , pve-devel@lists.proxmox.com References: <20260907124335.2822329-1-d.csapak@proxmox.com> <20260907124335.2822329-5-d.csapak@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260907124335.2822329-5-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788861565457 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.713 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: WN3UKWY3KGQ3DRYHNAPI5R4TZCIQV4LT X-Message-ID-Hash: WN3UKWY3KGQ3DRYHNAPI5R4TZCIQV4LT X-MailFrom: f.ebner@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: Am 07.09.26 um 2:44 PM schrieb Dominik Csapak: > Since qemu machine version 2.3, we add the first two bridges by default > (3 in q35), so wiring the bridge adding logic through print_pci_addr was > only necessary in a few (easily checkable) circumstances. > > So to not have to pass and collect the bridge devices throughout our > code, instead check those conditions directly and add them upfront. > (This also eliminates the need to insert the bridge devices at specific > point in the commandline via splice/unshift) > > The only functional change here should be that we now reject vms with > machine version < 2.3 outright, but QEMU would do so later anyway. > > 'print_pcie_root_port' was removed from the imports in QemuServer.pm, > since it wasn't used there anymore anyway. > > Signed-off-by: Dominik Csapak Nice cleanup! > --- > src/PVE/QemuServer.pm | 110 +++++++++--------------------- > src/PVE/QemuServer/Drive.pm | 2 +- > src/PVE/QemuServer/DriveDevice.pm | 23 ++++++- > src/PVE/QemuServer/PCI.pm | 71 +++++++++++++++++-- > src/PVE/QemuServer/RNG.pm | 4 +- > src/PVE/QemuServer/USB.pm | 8 +-- > 6 files changed, 128 insertions(+), 90 deletions(-) > > diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > index ab13bc41..ca77ca74 100644 > --- a/src/PVE/QemuServer.pm > +++ b/src/PVE/QemuServer.pm > @@ -86,7 +86,7 @@ use PVE::QemuServer::MetaInfo; > use PVE::QemuServer::Monitor qw(mon_cmd qmp_cmd vm_qmp_peer); > use PVE::QemuServer::Network; > use PVE::QemuServer::OVMF; > -use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr print_pcie_root_port parse_hostpci); > +use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr parse_hostpci get_bridges); You can also drop the export of print_pcie_root_port from the PCI module and make the sub private. ---snip 8<--- > diff --git a/src/PVE/QemuServer/DriveDevice.pm b/src/PVE/QemuServer/DriveDevice.pm > index 37b611f0..bff43dc4 100644 > --- a/src/PVE/QemuServer/DriveDevice.pm > +++ b/src/PVE/QemuServer/DriveDevice.pm > @@ -27,6 +27,25 @@ our @EXPORT_OK = qw( > scsihw_infos > ); > > +# Gets the maximum scsihw index that will be used > +sub get_max_scsihw_index { Style nit: could live below scsihw_infos(), since it calls that. > + my ($conf) = @_; > + > + my $max_index = 0; > + > + for (my $i = 0; $i < $PVE::QemuServer::Drive::MAX_SCSI_DISKS; $i++) { > + next if !defined($conf->{"scsi$i"}); > + my (undef, $index, $prefix) = scsihw_infos($conf->{scsihw}, $i); > + last if $prefix ne 'scsihw'; # must be the same for all > + > + if ($index > $max_index) { > + $max_index = $index; > + } > + } > + > + return $max_index; > +} > + > sub scsihw_infos { > my ($scsihw, $drive_index) = @_; > ---snip 8<--- > @@ -292,8 +293,19 @@ my $get_addr_mapping_from_id = sub { > return { bus => $d->{bus}, addr => sprintf("0x%x", $d->{addr}) }; > }; > > +sub get_bridge_for_device { > + my ($id) = @_; > + > + my $map = get_pci_addr_map(); > + if (my $d = $get_addr_mapping_from_id->($map, $id)) { > + return $d->{bus}; > + } > + > + return; > +} Adding this and the change in qemu_add_pci_device() could be it's own patch. > + > sub print_pci_addr { > - my ($id, $bridges, $arch) = @_; > + my ($id, $arch) = @_; > > die "aarch64 cannot use IDE devices\n" if $arch eq 'aarch64' && $id =~ /^ide/; > > @@ -306,7 +318,6 @@ sub print_pci_addr { > my $busname = $arch eq 'aarch64' && $d->{bus} eq 0 ? 'pcie' : 'pci'; > > $res = ",bus=$busname.$d->{bus},addr=$d->{addr}"; > - $bridges->{ $d->{bus} } = 1 if $bridges; > } > > return $res; > @@ -617,7 +628,7 @@ sub choose_hostpci_devices { > } > > sub print_hostpci_devices { > - my ($vmid, $conf, $devices, $vga, $winversion, $bridges, $arch, $bootorder, $dry_run) = @_; > + my ($vmid, $conf, $devices, $vga, $winversion, $arch, $bootorder, $dry_run) = @_; > > my $kvm_off = 0; > my $gpu_passthrough = 0; > @@ -648,7 +659,7 @@ sub print_hostpci_devices { > } > } else { > my $pci_name = $d->{'legacy-igd'} ? 'legacy-igd' : $id; > - $pciaddr = print_pci_addr($pci_name, $bridges, $arch); > + $pciaddr = print_pci_addr($pci_name, $arch); > } > > my $num_devices = scalar($d->{ids}->@*); > @@ -871,4 +882,56 @@ sub reserve_pci_usage { > die $@ if $@; > } > > +# Returns a list of bridge devices which are necessary for the remaining > +# devices. > +sub get_bridges { I'd suggest get_pci_bridges, since "bridge" is also used for network bridges. Or alternatively not export the function, so callers have to specify the module as a prefix. > + my ($conf, $arch, $q35, $max_scsihw, $version_guard) = @_; > + > + # older machine versions didn't add the bridges by default. They're > + # not supported by modern qemu anymore, so don't try to start them. > + if (!$version_guard->(2, 3)) { > + die "unsupported old machine version detected\n"; > + } I'd put this on top of config_to_command() and explicitly mention what the configured machine version is in the error. Since commit c6b32373 ("pbs-restore: set 'no-cache' on block devices backed by zfspool"), we have a dependency on QEMU 11.0, meaning the check can already reject everything strictly before machine version 5.0. Should be its own patch up front. > + > + my $bridges = { > + # 0 => 1, always present > + 1 => 1, > + 2 => 1, > + }; > + > + $bridges->{3} = 1 if ($conf->{scsihw} // '') =~ m/^virtio-scsi-single/; > + > + # some scsi controllers can only have 7 scsi disks per controller, > + # so scsi14 and upwards need scsihw2,3,4 which live on bridge 4 > + $bridges->{4} = 1 if $max_scsihw > 1; > + > + # use cheap legacy igd check instead full parse_hostpci > + my $legacy_igd = 0; > + for (my $i = 0; $i < $MAX_HOSTPCI_DEVICES; $i++) { > + next if !defined($conf->{"hostpci$i"}); > + my $res = PVE::JSONSchema::parse_property_string($hostpci_fmt, $conf->{"hostpci$i"}); > + next if !defined($res); > + if ($res->{'legacy-igd'}) { Style nit: could be $res && $res->{'legacy-igd'} to get rid of the line with 'next' > + $legacy_igd = 1; > + last; > + } > + } > + > + my $devices = []; > + for my $k (sort { $a <=> $b } keys %$bridges) { > + next if $q35 && $k < 4; # q35.cfg already includes bridges up to 3 > + > + my $k_name = $k; > + if ($k == 2 && $legacy_igd) { > + $k_name = "$k-igd"; > + } > + my $pciaddr = print_pci_addr("pci.$k_name", $arch); > + my $devstr = "pci-bridge,id=pci.$k,chassis_nr=$k$pciaddr"; > + > + push @$devices, '-device', $devstr; > + } > + > + return $devices; > +} > + > 1;