[pve-devel] [PATCH 0/7] Work around QEMU 5.2 windows incompatibility

[pve-devel] [PATCH 0/7] Work around QEMU 5.2 windows incompatibility

[Stefan Reiter]

...and make machine version configurable in the GUI, to allow users to return to
the default behaviour of always using the latest one.

The workaround is based on a postinst script running when upgrading the
qemu-server package, which sets the machine version of all effected Windows VMs
to pc-(i440fx|q35)-5.1 to pin them to the last version still using the old ACPI
table layout.

Note: the postinst patch requires Fabian's #3301 series:
https://lists.proxmox.com/pipermail/pve-devel/2021-March/047175.html


pve-qemu: Stefan Reiter (2):
  add static supported machines file
  add ACPI compat patch for 5.1 and older machine types

 debian/parse-machines.pl                      |  21 ++++
 ...restore-device-paths-for-pre-5.1-vms.patch | 108 ++++++++++++++++++
 debian/patches/series                         |   1 +
 debian/rules                                  |   4 +-
 4 files changed, 133 insertions(+), 1 deletion(-)
 create mode 100755 debian/parse-machines.pl
 create mode 100644 debian/patches/extra/0009-i386-acpi-restore-device-paths-for-pre-5.1-vms.patch

qemu-server: Stefan Reiter (2):
  api: add Machine module to query machine types
  add postinst with Windows device incompatibility workaround

 PVE/API2/Qemu/Machine.pm | 49 +++++++++++++++++++++
 PVE/API2/Qemu/Makefile   |  2 +-
 debian/postinst          | 95 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+), 1 deletion(-)
 create mode 100644 PVE/API2/Qemu/Machine.pm
 create mode 100755 debian/postinst

manager: Stefan Reiter (3):
  api: register Qemu::Machine call
  ui: create MachineEdit window
  ui: MachineEdit: add option for machine version pinning

 PVE/API2/Nodes.pm                 |   6 ++
 www/manager6/Makefile             |   1 +
 www/manager6/qemu/HardwareView.js |  16 +----
 www/manager6/qemu/MachineEdit.js  | 107 ++++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+), 15 deletions(-)
 create mode 100644 www/manager6/qemu/MachineEdit.js

-- 
2.20.1

[pve-devel] [PATCH pve-qemu 1/7] add static supported machines file

[Stefan Reiter]

Same rationale as the CPU flags file, avoids calling QEMU binary just to
query machine types.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 debian/parse-machines.pl | 21 +++++++++++++++++++++
 debian/rules             |  4 +++-
 2 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100755 debian/parse-machines.pl

diff --git a/debian/parse-machines.pl b/debian/parse-machines.pl
new file mode 100755
index 0000000..8ad335d
--- /dev/null
+++ b/debian/parse-machines.pl
@@ -0,0 +1,21 @@
+#!/usr/bin/perl
+
+use warnings;
+use strict;
+
+my @machines = ();
+
+while (<STDIN>) {
+    if (/^\s*Supported machines are:/) {
+	next;
+    }
+
+    s/^\s+//;
+    my @machine = split(/\s+/);
+    next if $machine[0] !~ m/^pc-(i440fx|q35)/;
+    push @machines, $machine[0];
+}
+
+die "no QEMU machine types detected from STDIN input" if scalar (@machines) <= 0;
+
+print join("\n", @machines) or die "$!\n";
diff --git a/debian/rules b/debian/rules
index 4a6b472..386ac4b 100755
--- a/debian/rules
+++ b/debian/rules
@@ -22,6 +22,7 @@ PACKAGE=pve-qemu-kvm
 destdir := $(CURDIR)/debian/$(PACKAGE)
 
 flagfile := $(destdir)/usr/share/kvm/recognized-CPUID-flags-x86_64
+machinefile := $(destdir)/usr/share/kvm/machine-versions-x86_64
 
 # default QEMU out-of-tree build directory is ./build
 BUILDDIR=build
@@ -124,7 +125,7 @@ install: build
 	rm $(destdir)/usr/share/kvm/qemu_vga.ndrv
 	rm $(destdir)/usr/share/kvm/slof.bin
 	rm $(destdir)/usr/share/kvm/u-boot.e500
-	# remove Aplha files
+	# remove Alpha files
 	rm $(destdir)/usr/share/kvm/palcode-clipper
 	# remove RISC-V files
 	rm $(destdir)/usr/share/kvm/opensbi-riscv32-generic-fw_dynamic.elf
@@ -138,6 +139,7 @@ install: build
 
 	# CPU flags are static for QEMU version, allows avoiding more costly checks
 	$(destdir)/usr/bin/qemu-system-x86_64 -cpu help | ./debian/parse-cpu-flags.pl > $(flagfile)
+	$(destdir)/usr/bin/qemu-system-x86_64 -machine help | ./debian/parse-machines.pl > $(machinefile)
 
 # Build architecture-independent files here.
 binary-indep: build install
-- 
2.20.1

[pve-devel] [PATCH pve-qemu 2/7] add ACPI compat patch for 5.1 and older machine types

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

