[PATCH qemu-server v3 1/2] cpu config: Add 'arch' property to cpu_fmt

[PATCH qemu-server v3 1/2] cpu config: Add 'arch' property to cpu_fmt

[Arthur Bied-Charreton]

Preparatory step for adding support for configuring custom CPU types in
the PVE UI.

Add optional property 'arch' (x86_64|aarch64) to cpu_fmt to allow custom
models to indicate which architecture they belong to, default to x86_64
for backwards compatibility.

Add checks to get_cpu_options and validate_cpu_conf to deny illegal
configs early.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
Changes since v1:
* add checks to validate_cpu_conf + get_cpu_options to fail early on
  misconfigurations
* reuse the pve-qm-cpu-arch standard option for cpu_fmt's arch property
* do not use single quotes for hash keys if not needed
* do not use parentheses for post-ifs

Changes since v2:
* use get_standard_option for getting the pve-qm-cpu-arch standard
  option instead of accessing the arch_desc directly

 src/PVE/QemuServer/CPUConfig.pm | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/PVE/QemuServer/CPUConfig.pm b/src/PVE/QemuServer/CPUConfig.pm
index 32ec4954..a8e146d7 100644
--- a/src/PVE/QemuServer/CPUConfig.pm
+++ b/src/PVE/QemuServer/CPUConfig.pm
@@ -374,6 +374,12 @@ my $cpu_fmt = {
             . " note that doing so will break live migration to CPUs with other values.",
         optional => 1,
     },
