From 322707ba8a3504278652d56b33b199ed90689416 Mon Sep 17 00:00:00 2001
From: Jiri Denemark <jdenemar@redhat.com>
Date: Fri, 15 Jan 2010 16:58:59 +0100
Subject: [PATCH] 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>
---
 src/cpu/cpu_x86.c | 122 +++++++++++++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 44 deletions(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 7c50261f82..51f5d54346 100644
--- 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;
 }
-- 
GitLab