This is required to make the whole deal work, otherwise even setting back the
machine version to 5.1 or lower won't actually fix the issue.

 ...restore-device-paths-for-pre-5.1-vms.patch | 108 ++++++++++++++++++
 debian/patches/series                         |   1 +
 2 files changed, 109 insertions(+)
 create mode 100644 debian/patches/extra/0009-i386-acpi-restore-device-paths-for-pre-5.1-vms.patch

diff --git a/debian/patches/extra/0009-i386-acpi-restore-device-paths-for-pre-5.1-vms.patch b/debian/patches/extra/0009-i386-acpi-restore-device-paths-for-pre-5.1-vms.patch
new file mode 100644
index 0000000..0539608
--- /dev/null
+++ b/debian/patches/extra/0009-i386-acpi-restore-device-paths-for-pre-5.1-vms.patch
@@ -0,0 +1,108 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Vitaly Cheptsov <cheptsov@ispras.ru>
+Date: Tue, 2 Mar 2021 09:21:10 -0500
+Subject: [PATCH] i386/acpi: restore device paths for pre-5.1 vms
+
+After fixing the _UID value for the primary PCI root bridge in
+af1b80ae it was discovered that this change updates Windows
+configuration in an incompatible way causing network configuration
+failure unless DHCP is used. More details provided on the list:
+
+https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg08484.html
+
+This change reverts the _UID update from 1 to 0 for q35 and i440fx
+VMs before version 5.2 to maintain the original behaviour when
+upgrading.
+
+Cc: qemu-stable@nongnu.org
+Cc: qemu-devel@nongnu.org
+Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
+Suggested-by: Michael S. Tsirkin <mst@redhat.com>
+Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru>
+Message-Id: <20210301195919.9333-1-cheptsov@ispras.ru>
+Tested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
+Reviewed-by: Igor Mammedov <imammedo@redhat.com>
+Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
+Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
+Fixes: af1b80ae56c9 ("i386/acpi: fix inconsistent QEMU/OVMF device paths")
+---
+ hw/i386/acpi-build.c | 4 ++--
+ hw/i386/pc_piix.c    | 2 ++
+ hw/i386/pc_q35.c     | 2 ++
+ include/hw/i386/pc.h | 1 +
+ 4 files changed, 7 insertions(+), 2 deletions(-)
+
+diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
+index 1f5c211245..b5616582a5 100644
+--- a/hw/i386/acpi-build.c
++++ b/hw/i386/acpi-build.c
+@@ -1513,7 +1513,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
+         dev = aml_device("PCI0");
+         aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
+         aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
+-        aml_append(dev, aml_name_decl("_UID", aml_int(0)));
++        aml_append(dev, aml_name_decl("_UID", aml_int(pcmc->pci_root_uid)));
+         aml_append(sb_scope, dev);
+         aml_append(dsdt, sb_scope);
+ 
+@@ -1530,7 +1530,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
+         aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
+         aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
+         aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
+-        aml_append(dev, aml_name_decl("_UID", aml_int(0)));
++        aml_append(dev, aml_name_decl("_UID", aml_int(pcmc->pci_root_uid)));
+         aml_append(dev, build_q35_osc_method());
+         aml_append(sb_scope, dev);
+ 
+diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
+index 13d1628f13..2524c96216 100644
+--- a/hw/i386/pc_piix.c
++++ b/hw/i386/pc_piix.c
+@@ -417,6 +417,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
+ {
+     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+     pcmc->default_nic_model = "e1000";
++    pcmc->pci_root_uid = 0;
+ 
+     m->family = "pc_piix";
+     m->desc = "Standard PC (i440FX + PIIX, 1996)";
+@@ -448,6 +449,7 @@ static void pc_i440fx_5_1_machine_options(MachineClass *m)
+     compat_props_add(m->compat_props, hw_compat_5_1, hw_compat_5_1_len);
+     compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
+     pcmc->kvmclock_create_always = false;
++    pcmc->pci_root_uid = 1;
+ }
+ 
+ DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
+diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
+index a3f4959c43..c58dad5ae3 100644
+--- a/hw/i386/pc_q35.c
++++ b/hw/i386/pc_q35.c
+@@ -329,6 +329,7 @@ static void pc_q35_machine_options(MachineClass *m)
+ {
+     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+     pcmc->default_nic_model = "e1000e";
++    pcmc->pci_root_uid = 0;
+ 
+     m->family = "pc_q35";
+     m->desc = "Standard PC (Q35 + ICH9, 2009)";
+@@ -364,6 +365,7 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
+     compat_props_add(m->compat_props, hw_compat_5_1, hw_compat_5_1_len);
+     compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
+     pcmc->kvmclock_create_always = false;
++    pcmc->pci_root_uid = 1;
+ }
+ 
+ DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
+diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
+index 911e460097..7f8e1a791f 100644
+--- a/include/hw/i386/pc.h
++++ b/include/hw/i386/pc.h
+@@ -99,6 +99,7 @@ struct PCMachineClass {
+     int legacy_acpi_table_size;
+     unsigned acpi_data_size;
+     bool do_not_add_smb_acpi;
++    int pci_root_uid;
+ 
+     /* SMBIOS compat: */
+     bool smbios_defaults;
diff --git a/debian/patches/series b/debian/patches/series
index 1b30d97..f29a15d 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -6,6 +6,7 @@ extra/0005-virtiofsd-optionally-return-inode-pointer-from-lo_do.patch
 extra/0006-virtiofsd-prevent-opening-of-special-files-CVE-2020-.patch
 extra/0007-virtiofsd-Add-_llseek-to-the-seccomp-whitelist.patch
 extra/0008-virtiofsd-Add-restart_syscall-to-the-seccomp-whiteli.patch
+extra/0009-i386-acpi-restore-device-paths-for-pre-5.1-vms.patch
 bitmap-mirror/0001-drive-mirror-add-support-for-sync-bitmap-mode-never.patch
 bitmap-mirror/0002-drive-mirror-add-support-for-conditional-and-always-.patch
 bitmap-mirror/0003-mirror-add-check-for-bitmap-mode-without-bitmap.patch
-- 
2.20.1

[pve-devel] [PATCH qemu-server 3/7] api: add Machine module to query machine types

[Stefan Reiter]

The file is provided by pve-qemu-kvm.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 PVE/API2/Qemu/Machine.pm | 49 ++++++++++++++++++++++++++++++++++++++++
 PVE/API2/Qemu/Makefile   |  2 +-
 2 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 PVE/API2/Qemu/Machine.pm

diff --git a/PVE/API2/Qemu/Machine.pm b/PVE/API2/Qemu/Machine.pm
new file mode 100644
index 0000000..c0a8c57
--- /dev/null
+++ b/PVE/API2/Qemu/Machine.pm
@@ -0,0 +1,49 @@
+package PVE::API2::Qemu::Machine;
+
+use strict;
+use warnings;
+
+use PVE::RESTHandler;
+use PVE::JSONSchema qw(get_standard_option);
+use PVE::Tools qw(file_get_contents);
+
+use base qw(PVE::RESTHandler);
+
+__PACKAGE__->register_method({
+    name => 'types',
+    path => '',
+    method => 'GET',
+    proxyto => 'node',
+    description => "Get available QEMU/KVM machine types.",
+    permissions => {
+	user => 'all',
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	},
+    },
+    returns => {
+	type => 'array',
+	items => {
+	    type => 'object',
+	    properties => {
+		name => {
+		    type => 'string',
+		    description => "Name of machine type.",
+		},
+	    },
+	},
+    },
+    code => sub {
+	my $content = eval {
+	    file_get_contents("/usr/share/kvm/machine-versions-x86_64");
+	};
+	die "could not get supported machine versions (try updating 'pve-qemu-kvm') - $@" if $@;
+	my @data = split(m/\n/, $content);
+	@data = map { { name => $_ } } @data;
+	return \@data;
+    }});
+
+1;
diff --git a/PVE/API2/Qemu/Makefile b/PVE/API2/Qemu/Makefile
index f4b7be6..5d4abda 100644
--- a/PVE/API2/Qemu/Makefile
+++ b/PVE/API2/Qemu/Makefile
@@ -1,4 +1,4 @@
-SOURCES=Agent.pm CPU.pm
+SOURCES=Agent.pm CPU.pm Machine.pm
 
 .PHONY: install
 install:
