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

[Fiona Ebner <f.ebner@proxmox.com>]

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;