[pve-devel] [PATCH-SERIES v7 qemu-server, common] Prevent starting a 32-bit VM using a 64-bit OVMF BIOS

[pve-devel] [PATCH-SERIES v7 qemu-server, common] Prevent starting a 32-bit VM using a 64-bit OVMF BIOS

[Filip Schauer]

This patch series prevents starting a 32-bit VM using a 64-bit OVMF BIOS
and makes the default value for 'kvm' during CPU hotplug consistent with
the rest of the code. This is a breaking change for VMs with a different
CPU architecture running on an x86_64 host.

Changes since v6:
* Skip the CPU bitness check if $forcecpu is set
* Take custom CPU types into account
* Add a helper for getting the default CPU type
* Unify the default value for 'kvm'
* Move is_native from PVE::QemuServer to PVE::Tools

Changes since v5:
* Refactor get_cpu_bitness to die instead of returning undef
* Pass cputype instead of the entire vm config to get_cpu_bitness

Changes since v4:
* Remove parentheses around post-if in get_cpu_bitness

Changes since v3:
* Move the cputypes_32bit list from QemuServer.pm to CPUConfig.pm
* Turn cputypes_32bit into a hash for lookup
* Create a helper get_cpu_bitness function in CPUConfig.pm
* Describe how the list of 32-bit CPU types was obtained

qemu-server:

Filip Schauer (4):
  cpu config: Add helper to get the default CPU type
  Prevent starting a 32-bit VM using a 64-bit OVMF BIOS
  Move is_native from PVE::QemuServer to PVE::Tools
  Unify the default value for 'kvm'

 PVE/QemuServer.pm           | 18 ++++++-------
 PVE/QemuServer/CPUConfig.pm | 53 +++++++++++++++++++++++++++++++------
 PVE/QemuServer/Helpers.pm   | 10 +++++++
 3 files changed, 64 insertions(+), 17 deletions(-)

common:

Filip Schauer (1):
  tools: Add is_native sub to compare the CPU architecture

 src/PVE/Tools.pm | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.39.2

[pve-devel] [PATCH common 1/1] tools: Add is_native sub to compare the CPU architecture

[Filip Schauer]

Add an is_native($arch) subroutine to compare a CPU architecture to the
host CPU architecture. This is brought in from PVE::QemuServer.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/Tools.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 766c809..7bb1809 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -52,6 +52,7 @@ extract_param
 extract_sensitive_params
 file_copy
 get_host_arch
+is_native
 O_PATH
 O_TMPFILE
 AT_EMPTY_PATH
@@ -1841,6 +1842,11 @@ sub get_host_arch {
     return $host_arch;
 }
 
