[PATCH qemu-server v2 0/4] pci: bridges: cleanup and hotplug fix

[PATCH qemu-server v2 0/4] pci: bridges: cleanup and hotplug fix

[Dominik Csapak]

Main motivation for this series was a defect in how we handle
hotplugging certain devices:

When e.g. adding scsi15 on a vm with i440fx + lsi controller, we'd need
scsihw2 (since only 7 devices can live on scsihw0 and scsihw1
respectively) which sits on bus pci.4. This itself is not added by
default to the config, so we tried to hotplug the bridge. This works,
but without SHPC (which is neither the default, nor does it work
reliably in all guests) one cannot hot-plug anything into a hot-plugged
bridge. So when hotplugging scsi15 in this example, we'd hot-plug
pci.4, but adding scsihw2 would fail and leave the disk in 'pending'.

When now a live-migration is initiated, the already added bridge is
never recorded in the config, and the guest crashes on the remote
after it's resumed.

Since this is a rather narrow case, I'd opted for preventing
hot-plugging bridges completely.

Further I tried to refactor and  cleanup the bridge adding code a bit,
so we don't have to pass the 'bridges' hash around anymore. (patch 4/4)

To be sure, I added two tests for cases we didn't cover yet. (patches 1
and 2, these could be applied anyway since they just test the status
quo).

Further improvement could be to always add pci.4 with the next machine
version, but this can be done as a follow up or separate series later
too.

changes from v1:
* updated a few callsites of methods which removed the bridges parameter
  which i originally forgot (used 'undef' as that parameter)
* added a fixes trailer for the hotplug commit
* use hostpci_fmt schema to check legacy-igd. parsing twice is not
  optimal, but it's only 16 lines and we can improve upon that later too

Dominik Csapak (4):
  tests: add test with q35 and scsihw2
  tests: add test for legacy-igd passthrough
  pci: don't try to hotplug bridges
  pci: bridges: add bridges to devices up front

 src/PVE/QemuServer.pm                         | 117 +++++-------------
 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 +-
 .../cfg2cmd/linux-hostpci-legacy-igd.conf     |  16 +++
 .../cfg2cmd/linux-hostpci-legacy-igd.conf.cmd |  38 ++++++
 src/test/cfg2cmd/q35-scsihw2.conf             |  13 ++
 src/test/cfg2cmd/q35-scsihw2.conf.cmd         |  37 ++++++
 src/test/run_config2command_tests.pl          |   1 +
 11 files changed, 233 insertions(+), 97 deletions(-)
 create mode 100644 src/test/cfg2cmd/linux-hostpci-legacy-igd.conf
 create mode 100644 src/test/cfg2cmd/linux-hostpci-legacy-igd.conf.cmd
 create mode 100644 src/test/cfg2cmd/q35-scsihw2.conf
 create mode 100644 src/test/cfg2cmd/q35-scsihw2.conf.cmd

-- 
2.47.3

[PATCH qemu-server v2 1/4] tests: add test with q35 and scsihw2

[Dominik Csapak]

this needs the pci.4 bridge, which was not tested at all yet.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/test/cfg2cmd/q35-scsihw2.conf     | 13 ++++++++++
 src/test/cfg2cmd/q35-scsihw2.conf.cmd | 37 +++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)
 create mode 100644 src/test/cfg2cmd/q35-scsihw2.conf
 create mode 100644 src/test/cfg2cmd/q35-scsihw2.conf.cmd

