[pve-devel] [PATCH v2 qemu-server 02/12] Add QEMU CPU flag querying helpers

[Thomas Lamprecht]

On 9/30/19 12:58 PM, Stefan Reiter wrote:
> * query_understood_cpu_flags returns all flags that QEMU/KVM knows about
> * query_supported_cpu_flags returns all flags that QEMU/KVM can use on
>   this particular host.
> 
> To get supported flags, a temporary VM is started with QEMU, so we can
> issue the "query-cpu-model-expansion" QMP command. This is how libvirt
> queries supported flags for its "host-passthrough" CPU type.
> query_supported_cpu_flags is thus rather slow and shouldn't be called
> unnecessarily.
> 
> Currently only supports x86_64, because QEMU-aarch64 doesn't provide the
> necessary querying functions.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> 
> query_understood_cpu_flags is currently not used, but will be very useful for
> the later UI part. I think it thematically fits best with this patch though.
> 
> v1 -> v2:
> * Change order of functions and add a single, more useful comment on usage
> * Do s/\.|-/_/g directly in query_understood_cpu_flags, since the other format
>   is useless anyway
> * Add "die" and FIXME for aarch64, since it doesn't support querying atm
>   (still, use get_host_arch()/get_basic_machine_info() for now, so once QEMU
>   supports it, we theoretically just have to remove the "die")
> * Do QMP in extra eval, so we don't die before calling vm_stop
> 
> 
>  PVE/QemuServer.pm | 112 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 70ed910..20c1061 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3531,6 +3531,118 @@ sub get_command_for_arch($) {
>      return $cmd;
>  }
>  
> +# 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
> +#    however.
> +#
> +# query_supported_ needs to start a temporary VM 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').

mentioning that pvestatd broadcasts this in an intelligent way would be nice
above, else one may wonder how/why that can/should be used.

> +#
> +sub query_supported_cpu_flags {
> +    my $flags = [];
> +
> +    my $vmid = -1;

maybe 's/vmid/fakevmid/' ? or tempvmid, virtvmid ?

do underline that it's intended that this cannot exists.

> +    my $pidfile = pidfile_name($vmid);
> +
> +    my ($arch, $default_machine) = get_basic_machine_info();
> +
> +    # FIXME: Once this is merged, the code below should work for ARM without any
> +    # further modifications:
> +    # 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";
> +
> +    PVE::QemuConfig->lock_config($vmid, sub {
> +	# We start a temporary (frozen) VM with vmid -1 to allow us to send a QMP command
> +	my $rc = run_command([
> +	    get_command_for_arch($arch),
> +	    '-machine', $default_machine,
> +	    '-display', 'none',
> +	    '-chardev', "socket,id=qmp,path=/var/run/qemu-server/$vmid.qmp,server,nowait",
> +	    '-mon', 'chardev=qmp,mode=control',
> +	    '-pidfile', $pidfile,
> +	    '-S', '-daemonize'
> +	], noerr => 1, quiet => 0);
> +	return if $rc;
> +
> +	eval {
> +	    my $cmd_result = vm_mon_cmd_nocheck(
> +		$vmid,
> +		'query-cpu-model-expansion',
> +		type => 'full',
> +		model => { name => 'host' }
> +	    );
> +
> +	    my $props = $cmd_result->{model}->{props};
> +	    if (%$props) {

AFAICT, above is not really required, the "for my $prop (%$foo) { }
can handle an empty %$foo

> +		foreach my $prop (keys %$props) {
> +		    push @$flags, $prop if "$props->{$prop}" eq '1';
> +		}
> +	    }
> +	};
> +	my $err = $@;
> +
> +	# force stop with 10 sec timeout and 'nocheck'
> +	# always stop, even if QMP failed
> +	vm_stop(undef, $vmid, 1, 1, 10, 0, 1);

> +
> +	die $err if $err;
> +    });
> +
> +    # QEMU returns some flags multiple times, with '_', '.' or '-' as separator
> +    # (e.g. lahf_lm and lahf-lm; sse4.2, sse4-2 and sse4_2; ...).
> +    # We only keep those with underscores, since they match the ones from
> +    # /proc/cpuinfo (they do the same thing, but we get rid of duplicates).
> +    @$flags = grep {
> +	my $replaced = (my $underscore = $_) =~ s/\.|-/_/g;
> +	!($replaced && grep { $_ eq $underscore } @$flags)
> +    } @$flags;

hmm, the assignmend to the same variable and double use in two grep's
is a bit to much for me to understand in contrast of what you want to
achieve ^^

Why not do it in the for my $prop %$pros loop already? maybe using a
hash (internally) could be easier? It's properties helps your cause here.

my $flags = {};

...

for my $prop (keys %$props) {
    if ($props->{$prop}) {
        # QEMU tells us about the same flag in different way (e.g., sse4.2, sse4-2
        # and sse4_2) just record the underscore separated one, like /proc/cpuinfo uses
        $prop = $prop s/\.|-/_/g;
        $flags->{$prop} = 1;
    }
}


then you do not need to do anything here anymore and if you want to return
an array you can do:

return [ keys %$flags ];

IMO a bit simpler

> +
> +    return $flags;
> +}
> +
> +sub query_understood_cpu_flags {
> +    my $flags = [];
> +    my $flag_section = 0;
> +
> +    my $arch = get_host_arch();
> +
> +    # FIXME: Once the patch mentioned in query_supported_cpu_flags is merged,
> +    # depending on if the ARM version of query-cpu-model-expansion also returns
> +    # non-flags, this function might become irrelevant for ARM entirely.
> +    die "CPU Flag detection only supported on x86_64\n"
> +	if $arch ne "x86_64";
> +
> +    run_command(
> +	[get_command_for_arch($arch), '-cpu', 'help'],
> +	noerr => 1,
> +	quiet => 1,
> +	outfunc => sub {
> +	    my ($line) = @_;
> +
> +	    if ($flag_section) {
> +		return if $line =~ m/^\s*$/;
> +		$line =~ s/^\s*|\s*$//g;
> +		# Convert all flags to underscore format, since that's what
> +		# /proc/cpuinfo and query_supported_cpu_flags use
> +		$line =~ s/\.|-/_/g;
> +		push @$flags, split(/\s+/, $line);
> +	    } elsif ($line =~ m/^\s*Recognized CPUID flags:\s*$/) {
> +		$flag_section = 1;
> +	    }
> +	}
> +    );
> +
> +    return $flags;
> +}
> +
>  sub get_cpu_options {
>      my ($conf, $arch, $kvm, $machine_type, $kvm_off, $kvmver, $winversion, $gpu_passthrough) = @_;
>  
>