-- 
2.20.1

[pve-devel] [PATCH qemu-server 4/7] add postinst with Windows device incompatibility workaround

[Stefan Reiter]

...for QEMU 5.2

Add a postinst with some inline perl (to avoid having to install a
seperate file to run) that conservatively does its best to set the
machine version for all VMs the bug might affect to 5.1.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Requires Fabian's #3301 series:
https://lists.proxmox.com/pipermail/pve-devel/2021-March/047175.html

I did it as inline perl because I wasn't sure where to put a file containing
this to be able to execute it from postinst without cluttering the machine after
it is done. Feel free to extract it somewhere else if you see fit.

Might also make sense to update the link from the forum for "more details" to
the cover of this series, or something else...

 debian/postinst | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100755 debian/postinst

diff --git a/debian/postinst b/debian/postinst
new file mode 100755
index 0000000..c93dd5d
--- /dev/null
+++ b/debian/postinst
@@ -0,0 +1,95 @@
+#!/bin/sh
+
+# Abort if any command returns an error value
+set -e
+
+# This script is called as the last step of the installation of the 
+# package.  All the package's files are in place, dpkg has already
+# done its automatic conffile handling, and all the packages we depend
+# of are already fully installed and configured.
+
+case "$1" in
+  configure)
+    if test -n "$2"; then
+	if dpkg --compare-versions "$2" 'lt' '6.3-6'; then
+            echo
+            perl <<"EOPERL"
+
+use warnings;
+use strict;
+
+use PVE::Cluster;
+use PVE::QemuConfig;
+use PVE::QemuServer;
+use PVE::QemuServer::Helpers qw(min_version);
+
+print "Pinning Windows VM machine versions to 5.1\n";
+print "See https://forum.proxmox.com/threads/warning-latest-patch-just-broke-all-my-windows-vms-6-3-4-patch-inside.84915/ for details\n";
+
+PVE::Cluster::check_cfs_is_mounted();
+PVE::Cluster::cfs_update();
+
+my $vms = PVE::QemuServer::vmstatus(undef, 1);
+foreach my $vmid (sort keys %$vms) {
+    my $running = defined($vms->{$vmid}->{pid}) ? 1 : 0;
+    eval {
+        PVE::QemuConfig->lock_config($vmid, sub {
+            my $conf = PVE::QemuConfig->load_config($vmid);
+            PVE::QemuConfig->check_lock($conf);
+
+            return if !$conf->{ostype} || !PVE::QemuServer::windows_version($conf->{ostype});
+
+            if ($running) {
+                my $cur_machine = $vms->{$vmid}->{'running-machine'};
+                $cur_machine =~ s/pc(-i440fx|-q35)?-//;
+                if (min_version($cur_machine, 5, 2)) {
+                    print "$vmid: [skip] already running with machine version '$cur_machine'\n";
+                    return;
+                }
+            }
+
+            if ($conf->{machine} && $conf->{machine} =~ m/^pc-/) {
+                print "$vmid: [skip] already pinned to '$conf->{machine}'\n";
+                return;
+            }
+
+            if ($conf->{pending}->{machine}) {
+                print "$vmid: [skip] machine changed in pending\n";
+                return;
+            }
+
+            my $cur = $conf->{machine} || "i440fx";
+            $cur = "i440fx" if $cur eq "pc";
+            my $new = "pc-$cur-5.1";
+
+            my $notice = "";
+            if ($running) {
+                $conf->{pending}->{machine} = $new;
+                $notice = " (after restart)";
+            } else {
+                $conf->{machine} = $new;
+            }
+
+            PVE::QemuConfig->write_config($vmid, $conf);
+            print "$vmid: [ OK ] now pinned to '$new'$notice\n";
+        });
+    };
+    warn "$vmid: [warn] failed to update: $@\n" if $@;
+}
+
+print "If you want to upgrade to the newest version, set the machine back to 'latest' in the GUI, or 'pc'/'q35' on the CLI.\n";
+
+EOPERL
+        echo
+	fi
+    fi
+    ;;
+
+  abort-upgrade|abort-remove|abort-deconfigure)
+    ;;
+
+  *) echo "$0: didn't understand being called with \`$1'" 1>&2
+     exit 0;;
+esac
+
+exit 0
-- 
2.20.1

