[pve-devel] [PATCH v3 qemu-server 1/7] cpuconfig: add new x86-64-vX models

[Fiona Ebner]

Am 22.05.23 um 12:25 schrieb Alexandre Derumier:
> https://lore.kernel.org/all/20210526144038.278899-1-berrange@redhat.com/T/

Can we reference v3 instead?
https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg01592.html

> "
> In 2020, AMD, Intel, Red Hat, and SUSE worked together to define
> three microarchitecture levels on top of the historical x86-64
> baseline:
> 
>   * x86-64:    original x86_64 baseline instruction set
>   * x86-64-v2: vector instructions up to Streaming SIMD
>                Extensions 4.2 (SSE4.2)  and Supplemental
>                Streaming SIMD Extensions 3 (SSSE3), the
>                POPCNT instruction, and CMPXCHG16B
>   * x86-64-v3: vector instructions up to AVX2, MOVBE,
>                and additional bit-manipulation instructions.
>   * x86-64-v4: vector instructions from some of the
>                AVX-512 variants.
> "

The mail goes on, here I'm quoting v3:

> More precisely the models were defined as:
>
>  * x86-64-abi1: close match for Opteron_G1, minus
>                 vme
>  * x86-64-abi2: perfect match for Nehalem
>  * x86-64-abi3: close match of Haswell-noTSX, minus
>                 aes pcid erms invpcid tsc-deadline
>                 x2apic pclmulqdq
>  * x86-64-abi4: close match of Skylake-Server-noTSX-IBRS, minus
>                 spec-ctrl

But what you write below is different:

> This patch add new builtin model derivated from original models,
> to be compatible between intel/amd.
> 
> x86-64-v1 : Derived from Opteron_G1, minus vme
> x86-64-v2 : Derived from Nehalem, -vme,+aes

Why the additional flags? Above says it's exactly Nehalem. And below,
you don't do "-vme".

> min intel: Westmere (because of aes)
> min amd : Opteron_G3
> 
> x86-64-v3 : Derived from Haswell-noTSX, -pcid,-erms,-invpcid,-tsc-deadline,-x2apic,-pclmulqdq,+aes

It should be -aes according to the above.

> min intel: Haswell
> min amd : EPYC_v1
> 
> x86-64-v4 : Derived from Skylake-Server-noTSX-IBRS, -spec-ctrl
> 
> min intel: Skylake
> min amd : EPYC_v4
> 
> (v4 model not yet exposed, because not yet tested, other models have been tested)
> Signed-off-by: Alexandre Derumier <aderumier at odiso.com>
> ---
>  PVE/QemuServer/CPUConfig.pm | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
> index fb0861b..54bbd55 100644
> --- a/PVE/QemuServer/CPUConfig.pm
> +++ b/PVE/QemuServer/CPUConfig.pm
> @@ -31,6 +31,25 @@ sub load_custom_model_conf {
>      return cfs_read_file($default_filename);
>  }
>  
> +my $builtin_models = {
> +    'x86-64-v1' => {
> +	'reported-model' => 'Opteron_G1',

It's unfortunate that we'll report this model and hence also AMD as
vendor even on Intel hosts and vice versa for the other models. We could
set the vendor to the host's vendor (in get_cpu_options() handle getting
the vendor for the built-in models differently), but that's also
strange, because then it would be Opteron_G1 with vendor GenuineIntel :/
So maybe better to just leave it?

> +	flags => "-vme;-svm;-vmx",

Why remove the svm and vmx flags? They are not exposed by us, so a user
cannot even enable them back if needed, but needs to switch to a
different CPU type.

> +    },
> +    'x86-64-v2' => {
> +	'reported-model' => 'Nehalem',
> +	flags => "+aes;-svm;-vmx",
> +    },
> +    'x86-64-v3' => {
> +	'reported-model' => 'Haswell-noTSX',
> +	flags => "+aes;-pcid;-erms;-invpcid;-tsc-deadline;-x2apic;-pclmulqdq;-svm;-vmx",
> +    },
> +#    'x86-64-v4' => {
> +#	'reported-model' => 'Skylake-Server-noTSX-IBRS',
> +#	flags => "+aes;-spec-ctrl;-svm;-vmx",
> +#    },

Even if you didn't test it, should we just take it in? Also, neither the
original mail nor your commit message mention "+aes" for this one.

> +};
> +
>  my $depreacated_cpu_map = {
>      # there never was such a client CPU, so map it to the server one for backward compat
>      'Icelake-Client' => 'Icelake-Server',
> @@ -96,6 +115,9 @@ my $cpu_vendor_list = {
>      kvm64 => 'default',
>      qemu32 => 'default',
>      qemu64 => 'default',
> +    'x86-64-v1' => 'default',
> +    'x86-64-v2' => 'default',
> +    'x86-64-v3' => 'default',


Currently all of the others are actual models we can pass directly to
QEMU/KVM. I'd rather not add these custom built-in ones here. You'll
need to adapt validate_vm_cpu_conf() of course, to also accept the
built-in ones.

Because of adding them here, I can also set them as the 'reported-model'
for a custom CPU in /etc/pve/virtual-guest/cpu-models.conf and parsing
the file will work, but then starting a VM with that custom CPU will
fail with kvm: unable to find CPU model 'x86-64-v1'.

If we'd like to enable using the built-in ones as base for custom CPU
models, we'll need to handle them differently, but I'm not sure we
should until there is enough user demand.

>      max => 'default',
>  };
>  
> @@ -359,7 +381,10 @@ sub print_cpu_device {
>  	    or die "Cannot parse cpu description: $cputype\n";
>  	$cpu = $cpuconf->{cputype};
>  
> -	if (is_custom_model($cpu)) {
> +	if (my $model = $builtin_models->{$cpu}) {
> +	    $cpu = $model->{'reported-model'} // $cpu_fmt->{'reported-model'}->{default};
> +	}

using elsif seems cleaner

> +	if (is_custom_model($cputype)) {
>  	    my $custom_cpu = get_custom_model($cpu);
>  
>  	    $cpu = $custom_cpu->{'reported-model'} // $cpu_fmt->{'reported-model'}->{default};
> @@ -474,7 +499,11 @@ sub get_cpu_options {
>  	    or die "Cannot parse cpu description: $cpu_prop_str\n";
>  
>  	$cputype = $cpu->{cputype};
> -
> +	if (my $model = $builtin_models->{$cputype}) {
> +	    my $model = $builtin_models->{$cputype};
> +	    $cputype = $model->{'reported-model'} // $cpu_fmt->{'reported-model'}->{default};
> +	    $custom_cpu->{flags} = $model->{'flags'};

It's not a custom_cpu, but a built-in one. Please define a new variable
for this instead and pass it to resolve_cpu_flags() below.

> +	}

Again using elsif seems cleaner

>  	if (is_custom_model($cputype)) {
>  	    $custom_cpu = get_custom_model($cputype);
>