[pve-devel] [PATCH v2 qemu-server 04/12] Adapt CPUConfig to handle custom models

[Stefan Reiter]

Thanks for the review, I'll prepare a v3 :)

Just some clarifications inline for this patch.

On 10/2/19 7:49 AM, Thomas Lamprecht wrote:
> On 9/30/19 12:58 PM, Stefan Reiter wrote:
>> Turn CPUConfig into a SectionConfig with parsing/writing support for
>> custom CPU models. IO is handled using cfs.
>>
>> The "custom" parameter provides differentiation between custom and
>> default types, even if the name is the same ('namespacing').
>>
>> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
>> ---
>>   PVE/QemuServer/CPUConfig.pm | 59 ++++++++++++++++++++++++++++++++++---
>>   1 file changed, 55 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
>> index c994228..3fde987 100644
>> --- a/PVE/QemuServer/CPUConfig.pm
>> +++ b/PVE/QemuServer/CPUConfig.pm
>> @@ -3,6 +3,8 @@ package PVE::QemuServer::CPUConfig;
>>   use strict;
>>   use warnings;
>>   use PVE::JSONSchema;
>> +use PVE::Cluster qw(cfs_register_file cfs_read_file);
>> +use base qw(PVE::SectionConfig);
>>   
>>   use base 'Exporter';
>>   our @EXPORT_OK = qw(
>> @@ -11,6 +13,15 @@ get_cpu_options
>>   qemu_machine_feature_enabled
>>   );
>>   
>> +my $default_filename = "cpu-models.conf";
>> +cfs_register_file($default_filename,
>> +		  sub { PVE::QemuServer::CPUConfig->parse_config(@_); },
>> +		  sub { PVE::QemuServer::CPUConfig->write_config(@_); });
>> +
>> +sub load_custom_model_conf {
>> +    return cfs_read_file($default_filename);
>> +}
>> +
>>   my $cpu_vendor_list = {
>>       # Intel CPUs
>>       486 => 'GenuineIntel',
>> @@ -80,11 +91,27 @@ my $cpu_flag = qr/[+-](@{[join('|', @supported_cpu_flags)]})/;
>>   
>>   our $cpu_fmt = {
>>       cputype => {
>> -	description => "Emulated CPU type.",
>> +	description => "Emulated CPU type. Can be default or custom name.",
>>   	type => 'string',
>> -	enum => [ sort { "\L$a" cmp "\L$b" } keys %$cpu_vendor_list ],
>> +	format_description => 'string',
>>   	default => 'kvm64',
>>   	default_key => 1,
>> +	optional => 1,
>> +    },
>> +    custom => {
>> +	description => "True if 'cputype' is a custom model, false if otherwise.",
> 
> maybe negate it and call it built-in? We use that as name for Permission roles which
> are generated by code, we probably want to call this also built-in in the UI, and be
> it only to re-use translations ;)
>  >> +	type => 'boolean',
>> +	default => 0,
>> +	optional => 1,
>> +    },
>> +    'reported-model' => {
>> +	description => "CPU model and vendor to report to the guest. Must be a QEMU/KVM supported model."
>> +		     . " Only valid for custom CPU model definitions, default models will always report themselves to the guest OS.",
>> +	type => 'string',
>> +	enum => [ sort { "\L$a" cmp "\L$b" } keys %$cpu_vendor_list ],
> 
> we do not use \L at all in our code, AFAICT, just use the a bit more
> expressive, or at least already used all over the place, lc()
> 

TBH I just moved that part from the existing "cputype" enum, but I can 
change it in v3 - it sure looks clearer with lc().

>> +	default => 'kvm64',
>> +	custom_only => 1,
> .     	^^^^^^^^^^^
> That's a fake JSONSchema attribute, we do not do those normally, either
> some mechanism gets added to JSONSchema to really represent this in general
> or just omit it.
> >> +	optional => 1,
>>       },
>>       hidden => {
>>   	description => "Do not identify as a KVM virtual machine.",
>> @@ -102,14 +129,34 @@ our $cpu_fmt = {
>>       flags => {
>>   	description => "List of additional CPU flags separated by ';'."
>>   		     . " Use '+FLAG' to enable, '-FLAG' to disable a flag."
>> -		     . " Currently supported flags: @{[join(', ', @supported_cpu_flags)]}.",
>> +		     . " Custom CPU models can specify any flag supported by"
>> +		     . " QEMU/KVM, VM-specific flags must be from the following"
>> +		     . " set for security reasons: @{[join(', ', @supported_cpu_flags)]}.",
> 
> is the last part still true if the pattern restriction to $cpu_flag are lifted
> below?
> 

Yes, because it is checked in verify_vm_cpu_conf from patch 06/12. I 
admit it's not entirely clear from just looking at this property 
definition, but it was the only way I could think of to somewhat cleanly 
handle the re-use of $cpu_fmt for VM-specific settings and custom models 
- maybe I'll add a comment.

>>   	format_description => '+FLAG[;-FLAG...]',
>>   	type => 'string',
>> -	pattern => qr/$cpu_flag(;$cpu_flag)*/,
>> +	pattern => qr/[+-][a-zA-Z0-9\-_\.]+(;[+-][a-zA-Z0-9\-_\.]+)*/,
>>   	optional => 1,
>>       },
>>   };
>>   
>> +# Section config settings
>> +my $defaultData = {
>> +    # shallow copy, since SectionConfig modifies propertyList internally
>> +    propertyList => { %$cpu_fmt },
>> +};
>> +
>> +sub private {
>> +    return $defaultData;
>> +}
>> +
>> +sub options {
>> +    return { %$cpu_fmt };
>> +}
>> +
>> +sub type {
>> +    return 'cpu-model';
>> +}
>> +
>>   # Print a QEMU device node for a given VM configuration for hotplugging CPUs
>>   sub print_cpu_device {
>>       my ($conf, $id) = @_;
>> @@ -248,4 +295,8 @@ sub qemu_machine_feature_enabled {
>>   		 $current_minor >= $version_minor);
>>   }
>>   
>> +
>> +__PACKAGE__->register();
>> +__PACKAGE__->init();
>> +
>>   1;
>>
> 
>   
>