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 DAFF91FF0B4 for ; Tue, 08 Sep 2026 11:59:32 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id EF2C521582; Tue, 08 Sep 2026 11:59:29 +0200 (CEST) Message-ID: Date: Tue, 8 Sep 2026 11:59:22 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server v2 3/4] pci: don't try to hotplug bridges To: Dominik Csapak , pve-devel@lists.proxmox.com References: <20260907124335.2822329-1-d.csapak@proxmox.com> <20260907124335.2822329-4-d.csapak@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260907124335.2822329-4-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: 1788861559338 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.716 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: RRHL3RI4YBQOXOJTQAVRCCIIN7YLS5FA X-Message-ID-Hash: RRHL3RI4YBQOXOJTQAVRCCIIN7YLS5FA 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:43 PM schrieb Dominik Csapak: > While bridges can be hotplugged (on PCI on i440fx), no device can be > hotplugged in that afterwards. For that to work the SHPC option would > have to be enabled and the guest must support that. Since this is not > guaranteed to work and bridges don't show up in our config, this could > lead to bridges added that are not represented in the config. > > To be on the safe side, simply don't allow hotplugging bridges at all. > Luckily, the only bridge we ever tried hotplugging (since machine > version 2.3) was pci.4 which only houses scsihw2/3/4 at the moment. > > These are only used for scsiX where X > 13 and only if the scsihw is an > LSI controller, so not very likely to occur. > > This fixes an issue where trying to hotplug a scsi disk with index >=14 > on a i440fx machine with an LSI scsi controller would leave the bridge > around after failing to add the scsi controller, and the machine would > subsequently crash on live migration. > > Fixes: 2513b862 (fix #2566: increase scsi limit to 31) > Signed-off-by: Dominik Csapak > --- > src/PVE/QemuServer.pm | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > index 149f17be..ab13bc41 100644 > --- a/src/PVE/QemuServer.pm > +++ b/src/PVE/QemuServer.pm > @@ -3992,13 +3992,6 @@ sub vm_deviceplug { > warn $@ if $@; > die $err; > } > - } elsif (!$q35 && $deviceid =~ m/^(pci\.)(\d+)$/) { > - my $bridgeid = $2; > - my $pciaddr = print_pci_addr($deviceid, undef, $arch); > - my $devicefull = "pci-bridge,id=pci.$bridgeid,chassis_nr=$bridgeid$pciaddr"; > - > - qemu_deviceadd($vmid, $devicefull); > - qemu_deviceaddverify($vmid, $deviceid); > } else { > die "can't hotplug device '$deviceid'\n"; > } The single caller that passes pci.N to vm_deviceplug() is qemu_add_pci_bridge(). That in turn has a single caller at the beginning of vm_deviceplug(). But after removing the actual hotplug, the code reads confusingly, since it still suggests that a bridge will be added. I think we should either remove qemu_add_pci_bridge() altogether or turn it into an assert_pci_bridge_present(), replacing its vm_deviceplug() call with a die. The latter is nicer for getting clearer errors.