[pve-devel] [PATCH qemu-server 4/7] Add CustomCPUConfig for storing/parsing custom CPU models

Stefan Reiter s.reiter at proxmox.com
Mon Sep 9 14:56:21 CEST 2019


Thanks again for the thorough review, I'll try to address everything 
mentioned in v2. Some stuff inline for this patch in particular.

On 9/9/19 11:53 AM, Fabian Grünbichler wrote:
> On September 2, 2019 4:27 pm, Stefan Reiter wrote:
>> Inherits from SectionConfig to provide base parsing infrastructure.
>>
>> Use with helper functions:
>> * config_from_file gives bless'd config
>> * get_model_by_name returns a "formatted" hash for a single CPU model
>> * config_to_file writes changes back
>>
>> File reads are cached in a local hash.
> 
> high-level:
> 
> use cfs_register/write/read_file please (it's our mechanism for handling
> config files on pmxcfs after all ;))
> 
> is there a reason you need a class-like interface here? we usually use
> that if we want to have multiple implementations of a certain interface
> with a common shared code base, but that is not the case here..

As discussed off-list, I will change the code to use the cfs helpers and 
not use 'bless', the former primarily since I'd need to change the 
cluster code anyway (for locking).

> 
>>
>> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
>> ---
>>
>> This will definitely require some sort of versioning mechanism, otherwise CPU
>> definitions could be changed after starting a VM, thus breaking live-migration
>> by starting the migration-target with different parameters.
>>
>> Hints, ideas, recommendations?
> 
> versioning is one possibility (cumbersome with SectionConfig though).
> 
> another would be to retrieve the CPU model from the running Qemu
> instance, and convert that back to a '-cpu ...' string for starting the
> target instance. if nothing hot-pluggable can change the generated -cpu
> string, we could also record that somewhere (or take it from
> /proc/$pid/cmdline) and override the target instance with that.
> 

I like the idea of migrating the "-cpu" data together with the VM. I 
don't *think* hotplugging could change that, since topology is a 
seperate parameter (-smp), so I will probably use the /proc/$pid/cmdline 
version for now, though that is a detail that we can easily change later on.

>>   PVE/QemuServer/CustomCPUConfig.pm | 129 ++++++++++++++++++++++++++++++
>>   PVE/QemuServer/Makefile           |   1 +
>>   2 files changed, 130 insertions(+)
>>   create mode 100644 PVE/QemuServer/CustomCPUConfig.pm
>>
>> diff --git a/PVE/QemuServer/CustomCPUConfig.pm b/PVE/QemuServer/CustomCPUConfig.pm
>> new file mode 100644
>> index 0000000..87ba9e6
>> --- /dev/null
>> +++ b/PVE/QemuServer/CustomCPUConfig.pm
>> @@ -0,0 +1,129 @@
>> +package PVE::QemuServer::CustomCPUConfig;
>> +
>> +use strict;
>> +use warnings;
>> +use PVE::Tools qw(file_get_contents file_set_contents);
>> +
>> +use base qw(PVE::SectionConfig);
>> +
>> +my $defaultData = {
>> +    propertyList => {
>> +	basemodel => {
>> +	    description => "Emulated CPU type to inherit defaults from.",
>> +	    type => 'string',
>> +	    format_description => 'string',
>> +	    default => 'kvm64',
> 
> should have the same pattern/format as QemuServer's $cpu_fmt->{cputype}.
> in other words, it probably makes sense to extract and re-use that ;)
> 

The formats in this version of the patch are not *quite* the same, hence 
the seperation (e.g. $cpu_fmt doesn't support arbitrary flags but rather 
a whitelisted set). I'll see what I can do for v2, I agree that it would 
be nicer.

>> +	},
>> +	flags => {
>> +	    description => "List of additional CPU flags separated by ';'."
>> +			 . " Use '+FLAG' to enable, '-FLAG' to disable a flag."
>> +			 . " Supports all flags supported by QEMU/KVM.",
>> +	    format_description => '+FLAG[;-FLAG...]',
>> +	    type => 'string',
>> +	    pattern => qr/[+-][a-zA-Z0-9\-_\.]+(;[+-][a-zA-Z0-9\-_\.]+)*/,
> 
> same here (different description, please just use a single definition
> for both instances)
> 
>> +	    optional => 1,
>> +	},
>> +	'phys-bits' => {
>> +	    type => 'integer',
>> +	    minimum => 1,
>> +	    maximum => 64,
>> +	    optional => 1,
>> +	    description => "The physical memory bits that are reported to the guest OS. Must be smaller or equal to the host.",
> 
> this one might make sense to also allow in the regular, per-VM 'cpu'
> option (and then we could again re-use the definition)
> 

I can add that in, but I think this would be an API/CLI only thing, I 
don't think this is a common enough issue to confuse the 'average' GUI 
user with.

>> +	},
>> +	'host-phys-bits' => {
>> +	    type => 'boolean',
>> +	    default => 0,
>> +	    description => "Whether to report the host's physical memory bits. Overrides 'phys-bits' when set.",
>> +	},
> 
> same
> 
>> +	vendor => {
>> +	    type => 'string',
>> +	    enum => [qw(default AuthenticAMD GenuineIntel)],
>> +	    default => 'default',
>> +	    description => "The CPU vendor to report. 'default' uses the host's.",
>> +	},
>> +    }
> 
> this probably only makes sense for the custom types. the default value
> means that you never inherit the vendor from your basemodel - is that
> intended?

