On Fri, Mar 20, 2026 at 06:20:38PM +0100, Fiona Ebner wrote:
> Nit: the qemu-server patches should come first, since the UI patches
> depend on them. And it's always good to mention in the cover letter
> which inter-package dependencies there are, i.e. pve-manager needs a
> versioned dependency bump for qemu-server. It's quite obvious in this
> case, but still helps and mostly writing this for the future.
>
Makes sense, will reorder the patches in v2
> The prefix 'qemu' doesn't really add any information here. It should
> rather be something like 'cpu config:'
>
> Am 12.03.26 um 9:39 AM schrieb Arthur Bied-Charreton:
> > Add functionality to lock & write the custom CPU config and some other
> > helpers that will be needed in custom CPU models CRUD endpoints.
> >
> > Original patch:
> > https://lore.proxmox.com/pve-devel/[email protected]/
> >
> > Originally-by: Stefan Reiter <[email protected]>
>
> If you want, you can put a short high-level changelog here to document
> how the patch changed, e.g.:
>
> [AB: split patch into adding helpers and adding API endpoints
> some other change]
>
> This can help follow history better later on.
>
Noted, thanks!
> > Signed-off-by: Arthur Bied-Charreton <[email protected]>
> > ---
> > src/PVE/QemuServer/CPUConfig.pm | 35 ++++++++++++++++++++++++++++++---
> > 1 file changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/PVE/QemuServer/CPUConfig.pm
> > b/src/PVE/QemuServer/CPUConfig.pm
> > index 32ec4954..2b6665a7 100644
> > --- a/src/PVE/QemuServer/CPUConfig.pm
> > +++ b/src/PVE/QemuServer/CPUConfig.pm
> > @@ -6,10 +6,10 @@ use warnings;
> > use JSON;
> >
> > use PVE::JSONSchema qw(json_bool);
> > -use PVE::Cluster qw(cfs_register_file cfs_read_file);
> > +use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file
> > cfs_lock_file);
> > use PVE::ProcFSTools;
> > use PVE::RESTEnvironment qw(log_warn);
> > -use PVE::Tools qw(run_command);
> > +use PVE::Tools qw(run_command lock_file);
> >
> > use PVE::QemuServer::Helpers qw(min_version);
> >
> > @@ -51,6 +51,19 @@ sub load_custom_model_conf {
> > return cfs_read_file($default_filename);
> > }
> >
> > +sub write_custom_model_conf {
> > + my ($conf) = @_;
> > + cfs_write_file($default_filename, $conf);
>
> Pre-existing, but could you add a preparatory commit renaming the
> variable $default_filename to something more telling? Maybe
> $cpu_models_filename?
>
Will do
> > +}
> > +
> > +sub lock_cpu_config {
> > + my ($code) = @_;
> > + cfs_lock_file($default_filename, undef, $code);
> > + if (my $err = $@) {
> > + die $err;
> > + }
>
> Style nit: could also just be
> die $@ if $@;
>
Noted
> > +}
> > +
> > #builtin models : reported-model is mandatory
> > my $builtin_models_by_arch = {
> > x86_64 => {
> > @@ -298,6 +311,19 @@ sub get_supported_cpu_flags {
> > return $supported_cpu_flags_by_arch->{$arch};
> > }
> >
> > +sub description_by_flag {
> > + my ($arch) = @_;
> > + return { map { $_->{name} => $_->{description} }
> > get_supported_cpu_flags($arch)->@* };
> > +}
> > +
> > +sub get_cpu_vendor {
> > + my ($cputype, $arch) = @_;
> > + $arch = $host_arch if !defined($arch);
>
> The $host_arch variable was recently dropped, needs a rebase.
>
Noted
> > + my $retval = $cpu_models_by_arch->{$arch}->{$cputype}
> > + or die "Built-in CPU type '$cputype' does not exist";
> > + return $retval;
> > +}
>
> These are quite small helpers and I do not see a big reason why they
> should be added in a separate commit rather than just directly in the
> commits with their users. It can be okay to add helpers up-front,
> depending on the scenario, but here:
> * 2 of the helpers are for config file handling
> * 2 of the helpers are for cpu model property details, but also quite
> unrelated
> * the helpers are not complex
>
> So I don't see enough semantic grouping that would justify having this
> as a separate patch.
>
Makes sense, will squash helpers into the commits that use them
> > +
> > my $all_supported_cpu_flags = {};
> > for my $arch ($supported_cpu_flags_by_arch->%*) {
> > for my $flag ($supported_cpu_flags_by_arch->{$arch}->@*) {
> > @@ -375,6 +401,7 @@ my $cpu_fmt = {
> > optional => 1,
> > },
> > };
> > +PVE::JSONSchema::register_standard_option('pve-qemu-cpu', $cpu_fmt);
>
> I'd rather have this as a separate commit or squashed into the commit
> with its first user.
>
Noted
> >
> > my $sev_fmt = {
> > type => {
> > @@ -564,6 +591,7 @@ sub write_config {
> >
> > # saved in section header
> > delete $model_conf->{cputype};
> > + $model_conf->{type} = $class->type();
>
> The commit message should at least describe why this is necessary. I
> suppose for writing to actually work? Could also be a separate fix then,
> but its fine to add together with the helper.
>
> > }
Yes, this was in the original commit but I somehow lost it, sorry! Will
be in v2.
> >
> > $class->SUPER::write_config($filename, $cfg);
> > @@ -612,7 +640,8 @@ sub get_cpu_models {
> >
> > my $conf = load_custom_model_conf();
> > for my $custom_model (keys %{ $conf->{ids} }) {
> > - my $reported_model =
> > $conf->{ids}->{$custom_model}->{'reported-model'};
> > + my $model = $conf->{ids}->{$custom_model};
> > + my $reported_model = $model->{'reported-model'};
>
> This change is unrelated to the rest.
>
Will remove it, sorry about that.
> > $reported_model //= $cpu_fmt->{'reported-model'}->{default};
> > my $vendor = $all_cpu_models->{$reported_model};
> > push @$models,
>
