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

[Fiona Ebner <f.ebner@proxmox.com>]

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.