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

[Fiona Ebner <f.ebner@proxmox.com>]

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.

> 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

>  
>  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'.

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.

> +        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.

> +        }
> +
> +        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.

> +
> +        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.

> +        };
> +
> +        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.

> +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.

> +    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).

> +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.

> +    ];
> +}
>  1;