Re: [PATCH qemu-server v4 04/17] cpu flags: add helper querying CPU flags with nodes supporting them

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

Am 30.04.26 um 5:59 PM schrieb Arthur Bied-Charreton:
> The get_supported_cpu_flags helper returns a hardcoded list of
> VM-specific flags, without any information on which nodes actually
> support them.
> 
> Add query_available_cpu_flags, which annotates the flags QEMU recognizes
> as `-cpu` arguments with the nodes that actually support them to give an
> accurate picture of which flags will be accepted when starting a VM. Flags
> can be filtered by scope (vm-specific or all) and queried for the
> specific acceleration type (kvm/tcg), which determines which nodes report
> supporting which flag.
> 
> This currently returns early when $arch is set to `aarch64`. This is
> intended for vm-specific flags, since there are none that are currently
> meant to be settable for VMs. For the general flags list, this is a
> limitation due to the fact that it is a lot harder to get a list of
> understood flags at QEMU build time for `aarch64`, as opposed to
> `86_64`, where `-cpu help` will just list all of them. It is left for a

Typo: should be x86_64

> future TODO.
> 
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  src/PVE/QemuServer/CPUFlags.pm | 104 +++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/src/PVE/QemuServer/CPUFlags.pm b/src/PVE/QemuServer/CPUFlags.pm
> index 51d7753e..0bf560b4 100644
> --- a/src/PVE/QemuServer/CPUFlags.pm
> +++ b/src/PVE/QemuServer/CPUFlags.pm
> @@ -14,6 +14,7 @@ our @EXPORT_OK = qw(
>      supported_cpu_flags_names
>      get_supported_cpu_flags
>      query_understood_cpu_flags
> +    query_available_cpu_flags
>  );
>  
>  my $supported_vm_specific_cpu_flags_by_arch = {
> @@ -98,6 +99,10 @@ sub supported_cpu_flags_names() {
>      return sort keys $all_supported_vm_specific_cpu_flags->%*;
>  }
>  
> +sub supported_cpu_flags_names_by_arch($arch) {
> +    return sort map { $_->{name} } $supported_vm_specific_cpu_flags_by_arch->{$arch}->@*;

Again, perlcritic complains here about the "return sort"

> +}
> +
>  sub cpu_flag_supported_re() {
>      return qr/([+-])(@{[join('|', supported_cpu_flags_names())]})/;
>  }
> @@ -123,4 +128,103 @@ sub query_understood_cpu_flags($arch) {
>  
>      return \@flags;
>  }
> +
> +# Return true if `$flag` may be set for a single VM.

Nit: s/for a single VM/for a VM's CPU flags configuration/

> +sub flag_is_vm_specific($flag) {
> +    return defined($all_supported_vm_specific_cpu_flags->{$flag});
> +}
> +
> +# Flag is represented by the `nested-virt` alias
> +sub flag_is_aliased($flag) {

If we add such a helper, it should rather return the actual alias as a
result already.

> +    return $flag eq 'vmx' || $flag eq 'svm';
> +}
> +
> +sub description_by_flag($arch) {

Maybe just flag_descriptions? The current name kinda makes it sound like
I should be passing a flag along.

> +    return { map { $_->{name} => $_->{description} }
> +        $supported_vm_specific_cpu_flags_by_arch->{$arch}->@* };
> +}
> +
> +# Retrieve a list of available flags, i.e., flags that will be accepted as `-cpu` arguments by QEMU
> +# when attempting to spawn a VM. Each flag is returned along with a list of nodes that support it.
> +# Flags that are not supported on any node are also returned, filtering them out is up to the consumer
> +# of this API.
> +#
> +# The $accel parameter (`kvm`|`tcg`) selects which acceleration type flag <> node compatibility should
> +# be evaluated.
> +#
> +# The $vm_specific boolean parameter allows filtering by flag scope. When set to 1, return only
> +# vm-specific flags, otherwise return all flags.
> +#
> +# The $arch parameter (`x86_64`|`aarch64`) specifies which architecture to query flags for. Note that
> +# in both scopes (`vm-specific`,`all`), `aarch64` returns empty lists: this is wanted for `vm-specific`,
> +# as no `aarch64` flags are currently intended to be settable for a specific VM, however this is a
> +# known limitation for `all`. PVE does not currently ship a list of understood flags for `aarch64`,
> +# it is not as trivial to get as for `x86_64`, whose flags are easy to parse out of QEMU's `-cpu help`
> +# output.
> +#
> +# In order to get an accurate picture of which flags can actually be used, we combine two sources (see
> +# PVE::QemuServer::query_supported_cpu_flags for details):
> +#
> +# 1. The understood CPU flags, i.e., all flags QEMU accepts as `-cpu` arguments in principle, regardless
> +#    whether the host CPU actually supports them.
> +# 2. The supported CPU flags: the flags the host CPU actually supports, cached in the node KV store by
> +#    `pvestatd`. This is node-specific.
> +#
> +# Each flag from 1. is annotated with the subset of nodes from 2. that report supporting it.

Style nit: some lines are too long

Nit: if not too much effort, it would be nice to use POD for such
documentation like we started doing elsewhere

> +sub query_available_cpu_flags($accel, $vm_specific, $arch) {
> +    # TODO: Find out a way to get supported flags for aarch64. This is not done because PVE
> +    # does not currently ship a list of understood flags for aarch64, as it's more difficult
> +    # to obtain during QEMU build - for x86_64, qemu -cpu help will just list the flags.
> +    return [] if $arch eq 'aarch64';
> +
> +    my $descriptions = description_by_flag($arch);
> +    my $base =
> +        !$vm_specific
> +        ? query_understood_cpu_flags($arch)
> +        : [supported_cpu_flags_names_by_arch($arch)];
> +    my $available_flags = {
> +        map {
> +            $_ => { name => $_, 'supported-on' => {}, description => $descriptions->{$_} }
> +        } @$base
> +    };
> +
> +    my $kv_store = $accel eq 'kvm' ? 'cpuflags-kvm' : 'cpuflags-tcg';

Nit: could be "cpuflags-$accel"

> +    my $flags = PVE::Cluster::get_node_kv($kv_store);
> +
> +    $available_flags->{'nested-virt'} = {
> +        name => 'nested-virt',
> +        'supported-on' => {},
> +        description => $descriptions->{'nested-virt'},
> +    };
> +
> +    my $add_flag = sub($node, $name) {
> +        return if !defined($available_flags->{$name});
> +        $available_flags->{$name}->{'supported-on'}->{$node} = 1;
> +    };
> +
> +    for my $node (keys %$flags) {
> +        # This depends on `pvestatd` storing the flags in space-separated format, which is the case
> +        # at the time of this commit.
> +        for (split(' ', $flags->{$node})) {
> +            my $aliased = flag_is_aliased($_);
> +            next if $vm_specific && !flag_is_vm_specific($_) && !$aliased;
> +            $add_flag->($node, $_) if !($vm_specific && $aliased);
> +            $add_flag->($node, 'nested-virt') if $aliased;
> +        }
> +    }
> +
> +    for my $flag (values %$available_flags) {
> +        $flag->{'supported-on'} = [sort keys %{ $flag->{'supported-on'} }];

Style nit: prefer using ->%* in such cases for dereference

> +    }
> +
> +    # Make sure 'nested-virt' is always shown first.
> +    my $nested_virt = delete $available_flags->{'nested-virt'};
> +    my @sorted = sort {
> +        (scalar(@{ $a->{'supported-on'} }) == 0) <=> (scalar(@{ $b->{'supported-on'} }) == 0)

Style nit: prefer using ->@* in such cases for dereference

There could be a comment that we are ordering flags that are not
supported anywhere to the end, to avoid people needing to interpret the
code each time.

> +            || $a->{name} cmp $b->{name}
> +    } values %$available_flags;
> +
> +    return [defined($nested_virt) ? ($nested_virt, @sorted) : @sorted];
> +}
> +
>  1;