[pve-devel] [PATCH qemu-server v2 24/32] qm: showcmd: never reserve PCI devices

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

Never reserve PCI devices when config_to_command() is called for
'showcmd'. Previously, choose_hostpci_devices() only skipped reserving
PCI devices when the VM was already running as a heuristic. Make it
explicit via a new parameter. Adapt the config-to-command test to
never expect actual reservation. There is a dedicated test for this
since commit "test: add tests for PCI reservations".

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v2.

 src/PVE/QemuServer.pm                         | 19 +++++--------------
 src/PVE/QemuServer/PCI.pm                     | 15 ++++++---------
 .../q35-linux-hostpci-mapping.conf.cmd        |  2 +-
 src/test/run_config2command_tests.pl          | 14 ++------------
 4 files changed, 14 insertions(+), 36 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 245e1a26..843db76a 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3528,8 +3528,8 @@ my sub get_vga_properties {
 sub config_to_command {
     my ($storecfg, $vmid, $conf, $defaults, $options) = @_;
 
-    my ($forcemachine, $forcecpu, $live_restore_backing) =
-        $options->@{qw(force-machine force-cpu live-restore-backing)};
+    my ($forcemachine, $forcecpu, $live_restore_backing, $dry_run) =
+        $options->@{qw(force-machine force-cpu live-restore-backing dry-run)};
 
     # minimize config for templates, they can only start for backup,
     # so most options besides the disks are irrelevant
@@ -3738,7 +3738,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,
+            $vmid, $conf, $devices, $vga, $winversion, $bridges, $arch, $bootorder, $dry_run,
         );
 
     # usb devices
@@ -6325,7 +6325,7 @@ sub vm_commandline {
 
     my $conf = PVE::QemuConfig->load_config($vmid);
 
-    my $options = {};
+    my $options = { 'dry-run' => 1 };
     if ($snapname) {
         my $snapshot = $conf->{snapshots}->{$snapname};
         die "snapshot '$snapname' does not exist\n" if !defined($snapshot);
@@ -6350,18 +6350,9 @@ sub vm_commandline {
         PVE::Storage::activate_volumes($storecfg, $volumes);
     }
 
-    my $cmd;
-    eval { $cmd = config_to_command($storecfg, $vmid, $conf, $defaults, $options); };
-    my $err = $@;
-
-    # If the vm is not running, need to clean up the reserved/created devices.
     # There might be concurrent operations on the volumes, so do not deactivate.
-    if (!$running) {
-        eval { cleanup_pci_devices($vmid, $conf) };
-        warn $@ if $@;
-    }
 
-    die $err if $err;
+    my $cmd = config_to_command($storecfg, $vmid, $conf, $defaults, $options);
 
     return PVE::Tools::cmd2string($cmd);
 }
diff --git a/src/PVE/QemuServer/PCI.pm b/src/PVE/QemuServer/PCI.pm
index 2dbc87a7..ddf2f028 100644
--- a/src/PVE/QemuServer/PCI.pm
+++ b/src/PVE/QemuServer/PCI.pm
@@ -577,10 +577,7 @@ my sub create_nvidia_device {
 # mdev devices must be chosen later when we actually allocate it, but we
 # flatten the inner list since there can only be one device per alternative anyway
 sub choose_hostpci_devices {
-    my ($devices, $vmid) = @_;
-
-    # if the vm is running, we must be in 'showcmd', so don't actually reserve or create anything
-    my $is_running = PVE::QemuServer::Helpers::vm_running_locally($vmid) ? 1 : 0;
+    my ($devices, $vmid, $dry_run) = @_;
 
     my $used = {};
 
@@ -606,7 +603,7 @@ sub choose_hostpci_devices {
             # we only have one alternative, use that
             $device->{ids} = $device->{ids}->[0];
             $add_used_device->($device->{ids});
-            if ($device->{nvidia} && !$is_running) {
+            if ($device->{nvidia} && !$dry_run) {
                 reserve_pci_usage($device->{ids}->[0]->{id}, $vmid, 10, undef);
                 create_nvidia_device($device->{ids}->[0]->{id}, $device->{nvidia});
             }
@@ -618,12 +615,12 @@ sub choose_hostpci_devices {
             my $ids = [map { $_->{id} } @$alternative];
 
             next if grep { defined($used->{$_}) } @$ids; # already used
-            if (!$is_running) {
+            if (!$dry_run) {
                 eval { reserve_pci_usage($ids, $vmid, 10, undef) };
                 next if $@;
             }
 
-            if ($device->{nvidia} && !$is_running) {
+            if ($device->{nvidia} && !$dry_run) {
                 eval { create_nvidia_device($ids->[0], $device->{nvidia}) };
                 if (my $err = $@) {
                     warn $err;
@@ -645,14 +642,14 @@ sub choose_hostpci_devices {
 }
 
 sub print_hostpci_devices {
-    my ($vmid, $conf, $devices, $vga, $winversion, $bridges, $arch, $bootorder) = @_;
+    my ($vmid, $conf, $devices, $vga, $winversion, $bridges, $arch, $bootorder, $dry_run) = @_;
 
     my $kvm_off = 0;
     my $gpu_passthrough = 0;
     my $legacy_igd = 0;
 
     my $pciaddr;
-    my $pci_devices = choose_hostpci_devices(parse_hostpci_devices($conf), $vmid);
+    my $pci_devices = choose_hostpci_devices(parse_hostpci_devices($conf), $vmid, $dry_run);
 
     for (my $i = 0; $i < $MAX_HOSTPCI_DEVICES; $i++) {
         my $id = "hostpci$i";
diff --git a/src/test/cfg2cmd/q35-linux-hostpci-mapping.conf.cmd b/src/test/cfg2cmd/q35-linux-hostpci-mapping.conf.cmd
index 533e1983..6df8b5f2 100644
--- a/src/test/cfg2cmd/q35-linux-hostpci-mapping.conf.cmd
+++ b/src/test/cfg2cmd/q35-linux-hostpci-mapping.conf.cmd
@@ -29,7 +29,7 @@
   -device 'usb-tablet,id=tablet,bus=ehci.0,port=1' \
   -device 'vfio-pci,host=0000:07:10.0,id=hostpci0,bus=pci.0,addr=0x10' \
   -device 'vfio-pci,sysfsdev=/sys/bus/mdev/devices/00000001-0000-0000-0000-000000008006,id=hostpci1,bus=pci.0,addr=0x11' \
-  -device 'vfio-pci,host=0000:07:10.4,id=hostpci2,bus=pci.0,addr=0x1b' \
+  -device 'vfio-pci,host=0000:07:10.1,id=hostpci2,bus=pci.0,addr=0x1b' \
   -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' \
diff --git a/src/test/run_config2command_tests.pl b/src/test/run_config2command_tests.pl
index cbd8cd8b..9f4ecabf 100755
--- a/src/test/run_config2command_tests.pl
+++ b/src/test/run_config2command_tests.pl
@@ -474,20 +474,10 @@ $mapping_pci_module->mock(
 my $pci_module = Test::MockModule->new("PVE::QemuServer::PCI");
 $pci_module->mock(
     reserve_pci_usage => sub {
-        my ($ids, $vmid, $timeout, $pid, $dryrun) = @_;
-
-        $ids = [$ids] if !ref($ids);
-
-        for my $id (@$ids) {
-            if ($id eq "0000:07:10.1") {
-                die "reserved";
-            }
-        }
-
-        return undef;
+        die "reserve_pci_usage should not be called for 'qm showcmd'\n";
     },
     create_nvidia_device => sub {
-        return 1;
+        die "create_nvidia_device should not be called for 'qm showcmd'\n";
     },
 );
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel