From 752c74eeae67d41e7550991cb3bbe289984ec9d3 Mon Sep 17 00:00:00 2001

Message-Id: <752c74eeae67d41e7550991cb3bbe289984ec9d3@dist-git>

From: Jiri Denemark <jdenemar@redhat.com>

Date: Fri, 29 Apr 2022 10:35:02 +0200

Subject: [PATCH] cpu_x86: Ignore enabled features for input models in

 x86DecodeUseCandidate

While we don't want to aim for the shortest list of disabled features in

the baseline result (it would select a very old model), we want to do so

while looking at any of the input models for which we're trying to

compute a baseline CPU model. Given a set of input models, we always

want to take the least capable one of them (i.e., the one with shortest

list of disabled features) or a better model which is not one of the

input models.

So when considering an input model, we just check whether its list of

disabled features is shorter than the currently best one. When looking

at other models we check both enabled and disabled features while

penalizing disabled features as implemented by the previous patch.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

(cherry picked from commit bb6cedd2082599323257ee0df18c93a6e0551b0b)

https://bugzilla.redhat.com/show_bug.cgi?id=1851227

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/cpu/cpu_x86.c                             | 66 ++++++++++++-------

 ...4-baseline-Westmere+Nehalem-migratable.xml |  8 ++-

 ...86_64-baseline-Westmere+Nehalem-result.xml |  8 ++-

 ...-cpuid-baseline-Cooperlake+Cascadelake.xml | 13 ++--

 4 files changed, 64 insertions(+), 31 deletions(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c

index ebcd96edb1..7b59dad8bf 100644

--- a/src/cpu/cpu_x86.c

+++ b/src/cpu/cpu_x86.c

@@ -1975,7 +1975,8 @@ virCPUx86Compare(virCPUDef *host,

 static int

 virCPUx86CompareCandidateFeatureList(virCPUDef *cpuCurrent,

-                                     virCPUDef *cpuCandidate)

+                                     virCPUDef *cpuCandidate,

+                                     bool isPreferred)

 {

     size_t current = cpuCurrent->nfeatures;

     size_t enabledCurrent = current;

@@ -2017,6 +2018,14 @@ virCPUx86CompareCandidateFeatureList(virCPUDef *cpuCurrent,

         return 1;

     }

+    if (isPreferred && disabled < disabledCurrent) {

+        VIR_DEBUG("%s is in the list of preferred models and provides fewer "

+                  "disabled features than %s: %zu < %zu",

+                  cpuCandidate->model, cpuCurrent->model,

+                  disabled, disabledCurrent);

+        return 1;

+    }

+

     VIR_DEBUG("%s is not better than %s: %zu (%zu, %zu) >= %zu (%zu, %zu)",

               cpuCandidate->model, cpuCurrent->model,

               candidate, enabled, disabled,

@@ -2039,8 +2048,10 @@ x86DecodeUseCandidate(virCPUx86Model *current,

                       virCPUx86Model *candidate,

                       virCPUDef *cpuCandidate,

                       uint32_t signature,

-                      const char *preferred)

+                      const char **preferred)

 {

+    bool isPreferred = false;

+

     if (cpuCandidate->type == VIR_CPU_TYPE_HOST &&

         !candidate->decodeHost) {

         VIR_DEBUG("%s is not supposed to be used for host CPU definition",

@@ -2064,9 +2075,13 @@ x86DecodeUseCandidate(virCPUx86Model *current,

         }

     }

-    if (preferred && STREQ(cpuCandidate->model, preferred)) {

-        VIR_DEBUG("%s is the preferred model", cpuCandidate->model);

-        return 2;

+    if (preferred) {

+        isPreferred = g_strv_contains(preferred, cpuCandidate->model);

+

+        if (isPreferred && !preferred[1]) {

+            VIR_DEBUG("%s is the preferred model", cpuCandidate->model);

+            return 2;

+        }

     }

     if (!cpuCurrent) {

@@ -2093,7 +2108,8 @@ x86DecodeUseCandidate(virCPUx86Model *current,

         }

     }

-    return virCPUx86CompareCandidateFeatureList(cpuCurrent, cpuCandidate);

+    return virCPUx86CompareCandidateFeatureList(cpuCurrent, cpuCandidate,

+                                                isPreferred);

 }

@@ -2136,7 +2152,7 @@ static int

 x86Decode(virCPUDef *cpu,

           const virCPUx86Data *cpuData,

           virDomainCapsCPUModels *models,

-          const char *preferred,

+          const char **preferred,

           bool migratable)

 {

     virCPUx86Map *map;

@@ -2169,6 +2185,9 @@ x86Decode(virCPUDef *cpu,

     x86DataFilterTSX(&data, vendor, map);

+    if (preferred && !preferred[0])

+        preferred = NULL;

+

     /* Walk through the CPU models in reverse order to check newest

      * models first.

      */

@@ -2176,16 +2195,18 @@ x86Decode(virCPUDef *cpu,

         candidate = map->models[i];

         if (models &&

             !(hvModel = virDomainCapsCPUModelsGet(models, candidate->name))) {

-            if (preferred && STREQ(candidate->name, preferred)) {

+            if (preferred &&

+                !preferred[1] &&

+                STREQ(candidate->name, preferred[0])) {

                 if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) {

                     virReportError(VIR_ERR_CONFIG_UNSUPPORTED,

                                    _("CPU model %s is not supported by hypervisor"),

-                                   preferred);

+                                   preferred[0]);

                     return -1;

                 } else {

                     VIR_WARN("Preferred CPU model %s not allowed by"

                              " hypervisor; closest supported model will be"

-                             " used", preferred);

+                             " used", preferred[0]);

                 }

             } else {

                 VIR_DEBUG("CPU model %s not allowed by hypervisor; ignoring",

@@ -2793,8 +2814,8 @@ virCPUx86Baseline(virCPUDef **cpus,

     size_t i;

     virCPUx86Vendor *vendor = NULL;

     bool outputVendor = true;

-    const char *modelName;

-    bool matchingNames = true;

+    g_autofree char **modelNames = NULL;

+    size_t namesLen = 0;

     g_autoptr(virCPUData) featData = NULL;

     if (!(map = virCPUx86GetMap()))

@@ -2816,19 +2837,17 @@ virCPUx86Baseline(virCPUDef **cpus,

         return NULL;

     }

-    modelName = cpus[0]->model;

+    modelNames = g_new0(char *, ncpus + 1);

+    if (cpus[0]->model)

+        modelNames[namesLen++] = cpus[0]->model;

+

     for (i = 1; i < ncpus; i++) {

         g_autoptr(virCPUx86Model) model = NULL;

         const char *vn = NULL;

-        if (matchingNames && cpus[i]->model) {

-            if (!modelName) {

-                modelName = cpus[i]->model;

-            } else if (STRNEQ(modelName, cpus[i]->model)) {

-                modelName = NULL;

-                matchingNames = false;

-            }

-        }

+        if (cpus[i]->model &&

+            !g_strv_contains((const char **) modelNames, cpus[i]->model))

+            modelNames[namesLen++] = cpus[i]->model;

         if (!(model = x86ModelFromCPU(cpus[i], map, -1)))

             return NULL;

@@ -2891,10 +2910,11 @@ virCPUx86Baseline(virCPUDef **cpus,

         virCPUx86DataAddItem(&base_model->data, &vendor->data) < 0)

         return NULL;

-    if (x86Decode(cpu, &base_model->data, models, modelName, migratable) < 0)

+    if (x86Decode(cpu, &base_model->data, models,

+                  (const char **) modelNames, migratable) < 0)

         return NULL;

-    if (STREQ_NULLABLE(cpu->model, modelName))

+    if (namesLen == 1 && STREQ(cpu->model, modelNames[0]))

         cpu->fallback = VIR_CPU_FALLBACK_FORBID;

     if (!outputVendor)

diff --git a/tests/cputestdata/x86_64-baseline-Westmere+Nehalem-migratable.xml b/tests/cputestdata/x86_64-baseline-Westmere+Nehalem-migratable.xml

index 775a27de2e..f5846b1619 100644

--- a/tests/cputestdata/x86_64-baseline-Westmere+Nehalem-migratable.xml

+++ b/tests/cputestdata/x86_64-baseline-Westmere+Nehalem-migratable.xml

@@ -1,10 +1,14 @@

 <cpu mode='custom' match='exact'>

-  <model fallback='allow'>SandyBridge</model>

+  <model fallback='allow'>Westmere</model>

   <vendor>Intel</vendor>

   <feature policy='require' name='vme'/>

   <feature policy='require' name='ss'/>

+  <feature policy='require' name='pclmuldq'/>

   <feature policy='require' name='pcid'/>

+  <feature policy='require' name='x2apic'/>

+  <feature policy='require' name='tsc-deadline'/>

+  <feature policy='require' name='xsave'/>

   <feature policy='require' name='osxsave'/>

+  <feature policy='require' name='avx'/>

   <feature policy='require' name='hypervisor'/>

-  <feature policy='disable' name='rdtscp'/>

 </cpu>

diff --git a/tests/cputestdata/x86_64-baseline-Westmere+Nehalem-result.xml b/tests/cputestdata/x86_64-baseline-Westmere+Nehalem-result.xml

index cafca97d62..166833276c 100644

--- a/tests/cputestdata/x86_64-baseline-Westmere+Nehalem-result.xml

+++ b/tests/cputestdata/x86_64-baseline-Westmere+Nehalem-result.xml

@@ -1,11 +1,15 @@

 <cpu mode='custom' match='exact'>

-  <model fallback='allow'>SandyBridge</model>

+  <model fallback='allow'>Westmere</model>

   <vendor>Intel</vendor>

   <feature policy='require' name='vme'/>

   <feature policy='require' name='ss'/>

+  <feature policy='require' name='pclmuldq'/>

   <feature policy='require' name='pcid'/>

+  <feature policy='require' name='x2apic'/>

+  <feature policy='require' name='tsc-deadline'/>

+  <feature policy='require' name='xsave'/>

   <feature policy='require' name='osxsave'/>

+  <feature policy='require' name='avx'/>

   <feature policy='require' name='hypervisor'/>

   <feature policy='require' name='invtsc'/>

-  <feature policy='disable' name='rdtscp'/>

 </cpu>

diff --git a/tests/cputestdata/x86_64-cpuid-baseline-Cooperlake+Cascadelake.xml b/tests/cputestdata/x86_64-cpuid-baseline-Cooperlake+Cascadelake.xml

index 46c32c996f..ecac749b97 100644

--- a/tests/cputestdata/x86_64-cpuid-baseline-Cooperlake+Cascadelake.xml

+++ b/tests/cputestdata/x86_64-cpuid-baseline-Cooperlake+Cascadelake.xml

@@ -1,17 +1,22 @@

 <cpu mode='custom' match='exact'>

-  <model fallback='allow'>Cooperlake</model>

+  <model fallback='allow'>Cascadelake-Server</model>

   <vendor>Intel</vendor>

   <feature policy='require' name='ss'/>

   <feature policy='require' name='vmx'/>

   <feature policy='require' name='hypervisor'/>

   <feature policy='require' name='tsc_adjust'/>

-  <feature policy='require' name='mpx'/>

   <feature policy='require' name='umip'/>

+  <feature policy='require' name='pku'/>

   <feature policy='require' name='md-clear'/>

+  <feature policy='require' name='stibp'/>

+  <feature policy='require' name='arch-capabilities'/>

   <feature policy='require' name='xsaves'/>

   <feature policy='require' name='ibpb'/>

   <feature policy='require' name='amd-ssbd'/>

+  <feature policy='require' name='rdctl-no'/>

+  <feature policy='require' name='ibrs-all'/>

+  <feature policy='require' name='skip-l1dfl-vmentry'/>

+  <feature policy='require' name='mds-no'/>

+  <feature policy='require' name='pschange-mc-no'/>

   <feature policy='require' name='tsx-ctrl'/>

-  <feature policy='disable' name='avx512-bf16'/>

-  <feature policy='disable' name='taa-no'/>

 </cpu>

-- 

2.35.1