[PATCH qemu-server 0/5] small cleanup/refactor/fixes

[PATCH qemu-server 0/5] small cleanup/refactor/fixes

[Dominik Csapak]

this series contains a few (more or less independent) cleanups,
refactors and a one fix (3/5).

This is a pre-requsisite for my upcoming rfc v2 of the pci-layout
changes.

While I based this on my recent hotplug v3 series [0], it should
apply cleanly to master.

0: https://lore.proxmox.com/all/20261007142145.984970-1-d.csapak@proxmox.com/

Dominik Csapak (5):
  helpers: factor out the version parts parsing
  usb: drop always-true spice_usb3 feature flag
  net: use version guard for the machine version checks of NICs
  usb: factor out the default usb bus of the tablet
  drive device: check the machine version for blockdev only once

 src/PVE/QemuServer.pm                        | 41 ++++++++------------
 src/PVE/QemuServer/DriveDevice.pm            | 14 +++----
 src/PVE/QemuServer/Helpers.pm                | 22 ++++++++++-
 src/PVE/QemuServer/USB.pm                    | 13 ++++++-
 src/test/cfg2cmd/simple-11.0-binary.conf.cmd |  2 +-
 5 files changed, 55 insertions(+), 37 deletions(-)

-- 
2.47.3

[PATCH qemu-server 1/5] helpers: factor out the version parts parsing

[Dominik Csapak]

this can be useful on it's own when needing to pass the individual parts
of the version to something else.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/QemuServer/Helpers.pm | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/PVE/QemuServer/Helpers.pm b/src/PVE/QemuServer/Helpers.pm
index dd17eef5..1e55ad6c 100644
--- a/src/PVE/QemuServer/Helpers.pm
+++ b/src/PVE/QemuServer/Helpers.pm
@@ -21,6 +21,7 @@ our @EXPORT_OK = qw(
     parse_number_sets
     windows_version
     get_host_arch
+    version_parts
 );
 
 my $nodename = PVE::INotify::nodename();
@@ -224,11 +225,28 @@ sub vm_running_locally {
     return instance_running_locally($pidfile);
 }
 
