[pve-devel] [PATCH v5 qemu-server] Prevent starting a 32-bit VM using a 64-bit OVMF BIOS

[pve-devel] [PATCH v5 qemu-server] 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 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>
---
Changes since v2:
* Simplify the check whether a 32-bit CPU type is used in combination
  with OVMF

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

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

 PVE/QemuServer.pm           |  5 ++++-
 PVE/QemuServer/CPUConfig.pm | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 2063e66..5bfa11f 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 get_cpu_bitness($conf, $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 ca2946b..908e47a 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',
@@ -719,6 +731,26 @@ sub get_cpu_from_running_vm {
     return $1;
 }
 
+sub get_cpu_bitness {
+    my ($conf, $arch) = @_;
+
+    return if !$conf or !$arch;
+
+    if ($arch eq 'x86_64') {
+	if (my $cpu_prop_str = $conf->{cpu}) {
+	    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};
+	    return 32 if $cputypes_32bit->{$cputype};
+	}
+
+	return 64;
+    }
+
+    return 64 if $arch eq 'aarch64';
+}
+
 __PACKAGE__->register();
 __PACKAGE__->init();
 
-- 
2.39.2

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

[Fiona Ebner]

Am 13.12.23 um 17:58 schrieb Filip Schauer:
> @@ -719,6 +731,26 @@ sub get_cpu_from_running_vm {
>      return $1;
>  }
>  
> +sub get_cpu_bitness {

Learned a new word today :)

> +    my ($conf, $arch) = @_;

Please pass either the CPU property string or the CPU type directly
instead of the whole config. Makes it more re-usable and modular.

> +
> +    return if !$conf or !$arch;

There always is an arch and a CPU type, so IMHO, we should make the
caller responsible for passing in something valid. I.e. I'd rather die
than "hide" the issue by returning undef. For the CPU type, we could
also fall back to the default if nothing got passed in (we got access to
$cpu_fmt in the module).

> +
> +    if ($arch eq 'x86_64') {
> +	if (my $cpu_prop_str = $conf->{cpu}) {
> +	    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};
> +	    return 32 if $cputypes_32bit->{$cputype};
> +	}
> +
> +	return 64;
> +    }
> +
> +    return 64 if $arch eq 'aarch64';
> +}

I'd rather die then return undef if it's an unknown arch. Then it will
be more obvious if we forget to extend the helper. Because Perl itself
will not complain if we forget.

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

[Filip Schauer]

Patch v6 available:

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

On 14/12/2023 10:46, Fiona Ebner wrote:
> Am 13.12.23 um 17:58 schrieb Filip Schauer:
>> @@ -719,6 +731,26 @@ sub get_cpu_from_running_vm {
>>       return $1;
>>   }
>>   
>> +sub get_cpu_bitness {
> Learned a new word today :)
>
>> +    my ($conf, $arch) = @_;
> Please pass either the CPU property string or the CPU type directly
> instead of the whole config. Makes it more re-usable and modular.
>
>> +
>> +    return if !$conf or !$arch;
> There always is an arch and a CPU type, so IMHO, we should make the
> caller responsible for passing in something valid. I.e. I'd rather die
> than "hide" the issue by returning undef. For the CPU type, we could
> also fall back to the default if nothing got passed in (we got access to
> $cpu_fmt in the module).
>
>> +
>> +    if ($arch eq 'x86_64') {
>> +	if (my $cpu_prop_str = $conf->{cpu}) {
>> +	    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};
>> +	    return 32 if $cputypes_32bit->{$cputype};
>> +	}
>> +
>> +	return 64;
>> +    }
>> +
>> +    return 64 if $arch eq 'aarch64';
>> +}
> I'd rather die then return undef if it's an unknown arch. Then it will
> be more obvious if we forget to extend the helper. Because Perl itself
> will not complain if we forget.