[pve-devel] [PATCH manager 5/7] api: register Qemu::Machine call

[Stefan Reiter]

as 'machine-types', so it is clear this refers to QEMU machines, not the
local machine (as one might think, this being a 'node' API call).

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Requires dependency bump on updated qemu-server.

 PVE/API2/Nodes.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 8172231e..7a39320a 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -35,6 +35,7 @@ use PVE::API2::Storage::Scan;
 use PVE::API2::Storage::Status;
 use PVE::API2::Qemu;
 use PVE::API2::Qemu::CPU;
+use PVE::API2::Qemu::Machine;
 use PVE::API2::LXC;
 use PVE::API2::LXC::Status;
 use PVE::API2::VZDump;
@@ -71,6 +72,11 @@ __PACKAGE__->register_method ({
     path => 'cpu',
 });
 
+__PACKAGE__->register_method ({
+    subclass => "PVE::API2::Qemu::Machine",
+    path => 'machine-types',
+});
+
 __PACKAGE__->register_method ({
     subclass => "PVE::API2::LXC",
     path => 'lxc',
-- 
2.20.1

[pve-devel] [PATCH manager 6/7] ui: create MachineEdit window

[Stefan Reiter]

no functional change intended

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 www/manager6/Makefile             |  1 +
 www/manager6/qemu/HardwareView.js | 16 +------------
 www/manager6/qemu/MachineEdit.js  | 38 +++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 15 deletions(-)
 create mode 100644 www/manager6/qemu/MachineEdit.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 85f90ecd..a2f7be6d 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -204,6 +204,7 @@ JSSRC= 							\
 	qemu/HardwareView.js				\
 	qemu/IPConfigEdit.js				\
 	qemu/KeyboardEdit.js				\
+	qemu/MachineEdit.js				\
 	qemu/MemoryEdit.js				\
 	qemu/Monitor.js					\
 	qemu/NetworkEdit.js				\
diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 41d65b40..470baa46 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -154,21 +154,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	    },
 	    machine: {
 		header: gettext('Machine'),
-		editor: caps.vms['VM.Config.HWType'] ? {
-		    xtype: 'proxmoxWindowEdit',
-		    subject: gettext('Machine'),
-		    width: 350,
-		    items: [{
-			xtype: 'proxmoxKVComboBox',
-			name: 'machine',
-			value: '__default__',
-			fieldLabel: gettext('Machine'),
-			comboItems: [
-			    ['__default__', PVE.Utils.render_qemu_machine('')],
-			    ['q35', 'q35'],
-			],
-		    }],
-} : undefined,
+		editor: caps.vms['VM.Config.HWType'] ? 'PVE.qemu.MachineEdit' : undefined,
 		iconCls: 'cogs',
 		never_delete: true,
 		group: 6,
diff --git a/www/manager6/qemu/MachineEdit.js b/www/manager6/qemu/MachineEdit.js
new file mode 100644
index 00000000..8a3a6f7a
--- /dev/null
+++ b/www/manager6/qemu/MachineEdit.js
@@ -0,0 +1,38 @@
+Ext.define('PVE.qemu.MachineInputPanel', {
+    extend: 'Proxmox.panel.InputPanel',
+    xtype: 'pveMachineInputPanel',
+
+    items: [{
+	name: 'machine',
+	xtype: 'proxmoxKVComboBox',
+	fieldLabel: gettext('Machine'),
+	comboItems: [
+	    ['__default__', PVE.Utils.render_qemu_machine('')],
+	    ['q35', 'q35'],
+	],
+    }],
+});
+
+Ext.define('PVE.qemu.MachineEdit', {
+    extend: 'Proxmox.window.Edit',
+
+    subject: gettext('Machine'),
+
+    items: [{
+	xtype: 'pveMachineInputPanel',
+    }],
+
+    initComponent: function() {
+	let me = this;
+
+	me.callParent();
+
+	me.load({
+	    success: function(response) {
+		let vmconfig = response.result.data;
+		let machine = vmconfig.machine || '__default__';
+		me.setValues({ machine: machine });
+	    },
+	});
+    },
+});
-- 
2.20.1

[pve-devel] [PATCH manager 7/7] ui: MachineEdit: add option for machine version pinning

[Stefan Reiter]

Hidden behind "Advanced" options, as to not confuse inexperienced users.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 www/manager6/qemu/MachineEdit.js | 69 ++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/www/manager6/qemu/MachineEdit.js b/www/manager6/qemu/MachineEdit.js
index 8a3a6f7a..4eab6b8f 100644
--- a/www/manager6/qemu/MachineEdit.js
+++ b/www/manager6/qemu/MachineEdit.js
@@ -2,8 +2,51 @@ Ext.define('PVE.qemu.MachineInputPanel', {
     extend: 'Proxmox.panel.InputPanel',
     xtype: 'pveMachineInputPanel',
 
+    controller: {
+	xclass: 'Ext.app.ViewController',
+	control: {
+	    'combobox[name=machine]': {
+		change: 'onMachineChange',
+	    },
+	},
+	onMachineChange: function(field, value) {
+	    let me = this;
+	    let version = me.lookup('version');
+	    let store = version.getStore();
+	    let type = value === 'q35' ? 'q35' : 'i440fx';
+	    store.clearFilter();
+	    store.addFilter(val =>
+		(val.data.name === 'latest' || val.data.name.indexOf(type) !== -1));
+	    version.setValue('latest');
+	},
+    },
+
+    onGetValues: function(values) {
+	if (values.version && values.version !== 'latest') {
+	    values.machine = values.version;
+	    delete values.delete;
+	}
+	delete values.version;
+	return values;
+    },
+
+    setValues: function(values) {
+	let me = this;
+
+	if (values.machine !== '__default__' && values.machine !== 'q35') {
+	    values.version = values.machine;
+	    values.machine = values.version.match(/q35/) ? 'q35' : '__default__';
+
+	    // avoid hiding a pinned version
+	    Ext.ComponentQuery.query("#advancedcb")[0].setValue(true);
+	}
+
+	this.callParent(arguments);
+    },
+
     items: [{
 	name: 'machine',
+	reference: 'machine',
 	xtype: 'proxmoxKVComboBox',
 	fieldLabel: gettext('Machine'),
 	comboItems: [
@@ -11,6 +54,32 @@ Ext.define('PVE.qemu.MachineInputPanel', {
 	    ['q35', 'q35'],
 	],
     }],
+
+    advancedItems: [{
+	name: 'version',
+	reference: 'version',
+	xtype: 'combobox',
+	fieldLabel: gettext('Version'),
+	value: 'latest',
+	allowBlank: false,
+	editable: false,
+	valueField: 'name',
+	displayField: 'name',
+	queryParam: false,
+	store: {
+	    autoLoad: true,
+	    fields: ['name'],
+	    proxy: {
+		type: 'proxmox',
+		url: "/api2/json/nodes/localhost/machine-types",
+	    },
+	    listeners: {
+		load: function(records) {
+		    this.insert(0, { name: 'latest' });
+		},
+	    },
+	},
+    }],
 });
 
 Ext.define('PVE.qemu.MachineEdit', {
-- 
2.20.1

Re: [pve-devel] [PATCH qemu-server 4/7] add postinst with Windows device incompatibility workaround

[Stefan Reiter]

I suppose we should also detect old QEMU versions, so we don't set 5.1 
machine versions for QEMU < 5.1. Unlikely, but easy to detect:

     print "QEMU version too old for workaround" if 
!min_version(PVE::QemuServer::kvm_user_version(), 5, 1);

or similar... I can send a v2 if required.

On 04/03/2021 13:52, Stefan Reiter wrote:
> ...for QEMU 5.2
> 
> Add a postinst with some inline perl (to avoid having to install a
> seperate file to run) that conservatively does its best to set the
> machine version for all VMs the bug might affect to 5.1.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Requires Fabian's #3301 series:
> https://lists.proxmox.com/pipermail/pve-devel/2021-March/047175.html
> 
> I did it as inline perl because I wasn't sure where to put a file containing
> this to be able to execute it from postinst without cluttering the machine after
> it is done. Feel free to extract it somewhere else if you see fit.
> 
> Might also make sense to update the link from the forum for "more details" to
> the cover of this series, or something else...
> 
>   debian/postinst | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 95 insertions(+)
>   create mode 100755 debian/postinst
> 
> diff --git a/debian/postinst b/debian/postinst
> new file mode 100755
> index 0000000..c93dd5d
> --- /dev/null
> +++ b/debian/postinst
> @@ -0,0 +1,95 @@
> +#!/bin/sh
> +
> +# Abort if any command returns an error value
> +set -e
> +
> +# This script is called as the last step of the installation of the
> +# package.  All the package's files are in place, dpkg has already
> +# done its automatic conffile handling, and all the packages we depend
> +# of are already fully installed and configured.
> +
> +case "$1" in
> +  configure)
> +    if test -n "$2"; then
> +	if dpkg --compare-versions "$2" 'lt' '6.3-6'; then
> +            echo
> +            perl <<"EOPERL"
> +
> +use warnings;
> +use strict;
> +
> +use PVE::Cluster;
> +use PVE::QemuConfig;
> +use PVE::QemuServer;
> +use PVE::QemuServer::Helpers qw(min_version);
> +
> +print "Pinning Windows VM machine versions to 5.1\n";
> +print "See https://forum.proxmox.com/threads/warning-latest-patch-just-broke-all-my-windows-vms-6-3-4-patch-inside.84915/ for details\n";
> +
> +PVE::Cluster::check_cfs_is_mounted();
> +PVE::Cluster::cfs_update();
> +
> +my $vms = PVE::QemuServer::vmstatus(undef, 1);
> +foreach my $vmid (sort keys %$vms) {
> +    my $running = defined($vms->{$vmid}->{pid}) ? 1 : 0;
> +    eval {
> +        PVE::QemuConfig->lock_config($vmid, sub {
> +            my $conf = PVE::QemuConfig->load_config($vmid);
> +            PVE::QemuConfig->check_lock($conf);
> +
> +            return if !$conf->{ostype} || !PVE::QemuServer::windows_version($conf->{ostype});
> +
> +            if ($running) {
> +                my $cur_machine = $vms->{$vmid}->{'running-machine'};
> +                $cur_machine =~ s/pc(-i440fx|-q35)?-//;
> +                if (min_version($cur_machine, 5, 2)) {
> +                    print "$vmid: [skip] already running with machine version '$cur_machine'\n";
> +                    return;
> +                }
> +            }
> +
> +            if ($conf->{machine} && $conf->{machine} =~ m/^pc-/) {
> +                print "$vmid: [skip] already pinned to '$conf->{machine}'\n";
> +                return;
> +            }
> +
> +            if ($conf->{pending}->{machine}) {
> +                print "$vmid: [skip] machine changed in pending\n";
> +                return;
> +            }
> +
> +            my $cur = $conf->{machine} || "i440fx";
> +            $cur = "i440fx" if $cur eq "pc";
> +            my $new = "pc-$cur-5.1";
> +
> +            my $notice = "";
> +            if ($running) {
> +                $conf->{pending}->{machine} = $new;
> +                $notice = " (after restart)";
> +            } else {
> +                $conf->{machine} = $new;
> +            }
> +
> +            PVE::QemuConfig->write_config($vmid, $conf);
> +            print "$vmid: [ OK ] now pinned to '$new'$notice\n";
> +        });
> +    };
> +    warn "$vmid: [warn] failed to update: $@\n" if $@;
> +}
> +
> +print "If you want to upgrade to the newest version, set the machine back to 'latest' in the GUI, or 'pc'/'q35' on the CLI.\n";
> +
> +EOPERL
> +        echo
> +	fi
> +    fi
> +    ;;
> +
> +  abort-upgrade|abort-remove|abort-deconfigure)
> +    ;;
> +
> +  *) echo "$0: didn't understand being called with \`$1'" 1>&2
> +     exit 0;;
> +esac
> +
> +exit 0
> 

Re: [pve-devel] [PATCH 0/7] Work around QEMU 5.2 windows incompatibility

[Fabian Ebner]

Didn't follow this issue too closely, but what about restoring a 
pre-QEMU-ACPI-patch Windows VM backup in a post-QEMU-ACPI-patch world? 
Those would need to get pinned as well, right? Doing the pinning itself 
on restore shouldn't be difficult, but a good way to detect when it's 
needed is needed.

Am 04.03.21 um 13:52 schrieb Stefan Reiter:
> ...and make machine version configurable in the GUI, to allow users to return to
> the default behaviour of always using the latest one.
> 
> The workaround is based on a postinst script running when upgrading the
> qemu-server package, which sets the machine version of all effected Windows VMs
> to pc-(i440fx|q35)-5.1 to pin them to the last version still using the old ACPI
> table layout.
> 
> Note: the postinst patch requires Fabian's #3301 series:
> https://lists.proxmox.com/pipermail/pve-devel/2021-March/047175.html
> 
> 
> pve-qemu: Stefan Reiter (2):
>    add static supported machines file
>    add ACPI compat patch for 5.1 and older machine types
> 
>   debian/parse-machines.pl                      |  21 ++++
>   ...restore-device-paths-for-pre-5.1-vms.patch | 108 ++++++++++++++++++
>   debian/patches/series                         |   1 +
>   debian/rules                                  |   4 +-
>   4 files changed, 133 insertions(+), 1 deletion(-)
>   create mode 100755 debian/parse-machines.pl
>   create mode 100644 debian/patches/extra/0009-i386-acpi-restore-device-paths-for-pre-5.1-vms.patch
> 
> qemu-server: Stefan Reiter (2):
>    api: add Machine module to query machine types
>    add postinst with Windows device incompatibility workaround
> 
>   PVE/API2/Qemu/Machine.pm | 49 +++++++++++++++++++++
>   PVE/API2/Qemu/Makefile   |  2 +-
>   debian/postinst          | 95 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 145 insertions(+), 1 deletion(-)
>   create mode 100644 PVE/API2/Qemu/Machine.pm
>   create mode 100755 debian/postinst
> 
> manager: Stefan Reiter (3):
>    api: register Qemu::Machine call
>    ui: create MachineEdit window
>    ui: MachineEdit: add option for machine version pinning
> 
>   PVE/API2/Nodes.pm                 |   6 ++
>   www/manager6/Makefile             |   1 +
>   www/manager6/qemu/HardwareView.js |  16 +----
>   www/manager6/qemu/MachineEdit.js  | 107 ++++++++++++++++++++++++++++++
>   4 files changed, 115 insertions(+), 15 deletions(-)
>   create mode 100644 www/manager6/qemu/MachineEdit.js
> 

Re: [pve-devel] [PATCH qemu-server 4/7] add postinst with Windows device incompatibility workaround

[Fabian Ebner]

Am 04.03.21 um 13:52 schrieb Stefan Reiter:
> ...for QEMU 5.2
> 
> Add a postinst with some inline perl (to avoid having to install a
> seperate file to run) that conservatively does its best to set the
> machine version for all VMs the bug might affect to 5.1.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Requires Fabian's #3301 series:
> https://lists.proxmox.com/pipermail/pve-devel/2021-March/047175.html
> 
> I did it as inline perl because I wasn't sure where to put a file containing
> this to be able to execute it from postinst without cluttering the machine after
> it is done. Feel free to extract it somewhere else if you see fit.
> 
> Might also make sense to update the link from the forum for "more details" to
> the cover of this series, or something else...
> 
>   debian/postinst | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 95 insertions(+)
>   create mode 100755 debian/postinst
> 
> diff --git a/debian/postinst b/debian/postinst
> new file mode 100755
> index 0000000..c93dd5d
> --- /dev/null
> +++ b/debian/postinst
> @@ -0,0 +1,95 @@
> +#!/bin/sh
> +
> +# Abort if any command returns an error value
> +set -e
> +
> +# This script is called as the last step of the installation of the
> +# package.  All the package's files are in place, dpkg has already
> +# done its automatic conffile handling, and all the packages we depend
> +# of are already fully installed and configured.
> +
> +case "$1" in
> +  configure)
> +    if test -n "$2"; then
> +	if dpkg --compare-versions "$2" 'lt' '6.3-6'; then
> +            echo
> +            perl <<"EOPERL"
> +
> +use warnings;
> +use strict;
> +
> +use PVE::Cluster;
> +use PVE::QemuConfig;
> +use PVE::QemuServer;
> +use PVE::QemuServer::Helpers qw(min_version);
> +
> +print "Pinning Windows VM machine versions to 5.1\n";
> +print "See https://forum.proxmox.com/threads/warning-latest-patch-just-broke-all-my-windows-vms-6-3-4-patch-inside.84915/ for details\n";
> +
> +PVE::Cluster::check_cfs_is_mounted();
> +PVE::Cluster::cfs_update();
> +
> +my $vms = PVE::QemuServer::vmstatus(undef, 1);
> +foreach my $vmid (sort keys %$vms) {
> +    my $running = defined($vms->{$vmid}->{pid}) ? 1 : 0;
> +    eval {
> +        PVE::QemuConfig->lock_config($vmid, sub {
> +            my $conf = PVE::QemuConfig->load_config($vmid);
> +            PVE::QemuConfig->check_lock($conf);

Is it worth trying to handle e.g. 'suspended' VMs too?

A more general thing: what about snapshots? Do we need to pin the 
version there too? Could be done here or with a hook on rollback, but 
for the latter a mechanism to detect when it's needed is required.

> +
> +            return if !$conf->{ostype} || !PVE::QemuServer::windows_version($conf->{ostype});
> +
> +            if ($running) {
> +                my $cur_machine = $vms->{$vmid}->{'running-machine'};
> +                $cur_machine =~ s/pc(-i440fx|-q35)?-//;
> +                if (min_version($cur_machine, 5, 2)) {
> +                    print "$vmid: [skip] already running with machine version '$cur_machine'\n";
> +                    return;
> +                }
> +            }
> +
> +            if ($conf->{machine} && $conf->{machine} =~ m/^pc-/) {
> +                print "$vmid: [skip] already pinned to '$conf->{machine}'\n";
> +                return;
> +            }
> +
> +            if ($conf->{pending}->{machine}) {
> +                print "$vmid: [skip] machine changed in pending\n";
> +                return;
> +            }
> +
> +            my $cur = $conf->{machine} || "i440fx";
> +            $cur = "i440fx" if $cur eq "pc";
> +            my $new = "pc-$cur-5.1";
> +
> +            my $notice = "";
> +            if ($running) {
> +                $conf->{pending}->{machine} = $new;
> +                $notice = " (after restart)";
> +            } else {
> +                $conf->{machine} = $new;
> +            }
> +
> +            PVE::QemuConfig->write_config($vmid, $conf);
> +            print "$vmid: [ OK ] now pinned to '$new'$notice\n";
> +        });
> +    };
> +    warn "$vmid: [warn] failed to update: $@\n" if $@;
> +}
> +
> +print "If you want to upgrade to the newest version, set the machine back to 'latest' in the GUI, or 'pc'/'q35' on the CLI.\n";
> +
> +EOPERL
> +        echo
> +	fi
> +    fi
> +    ;;
> +
> +  abort-upgrade|abort-remove|abort-deconfigure)
> +    ;;
> +
> +  *) echo "$0: didn't understand being called with \`$1'" 1>&2
> +     exit 0;;
> +esac
> +
> +exit 0
> 

