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 41E3D1FF136 for ; Mon, 23 Mar 2026 08:25:58 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A7B45C046; Mon, 23 Mar 2026 08:26:14 +0100 (CET) Date: Mon, 23 Mar 2026 08:25:37 +0100 From: Arthur Bied-Charreton To: Fiona Ebner Subject: Re: [PATCH qemu-server 7/8] api: qemu: Extend cpu-flags endpoint to return actually supported flags Message-ID: References: <20260312084021.124465-1-a.bied-charreton@proxmox.com> <20260312084021.124465-8-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774250692837 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.854 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: UTVOO276TBBWPNQOOJFH4QRXWXKWTL7W X-Message-ID-Hash: UTVOO276TBBWPNQOOJFH4QRXWXKWTL7W X-MailFrom: a.bied-charreton@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: pve-devel@lists.proxmox.com, 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: 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 > > 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 > > --- > > 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; >