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

["DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>]

> 
> 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".
> 
Sorry, indeed the -vme is not done for Nehalem in the patch (wrong
description in the mail message).
For aes see my reply in the other patch.


> 
> > 
> > (v4 model not yet exposed, because not yet tested, other models
> > have been tested)
> > Signed-off-by: Alexandre Derumier <aderumier@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), 
I think it'll break if you migrate between intel/amd host anyway ?

> but that's also
> strange, because then it would be Opteron_G1 with vendor GenuineIntel
> :/
> So maybe better to just leave it?
Well, kvm64 guest have vendor Authentic amd (even on intel host;), with
modelname "common kvm processor")
cat /proc/cpuinfo
vendor_id	: AuthenticAmd
model name	: "Common KVM processor"


If we don't want to expose the original modelname from where we
derivate, afaik, the only way is to patch qemu directly (like in my
v1).



> 
> > +       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.
yes, that's was the idea to forbid user to enable them, as it's
breaking livemigration, so it don't make any sense to use this model
instead host model.

But I can remove them, no problem.

> 
> > +    },
> > +    '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.
> 
Maybe it could be simplier to really add true build-model in qemu ?
(The qemu patch is pretty small, and shouldn't be difficult to
maintain)

I'm not sure, but maybe user will think that it's strange than x86-64-
v2 will display nahelem in guest && in qemu command line ?



> >      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);
> >  
> 
> 
>