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

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

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/