Use closest CPU model when decoding from CPUID

Current implementation of x86Decode() used for CPUID -> model+features
translation does not always select the closest CPU model. When walking
through all models from cpu_map.xml the function considers a new
candidate as a better choice than a previously selected candidate only
if the new one is a superset of the old one. In case the new candidate
is closer to host CPU but lacks some feature comparing to the old
candidate, the function does not choose well.

This patch changes the algorithm so that the closest model is always
selected. That is, the model which requires the lowest number of
additional features to describe host CPU.

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

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 7c50261f82e91ca42208694387730997892734c7..51f5d5434694a7f05177eeac1fae21174e8978a7 100644 (file)
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -238,6 +238,55 @@ x86DataFromModel(const struct x86_model *model)
 }
 
 
+static virCPUDefPtr
+x86DataToCPU(const union cpuData *data,
+             const struct x86_model *model,
+             const struct x86_map *map)
+{
+    virCPUDefPtr cpu;
+    union cpuData *tmp = NULL;
+    struct cpuX86cpuid *cpuid;
+    const struct x86_feature *feature;
+    int i;
+
+    if (VIR_ALLOC(cpu) < 0 ||
+        (cpu->model = strdup(model->name)) == NULL ||
+        (tmp = x86DataCopy(data)) == NULL)
+        goto no_memory;
+
+    for (i = 0; i < model->ncpuid; i++) {
+        x86cpuidClearBits(x86DataCpuid(tmp, model->cpuid[i].function),
+                          model->cpuid + i);
+    }
+
+    feature = map->features;
+    while (feature != NULL) {
+        for (i = 0; i < feature->ncpuid; i++) {
+            if ((cpuid = x86DataCpuid(tmp, feature->cpuid[i].function))
+                && x86cpuidMatchMasked(cpuid, feature->cpuid + i)) {
+                x86cpuidClearBits(cpuid, feature->cpuid + i);
+                if (virCPUDefAddFeature(NULL, cpu, feature->name,
+                                        VIR_CPU_FEATURE_REQUIRE) < 0)
+                    goto error;
+            }
+        }
+
+        feature = feature->next;
+    }
+
+cleanup:
+    x86DataFree(tmp);
+    return cpu;
+
+no_memory:
+    virReportOOMError(NULL);
+error:
+    virCPUDefFree(cpu);
+    cpu = NULL;
+    goto cleanup;
+}
+
+
 static void
 x86FeatureFree(struct x86_feature *feature)
 {
@@ -887,10 +936,9 @@ x86Decode(virCPUDefPtr cpu,
 {
     int ret = -1;
     struct x86_map *map;
-    const struct x86_feature *feature;
-    const struct x86_model *model = NULL;
     const struct x86_model *candidate;
-    union cpuData *tmp = NULL;
+    virCPUDefPtr cpuCandidate;
+    virCPUDefPtr cpuModel = NULL;
     struct cpuX86cpuid *cpuid;
     int i;
 
@@ -899,6 +947,8 @@ x86Decode(virCPUDefPtr cpu,
 
     candidate = map->models;
     while (candidate != NULL) {
+        bool allowed = (models == NULL);
+
         for (i = 0; i < candidate->ncpuid; i++) {
             cpuid = x86DataCpuid(data, candidate->cpuid[i].function);
             if (cpuid == NULL
@@ -906,65 +956,49 @@ x86Decode(virCPUDefPtr cpu,
                 goto next;
         }
 
-        if (model == NULL
-            || x86ModelCompare(model, candidate) == SUBSET) {
-            bool found = false;
-            for (i = 0; i < nmodels; i++) {
-                if (STREQ(models[i], candidate->name)) {
-                    found = true;
-                    break;
-                }
+        for (i = 0; i < nmodels; i++) {
+            if (STREQ(models[i], candidate->name)) {
+                allowed = true;
+                break;
             }
+        }
 
-            if (nmodels > 0 && !found) {
-                VIR_DEBUG("CPU model %s not allowed by hypervisor; ignoring",
-                          candidate->name);
-            }
-            else
-                model = candidate;
+        if (!allowed) {
+            VIR_DEBUG("CPU model %s not allowed by hypervisor; ignoring",
+                      candidate->name);
+            goto next;
         }
 
+        if (!(cpuCandidate = x86DataToCPU(data, candidate, map)))
+            goto out;
+
+        if (cpuModel == NULL
+            || cpuModel->nfeatures > cpuCandidate->nfeatures) {
+            virCPUDefFree(cpuModel);
+            cpuModel = cpuCandidate;
+        } else
+            virCPUDefFree(cpuCandidate);
+
     next:
         candidate = candidate->next;
     }
 
-    if (model == NULL) {
+    if (cpuModel == NULL) {
         virCPUReportError(NULL, VIR_ERR_INTERNAL_ERROR,
                 "%s", _("Cannot find suitable CPU model for given data"));
         goto out;
     }
 
-    if ((cpu->model = strdup(model->name)) == NULL
-        || (tmp = x86DataCopy(data)) == NULL) {
-        virReportOOMError(NULL);
-        goto out;
-    }
-
-    for (i = 0; i < model->ncpuid; i++) {
-        x86cpuidClearBits(x86DataCpuid(tmp, model->cpuid[i].function),
-                          model->cpuid + i);
-    }
-
-    feature = map->features;
-    while (feature != NULL) {
-        for (i = 0; i < feature->ncpuid; i++) {
-            if ((cpuid = x86DataCpuid(tmp, feature->cpuid[i].function))
-                && x86cpuidMatchMasked(cpuid, feature->cpuid + i)) {
-                x86cpuidClearBits(cpuid, feature->cpuid + i);
-                if (virCPUDefAddFeature(NULL, cpu, feature->name,
-                                        VIR_CPU_FEATURE_REQUIRE) < 0)
-                    goto out;
-            }
-        }
-
-        feature = feature->next;
-    }
+    cpu->model = cpuModel->model;
+    cpu->nfeatures = cpuModel->nfeatures;
+    cpu->features = cpuModel->features;
+    VIR_FREE(cpuModel);
 
     ret = 0;
 
 out:
-    x86DataFree(tmp);
     x86MapFree(map);
+    virCPUDefFree(cpuModel);
 
     return ret;
 }