+    arch => {
+        %{ PVE::JSONSchema::get_standard_option('pve-qm-cpu-arch') },
+        default => 'x86_64',
+        description => 'The architecture the CPU model belongs to.',
+        optional => 1,
+    },
 };
 
 my $sev_fmt = {
@@ -475,6 +481,13 @@ sub validate_cpu_conf {
     my ($cpu) = @_;
     # required, but can't be forced in schema since it's encoded in section header for custom models
     die "CPU is missing cputype\n" if !$cpu->{cputype};
+
+    if (my $reported_model = $cpu->{'reported-model'}) {
+        my $arch = $cpu->{arch} // $cpu_fmt->{arch}->{default};
+        die "reported model '$reported_model' is not valid for architecture '$arch'\n"
+            if !$cpu_models_by_arch->{$arch}->{$reported_model};
+    }
+
     return $cpu;
 }
 PVE::JSONSchema::register_format('pve-vm-cpu-conf', $cpu_fmt, \&validate_vm_cpu_conf);
@@ -612,6 +625,10 @@ sub get_cpu_models {
 
     my $conf = load_custom_model_conf();
     for my $custom_model (keys %{ $conf->{ids} }) {
+        my $custom_model_arch = $conf->{ids}->{$custom_model}->{arch};
+        $custom_model_arch //= $cpu_fmt->{arch}->{default};
+        next if $custom_model_arch ne $arch;
+
         my $reported_model = $conf->{ids}->{$custom_model}->{'reported-model'};
         $reported_model //= $cpu_fmt->{'reported-model'}->{default};
         my $vendor = $all_cpu_models->{$reported_model};
@@ -854,6 +871,9 @@ sub get_cpu_options {
             $builtin_cpu->{flags} = $model->{'flags'};
         } elsif (is_custom_model($cputype)) {
             $custom_cpu = get_custom_model($cputype);
+            my $custom_cpu_arch = $custom_cpu->{arch} // $cpu_fmt->{arch}->{default};
+            die "custom CPU model has architecture '$custom_cpu_arch', but VM has '$arch'\n"
+                if $custom_cpu_arch ne $arch;
 
             $cputype = $custom_cpu->{'reported-model'} // $cpu_fmt->{'reported-model'}->{default};
             $kvm_off = $custom_cpu->{hidden} if defined($custom_cpu->{hidden});
-- 
2.47.3

[PATCH qemu-server v3 2/2] cpu config: Add tests for arch/reported-model misconfigurations

[Arthur Bied-Charreton]

Test that validate_cpu_conf denies custom CPU models with
a 'reported-model' that does not exist on the specified (or
default) 'arch'.

Test that get_cpu_options denies configurations with custom
CPU model arch not matching the VM arch.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
Changes since v2:
* fix expected error message in custom-cpu-model-wrong-arch.conf,
  got out of sync with actual error message before submitting v2

 src/test/Makefile                             |  5 +-
 .../cfg2cmd/custom-cpu-model-wrong-arch.conf  |  9 +++
 src/test/run_config2command_tests.pl          |  4 ++
 src/test/run_cpu_config_tests.pl              | 57 +++++++++++++++++++
 4 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 src/test/cfg2cmd/custom-cpu-model-wrong-arch.conf
 create mode 100755 src/test/run_cpu_config_tests.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index 2ef9073a..dcfab6d6 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -1,6 +1,6 @@
 all: test
 
-test: test_snapshot test_cfg_to_cmd test_pci_addr_conflicts test_pci_reservation test_qemu_img_convert test_migration test_restore_config test_parse_config
+test: test_snapshot test_cfg_to_cmd test_pci_addr_conflicts test_pci_reservation test_qemu_img_convert test_migration test_restore_config test_parse_config test_cpu_config
 
 test_snapshot: run_snapshot_tests.pl
 	./run_snapshot_tests.pl
@@ -31,6 +31,9 @@ test_restore_config: run_qemu_restore_config_tests.pl
 test_parse_config: run_parse_config_tests.pl
 	./run_parse_config_tests.pl
 
+test_cpu_config: run_cpu_config_tests.pl
+	perl -I../ ./run_cpu_config_tests.pl
+
 .PHONY: clean
 clean:
 	rm -rf MigrationTest/run parse-config-output
diff --git a/src/test/cfg2cmd/custom-cpu-model-wrong-arch.conf b/src/test/cfg2cmd/custom-cpu-model-wrong-arch.conf
new file mode 100644
index 00000000..d1b9b08a
--- /dev/null
+++ b/src/test/cfg2cmd/custom-cpu-model-wrong-arch.conf
@@ -0,0 +1,9 @@
+# TEST: custom CPU model with wrong arch for VM is rejected
+# EXPECT_ERROR: custom CPU model has architecture 'aarch64', but VM has 'x86_64'
+cores: 2
+cpu: custom-aarch64cpu
+name: wrong-arch-cpu
+numa: 0
+ostype: l26
+smbios1: uuid=2ea3f676-dfa5-11e9-ae82-c721e12f3fcd
+sockets: 1
diff --git a/src/test/run_config2command_tests.pl b/src/test/run_config2command_tests.pl
index 4c872d1c..dec07bda 100755
--- a/src/test/run_config2command_tests.pl
+++ b/src/test/run_config2command_tests.pl
@@ -408,6 +408,10 @@ cpu-model: qemu64
 
 cpu-model: alldefault
 
+cpu-model: aarch64cpu
+    reported-model cortex-a76
+    arch aarch64
+
 EOF
         );
     },
diff --git a/src/test/run_cpu_config_tests.pl b/src/test/run_cpu_config_tests.pl
new file mode 100755
index 00000000..d3e587cc
--- /dev/null
+++ b/src/test/run_cpu_config_tests.pl
@@ -0,0 +1,57 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use Test::More;
+
+use PVE::JSONSchema;
+use PVE::QemuServer::CPUConfig;
+
+# Test that validate_cpu_conf rejects a custom CPU model whose reported-model
+# doesn't match the specified arch.
+
+my @tests = (
+    {
+        name => 'x86_64 reported-model with aarch64 arch is rejected',
+        input => 'custom-mycpu,reported-model=qemu64,arch=aarch64',
+        expected_error => qr/reported model 'qemu64' is not valid for architecture 'aarch64'/,
+    },
+    {
+        name => 'aarch64 reported-model with x86_64 arch is rejected',
+        input => 'custom-mycpu,reported-model=cortex-a76,arch=x86_64',
+        expected_error =>
+            qr/reported model 'cortex-a76' is not valid for architecture 'x86_64'/,
+    },
+    {
+        name => 'aarch64 reported-model with default (x86_64) arch is rejected',
+        input => 'custom-mycpu,reported-model=cortex-a76',
+        expected_error =>
+            qr/reported model 'cortex-a76' is not valid for architecture 'x86_64'/,
+    },
+    {
+        name => 'x86_64 reported-model with matching arch is accepted',
+        input => 'custom-mycpu,reported-model=qemu64,arch=x86_64',
+        expected_error => undef,
+    },
+    {
+        name => 'aarch64 reported-model with matching arch is accepted',
+        input => 'custom-mycpu,reported-model=cortex-a76,arch=aarch64',
+        expected_error => undef,
+    },
+);
+
+for my $test (@tests) {
+    eval { PVE::JSONSchema::parse_property_string('pve-cpu-conf', $test->{input}); };
+    my $err = $@;
+
+    if ($test->{expected_error}) {
+        like($err, $test->{expected_error}, $test->{name});
+    } else {
+        is($err, '', $test->{name});
+    }
+}
+
+done_testing();
-- 
2.47.3

Re: [PATCH qemu-server v3 1/2] cpu config: Add 'arch' property to cpu_fmt

[Fiona Ebner]

Please always send a cover-letter if there is more than one patch, see
[0]. This helps with tooling like b4 (e.g. a R-b for the cover-letter
will be picked up for the whole series).

Am 17.02.26 um 3:11 PM schrieb Arthur Bied-Charreton:
> Preparatory step for adding support for configuring custom CPU types in
> the PVE UI.
> 
> Add optional property 'arch' (x86_64|aarch64) to cpu_fmt to allow custom
> models to indicate which architecture they belong to, default to x86_64
> for backwards compatibility.
> 
> Add checks to get_cpu_options and validate_cpu_conf to deny illegal
> configs early.
> 
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>

I still gave a review below, since the comments are worthwhile for the
future too, but I'm really not sure anymore if we actually want this.
Since this was my idea, sorry about that! We'd at least need to also
check in validate_vm_cpu_conf(), since $cpu_fmt is used there too. But
again, I don't think it's worthwhile:

The only use case for an explicit arch for custom models is when the
reported-model cannot uniquely be associated to an arch. In all other
cases, the option is just additional friction. Right now, the only
affected models are 'host' and 'max' models. Supporting mixed arch
clusters is not planned right now AFAIK, so actually just 'max'. And in
practice, we can just associate the model to the arch of the VM its
assigned to. If really needed, users can just define two custom models,
one with flags for x86_64 and one with flags for aarch64. Even if at
some point there is a model name conflict between archs for other
models, it's gonna be very rare that users need both and if they do,
they can just define two custom models.

What do you think?

If we really do see the need to add it (in the future), I think the
default should rather be "arch of the reported-model and if that is not
unique, arch of the VM it's assigned to".

> ---
> Changes since v1:
> * add checks to validate_cpu_conf + get_cpu_options to fail early on
>   misconfigurations
> * reuse the pve-qm-cpu-arch standard option for cpu_fmt's arch property
> * do not use single quotes for hash keys if not needed
> * do not use parentheses for post-ifs
> 
> Changes since v2:
> * use get_standard_option for getting the pve-qm-cpu-arch standard
>   option instead of accessing the arch_desc directly
> 
>  src/PVE/QemuServer/CPUConfig.pm | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/src/PVE/QemuServer/CPUConfig.pm b/src/PVE/QemuServer/CPUConfig.pm
> index 32ec4954..a8e146d7 100644
> --- a/src/PVE/QemuServer/CPUConfig.pm
> +++ b/src/PVE/QemuServer/CPUConfig.pm
> @@ -374,6 +374,12 @@ my $cpu_fmt = {
>              . " note that doing so will break live migration to CPUs with other values.",
>          optional => 1,
>      },
> +    arch => {
> +        %{ PVE::JSONSchema::get_standard_option('pve-qm-cpu-arch') },
> +        default => 'x86_64',
> +        description => 'The architecture the CPU model belongs to.',
> +        optional => 1,
> +    },

Please note that get_standard_option() has an explicit way to specify a
base via its second argument, which is a bit more explicit.

>  };
>  
>  my $sev_fmt = {
> @@ -475,6 +481,13 @@ sub validate_cpu_conf {
>      my ($cpu) = @_;
>      # required, but can't be forced in schema since it's encoded in section header for custom models
>      die "CPU is missing cputype\n" if !$cpu->{cputype};
> +
> +    if (my $reported_model = $cpu->{'reported-model'}) {
> +        my $arch = $cpu->{arch} // $cpu_fmt->{arch}->{default};

We use this logic three times and the $cpu_fmt name is a bit generic,
which both suggest that having a helper function get_custom_cpu_arch()
would be nice.

> +        die "reported model '$reported_model' is not valid for architecture '$arch'\n"
> +            if !$cpu_models_by_arch->{$arch}->{$reported_model};
> +    }
> +
>      return $cpu;
>  }
>  PVE::JSONSchema::register_format('pve-vm-cpu-conf', $cpu_fmt, \&validate_vm_cpu_conf);
> @@ -612,6 +625,10 @@ sub get_cpu_models {
>  
>      my $conf = load_custom_model_conf();
>      for my $custom_model (keys %{ $conf->{ids} }) {
> +        my $custom_model_arch = $conf->{ids}->{$custom_model}->{arch};
> +        $custom_model_arch //= $cpu_fmt->{arch}->{default};
> +        next if $custom_model_arch ne $arch;
> +
>          my $reported_model = $conf->{ids}->{$custom_model}->{'reported-model'};
>          $reported_model //= $cpu_fmt->{'reported-model'}->{default};
>          my $vendor = $all_cpu_models->{$reported_model};
> @@ -854,6 +871,9 @@ sub get_cpu_options {
>              $builtin_cpu->{flags} = $model->{'flags'};
>          } elsif (is_custom_model($cputype)) {
>              $custom_cpu = get_custom_model($cputype);
> +            my $custom_cpu_arch = $custom_cpu->{arch} // $cpu_fmt->{arch}->{default};
> +            die "custom CPU model has architecture '$custom_cpu_arch', but VM has '$arch'\n"
> +                if $custom_cpu_arch ne $arch;
>  
>              $cputype = $custom_cpu->{'reported-model'} // $cpu_fmt->{'reported-model'}->{default};
>              $kvm_off = $custom_cpu->{hidden} if defined($custom_cpu->{hidden});

[0]:
https://lore.proxmox.com/all/176174696524.1776006.10760334798456181474.b4-ty@proxmox.com/