From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id D5A8F1FF13E for ; Fri, 20 Mar 2026 18:20:35 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EA9901D11D; Fri, 20 Mar 2026 18:20:49 +0100 (CET) Message-ID: Date: Fri, 20 Mar 2026 18:20:40 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Fiona Ebner Subject: Re: [PATCH qemu-server 7/8] api: qemu: Extend cpu-flags endpoint to return actually supported flags To: Arthur Bied-Charreton , pve-devel@lists.proxmox.com References: <20260312084021.124465-1-a.bied-charreton@proxmox.com> <20260312084021.124465-8-a.bied-charreton@proxmox.com> Content-Language: en-US In-Reply-To: <20260312084021.124465-8-a.bied-charreton@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774027196990 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.005 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: PZCB6NGUBPLBVSEELNA7RS5N5ZW5ARI6 X-Message-ID-Hash: PZCB6NGUBPLBVSEELNA7RS5N5ZW5ARI6 X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Wolfgang Bumiller X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 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 > --- > 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;