+sub is_native($) {
+    my ($arch) = @_;
+    return get_host_arch() eq $arch;
+}
+
 # Devices are: [ (12 bits minor) (12 bits major) (8 bits minor) ]
 sub dev_t_major($) {
     my ($dev_t) = @_;
-- 
2.39.2

Re: [pve-devel] [PATCH common 1/1] tools: Add is_native sub to compare the CPU architecture

[Fiona Ebner]

Am 19.12.23 um 10:40 schrieb Filip Schauer:
> Add an is_native($arch) subroutine to compare a CPU architecture to the
> host CPU architecture. This is brought in from PVE::QemuServer.
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/PVE/Tools.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 766c809..7bb1809 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -52,6 +52,7 @@ extract_param
>  extract_sensitive_params
>  file_copy
>  get_host_arch
> +is_native
>  O_PATH
>  O_TMPFILE
>  AT_EMPTY_PATH
> @@ -1841,6 +1842,11 @@ sub get_host_arch {
>      return $host_arch;
>  }
>  
> +sub is_native($) {

This is a too generic name to put in such a generic module like Tools.
Admittedly, it's also a too generic name in QemuServer IMHO ;) Maybe
arch_is_native() or is_native_arch()?

I'm not fully convinced the move is worth it, but it does belong to
get_host_arch() semantically, so fine by me.

> +    my ($arch) = @_;
> +    return get_host_arch() eq $arch;
> +}
> +
>  # Devices are: [ (12 bits minor) (12 bits major) (8 bits minor) ]
>  sub dev_t_major($) {
>      my ($dev_t) = @_;

Re: [pve-devel] [PATCH common 1/1] tools: Add is_native sub to compare the CPU architecture

[Filip Schauer]

Patch v8 available:

https://lists.proxmox.com/pipermail/pve-devel/2024-February/061899.html

On 19/02/2024 15:46, Fiona Ebner wrote:
> Am 19.12.23 um 10:40 schrieb Filip Schauer:
>> Add an is_native($arch) subroutine to compare a CPU architecture to the
>> host CPU architecture. This is brought in from PVE::QemuServer.
>>
>> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
>> ---
>>   src/PVE/Tools.pm | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
>> index 766c809..7bb1809 100644
>> --- a/src/PVE/Tools.pm
>> +++ b/src/PVE/Tools.pm
>> @@ -52,6 +52,7 @@ extract_param
>>   extract_sensitive_params
>>   file_copy
>>   get_host_arch
>> +is_native
>>   O_PATH
>>   O_TMPFILE
>>   AT_EMPTY_PATH
>> @@ -1841,6 +1842,11 @@ sub get_host_arch {
>>       return $host_arch;
>>   }
>>   
>> +sub is_native($) {
> This is a too generic name to put in such a generic module like Tools.
> Admittedly, it's also a too generic name in QemuServer IMHO ;) Maybe
> arch_is_native() or is_native_arch()?
>
> I'm not fully convinced the move is worth it, but it does belong to
> get_host_arch() semantically, so fine by me.
>
>> +    my ($arch) = @_;
>> +    return get_host_arch() eq $arch;
>> +}
>> +
>>   # Devices are: [ (12 bits minor) (12 bits major) (8 bits minor) ]
>>   sub dev_t_major($) {
>>       my ($dev_t) = @_;

[pve-devel] [PATCH qemu-server 1/4] cpu config: Add helper to get the default CPU type

[Filip Schauer]

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 PVE/QemuServer/CPUConfig.pm |  9 +++------
 PVE/QemuServer/Helpers.pm   | 10 ++++++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
index ca2946b..c25c2c8 100644
--- a/PVE/QemuServer/CPUConfig.pm
+++ b/PVE/QemuServer/CPUConfig.pm
@@ -5,7 +5,7 @@ use warnings;
 
 use PVE::JSONSchema;
 use PVE::Cluster qw(cfs_register_file cfs_read_file);
-use PVE::QemuServer::Helpers qw(min_version);
+use PVE::QemuServer::Helpers qw(get_default_cpu_type min_version);
 
 use base qw(PVE::SectionConfig Exporter);
 
@@ -405,7 +405,7 @@ sub print_cpu_device {
     my ($conf, $id) = @_;
 
     my $kvm = $conf->{kvm} // 1;
-    my $cpu = $kvm ? "kvm64" : "qemu64";
+    my $cpu = get_default_cpu_type('x86_64', $kvm);
     if (my $cputype = $conf->{cpu}) {
 	my $cpuconf = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $cputype)
 	    or die "Cannot parse cpu description: $cputype\n";
@@ -515,10 +515,7 @@ sub parse_cpuflag_list {
 sub get_cpu_options {
     my ($conf, $arch, $kvm, $kvm_off, $machine_version, $winversion, $gpu_passthrough) = @_;
 
-    my $cputype = $kvm ? "kvm64" : "qemu64";
-    if ($arch eq 'aarch64') {
-	$cputype = 'cortex-a57';
-    }
+    my $cputype = get_default_cpu_type($arch, $kvm);
 
     my $cpu = {};
     my $custom_cpu;
diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
index 0afb631..77052ad 100644
--- a/PVE/QemuServer/Helpers.pm
+++ b/PVE/QemuServer/Helpers.pm
@@ -15,6 +15,7 @@ min_version
 config_aware_timeout
 parse_number_sets
 windows_version
+get_default_cpu_type
 );
 
 my $nodename = PVE::INotify::nodename();
@@ -225,4 +226,13 @@ sub windows_version {
     return $winversion;
 }
 
+sub get_default_cpu_type {
+    my ($arch, $kvm) = @_;
+
+    my $cputype = $kvm ? 'kvm64' : 'qemu64';
+    $cputype = 'cortex-a57' if $arch eq 'aarch64';
+
+    return $cputype;
+}
+
 1;
-- 
2.39.2

Re: [pve-devel] [PATCH qemu-server 1/4] cpu config: Add helper to get the default CPU type

[Fiona Ebner]

Am 19.12.23 um 10:40 schrieb Filip Schauer:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  PVE/QemuServer/CPUConfig.pm |  9 +++------
>  PVE/QemuServer/Helpers.pm   | 10 ++++++++++
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
> index ca2946b..c25c2c8 100644
> --- a/PVE/QemuServer/CPUConfig.pm
> +++ b/PVE/QemuServer/CPUConfig.pm
> @@ -5,7 +5,7 @@ use warnings;
>  
>  use PVE::JSONSchema;
>  use PVE::Cluster qw(cfs_register_file cfs_read_file);
> -use PVE::QemuServer::Helpers qw(min_version);
> +use PVE::QemuServer::Helpers qw(get_default_cpu_type min_version);
>  
>  use base qw(PVE::SectionConfig Exporter);
>  
> @@ -405,7 +405,7 @@ sub print_cpu_device {
>      my ($conf, $id) = @_;
>  
>      my $kvm = $conf->{kvm} // 1;
> -    my $cpu = $kvm ? "kvm64" : "qemu64";
> +    my $cpu = get_default_cpu_type('x86_64', $kvm);

Not a new issue, but I think we should check the configured arch and die
with a clean error when it's not x86_64 (and also move the FIXME up
here), rather than continuing for some time and attempting a hotplug
with some known non-working device commandline.

>      if (my $cputype = $conf->{cpu}) {
>  	my $cpuconf = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $cputype)
>  	    or die "Cannot parse cpu description: $cputype\n";
> @@ -515,10 +515,7 @@ sub parse_cpuflag_list {
>  sub get_cpu_options {
>      my ($conf, $arch, $kvm, $kvm_off, $machine_version, $winversion, $gpu_passthrough) = @_;
>  
> -    my $cputype = $kvm ? "kvm64" : "qemu64";
> -    if ($arch eq 'aarch64') {
> -	$cputype = 'cortex-a57';
> -    }
> +    my $cputype = get_default_cpu_type($arch, $kvm);
>  
>      my $cpu = {};
>      my $custom_cpu;
> diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
> index 0afb631..77052ad 100644
> --- a/PVE/QemuServer/Helpers.pm
> +++ b/PVE/QemuServer/Helpers.pm
> @@ -15,6 +15,7 @@ min_version
>  config_aware_timeout
>  parse_number_sets
>  windows_version
> +get_default_cpu_type
>  );
>  
>  my $nodename = PVE::INotify::nodename();
> @@ -225,4 +226,13 @@ sub windows_version {
>      return $winversion;
>  }
>  
> +sub get_default_cpu_type {

Why put this into the Helpers module rather than CPUConfig? That would
be the most natural choice, because this is precisely concerned with the
CPU config and it's only used there too.

> +    my ($arch, $kvm) = @_;
> +
> +    my $cputype = $kvm ? 'kvm64' : 'qemu64';
> +    $cputype = 'cortex-a57' if $arch eq 'aarch64';
> +
> +    return $cputype;
> +}
> +
>  1;

[pve-devel] [PATCH v7 qemu-server 2/4] Prevent starting a 32-bit VM using a 64-bit OVMF BIOS

[Filip Schauer]

Instead of starting a VM with a 32-bit CPU type and a 64-bit OVMF image,
throw an error before starting the VM telling the user that OVMF is not
supported on 32-bit CPU types.

To obtain a list of 32-bit CPU types, refer to the builtin_x86_defs in
target/i386/cpu.c of QEMU. Exclude any entries that have the long mode
feature (CPUID_EXT2_LM).

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 PVE/QemuServer.pm           |  5 ++++-
 PVE/QemuServer/CPUConfig.pm | 39 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 2063e66..a7b237e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -52,7 +52,7 @@ use PVE::QemuConfig;
 use PVE::QemuServer::Helpers qw(config_aware_timeout min_version windows_version);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
-use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options);
+use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options get_cpu_bitness);
 use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom drive_is_read_only parse_drive print_drive);
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Memory qw(get_current_memory);
@@ -3689,6 +3689,9 @@ sub config_to_command {
     }
 
     if ($conf->{bios} && $conf->{bios} eq 'ovmf') {
+	die "OVMF (UEFI) BIOS is not supported on 32-bit CPU types\n"
+	    if !$forcecpu && get_cpu_bitness($conf->{cpu}, $arch) == 32;
+
 	my ($code_drive_str, $var_drive_str) =
 	    print_ovmf_drive_commandlines($conf, $storecfg, $vmid, $arch, $q35, $version_guard);
 	push $cmd->@*, '-drive', $code_drive_str;
diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
index c25c2c8..abf3198 100644
--- a/PVE/QemuServer/CPUConfig.pm
+++ b/PVE/QemuServer/CPUConfig.pm
@@ -12,6 +12,7 @@ use base qw(PVE::SectionConfig Exporter);
 our @EXPORT_OK = qw(
 print_cpu_device
 get_cpu_options
+get_cpu_bitness
 );
 
 # under certain race-conditions, this module might be loaded before pve-cluster
@@ -57,6 +58,17 @@ my $depreacated_cpu_map = {
     'Icelake-Client-noTSX' => 'Icelake-Server-noTSX',
 };
 
+my $cputypes_32bit = {
+    '486' => 1,
+    'pentium' => 1,
+    'pentium2' => 1,
+    'pentium3' => 1,
+    'coreduo' => 1,
+    'athlon' => 1,
+    'kvm32' => 1,
+    'qemu32' => 1,
+};
+
 my $cpu_vendor_list = {
     # Intel CPUs
     486 => 'GenuineIntel',
@@ -716,6 +728,33 @@ sub get_cpu_from_running_vm {
     return $1;
 }
 
+sub get_cpu_bitness {
+    my ($cpu_prop_str, $arch) = @_;
+
+    die "missing 'arch'\n" if !$arch;
+
+    my $cputype = get_default_cpu_type($arch, 0);
+
+    if ($cpu_prop_str) {
+	my $cpu = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $cpu_prop_str)
+	    or die "Cannot parse cpu description: $cpu_prop_str\n";
+
+	my $cputype = $cpu->{cputype};
+
+	if (my $model = $builtin_models->{$cputype}) {
+	    $cputype = $model->{'reported-model'};
+	} elsif (is_custom_model($cputype)) {
+	    my $custom_cpu = get_custom_model($cputype);
+	    $cputype = $custom_cpu->{'reported-model'} // $cpu_fmt->{'reported-model'}->{default};
+	}
+    }
+
+    return $cputypes_32bit->{$cputype} ? 32 : 64 if $arch eq 'x86_64';
+    return 64 if $arch eq 'aarch64';
+
+    die "unsupported architecture '$arch'\n";
+}
+
 __PACKAGE__->register();
 __PACKAGE__->init();
 
-- 
2.39.2

Re: [pve-devel] [PATCH v7 qemu-server 2/4] Prevent starting a 32-bit VM using a 64-bit OVMF BIOS

[Fiona Ebner]

Am 19.12.23 um 10:40 schrieb Filip Schauer:
> @@ -716,6 +728,33 @@ sub get_cpu_from_running_vm {
>      return $1;
>  }
>  
> +sub get_cpu_bitness {
> +    my ($cpu_prop_str, $arch) = @_;
> +
> +    die "missing 'arch'\n" if !$arch;

The config's 'arch' defaults to the host arch, so we could do the same
here. Then callers can just pass $conf->{arch} without checking if
explicitly set. But this is also fine by me.

> +
> +    my $cputype = get_default_cpu_type($arch, 0);
> +
> +    if ($cpu_prop_str) {
> +	my $cpu = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $cpu_prop_str)
> +	    or die "Cannot parse cpu description: $cpu_prop_str\n";
> +
> +	my $cputype = $cpu->{cputype};
> +
> +	if (my $model = $builtin_models->{$cputype}) {
> +	    $cputype = $model->{'reported-model'};
> +	} elsif (is_custom_model($cputype)) {
> +	    my $custom_cpu = get_custom_model($cputype);
> +	    $cputype = $custom_cpu->{'reported-model'} // $cpu_fmt->{'reported-model'}->{default};
> +	}

Missing the logic for the replacement type, i.e.
	if (my $replacement_type = $depreacated_cpu_map->{$cputype}) {
	    $cputype = $replacement_type;
	}

> +    }
> +
> +    return $cputypes_32bit->{$cputype} ? 32 : 64 if $arch eq 'x86_64';
> +    return 64 if $arch eq 'aarch64';
> +
> +    die "unsupported architecture '$arch'\n";
> +}
> +
>  __PACKAGE__->register();
>  __PACKAGE__->init();
>  

Re: [pve-devel] [PATCH v7 qemu-server 2/4] Prevent starting a 32-bit VM using a 64-bit OVMF BIOS

[Fiona Ebner]

Am 19.02.24 um 15:47 schrieb Fiona Ebner:
>> +
>> +	if (my $model = $builtin_models->{$cputype}) {
>> +	    $cputype = $model->{'reported-model'};
>> +	} elsif (is_custom_model($cputype)) {
>> +	    my $custom_cpu = get_custom_model($cputype);
>> +	    $cputype = $custom_cpu->{'reported-model'} // $cpu_fmt->{'reported-model'}->{default};
>> +	}
> 
> Missing the logic for the replacement type, i.e.
> 	if (my $replacement_type = $depreacated_cpu_map->{$cputype}) {
> 	    $cputype = $replacement_type;
> 	}
> 

Well, I suppose it doesn't matter, because we can expect the replacement
type to have the same bitness.

[pve-devel] [PATCH qemu-server 3/4] Move is_native from PVE::QemuServer to PVE::Tools

[Filip Schauer]

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 PVE/QemuServer.pm | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a7b237e..1a1080d 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -45,7 +45,7 @@ use PVE::RPCEnvironment;
 use PVE::Storage;
 use PVE::SysFSTools;
 use PVE::Systemd;
-use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach get_host_arch $IPV6RE);
+use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach get_host_arch is_native $IPV6RE);
 
 use PVE::QMPClient;
 use PVE::QemuConfig;
@@ -3286,11 +3286,6 @@ sub vga_conf_has_spice {
     return $1 || 1;
 }
 
-sub is_native($) {
-    my ($arch) = @_;
-    return get_host_arch() eq $arch;
-}
-
 sub get_vm_arch {
     my ($conf) = @_;
     return $conf->{arch} // get_host_arch();
-- 
2.39.2

[pve-devel] [PATCH qemu-server 4/4] cpu config: Unify the default value for 'kvm'

[Filip Schauer]

Make the default value for 'kvm' consistent and take into account
whether the VM will run on the same CPU architecture as the host. This
is a breaking change for VMs with a different CPU architecture running
on an x86_64 host, since in this case the default CPU type for
CPU hotplug switches from 'kvm64' to 'qemu64'.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 PVE/QemuServer.pm           | 6 ++++--
 PVE/QemuServer/CPUConfig.pm | 5 +++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1a1080d..b2d71e1 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3815,7 +3815,7 @@ sub config_to_command {
     if ($hotplug_features->{cpu} && min_version($machine_version, 2, 7)) {
 	push @$cmd, '-smp', "1,sockets=$sockets,cores=$cores,maxcpus=$maxcpus";
         for (my $i = 2; $i <= $vcpus; $i++)  {
-	    my $cpustr = print_cpu_device($conf,$i);
+	    my $cpustr = print_cpu_device($conf, $arch, $i);
 	    push @$cmd, '-device', $cpustr;
 	}
 
@@ -4656,10 +4656,12 @@ sub qemu_cpu_hotplug {
     die "vcpus in running vm does not match its configuration\n"
 	if scalar(@{$currentrunningvcpus}) != $currentvcpus;
 
+    my $arch = get_vm_arch($conf);
+
     if (PVE::QemuServer::Machine::machine_version($machine_type, 2, 7)) {
 
 	for (my $i = $currentvcpus+1; $i <= $vcpus; $i++) {
-	    my $cpustr = print_cpu_device($conf, $i);
+	    my $cpustr = print_cpu_device($conf, $arch, $i);
 	    qemu_deviceadd($vmid, $cpustr);
 
 	    my $retry = 0;
diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
index abf3198..38d80ca 100644
--- a/PVE/QemuServer/CPUConfig.pm
+++ b/PVE/QemuServer/CPUConfig.pm
@@ -5,6 +5,7 @@ use warnings;
 
 use PVE::JSONSchema;
 use PVE::Cluster qw(cfs_register_file cfs_read_file);
+use PVE::Tools qw(is_native);
 use PVE::QemuServer::Helpers qw(get_default_cpu_type min_version);
 
 use base qw(PVE::SectionConfig Exporter);
@@ -414,9 +415,9 @@ sub get_custom_model {
 
 # Print a QEMU device node for a given VM configuration for hotplugging CPUs
 sub print_cpu_device {
-    my ($conf, $id) = @_;
+    my ($conf, $arch, $id) = @_;
 
-    my $kvm = $conf->{kvm} // 1;
+    my $kvm = $conf->{kvm} // is_native($arch);
     my $cpu = get_default_cpu_type('x86_64', $kvm);
     if (my $cputype = $conf->{cpu}) {
 	my $cpuconf = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $cputype)
-- 
2.39.2

Re: [pve-devel] [PATCH qemu-server 4/4] cpu config: Unify the default value for 'kvm'

[Fiona Ebner]

Am 19.12.23 um 10:40 schrieb Filip Schauer:
> Make the default value for 'kvm' consistent and take into account
> whether the VM will run on the same CPU architecture as the host. This
> is a breaking change for VMs with a different CPU architecture running
> on an x86_64 host, since in this case the default CPU type for
> CPU hotplug switches from 'kvm64' to 'qemu64'.
> 

On an x86_64 host, for guests using a different architecture (i.e.
aarch64), hot-plugging is already broken, because we try to hotplug a
CPU of type "$cpu-x86_64-cpu,XYZ" which won't work anyways:

vcpus: hotplug problem - VM 130 qmp command 'device_add' failed -
'kvm64-x86_64-cpu' is not a valid device model name

The actual breaking change is for the host arch being something other
than x86_64 (which isn't officially supported) and the VM being x86_64, ...

> @@ -414,9 +415,9 @@ sub get_custom_model {
>  
>  # Print a QEMU device node for a given VM configuration for hotplugging CPUs
>  sub print_cpu_device {
> -    my ($conf, $id) = @_;
> +    my ($conf, $arch, $id) = @_;
>  
> -    my $kvm = $conf->{kvm} // 1;
> +    my $kvm = $conf->{kvm} // is_native($arch);
>      my $cpu = get_default_cpu_type('x86_64', $kvm);

...because in that case, before this patch we got kvm64 here, but with
the patch we get qemu64 which would be a problem for live-migration.

>      if (my $cputype = $conf->{cpu}) {
>  	my $cpuconf = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $cputype)

Re: [pve-devel] [PATCH qemu-server 4/4] cpu config: Unify the default value for 'kvm'

[Filip Schauer]

On 19/02/2024 15:47, Fiona Ebner wrote:
> On an x86_64 host, for guests using a different architecture (i.e.
> aarch64), hot-plugging is already broken, because we try to hotplug a
> CPU of type "$cpu-x86_64-cpu,XYZ" which won't work anyways:
>
> vcpus: hotplug problem - VM 130 qmp command 'device_add' failed -
> 'kvm64-x86_64-cpu' is not a valid device model name
>
> The actual breaking change is for the host arch being something other
> than x86_64 (which isn't officially supported) and the VM being x86_64, ...
>
>> @@ -414,9 +415,9 @@ sub get_custom_model {
>>   
>>   # Print a QEMU device node for a given VM configuration for hotplugging CPUs
>>   sub print_cpu_device {
>> -    my ($conf, $id) = @_;
>> +    my ($conf, $arch, $id) = @_;
>>   
>> -    my $kvm = $conf->{kvm} // 1;
>> +    my $kvm = $conf->{kvm} // is_native($arch);
>>       my $cpu = get_default_cpu_type('x86_64', $kvm);
> ...because in that case, before this patch we got kvm64 here, but with
> the patch we get qemu64 which would be a problem for live-migration.

I expressed my opinion on this matter in the following mail:

https://lists.proxmox.com/pipermail/pve-devel/2023-December/061131.html

Re: [pve-devel] [PATCH qemu-server 4/4] cpu config: Unify the default value for 'kvm'

[Fiona Ebner]

Am 21.02.24 um 16:39 schrieb Filip Schauer:
> On 19/02/2024 15:47, Fiona Ebner wrote:
>> On an x86_64 host, for guests using a different architecture (i.e.
>> aarch64), hot-plugging is already broken, because we try to hotplug a
>> CPU of type "$cpu-x86_64-cpu,XYZ" which won't work anyways:
>>
>> vcpus: hotplug problem - VM 130 qmp command 'device_add' failed -
>> 'kvm64-x86_64-cpu' is not a valid device model name
>>
>> The actual breaking change is for the host arch being something other
>> than x86_64 (which isn't officially supported) and the VM being
>> x86_64, ...
>>
>>> @@ -414,9 +415,9 @@ sub get_custom_model {
>>>     # Print a QEMU device node for a given VM configuration for
>>> hotplugging CPUs
>>>   sub print_cpu_device {
>>> -    my ($conf, $id) = @_;
>>> +    my ($conf, $arch, $id) = @_;
>>>   -    my $kvm = $conf->{kvm} // 1;
>>> +    my $kvm = $conf->{kvm} // is_native($arch);
>>>       my $cpu = get_default_cpu_type('x86_64', $kvm);
>> ...because in that case, before this patch we got kvm64 here, but with
>> the patch we get qemu64 which would be a problem for live-migration.
> 
> I expressed my opinion on this matter in the following mail:
> 
> https://lists.proxmox.com/pipermail/pve-devel/2023-December/061131.html
> 

Yes, I also think the change is fine. But breaking ARM64 guests on a
x86_64 host would not be fine. The point is CPU hotplug already doesn't
work here, so the commit message should be adapted to mention that.

I saw you completely removed the commit message in v8. Should be added
back with the additional information, but that alone doesn't warrant a
v9, can also be done when applying.

Re: [pve-devel] [PATCH qemu-server 4/4] cpu config: Unify the default value for 'kvm'

[Filip Schauer]

On 22/02/2024 10:35, Fiona Ebner wrote:
> Yes, I also think the change is fine. But breaking ARM64 guests on a
> x86_64 host would not be fine. The point is CPU hotplug already doesn't
> work here, so the commit message should be adapted to mention that.
>
> I saw you completely removed the commit message in v8. Should be added
> back with the additional information, but that alone doesn't warrant a
> v9, can also be done when applying.


The commit message was updated here:

https://lists.proxmox.com/pipermail/pve-devel/2024-February/061937.html