+# Parses a version string like X.Y.Z-pveW and returns (X, Y, Z, W) as numeric values.
+sub version_parts {
+    my ($verstr) = @_;
+
+    if ($verstr =~ m/^(\d+)\.(\d+)(?:\.(\d+))?(?:\+pve(\d+))?/) {
+        my $major = $1 + 0;
+        my $minor = $2 + 0;
+        my $patch = $3;
+        $patch += 0 if defined($patch);
+        my $pve = $4;
+        $pve += 0 if defined($pve);
+        return ($major, $minor, $patch, $pve);
+    }
+
+    return;
+}
+
 sub min_version {
     my ($verstr, $major, $minor, $pve) = @_;
 
-    if ($verstr =~ m/^(\d+)\.(\d+)(?:\.(\d+))?(?:\+pve(\d+))?/) {
-        return 1 if version_cmp($1, $major, $2, $minor, $4, $pve) >= 0;
+    if (my ($got_major, $got_minor, $got_patch, $got_pve) = version_parts($verstr)) {
+        return 1 if version_cmp($got_major, $major, $got_minor, $minor, $got_pve, $pve) >= 0;
         return 0;
     }
 
-- 
2.47.3

[PATCH qemu-server 2/5] usb: drop always-true spice_usb3 feature flag

[Dominik Csapak]

The 'spice_usb3' feature was only set for machine versions 4.0 and
newer. Since config_to_command refuses to start machines older than
5.0, the flag is always set, so a SPICE USB port with 'usb3' enabled
always ends up on the xhci controller anyway.

Drop the feature hash from get_usb_devices and check 'usb3' directly.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/QemuServer.pm     | 8 +-------
 src/PVE/QemuServer/USB.pm | 4 ++--
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 7e5f7bbb..097c88b6 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3325,13 +3325,7 @@ sub config_to_command {
             $vmid, $conf, $devices, $vga, $winversion, $arch, $bootorder, $dry_run,
         );
 
-    # usb devices
-    my $usb_dev_features = {};
-    $usb_dev_features->{spice_usb3} = 1 if min_version($machine_version, 4, 0);
-
-    my @usbdevices = PVE::QemuServer::USB::get_usb_devices(
-        $conf, $usb_dev_features, $bootorder, $machine_version,
-    );
+    my @usbdevices = PVE::QemuServer::USB::get_usb_devices($conf, $bootorder, $machine_version);
     push @$devices, @usbdevices if @usbdevices;
 
     # serial devices
diff --git a/src/PVE/QemuServer/USB.pm b/src/PVE/QemuServer/USB.pm
index d75507e4..bf2741f6 100644
--- a/src/PVE/QemuServer/USB.pm
+++ b/src/PVE/QemuServer/USB.pm
@@ -168,7 +168,7 @@ sub get_usb_controllers {
 }
 
 sub get_usb_devices {
-    my ($conf, $features, $bootorder, $machine_version) = @_;
+    my ($conf, $bootorder, $machine_version) = @_;
 
     my $devices = [];
 
@@ -190,7 +190,7 @@ sub get_usb_devices {
         if ($d->{host} && $d->{host} =~ m/^spice$/) {
             # usb redir support for spice
             my $bus = 'ehci';
-            $bus = 'xhci' if ($d->{usb3} && $features->{spice_usb3}) || $use_qemu_xhci;
+            $bus = 'xhci' if $d->{usb3} || $use_qemu_xhci;
 
             push @$devices, '-chardev', "spicevmc,id=usbredirchardev$i,name=usbredir";
             push @$devices, '-device', print_spice_usbdevice($i, $bus, $port);
-- 
2.47.3

[PATCH qemu-server 3/5] net: use version guard for the machine version checks of NICs

[Dominik Csapak]

Since commit 41734ab8 ("fix #7627: net: virtio: disable host_tunnel
feature again with 11.0+pve1"), VirtIO NICs get 'host_tunnel=off' with
machine version 11.0+pve1 and newer. The check uses min_version, which
does not record the required PVE machine revision.

So a guest started with an unversioned machine type on QEMU 11.0 got
'host_tunnel=off', but its machine type was recorded as '+pve0'.
Migration, snapshots and hibernation then use that recorded machine
type, for which the NIC is generated without 'host_tunnel=off'.

The checks in print_netdevice_full could not use the version guard,
since the function is also called for hotplug, where there is none.
The 'host_mtu' differentiation for 10.0+pve1 works around that with a
separate guard call in config_to_command.

Instead, pass the version guard to print_netdevice_full and use it for
all of its machine version checks, so the required revision gets
recorded where the feature is decided. For hotplug, pass a stub
around min_version, as the machine is already running there.

The separate guard call for 'host_mtu' is kept, as it also records
'+pve1' for NICs that skip the 'host_mtu' check, for example when it
is omitted for migration compatibility.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/QemuServer.pm                        | 23 ++++++++++++--------
 src/test/cfg2cmd/simple-11.0-binary.conf.cmd |  2 +-
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 097c88b6..39096199 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1327,7 +1327,7 @@ sub print_netdevice_full {
         $netid,
         $use_old_bios_files,
         $arch,
-        $machine_version,
+        $version_guard, # checks the machine version and records the required +pveX revision
         $host_mtu_migration, # force this value for host_mtu, 0 means force absence of param
     ) = @_;
 
@@ -1343,12 +1343,12 @@ sub print_netdevice_full {
         # and out of each queue plus one config interrupt and control vector queue
         my $vectors = $net->{queues} * 2 + 2;
         $tmpstr .= ",vectors=$vectors,mq=on";
-        if (min_version($machine_version, 7, 1)) {
+        if ($version_guard->(7, 1)) {
             $tmpstr .= ",packed=on";
         }
     }
 
-    if (min_version($machine_version, 7, 1) && $net->{model} eq 'virtio') {
+    if ($version_guard->(7, 1) && $net->{model} eq 'virtio') {
         $tmpstr .= ",rx_queue_size=1024,tx_queue_size=256";
     }
 
@@ -1378,7 +1378,7 @@ sub print_netdevice_full {
                 . " that the bridge is configured as desired.\n";
         }
 
-        if (min_version($machine_version, 10, 0, 1) || $host_mtu_migration) {
+        if ($version_guard->(10, 0, 1) || $host_mtu_migration) {
             # Always add host_mtu for migration compatibility, because the presence of host_mtu
             # means that the virtual hardware is generated differently (at least for i440fx)
             $tmpstr .= ",host_mtu=$mtu";
@@ -1390,7 +1390,7 @@ sub print_netdevice_full {
         if ($migration_skip_host_mtu) {
             # When the machine version is less than 10.0+pve1 and the MTU is 1500, not having the
             # host_mtu parameter is fully expected. Only log when not expected to avoid confusion.
-            if (min_version($machine_version, 10, 0, 1) || $mtu != 1500) {
+            if ($version_guard->(10, 0, 1) || $mtu != 1500) {
                 log_warn(
                     "$msg_prefix, not used on the source side according to migration parameters");
             }
@@ -1423,10 +1423,10 @@ sub print_netdevice_full {
     # include the fix "virtio_net: do not allow tunnel csum offload for non GSO packets" with a
     # prominent notice in the upgrade guide.
     if ($net->{model} eq 'virtio') {
-        if (min_version($machine_version, 11, 0, 1)) {
+        if ($version_guard->(11, 0, 1)) {
             # Turn off if not explicitly enabled starting with 11.0+pve1
             $tmpstr .= ",host_tunnel=off" if !$net->{'host-tunnel'};
-        } elsif (min_version($machine_version, 10, 2)) {
+        } elsif ($version_guard->(10, 2)) {
             # The feature was introduced in QEMU 10.2. Turn off if explicitly disabled.
             $tmpstr .= ",host_tunnel=off"
                 if defined($net->{'host-tunnel'}) && !$net->{'host-tunnel'};
@@ -3705,7 +3705,7 @@ sub config_to_command {
             $netname,
             $use_old_bios_files,
             $arch,
-            $machine_version,
+            $version_guard,
             $nets_host_mtu->{$netname},
         );
 
@@ -3949,8 +3949,13 @@ sub vm_deviceplug {
         my $use_old_bios_files = undef;
         ($use_old_bios_files, $machine_type) = qemu_use_old_bios_files($machine_type);
 
+        # the machine is already running, so there is no required version to record
+        my $version_guard = sub {
+            my ($major, $minor, $pve) = @_;
+            return min_version($machine_version, $major, $minor, $pve);
+        };
         my $netdevicefull = print_netdevice_full(
-            $vmid, $conf, $device, $deviceid, $use_old_bios_files, $arch, $machine_version,
+            $vmid, $conf, $device, $deviceid, $use_old_bios_files, $arch, $version_guard,
         );
         qemu_deviceadd($vmid, $netdevicefull);
         eval {
diff --git a/src/test/cfg2cmd/simple-11.0-binary.conf.cmd b/src/test/cfg2cmd/simple-11.0-binary.conf.cmd
index 924d8041..5d01263e 100644
--- a/src/test/cfg2cmd/simple-11.0-binary.conf.cmd
+++ b/src/test/cfg2cmd/simple-11.0-binary.conf.cmd
@@ -32,4 +32,4 @@
   -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,device_id=drive-scsi0,bootindex=100,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=A2:C0:43:77:08:A0,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 'hpet=off,type=pc+pve0'
+  -machine 'hpet=off,type=pc+pve1'
-- 
2.47.3

[PATCH qemu-server 4/5] usb: factor out the default usb bus of the tablet

[Dominik Csapak]

The bus of the tablet depends on which USB controllers exist, which is
decided in get_usb_controllers. Move the choice next to it, so both can
be adapted together when the controllers change.

No functional change intended.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/QemuServer.pm     | 10 +---------
 src/PVE/QemuServer/USB.pm |  9 +++++++++
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 39096199..4a67d3a0 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1182,15 +1182,7 @@ sub assert_clipboard_config {
 sub print_tabletdevice_full {
     my ($conf, $arch) = @_;
 
-    my $q35 = PVE::QemuServer::Machine::machine_type_is_q35($conf);
-
-    # we use uhci for old VMs because tablet driver was buggy in older qemu
-    my $usbbus;
-    if ($q35 || $arch eq 'aarch64') {
-        $usbbus = 'ehci';
-    } else {
-        $usbbus = 'uhci';
-    }
+    my $usbbus = PVE::QemuServer::USB::get_default_usb_bus($conf, $arch);
 
     return "usb-tablet,id=tablet,bus=$usbbus.0,port=1";
 }
diff --git a/src/PVE/QemuServer/USB.pm b/src/PVE/QemuServer/USB.pm
index bf2741f6..0ab43614 100644
--- a/src/PVE/QemuServer/USB.pm
+++ b/src/PVE/QemuServer/USB.pm
@@ -119,6 +119,15 @@ my sub assert_usb_index_is_useable {
     return undef;
 }
 
+sub get_default_usb_bus {
+    my ($conf, $arch) = @_;
+
+    my $q35 = PVE::QemuServer::Machine::machine_type_is_q35($conf);
+
+    # we use uhci for old VMs because tablet driver was buggy in older qemu
+    return $q35 || $arch eq 'aarch64' ? 'ehci' : 'uhci';
+}
+
 sub get_usb_controllers {
     my ($conf, $arch, $machine_version) = @_;
 
-- 
2.47.3

[PATCH qemu-server 5/5] drive device: check the machine version for blockdev only once

[Dominik Csapak]

print_drivedevice_full checks for machine version 10.0 in five places
to decide whether the drive uses -blockdev. Do the check once, so the
places read as what they decide and only one call needs adapting when
the source of the machine version changes.

No functional change intended.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/QemuServer/DriveDevice.pm | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/PVE/QemuServer/DriveDevice.pm b/src/PVE/QemuServer/DriveDevice.pm
index 8974bae0..621fcc2b 100644
--- a/src/PVE/QemuServer/DriveDevice.pm
+++ b/src/PVE/QemuServer/DriveDevice.pm
@@ -78,12 +78,14 @@ sub print_drivedevice_full {
         PVE::QemuServer::Machine::extract_version($machine_type, kvm_user_version());
     my $has_write_cache = 1; # whether the device has a 'write-cache' option
 
+    my $use_blockdev = min_version($machine_version, 10, 0);
+
     my $drive_id = PVE::QemuServer::Drive::get_drive_id($drive);
     if ($drive->{interface} eq 'virtio') {
         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') {
+        if (!$use_blockdev || $drive->{file} ne 'none') {
             $device .= ",drive=drive-$drive_id";
         }
         $device .= ",id=${drive_id}${pciaddr}";
@@ -104,16 +106,14 @@ sub print_drivedevice_full {
                 . ",lun=$drive->{index}";
         }
         # for the switch to -blockdev, there is no blockdev for 'none'
-        if (!min_version($machine_version, 10, 0) || $drive->{file} ne 'none') {
+        if (!$use_blockdev || $drive->{file} ne 'none') {
             $device .= ",drive=drive-$drive_id";
         }
         $device .= ",id=$drive_id";
 
         # For the switch to -blockdev, the SCSI device ID needs to be explicitly specified. Note
         # that only ide-cd and ide-hd have a 'device_id' option.
-        if (
-            min_version($machine_version, 10, 0) && ($device_type eq 'cd' || $device_type eq 'hd')
-        ) {
+        if ($use_blockdev && ($device_type eq 'cd' || $device_type eq 'hd')) {
             $device .= ",device_id=drive-${drive_id}";
         }
 
@@ -169,7 +169,7 @@ sub print_drivedevice_full {
         } else {
             $device .= ",bus=ahci$controller.$unit";
         }
-        if (!min_version($machine_version, 10, 0) || $drive->{file} ne 'none') {
+        if (!$use_blockdev || $drive->{file} ne 'none') {
             $device .= ",drive=drive-$drive_id";
         }
         $device .= ",id=$drive_id";
@@ -198,7 +198,7 @@ sub print_drivedevice_full {
         $device .= ",serial=$serial";
     }
 
-    if (min_version($machine_version, 10, 0)) { # for the switch to -blockdev
+    if ($use_blockdev) { # for the switch to -blockdev
         if (!drive_is_cdrom($drive) && $has_write_cache) {
             my $write_cache = 'on';
             if (my $cache = $drive->{cache}) {
-- 
2.47.3