[pve-devel] applied: [PATCH pve-qemu 1/7] add static supported machines file

[Thomas Lamprecht]

On 04.03.21 13:52, Stefan Reiter wrote:
> Same rationale as the CPU flags file, avoids calling QEMU binary just to
> query machine types.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  debian/parse-machines.pl | 21 +++++++++++++++++++++
>  debian/rules             |  4 +++-
>  2 files changed, 24 insertions(+), 1 deletion(-)
>  create mode 100755 debian/parse-machines.pl
> 
>

applied, thanks! Added a followup to output JSON instead.

[pve-devel] applied: [PATCH pve-qemu 2/7] add ACPI compat patch for 5.1 and older machine types

[Thomas Lamprecht]

On 04.03.21 13:52, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> This is required to make the whole deal work, otherwise even setting back the
> machine version to 5.1 or lower won't actually fix the issue.
> 
>  ...restore-device-paths-for-pre-5.1-vms.patch | 108 ++++++++++++++++++
>  debian/patches/series                         |   1 +
>  2 files changed, 109 insertions(+)
>  create mode 100644 debian/patches/extra/0009-i386-acpi-restore-device-paths-for-pre-5.1-vms.patch
> 
>

applied, thanks!

[pve-devel] applied: [PATCH qemu-server 3/7] api: add Machine module to query machine types

