[pve-devel] [RFC qemu-server 4/6] Add QEMU CPU flag querying helpers

[Fabian Grünbichler]

approach looks good, some style / code re-use comments inline.

On Wed, Jul 17, 2019 at 03:03:46PM +0200, 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>
> ---
>  PVE/QemuServer.pm | 79 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 9f29927..9ee9722 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3448,6 +3448,85 @@ sub get_command_for_arch($) {
>      return $cmd;
>  }
>  
> +sub query_understood_cpu_flags {
> +    my $flags = [];
> +    my $flag_section = 0;
> +
> +    run_command(
> +	[get_command_for_arch('x86_64'), '-cpu', 'help'],
> +	noerr => 1,
> +	quiet => 1,
> +	outfunc => sub {
> +	    my ($line) = @_;
> +
> +	    if ($flag_section) {
> +		return if $line =~ m/^\s*$/;
> +		$line =~ m/^\s*(.+?)\s*$/;
> +		($line = $1) =~ s/-|\./_/g;
> +		my @flags_line = split(m/\s+/, $line);
> +		@$flags = (@$flags, @flags_line);

this is a bit confusing to read, how about the following:

		return if $line =~ m/^\s*$/;
		$line =~ s/^\s*|\s*$//g;
		$line =~ s/-|\./_/g;
		push @$flags, split /\s+/, $line;

I assume replacing - and . with _ is to be compatible with
/proc/cpuinfo? wouldn't it make more sense to keep the Qemu-provided
values in the format that Qemu uses and understands, and just do the
conversion when we want to check for membership/intersections between
the two sets?

> +	    } elsif ($line =~ m/^\s*Recognized CPUID flags:\s*$/) {
> +		$flag_section = 1;
> +	}

last } is wrongly indented in the block above, it closes the elsif, not
the sub block

> +    });


> +
> +    return $flags;
> +}
> +

a comment here that query_supported_cpu_flags is very expensive, and
that the information is available via pvestatd/pmxcfs/node_kv "for free"
would be nice to prevent future misuse ;)

> +sub query_supported_cpu_flags {
> +    my $flags = [];
> +
> +    my $pidfile = '/var/run/qemu-server/-1.pid';

please re-use "pidfile_name" to avoid problems in the future, and move
the special '-1' value into a variable.

> +
> +    # We start a temporary (frozen) VM with vmid -1 to allow us to send a QMP command

this is missing locking, otherwise you get races and failing calls. I
think you can re-use the regular local flock for the VM config
(QemuConfig->lock_config), and don't need to invent anything special.

> +    my $rc = run_command([
> +	get_command_for_arch('x86_64'),
> +	'-cpu', 'kvm64',
> +	'-display', 'none',
> +	'-chardev', 'socket,id=qmp,path=/var/run/qemu-server/-1.qmp,server,nowait',
> +	'-mon', 'chardev=qmp,mode=control',
> +	'-pidfile', $pidfile,
> +	'-S',
> +	'-daemonize'
> +    ], noerr => 1, quiet => 1);
> +    return [] if $rc;

I wonder whether we could do better error handling here? maybe also let
pvestatd not overwrite potentially existing values in the KV store if
this dies/returns empty?

> +
> +    my $qmpclient = PVE::QMPClient->new();
> +    $qmpclient->queue_cmd(
> +	-1,
> +	sub {
> +	    my ($vmid, $response) = @_;
> +
> +	    return if %{$response->{error}};
> +
> +	    my $props = $response->{return}->{model}->{props};
> +	    return if !%$props;
> +
> +	    foreach my $prop_raw (keys %$props) {
> +		(my $prop = $prop_raw) =~ s/-|\./_/g;

same as above with the conversion

> +		push @$flags, $prop if "$props->{$prop_raw}" eq '1';
> +	    }
> +	},
> +	'query-cpu-model-expansion',
> +	type => 'full',
> +	model => { name => 'host' });
> +    $qmpclient->queue_cmd(-1, undef, 'quit');

why not re-use vm_stop with $nocheck? it does all the waiting, kill
with increasingly heavy signals, etc.pp.

> +    $qmpclient->queue_execute(3, 2);
> +
> +    # Wait for QEMU to terminate
> +    sleep 1;

> +
> +    if (-e $pidfile) {
> +	$rc = run_command([
> +	    'pkill', '-F', $pidfile
> +	], noerr => 1, quiet => 1);
> +
> +	warn 'QEMU instance used for host CPU flag detection could not be killed\n' if $rc;

all of this would then just be an eval + proper error message ;) you
could also use a simple vm_mon_cmd_nocheck instead of the whole queue
thing.

> +    }
> +
> +    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