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.rei...@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@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
