[pve-devel] [PATCH v4 qemu-server 09/12] Add helpers to better structure CPU option handling

[Fabian Grünbichler]

On October 7, 2019 2:47 pm, Stefan Reiter wrote:
> To avoid hardcoding even more CPU-flag related things for custom CPU
> models, introduce a dynamic approach to resolving flags.
> 
> resolve_cpu_flags takes a list of hashes (as documented in the
> comment) and resolves them to a valid "-cpu" argument without
> duplicates. This also helps by providing a reason why specific CPU flags
> have been added, and thus allows for useful warning messages should a
> flag be overwritten by another.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
>  PVE/QemuServer/CPUConfig.pm | 67 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
> index fb8e852..405ad86 100644
> --- a/PVE/QemuServer/CPUConfig.pm
> +++ b/PVE/QemuServer/CPUConfig.pm
> @@ -298,6 +298,73 @@ sub print_cpu_device {
>      return "$cpu-x86_64-cpu,id=cpu$id,socket-id=$current_socket,core-id=$current_core,thread-id=0";
>  }
>  
> +# Resolves multiple arrays of hashes representing CPU flags with metadata to a
> +# single string in QEMU "-cpu" compatible format. Later arrays have higher
> +# priority.
> +#
> +# Hashes take the following format:
> +# {
> +#     aes => {
> +#         op => "+", # defaults to "" if undefined
> +#         reason => "to support AES acceleration", # for override warnings
> +#         value => "" # needed for kvm=off (value: off) etc...
> +#     },
> +#     ...
> +# }
> +sub resolve_cpu_flags {
> +    my $flag_hashes = \@_;

why? it's only used once two lines below, where you immediately 
derefence it again.

> +
> +    my $flags = {};
> +
> +    for my $hash (@$flag_hashes) {
> +	for my $flag_name (keys %$hash) {
> +	    my $flag = $hash->{$flag_name};
> +	    my $old_flag = $flags->{$flag_name};
> +
> +	    $flag->{op} //= "";
> +
> +	    if ($old_flag) {
> +		my $value_changed = defined($flag->{value}) != defined($old_flag->{value});
> +		if (!$value_changed && defined($flag->{value})) {
> +		    $value_changed = $flag->{value} eq $old_flag->{value};
> +		}

eq should be ne, right? either the definedness changed, or it was and is 
defined and the value changed?

AFAICT, this is in fact the following nested conditional:

my $value_changed = (defined($flag->{value}) != defined($old_flag->{value})) ||
                    (defined($flag->{value}) && $flag->{value} ne $old_flag->{value});

which is more readable IMHO, but you could also expand it more with 
additional variables.

> +
> +		if ($old_flag->{op} eq $flag->{op} && !$value_changed) {
> +		    $flags->{$flag_name}->{reason} .= " & $flag->{reason}";
> +		    next;
> +		}
> +
> +		my $old = print_cpuflag_hash($flag_name, $flags->{$flag_name});
> +		my $new = print_cpuflag_hash($flag_name, $flag);
> +		warn "warning: CPU flag/setting $new overwrites $old\n";
> +	    }
> +
> +	    $flags->{$flag_name} = $flag;
> +	}
> +    }
> +
> +    my $flag_str = '';
> +    # sort for command line stability
> +    for my $flag_name (sort keys %$flags) {
> +	$flag_str .= ',';
> +	$flag_str .= $flags->{$flag_name}->{op};
> +	$flag_str .= $flag_name;
> +	$flag_str .= "=$flags->{$flag_name}->{value}"
> +	    if $flags->{$flag_name}->{value};
> +    }
> +
> +    return $flag_str;
> +}
> +
> +sub print_cpuflag_hash {
> +    my ($flag_name, $flag) = @_;
> +    my $formatted = "'$flag->{op}$flag_name";
> +    $formatted .= "=$flag->{value}" if defined($flag->{value});
> +    $formatted .= "'";
> +    $formatted .= " ($flag->{reason})" if defined($flag->{reason});
> +    return $formatted;
> +}
> +
>  # Calculate QEMU's '-cpu' argument from a given VM configuration
>  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
> 
>