[pve-devel] [PATCH qemu-server 3/7] Add QEMU CPU flag querying helpers

[Stefan Reiter]

Will implement all of this for v2, just a note inline.

On 9/6/19 1:42 PM, Fabian Grünbichler wrote:
> On September 2, 2019 4:27 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.
>>
>> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
>> ---
>>
>> Changes from RFC:
>>
>> * Clearer regexes
>> * Use existing QMP infrastructure
>> * Add locking for temporary VM start
>> * Add comments
>>
>>
>>   PVE/QemuServer.pm | 84 +++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 84 insertions(+)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 8c519b5..97fa955 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -3500,6 +3500,90 @@ sub get_command_for_arch($) {
>>       return $cmd;
>>   }
>>   
>> +# The format of the flags returned here uses dashes '-' as seperators,
> 
> nit: s/seperator/separator/a
> 
>> +# this is neither the default for 'query_supported_cpu_flags' below, nor for
>> +# /proc/cpuinfo.
>> +#
>> +# To compare (or array_intersect) flags, it's a good idea to convert them all
>> +# to a common format first (e.g. map s/\.|-/_/g).
>> +sub query_understood_cpu_flags {
> 
> not used anywhere in this series? also, confusingly, this returns less
> values then the other helper (maybe because of some aliases?), and it's
> not a strict subset :-/
>

Yes, this is for exposing via the API (for the GUI to create/edit custom 
models I have in mind). I just thought it fits with this patch well, 
since the comments refer to each other.

Also, query_understood_cpu_flags returns 188 flags for me, while 
query_supported_cpu_flags only returns 104...

The discrepancies generally arise because
a) query_understood_cpu_flags returns flags the host cannot use and
b) query_supported_cpu_flags (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.

The intersection of those provides only flags that can be passed to 
"-cpu" without issues in my testing. Since query_understood_cpu_flags is 
not host specific though, it doesn't need broadcasting.

> I guess this is in preparation of exposing it via the API? some sort of
> comment how _understood and _supported interact/are supposed to interact
> would be nice. either as comment or part of the commit message
> 
I'll try to make the comments/message more informative.

>> +    my $flags = [];
>> +    my $flag_section = 0;
>> +
>> +    run_command(
>> +	[get_command_for_arch('x86_64'), '-cpu', 'help'],
> 
> why not get_host_arch()?
> 
>> +	noerr => 1,
>> +	quiet => 1,
>> +	outfunc => sub {
>> +	    my ($line) = @_;
>> +
>> +	    if ($flag_section) {
>> +		return if $line =~ m/^\s*$/;
>> +		$line =~ s/^\s*|\s*$//g;
>> +		push @$flags, split(/\s+/, $line);
>> +	    } elsif ($line =~ m/^\s*Recognized CPUID flags:\s*$/) {
>> +		$flag_section = 1;
>> +	    }
>> +	}
>> +    );
>> +
>> +    return $flags;
>> +}
>> +
>> +# This function 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
>> +# pvestatd using PVE::Cluster::get_node_kv('cpuflags').
> 
> s/pvestatd/pmxcfs/
> 
> pvestatd is just the one putting it there ;)
> 
>> +#
>> +# See also note to query_understood_cpu_flags above.
>> +sub query_supported_cpu_flags {
>> +    my $flags = [];
>> +
>> +    my $vmid = -1;
>> +    my $pidfile = pidfile_name($vmid);
>> +
>> +    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('x86_64'),
> 
> again, why not get_host_arch()?
> 
>> +	    '-cpu', 'kvm64',
>> +	    '-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 => 1);
>> +	return if $rc;
>> +
>> +	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) {
>> +	    foreach my $prop (keys %$props) {
>> +		push @$flags, $prop if "$props->{$prop}" eq '1';
>> +	    }
>> +	}
>> +
>> +	# force stop with 10 sec timeout and 'nocheck'
>> +	vm_stop(undef, $vmid, 1, 1, 10, 0, 1);
>> +    });
>> +
>> +    # QEMU returns some flags multiple times, with '_', '.' or '-' as seperator
>> +    # (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;
>> +
>> +    return $flags;
>> +}
>> +
>>   sub get_cpu_options {
>>       my ($conf, $arch, $kvm, $machine_type, $kvm_off, $kvmver, $winversion, $gpu_passthrough) = @_;
>>   
>> -- 
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>