[Thomas Lamprecht]

On 04.03.21 13:52, Stefan Reiter wrote:
> The file is provided by pve-qemu-kvm.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  PVE/API2/Qemu/Machine.pm | 49 ++++++++++++++++++++++++++++++++++++++++
>  PVE/API2/Qemu/Makefile   |  2 +-
>  2 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100644 PVE/API2/Qemu/Machine.pm
> 
>

applied, and adapted to new JSON output in followup, thanks!

[pve-devel] NAK: [PATCH qemu-server 4/7] add postinst with Windows device incompatibility workaround

[Thomas Lamprecht]

On 04.03.21 13:52, Stefan Reiter wrote:
> ...for QEMU 5.2
> 
> Add a postinst with some inline perl (to avoid having to install a
> seperate file to run) that conservatively does its best to set the
> machine version for all VMs the bug might affect to 5.1.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Requires Fabian's #3301 series:
> https://lists.proxmox.com/pipermail/pve-devel/2021-March/047175.html
> 
> I did it as inline perl because I wasn't sure where to put a file containing
> this to be able to execute it from postinst without cluttering the machine after
> it is done. Feel free to extract it somewhere else if you see fit.
> 
> Might also make sense to update the link from the forum for "more details" to
> the cover of this series, or something else...
> 
>  debian/postinst | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100755 debian/postinst
> 

