[pve-devel] [PATCH qemu-server 0/8] disable S3/S4 power states by default

[pve-devel] [PATCH qemu-server 0/8] disable S3/S4 power states by default

[Dominik Csapak]

since they make some problems (e.g. windows hybrid shutdown is enabled
by default then -> which makes vGPU problem). Libvirt/virsh also
disables that by default (and tries preventing enabling it.)

This series introduces a new pve1 version for 9.2 machine versions, and
pins new windows guests to that. For this, we need to save
the current pve version in the meta info too now.

Additionally I introduce 'enable-s{3,4}' settings so that users can
still manually enable that (in case it's needed), or disable it for
older machine types. (non hotpluggable).

I deliberatly did not introduce a GUI option for the enabling of
S3/S4 power states, since I don't think it'll come up often.
(If it does, we can still add something to the UI)

The only ugly thing left, is that one cannot select the
9.2+pve1 variant in the gui manually (e.g. for windows), since
we don't have anything in the UI for that.

The question here is if I should introduce a pve version selector.
(I personally think it's not a great idea, because it does not really
convey any meaning to the user if e.g. pve0 or pve1 is better...)

Note that patch 1 is only an RFC and has only tangentially to do with
the series. I sent it along, because the way we test will change
the tests again if we apply the rest of this series and bump to the next
qemu version (due to the +pve1 -> +pve0 change).

Dominik Csapak (8):
  tests: cfg2cmd: pin QEMU version
  config to command: add one '-global' option for each flag
  meta info: also add current pve-machine version
  machine: correctly select pve machine version for non pinned windows
    guests
  machine: incorporate pve machine version when pinning windows guests
  machine: add S3/S4 power state properties
  machine: bump pve machine version and reverse the s3/s4 defaults
  tests: cfg2cmd: add test for windows machine pinning from meta info

 PVE/API2/Qemu.pm                              |  4 +-
 PVE/QemuServer.pm                             |  7 +-
 PVE/QemuServer/Machine.pm                     | 71 +++++++++++++++++--
 PVE/QemuServer/MetaInfo.pm                    | 28 +++++---
 test/cfg2cmd/bootorder-empty.conf.cmd         |  4 +-
 test/cfg2cmd/bootorder-legacy.conf.cmd        |  4 +-
 test/cfg2cmd/bootorder.conf.cmd               |  4 +-
 ...putype-icelake-client-deprecation.conf.cmd |  4 +-
 .../custom-cpu-model-defaults.conf.cmd        |  4 +-
 test/cfg2cmd/efi-raw-template.conf.cmd        |  4 +-
 test/cfg2cmd/efi-raw.conf.cmd                 |  4 +-
 test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd |  4 +-
 test/cfg2cmd/efi-secboot-and-tpm.conf.cmd     |  4 +-
 test/cfg2cmd/efidisk-on-rbd.conf.cmd          |  4 +-
 test/cfg2cmd/i440fx-viommu-virtio.conf.cmd    |  4 +-
 test/cfg2cmd/ide.conf.cmd                     |  4 +-
 .../cfg2cmd/memory-hotplug-hugepages.conf.cmd |  4 +-
 test/cfg2cmd/memory-hotplug.conf.cmd          |  4 +-
 test/cfg2cmd/memory-hugepages-1g.conf.cmd     |  4 +-
 test/cfg2cmd/memory-hugepages-2m.conf.cmd     |  4 +-
 test/cfg2cmd/minimal-defaults.conf.cmd        |  4 +-
 test/cfg2cmd/netdev-7.1-multiqueues.conf.cmd  |  4 +-
 test/cfg2cmd/netdev-7.1.conf.cmd              |  4 +-
 test/cfg2cmd/q35-ide.conf.cmd                 |  4 +-
 .../q35-linux-hostpci-mapping.conf.cmd        |  4 +-
 .../q35-linux-hostpci-multifunction.conf.cmd  |  4 +-
 .../q35-linux-hostpci-template.conf.cmd       |  4 +-
 ...q35-linux-hostpci-x-pci-overrides.conf.cmd |  4 +-
 test/cfg2cmd/q35-linux-hostpci.conf.cmd       |  4 +-
 test/cfg2cmd/q35-simple.conf.cmd              |  4 +-
 test/cfg2cmd/q35-viommu-intel.conf.cmd        |  4 +-
 test/cfg2cmd/q35-viommu-virtio.conf.cmd       |  4 +-
 test/cfg2cmd/q35-windows-pinning-pvever.conf  |  5 ++
 .../q35-windows-pinning-pvever.conf.cmd       | 26 +++++++
 test/cfg2cmd/q35-windows-pinning.conf         |  5 ++
 test/cfg2cmd/q35-windows-pinning.conf.cmd     | 24 +++++++
 test/cfg2cmd/seabios_serial.conf.cmd          |  4 +-
 test/cfg2cmd/simple-btrfs.conf.cmd            |  4 +-
 test/cfg2cmd/simple-virtio-blk.conf.cmd       |  4 +-
 test/cfg2cmd/simple1-template.conf.cmd        |  4 +-
 test/cfg2cmd/simple1.conf.cmd                 |  4 +-
 test/cfg2cmd/vnc-clipboard-spice.conf.cmd     |  4 +-
 test/cfg2cmd/vnc-clipboard-std.conf.cmd       |  4 +-
 test/run_config2command_tests.pl              | 23 +++++-
 44 files changed, 281 insertions(+), 52 deletions(-)
 create mode 100644 test/cfg2cmd/q35-windows-pinning-pvever.conf
 create mode 100644 test/cfg2cmd/q35-windows-pinning-pvever.conf.cmd
 create mode 100644 test/cfg2cmd/q35-windows-pinning.conf
 create mode 100644 test/cfg2cmd/q35-windows-pinning.conf.cmd

-- 
2.39.5



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

[pve-devel] [RFC PATCH qemu-server 1/8] tests: cfg2cmd: pin QEMU version

[Dominik Csapak]

but warn when we're out of date compared to the installed one, and die
when we're one major (+1 minor) release behind.
(the warning is not very visible when running tests or when building)

We don't want to depend on the installed QEMU version for such tests,
otherwise a developer might need to adapt tests because the installed
QEMU version is different to what is e.g. in the build environment for
our packaging.

Also, this way, we have to show intent for bumping this and it will be
obvious that we need to adapt tests because of a changed QEMU version.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 test/run_config2command_tests.pl | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index 7e3d10e6..5fa6f2de 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -20,6 +20,10 @@ use PVE::QemuServer::Monitor;
 use PVE::QemuServer::QMPHelpers;
 use PVE::QemuServer::CPUConfig;
 
+# bump when QEMU version changes
+my $tested_version_major = 9;
+my $tested_version_minor = 2;
+
 my $base_env = {
     storage_config => {
 	ids => {
@@ -80,6 +84,7 @@ my $base_env = {
 	}
     },
     vmid => 8006,
+    tested_qemu_version => "$tested_version_major.$tested_version_minor",
     real_qemu_version => PVE::QemuServer::Helpers::kvm_user_version(), # not yet mocked
 };
 
@@ -184,7 +189,7 @@ sub parse_test($) {
 }
 
 sub get_test_qemu_version {
-    $current_test->{qemu_version} // $base_env->{real_qemu_version} // '2.12';
+    $current_test->{qemu_version} // $base_env->{tested_qemu_version} // '2.12';
 }
 
 my $qemu_server_module;
@@ -528,3 +533,19 @@ if (my $file = shift) {
 }
 
 done_testing();
+
+# reset warn
+$SIG{__WARN__} = undef;
+if ($base_env->{real_qemu_version} =~ m/^(\d+.\d+)/) {
+	if (PVE::QemuServer::Helpers::version_cmp($1, $tested_version_major, $2, $tested_version_minor) < 0) {
+	    warn "\nWARNING: installed QEMU version bigger than tested one, please bump!\n";
+	}
+
+	# if we did not bump since the last major QEMU (+1 minor) release fail the test
+	if (PVE::QemuServer::Helpers::version_cmp($1, $tested_version_major + 1, $2, 1) >= 0) {
+	    die "\nERROR: installed QEMU version one major release bigger than tested one, please bump!\n";
+	}
+
+} else {
+    die "\nERROR: invalid QEMU version\n";
+}
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 2/8] config to command: add one '-global' option for each flag

[Dominik Csapak]

If we have multiple 'globalFlags', we have to encode each one separately
on the commandline with '-global OPTION', since QEMU does not allow to
have multiple options here.

We currently only have one such flag that used the 'globalFlags' list,
so it never popped up. (All other uses directly add an option to the
commandline)

Avoid future bugs by fixing it now.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/QemuServer.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b6fc1f17..13af495d 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4001,7 +4001,9 @@ sub config_to_command {
     push @$cmd, @$devices;
     push @$cmd, '-rtc', join(',', @$rtcFlags) if scalar(@$rtcFlags);
     push @$cmd, '-machine', join(',', @$machineFlags) if scalar(@$machineFlags);
-    push @$cmd, '-global', join(',', @$globalFlags) if scalar(@$globalFlags);
+    for my $flag ($globalFlags->@*) {
+	push @$cmd, '-global', $flag;
+    }
 
     if (my $vmstate = $conf->{vmstate}) {
 	my $statepath = PVE::Storage::path($storecfg, $vmstate);
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 3/8] meta info: also add current pve-machine version

[Dominik Csapak]

when we bump the pve machine version, we also want to include that info
in the meta creation info. With that we can pin guests to the specific
version they were created on.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
we could also give the kvm_version as parameter directly here, since
we already need it...

 PVE/API2/Qemu.pm           |  4 +++-
 PVE/QemuServer/MetaInfo.pm | 28 +++++++++++++++++++---------
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index dc8915a7..0249c0d0 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1207,7 +1207,9 @@ __PACKAGE__->register_method({
 		    assert_scsi_feature_compatibility($opt, $conf, $storecfg, $param->{$opt});
 		}
 
-		$conf->{meta} = PVE::QemuServer::MetaInfo::new_meta_info_string();
+		my $kvm_version = PVE::QemuServer::Helpers::kvm_user_version();
+		my $pve_machine = PVE::QemuServer::Machine::get_pve_version($kvm_version);
+		$conf->{meta} = PVE::QemuServer::MetaInfo::new_meta_info_string($pve_machine);
 
 		my $vollist = [];
 		eval {
diff --git a/PVE/QemuServer/MetaInfo.pm b/PVE/QemuServer/MetaInfo.pm
index a8cb6c5e..78526eb2 100644
--- a/PVE/QemuServer/MetaInfo.pm
+++ b/PVE/QemuServer/MetaInfo.pm
@@ -20,6 +20,13 @@ our $meta_info_fmt = {
 	pattern => '\d+(\.\d+)+',
 	optional => 1,
     },
+    'creation-pve-machine' => {
+	type => 'integer',
+	description => "The PVE machine version current at the time this VM was created.",
+	default => 0,
+	minimum => 1,
+	optional => 1,
+    },
 };
 
 sub parse_meta_info {
@@ -33,15 +40,18 @@ sub parse_meta_info {
 }
 
 sub new_meta_info_string {
-    my () = @_; # for now do not allow to override any value
-
-    return PVE::JSONSchema::print_property_string(
-	{
-	    'creation-qemu' => PVE::QemuServer::Helpers::kvm_user_version(),
-	    ctime => "". int(time()),
-	},
-	$meta_info_fmt,
-    );
+    my ($pve_machine) = @_;
+
+    my $data = {
+	'creation-qemu' => PVE::QemuServer::Helpers::kvm_user_version(),
+	ctime => "". int(time()),
+    };
+
+    if (defined($pve_machine) && $pve_machine > 0) {
+	$data->{'creation-pve-machine'} = $pve_machine;
+    }
+
+    return PVE::JSONSchema::print_property_string($data, $meta_info_fmt);
 }
 
 1;
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 4/8] machine: correctly select pve machine version for non pinned windows guests

[Dominik Csapak]

when we don't have a specific machine version on a windows guest, we use
the creation meta info to pin the machine version. Currently we always
append the pve machine version from the current installed kvm version,
which is not necessarily the version we pinned the guest to.

Instead, use either the info from the creation meta info if it exists,
or use 'pve0'.

For non-windows machines, we used the current QEMU machine version so we
should use the pve machine version from that too.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/QemuServer/Machine.pm | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
index f1acde8f..e3da8e21 100644
--- a/PVE/QemuServer/Machine.pm
+++ b/PVE/QemuServer/Machine.pm
@@ -237,14 +237,19 @@ sub get_vm_machine {
 		if (PVE::QemuServer::Helpers::min_version($meta->{'creation-qemu'}, 9, 1)) {
 		    # need only major.minor
 		    ($base_version) = ($meta->{'creation-qemu'} =~ m/^(\d+.\d+)/);
+		    # append pve machine version if we have one
+		    if (my $pvever = $meta->{'creation-pve-machine'}) {
+			$base_version .= "+pve$pvever"
+		    }
 		}
 	    }
 	    $machine = windows_get_pinned_machine_version($machine, $base_version, $kvmversion);
+	} else{
+	    $arch //= 'x86_64';
+	    $machine ||= default_machine_for_arch($arch);
+	    my $pvever = get_pve_version($kvmversion);
+	    $machine .= "+pve$pvever";
 	}
-	$arch //= 'x86_64';
-	$machine ||= default_machine_for_arch($arch);
-	my $pvever = get_pve_version($kvmversion);
-	$machine .= "+pve$pvever";
     }
 
     if ($machine !~ m/\+pve\d+?(?:\.pxe)?$/) {
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 5/8] machine: incorporate pve machine version when pinning windows guests

[Dominik Csapak]

When creating or updating guests with ostype windows, we want to pin the
machine version to a specific one. Since introduction of that feature,
we never bumped the pve machine version, so this was missing.

Append the pve machine version if it's not 0 so we don't add that
unnecessarily.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/QemuServer/Machine.pm | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
index e3da8e21..ebaf2dcc 100644
--- a/PVE/QemuServer/Machine.pm
+++ b/PVE/QemuServer/Machine.pm
@@ -274,7 +274,17 @@ sub check_and_pin_machine_string {
     if (!$machine || $machine =~ m/^(?:pc|q35|virt)$/) {
 	# always pin Windows' machine version on create, they get confused too easily
 	if (PVE::QemuServer::Helpers::windows_version($ostype)) {
-	    $machine_conf->{type} = windows_get_pinned_machine_version($machine);
+	    my $kvmversion = PVE::QemuServer::Helpers::kvm_user_version();
+	    my $pin_version = get_installed_machine_version($kvmversion);
+
+	    # pin to the current pveX version to make use of most current features if > 0
+	    my $pvever = get_pve_version($kvmversion);
+	    if ($pvever > 0) {
+		$pin_version .= "+pve$pvever";
+	    }
+
+	    $machine_conf->{type} = windows_get_pinned_machine_version($machine, $pin_version, $kvmversion);
+
 	    print "pinning machine type to '$machine_conf->{type}' for Windows guest OS\n";
 	}
     }
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 6/8] machine: add S3/S4 power state properties

[Dominik Csapak]

So users can disable them (they're enabled by default in QEMU)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
This patch may make sense, regardless if we'll apply the reversal of the
default...

 PVE/QemuServer.pm         |  2 ++
 PVE/QemuServer/Machine.pm | 40 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 13af495d..b8ce4750 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3975,6 +3975,8 @@ sub config_to_command {
 	push @$machineFlags, 'accel=tcg';
     }
 
+    PVE::QemuServer::Machine::check_and_set_power_state_flags($globalFlags, $machine_conf);
+
     push @$machineFlags, 'smm=off' if should_disable_smm($conf, $vga, $machine_type);
 
     my $machine_type_min = $machine_type;
diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
index ebaf2dcc..377abc8a 100644
--- a/PVE/QemuServer/Machine.pm
+++ b/PVE/QemuServer/Machine.pm
@@ -31,6 +31,16 @@ my $machine_fmt = {
 	enum => ['intel', 'virtio'],
 	optional => 1,
     },
+    'enable-s3' => {
+	type => 'boolean',
+	description => "Enables S3 power state. Defaults to true.",
+	optional => 1,
+    },
+    'enable-s4' => {
+	type => 'boolean',
+	description => "Enables S4 power state. Defaults to true.",
+	optional => 1,
+    },
 };
 
 PVE::JSONSchema::register_format('pve-qemu-machine-fmt', $machine_fmt);
@@ -293,4 +303,34 @@ sub check_and_pin_machine_string {
     return print_machine($machine_conf);
 }
 
+# disable s3/s4 by default for 9.2+pve1 machine types
+sub check_and_set_power_state_flags {
+    my ($globalFlags, $machine_conf) = @_;
+
+    my $get_flag = sub {
+	my ($q35, $type, $value) = @_;
+
+	if ($q35) {
+	    return "ICH9-LPC.disable_${type}=${value}";
+	} else {
+	    return "PIIX4_PM.disable_${type}=${value}";
+	}
+    };
+
+    my $q35 = $machine_conf->{type} && ($machine_conf->{type} =~ m/q35/) ? 1 : 0;
+
+    my $s3 = $machine_conf->{'enable-s3'} // 1;
+    my $s4 = $machine_conf->{'enable-s4'} // 1;
+
+    # they're enabled by default in QEMU, so only add the flags to disable them
+    if (!$s3) {
+	push $globalFlags->@*, $get_flag->($q35, 's3', 1)
+    }
+    if (!$s4) {
+	push $globalFlags->@*, $get_flag->($q35, 's4', 1)
+    }
+
+    return;
+}
+
 1;
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 7/8] machine: bump pve machine version and reverse the s3/s4 defaults