diff --git a/src/test/cfg2cmd/q35-scsihw2.conf b/src/test/cfg2cmd/q35-scsihw2.conf
new file mode 100644
index 00000000..e0452cca
--- /dev/null
+++ b/src/test/cfg2cmd/q35-scsihw2.conf
@@ -0,0 +1,13 @@
+# TEST: Config with q35, Linux & scsi15 with default controller to test scsihw2
+#
+bios: ovmf
+bootdisk: scsi0
+cores: 2
+efidisk0: local:100/vm-100-disk-1.qcow2,size=128K
+machine: q35
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi15: local:100/vm-100-disk-2.qcow2,size=1G
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/src/test/cfg2cmd/q35-scsihw2.conf.cmd b/src/test/cfg2cmd/q35-scsihw2.conf.cmd
new file mode 100644
index 00000000..169539a1
--- /dev/null
+++ b/src/test/cfg2cmd/q35-scsihw2.conf.cmd
@@ -0,0 +1,37 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name vm8006 \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect-ms=5000' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
+  -object '{"id":"throttle-drive-efidisk0","limits":{},"qom-type":"throttle-group"}' \
+  -blockdev '{"driver":"raw","file":{"driver":"file","filename":"/usr/share/pve-edk2-firmware//OVMF_CODE.fd"},"node-name":"pflash0","read-only":true}' \
+  -blockdev '{"detect-zeroes":"on","discard":"ignore","driver":"throttle","file":{"cache":{"direct":false,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"qcow2","file":{"aio":"io_uring","cache":{"direct":false,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"file","filename":"/var/lib/vz/images/100/vm-100-disk-1.qcow2","node-name":"e70e3017c5a79fdee5a04aa92ac1e9c","read-only":false},"node-name":"f70e3017c5a79fdee5a04aa92ac1e9c","read-only":false},"node-name":"drive-efidisk0","read-only":false,"throttle-group":"throttle-drive-efidisk0"}' \
+  -global 'ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off' \
+  -smp '2,sockets=1,cores=2,maxcpus=2' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -object '{"id":"throttle-drive-scsi15","limits":{},"qom-type":"throttle-group"}' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1' \
+  -readconfig /usr/share/qemu-server/pve-q35-4.0.cfg \
+  -device 'pci-bridge,id=pci.4,chassis_nr=4,bus=pci.1,addr=0x1c' \
+  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
+  -device 'usb-tablet,id=tablet,bus=ehci.0,port=1' \
+  -device 'VGA,id=vga,bus=pcie.0,addr=0x1' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -device 'lsi,id=scsihw2,bus=pci.4,addr=0x1' \
+  -blockdev '{"detect-zeroes":"on","discard":"ignore","driver":"throttle","file":{"cache":{"direct":true,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"qcow2","file":{"aio":"io_uring","cache":{"direct":true,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"file","filename":"/var/lib/vz/images/100/vm-100-disk-2.qcow2","node-name":"ee7a5fa0313fa9ceafb6db055fb78a4","read-only":false},"node-name":"fe7a5fa0313fa9ceafb6db055fb78a4","read-only":false},"node-name":"drive-scsi15","read-only":false,"throttle-group":"throttle-drive-scsi15"}' \
+  -device 'scsi-hd,bus=scsihw2.0,scsi-id=1,drive=drive-scsi15,id=scsi15,device_id=drive-scsi15,write-cache=on' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/usr/libexec/qemu-server/pve-bridge,downscript=/usr/libexec/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,rx_queue_size=1024,tx_queue_size=256,bootindex=300,host_mtu=1500,host_tunnel=off' \
+  -machine 'pflash0=pflash0,pflash1=drive-efidisk0,hpet=off,type=q35+pve0'
-- 
2.47.3

[PATCH qemu-server v2 2/4] tests: add test for legacy-igd passthrough

[Dominik Csapak]

this has some implications for pci address calculation (e.g. for pci.2)
so having a test for it will avoid some problems.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 .../cfg2cmd/linux-hostpci-legacy-igd.conf     | 16 ++++++++
 .../cfg2cmd/linux-hostpci-legacy-igd.conf.cmd | 38 +++++++++++++++++++
 src/test/run_config2command_tests.pl          |  1 +
 3 files changed, 55 insertions(+)
 create mode 100644 src/test/cfg2cmd/linux-hostpci-legacy-igd.conf
 create mode 100644 src/test/cfg2cmd/linux-hostpci-legacy-igd.conf.cmd

