[PATCH qemu-server] cpu config: Move CPU flags utils to CPUConfig module

[PATCH qemu-server] cpu config: Move CPU flags utils to CPUConfig module

[Arthur Bied-Charreton]

...where they fit better alongside other CPU-related utilities.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/QemuServer.pm           | 131 +-------------------------------
 src/PVE/QemuServer/CPUConfig.pm | 131 +++++++++++++++++++++++++++++++-
 2 files changed, 131 insertions(+), 131 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 5d2dbe03..2121f7c8 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -46,8 +46,7 @@ use PVE::SafeSyslog;
 use PVE::Storage;
 use PVE::SysFSTools;
 use PVE::Systemd;
-use PVE::Tools
-    qw(run_command file_read_firstline file_get_contents dir_glob_foreach get_host_arch $IPV6RE);
+use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach $IPV6RE);
 
 use PVE::QMPClient;
 use PVE::QemuConfig;
@@ -2911,134 +2910,6 @@ sub vga_conf_has_spice {
     return $1 || 1;
 }
 
-# To use query_supported_cpu_flags and query_understood_cpu_flags to get flags
-# to use in a QEMU command line (-cpu element), first array_intersect the result
-# of query_supported_ with query_understood_. This is necessary because:
-#
-# a) query_understood_ returns flags the host cannot use and
-# b) query_supported_ (rather the QMP call) doesn't actually return CPU
-#    flags, but CPU settings - with most of them being flags. Those settings
-#    (and some flags, curiously) cannot be specified as a "-cpu" argument.
-#
-# query_supported_ needs to start up to 2 temporary VMs and is therefore rather
-# expensive. If you need the value returned from this, you can get it much
-# cheaper from pmxcfs using PVE::Cluster::get_node_kv('cpuflags-$accel') with
-# $accel being 'kvm' or 'tcg'.
-#
-# pvestatd calls this function on startup and whenever the QEMU/KVM version
-# changes, automatically populating pmxcfs.
-#
-# Returns: { kvm => [ flagX, flagY, ... ], tcg => [ flag1, flag2, ... ] }
-# since kvm and tcg machines support different flags
-#
-sub query_supported_cpu_flags {
-    my ($arch) = @_;
-
-    $arch //= get_host_arch();
-    my $default_machine = PVE::QemuServer::Machine::default_machine_for_arch($arch);
-
-    my $flags = {};
-
-    # FIXME: Once this is merged, the code below should work for ARM as well:
-    # https://lists.nongnu.org/archive/html/qemu-devel/2019-06/msg04947.html
-    die "QEMU/KVM cannot detect CPU flags on ARM (aarch64)\n"
-        if $arch eq "aarch64";
-
-    my $kvm_supported = defined(kvm_version());
-    my $qemu_cmd = PVE::QemuServer::Helpers::get_command_for_arch($arch);
-    my $fakevmid = -1;
-    my $pidfile = PVE::QemuServer::Helpers::vm_pidfile_name($fakevmid);
-
-    # Start a temporary (frozen) VM with vmid -1 to allow sending a QMP command
-    my $query_supported_run_qemu = sub {
-        my ($kvm) = @_;
-
-        my $flags = {};
-        my $cmd = [
-            $qemu_cmd,
-            '-machine',
-            $default_machine,
-            '-display',
-            'none',
-            '-chardev',
-            "socket,id=qmp,path=/var/run/qemu-server/$fakevmid.qmp,server=on,wait=off",
-            '-mon',
-            'chardev=qmp,mode=control',
-            '-pidfile',
-            $pidfile,
-            '-S',
-            '-daemonize',
-        ];
-
-        if (!$kvm) {
-            push @$cmd, '-accel', 'tcg';
-        }
-
-        my $rc = run_command($cmd, noerr => 1, quiet => 0);
-        die "QEMU flag querying VM exited with code " . $rc if $rc;
-
-        eval {
-            my $cmd_result = mon_cmd(
-                $fakevmid,
-                'query-cpu-model-expansion',
-                type => 'full',
-                model => { name => 'host' },
-            );
-
-            my $props = $cmd_result->{model}->{props};
-            foreach my $prop (keys %$props) {
-                next if $props->{$prop} ne '1';
-                # QEMU returns some flags multiple times, with '_', '.' or '-'
-                # (e.g. lahf_lm and lahf-lm; sse4.2, sse4-2 and sse4_2; ...).
-                # We only keep those with underscores, to match /proc/cpuinfo
-                $prop =~ s/\.|-/_/g;
-                $flags->{$prop} = 1;
-            }
-        };
-        my $err = $@;
-
-        # force stop with 10 sec timeout and 'nocheck', always stop, even if QMP failed
-        vm_stop(undef, $fakevmid, 1, 1, 10, 0, 1);
-
-        die $err if $err;
-
-        return [sort keys %$flags];
-    };
-
-    # We need to query QEMU twice, since KVM and TCG have different supported flags
-    PVE::QemuConfig->lock_config(
-        $fakevmid,
-        sub {
-            $flags->{tcg} = eval { $query_supported_run_qemu->(0) };
-            warn "warning: failed querying supported tcg flags: $@\n" if $@;
-
-            if ($kvm_supported) {
-                $flags->{kvm} = eval { $query_supported_run_qemu->(1) };
-                warn "warning: failed querying supported kvm flags: $@\n" if $@;
-            }
-        },
-    );
-
-    return $flags;
-}
-
-# Understood CPU flags are written to a file at 'pve-qemu' compile time
-my $understood_cpu_flag_dir = "/usr/share/kvm";
-
-sub query_understood_cpu_flags {
-    my $arch = get_host_arch();
-    my $filepath = "$understood_cpu_flag_dir/recognized-CPUID-flags-$arch";
-
-    die "Cannot query understood QEMU CPU flags for architecture: $arch (file not found)\n"
-        if !-e $filepath;
-
-    my $raw = file_get_contents($filepath);
-    $raw =~ s/^\s+|\s+$//g;
-    my @flags = split(/\s+/, $raw);
-
-    return \@flags;
-}
-
 # Since commit 277d33454f77ec1d1e0bc04e37621e4dd2424b67 in pve-qemu, smm is not off by default
 # anymore. But smm=off seems to be required when using SeaBIOS and serial display.
 my sub should_disable_smm {
diff --git a/src/PVE/QemuServer/CPUConfig.pm b/src/PVE/QemuServer/CPUConfig.pm
index 32ec4954..e53f0978 100644
--- a/src/PVE/QemuServer/CPUConfig.pm
+++ b/src/PVE/QemuServer/CPUConfig.pm
@@ -9,7 +9,8 @@ use PVE::JSONSchema qw(json_bool);
 use PVE::Cluster qw(cfs_register_file cfs_read_file);
 use PVE::ProcFSTools;
 use PVE::RESTEnvironment qw(log_warn);
-use PVE::Tools qw(run_command);
+use PVE::Tools qw(run_command get_host_arch);
+use PVE::QemuServer::Monitor qw(mon_cmd);
 
 use PVE::QemuServer::Helpers qw(min_version);
 
@@ -579,6 +580,134 @@ sub add_cpu_json_properties {
     return $prop;
 }
 
+# To use query_supported_cpu_flags and query_understood_cpu_flags to get flags
+# to use in a QEMU command line (-cpu element), first array_intersect the result
+# of query_supported_ with query_understood_. This is necessary because:
+#
+# a) query_understood_ returns flags the host cannot use and
+# b) query_supported_ (rather the QMP call) doesn't actually return CPU
+#    flags, but CPU settings - with most of them being flags. Those settings
+#    (and some flags, curiously) cannot be specified as a "-cpu" argument.
+#
+# query_supported_ needs to start up to 2 temporary VMs and is therefore rather
+# expensive. If you need the value returned from this, you can get it much
+# cheaper from pmxcfs using PVE::Cluster::get_node_kv('cpuflags-$accel') with
+# $accel being 'kvm' or 'tcg'.
+#
+# pvestatd calls this function on startup and whenever the QEMU/KVM version
+# changes, automatically populating pmxcfs.
+#
+# Returns: { kvm => [ flagX, flagY, ... ], tcg => [ flag1, flag2, ... ] }
+# since kvm and tcg machines support different flags
+#
+sub query_supported_cpu_flags {
+    my ($arch) = @_;
+
+    $arch //= get_host_arch();
+    my $default_machine = PVE::QemuServer::Machine::default_machine_for_arch($arch);
+
+    my $flags = {};
+
+    # FIXME: Once this is merged, the code below should work for ARM as well:
+    # https://lists.nongnu.org/archive/html/qemu-devel/2019-06/msg04947.html
+    die "QEMU/KVM cannot detect CPU flags on ARM (aarch64)\n"
+        if $arch eq "aarch64";
+
+    my $kvm_supported = defined(PVE::QemuServer::kvm_version());
+    my $qemu_cmd = PVE::QemuServer::Helpers::get_command_for_arch($arch);
+    my $fakevmid = -1;
+    my $pidfile = PVE::QemuServer::Helpers::vm_pidfile_name($fakevmid);
+
+    # Start a temporary (frozen) VM with vmid -1 to allow sending a QMP command
+    my $query_supported_run_qemu = sub {
+        my ($kvm) = @_;
+
+        my $flags = {};
+        my $cmd = [
+            $qemu_cmd,
+            '-machine',
+            $default_machine,
+            '-display',
+            'none',
+            '-chardev',
+            "socket,id=qmp,path=/var/run/qemu-server/$fakevmid.qmp,server=on,wait=off",
+            '-mon',
+            'chardev=qmp,mode=control',
+            '-pidfile',
+            $pidfile,
+            '-S',
+            '-daemonize',
+        ];
+
+        if (!$kvm) {
+            push @$cmd, '-accel', 'tcg';
+        }
+
+        my $rc = run_command($cmd, noerr => 1, quiet => 0);
+        die "QEMU flag querying VM exited with code " . $rc if $rc;
+
+        eval {
+            my $cmd_result = mon_cmd(
+                $fakevmid,
+                'query-cpu-model-expansion',
+                type => 'full',
+                model => { name => 'host' },
+            );
+
+            my $props = $cmd_result->{model}->{props};
+            foreach my $prop (keys %$props) {
+                next if $props->{$prop} ne '1';
+                # QEMU returns some flags multiple times, with '_', '.' or '-'
+                # (e.g. lahf_lm and lahf-lm; sse4.2, sse4-2 and sse4_2; ...).
+                # We only keep those with underscores, to match /proc/cpuinfo
+                $prop =~ s/\.|-/_/g;
+                $flags->{$prop} = 1;
+            }
+        };
+        my $err = $@;
+
+        # force stop with 10 sec timeout and 'nocheck', always stop, even if QMP failed
+        PVE::QemuServer::vm_stop(undef, $fakevmid, 1, 1, 10, 0, 1);
+
+        die $err if $err;
+
+        return [sort keys %$flags];
+    };
+
+    # We need to query QEMU twice, since KVM and TCG have different supported flags
+    PVE::QemuConfig->lock_config(
+        $fakevmid,
+        sub {
+            $flags->{tcg} = eval { $query_supported_run_qemu->(0) };
+            warn "warning: failed querying supported tcg flags: $@\n" if $@;
+
+            if ($kvm_supported) {
+                $flags->{kvm} = eval { $query_supported_run_qemu->(1) };
+                warn "warning: failed querying supported kvm flags: $@\n" if $@;
+            }
+        },
+    );
+
+    return $flags;
+}
+
+# Understood CPU flags are written to a file at 'pve-qemu' compile time
+my $understood_cpu_flag_dir = "/usr/share/kvm";
+
+sub query_understood_cpu_flags {
+    my $arch = get_host_arch();
+    my $filepath = "$understood_cpu_flag_dir/recognized-CPUID-flags-$arch";
+
+    die "Cannot query understood QEMU CPU flags for architecture: $arch (file not found)\n"
+        if !-e $filepath;
+
+    my $raw = PVE::Tools::file_get_contents($filepath);
+    $raw =~ s/^\s+|\s+$//g;
+    my @flags = split(/\s+/, $raw);
+
+    return \@flags;
+}
+
 sub get_cpu_models {
     my ($include_custom, $arch) = @_;
 
-- 
2.47.3

[PATCH pve-manager] cpu config: Update path to query_supported_cpu_flags

[Arthur Bied-Charreton]

Update call site of query_supported_cpu_flags, which has been moved from
QemuServer to the CPUConfig module.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 PVE/Service/pvestatd.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/Service/pvestatd.pm b/PVE/Service/pvestatd.pm
index 98d421f4..854df170 100755
--- a/PVE/Service/pvestatd.pm
+++ b/PVE/Service/pvestatd.pm
@@ -96,7 +96,7 @@ sub update_supported_cpuflags {
     }
     $next_flag_update_time = 0;
 
-    my $supported_cpuflags = eval { PVE::QemuServer::query_supported_cpu_flags() };
+    my $supported_cpuflags = eval { PVE::QemuServer::CPUConfig::query_supported_cpu_flags() };
     warn $@ if $@;
 
     if (
-- 
2.47.3

Re: [PATCH qemu-server] cpu config: Move CPU flags utils to CPUConfig module

[Fiona Ebner]

Am 17.02.26 um 11:51 AM schrieb Arthur Bied-Charreton:
> ...where they fit better alongside other CPU-related utilities.
> 
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>

When sending multiple patches, please always use a cover letter [0].
It's really helpful for tooling like b4 and to keep a better overview.

> ---
>  src/PVE/QemuServer.pm           | 131 +-------------------------------
>  src/PVE/QemuServer/CPUConfig.pm | 131 +++++++++++++++++++++++++++++++-
>  2 files changed, 131 insertions(+), 131 deletions(-)
> 
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 5d2dbe03..2121f7c8 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -46,8 +46,7 @@ use PVE::SafeSyslog;
>  use PVE::Storage;
>  use PVE::SysFSTools;
>  use PVE::Systemd;
> -use PVE::Tools
> -    qw(run_command file_read_firstline file_get_contents dir_glob_foreach get_host_arch $IPV6RE);
> +use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach $IPV6RE);
>  
>  use PVE::QMPClient;
>  use PVE::QemuConfig;
> @@ -2911,134 +2910,6 @@ sub vga_conf_has_spice {
>      return $1 || 1;
>  }
>  
> -# To use query_supported_cpu_flags and query_understood_cpu_flags to get flags
> -# to use in a QEMU command line (-cpu element), first array_intersect the result
> -# of query_supported_ with query_understood_. This is necessary because:
> -#
> -# a) query_understood_ returns flags the host cannot use and
> -# b) query_supported_ (rather the QMP call) doesn't actually return CPU
> -#    flags, but CPU settings - with most of them being flags. Those settings
> -#    (and some flags, curiously) cannot be specified as a "-cpu" argument.
> -#
> -# query_supported_ needs to start up to 2 temporary VMs and is therefore rather
> -# expensive. If you need the value returned from this, you can get it much
> -# cheaper from pmxcfs using PVE::Cluster::get_node_kv('cpuflags-$accel') with
> -# $accel being 'kvm' or 'tcg'.
> -#
> -# pvestatd calls this function on startup and whenever the QEMU/KVM version
> -# changes, automatically populating pmxcfs.
> -#
> -# Returns: { kvm => [ flagX, flagY, ... ], tcg => [ flag1, flag2, ... ] }
> -# since kvm and tcg machines support different flags
> -#
> -sub query_supported_cpu_flags {

If you remove it here, it will require having a versioned Breaks in
qemu-server's d/control towards older pve-manager. I'd keep a wrapper
here, which we can drop in the future.

---snip 8<---

> diff --git a/src/PVE/QemuServer/CPUConfig.pm b/src/PVE/QemuServer/CPUConfig.pm
> index 32ec4954..e53f0978 100644
> --- a/src/PVE/QemuServer/CPUConfig.pm
> +++ b/src/PVE/QemuServer/CPUConfig.pm
> @@ -9,7 +9,8 @@ use PVE::JSONSchema qw(json_bool);
>  use PVE::Cluster qw(cfs_register_file cfs_read_file);
>  use PVE::ProcFSTools;
>  use PVE::RESTEnvironment qw(log_warn);
> -use PVE::Tools qw(run_command);
> +use PVE::Tools qw(run_command get_host_arch);
> +use PVE::QemuServer::Monitor qw(mon_cmd);
>  
>  use PVE::QemuServer::Helpers qw(min_version);
>  
> @@ -579,6 +580,134 @@ sub add_cpu_json_properties {
>      return $prop;
>  }
>  
> +# To use query_supported_cpu_flags and query_understood_cpu_flags to get flags
> +# to use in a QEMU command line (-cpu element), first array_intersect the result
> +# of query_supported_ with query_understood_. This is necessary because:
> +#
> +# a) query_understood_ returns flags the host cannot use and
> +# b) query_supported_ (rather the QMP call) doesn't actually return CPU
> +#    flags, but CPU settings - with most of them being flags. Those settings
> +#    (and some flags, curiously) cannot be specified as a "-cpu" argument.
> +#
> +# query_supported_ needs to start up to 2 temporary VMs and is therefore rather
> +# expensive. If you need the value returned from this, you can get it much
> +# cheaper from pmxcfs using PVE::Cluster::get_node_kv('cpuflags-$accel') with
> +# $accel being 'kvm' or 'tcg'.
> +#
> +# pvestatd calls this function on startup and whenever the QEMU/KVM version
> +# changes, automatically populating pmxcfs.
> +#
> +# Returns: { kvm => [ flagX, flagY, ... ], tcg => [ flag1, flag2, ... ] }
> +# since kvm and tcg machines support different flags
> +#
> +sub query_supported_cpu_flags {
> +    my ($arch) = @_;
> +
> +    $arch //= get_host_arch();
> +    my $default_machine = PVE::QemuServer::Machine::default_machine_for_arch($arch);
> +
> +    my $flags = {};
> +
> +    # FIXME: Once this is merged, the code below should work for ARM as well:
> +    # https://lists.nongnu.org/archive/html/qemu-devel/2019-06/msg04947.html
> +    die "QEMU/KVM cannot detect CPU flags on ARM (aarch64)\n"
> +        if $arch eq "aarch64";
> +
> +    my $kvm_supported = defined(PVE::QemuServer::kvm_version());

Ah, we cannot depend on PVE::QemuServer here, because that is a cyclic
dependency. That helper could be moved to the Helpers module anyways,
but...[continued below]

> +    my $qemu_cmd = PVE::QemuServer::Helpers::get_command_for_arch($arch);
> +    my $fakevmid = -1;
> +    my $pidfile = PVE::QemuServer::Helpers::vm_pidfile_name($fakevmid);
> +
> +    # Start a temporary (frozen) VM with vmid -1 to allow sending a QMP command
> +    my $query_supported_run_qemu = sub {
> +        my ($kvm) = @_;
> +
> +        my $flags = {};
> +        my $cmd = [
> +            $qemu_cmd,
> +            '-machine',
> +            $default_machine,
> +            '-display',
> +            'none',
> +            '-chardev',
> +            "socket,id=qmp,path=/var/run/qemu-server/$fakevmid.qmp,server=on,wait=off",
> +            '-mon',
> +            'chardev=qmp,mode=control',
> +            '-pidfile',
> +            $pidfile,
> +            '-S',
> +            '-daemonize',
> +        ];
> +
> +        if (!$kvm) {
> +            push @$cmd, '-accel', 'tcg';
> +        }
> +
> +        my $rc = run_command($cmd, noerr => 1, quiet => 0);
> +        die "QEMU flag querying VM exited with code " . $rc if $rc;
> +
> +        eval {
> +            my $cmd_result = mon_cmd(
> +                $fakevmid,
> +                'query-cpu-model-expansion',
> +                type => 'full',
> +                model => { name => 'host' },
> +            );
> +
> +            my $props = $cmd_result->{model}->{props};
> +            foreach my $prop (keys %$props) {
> +                next if $props->{$prop} ne '1';
> +                # QEMU returns some flags multiple times, with '_', '.' or '-'
> +                # (e.g. lahf_lm and lahf-lm; sse4.2, sse4-2 and sse4_2; ...).
> +                # We only keep those with underscores, to match /proc/cpuinfo
> +                $prop =~ s/\.|-/_/g;
> +                $flags->{$prop} = 1;
> +            }
> +        };
> +        my $err = $@;
> +
> +        # force stop with 10 sec timeout and 'nocheck', always stop, even if QMP failed
> +        PVE::QemuServer::vm_stop(undef, $fakevmid, 1, 1, 10, 0, 1);

...the vm_stop() call will be harder to deal with. The right module for
it would be the RunState module, but this requires more untangling
first. In particular, vm_stop_cleanup() calls vmconfig_apply_pending()
so that would also need to move to another module first. This is out of
scope for the feature you are working on. I'm also wondering if a
Capabilities::CPU module would not be better fitting than the CPUConfig
too. Let's just keep the functions in QemuServer.pm for now.

> +
> +        die $err if $err;
> +
> +        return [sort keys %$flags];
> +    };
> +
> +    # We need to query QEMU twice, since KVM and TCG have different supported flags
> +    PVE::QemuConfig->lock_config(

You would also need to include the QemuConfig module, but this would not
be a dealbreaker from a first glance.

> +        $fakevmid,
> +        sub {
> +            $flags->{tcg} = eval { $query_supported_run_qemu->(0) };
> +            warn "warning: failed querying supported tcg flags: $@\n" if $@;
> +
> +            if ($kvm_supported) {
> +                $flags->{kvm} = eval { $query_supported_run_qemu->(1) };
> +                warn "warning: failed querying supported kvm flags: $@\n" if $@;
> +            }
> +        },
> +    );
> +
> +    return $flags;
> +}
> +
> +# Understood CPU flags are written to a file at 'pve-qemu' compile time
> +my $understood_cpu_flag_dir = "/usr/share/kvm";
> +
> +sub query_understood_cpu_flags {
> +    my $arch = get_host_arch();
> +    my $filepath = "$understood_cpu_flag_dir/recognized-CPUID-flags-$arch";
> +
> +    die "Cannot query understood QEMU CPU flags for architecture: $arch (file not found)\n"
> +        if !-e $filepath;
> +
> +    my $raw = PVE::Tools::file_get_contents($filepath);
> +    $raw =~ s/^\s+|\s+$//g;
> +    my @flags = split(/\s+/, $raw);
> +
> +    return \@flags;
> +}
> +
>  sub get_cpu_models {
>      my ($include_custom, $arch) = @_;
>  

[0]:
https://lore.proxmox.com/all/176174696524.1776006.10760334798456181474.b4-ty@proxmox.com/