[Dominik Csapak]

so new guests (or guests with the 'latest' machine type) have
that setting automatically disabled.

The previous default (enabling S3/S4), does not make too much sense in a
virtual environment, and sometimes makes problems, e.g. Windows defaults
to using 'hybrid shutdown' and 'fast startup' when S4 is enabled, which
leads to NVIDIA vGPU being broken on the boot after that.

Since the tests don't pin the pve version themselves, we have to update
all the ones where the machine versions are derived from the installed
QEMU version.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/QemuServer.pm                                |  3 ++-
 PVE/QemuServer/Machine.pm                        | 16 +++++++++++-----
 test/cfg2cmd/bootorder-empty.conf.cmd            |  4 +++-
 test/cfg2cmd/bootorder-legacy.conf.cmd           |  4 +++-
 test/cfg2cmd/bootorder.conf.cmd                  |  4 +++-
 .../cputype-icelake-client-deprecation.conf.cmd  |  4 +++-
 test/cfg2cmd/custom-cpu-model-defaults.conf.cmd  |  4 +++-
 test/cfg2cmd/efi-raw-template.conf.cmd           |  4 +++-
 test/cfg2cmd/efi-raw.conf.cmd                    |  4 +++-
 test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd    |  4 +++-
 test/cfg2cmd/efi-secboot-and-tpm.conf.cmd        |  4 +++-
 test/cfg2cmd/efidisk-on-rbd.conf.cmd             |  4 +++-
 test/cfg2cmd/i440fx-viommu-virtio.conf.cmd       |  4 +++-
 test/cfg2cmd/ide.conf.cmd                        |  4 +++-
 test/cfg2cmd/memory-hotplug-hugepages.conf.cmd   |  4 +++-
 test/cfg2cmd/memory-hotplug.conf.cmd             |  4 +++-
 test/cfg2cmd/memory-hugepages-1g.conf.cmd        |  4 +++-
 test/cfg2cmd/memory-hugepages-2m.conf.cmd        |  4 +++-
 test/cfg2cmd/minimal-defaults.conf.cmd           |  4 +++-
 test/cfg2cmd/netdev-7.1-multiqueues.conf.cmd     |  4 +++-
 test/cfg2cmd/netdev-7.1.conf.cmd                 |  4 +++-
 test/cfg2cmd/q35-ide.conf.cmd                    |  4 +++-
 test/cfg2cmd/q35-linux-hostpci-mapping.conf.cmd  |  4 +++-
 .../q35-linux-hostpci-multifunction.conf.cmd     |  4 +++-
 test/cfg2cmd/q35-linux-hostpci-template.conf.cmd |  4 +++-
 .../q35-linux-hostpci-x-pci-overrides.conf.cmd   |  4 +++-
 test/cfg2cmd/q35-linux-hostpci.conf.cmd          |  4 +++-
 test/cfg2cmd/q35-simple.conf.cmd                 |  4 +++-
 test/cfg2cmd/q35-viommu-intel.conf.cmd           |  4 +++-
 test/cfg2cmd/q35-viommu-virtio.conf.cmd          |  4 +++-
 test/cfg2cmd/seabios_serial.conf.cmd             |  4 +++-
 test/cfg2cmd/simple-btrfs.conf.cmd               |  4 +++-
 test/cfg2cmd/simple-virtio-blk.conf.cmd          |  4 +++-
 test/cfg2cmd/simple1-template.conf.cmd           |  4 +++-
 test/cfg2cmd/simple1.conf.cmd                    |  4 +++-
 test/cfg2cmd/vnc-clipboard-spice.conf.cmd        |  4 +++-
 test/cfg2cmd/vnc-clipboard-std.conf.cmd          |  4 +++-
 37 files changed, 118 insertions(+), 41 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b8ce4750..07fcc97d 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3975,7 +3975,8 @@ sub config_to_command {
 	push @$machineFlags, 'accel=tcg';
     }
 
-    PVE::QemuServer::Machine::check_and_set_power_state_flags($globalFlags, $machine_conf);
+    PVE::QemuServer::Machine::check_and_set_power_state_flags(
+	$globalFlags, $machine_conf, $version_guard);
 
     push @$machineFlags, 'smm=off' if should_disable_smm($conf, $vga, $machine_type);
 
diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
index 377abc8a..20117675 100644
--- a/PVE/QemuServer/Machine.pm
+++ b/PVE/QemuServer/Machine.pm
@@ -12,6 +12,7 @@ use PVE::JSONSchema qw(get_standard_option parse_property_string print_property_
 # version stays the same)
 our $PVE_MACHINE_VERSION = {
     '4.1' => 2,
+    '9.2' => 1,
 };
 
 my $machine_fmt = {
@@ -33,12 +34,12 @@ my $machine_fmt = {
     },
     'enable-s3' => {
 	type => 'boolean',
-	description => "Enables S3 power state. Defaults to true.",
+	description => "Enables S3 power state. Defaults to false beginning with machine types 9.2+pve1, true before.",
 	optional => 1,
     },
     'enable-s4' => {
 	type => 'boolean',
-	description => "Enables S4 power state. Defaults to true.",
+	description => "Enables S4 power state. Defaults to false beginning with machine types 9.2+pve1, true before.",
 	optional => 1,
     },
 };
@@ -305,7 +306,7 @@ sub check_and_pin_machine_string {
 
 # disable s3/s4 by default for 9.2+pve1 machine types
 sub check_and_set_power_state_flags {
-    my ($globalFlags, $machine_conf) = @_;
+    my ($globalFlags, $machine_conf, $version_guard) = @_;
 
     my $get_flag = sub {
 	my ($q35, $type, $value) = @_;
@@ -319,8 +320,13 @@ sub check_and_set_power_state_flags {
 
     my $q35 = $machine_conf->{type} && ($machine_conf->{type} =~ m/q35/) ? 1 : 0;
 
-    my $s3 = $machine_conf->{'enable-s3'} // 1;
-    my $s4 = $machine_conf->{'enable-s4'} // 1;
+    my $default = 1;
+    if ($version_guard->(9, 2, 1)) {
+	$default = 0;
+    }
+
+    my $s3 = $machine_conf->{'enable-s3'} // $default;
+    my $s4 = $machine_conf->{'enable-s4'} // $default;
 
     # they're enabled by default in QEMU, so only add the flags to disable them
     if (!$s3) {
diff --git a/test/cfg2cmd/bootorder-empty.conf.cmd b/test/cfg2cmd/bootorder-empty.conf.cmd
index 87fa6c28..9710a70f 100644
--- a/test/cfg2cmd/bootorder-empty.conf.cmd
+++ b/test/cfg2cmd/bootorder-empty.conf.cmd
@@ -36,4 +36,6 @@
   -device 'virtio-blk-pci,drive=drive-virtio1,id=virtio1,bus=pci.0,addr=0xb,iothread=iothread-virtio1' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/bootorder-legacy.conf.cmd b/test/cfg2cmd/bootorder-legacy.conf.cmd
index a4c3f050..e0dbb174 100644
--- a/test/cfg2cmd/bootorder-legacy.conf.cmd
+++ b/test/cfg2cmd/bootorder-legacy.conf.cmd
@@ -36,4 +36,6 @@
   -device 'virtio-blk-pci,drive=drive-virtio1,id=virtio1,bus=pci.0,addr=0xb,iothread=iothread-virtio1,bootindex=302' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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=100' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/bootorder.conf.cmd b/test/cfg2cmd/bootorder.conf.cmd
index 76bd55d7..a0d1e05b 100644
--- a/test/cfg2cmd/bootorder.conf.cmd
+++ b/test/cfg2cmd/bootorder.conf.cmd
@@ -36,4 +36,6 @@
   -device 'virtio-blk-pci,drive=drive-virtio1,id=virtio1,bus=pci.0,addr=0xb,iothread=iothread-virtio1,bootindex=100' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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=101' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/cputype-icelake-client-deprecation.conf.cmd b/test/cfg2cmd/cputype-icelake-client-deprecation.conf.cmd
index bf084432..95cbe8f9 100644
--- a/test/cfg2cmd/cputype-icelake-client-deprecation.conf.cmd
+++ b/test/cfg2cmd/cputype-icelake-client-deprecation.conf.cmd
@@ -28,4 +28,6 @@
   -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \
   -drive 'file=/var/lib/vz/images/8006/base-8006-disk-0.qcow2,if=none,id=drive-scsi0,discard=on,format=qcow2,cache=none,aio=io_uring,detect-zeroes=unmap' \
   -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/custom-cpu-model-defaults.conf.cmd b/test/cfg2cmd/custom-cpu-model-defaults.conf.cmd
index 15b31fb0..16f04bd0 100644
--- a/test/cfg2cmd/custom-cpu-model-defaults.conf.cmd
+++ b/test/cfg2cmd/custom-cpu-model-defaults.conf.cmd
@@ -22,4 +22,6 @@
   -device 'VGA,id=vga,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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/efi-raw-template.conf.cmd b/test/cfg2cmd/efi-raw-template.conf.cmd
index 3e90c335..0d22095e 100644
--- a/test/cfg2cmd/efi-raw-template.conf.cmd
+++ b/test/cfg2cmd/efi-raw-template.conf.cmd
@@ -23,5 +23,7 @@
   -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
   -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' \
-  -machine 'accel=tcg,type=pc+pve0' \
+  -machine 'accel=tcg,type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1' \
   -snapshot
diff --git a/test/cfg2cmd/efi-raw.conf.cmd b/test/cfg2cmd/efi-raw.conf.cmd
index cf9804bc..94351e4e 100644
--- a/test/cfg2cmd/efi-raw.conf.cmd
+++ b/test/cfg2cmd/efi-raw.conf.cmd
@@ -24,4 +24,6 @@
   -device 'VGA,id=vga,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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd b/test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd
index 911ead05..538a3062 100644
--- a/test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd
+++ b/test/cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd
@@ -25,4 +25,6 @@
   -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' \
-  -machine 'type=q35+pve0'
+  -machine 'type=q35+pve1' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1'
diff --git a/test/cfg2cmd/efi-secboot-and-tpm.conf.cmd b/test/cfg2cmd/efi-secboot-and-tpm.conf.cmd
index 68a85ea0..4b4f80f6 100644
--- a/test/cfg2cmd/efi-secboot-and-tpm.conf.cmd
+++ b/test/cfg2cmd/efi-secboot-and-tpm.conf.cmd
@@ -27,4 +27,6 @@
   -device 'VGA,id=vga,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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/efidisk-on-rbd.conf.cmd b/test/cfg2cmd/efidisk-on-rbd.conf.cmd
index f02039a1..02738217 100644
--- a/test/cfg2cmd/efidisk-on-rbd.conf.cmd
+++ b/test/cfg2cmd/efidisk-on-rbd.conf.cmd
@@ -29,4 +29,6 @@
   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/i440fx-viommu-virtio.conf.cmd b/test/cfg2cmd/i440fx-viommu-virtio.conf.cmd
index 0352354f..8dc8c129 100644
--- a/test/cfg2cmd/i440fx-viommu-virtio.conf.cmd
+++ b/test/cfg2cmd/i440fx-viommu-virtio.conf.cmd
@@ -22,4 +22,6 @@
   -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 virtio-iommu-pci \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/ide.conf.cmd b/test/cfg2cmd/ide.conf.cmd
index 33c6aadc..86920bc7 100644
--- a/test/cfg2cmd/ide.conf.cmd
+++ b/test/cfg2cmd/ide.conf.cmd
@@ -36,4 +36,6 @@
   -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/memory-hotplug-hugepages.conf.cmd b/test/cfg2cmd/memory-hotplug-hugepages.conf.cmd
index f8a8bcb7..64a1d8e5 100644
--- a/test/cfg2cmd/memory-hotplug-hugepages.conf.cmd
+++ b/test/cfg2cmd/memory-hotplug-hugepages.conf.cmd
@@ -59,4 +59,6 @@
   -device 'VGA,id=vga,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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/memory-hotplug.conf.cmd b/test/cfg2cmd/memory-hotplug.conf.cmd
index 859c889d..66834031 100644
--- a/test/cfg2cmd/memory-hotplug.conf.cmd
+++ b/test/cfg2cmd/memory-hotplug.conf.cmd
@@ -171,4 +171,6 @@
   -device 'VGA,id=vga,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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/memory-hugepages-1g.conf.cmd b/test/cfg2cmd/memory-hugepages-1g.conf.cmd
index 352242c4..29187c18 100644
--- a/test/cfg2cmd/memory-hugepages-1g.conf.cmd
+++ b/test/cfg2cmd/memory-hugepages-1g.conf.cmd
@@ -27,4 +27,6 @@
   -device 'VGA,id=vga,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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/memory-hugepages-2m.conf.cmd b/test/cfg2cmd/memory-hugepages-2m.conf.cmd
index 5594e878..a0175160 100644
--- a/test/cfg2cmd/memory-hugepages-2m.conf.cmd
+++ b/test/cfg2cmd/memory-hugepages-2m.conf.cmd
@@ -27,4 +27,6 @@
   -device 'VGA,id=vga,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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/minimal-defaults.conf.cmd b/test/cfg2cmd/minimal-defaults.conf.cmd
index 8da69fee..1def953a 100644
--- a/test/cfg2cmd/minimal-defaults.conf.cmd
+++ b/test/cfg2cmd/minimal-defaults.conf.cmd
@@ -22,4 +22,6 @@
   -device 'VGA,id=vga,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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/netdev-7.1-multiqueues.conf.cmd b/test/cfg2cmd/netdev-7.1-multiqueues.conf.cmd
index 2c6c9054..9c08eb32 100644
--- a/test/cfg2cmd/netdev-7.1-multiqueues.conf.cmd
+++ b/test/cfg2cmd/netdev-7.1-multiqueues.conf.cmd
@@ -23,4 +23,6 @@
   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on,queues=2' \
   -device 'virtio-net-pci,mac=A2:C0:43:77:08:A0,netdev=net0,bus=pci.0,addr=0x12,id=net0,vectors=6,mq=on,packed=on,rx_queue_size=1024,tx_queue_size=256,bootindex=300,host_mtu=900' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/netdev-7.1.conf.cmd b/test/cfg2cmd/netdev-7.1.conf.cmd
index 6ffa9717..d77dcb82 100644
--- a/test/cfg2cmd/netdev-7.1.conf.cmd
+++ b/test/cfg2cmd/netdev-7.1.conf.cmd
@@ -23,4 +23,6 @@
   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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=900' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/q35-ide.conf.cmd b/test/cfg2cmd/q35-ide.conf.cmd
index dd4f1bbe..45134fc5 100644
--- a/test/cfg2cmd/q35-ide.conf.cmd
+++ b/test/cfg2cmd/q35-ide.conf.cmd
@@ -35,4 +35,6 @@
   -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=q35+pve0'
+  -machine 'type=q35+pve1' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1'
diff --git a/test/cfg2cmd/q35-linux-hostpci-mapping.conf.cmd b/test/cfg2cmd/q35-linux-hostpci-mapping.conf.cmd
index bc48c5ae..214689c6 100644
--- a/test/cfg2cmd/q35-linux-hostpci-mapping.conf.cmd
+++ b/test/cfg2cmd/q35-linux-hostpci-mapping.conf.cmd
@@ -33,4 +33,6 @@
   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=q35+pve0'
+  -machine 'type=q35+pve1' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1'
diff --git a/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
index 0b1d85af..72c7c5fc 100644
--- a/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
+++ b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
@@ -33,4 +33,6 @@
   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=q35+pve0'
+  -machine 'type=q35+pve1' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1'
diff --git a/test/cfg2cmd/q35-linux-hostpci-template.conf.cmd b/test/cfg2cmd/q35-linux-hostpci-template.conf.cmd
index cda10630..db2d8676 100644
--- a/test/cfg2cmd/q35-linux-hostpci-template.conf.cmd
+++ b/test/cfg2cmd/q35-linux-hostpci-template.conf.cmd
@@ -26,5 +26,7 @@
   -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \
   -drive 'file=/var/lib/vz/images/100/base-100-disk-2.raw,if=none,id=drive-scsi0,format=raw,cache=none,aio=io_uring,detect-zeroes=on,readonly=on' \
   -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0' \
-  -machine 'accel=tcg,type=pc+pve0' \
+  -machine 'accel=tcg,type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1' \
   -snapshot
diff --git a/test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf.cmd b/test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf.cmd
index c7698d17..0f0753cf 100644
--- a/test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf.cmd
+++ b/test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf.cmd
@@ -32,4 +32,6 @@
   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=q35+pve0'
+  -machine 'type=q35+pve1' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1'
diff --git a/test/cfg2cmd/q35-linux-hostpci.conf.cmd b/test/cfg2cmd/q35-linux-hostpci.conf.cmd
index 5289ec69..1c154a14 100644
--- a/test/cfg2cmd/q35-linux-hostpci.conf.cmd
+++ b/test/cfg2cmd/q35-linux-hostpci.conf.cmd
@@ -38,4 +38,6 @@
   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=q35+pve0'
+  -machine 'type=q35+pve1' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1'
diff --git a/test/cfg2cmd/q35-simple.conf.cmd b/test/cfg2cmd/q35-simple.conf.cmd
index 98b22f46..5bdae382 100644
--- a/test/cfg2cmd/q35-simple.conf.cmd
+++ b/test/cfg2cmd/q35-simple.conf.cmd
@@ -26,4 +26,6 @@
   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=q35+pve0'
+  -machine 'type=q35+pve1' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1'
diff --git a/test/cfg2cmd/q35-viommu-intel.conf.cmd b/test/cfg2cmd/q35-viommu-intel.conf.cmd
index 24e873d4..6d9a76e0 100644
--- a/test/cfg2cmd/q35-viommu-intel.conf.cmd
+++ b/test/cfg2cmd/q35-viommu-intel.conf.cmd
@@ -20,4 +20,6 @@
   -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' \
-  -machine 'type=q35+pve0,kernel-irqchip=split'
+  -machine 'type=q35+pve1,kernel-irqchip=split' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1'
diff --git a/test/cfg2cmd/q35-viommu-virtio.conf.cmd b/test/cfg2cmd/q35-viommu-virtio.conf.cmd
index 294c353d..72397d27 100644
--- a/test/cfg2cmd/q35-viommu-virtio.conf.cmd
+++ b/test/cfg2cmd/q35-viommu-virtio.conf.cmd
@@ -20,4 +20,6 @@
   -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 virtio-iommu-pci \
-  -machine 'type=q35+pve0'
+  -machine 'type=q35+pve1' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1'
diff --git a/test/cfg2cmd/seabios_serial.conf.cmd b/test/cfg2cmd/seabios_serial.conf.cmd
index 1c4e102c..e57551dc 100644
--- a/test/cfg2cmd/seabios_serial.conf.cmd
+++ b/test/cfg2cmd/seabios_serial.conf.cmd
@@ -30,4 +30,6 @@
   -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'smm=off,type=pc+pve0'
+  -machine 'smm=off,type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/simple-btrfs.conf.cmd b/test/cfg2cmd/simple-btrfs.conf.cmd
index c2354887..fff2eec2 100644
--- a/test/cfg2cmd/simple-btrfs.conf.cmd
+++ b/test/cfg2cmd/simple-btrfs.conf.cmd
@@ -30,4 +30,6 @@
   -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/simple-virtio-blk.conf.cmd b/test/cfg2cmd/simple-virtio-blk.conf.cmd
index d19aca6b..4a50531b 100644
--- a/test/cfg2cmd/simple-virtio-blk.conf.cmd
+++ b/test/cfg2cmd/simple-virtio-blk.conf.cmd
@@ -30,4 +30,6 @@
   -device 'virtio-blk-pci,drive=drive-virtio0,id=virtio0,bus=pci.0,addr=0xa,iothread=iothread-virtio0,bootindex=100' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/simple1-template.conf.cmd b/test/cfg2cmd/simple1-template.conf.cmd
index 35484600..9157cdd8 100644
--- a/test/cfg2cmd/simple1-template.conf.cmd
+++ b/test/cfg2cmd/simple1-template.conf.cmd
@@ -29,5 +29,7 @@
   -device 'ahci,id=ahci0,multifunction=on,bus=pci.0,addr=0x7' \
   -drive 'file=/var/lib/vz/images/8006/base-8006-disk-0.qcow2,if=none,id=drive-sata0,discard=on,format=qcow2,cache=none,aio=io_uring,detect-zeroes=unmap' \
   -device 'ide-hd,bus=ahci0.0,drive=drive-sata0,id=sata0' \
-  -machine 'accel=tcg,smm=off,type=pc+pve0' \
+  -machine 'accel=tcg,smm=off,type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1' \
   -snapshot
diff --git a/test/cfg2cmd/simple1.conf.cmd b/test/cfg2cmd/simple1.conf.cmd
index ecd14bcc..b52f91e7 100644
--- a/test/cfg2cmd/simple1.conf.cmd
+++ b/test/cfg2cmd/simple1.conf.cmd
@@ -30,4 +30,6 @@
   -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/vnc-clipboard-spice.conf.cmd b/test/cfg2cmd/vnc-clipboard-spice.conf.cmd
index f24cc7f0..23545dfa 100644
--- a/test/cfg2cmd/vnc-clipboard-spice.conf.cmd
+++ b/test/cfg2cmd/vnc-clipboard-spice.conf.cmd
@@ -24,4 +24,6 @@
   -spice 'tls-port=61000,addr=127.0.0.1,tls-ciphers=HIGH,seamless-migration=on' \
   -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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
diff --git a/test/cfg2cmd/vnc-clipboard-std.conf.cmd b/test/cfg2cmd/vnc-clipboard-std.conf.cmd
index c0c6cd28..8e2f4d89 100644
--- a/test/cfg2cmd/vnc-clipboard-std.conf.cmd
+++ b/test/cfg2cmd/vnc-clipboard-std.conf.cmd
@@ -24,4 +24,6 @@
   -device 'virtserialport,chardev=vdagent,name=com.redhat.spice.0' \
   -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' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve1' \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1'
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 8/8] tests: cfg2cmd: add test for windows machine pinning from meta info

[Dominik Csapak]

once with included pve machine and once without

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 test/cfg2cmd/q35-windows-pinning-pvever.conf  |  5 ++++
 .../q35-windows-pinning-pvever.conf.cmd       | 26 +++++++++++++++++++
 test/cfg2cmd/q35-windows-pinning.conf         |  5 ++++
 test/cfg2cmd/q35-windows-pinning.conf.cmd     | 24 +++++++++++++++++
 4 files changed, 60 insertions(+)
 create mode 100644 test/cfg2cmd/q35-windows-pinning-pvever.conf
 create mode 100644 test/cfg2cmd/q35-windows-pinning-pvever.conf.cmd
 create mode 100644 test/cfg2cmd/q35-windows-pinning.conf
 create mode 100644 test/cfg2cmd/q35-windows-pinning.conf.cmd

diff --git a/test/cfg2cmd/q35-windows-pinning-pvever.conf b/test/cfg2cmd/q35-windows-pinning-pvever.conf
new file mode 100644
index 00000000..45d87396
--- /dev/null
+++ b/test/cfg2cmd/q35-windows-pinning-pvever.conf
@@ -0,0 +1,5 @@
+# TEST: Config with q35, windows, meta (incl pvever) to test version pinning
+#
+machine: q35
+meta: creation-qemu=9.2.0,ctime=1741179133,creation-pve-machine=1
+ostype: win11
diff --git a/test/cfg2cmd/q35-windows-pinning-pvever.conf.cmd b/test/cfg2cmd/q35-windows-pinning-pvever.conf.cmd
new file mode 100644
index 00000000..06ae3f27
--- /dev/null
+++ b/test/cfg2cmd/q35-windows-pinning-pvever.conf.cmd
@@ -0,0 +1,26 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -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=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smp '1,sockets=1,cores=1,maxcpus=1' \
+  -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,hv_ipi,hv_relaxed,hv_reset,hv_runtime,hv_spinlocks=0x1fff,hv_stimer,hv_synic,hv_time,hv_vapic,hv_vpindex,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep' \
+  -m 512 \
+  -readconfig /usr/share/qemu-server/pve-q35-4.0.cfg \
+  -device 'usb-tablet,id=tablet,bus=ehci.0,port=1' \
+  -device 'VGA,id=vga,bus=pcie.0,addr=0x1,edid=off' \
+  -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' \
+  -rtc 'driftfix=slew,base=localtime' \
+  -machine 'hpet=off,type=pc-q35-9.2+pve1' \
+  -global 'kvm-pit.lost_tick_policy=discard' \
+  -global 'ICH9-LPC.disable_s3=1' \
+  -global 'ICH9-LPC.disable_s4=1'
diff --git a/test/cfg2cmd/q35-windows-pinning.conf b/test/cfg2cmd/q35-windows-pinning.conf
new file mode 100644
index 00000000..8bd47b0a
--- /dev/null
+++ b/test/cfg2cmd/q35-windows-pinning.conf
@@ -0,0 +1,5 @@
+# TEST: Config with q35, win, meta to test version pinning
+#
+machine: q35
+meta: creation-qemu=9.2.0,ctime=1741179133
+ostype: win11
diff --git a/test/cfg2cmd/q35-windows-pinning.conf.cmd b/test/cfg2cmd/q35-windows-pinning.conf.cmd
new file mode 100644
index 00000000..d8570b2c
--- /dev/null
+++ b/test/cfg2cmd/q35-windows-pinning.conf.cmd
@@ -0,0 +1,24 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -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=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smp '1,sockets=1,cores=1,maxcpus=1' \
+  -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,hv_ipi,hv_relaxed,hv_reset,hv_runtime,hv_spinlocks=0x1fff,hv_stimer,hv_synic,hv_time,hv_vapic,hv_vpindex,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep' \
+  -m 512 \
+  -readconfig /usr/share/qemu-server/pve-q35-4.0.cfg \
+  -device 'usb-tablet,id=tablet,bus=ehci.0,port=1' \
+  -device 'VGA,id=vga,bus=pcie.0,addr=0x1,edid=off' \
+  -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' \
+  -rtc 'driftfix=slew,base=localtime' \
+  -machine 'hpet=off,type=pc-q35-9.2+pve0' \
+  -global 'kvm-pit.lost_tick_policy=discard'
-- 
2.39.5



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

Re: [pve-devel] [RFC PATCH qemu-server 1/8] tests: cfg2cmd: pin QEMU version

[Fiona Ebner]

Am 06.03.25 um 11:44 schrieb Dominik Csapak:
> but warn when we're out of date compared to the installed one, and die
> when we're one major (+1 minor) release behind.
> (the warning is not very visible when running tests or when building)
> 
> We don't want to depend on the installed QEMU version for such tests,
> otherwise a developer might need to adapt tests because the installed
> QEMU version is different to what is e.g. in the build environment for
> our packaging.
> 
> Also, this way, we have to show intent for bumping this and it will be
> obvious that we need to adapt tests because of a changed QEMU version.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  test/run_config2command_tests.pl | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
> index 7e3d10e6..5fa6f2de 100755
> --- a/test/run_config2command_tests.pl
> +++ b/test/run_config2command_tests.pl
> @@ -20,6 +20,10 @@ use PVE::QemuServer::Monitor;
>  use PVE::QemuServer::QMPHelpers;
>  use PVE::QemuServer::CPUConfig;
>  
> +# bump when QEMU version changes
> +my $tested_version_major = 9;
> +my $tested_version_minor = 2;
> +
>  my $base_env = {
>      storage_config => {
>  	ids => {
> @@ -80,6 +84,7 @@ my $base_env = {
>  	}
>      },
>      vmid => 8006,
> +    tested_qemu_version => "$tested_version_major.$tested_version_minor",
>      real_qemu_version => PVE::QemuServer::Helpers::kvm_user_version(), # not yet mocked

I'd not keep the real_qemu_version in base_env anymore, but have it as a
stand-alone variable for the version comparison check.

>  };
>  
> @@ -184,7 +189,7 @@ sub parse_test($) {
>  }
>  
>  sub get_test_qemu_version {
> -    $current_test->{qemu_version} // $base_env->{real_qemu_version} // '2.12';
> +    $current_test->{qemu_version} // $base_env->{tested_qemu_version} // '2.12';
>  }
>  
>  my $qemu_server_module;
> @@ -528,3 +533,19 @@ if (my $file = shift) {
>  }
>  
>  done_testing();

Nit: Since the check below can die, I'd put it at the very beginning
rather than at the end.

> +
> +# reset warn

IMHO "Why is the reset needed?" is the interesting part worth commenting
here ;)

> +$SIG{__WARN__} = undef;
> +if ($base_env->{real_qemu_version} =~ m/^(\d+.\d+)/) {
> +	if (PVE::QemuServer::Helpers::version_cmp($1, $tested_version_major, $2, $tested_version_minor) < 0) {
> +	    warn "\nWARNING: installed QEMU version bigger than tested one, please bump!\n";
> +	}
> +
> +	# if we did not bump since the last major QEMU (+1 minor) release fail the test
> +	if (PVE::QemuServer::Helpers::version_cmp($1, $tested_version_major + 1, $2, 1) >= 0) {

I'd rather have a fixed difference between versions trigger the die.
Your check behaves the same when the tested version is 10.0 and when the
tested version is 10.2. In both cases, having the real version 11.1 will
trigger the die.

> +	    die "\nERROR: installed QEMU version one major release bigger than tested one, please bump!\n";
> +	}
> +
> +} else {
> +    die "\nERROR: invalid QEMU version\n";
> +}



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

Re: [pve-devel] [RFC PATCH qemu-server 1/8] tests: cfg2cmd: pin QEMU version

[Dominik Csapak]

On 3/6/25 13:00, Fiona Ebner wrote:
> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>> but warn when we're out of date compared to the installed one, and die
>> when we're one major (+1 minor) release behind.
>> (the warning is not very visible when running tests or when building)
>>
>> We don't want to depend on the installed QEMU version for such tests,
>> otherwise a developer might need to adapt tests because the installed
>> QEMU version is different to what is e.g. in the build environment for
>> our packaging.
>>
>> Also, this way, we have to show intent for bumping this and it will be
>> obvious that we need to adapt tests because of a changed QEMU version.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   test/run_config2command_tests.pl | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
>> index 7e3d10e6..5fa6f2de 100755
>> --- a/test/run_config2command_tests.pl
>> +++ b/test/run_config2command_tests.pl
>> @@ -20,6 +20,10 @@ use PVE::QemuServer::Monitor;
>>   use PVE::QemuServer::QMPHelpers;
>>   use PVE::QemuServer::CPUConfig;
>>   
>> +# bump when QEMU version changes
>> +my $tested_version_major = 9;
>> +my $tested_version_minor = 2;
>> +
>>   my $base_env = {
>>       storage_config => {
>>   	ids => {
>> @@ -80,6 +84,7 @@ my $base_env = {
>>   	}
>>       },
>>       vmid => 8006,
>> +    tested_qemu_version => "$tested_version_major.$tested_version_minor",
>>       real_qemu_version => PVE::QemuServer::Helpers::kvm_user_version(), # not yet mocked
> 
> I'd not keep the real_qemu_version in base_env anymore, but have it as a
> stand-alone variable for the version comparison check.

makes sense

> 
>>   };
>>   
>> @@ -184,7 +189,7 @@ sub parse_test($) {
>>   }
>>   
>>   sub get_test_qemu_version {
>> -    $current_test->{qemu_version} // $base_env->{real_qemu_version} // '2.12';
>> +    $current_test->{qemu_version} // $base_env->{tested_qemu_version} // '2.12';
>>   }
>>   
>>   my $qemu_server_module;
>> @@ -528,3 +533,19 @@ if (my $file = shift) {
>>   }
>>   
>>   done_testing();
> 
> Nit: Since the check below can die, I'd put it at the very beginning
> rather than at the end.

mhmm ok, the rationale was that when letting the tests run manually,
the warning can be seen when it's at the end, but if it's printed at the
start, it'll most likely be overlooked...

i played around with requiring input from the user to continue,
but that didn't work at all when building (and is probably not
a good idea in general when running tests..)

Not sure how we can make the warning more visible while not failing the
tests at the same time...

> 
>> +
>> +# reset warn
> 
> IMHO "Why is the reset needed?" is the interesting part worth commenting
> here ;)

true ;), we override the warn handler above to check the warning for the expected
ones, we could of course add some code there to handle this case here,
but it seemed shorter/more elegant to just reset the handler completely
when we're done with testing...

> 
>> +$SIG{__WARN__} = undef;
>> +if ($base_env->{real_qemu_version} =~ m/^(\d+.\d+)/) {
>> +	if (PVE::QemuServer::Helpers::version_cmp($1, $tested_version_major, $2, $tested_version_minor) < 0) {
>> +	    warn "\nWARNING: installed QEMU version bigger than tested one, please bump!\n";
>> +	}
>> +
>> +	# if we did not bump since the last major QEMU (+1 minor) release fail the test
>> +	if (PVE::QemuServer::Helpers::version_cmp($1, $tested_version_major + 1, $2, 1) >= 0) {
> 
> I'd rather have a fixed difference between versions trigger the die.
> Your check behaves the same when the tested version is 10.0 and when the
> tested version is 10.2. In both cases, having the real version 11.1 will
> trigger the die.
> 

not exactly sure what you mean here:

IIUC, in the scenario where we pinned X.Y, you want the test to fail when the real version is:

X+1.Y ?

or when

X+1.Y+1 ?

or when X+1.Y-1 ?

my thought here was that we want to update at least once per major release (not sure
if that means anything for qemu though), but leave a wiggle room until the first point release

>> +	    die "\nERROR: installed QEMU version one major release bigger than tested one, please bump!\n";
>> +	}
>> +
>> +} else {
>> +    die "\nERROR: invalid QEMU version\n";
>> +}
> 



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

Re: [pve-devel] [PATCH qemu-server 2/8] config to command: add one '-global' option for each flag

[Fiona Ebner]

Am 06.03.25 um 11:44 schrieb Dominik Csapak:
> If we have multiple 'globalFlags', we have to encode each one separately
> on the commandline with '-global OPTION', since QEMU does not allow to
> have multiple options here.
> 
> We currently only have one such flag that used the 'globalFlags' list,
> so it never popped up. (All other uses directly add an option to the
> commandline)
> 
> Avoid future bugs by fixing it now.
> 

So there is no real point to collecting the flags in the first place?
I.e. we could also get rid of the variable and have the single current
user of the variable add the flag directly on the commandline too. Or
otherwise, we could change the other users and collect all flags with
this variable. Pre-existing of course, but ideally, we could avoid the
mishmash.

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> ---
>  PVE/QemuServer.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index b6fc1f17..13af495d 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4001,7 +4001,9 @@ sub config_to_command {
>      push @$cmd, @$devices;
>      push @$cmd, '-rtc', join(',', @$rtcFlags) if scalar(@$rtcFlags);
>      push @$cmd, '-machine', join(',', @$machineFlags) if scalar(@$machineFlags);
> -    push @$cmd, '-global', join(',', @$globalFlags) if scalar(@$globalFlags);
> +    for my $flag ($globalFlags->@*) {
> +	push @$cmd, '-global', $flag;
> +    }
>  
>      if (my $vmstate = $conf->{vmstate}) {
>  	my $statepath = PVE::Storage::path($storecfg, $vmstate);



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

Re: [pve-devel] [PATCH qemu-server 2/8] config to command: add one '-global' option for each flag

[Dominik Csapak]

On 3/6/25 13:13, Fiona Ebner wrote:
> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>> If we have multiple 'globalFlags', we have to encode each one separately
>> on the commandline with '-global OPTION', since QEMU does not allow to
>> have multiple options here.
>>
>> We currently only have one such flag that used the 'globalFlags' list,
>> so it never popped up. (All other uses directly add an option to the
>> commandline)
>>
>> Avoid future bugs by fixing it now.
>>
> 
> So there is no real point to collecting the flags in the first place?
> I.e. we could also get rid of the variable and have the single current
> user of the variable add the flag directly on the commandline too. Or
> otherwise, we could change the other users and collect all flags with
> this variable. Pre-existing of course, but ideally, we could avoid the
> mishmash.
> 

Sorry this could have been more clear here:
I add to the flags in one of the following patches, so i sent this
in preparation of that (could possibly be squashed)

I did not want to touch the other places, since that in turn changes
the order of the qemu commandline (which sometimes has unintended side
effects, e.g. in combination with the 'args' parameter)

>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> 
> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> 
>> ---
>>   PVE/QemuServer.pm | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index b6fc1f17..13af495d 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -4001,7 +4001,9 @@ sub config_to_command {
>>       push @$cmd, @$devices;
>>       push @$cmd, '-rtc', join(',', @$rtcFlags) if scalar(@$rtcFlags);
>>       push @$cmd, '-machine', join(',', @$machineFlags) if scalar(@$machineFlags);
>> -    push @$cmd, '-global', join(',', @$globalFlags) if scalar(@$globalFlags);
>> +    for my $flag ($globalFlags->@*) {
>> +	push @$cmd, '-global', $flag;
>> +    }
>>   
>>       if (my $vmstate = $conf->{vmstate}) {
>>   	my $statepath = PVE::Storage::path($storecfg, $vmstate);
> 



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

Re: [pve-devel] [RFC PATCH qemu-server 1/8] tests: cfg2cmd: pin QEMU version

[Fiona Ebner]

Am 06.03.25 um 13:07 schrieb Dominik Csapak:
> On 3/6/25 13:00, Fiona Ebner wrote:
>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>> @@ -528,3 +533,19 @@ if (my $file = shift) {
>>>   }
>>>     done_testing();
>>
>> Nit: Since the check below can die, I'd put it at the very beginning
>> rather than at the end.
> 
> mhmm ok, the rationale was that when letting the tests run manually,
> the warning can be seen when it's at the end, but if it's printed at the
> start, it'll most likely be overlooked...
> 
> i played around with requiring input from the user to continue,
> but that didn't work at all when building (and is probably not
> a good idea in general when running tests..)
> 
> Not sure how we can make the warning more visible while not failing the
> tests at the same time...
Thinking again about the whole pinning, I'm not sure we really want to.
If there are changes based on the binary version (rare), I do want to
have the actual tests be run immediately. Tests should not change
between binary versions without intent. The exception is here, with the
bumped pve version. And I don't mean the bumping itself, that has intent
and is fine. If we bump the pve version, that QEMU version is already
public (or we could've still made the change based on the QEMU version
rather than pve version ;)). The actual issue is with QEMU 10.0 where we
will need to adapt the tests for un-bumping the pve version again. That
does not have intent and that will temporarily break build.

Maybe we can still use the installed version for running tests by
default. We can add an environment variable or similar for overriding
the test version. Additionally, record the expected version in the test
script and die if it doesn't match the installed binary, suggesting to
use the override.

Then the package can still be built by setting the environment variable
as an escape hatch. While usual builds will use and test the current
version that will be running on people's systems.

> 
>>
>>> +
>>> +# reset warn
>>
>> IMHO "Why is the reset needed?" is the interesting part worth commenting
>> here ;)
> 
> true ;), we override the warn handler above to check the warning for the
> expected
> ones, we could of course add some code there to handle this case here,
> but it seemed shorter/more elegant to just reset the handler completely
> when we're done with testing...
> 
>>
>>> +$SIG{__WARN__} = undef;
>>> +if ($base_env->{real_qemu_version} =~ m/^(\d+.\d+)/) {
>>> +    if (PVE::QemuServer::Helpers::version_cmp($1,
>>> $tested_version_major, $2, $tested_version_minor) < 0) {
>>> +        warn "\nWARNING: installed QEMU version bigger than tested
>>> one, please bump!\n";
>>> +    }
>>> +
>>> +    # if we did not bump since the last major QEMU (+1 minor)
>>> release fail the test
>>> +    if (PVE::QemuServer::Helpers::version_cmp($1,
>>> $tested_version_major + 1, $2, 1) >= 0) {
>>
>> I'd rather have a fixed difference between versions trigger the die.
>> Your check behaves the same when the tested version is 10.0 and when the
>> tested version is 10.2. In both cases, having the real version 11.1 will
>> trigger the die.
>>
> 
> not exactly sure what you mean here:
> 
> IIUC, in the scenario where we pinned X.Y, you want the test to fail
> when the real version is:
> 
> X+1.Y ?

Yes, for example. I don't think we should wait longer.

> 
> or when
> 
> X+1.Y+1 ?

That release might not exist, e.g. 10.3 will never exist. Of course you
can make what you want work by distinguishing based on major version
difference.

> 
> or when X+1.Y-1 ?

Similar here.

> 
> my thought here was that we want to update at least once per major
> release (not sure
> if that means anything for qemu though), but leave a wiggle room until
> the first point release

Yes, but I'd prefer being reminded after a fixed difference.


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

Re: [pve-devel] [PATCH qemu-server 2/8] config to command: add one '-global' option for each flag

[Fiona Ebner]

Am 06.03.25 um 13:15 schrieb Dominik Csapak:
> On 3/6/25 13:13, Fiona Ebner wrote:
>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>> If we have multiple 'globalFlags', we have to encode each one separately
>>> on the commandline with '-global OPTION', since QEMU does not allow to
>>> have multiple options here.
>>>
>>> We currently only have one such flag that used the 'globalFlags' list,
>>> so it never popped up. (All other uses directly add an option to the
>>> commandline)
>>>
>>> Avoid future bugs by fixing it now.
>>>
>>
>> So there is no real point to collecting the flags in the first place?
>> I.e. we could also get rid of the variable and have the single current
>> user of the variable add the flag directly on the commandline too. Or
>> otherwise, we could change the other users and collect all flags with
>> this variable. Pre-existing of course, but ideally, we could avoid the
>> mishmash.
>>
> 
> Sorry this could have been more clear here:
> I add to the flags in one of the following patches, so i sent this
> in preparation of that (could possibly be squashed)

Yes, I understand that. I still think the status quo with mixing two
different approaches might not be best. It's not going to be a blocker
for the series, but I wanted to mention it, if you want to go for
avoiding it.

> I did not want to touch the other places, since that in turn changes
> the order of the qemu commandline (which sometimes has unintended side
> effects, e.g. in combination with the 'args' parameter)

Are you sure? Custom 'args' are always added last so that shouldn't matter.

The only thing that would change by removing the global flags variable
is having "-global kvm-pit.lost_tick_policy=discard" earlier in the
commandline. I think that should be fine. In particular QEMU's
qemu_init() function has a call to user_register_global_props() which
handles all global properties at the same time, so I think changing the
order should be fine in (almost?) all cases.


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

Re: [pve-devel] [PATCH qemu-server 4/8] machine: correctly select pve machine version for non pinned windows guests

[Fiona Ebner]

Am 06.03.25 um 11:44 schrieb Dominik Csapak:
> when we don't have a specific machine version on a windows guest, we use
> the creation meta info to pin the machine version. Currently we always
> append the pve machine version from the current installed kvm version,
> which is not necessarily the version we pinned the guest to.
> 
> Instead, use either the info from the creation meta info if it exists,
> or use 'pve0'.
> 
> For non-windows machines, we used the current QEMU machine version so we
> should use the pve machine version from that too.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/QemuServer/Machine.pm | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
> index f1acde8f..e3da8e21 100644
> --- a/PVE/QemuServer/Machine.pm
> +++ b/PVE/QemuServer/Machine.pm
> @@ -237,14 +237,19 @@ sub get_vm_machine {
>  		if (PVE::QemuServer::Helpers::min_version($meta->{'creation-qemu'}, 9, 1)) {
>  		    # need only major.minor
>  		    ($base_version) = ($meta->{'creation-qemu'} =~ m/^(\d+.\d+)/);
> +		    # append pve machine version if we have one
> +		    if (my $pvever = $meta->{'creation-pve-machine'}) {
> +			$base_version .= "+pve$pvever"

Since this is only the fallback handling for the rare edge case where no
explicit machine version is set for a Windows guest, not sure if it's
even worth doing this. I.e. can we just avoid the additional meta
property and always use pve0 here? Did you intend any other use for the
creation-pve-machine?

Further below we already have:

> 	# for version-pinned machines that do not include a pve-version (e.g.
> 	# pc-q35-4.1), we assume 0 to keep them stable in case we bump
> 	$machine .= '+pve0';

so let's just follow this here too?


> +		    }
>  		}
>  	    }
>  	    $machine = windows_get_pinned_machine_version($machine, $base_version, $kvmversion);
> +	} else{
> +	    $arch //= 'x86_64';
> +	    $machine ||= default_machine_for_arch($arch);
> +	    my $pvever = get_pve_version($kvmversion);
> +	    $machine .= "+pve$pvever";
>  	}
> -	$arch //= 'x86_64';
> -	$machine ||= default_machine_for_arch($arch);
> -	my $pvever = get_pve_version($kvmversion);
> -	$machine .= "+pve$pvever";
>      }
>  
>      if ($machine !~ m/\+pve\d+?(?:\.pxe)?$/) {



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

Re: [pve-devel] [PATCH qemu-server 4/8] machine: correctly select pve machine version for non pinned windows guests

[Dominik Csapak]

On 3/6/25 14:10, Fiona Ebner wrote:
> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>> when we don't have a specific machine version on a windows guest, we use
>> the creation meta info to pin the machine version. Currently we always
>> append the pve machine version from the current installed kvm version,
>> which is not necessarily the version we pinned the guest to.
>>
>> Instead, use either the info from the creation meta info if it exists,
>> or use 'pve0'.
>>
>> For non-windows machines, we used the current QEMU machine version so we
>> should use the pve machine version from that too.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/QemuServer/Machine.pm | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>> index f1acde8f..e3da8e21 100644
>> --- a/PVE/QemuServer/Machine.pm
>> +++ b/PVE/QemuServer/Machine.pm
>> @@ -237,14 +237,19 @@ sub get_vm_machine {
>>   		if (PVE::QemuServer::Helpers::min_version($meta->{'creation-qemu'}, 9, 1)) {
>>   		    # need only major.minor
>>   		    ($base_version) = ($meta->{'creation-qemu'} =~ m/^(\d+.\d+)/);
>> +		    # append pve machine version if we have one
>> +		    if (my $pvever = $meta->{'creation-pve-machine'}) {
>> +			$base_version .= "+pve$pvever"
> 
> Since this is only the fallback handling for the rare edge case where no
> explicit machine version is set for a Windows guest, not sure if it's
> even worth doing this. I.e. can we just avoid the additional meta
> property and always use pve0 here? Did you intend any other use for the
> creation-pve-machine?
> 

I though the intention of the code was to pin the guest to that version where it
was created. If we omit the machine version, this is not true anymore,
since I'd then create a windows vm with e.g. pc-q35-9.2+pve1 then set it
to q35, and would get pc-q35-9.2+pve0

and while i don't have anymore use cases for the current case, wouldn't
this approach here correct also when we decide to bump again in the future?

also recording with which pve version the vm was created could help in debugging
issues at some point.. (my patch only adds the version in the meta info if it's > 0 anyway)

> Further below we already have:
> 
>> 	# for version-pinned machines that do not include a pve-version (e.g.
>> 	# pc-q35-4.1), we assume 0 to keep them stable in case we bump
>> 	$machine .= '+pve0';
> 
> so let's just follow this here too?

this here make sense IMHO, since the user wanted a specific version but without
pve versions info, so default to pve0 sounds fine to me?

> 
> 
>> +		    }
>>   		}
>>   	    }
>>   	    $machine = windows_get_pinned_machine_version($machine, $base_version, $kvmversion);
>> +	} else{
>> +	    $arch //= 'x86_64';
>> +	    $machine ||= default_machine_for_arch($arch);
>> +	    my $pvever = get_pve_version($kvmversion);
>> +	    $machine .= "+pve$pvever";
>>   	}
>> -	$arch //= 'x86_64';
>> -	$machine ||= default_machine_for_arch($arch);
>> -	my $pvever = get_pve_version($kvmversion);
>> -	$machine .= "+pve$pvever";
>>       }
>>   
>>       if ($machine !~ m/\+pve\d+?(?:\.pxe)?$/) {
> 



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

Re: [pve-devel] [PATCH qemu-server 4/8] machine: correctly select pve machine version for non pinned windows guests

[Fiona Ebner]

Am 06.03.25 um 14:36 schrieb Dominik Csapak:
> On 3/6/25 14:10, Fiona Ebner wrote:
>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>>> index f1acde8f..e3da8e21 100644
>>> --- a/PVE/QemuServer/Machine.pm
>>> +++ b/PVE/QemuServer/Machine.pm
>>> @@ -237,14 +237,19 @@ sub get_vm_machine {
>>>           if (PVE::QemuServer::Helpers::min_version($meta-
>>> >{'creation-qemu'}, 9, 1)) {
>>>               # need only major.minor
>>>               ($base_version) = ($meta->{'creation-qemu'} =~ m/^(\d+.
>>> \d+)/);
>>> +            # append pve machine version if we have one
>>> +            if (my $pvever = $meta->{'creation-pve-machine'}) {
>>> +            $base_version .= "+pve$pvever"
>>
>> Since this is only the fallback handling for the rare edge case where no
>> explicit machine version is set for a Windows guest, not sure if it's
>> even worth doing this. I.e. can we just avoid the additional meta
>> property and always use pve0 here? Did you intend any other use for the
>> creation-pve-machine?
>>
> 
> I though the intention of the code was to pin the guest to that version
> where it
> was created. If we omit the machine version, this is not true anymore,
> since I'd then create a windows vm with e.g. pc-q35-9.2+pve1 then set it
> to q35, and would get pc-q35-9.2+pve0

Fair enough. There might be people manually changing windows guests to
use the latest machine even if we don't recommend it or allow it in the
UI. Still, it would just mean a pve machine version downgrade for
subsequent boots, which also can happen if you offline migrate to a node
that does not yet support the newest pve machine version.

> 
> and while i don't have anymore use cases for the current case, wouldn't
> this approach here correct also when we decide to bump again in the future?

Correct what?

> 
> also recording with which pve version the vm was created could help in
> debugging
> issues at some point.. (my patch only adds the version in the meta info
> if it's > 0 anyway)
> 

In principle, yes. But honestly, it's much less informative than the
creation QEMU version, and even that is rather rarely useful for
debugging in my experience.

So not a huge fan, since I'd find it nicer to keep the meta info slick,
but we can go for it if you really think it's worth it.


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

Re: [pve-devel] [PATCH qemu-server 4/8] machine: correctly select pve machine version for non pinned windows guests

[Fiona Ebner]

Am 06.03.25 um 15:10 schrieb Fiona Ebner:
> Am 06.03.25 um 14:36 schrieb Dominik Csapak:
>> On 3/6/25 14:10, Fiona Ebner wrote:
>>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>>>> index f1acde8f..e3da8e21 100644
>>>> --- a/PVE/QemuServer/Machine.pm
>>>> +++ b/PVE/QemuServer/Machine.pm
>>>> @@ -237,14 +237,19 @@ sub get_vm_machine {
>>>>           if (PVE::QemuServer::Helpers::min_version($meta-
>>>>> {'creation-qemu'}, 9, 1)) {
>>>>               # need only major.minor
>>>>               ($base_version) = ($meta->{'creation-qemu'} =~ m/^(\d+.
>>>> \d+)/);
>>>> +            # append pve machine version if we have one
>>>> +            if (my $pvever = $meta->{'creation-pve-machine'}) {
>>>> +            $base_version .= "+pve$pvever"
>>>
>>> Since this is only the fallback handling for the rare edge case where no
>>> explicit machine version is set for a Windows guest, not sure if it's
>>> even worth doing this. I.e. can we just avoid the additional meta
>>> property and always use pve0 here? Did you intend any other use for the
>>> creation-pve-machine?
>>>
>>
>> I though the intention of the code was to pin the guest to that version
>> where it
>> was created. If we omit the machine version, this is not true anymore,
>> since I'd then create a windows vm with e.g. pc-q35-9.2+pve1 then set it
>> to q35, and would get pc-q35-9.2+pve0
> 
> Fair enough. There might be people manually changing windows guests to
> use the latest machine even if we don't recommend it or allow it in the
> UI. Still, it would just mean a pve machine version downgrade for
> subsequent boots, which also can happen if you offline migrate to a node
> that does not yet support the newest pve machine version.

No wait, if you set it to q35 it will mean "use the fallback" not "use
the latest". So that is not even a valid use case I had in mind. And we
can define the fallback to be just with pve0 always.


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

Re: [pve-devel] [PATCH qemu-server 4/8] machine: correctly select pve machine version for non pinned windows guests

[Dominik Csapak]

On 3/6/25 15:10, Fiona Ebner wrote:
> Am 06.03.25 um 14:36 schrieb Dominik Csapak:
>> On 3/6/25 14:10, Fiona Ebner wrote:
>>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>>>> index f1acde8f..e3da8e21 100644
>>>> --- a/PVE/QemuServer/Machine.pm
>>>> +++ b/PVE/QemuServer/Machine.pm
>>>> @@ -237,14 +237,19 @@ sub get_vm_machine {
>>>>            if (PVE::QemuServer::Helpers::min_version($meta-
>>>>> {'creation-qemu'}, 9, 1)) {
>>>>                # need only major.minor
>>>>                ($base_version) = ($meta->{'creation-qemu'} =~ m/^(\d+.
>>>> \d+)/);
>>>> +            # append pve machine version if we have one
>>>> +            if (my $pvever = $meta->{'creation-pve-machine'}) {
>>>> +            $base_version .= "+pve$pvever"
>>>
>>> Since this is only the fallback handling for the rare edge case where no
>>> explicit machine version is set for a Windows guest, not sure if it's
>>> even worth doing this. I.e. can we just avoid the additional meta
>>> property and always use pve0 here? Did you intend any other use for the
>>> creation-pve-machine?
>>>
>>
>> I though the intention of the code was to pin the guest to that version
>> where it
>> was created. If we omit the machine version, this is not true anymore,
>> since I'd then create a windows vm with e.g. pc-q35-9.2+pve1 then set it
>> to q35, and would get pc-q35-9.2+pve0
> 
> Fair enough. There might be people manually changing windows guests to
> use the latest machine even if we don't recommend it or allow it in the
> UI. Still, it would just mean a pve machine version downgrade for
> subsequent boots, which also can happen if you offline migrate to a node
> that does not yet support the newest pve machine version.
> 
>>
>> and while i don't have anymore use cases for the current case, wouldn't
>> this approach here correct also when we decide to bump again in the future?
> 
> Correct what?
> 

i wanted to write 'wouldn't this approach here be correct [..]'



>>
>> also recording with which pve version the vm was created could help in
>> debugging
>> issues at some point.. (my patch only adds the version in the meta info
>> if it's > 0 anyway)
>>
> 
> In principle, yes. But honestly, it's much less informative than the
> creation QEMU version, and even that is rather rarely useful for
> debugging in my experience.
> 
> So not a huge fan, since I'd find it nicer to keep the meta info slick,
> but we can go for it if you really think it's worth it.


mhmm i get what you mean, would it be an ok compromise to record the pve version with
the creation machine maybe?

i'd have to touch the places where we read that ofc but that shouldn't be too many


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

Re: [pve-devel] [PATCH qemu-server 4/8] machine: correctly select pve machine version for non pinned windows guests

[Fiona Ebner]

Am 06.03.25 um 15:15 schrieb Dominik Csapak:
> On 3/6/25 15:10, Fiona Ebner wrote:
>> Am 06.03.25 um 14:36 schrieb Dominik Csapak:
>>> On 3/6/25 14:10, Fiona Ebner wrote:
>>>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>>>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>>>>> index f1acde8f..e3da8e21 100644
>>>>> --- a/PVE/QemuServer/Machine.pm
>>>>> +++ b/PVE/QemuServer/Machine.pm
>>>>> @@ -237,14 +237,19 @@ sub get_vm_machine {
>>>>>            if (PVE::QemuServer::Helpers::min_version($meta-
>>>>>> {'creation-qemu'}, 9, 1)) {
>>>>>                # need only major.minor
>>>>>                ($base_version) = ($meta->{'creation-qemu'} =~ m/^(\d+.
>>>>> \d+)/);
>>>>> +            # append pve machine version if we have one
>>>>> +            if (my $pvever = $meta->{'creation-pve-machine'}) {
>>>>> +            $base_version .= "+pve$pvever"
>>>>
>>>> Since this is only the fallback handling for the rare edge case
>>>> where no
>>>> explicit machine version is set for a Windows guest, not sure if it's
>>>> even worth doing this. I.e. can we just avoid the additional meta
>>>> property and always use pve0 here? Did you intend any other use for the
>>>> creation-pve-machine?
>>>>
>>>
>>> I though the intention of the code was to pin the guest to that version
>>> where it
>>> was created. If we omit the machine version, this is not true anymore,
>>> since I'd then create a windows vm with e.g. pc-q35-9.2+pve1 then set it
>>> to q35, and would get pc-q35-9.2+pve0
>>
>> Fair enough. There might be people manually changing windows guests to
>> use the latest machine even if we don't recommend it or allow it in the
>> UI. Still, it would just mean a pve machine version downgrade for
>> subsequent boots, which also can happen if you offline migrate to a node
>> that does not yet support the newest pve machine version.
>>
>>>
>>> and while i don't have anymore use cases for the current case, wouldn't
>>> this approach here correct also when we decide to bump again in the
>>> future?
>>
>> Correct what?
>>
> 
> i wanted to write 'wouldn't this approach here be correct [..]'

I never said it's not correct. Just questioning if it's worth it.

>>> also recording with which pve version the vm was created could help in
>>> debugging
>>> issues at some point.. (my patch only adds the version in the meta info
>>> if it's > 0 anyway)
>>>
>>
>> In principle, yes. But honestly, it's much less informative than the
>> creation QEMU version, and even that is rather rarely useful for
>> debugging in my experience.
>>
>> So not a huge fan, since I'd find it nicer to keep the meta info slick,
>> but we can go for it if you really think it's worth it.
> 
> 
> mhmm i get what you mean, would it be an ok compromise to record the pve
> version with
> the creation machine maybe?
> 
> i'd have to touch the places where we read that ofc but that shouldn't
> be too many
> 

The creation-qemu is the binary version, but the latest pve version is
related to the qemu-server version/machine version, so while I get where
you're coming from, I'd rather keep them separate.


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

Re: [pve-devel] [PATCH qemu-server 5/8] machine: incorporate pve machine version when pinning windows guests

[Fiona Ebner]

Am 06.03.25 um 11:44 schrieb Dominik Csapak:
> When creating or updating guests with ostype windows, we want to pin the
> machine version to a specific one. Since introduction of that feature,
> we never bumped the pve machine version, so this was missing.
> 
> Append the pve machine version if it's not 0 so we don't add that
> unnecessarily.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/QemuServer/Machine.pm | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
> index e3da8e21..ebaf2dcc 100644
> --- a/PVE/QemuServer/Machine.pm
> +++ b/PVE/QemuServer/Machine.pm
> @@ -274,7 +274,17 @@ sub check_and_pin_machine_string {
>      if (!$machine || $machine =~ m/^(?:pc|q35|virt)$/) {
>  	# always pin Windows' machine version on create, they get confused too easily
>  	if (PVE::QemuServer::Helpers::windows_version($ostype)) {
> -	    $machine_conf->{type} = windows_get_pinned_machine_version($machine);
> +	    my $kvmversion = PVE::QemuServer::Helpers::kvm_user_version();
> +	    my $pin_version = get_installed_machine_version($kvmversion);

Nit: I'd call this $base_version like the argument it's passed-in as.
It's not necessarily the version that is going to be pinned.

> +
> +	    # pin to the current pveX version to make use of most current features if > 0
> +	    my $pvever = get_pve_version($kvmversion);
> +	    if ($pvever > 0) {
> +		$pin_version .= "+pve$pvever";
> +	    }

I feel this logic should go into windows_get_pinned_machine_version()
itself.

> +
> +	    $machine_conf->{type} = windows_get_pinned_machine_version($machine, $pin_version, $kvmversion);
> +
>  	    print "pinning machine type to '$machine_conf->{type}' for Windows guest OS\n";
>  	}
>      }



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

Re: [pve-devel] [PATCH qemu-server 6/8] machine: add S3/S4 power state properties

[Fiona Ebner]

Am 06.03.25 um 11:44 schrieb Dominik Csapak:
> So users can disable them (they're enabled by default in QEMU)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> This patch may make sense, regardless if we'll apply the reversal of the
> default...
> 
>  PVE/QemuServer.pm         |  2 ++
>  PVE/QemuServer/Machine.pm | 40 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 13af495d..b8ce4750 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3975,6 +3975,8 @@ sub config_to_command {
>  	push @$machineFlags, 'accel=tcg';
>      }
>  
> +    PVE::QemuServer::Machine::check_and_set_power_state_flags($globalFlags, $machine_conf);
> +
>      push @$machineFlags, 'smm=off' if should_disable_smm($conf, $vga, $machine_type);
>  
>      my $machine_type_min = $machine_type;
> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
> index ebaf2dcc..377abc8a 100644
> --- a/PVE/QemuServer/Machine.pm
> +++ b/PVE/QemuServer/Machine.pm
> @@ -31,6 +31,16 @@ my $machine_fmt = {
>  	enum => ['intel', 'virtio'],
>  	optional => 1,
>      },
> +    'enable-s3' => {
> +	type => 'boolean',
> +	description => "Enables S3 power state. Defaults to true.",
> +	optional => 1,
> +    },
> +    'enable-s4' => {
> +	type => 'boolean',
> +	description => "Enables S4 power state. Defaults to true.",
> +	optional => 1,
> +    },
>  };
>  
>  PVE::JSONSchema::register_format('pve-qemu-machine-fmt', $machine_fmt);

Note that an UI patch is needed, because editing the machine there will
not preserve these values currently.

> @@ -293,4 +303,34 @@ sub check_and_pin_machine_string {
>      return print_machine($machine_conf);
>  }
>  
> +# disable s3/s4 by default for 9.2+pve1 machine types
> +sub check_and_set_power_state_flags {
> +    my ($globalFlags, $machine_conf) = @_;
> +
> +    my $get_flag = sub {
> +	my ($q35, $type, $value) = @_;

Style nit: $value is always 1

> +
> +	if ($q35) {
> +	    return "ICH9-LPC.disable_${type}=${value}";
> +	} else {
> +	    return "PIIX4_PM.disable_${type}=${value}";
> +	}
> +    };

Rather than this closure, could just be $object = $q35 ? 'ICH9-LPC' :
'PIIX4_PM' and then use that for constructing the string when pusing.

> +
> +    my $q35 = $machine_conf->{type} && ($machine_conf->{type} =~ m/q35/) ? 1 : 0;
> +
> +    my $s3 = $machine_conf->{'enable-s3'} // 1;
> +    my $s4 = $machine_conf->{'enable-s4'} // 1;
> +
> +    # they're enabled by default in QEMU, so only add the flags to disable them
> +    if (!$s3) {
> +	push $globalFlags->@*, $get_flag->($q35, 's3', 1)

Style nit: missing semicolon

> +    }
> +    if (!$s4) {
> +	push $globalFlags->@*, $get_flag->($q35, 's4', 1)

Style nit: missing semicolon

> +    }
> +
> +    return;
> +}
> +
>  1;



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

Re: [pve-devel] [PATCH qemu-server 7/8] machine: bump pve machine version and reverse the s3/s4 defaults

[Fiona Ebner]

Am 06.03.25 um 11:44 schrieb Dominik Csapak:
> so new guests (or guests with the 'latest' machine type) have
> that setting automatically disabled.
> 
> The previous default (enabling S3/S4), does not make too much sense in a
> virtual environment, and sometimes makes problems, e.g. Windows defaults
> to using 'hybrid shutdown' and 'fast startup' when S4 is enabled, which
> leads to NVIDIA vGPU being broken on the boot after that.
> 
> Since the tests don't pin the pve version themselves, we have to update
> all the ones where the machine versions are derived from the installed
> QEMU version.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>


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

Re: [pve-devel] [PATCH qemu-server 8/8] tests: cfg2cmd: add test for windows machine pinning from meta info

[Fiona Ebner]

Am 06.03.25 um 11:44 schrieb Dominik Csapak:
> once with included pve machine and once without
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

If we go for this fallback approach:

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

In either case, it would be nice to also have a test with an explicitly
pinned 9.2+pve0 too.


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

Re: [pve-devel] [PATCH qemu-server 2/8] config to command: add one '-global' option for each flag

[Dominik Csapak]

On 3/6/25 13:55, Fiona Ebner wrote:
> Am 06.03.25 um 13:15 schrieb Dominik Csapak:
>> On 3/6/25 13:13, Fiona Ebner wrote:
>>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>>> If we have multiple 'globalFlags', we have to encode each one separately
>>>> on the commandline with '-global OPTION', since QEMU does not allow to
>>>> have multiple options here.
>>>>
>>>> We currently only have one such flag that used the 'globalFlags' list,
>>>> so it never popped up. (All other uses directly add an option to the
>>>> commandline)
>>>>
>>>> Avoid future bugs by fixing it now.
>>>>
>>>
>>> So there is no real point to collecting the flags in the first place?
>>> I.e. we could also get rid of the variable and have the single current
>>> user of the variable add the flag directly on the commandline too. Or
>>> otherwise, we could change the other users and collect all flags with
>>> this variable. Pre-existing of course, but ideally, we could avoid the
>>> mishmash.
>>>
>>
>> Sorry this could have been more clear here:
>> I add to the flags in one of the following patches, so i sent this
>> in preparation of that (could possibly be squashed)
> 
> Yes, I understand that. I still think the status quo with mixing two
> different approaches might not be best. It's not going to be a blocker
> for the series, but I wanted to mention it, if you want to go for
> avoiding it.
> 
>> I did not want to touch the other places, since that in turn changes
>> the order of the qemu commandline (which sometimes has unintended side
>> effects, e.g. in combination with the 'args' parameter)
> 
> Are you sure? Custom 'args' are always added last so that shouldn't matter.
> 
> The only thing that would change by removing the global flags variable
> is having "-global kvm-pit.lost_tick_policy=discard" earlier in the
> commandline. I think that should be fine. In particular QEMU's
> qemu_init() function has a call to user_register_global_props() which
> handles all global properties at the same time, so I think changing the
> order should be fine in (almost?) all cases.

I'll test that, but imho it would better to do the reverse here?
So don't interject '-gloabl' parameters throughout config2command, but
add them to the globalFlags and output them together at the end?

we'd have to touch the same number of tests i think, but it seems less
confusing to me (also in the resulting commandline we'd have all
global options together then)

Or is there a better argument for injecting the global parameters
in the middle?


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

Re: [pve-devel] [PATCH qemu-server 4/8] machine: correctly select pve machine version for non pinned windows guests

[Dominik Csapak]

On 3/6/25 15:20, Fiona Ebner wrote:
> Am 06.03.25 um 15:15 schrieb Dominik Csapak:
>> On 3/6/25 15:10, Fiona Ebner wrote:
>>> Am 06.03.25 um 14:36 schrieb Dominik Csapak:
>>>> On 3/6/25 14:10, Fiona Ebner wrote:
>>>>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>>>>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>>>>>> index f1acde8f..e3da8e21 100644
>>>>>> --- a/PVE/QemuServer/Machine.pm
>>>>>> +++ b/PVE/QemuServer/Machine.pm
>>>>>> @@ -237,14 +237,19 @@ sub get_vm_machine {
>>>>>>             if (PVE::QemuServer::Helpers::min_version($meta-
>>>>>>> {'creation-qemu'}, 9, 1)) {
>>>>>>                 # need only major.minor
>>>>>>                 ($base_version) = ($meta->{'creation-qemu'} =~ m/^(\d+.
>>>>>> \d+)/);
>>>>>> +            # append pve machine version if we have one
>>>>>> +            if (my $pvever = $meta->{'creation-pve-machine'}) {
>>>>>> +            $base_version .= "+pve$pvever"
>>>>>
>>>>> Since this is only the fallback handling for the rare edge case
>>>>> where no
>>>>> explicit machine version is set for a Windows guest, not sure if it's
>>>>> even worth doing this. I.e. can we just avoid the additional meta
>>>>> property and always use pve0 here? Did you intend any other use for the
>>>>> creation-pve-machine?
>>>>>
>>>>
>>>> I though the intention of the code was to pin the guest to that version
>>>> where it
>>>> was created. If we omit the machine version, this is not true anymore,
>>>> since I'd then create a windows vm with e.g. pc-q35-9.2+pve1 then set it
>>>> to q35, and would get pc-q35-9.2+pve0
>>>
>>> Fair enough. There might be people manually changing windows guests to
>>> use the latest machine even if we don't recommend it or allow it in the
>>> UI. Still, it would just mean a pve machine version downgrade for
>>> subsequent boots, which also can happen if you offline migrate to a node
>>> that does not yet support the newest pve machine version.
>>>
>>>>
>>>> and while i don't have anymore use cases for the current case, wouldn't
>>>> this approach here correct also when we decide to bump again in the
>>>> future?
>>>
>>> Correct what?
>>>
>>
>> i wanted to write 'wouldn't this approach here be correct [..]'
> 
> I never said it's not correct. Just questioning if it's worth it.
> 
>>>> also recording with which pve version the vm was created could help in
>>>> debugging
>>>> issues at some point.. (my patch only adds the version in the meta info
>>>> if it's > 0 anyway)
>>>>
>>>
>>> In principle, yes. But honestly, it's much less informative than the
>>> creation QEMU version, and even that is rather rarely useful for
>>> debugging in my experience.
>>>
>>> So not a huge fan, since I'd find it nicer to keep the meta info slick,
>>> but we can go for it if you really think it's worth it.
>>
>>
>> mhmm i get what you mean, would it be an ok compromise to record the pve
>> version with
>> the creation machine maybe?
>>
>> i'd have to touch the places where we read that ofc but that shouldn't
>> be too many
>>
> 
> The creation-qemu is the binary version, but the latest pve version is
> related to the qemu-server version/machine version, so while I get where
> you're coming from, I'd rather keep them separate.

Ok, I'll send a new version without recording the PVE version


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

Re: [pve-devel] [PATCH qemu-server 5/8] machine: incorporate pve machine version when pinning windows guests

[Dominik Csapak]

On 3/6/25 15:32, Fiona Ebner wrote:
> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>> When creating or updating guests with ostype windows, we want to pin the
>> machine version to a specific one. Since introduction of that feature,
>> we never bumped the pve machine version, so this was missing.
>>
>> Append the pve machine version if it's not 0 so we don't add that
>> unnecessarily.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/QemuServer/Machine.pm | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>> index e3da8e21..ebaf2dcc 100644
>> --- a/PVE/QemuServer/Machine.pm
>> +++ b/PVE/QemuServer/Machine.pm
>> @@ -274,7 +274,17 @@ sub check_and_pin_machine_string {
>>       if (!$machine || $machine =~ m/^(?:pc|q35|virt)$/) {
>>   	# always pin Windows' machine version on create, they get confused too easily
>>   	if (PVE::QemuServer::Helpers::windows_version($ostype)) {
>> -	    $machine_conf->{type} = windows_get_pinned_machine_version($machine);
>> +	    my $kvmversion = PVE::QemuServer::Helpers::kvm_user_version();
>> +	    my $pin_version = get_installed_machine_version($kvmversion);
> 
> Nit: I'd call this $base_version like the argument it's passed-in as.
> It's not necessarily the version that is going to be pinned.

sure>
>> +
>> +	    # pin to the current pveX version to make use of most current features if > 0
>> +	    my $pvever = get_pve_version($kvmversion);
>> +	    if ($pvever > 0) {
>> +		$pin_version .= "+pve$pvever";
>> +	    }
> 
> I feel this logic should go into windows_get_pinned_machine_version()
> itself.
> 

if we'd do that, we'd automatically get the newest pve version for the windows workaround in
get_vm_machine, which is the reverse you suggested IIUC ? (e.g. use pve0 in that case)
and using a parameter to control that seem weird for this one call...

>> +
>> +	    $machine_conf->{type} = windows_get_pinned_machine_version($machine, $pin_version, $kvmversion);
>> +
>>   	    print "pinning machine type to '$machine_conf->{type}' for Windows guest OS\n";
>>   	}
>>       }
> 



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

Re: [pve-devel] [PATCH qemu-server 2/8] config to command: add one '-global' option for each flag

[Fiona Ebner]

Am 07.03.25 um 10:54 schrieb Dominik Csapak:
> On 3/6/25 13:55, Fiona Ebner wrote:
>> Am 06.03.25 um 13:15 schrieb Dominik Csapak:
>>> On 3/6/25 13:13, Fiona Ebner wrote:
>>>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>>>> If we have multiple 'globalFlags', we have to encode each one
>>>>> separately
>>>>> on the commandline with '-global OPTION', since QEMU does not allow to
>>>>> have multiple options here.
>>>>>
>>>>> We currently only have one such flag that used the 'globalFlags' list,
>>>>> so it never popped up. (All other uses directly add an option to the
>>>>> commandline)
>>>>>
>>>>> Avoid future bugs by fixing it now.
>>>>>
>>>>
>>>> So there is no real point to collecting the flags in the first place?
>>>> I.e. we could also get rid of the variable and have the single current
>>>> user of the variable add the flag directly on the commandline too. Or
>>>> otherwise, we could change the other users and collect all flags with
>>>> this variable. Pre-existing of course, but ideally, we could avoid the
>>>> mishmash.
>>>>
>>>
>>> Sorry this could have been more clear here:
>>> I add to the flags in one of the following patches, so i sent this
>>> in preparation of that (could possibly be squashed)
>>
>> Yes, I understand that. I still think the status quo with mixing two
>> different approaches might not be best. It's not going to be a blocker
>> for the series, but I wanted to mention it, if you want to go for
>> avoiding it.
>>
>>> I did not want to touch the other places, since that in turn changes
>>> the order of the qemu commandline (which sometimes has unintended side
>>> effects, e.g. in combination with the 'args' parameter)
>>
>> Are you sure? Custom 'args' are always added last so that shouldn't
>> matter.
>>
>> The only thing that would change by removing the global flags variable
>> is having "-global kvm-pit.lost_tick_policy=discard" earlier in the
>> commandline. I think that should be fine. In particular QEMU's
>> qemu_init() function has a call to user_register_global_props() which
>> handles all global properties at the same time, so I think changing the
>> order should be fine in (almost?) all cases.
> 
> I'll test that, but imho it would better to do the reverse here?
> So don't interject '-gloabl' parameters throughout config2command, but
> add them to the globalFlags and output them together at the end?
> 
> we'd have to touch the same number of tests i think, but it seems less
> confusing to me (also in the resulting commandline we'd have all
> global options together then)
> 
> Or is there a better argument for injecting the global parameters
> in the middle?

It avoids the need for the variable to collect and passing it around and
to remember adding future ones to that variable too.

It doesn't make a difference from QEMUs perspective, but would slightly
improve readability for humans looking at the commandline.

Note that I already suggested collecting all in the variable as an
approach above. I just want to avoid the mishmash.


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

Re: [pve-devel] [PATCH qemu-server 6/8] machine: add S3/S4 power state properties

[Dominik Csapak]

On 3/6/25 15:52, Fiona Ebner wrote:
> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>> So users can disable them (they're enabled by default in QEMU)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> This patch may make sense, regardless if we'll apply the reversal of the
>> default...
>>
>>   PVE/QemuServer.pm         |  2 ++
>>   PVE/QemuServer/Machine.pm | 40 +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 42 insertions(+)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 13af495d..b8ce4750 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -3975,6 +3975,8 @@ sub config_to_command {
>>   	push @$machineFlags, 'accel=tcg';
>>       }
>>   
>> +    PVE::QemuServer::Machine::check_and_set_power_state_flags($globalFlags, $machine_conf);
>> +
>>       push @$machineFlags, 'smm=off' if should_disable_smm($conf, $vga, $machine_type);
>>   
>>       my $machine_type_min = $machine_type;
>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>> index ebaf2dcc..377abc8a 100644
>> --- a/PVE/QemuServer/Machine.pm
>> +++ b/PVE/QemuServer/Machine.pm
>> @@ -31,6 +31,16 @@ my $machine_fmt = {
>>   	enum => ['intel', 'virtio'],
>>   	optional => 1,
>>       },
>> +    'enable-s3' => {
>> +	type => 'boolean',
>> +	description => "Enables S3 power state. Defaults to true.",
>> +	optional => 1,
>> +    },
>> +    'enable-s4' => {
>> +	type => 'boolean',
>> +	description => "Enables S4 power state. Defaults to true.",
>> +	optional => 1,
>> +    },
>>   };
>>   
>>   PVE::JSONSchema::register_format('pve-qemu-machine-fmt', $machine_fmt);
> 
> Note that an UI patch is needed, because editing the machine there will
> not preserve these values currently.
> 

alternative suggestions: I'd prefer to return the +pveX machines (where X > 0)
also in the 'capabilities/qemu/machine' api call listing. This would
1. allow the users of the api to see that there are variants
2. allows the UI to work automagically (since it uses that api call)
    - shows the correct version without losing info
    - makes it possible to select e.g. a +pve1 variant

what do you say?

>> @@ -293,4 +303,34 @@ sub check_and_pin_machine_string {
>>       return print_machine($machine_conf);
>>   }
>>   
>> +# disable s3/s4 by default for 9.2+pve1 machine types
>> +sub check_and_set_power_state_flags {
>> +    my ($globalFlags, $machine_conf) = @_;
>> +
>> +    my $get_flag = sub {
>> +	my ($q35, $type, $value) = @_;
> 
> Style nit: $value is always 1
> 
>> +
>> +	if ($q35) {
>> +	    return "ICH9-LPC.disable_${type}=${value}";
>> +	} else {
>> +	    return "PIIX4_PM.disable_${type}=${value}";
>> +	}
>> +    };
> 
> Rather than this closure, could just be $object = $q35 ? 'ICH9-LPC' :
> 'PIIX4_PM' and then use that for constructing the string when pusing.

ok makes sense, in an early version the code was structured differently
where this closure made more sense, and i didn't clean it up...

> 
>> +
>> +    my $q35 = $machine_conf->{type} && ($machine_conf->{type} =~ m/q35/) ? 1 : 0;
>> +
>> +    my $s3 = $machine_conf->{'enable-s3'} // 1;
>> +    my $s4 = $machine_conf->{'enable-s4'} // 1;
>> +
>> +    # they're enabled by default in QEMU, so only add the flags to disable them
>> +    if (!$s3) {
>> +	push $globalFlags->@*, $get_flag->($q35, 's3', 1)
> 
> Style nit: missing semicolon
> 
>> +    }
>> +    if (!$s4) {
>> +	push $globalFlags->@*, $get_flag->($q35, 's4', 1)
> 
> Style nit: missing semicolon
> 
>> +    }
>> +
>> +    return;
>> +}
>> +
>>   1;
> 



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

Re: [pve-devel] [PATCH qemu-server 2/8] config to command: add one '-global' option for each flag

[Dominik Csapak]

On 3/7/25 11:00, Fiona Ebner wrote:
> Am 07.03.25 um 10:54 schrieb Dominik Csapak:
>> On 3/6/25 13:55, Fiona Ebner wrote:
>>> Am 06.03.25 um 13:15 schrieb Dominik Csapak:
>>>> On 3/6/25 13:13, Fiona Ebner wrote:
>>>>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>>>>> If we have multiple 'globalFlags', we have to encode each one
>>>>>> separately
>>>>>> on the commandline with '-global OPTION', since QEMU does not allow to
>>>>>> have multiple options here.
>>>>>>
>>>>>> We currently only have one such flag that used the 'globalFlags' list,
>>>>>> so it never popped up. (All other uses directly add an option to the
>>>>>> commandline)
>>>>>>
>>>>>> Avoid future bugs by fixing it now.
>>>>>>
>>>>>
>>>>> So there is no real point to collecting the flags in the first place?
>>>>> I.e. we could also get rid of the variable and have the single current
>>>>> user of the variable add the flag directly on the commandline too. Or
>>>>> otherwise, we could change the other users and collect all flags with
>>>>> this variable. Pre-existing of course, but ideally, we could avoid the
>>>>> mishmash.
>>>>>
>>>>
>>>> Sorry this could have been more clear here:
>>>> I add to the flags in one of the following patches, so i sent this
>>>> in preparation of that (could possibly be squashed)
>>>
>>> Yes, I understand that. I still think the status quo with mixing two
>>> different approaches might not be best. It's not going to be a blocker
>>> for the series, but I wanted to mention it, if you want to go for
>>> avoiding it.
>>>
>>>> I did not want to touch the other places, since that in turn changes
>>>> the order of the qemu commandline (which sometimes has unintended side
>>>> effects, e.g. in combination with the 'args' parameter)
>>>
>>> Are you sure? Custom 'args' are always added last so that shouldn't
>>> matter.
>>>
>>> The only thing that would change by removing the global flags variable
>>> is having "-global kvm-pit.lost_tick_policy=discard" earlier in the
>>> commandline. I think that should be fine. In particular QEMU's
>>> qemu_init() function has a call to user_register_global_props() which
>>> handles all global properties at the same time, so I think changing the
>>> order should be fine in (almost?) all cases.
>>
>> I'll test that, but imho it would better to do the reverse here?
>> So don't interject '-gloabl' parameters throughout config2command, but
>> add them to the globalFlags and output them together at the end?
>>
>> we'd have to touch the same number of tests i think, but it seems less
>> confusing to me (also in the resulting commandline we'd have all
>> global options together then)
>>
>> Or is there a better argument for injecting the global parameters
>> in the middle?
> 
> It avoids the need for the variable to collect and passing it around and
> to remember adding future ones to that variable too.
> 
> It doesn't make a difference from QEMUs perspective, but would slightly
> improve readability for humans looking at the commandline.
> 
> Note that I already suggested collecting all in the variable as an
> approach above. I just want to avoid the mishmash.

OK, which approach would you prefer? (I don't have a strong feeling in either direction
and doing the cleanup now seems to not be a lot of work)


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

Re: [pve-devel] [PATCH qemu-server 5/8] machine: incorporate pve machine version when pinning windows guests

[Fiona Ebner]

Am 07.03.25 um 10:58 schrieb Dominik Csapak:
> On 3/6/25 15:32, Fiona Ebner wrote:
>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>> When creating or updating guests with ostype windows, we want to pin the
>>> machine version to a specific one. Since introduction of that feature,
>>> we never bumped the pve machine version, so this was missing.
>>>
>>> Append the pve machine version if it's not 0 so we don't add that
>>> unnecessarily.
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>>   PVE/QemuServer/Machine.pm | 12 +++++++++++-
>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>>> index e3da8e21..ebaf2dcc 100644
>>> --- a/PVE/QemuServer/Machine.pm
>>> +++ b/PVE/QemuServer/Machine.pm
>>> @@ -274,7 +274,17 @@ sub check_and_pin_machine_string {
>>>       if (!$machine || $machine =~ m/^(?:pc|q35|virt)$/) {
>>>       # always pin Windows' machine version on create, they get
>>> confused too easily
>>>       if (PVE::QemuServer::Helpers::windows_version($ostype)) {
>>> -        $machine_conf->{type} =
>>> windows_get_pinned_machine_version($machine);
>>> +        my $kvmversion = PVE::QemuServer::Helpers::kvm_user_version();
>>> +        my $pin_version = get_installed_machine_version($kvmversion);
>>
>> Nit: I'd call this $base_version like the argument it's passed-in as.
>> It's not necessarily the version that is going to be pinned.
> 
> sure>
>>> +
>>> +        # pin to the current pveX version to make use of most
>>> current features if > 0
>>> +        my $pvever = get_pve_version($kvmversion);
>>> +        if ($pvever > 0) {
>>> +        $pin_version .= "+pve$pvever";
>>> +        }
>>
>> I feel this logic should go into windows_get_pinned_machine_version()
>> itself.
>>
> 
> if we'd do that, we'd automatically get the newest pve version for the
> windows workaround in
> get_vm_machine, which is the reverse you suggested IIUC ? (e.g. use pve0
> in that case)
> and using a parameter to control that seem weird for this one call...
> 

Depends on where exactly you do it. If you do it in the branch when you
don't have an explicitly passed-in base version then you don't affect
that caller (except if too old QEMU binary, but that is even more of an
edge case and I don't see a reason not to use the newest pve version
then if we already need to use a pre-creation version).


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

Re: [pve-devel] [PATCH qemu-server 6/8] machine: add S3/S4 power state properties

[Fiona Ebner]

Am 07.03.25 um 11:02 schrieb Dominik Csapak:
> On 3/6/25 15:52, Fiona Ebner wrote:
>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>>> index ebaf2dcc..377abc8a 100644
>>> --- a/PVE/QemuServer/Machine.pm
>>> +++ b/PVE/QemuServer/Machine.pm
>>> @@ -31,6 +31,16 @@ my $machine_fmt = {
>>>       enum => ['intel', 'virtio'],
>>>       optional => 1,
>>>       },
>>> +    'enable-s3' => {
>>> +    type => 'boolean',
>>> +    description => "Enables S3 power state. Defaults to true.",
>>> +    optional => 1,
>>> +    },
>>> +    'enable-s4' => {
>>> +    type => 'boolean',
>>> +    description => "Enables S4 power state. Defaults to true.",
>>> +    optional => 1,
>>> +    },
>>>   };
>>>     PVE::JSONSchema::register_format('pve-qemu-machine-fmt',
>>> $machine_fmt);
>>
>> Note that an UI patch is needed, because editing the machine there will
>> not preserve these values currently.
>>
> 
> alternative suggestions: I'd prefer to return the +pveX machines (where
> X > 0)
> also in the 'capabilities/qemu/machine' api call listing. This would
> 1. allow the users of the api to see that there are variants
> 2. allows the UI to work automagically (since it uses that api call)
>    - shows the correct version without losing info
>    - makes it possible to select e.g. a +pve1 variant
> 
> what do you say?

Sounds good. We should also add something to the docs to describe what
+pveN versions are. That would've been useful even before, but if we
expose it in the UI it's high time to do so.


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

Re: [pve-devel] [PATCH qemu-server 2/8] config to command: add one '-global' option for each flag

[Fiona Ebner]

Am 07.03.25 um 11:05 schrieb Dominik Csapak:
> On 3/7/25 11:00, Fiona Ebner wrote:
>> Am 07.03.25 um 10:54 schrieb Dominik Csapak:
>>> On 3/6/25 13:55, Fiona Ebner wrote:
>>>> Am 06.03.25 um 13:15 schrieb Dominik Csapak:
>>>>> On 3/6/25 13:13, Fiona Ebner wrote:
>>>>>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>>>>>> If we have multiple 'globalFlags', we have to encode each one
>>>>>>> separately
>>>>>>> on the commandline with '-global OPTION', since QEMU does not
>>>>>>> allow to
>>>>>>> have multiple options here.
>>>>>>>
>>>>>>> We currently only have one such flag that used the 'globalFlags'
>>>>>>> list,
>>>>>>> so it never popped up. (All other uses directly add an option to the
>>>>>>> commandline)
>>>>>>>
>>>>>>> Avoid future bugs by fixing it now.
>>>>>>>
>>>>>>
>>>>>> So there is no real point to collecting the flags in the first place?
>>>>>> I.e. we could also get rid of the variable and have the single
>>>>>> current
>>>>>> user of the variable add the flag directly on the commandline too. Or
>>>>>> otherwise, we could change the other users and collect all flags with
>>>>>> this variable. Pre-existing of course, but ideally, we could avoid
>>>>>> the
>>>>>> mishmash.
>>>>>>
>>>>>
>>>>> Sorry this could have been more clear here:
>>>>> I add to the flags in one of the following patches, so i sent this
>>>>> in preparation of that (could possibly be squashed)
>>>>
>>>> Yes, I understand that. I still think the status quo with mixing two
>>>> different approaches might not be best. It's not going to be a blocker
>>>> for the series, but I wanted to mention it, if you want to go for
>>>> avoiding it.
>>>>
>>>>> I did not want to touch the other places, since that in turn changes
>>>>> the order of the qemu commandline (which sometimes has unintended side
>>>>> effects, e.g. in combination with the 'args' parameter)
>>>>
>>>> Are you sure? Custom 'args' are always added last so that shouldn't
>>>> matter.
>>>>
>>>> The only thing that would change by removing the global flags variable
>>>> is having "-global kvm-pit.lost_tick_policy=discard" earlier in the
>>>> commandline. I think that should be fine. In particular QEMU's
>>>> qemu_init() function has a call to user_register_global_props() which
>>>> handles all global properties at the same time, so I think changing the
>>>> order should be fine in (almost?) all cases.
>>>
>>> I'll test that, but imho it would better to do the reverse here?
>>> So don't interject '-gloabl' parameters throughout config2command, but
>>> add them to the globalFlags and output them together at the end?
>>>
>>> we'd have to touch the same number of tests i think, but it seems less
>>> confusing to me (also in the resulting commandline we'd have all
>>> global options together then)
>>>
>>> Or is there a better argument for injecting the global parameters
>>> in the middle?
>>
>> It avoids the need for the variable to collect and passing it around and
>> to remember adding future ones to that variable too.
>>
>> It doesn't make a difference from QEMUs perspective, but would slightly
>> improve readability for humans looking at the commandline.
>>
>> Note that I already suggested collecting all in the variable as an
>> approach above. I just want to avoid the mishmash.
> 
> OK, which approach would you prefer? (I don't have a strong feeling in
> either direction
> and doing the cleanup now seems to not be a lot of work)
> 

I don't have a strong opinion either, but slightly prefer the
no-variable/collect approach. Just because cfg2cmd is already quite
bloated. We can always switch to collecting again, if we continue adding
global flags and think it's worth for readability on the cmdline.


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

Re: [pve-devel] [PATCH qemu-server 0/8] disable S3/S4 power states by default

[Dominik Csapak]

superseded by v2:
https://lore.proxmox.com/pve-devel/20250307144436.122621-1-d.csapak@proxmox.com/


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