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

[pve-devel] [PATCH v6 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 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>
---
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

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

 PVE/QemuServer.pm           | 14 +++++++++++++-
 PVE/QemuServer/CPUConfig.pm | 24 ++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 2063e66..e610ba0 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,18 @@ sub config_to_command {
     }
 
     if ($conf->{bios} && $conf->{bios} eq 'ovmf') {
+	my $cputype;
+
+	if ($conf->{cpu}) {
+	    my $cpu = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $conf->{cpu})
+		or die "Cannot parse cpu description: $conf->{cpu}\n";
+
+	    $cputype = $cpu->{cputype};
+	}
+
+	die "OVMF (UEFI) BIOS is not supported on 32-bit CPU types\n"
+	    if get_cpu_bitness($cputype, $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..6c2700e 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,18 @@ sub get_cpu_from_running_vm {
     return $1;
 }
 
+sub get_cpu_bitness {
+    my ($cputype, $arch) = @_;
+
+    die "missing 'arch'\n" if !$arch;
+    $cputype = $cpu_fmt->{'cputype'}->{'default'} if !$cputype;
+
+    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 v6 qemu-server] Prevent starting a 32-bit VM using a 64-bit OVMF BIOS

[Fiona Ebner]

Am 14.12.23 um 12:09 schrieb Filip Schauer:> @@ -3689,6 +3689,18 @@ sub
config_to_command {
>      }
>  
>      if ($conf->{bios} && $conf->{bios} eq 'ovmf') {
> +	my $cputype;
> +
> +	if ($conf->{cpu}) {
> +	    my $cpu = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $conf->{cpu})
> +		or die "Cannot parse cpu description: $conf->{cpu}\n";
> +
> +	    $cputype = $cpu->{cputype};
> +	}
> +
> +	die "OVMF (UEFI) BIOS is not supported on 32-bit CPU types\n"
> +	    if get_cpu_bitness($cputype, $arch) == 32;
> +

If $forcecpu is set, we should probably just skip the check.

> @@ -719,6 +731,18 @@ sub get_cpu_from_running_vm {
>      return $1;
>  }
>  
> +sub get_cpu_bitness {
> +    my ($cputype, $arch) = @_;
> +

Sorry, I only noticed this now, but the cputype can also be a custom-xyz
one. And then you need to look at the reported model for the check
(compare with what print_cpu_device() and get_cpu_options() do). So I
think it's best to pass along the property string and do all the parsing
and figuring out what the actual cpu type is here.

> +    die "missing 'arch'\n" if !$arch;
> +    $cputype = $cpu_fmt->{'cputype'}->{'default'} if !$cputype;

Turns out, the default is actually not this :/ Again, comparing with
get_cpu_options():

    my $cputype = $kvm ? "kvm64" : "qemu64";
    if ($arch eq 'aarch64') {
        $cputype = 'cortex-a57';
    }

Doesn't matter for your function, because all are 64 bit, but maybe we
want to be explicit about it. Seems like the above should become a
helper for getting the default CPU type.

There's two small pre-existing bugs:

1. The schema claims that the default CPU type is 'kvm64' which we've
already seen is not always true.

And print_cpu_device() doesn't use the same default CPU type for aarch64
as get_cpu_options() does.

Now, starting qemu-system-aarch64 with kvm64 or qemu64 CPU type doesn't
work anyways so I think we can just make print_cpu_device() compatible
with get_cpu_options() by using the new helper.

Would be great if you could incorporate that into a patch series.

The following is independent and deserves more discussion:

2. The schema claims that the default for 'kvm' is 1, but
config_to_command() uses:

$kvm //= 1 if is_native($arch);

While print_cpu_device() uses:

my $kvm = $conf->{kvm} // 1;

Hot-plugging with aarch64 doesn't work at the moment anyways:
# FIXME: hot plugging other architectures like our unofficial arch64
support?

so we only need to care about the case when having a different host arch
and emulating an x86_64 VM. But there it does make a difference and
would be a breaking change because the default for hotplug would switch
from 'kvm64' to 'qemu64'. Question is if we care enough. It's not
officially supported to run Proxmox VE on different architectures at the
moment and IMHO it'd be better to break it now (or with Proxmox VE 9)
and be consistent for the future. And if we don't want to break it, add
a comment for it.

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

[Filip Schauer]

I think it makes sense to make the default for $kvm consistent and take
is_native($arch) into account. Since it is not officially supported to
run Proxmox VE on different architectures this breaking change would
likely not hurt.

On 15/12/2023 11:08, Fiona Ebner wrote:
> so we only need to care about the case when having a different host arch
> and emulating an x86_64 VM. But there it does make a difference and
> would be a breaking change because the default for hotplug would switch
> from 'kvm64' to 'qemu64'. Question is if we care enough. It's not
> officially supported to run Proxmox VE on different architectures at the
> moment and IMHO it'd be better to break it now (or with Proxmox VE 9)
> and be consistent for the future. And if we don't want to break it, add
> a comment for it.

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

[Filip Schauer]

Patch series v7 is available:

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

On 14/12/2023 12:09, Filip Schauer wrote:
> 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>
> ---
> 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
>
> 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
>
>   PVE/QemuServer.pm           | 14 +++++++++++++-
>   PVE/QemuServer/CPUConfig.pm | 24 ++++++++++++++++++++++++
>   2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 2063e66..e610ba0 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,18 @@ sub config_to_command {
>       }
>   
>       if ($conf->{bios} && $conf->{bios} eq 'ovmf') {
> +	my $cputype;
> +
> +	if ($conf->{cpu}) {
> +	    my $cpu = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $conf->{cpu})
> +		or die "Cannot parse cpu description: $conf->{cpu}\n";
> +
> +	    $cputype = $cpu->{cputype};
> +	}
> +
> +	die "OVMF (UEFI) BIOS is not supported on 32-bit CPU types\n"
> +	    if get_cpu_bitness($cputype, $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..6c2700e 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,18 @@ sub get_cpu_from_running_vm {
>       return $1;
>   }
>   
> +sub get_cpu_bitness {
> +    my ($cputype, $arch) = @_;
> +
> +    die "missing 'arch'\n" if !$arch;
> +    $cputype = $cpu_fmt->{'cputype'}->{'default'} if !$cputype;
> +
> +    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();
>