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@xxxxxxxxxx> > --- > 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