I think that should be a different parameter though, maybe 'host' and 
'base' instead of 'default' to clear things up.

> 
> there are two more from the regular one that we might want to include in
> custom ones as well:
> 
> hidden
> hv-vendor-id

Agreed.

> 
> I am not sure whether just moving some format definitions, or moving
> all/most CPU related stuff to a new module (e.g.,
> QemuServer/CPUConfig.pm instead of QemuServer/CustomCPUConfig.pm) is
> more clean / feasible. the latter probably would require moving some
> helpers from QemuServer.pm to another new module as well, as they'd be
> used by the CPU module and QemuServer.pm
> 
> I think we all agree that QemuServer.pm is way too big, so a patch
> (series) that adds new features and reduces that bloat would be more
> than welcome :D but if you think it's too much for this series, we can
> also do it as a follow-up since it's mostly about moving code and format
> definitions.
> 

I like the idea, but I'd rather not have that in this series. I'm trying 
to separate the code belonging to these patches anyway, but moving other 
code seems unfitting.

>> +};
>> +
>> +sub private {
>> +    return $defaultData;
>> +}
>> +
>> +sub options {
>> +    return {
>> +	basemodel => { optional => 1 },
>> +	flags => { optional => 1 },
>> +	'phys-bits' => { optional => 1 },
>> +	'host-phys-bits' => { optional => 1 },
>> +	vendor => { optional => 1 }
>> +    };
>> +}
>> +
>> +sub type {
>> +    return 'cpu-model';
>> +}
>> +
>> +# helpers for reading/parsing and writing/formatting of config files
>> +
>> +my $default_filename = "/etc/pve/cpu-models.conf";
>> +my $cfg_cache = {};
>> +
>> +sub config_from_file {
>> +    my ($filename) = @_;
>> +
>> +    $filename //= $default_filename;
>> +
>> +    if (my $cached = $cfg_cache->{$filename}) {
>> +	return $cached;
>> +    }
>> +
>> +    return undef if ! -e $filename;
>> +
>> +    my $raw = file_get_contents($filename);
>> +    my $cfg = __PACKAGE__->parse_config($filename, $raw);
>> +
>> +    bless $cfg, qw(PVE::QemuServer::CustomCPUConfig);
>> +    $cfg_cache->{$filename} = $cfg;
>> +
>> +    return $cfg;
>> +}
>> +
>> +sub config_to_file {
>> +    my ($class, $filename) = @_;
>> +
>> +    $filename //= $default_filename;
>> +
>> +    $cfg_cache->{$filename} = $class;
>> +
>> +    my $raw = $class->write_config($filename);
>> +    file_set_contents($filename, $raw);
>> +}
>> +
>> +# Use this to get a single model in the format described by $defaultData above
>> +# Includes the name as an additional property and sets default values for undefs
>> +# Returns undef for unknown $name
>> +sub get_model_by_name {
>> +    my ($class, $name) = @_;
>> +
>> +    return undef if !defined($class->{ids}->{$name});
>> +
>> +    my $model = {
>> +	name => $name
>> +    };
>> +
>> +    for my $property (keys %{$defaultData->{propertyList}}) {
>> +	next if $property eq 'type';
>> +
>> +	if (my $value = $class->{ids}->{$name}->{$property}) {
>> +	    $model->{$property} = $value;
>> +	} elsif (my $default = $defaultData->{propertyList}->{$property}->{default}) {
>> +	    $model->{$property} = $default;
>> +	}
>> +    }
>> +
>> +    return $model;
>> +}
>> +
>> +__PACKAGE__->register();
>> +__PACKAGE__->init();
>> +
>> +1;
>> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
>> index afc39a3..41b4cb6 100644
>> --- a/PVE/QemuServer/Makefile
>> +++ b/PVE/QemuServer/Makefile
>> @@ -5,6 +5,7 @@ SOURCES=PCI.pm		\
>>   	OVF.pm		\
>>   	Cloudinit.pm	\
>>   	Agent.pm	\
>> +	CustomCPUConfig.pm \
>>   
>>   .PHONY: install
>>   install: ${SOURCES}
>> -- 
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 




More information about the pve-devel mailing list