[pve-devel] [PATCH qemu-server 6/7] Handle CPU flags defined in custom CPU type

[Fabian Grünbichler]

On September 2, 2019 4:27 pm, Stefan Reiter wrote:
> Special care is taken not to overwrite any special flags, or ones
> manually set on the VM by the user. We warn if a flag is overruled.

hmm. I am unsure whether I like that behaviour or not. it's a bit 
strange that VM specific flags get applied earlier, and then we skip 
those from the models instead of just switching the order around.

maybe we need some other intermediate data structure, or some helper 
methods?

the following seems more logical to me:

1.) add flags from custom model
2.) add flags from VM config
3.) (selectively) add auto-generated flags (based on other settings, OS, ..)

at each step we want to remove conflicting flags from before (this would 
be where a different datastructure or a helper would come into play). 
maybe some of the auto-generated flags should not override explicitly 
set ones, but be skipped instead (at least for explicit negative 
flags, to allow opt-out).

> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
>  PVE/QemuServer.pm | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 417bea8..7cc1674 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3597,6 +3597,7 @@ sub get_cpu_options {
>      }
>      my $hv_vendor_id;
>      my $custom_cpu_config;
> +    my @vm_specific_flags;
>      if (my $cputype = $conf->{cpu}) {
>  	my $cpuconf = PVE::JSONSchema::parse_property_string($cpu_fmt, $cputype)
>  	    or die "Cannot parse cpu description: $cputype\n";
> @@ -3624,7 +3625,8 @@ sub get_cpu_options {
>  	$hv_vendor_id = $cpuconf->{'hv-vendor-id'};
>  
>  	if (defined(my $flags = $cpuconf->{flags})) {
> -	    push @$cpuFlags, split(";", $flags);
> +	    @vm_specific_flags = split(";", $flags);
> +	    push @$cpuFlags, @vm_specific_flags;
>  	}
>      }
>  
> @@ -3649,6 +3651,28 @@ sub get_cpu_options {
>  
>      push @$cpuFlags, 'kvm=off' if $kvm_off;
>  
> +    if (defined($custom_cpu_config) && defined($custom_cpu_config->{flags})) {
> +	my @custom_flags = split(/;/, $custom_cpu_config->{flags});
> +	foreach my $flag (@custom_flags) {
> +	    # find index of $flag in $cpuFlags while ignoring prefix [+-=]
> +	    $flag =~ m/^[+-=]?(.*)$/;
> +	    my $match_flag = $1;
> +	    my @match_index = grep { $cpuFlags->[$_] =~ m/[+-=]?\Q$match_flag/ }
> +		0..scalar(@$cpuFlags)-1;
> +
> +	    if (@match_index) {
> +		my $other_flag = $cpuFlags->[$match_index[0]];
> +		# warn only if prefix differs and flag is not vm specific
> +		warn "warning: custom CPU model flag '$flag' overruled by '$other_flag' due to either default handling of basemodel or selected OS.\n"

I don't understand that message, so probably users won't either ;) but I 
think the messages would be more clear if we apply the auto-generated 
flags last:

"unsetting flag '$other_flag', not compatible with ...." (OS foo, CPU bar, 
...)
"not setting flag '$flag', explicit '$other_flag' found." (explicit 
opt-out for auto-generated flags via VM or CPU model config)

> +		    if (substr $other_flag, 0, 1) ne (substr $flag, 0, 1) &&
> +		    (!@vm_specific_flags ||
> +			!grep(m/[+-=]?\Q$match_flag/, @vm_specific_flags));
> +	    } else {
> +		push(@$cpuFlags, $flag);
> +	    }
> +	}
> +    }
> +
>      if (defined($custom_cpu_config) && (my $custom_vendor = $custom_cpu_config->{vendor})) {
>  	push @$cpuFlags, "vendor=${custom_vendor}"
>  	    if $custom_vendor ne 'default';
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>