diff --git a/src/test/cfg2cmd/linux-hostpci-legacy-igd.conf b/src/test/cfg2cmd/linux-hostpci-legacy-igd.conf
new file mode 100644
index 00000000..37f3cfee
--- /dev/null
+++ b/src/test/cfg2cmd/linux-hostpci-legacy-igd.conf
@@ -0,0 +1,16 @@
+# TEST: Config with legacy-igd
+bios: ovmf
+bootdisk: scsi0
+cores: 1
+efidisk0: local:100/vm-100-disk-1.qcow2,size=128K
+hostpci0: 00:02.0,legacy-igd=on
+machine: pc
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+numa: 1
+ostype: l26
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+sockets: 2
+vga: none
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/src/test/cfg2cmd/linux-hostpci-legacy-igd.conf.cmd b/src/test/cfg2cmd/linux-hostpci-legacy-igd.conf.cmd
new file mode 100644
index 00000000..21dd22ef
--- /dev/null
+++ b/src/test/cfg2cmd/linux-hostpci-legacy-igd.conf.cmd
@@ -0,0 +1,38 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name vm8006 \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect-ms=5000' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
+  -object '{"id":"throttle-drive-efidisk0","limits":{},"qom-type":"throttle-group"}' \
+  -blockdev '{"driver":"raw","file":{"driver":"file","filename":"/usr/share/pve-edk2-firmware//OVMF_CODE.fd"},"node-name":"pflash0","read-only":true}' \
+  -blockdev '{"detect-zeroes":"on","discard":"ignore","driver":"throttle","file":{"cache":{"direct":false,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"qcow2","file":{"aio":"io_uring","cache":{"direct":false,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"file","filename":"/var/lib/vz/images/100/vm-100-disk-1.qcow2","node-name":"e70e3017c5a79fdee5a04aa92ac1e9c","read-only":false},"node-name":"f70e3017c5a79fdee5a04aa92ac1e9c","read-only":false},"node-name":"drive-efidisk0","read-only":false,"throttle-group":"throttle-drive-efidisk0"}' \
+  -smp '2,sockets=2,cores=1,maxcpus=2' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vga none \
+  -nographic \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -object 'memory-backend-ram,id=ram-node0,size=256M' \
+  -numa 'node,nodeid=0,cpus=0,memdev=ram-node0' \
+  -object 'memory-backend-ram,id=ram-node1,size=256M' \
+  -numa 'node,nodeid=1,cpus=1,memdev=ram-node1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1' \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.1,addr=0x1e' \
+  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'vfio-pci,host=0000:00:02.0,id=hostpci0,bus=pci.0,addr=0x2' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/usr/libexec/qemu-server/pve-bridge,downscript=/usr/libexec/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,rx_queue_size=1024,tx_queue_size=256,bootindex=300,host_mtu=1500,host_tunnel=off' \
+  -machine 'pflash0=pflash0,pflash1=drive-efidisk0,hpet=off,type=pc+pve0'
diff --git a/src/test/run_config2command_tests.pl b/src/test/run_config2command_tests.pl
index 47250c67..28f538d3 100755
--- a/src/test/run_config2command_tests.pl
+++ b/src/test/run_config2command_tests.pl
@@ -128,6 +128,7 @@ my $base_env = {
 };
 
 my $pci_devs = [
+    "0000:00:02.0",
     "0000:00:43.1",
     "0000:00:f4.0",
     "0000:00:ff.1",
-- 
2.47.3

[PATCH qemu-server v2 3/4] pci: don't try to hotplug bridges

[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 <d.csapak@proxmox.com>
---
 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";
     }
-- 
2.47.3

[PATCH qemu-server v2 4/4] pci: bridges: add bridges to devices up front

[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 <d.csapak@proxmox.com>
---
 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);
 use PVE::QemuServer::PCI::Mdev;
 use PVE::QemuServer::QemuImage;
 use PVE::QemuServer::QMPHelpers
@@ -1325,7 +1325,6 @@ sub print_netdevice_full {
         $conf,
         $net,
         $netid,
-        $bridges,
         $use_old_bios_files,
         $arch,
         $machine_version,
@@ -1337,7 +1336,7 @@ sub print_netdevice_full {
         $device = 'virtio-net-pci';
     }
 
-    my $pciaddr = print_pci_addr("$netid", $bridges, $arch);
+    my $pciaddr = print_pci_addr("$netid", $arch);
     my $tmpstr = "$device,mac=$net->{macaddr},netdev=$netid$pciaddr,id=$netid";
     if ($net->{queues} && $net->{queues} > 1 && $net->{model} eq 'virtio') {
         # Consider we have N queues, the number of vectors needed is 2 * N + 2, i.e., one per in
@@ -1497,7 +1496,7 @@ my sub map_vga_model {
 }
 
 sub print_vga_device {
-    my ($conf, $vga, $arch, $machine_version, $id, $qxlnum, $bridges) = @_;
+    my ($conf, $vga, $arch, $machine_version, $id, $qxlnum) = @_;
 
     my $type = map_vga_model($vga->{type}, $arch);
     my $vgamem_mb = $vga->{memory};
@@ -1546,7 +1545,7 @@ sub print_vga_device {
         # the first display uses pcie.0 bus on q35 machines
         $pciaddr = print_pcie_addr($vgaid);
     } else {
-        $pciaddr = print_pci_addr($vgaid, $bridges, $arch);
+        $pciaddr = print_pci_addr($vgaid, $arch);
     }
 
     if ($vga->{type} eq 'virtio-gl') {
@@ -3137,7 +3136,6 @@ sub config_to_command {
 
     my ($machineFlags, $rtcFlags) = ([], []);
     my $devices = [];
-    my $bridges = {};
     my $ostype = $conf->{ostype};
     my $winversion = windows_version($ostype);
     my $kvm = $conf->{kvm};
@@ -3285,13 +3283,17 @@ sub config_to_command {
         push @$cmd, $fixups->@*;
     }
 
+    my $max_scsihw = PVE::QemuServer::DriveDevice::get_max_scsihw_index($conf);
+    if (my $bridges = get_bridges($conf, $arch, $q35, $max_scsihw, $version_guard)) {
+        push @$devices, $bridges->@*;
+    }
+
     if ($conf->{vmgenid}) {
         push @$devices, '-device', 'vmgenid,guid=' . $conf->{vmgenid};
     }
 
     # add usb controllers
-    my @usbcontrollers =
-        PVE::QemuServer::USB::get_usb_controllers($conf, $bridges, $arch, $machine_version);
+    my @usbcontrollers = PVE::QemuServer::USB::get_usb_controllers($conf, $arch, $machine_version);
     push @$devices, @usbcontrollers if @usbcontrollers;
 
     my ($vga, $qxlnum) = get_vga_properties($conf, $arch, $machine_version, $winversion);
@@ -3315,7 +3317,7 @@ sub config_to_command {
     # host pci device passthrough
     my ($kvm_off, $gpu_passthrough, $legacy_igd, $pci_devices) =
         PVE::QemuServer::PCI::print_hostpci_devices(
-            $vmid, $conf, $devices, $vga, $winversion, $bridges, $arch, $bootorder, $dry_run,
+            $vmid, $conf, $devices, $vga, $winversion, $arch, $bootorder, $dry_run,
         );
 
     # usb devices
@@ -3359,7 +3361,7 @@ sub config_to_command {
     }
 
     if (min_version($machine_version, 4, 0) && (my $audio = conf_has_audio($conf))) {
-        my $audiopciaddr = print_pci_addr("audio0", $bridges, $arch);
+        my $audiopciaddr = print_pci_addr("audio0", $arch);
         my $audio_devs = audio_devs($audio, $audiopciaddr, $machine_version);
         push @$devices, @$audio_devs;
     }
@@ -3404,7 +3406,7 @@ sub config_to_command {
 
     if ($vga->{type} && $vga->{type} !~ m/^serial\d+$/ && $vga->{type} ne 'none') {
         push @$devices, '-device',
-            print_vga_device($conf, $vga, $arch, $machine_version, undef, $qxlnum, $bridges);
+            print_vga_device($conf, $vga, $arch, $machine_version, undef, $qxlnum);
 
         push @$cmd, '-display', 'egl-headless,gl=core' if $vga->{type} eq 'virtio-gl'; # VIRGL
 
@@ -3464,7 +3466,7 @@ sub config_to_command {
         push @$devices, '-chardev', "socket,path=$qgasocket,server=on,wait=off,id=qga0";
 
         if (!$guest_agent->{type} || $guest_agent->{type} eq 'virtio') {
-            my $pciaddr = print_pci_addr("qga0", $bridges, $arch);
+            my $pciaddr = print_pci_addr("qga0", $arch);
             push @$devices, '-device', "virtio-serial,id=qga0$pciaddr";
             push @$devices, '-device', 'virtserialport,chardev=qga0,name=org.qemu.guest_agent.0';
         } elsif ($guest_agent->{type} eq 'isa') {
@@ -3475,7 +3477,7 @@ sub config_to_command {
     my $rng = $conf->{rng0} ? parse_rng($conf->{rng0}) : undef;
     if ($rng && $version_guard->(4, 1, 2)) {
         my $rng_object = print_rng_object_commandline('rng0', $rng);
-        my $rng_device = print_rng_device_commandline('rng0', $rng, $bridges, $arch);
+        my $rng_device = print_rng_device_commandline('rng0', $rng, $arch);
         push @$devices, '-object', $rng_object;
         push @$devices, '-device', $rng_device;
     }
@@ -3489,10 +3491,9 @@ sub config_to_command {
         if ($qxlnum > 1) {
             if ($winversion) {
                 for (my $i = 1; $i < $qxlnum; $i++) {
-                    push @$devices, '-device',
-                        print_vga_device(
-                            $conf, $vga, $arch, $machine_version, $i, $qxlnum, $bridges,
-                        );
+                    push @$devices, '-device', print_vga_device(
+                        $conf, $vga, $arch, $machine_version, $i, $qxlnum,
+                    );
                 }
             } else {
                 # assume other OS works like Linux
@@ -3506,7 +3507,7 @@ sub config_to_command {
             }
         }
 
-        my $pciaddr = print_pci_addr("spice", $bridges, $arch);
+        my $pciaddr = print_pci_addr("spice", $arch);
 
         push @$devices, '-device', "virtio-serial,id=spice$pciaddr";
         if ($vga->{'clipboard'} && $vga->{'clipboard'} eq 'vnc') {
@@ -3544,7 +3545,7 @@ sub config_to_command {
 
     # enable balloon by default, unless explicitly disabled
     if (!defined($conf->{balloon}) || $conf->{balloon}) {
-        my $pciaddr = print_pci_addr("balloon0", $bridges, $arch);
+        my $pciaddr = print_pci_addr("balloon0", $arch);
         my $ballooncmd = "virtio-balloon-pci,id=balloon0$pciaddr";
         $ballooncmd .= ",free-page-reporting=on" if min_version($machine_version, 6, 2);
         push @$devices, '-device', $ballooncmd;
@@ -3552,7 +3553,7 @@ sub config_to_command {
 
     if ($conf->{watchdog}) {
         my $wdopts = parse_watchdog($conf->{watchdog});
-        my $pciaddr = print_pci_addr("watchdog", $bridges, $arch);
+        my $pciaddr = print_pci_addr("watchdog", $arch);
         my $watchdog = $wdopts->{model} || 'i6300esb';
         push @$devices, '-device', "$watchdog$pciaddr";
         push @$devices, '-watchdog-action', $wdopts->{action} if $wdopts->{action};
@@ -3594,7 +3595,7 @@ sub config_to_command {
                     "scsi$drive->{index}: machine version 4.1~pve2 or higher is required to use more than 14 SCSI disks\n"
                     if $drive->{index} > 13 && !&$version_guard(4, 1, 2);
 
-                my $pciaddr = print_pci_addr("$controller_prefix$controller", $bridges, $arch);
+                my $pciaddr = print_pci_addr("$controller_prefix$controller", $arch);
                 my $scsihw_type =
                     $scsihw =~ m/^virtio-scsi-single/ ? "virtio-scsi-pci" : $scsihw;
 
@@ -3629,7 +3630,7 @@ sub config_to_command {
 
             if ($drive->{interface} eq 'sata') {
                 my $controller = int($drive->{index} / $PVE::QemuServer::Drive::MAX_SATA_DISKS);
-                my $pciaddr = print_pci_addr("ahci$controller", $bridges, $arch);
+                my $pciaddr = print_pci_addr("ahci$controller", $arch);
                 push @$devices, '-device', "ahci,id=ahci$controller,multifunction=on$pciaddr"
                     if !$ahcicontroller->{$controller};
                 $ahcicontroller->{$controller} = 1;
@@ -3673,10 +3674,9 @@ sub config_to_command {
                 push @$devices, '-drive', $drive_cmd;
             }
 
-            push @$devices, '-device',
-                print_drivedevice_full(
-                    $storecfg, $conf, $vmid, $drive, $bridges, $arch, $machine_type,
-                );
+            push @$devices, '-device', print_drivedevice_full(
+                $storecfg, $conf, $vmid, $drive, $arch, $machine_type,
+            );
         },
     );
 
@@ -3704,7 +3704,6 @@ sub config_to_command {
             $conf,
             $d,
             $netname,
-            $bridges,
             $use_old_bios_files,
             $arch,
             $machine_version,
@@ -3721,7 +3720,7 @@ sub config_to_command {
         if ($q35) {
             $bus = print_pcie_addr("ivshmem");
         } else {
-            $bus = print_pci_addr("ivshmem", $bridges, $arch);
+            $bus = print_pci_addr("ivshmem", $arch);
         }
 
         my $ivshmem_name = $ivshmem->{name} // $vmid;
@@ -3732,34 +3731,6 @@ sub config_to_command {
             "memory-backend-file,id=ivshmem,share=on,mem-path=$path" . ",size=$ivshmem->{size}M";
     }
 
-    # pci.4 is nested in pci.1
-    $bridges->{1} = 1 if $bridges->{4};
-
-    if (!$q35) { # add pci bridges
-        if (min_version($machine_version, 2, 3)) {
-            $bridges->{1} = 1;
-            $bridges->{2} = 1;
-        }
-        $bridges->{3} = 1 if $scsihw =~ m/^virtio-scsi-single/;
-    }
-
-    for my $k (sort { $b cmp $a } 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", undef, $arch);
-        my $devstr = "pci-bridge,id=pci.$k,chassis_nr=$k$pciaddr";
-
-        if ($q35) { # add after -readconfig pve-q35.cfg
-            splice @$devices, 2, 0, '-device', $devstr;
-        } else {
-            unshift @$devices, '-device', $devstr if $k > 0;
-        }
-    }
-
     if (!$kvm) {
         push @$machineFlags, 'accel=tcg';
     }
@@ -3925,7 +3896,7 @@ sub vm_deviceplug {
 
         qemu_driveadd($storecfg, $vmid, $device);
         my $devicefull =
-            print_drivedevice_full($storecfg, $conf, $vmid, $device, undef, $arch, $machine_type);
+            print_drivedevice_full($storecfg, $conf, $vmid, $device, $arch, $machine_type);
 
         qemu_deviceadd($vmid, $devicefull);
         eval { qemu_deviceaddverify($vmid, $deviceid); };
@@ -3936,7 +3907,7 @@ sub vm_deviceplug {
         }
     } elsif ($deviceid =~ m/^(virtioscsi|scsihw)(\d+)$/) {
         my $scsihw = defined($conf->{scsihw}) ? $conf->{scsihw} : "lsi";
-        my $pciaddr = print_pci_addr($deviceid, undef, $arch);
+        my $pciaddr = print_pci_addr($deviceid, $arch);
         my $scsihw_type = $scsihw eq 'virtio-scsi-single' ? "virtio-scsi-pci" : $scsihw;
 
         my $devicefull = "$scsihw_type,id=$deviceid$pciaddr";
@@ -3957,7 +3928,7 @@ sub vm_deviceplug {
         qemu_driveadd($storecfg, $vmid, $device);
 
         my $devicefull =
-            print_drivedevice_full($storecfg, $conf, $vmid, $device, undef, $arch, $machine_type);
+            print_drivedevice_full($storecfg, $conf, $vmid, $device, $arch, $machine_type);
         eval { qemu_deviceadd($vmid, $devicefull); };
         if (my $err = $@) {
             eval { qemu_drivedel($vmid, $deviceid); };
@@ -3973,14 +3944,7 @@ sub vm_deviceplug {
         ($use_old_bios_files, $machine_type) = qemu_use_old_bios_files($machine_type);
 
         my $netdevicefull = print_netdevice_full(
-            $vmid,
-            $conf,
-            $device,
-            $deviceid,
-            undef,
-            $use_old_bios_files,
-            $arch,
-            $machine_version,
+            $vmid, $conf, $device, $deviceid, $use_old_bios_files, $arch, $machine_version,
         );
         qemu_deviceadd($vmid, $netdevicefull);
         eval {
@@ -4211,15 +4175,7 @@ sub qemu_deletescsihw {
 sub qemu_add_pci_bridge {
     my ($storecfg, $conf, $vmid, $device, $arch, $machine_type) = @_;
 
-    my $bridges = {};
-
-    my $bridgeid;
-
-    print_pci_addr($device, $bridges, $arch);
-
-    while (my ($k, $v) = each %$bridges) {
-        $bridgeid = $k;
-    }
+    my $bridgeid = PVE::QemuServer::PCI::get_bridge_for_device($device);
     return 1 if !defined($bridgeid) || $bridgeid < 1;
 
     my $bridge = "pci.$bridgeid";
@@ -4279,7 +4235,7 @@ sub qemu_usb_hotplug {
     my $devicelist = vm_devices_list($vmid);
 
     if (!$devicelist->{xhci}) {
-        my $pciaddr = print_pci_addr("xhci", undef, $arch);
+        my $pciaddr = print_pci_addr("xhci", $arch);
         qemu_deviceadd($vmid, PVE::QemuServer::USB::print_qemu_xhci_controller($pciaddr));
         $added_xhci = 1;
     }
diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
index b80b7dbb..17c46f36 100644
--- a/src/PVE/QemuServer/Drive.pm
+++ b/src/PVE/QemuServer/Drive.pm
@@ -152,7 +152,7 @@ sub get_path_and_format {
 }
 
 my $MAX_IDE_DISKS = 4;
-my $MAX_SCSI_DISKS = 31;
+our $MAX_SCSI_DISKS = 31;
 my $MAX_VIRTIO_DISKS = 16;
 our $MAX_SATA_DISKS = 6;
 our $MAX_UNUSED_DISKS = 256;
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 {
+    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) = @_;
 
@@ -50,7 +69,7 @@ sub scsihw_infos {
 }
 
 sub print_drivedevice_full {
-    my ($storecfg, $conf, $vmid, $drive, $bridges, $arch, $machine_type) = @_;
+    my ($storecfg, $conf, $vmid, $drive, $arch, $machine_type) = @_;
 
     my $device = '';
     my $maxdev = 0;
@@ -61,7 +80,7 @@ sub print_drivedevice_full {
 
     my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
     if ($drive->{interface} eq 'virtio') {
-        my $pciaddr = print_pci_addr("$drive_id", $bridges, $arch);
+        my $pciaddr = print_pci_addr("$drive_id", $arch);
         $device = 'virtio-blk-pci';
         # for the switch to -blockdev, there is no blockdev for 'none'
         if (!min_version($machine_version, 10, 0) || $drive->{file} ne 'none') {
diff --git a/src/PVE/QemuServer/PCI.pm b/src/PVE/QemuServer/PCI.pm
index 0b67943c..54bf9c17 100644
--- a/src/PVE/QemuServer/PCI.pm
+++ b/src/PVE/QemuServer/PCI.pm
@@ -21,6 +21,7 @@ our @EXPORT_OK = qw(
     print_pcie_addr
     print_pcie_root_port
     parse_hostpci
+    get_bridges
 );
 
 our $MAX_HOSTPCI_DEVICES = 16;
@@ -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;
+}
+
 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 {
+    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";
+    }
+
+    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'}) {
+            $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;
diff --git a/src/PVE/QemuServer/RNG.pm b/src/PVE/QemuServer/RNG.pm
index eb477a5d..f854dd50 100644
--- a/src/PVE/QemuServer/RNG.pm
+++ b/src/PVE/QemuServer/RNG.pm
@@ -87,7 +87,7 @@ sub check_rng_source {
 }
 
 sub print_rng_device_commandline {
-    my ($id, $rng, $bridges, $arch) = @_;
+    my ($id, $rng, $arch) = @_;
 
     die "no rng device specified\n" if !$rng;
 
@@ -98,7 +98,7 @@ sub print_rng_device_commandline {
         $limiter_str = ",max-bytes=$max_bytes,period=$period";
     }
 
-    my $rng_addr = print_pci_addr($id, $bridges, $arch);
+    my $rng_addr = print_pci_addr($id, $arch);
 
     return "virtio-rng-pci,rng=$id$limiter_str$rng_addr";
 }
diff --git a/src/PVE/QemuServer/USB.pm b/src/PVE/QemuServer/USB.pm
index c9408a42..d75507e4 100644
--- a/src/PVE/QemuServer/USB.pm
+++ b/src/PVE/QemuServer/USB.pm
@@ -120,7 +120,7 @@ my sub assert_usb_index_is_useable {
 }
 
 sub get_usb_controllers {
-    my ($conf, $bridges, $arch, $machine_version) = @_;
+    my ($conf, $arch, $machine_version) = @_;
 
     my $devices = [];
     my $pciaddr = "";
@@ -134,10 +134,10 @@ sub get_usb_controllers {
     my $is_q35 = PVE::QemuServer::Machine::machine_type_is_q35($conf);
 
     if ($arch eq 'aarch64') {
-        $pciaddr = print_pci_addr('ehci', $bridges, $arch);
+        $pciaddr = print_pci_addr('ehci', $arch);
         push @$devices, '-device', "usb-ehci,id=ehci$pciaddr";
     } elsif (!$is_q35) {
-        $pciaddr = print_pci_addr("piix3", $bridges, $arch);
+        $pciaddr = print_pci_addr("piix3", $arch);
         push @$devices, '-device', "piix3-usb-uhci,id=uhci$pciaddr.0x2";
     }
 
@@ -157,7 +157,7 @@ sub get_usb_controllers {
         push @$devices, '-readconfig', '/usr/share/qemu-server/pve-usb.cfg';
     }
 
-    $pciaddr = print_pci_addr("xhci", $bridges, $arch);
+    $pciaddr = print_pci_addr("xhci", $arch);
     if ($use_qemu_xhci && $any_usb) {
         push @$devices, '-device', print_qemu_xhci_controller($pciaddr);
     } elsif ($use_usb3) {
-- 
2.47.3

partially-applied: [PATCH qemu-server v2 0/4] pci: bridges: cleanup and hotplug fix

[Fiona Ebner]

On Mon, 07 Sep 2026 14:41:46 +0200, Dominik Csapak wrote:
> Main motivation for this series was a defect in how we handle
> hotplugging certain devices:
> 
> When e.g. adding scsi15 on a vm with i440fx + lsi controller, we'd need
> scsihw2 (since only 7 devices can live on scsihw0 and scsihw1
> respectively) which sits on bus pci.4. This itself is not added by
> default to the config, so we tried to hotplug the bridge. This works,
> but without SHPC (which is neither the default, nor does it work
> reliably in all guests) one cannot hot-plug anything into a hot-plugged
> bridge. So when hotplugging scsi15 in this example, we'd hot-plug
> pci.4, but adding scsihw2 would fail and leave the disk in 'pending'.
> 
> [...]

Already applied the first two patches, thanks!

[1/4] test: cfg2cmd: add test with q35 and scsihw2
      commit: 0e6118dc89c5558d764a61a120d96dae39532c1b
[2/4] test: cfg2cmd: add test for legacy-igd passthrough
      commit: 14c0f871fa926e04eee090afae5d2d8670aa4a79

Re: [PATCH qemu-server v2 3/4] pci: don't try to hotplug bridges

[Fiona Ebner]

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 <d.csapak@proxmox.com>
> ---
>  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.

Re: [PATCH qemu-server v2 4/4] pci: bridges: add bridges to devices up front

[Fiona Ebner]

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 <d.csapak@proxmox.com>

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;

superseded: [PATCH qemu-server v2 0/4] pci: bridges: cleanup and hotplug fix

[Dominik Csapak]

superseded by v3:
https://lore.proxmox.com/pve-devel/20260908135729.3869365-1-d.csapak@proxmox.com/T/#t