[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