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/[email protected]/
> >
> > Originally-by: Stefan Reiter <[email protected]>
>
> 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 <[email protected]>
> > ---
> > 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;
>
