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>

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 3001fc2b8fa67c7fac1b09a174311d53df689255..a4aca6055df9e6a9b6b1db7fc3731ed9ad5843e9 100644 (file)
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1961,7 +1961,8 @@ virCPUx86Compare(virCPUDef *host,
 
 static int
 virCPUx86CompareCandidateFeatureList(virCPUDef *cpuCurrent,
-                                     virCPUDef *cpuCandidate)
+                                     virCPUDef *cpuCandidate,
+                                     bool isPreferred)
 {
     size_t current = cpuCurrent->nfeatures;
     size_t enabledCurrent = current;
@@ -2003,6 +2004,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,
@@ -2025,8 +2034,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",
@@ -2050,9 +2061,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) {
@@ -2079,7 +2094,8 @@ x86DecodeUseCandidate(virCPUx86Model *current,
         }
     }
 
-    return virCPUx86CompareCandidateFeatureList(cpuCurrent, cpuCandidate);
+    return virCPUx86CompareCandidateFeatureList(cpuCurrent, cpuCandidate,
+                                                isPreferred);
 }
 
 
@@ -2122,7 +2138,7 @@ static int
 x86Decode(virCPUDef *cpu,
           const virCPUx86Data *cpuData,
           virDomainCapsCPUModels *models,
-          const char *preferred,
+          const char **preferred,
           bool migratable)
 {
     virCPUx86Map *map;
@@ -2155,6 +2171,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.
      */
@@ -2162,16 +2181,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",
@@ -2779,8 +2800,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()))
@@ -2802,19 +2823,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;
@@ -2877,10 +2896,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 775a27de2ed541741f42bd6aeb49d649d4d60658..f5846b1619df442d8551e15447cb28c914e85294 100644 (file)
--- 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 cafca97d62a3693ee9c539868d4cfe7c190ec639..166833276c2c9dbd97ba9532aff4fd7358e1724b 100644 (file)
--- 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 46c32c996f1795ae1b2d145e2b64d9d9b38b6236..ecac749b97229e05b668b6d788abaf895c0cf058 100644 (file)
--- 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>