Dropped this one, I know partially my fault in making you go this direction, but
Fabi is right, and we even talked about how to handle restore ourself in persona,
IIRC - so no idea how we could even go for this approach (dealing which such effects
of decades old design decisions, which naturally lack the benefit of the hindsight,
is always a PITA).

[pve-devel] applied: [PATCH manager 5/7] api: register Qemu::Machine call

[Thomas Lamprecht]

On 04.03.21 13:52, Stefan Reiter wrote:
> as 'machine-types', so it is clear this refers to QEMU machines, not the
> local machine (as one might think, this being a 'node' API call).
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Requires dependency bump on updated qemu-server.
> 
>  PVE/API2/Nodes.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
>

applied, but moved to a new capabilities endpoint folder, the CPU one should also
move there for 7.0 - maybe others too, there's already way to much going on in the
/nodes/{node} API folder.

[pve-devel] applied: [PATCH manager 6/7] ui: create MachineEdit window

[Thomas Lamprecht]

On 04.03.21 13:52, Stefan Reiter wrote:
> no functional change intended
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  www/manager6/Makefile             |  1 +
>  www/manager6/qemu/HardwareView.js | 16 +------------
>  www/manager6/qemu/MachineEdit.js  | 38 +++++++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+), 15 deletions(-)
>  create mode 100644 www/manager6/qemu/MachineEdit.js
> 
>

applied, thanks!

[pve-devel] applied: [PATCH manager 7/7] ui: MachineEdit: add option for machine version pinning

[Thomas Lamprecht]

On 04.03.21 13:52, Stefan Reiter wrote:
> Hidden behind "Advanced" options, as to not confuse inexperienced users.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  www/manager6/qemu/MachineEdit.js | 69 ++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
>

applied, and reworked quite a bit in followups.

General nit: please try to always keep the `xtype` property as first element in
a widget definition, to make it easier to see what widget it is (name/id/reference
should come after that, then label, then rest with listeners at the end - most of
our code even follows that style).