[PATCH qemu-server v3 7/8] cfg2cmd: pci: add bridges to device list up front

[Dominik Csapak <d.csapak@proxmox.com>]

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)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/QemuServer.pm             | 100 ++++++++++--------------------
 src/PVE/QemuServer/Drive.pm       |   2 +-
 src/PVE/QemuServer/DriveDevice.pm |  23 ++++++-
 src/PVE/QemuServer/PCI.pm         |  54 ++++++++++++++--
 src/PVE/QemuServer/RNG.pm         |   4 +-
 src/PVE/QemuServer/USB.pm         |   8 +--
 6 files changed, 110 insertions(+), 81 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 08580fed..a5737f59 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 parse_hostpci);
+use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr parse_hostpci get_pci_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};
@@ -3290,13 +3288,17 @@ sub config_to_command {
         push @$cmd, $fixups->@*;
     }
 
+    my $max_scsihw = PVE::QemuServer::DriveDevice::get_max_scsihw_index($conf);
+    if (my $bridges = get_pci_bridges($conf, $arch, $q35, $max_scsihw)) {
+        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);
@@ -3320,7 +3322,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
@@ -3364,7 +3366,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;
     }
@@ -3409,7 +3411,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
 
@@ -3469,7 +3471,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') {
@@ -3480,7 +3482,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;
     }
@@ -3494,10 +3496,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
@@ -3511,7 +3512,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') {
@@ -3549,7 +3550,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;
@@ -3557,7 +3558,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};
@@ -3599,7 +3600,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;
 
@@ -3634,7 +3635,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;
@@ -3678,10 +3679,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,
+            );
         },
     );
 
@@ -3709,7 +3709,6 @@ sub config_to_command {
             $conf,
             $d,
             $netname,
-            $bridges,
             $use_old_bios_files,
             $arch,
             $machine_version,
@@ -3726,7 +3725,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;
@@ -3737,34 +3736,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';
     }
@@ -3928,7 +3899,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); };
@@ -3939,7 +3910,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";
@@ -3960,7 +3931,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); };
@@ -3976,14 +3947,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 {
@@ -4274,7 +4238,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..8974bae0 100644
--- a/src/PVE/QemuServer/DriveDevice.pm
+++ b/src/PVE/QemuServer/DriveDevice.pm
@@ -49,8 +49,27 @@ sub scsihw_infos {
     return ($maxdev, $controller, $controller_prefix);
 }
 
+# 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 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 8069f104..2d10f2e1 100644
--- a/src/PVE/QemuServer/PCI.pm
+++ b/src/PVE/QemuServer/PCI.pm
@@ -20,6 +20,7 @@ our @EXPORT_OK = qw(
     print_pci_addr
     print_pcie_addr
     parse_hostpci
+    get_pci_bridges
 );
 
 our $MAX_HOSTPCI_DEVICES = 16;
@@ -303,7 +304,7 @@ sub get_pci_bridge_for_device {
 }
 
 sub print_pci_addr {
-    my ($id, $bridges, $arch) = @_;
+    my ($id, $arch) = @_;
 
     die "aarch64 cannot use IDE devices\n" if $arch eq 'aarch64' && $id =~ /^ide/;
 
@@ -316,7 +317,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;
@@ -627,7 +627,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;
@@ -658,7 +658,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}->@*);
@@ -881,4 +881,50 @@ sub reserve_pci_usage {
     die $@ if $@;
 }
 
+# Returns a list of bridge devices which are necessary for the remaining
+# devices.
+sub get_pci_bridges {
+    my ($conf, $arch, $q35, $max_scsihw) = @_;
+
+    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 of a 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