[libvirt PATCH 7/8] cpu_x86: Penalize disabled features when computing CPU model

[Michal Prívozník]

On 5/4/22 18:54, Jiri Denemark wrote:
> For finding the best matching CPU model for a given set of features
> while we don't know the CPU signature (i.e., when computing a baseline
> CPU model) we've been using a "shortest list of features" heuristics.
> This works well if new CPU models are supersets of older models, but
> that's not always the case. As a result it may actually select a new CPU
> model as a baseline while removing some features from it to make it
> compatible with older models. This is in general worse than using an old
> CPU model with a bunch of added features as a guest OS or apps may crash
> when using features that were disabled.
> 
> On the other hand we don't want to end up with a very old model which
> would guarantee no disabled features as it could stop a guest OS or apps
> from using some features provided by the CPU because they would not
> expect them on such an old CPU.
> 
> This patch changes the heuristics to something in between. Enabled and
> disabled features are counted separately so that a CPU model requiring
> some features to be disabled looks worse than a model with fewer
> disabled features even if its complete list of features is longer. The
> penalty given for each additional disabled feature gets bigger to make
> longer list of disabled features look even worse.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1851227
> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/cpu/cpu_x86.c                             | 44 ++++++++++++++++---
>  .../x86_64-cpuid-Atom-D510-guest.xml          |  5 ++-
>  .../x86_64-cpuid-Atom-N450-guest.xml          |  5 ++-
>  .../x86_64-cpuid-Phenom-B95-json.xml          | 21 +++++----
>  ...id-baseline-Broadwell-IBRS+Cascadelake.xml | 11 +++--
>  ..._64-cpuid-baseline-Cascadelake+Icelake.xml | 13 +++---
>  ...puid-baseline-Cascadelake+Skylake-IBRS.xml |  5 ++-
>  ...6_64-cpuid-baseline-Cooperlake+Icelake.xml | 13 +++---
>  .../x86_64-host+guest,models-result.xml       | 10 +++--
>  .../domaincapsdata/qemu_3.1.0-tcg.x86_64.xml  | 35 +++++++++------
>  .../domaincapsdata/qemu_4.0.0-tcg.x86_64.xml  | 36 ++++++++-------
>  .../domaincapsdata/qemu_4.1.0-tcg.x86_64.xml  | 37 +++++++++-------
>  .../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml  | 37 +++++++++-------
>  .../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml  | 36 +++++++++------
>  .../domaincapsdata/qemu_5.1.0-tcg.x86_64.xml  | 36 +++++++++------
>  .../domaincapsdata/qemu_5.2.0-tcg.x86_64.xml  | 36 +++++++++------
>  .../domaincapsdata/qemu_6.0.0-tcg.x86_64.xml  | 36 +++++++++------
>  .../domaincapsdata/qemu_6.1.0-tcg.x86_64.xml  | 36 +++++++++------
>  .../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml  | 36 +++++++++------
>  .../domaincapsdata/qemu_7.0.0-tcg.x86_64.xml  | 36 +++++++++------
>  tests/qemuxml2argvdata/cpu-fallback.args      |  2 +-
>  .../cpu-host-model-cmt.x86_64-4.0.0.args      |  2 +-
>  .../cpu-host-model-fallback.args              |  2 +-
>  23 files changed, 330 insertions(+), 200 deletions(-)
> 
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index fdee107ce9..3001fc2b8f 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -1956,23 +1956,57 @@ virCPUx86Compare(virCPUDef *host,
>  }
>  
>  
> +/* Base penalty for disabled features. */
> +#define BASE_PENALTY 2
> +
>  static int
>  virCPUx86CompareCandidateFeatureList(virCPUDef *cpuCurrent,
>                                       virCPUDef *cpuCandidate)
>  {
>      size_t current = cpuCurrent->nfeatures;
> +    size_t enabledCurrent = current;
> +    size_t disabledCurrent = 0;
>      size_t candidate = cpuCandidate->nfeatures;
> +    size_t enabled = candidate;
> +    size_t disabled = 0;
> +
> +    if (cpuCandidate->type != VIR_CPU_TYPE_HOST) {
> +        size_t i;
> +        int penalty = BASE_PENALTY;
> +
> +        for (i = 0; i < enabledCurrent; i++) {
> +            if (cpuCurrent->features[i].policy == VIR_CPU_FEATURE_DISABLE) {
> +                enabledCurrent--;
> +                disabledCurrent += penalty;
> +                penalty++;
> +            }
> +        }
> +        current = enabledCurrent + disabledCurrent;
> +
> +        penalty = BASE_PENALTY;
> +        for (i = 0; i < enabled; i++) {
> +            if (cpuCandidate->features[i].policy == VIR_CPU_FEATURE_DISABLE) {
> +                enabled--;
> +                disabled += penalty;
> +                penalty++;
> +            }
> +        }
> +        candidate = enabled + disabled;
> +    }
>  
> -    if (candidate < current) {
> -        VIR_DEBUG("%s is better than %s: %zu < %zu",
> +    if (candidate < current ||
> +        (candidate == current && disabled < disabledCurrent)) {
> +        VIR_DEBUG("%s is better than %s: %zu (%zu, %zu) < %zu (%zu, %zu)",
>                    cpuCandidate->model, cpuCurrent->model,
> -                  candidate, current);
> +                  candidate, enabled, disabled,
> +                  current, enabledCurrent, disabledCurrent);
>          return 1;
>      }
>  
> -    VIR_DEBUG("%s is not better than %s: %zu >= %zu",
> +    VIR_DEBUG("%s is not better than %s: %zu (%zu, %zu) >= %zu (%zu, %zu)",
>                cpuCandidate->model, cpuCurrent->model,
> -              candidate, current);
> +              candidate, enabled, disabled,
> +              current, enabledCurrent, disabledCurrent);
>      return 0;


What a nice algorithm you have here :-)

Michal