Re: [PATCH qemu-server 7/8] api: qemu: Extend cpu-flags endpoint to return actually supported flags

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

On Fri, Mar 20, 2026 at 06:20:40PM +0100, Fiona Ebner wrote:
> Similar comment regarding the 'qemu' prefix as in patch 6/8
> 
> Note that I'm not finished with the review and will continue next week
> with patch 8/8 and a quick look at the UI patches. Still sending this
> already for discussion.
> 
> Am 12.03.26 um 9:40 AM schrieb Arthur Bied-Charreton:
> > Previously the endpoint returned a hardcoded list of flags. It now
> > returns flags that are both recognized by QEMU, and supported by at
> > least one cluster node to give a somewhat accurate picture of what flags
> > can actually be used on the current cluster.
> > 
> > The 'nested-virt` entry is prepended. An optional 'accel' parameter
> > filters results by virtualization type (kvm or tcg) to help avoid
> > misconfigurations when assigning flags to VMs with a specific
> > acceleration setting.
> > 
> > This deviates from the original patch [0] by adding the functionality to
> > the `cpu-flags` endpoint instead of adding new endpoints. This is
> > because we never need the understood/supported flags alone in the
> > frontend, only their intersection. This also improves the VM CPU flag
> > selector by letting users select from all possible flags in their
> > cluster.
> > 
> > When passed `aarch64` as argument for `arch`, the index returns an empty
> > list, which is consistent with the behavior from before this patch.
> > 
> > [0]
> > https://lore.proxmox.com/pve-devel/20211028114150.3245864-3-s.reiter@proxmox.com/
> > 
> > Originally-by: Stefan Reiter <s.reiter@proxmox.com>
> 
> I don't think there is enough left of the original patch/approach in
> this case to keep that trailer.
> 
Noted
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
> >  src/PVE/API2/Qemu/CPUFlags.pm | 108 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 107 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/PVE/API2/Qemu/CPUFlags.pm b/src/PVE/API2/Qemu/CPUFlags.pm
> > index 672bd2d2..9baf6c3e 100644
> > --- a/src/PVE/API2/Qemu/CPUFlags.pm
> > +++ b/src/PVE/API2/Qemu/CPUFlags.pm
> > @@ -10,6 +10,9 @@ use PVE::QemuServer::CPUConfig;
> 
> Missing use statements for PVE::QemuServer and PVE::Cluster
> 
Argh, thanks!
> >  
> >  use base qw(PVE::RESTHandler);
> >  
> > +# vmx/svm are already represented by the nested-virt synthetic entry
> > +my %NESTED_VIRT_ALIASES = map { $_ => 1 } qw(vmx svm);
> 
> I'd prefer a short helper function with a bit more generic name, like
> flag_is_aliased(). And/or maybe have a filter_cpu_flags() function.
> 
> > +
> >  __PACKAGE__->register_method({
> >      name => 'index',
> >      path => '',
> > @@ -21,6 +24,13 @@ __PACKAGE__->register_method({
> >          properties => {
> >              node => get_standard_option('pve-node'),
> >              arch => get_standard_option('pve-qm-cpu-arch', { optional => 1 }),
> > +            accel => {
> > +                description =>
> > +                    'Virtualization type to filter flags by. If not provided, return all flags.',
> > +                type => 'string',
> > +                enum => [qw(kvm tcg)],
> > +                optional => 1,
> > +            },
> >          },
> >      },
> >      returns => {
> > @@ -35,6 +45,13 @@ __PACKAGE__->register_method({
> >                  description => {
> >                      type => 'string',
> >                      description => "Description of the CPU flag.",
> > +                    optional => 1,
> > +                },
> > +                'supported-on' => {
> > +                    description => 'List of nodes supporting the flag.',
> > +                    type => 'array',
> > +                    items => get_standard_option('pve-node'),
> > +                    optional => 1,
> >                  },
> >              },
> >          },
> > @@ -42,10 +59,99 @@ __PACKAGE__->register_method({
> >      code => sub {
> >          my ($param) = @_;
> >  
> > +        my $accel = extract_param($param, 'accel');
> >          my $arch = extract_param($param, 'arch');
> >  
> > -        return PVE::QemuServer::CPUConfig::get_supported_cpu_flags($arch);
> 
> NAK: this endpoint is used for the VM-specific selection right now and
> you can only configure flags that are allow-listed for a VM-specific CPU
> model, i.e. the get_supported_cpu_flags() flags, not all flags. Some
> flags do have security implications for the host too, that's why
> creating custom models with arbitrary flags must be a more privileged
> operation. I also think it would be a bit overkill to have all flags
> there, rather than the ones we deem most useful to expose.
> 
> So the API call should not start providing other flags by default,
> because the non-allow-listed flags cannot be set anyways for a
> VM-specific model. We can add a new API parameter for this, maybe
> "kind/type/something-else?" being an enum with values 'all|vm-specific'
> and by default having 'vm-specific'.
> 
Yes, the UI part of this series filters the flags in the UI, which is
admittedly not very pretty. I like the enum idea, will introduce this in
v2, this will simplify the UI code quite a bit as well. Thanks!
> I don't think there ever is a case where we want all flags regardless of
> accelerator, or is there? If not, we should make 'accel' have a default
> value to avoid the case where it's not provided at all being treated
> differently. I'd opt for 'kvm' as that's the more common use case.
> 
I don't think so either, will add the paramter in v2. I also think `kvm`
is a good default.
> > +        if (defined($arch) && $arch eq 'aarch64') {
> > +            return [];
> 
> We should document this in the description and add a TODO comment for
> the 'all flags' case. Not having any VM-specific flags is intentional
> right now. Not having any usable flags is not. It's just that we don't
> ship a list yet, as it's more difficult to obtain during QEMU build. For
> x86_64, kvm -cpu help will list the flags.
> 
Makes sense, will add a TODO comment and update the endpoint description.
> > +        }
> > +
> > +        my $descriptions = PVE::QemuServer::CPUConfig::description_by_flag($arch);
> 
> We could avoid passing around the descriptions, by having a function
> get_flag_description() and call that where needed. We can also have
> CPUConfig cache it with a hash to avoid re-grepping every time. That
> would feel a bit cleaner to me.
> 
Yes, the reason why I passed it around was to avoid re-grepping, having
it cached in CPUConfig does sound better though.
> > +
> > +        my $nested_virt = {
> > +            name => 'nested-virt',
> > +            description => $descriptions->{'nested-virt'},
> 
> Should we check which hosts do support it? I.e. check for svm|vmx and
> declare support if either is present on a host.
> 
Good call, I made the wrong assumption that they would always be
available - will add a check in v2. 
> > +        };
> > +
> > +        return [$nested_virt, @{ extract_flags($descriptions, $accel) }];
> >      },
> >  });
> >  
> > +# As described here [0], in order to get an accurate picture of which flags can actually be used, we need
> > +# to intersect:
> > +#
> > +# 1. The understood CPU flags, i.e., all flags QEMU accepts in theory, but that may not be actually
> > +#   supported by the host CPU, and
> > +# 2. The supported CPU flags, which returns some settings/flags that cannot be used as `-cpu` arguments.
> > +#
> > +# [0] https://git.proxmox.com/?p=qemu-server.git;a=blob;f=src/PVE/QemuServer.pm;h=09e7a19b2f11ef48d2cfc11837b70338c306817c;hb=refs/heads/master#l2916
> 
> I'd prefer having "See the documentation of query_supported_cpu_flags()
> and query_understood_cpu_flags() for more information." rather than
> putting a web link. Then the reference stays up-to-date with any changes
> there.
> 
Noted, will update the doc comment
> > +sub extract_flags($descriptions, $accel = undef) {
> 
> To me, the 'extract' in the function names here and for the others below
> is rather confusing. The functions take the hash of supported flags
> (currently called $descriptions) and adds more information/other flags.
> The only thing it's extracting is the descriptions.
> 
I will think about better naming, I agree that these are not very
good.
> > +    my $recognized = extract_understood($descriptions);
> > +    my $supported = extract_supported($descriptions, $accel);
> > +
> > +    my %recognized_set = map { $_->{name} => 1 } @$recognized;
> > +
> > +    return [
> > +        map { {
> > +            name => $_->{name},
> > +            'supported-on' => $_->{'supported-on'},
> > +            (defined($_->{description}) ? (description => $_->{description}) : ()),
> > +        } } grep {
> > +            $recognized_set{ $_->{name} }
> > +        } @$supported
> > +    ];
> > +}
> > +
> > +sub extract_understood($descriptions) {
> > +    my $understood_cpu_flags = PVE::QemuServer::query_understood_cpu_flags();
> > +
> > +    return [
> > +        map {
> > +            my $entry = { name => $_ };
> > +            $entry->{description} = $descriptions->{$_} if $descriptions->{$_};
> > +            $entry;
> > +        } grep {
> > +            !$NESTED_VIRT_ALIASES{$_}
> > +        } @$understood_cpu_flags
> > +    ];
> > +}
> > +
> > +# We do not use `PVE::QemuServer::CPUConfig::query_supported_cpu_flags`, which is quite expensive since
> > +# it needs to spawn QEMU instances in order to check which flags are supported. Rather, we use its cached
> > +# output, which is stored by `pvestatd` [0].
> > +#
> > +# [0] https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Service/pvestatd.pm;h=d0719446e3b9a5f1bd3c861dbe768432cb3d7a0e;hb=refs/heads/master#l87
> 
> Again, not a fan of putting a web reference here. I think it's enough to
> mention that there are entries in the node kv store (regularly updated
> by pvestatd).
> 
Noted
> > +sub extract_supported($descriptions, $accel = undef) {
> > +    my %hash;
> > +
> > +    my sub add_flags($flags_by_node) {
> > +        for my $node (keys %$flags_by_node) {
> > +            # This depends on `pvestatd` storing the flags in space-separated format, which is the case
> > +            # at the time of this commit.
> > +            for (split(' ', $flags_by_node->{$node})) {
> > +                if ($hash{$_}) {
> > +                    $hash{$_}->{'supported-on'}->{$node} = 1;
> > +                } else {
> > +                    $hash{$_} = { 'supported-on' => { $node => 1 }, name => $_ };
> > +                }
> > +            }
> > +        }
> > +    }
> 
> I didn't know you could do nested subs directly and not just by
> assigning a closure to a local variable. I'm not sure we have this
> anywhere else in the code base and if there are any gotchas when doing
> it like this.
> 
> @Wolfgang: anything to be aware of with this construct?
> 
> > +
> > +    add_flags(PVE::Cluster::get_node_kv('cpuflags-kvm')) if !defined($accel) || $accel eq 'kvm';
> > +    add_flags(PVE::Cluster::get_node_kv('cpuflags-tcg')) if !defined($accel) || $accel eq 'tcg';
> > +
> > +    return [
> > +        map {
> > +            my $entry = { %$_, 'supported-on' => [sort keys %{ $_->{'supported-on'} }] };
> > +            $entry->{description} = $descriptions->{ $_->{name} }
> > +                if $descriptions->{ $_->{name} };
> > +            $entry;
> > +        } sort {
> > +            $a->{name} cmp $b->{name}
> > +        } grep {
> > +            !$NESTED_VIRT_ALIASES{ $_->{name} }
> > +        } values %hash
> 
> Style nit: we usually write such things in a more mixed way rather than
> purely functional. One advantage is that one doesn't have to read
> bottom-to-top. Opinionated preference, but I think when you have
> multiple instructions inside a map expression, it is a good sign that
> it's better written as a loop.
> 
Noted, will change that
> > +    ];
> > +}
> >  1;
>