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

[Fabian Grünbichler]

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..

> 
> 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.

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

> +	},
> +	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)

> +	},
> +	'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?

there are two more from the regular one that we might want to include in 
custom ones as well:

hidden
hv-vendor-id

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.

> +};
> +
> +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
> 
>