Re: [PATCH qemu-server 6/8] qemu: Add helpers for new custom models endpoints

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

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/20211028114150.3245864-5-s.reiter@proxmox.com/
> > 
> > Originally-by: Stefan Reiter <s.reiter@proxmox.com>
> 
> 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 <a.bied-charreton@proxmox.com>
> > ---
> >  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,
>