cpu: Improve error reporting on incompatible CPUs

This patch modifies the CPU comparrison function to report the
incompatibilities in more detail to ease identification of problems.

* src/cpu/cpu.h:
    cpuGuestData(): Add argument to return detailed error message.
* src/cpu/cpu.c:
    cpuGuestData(): Add passthrough for error argument.
* src/cpu/cpu_x86.c
    x86FeatureNames(): Add function to convert a CPU definition to flag
                       names.
    x86Compute(): - Add error message parameter
                  - Add macro for reporting detailed error messages.
                  - Improve error reporting.
                  - Simplify calculation of forbidden flags.
    x86DataIteratorInit():
    x86cpuidMatchAny(): Remove functions that are no longer needed.
* src/qemu/qemu_command.c:
    qemuBuildCpuArgStr(): - Modify for new function prototype
                          - Add detailed error reports
                          - Change error code on incompatible processors
                            to VIR_ERR_CONFIG_UNSUPPORTED instead of
                            internal error
* tests/cputest.c:
    cpuTestGuestData(): Modify for new function prototype

src/cpu/cpu.c

@@ -248,11 +248,12 @@ cpuNodeData(const char *arch)
 virCPUCompareResult
 cpuGuestData(virCPUDefPtr host,
              virCPUDefPtr guest,
-             union cpuData **data)
+             union cpuData **data,
+             char **msg)
 {
     struct cpuArchDriver *driver;
 
-    VIR_DEBUG("host=%p, guest=%p, data=%p", host, guest, data);
+    VIR_DEBUG("host=%p, guest=%p, data=%p, msg=%p", host, guest, data, msg);
 
     if ((driver = cpuGetSubDriver(host->arch)) == NULL)
         return VIR_CPU_COMPARE_ERROR;
@@ -264,7 +265,7 @@ cpuGuestData(virCPUDefPtr host,
         return VIR_CPU_COMPARE_ERROR;
     }
 
-    return driver->guestData(host, guest, data);
+    return driver->guestData(host, guest, data, msg);
 }
 
 

src/cpu/cpu.h

@@ -70,7 +70,8 @@ typedef union cpuData *
 typedef virCPUCompareResult
 (*cpuArchGuestData) (virCPUDefPtr host,
                      virCPUDefPtr guest,
-                     union cpuData **data);
+                     union cpuData **data,
+                     char **message);
 
 typedef virCPUDefPtr
 (*cpuArchBaseline)  (virCPUDefPtr *cpus,
@@ -138,7 +139,8 @@ cpuNodeData (const char *arch);
 extern virCPUCompareResult
 cpuGuestData(virCPUDefPtr host,
              virCPUDefPtr guest,
-             union cpuData **data);
+             union cpuData **data,
+             char **msg);
 
 extern char *
 cpuBaselineXML(const char **xmlCPUs,

src/cpu/cpu_x86.c

@@ -31,6 +31,7 @@
 #include "cpu.h"
 #include "cpu_map.h"
 #include "cpu_x86.h"
+#include "buf.h"
 
 
 #define VIR_FROM_THIS VIR_FROM_CPU
@@ -89,16 +90,6 @@ struct data_iterator {
     { data, -1, false }
 
 
-static void
-x86DataIteratorInit(struct data_iterator *iter,
-                    union cpuData *data)
-{
-    struct data_iterator init = DATA_ITERATOR_INIT(data);
-
-    *iter = init;
-}
-
-
 static int
 x86cpuidMatch(const struct cpuX86cpuid *cpuid1,
               const struct cpuX86cpuid *cpuid2)
@@ -121,17 +112,6 @@ x86cpuidMatchMasked(const struct cpuX86cpuid *cpuid,
 }
 
 
-static int
-x86cpuidMatchAny(const struct cpuX86cpuid *cpuid,
-                 const struct cpuX86cpuid *mask)
-{
-    return ((cpuid->eax & mask->eax) ||
-            (cpuid->ebx & mask->ebx) ||
-            (cpuid->ecx & mask->ecx) ||
-            (cpuid->edx & mask->edx));
-}
-
-
 static void
 x86cpuidSetBits(struct cpuX86cpuid *cpuid,
                 const struct cpuX86cpuid *mask)
@@ -649,6 +629,34 @@ x86FeatureFind(const struct x86_map *map,
 }
 
 
+static char *
+x86FeatureNames(const struct x86_map *map,
+                const char *separator,
+                union cpuData *data)
+{
+    virBuffer ret = VIR_BUFFER_INITIALIZER;
+    bool first = true;
+
+    struct x86_feature *next_feature = map->features;
+
+    virBufferAdd(&ret, "", 0);
+
+    while (next_feature) {
+        if (x86DataIsSubset(data, next_feature->data)) {
+            if (!first)
+                virBufferAdd(&ret, separator, -1);
+            else
+                first = false;
+
+            virBufferAdd(&ret, next_feature->name, -1);
+        }
+        next_feature = next_feature->next;
+    }
+
+    return virBufferContentAndReset(&ret);
+}
+
+
 static int
 x86FeatureLoad(xmlXPathContextPtr ctxt,
                struct x86_map *map)
@@ -1115,10 +1123,34 @@ error:
 }
 
 
+/* A helper macro to exit the cpu computation function without writing
+ * redundant code:
+ * MSG: error message
+ * CPU_DEF: a union cpuData pointer with flags that are conflicting
+ * RET: return code to set
+ *
+ * This macro generates the error string outputs it into logs.
+ */
+#define virX86CpuIncompatible(MSG, CPU_DEF)                             \
+        do {                                                            \
+            char *flagsStr = NULL;                                      \
+            if (!(flagsStr = x86FeatureNames(map, ", ", (CPU_DEF))))    \
+                goto no_memory;                                         \
+            if (message &&                                              \
+                virAsprintf(message, "%s: %s", _(MSG), flagsStr) < 0) { \
+                VIR_FREE(flagsStr);                                     \
+                goto no_memory;                                         \
+            }                                                           \
+            VIR_DEBUG("%s: %s", MSG, flagsStr);                         \
+            VIR_FREE(flagsStr);                                         \
+            ret = VIR_CPU_COMPARE_INCOMPATIBLE;                         \
+        } while (0)
+
 static virCPUCompareResult
 x86Compute(virCPUDefPtr host,
            virCPUDefPtr cpu,
-           union cpuData **guest)
+           union cpuData **guest,
+           char **message)
 {
     struct x86_map *map = NULL;
     struct x86_model *host_model = NULL;
@@ -1129,8 +1161,6 @@ x86Compute(virCPUDefPtr host,
     struct x86_model *cpu_forbid = NULL;
     struct x86_model *diff = NULL;
     struct x86_model *guest_model = NULL;
-    struct data_iterator iter;
-    const struct cpuX86cpuid *cpuid;
     virCPUCompareResult ret;
     enum compare_result result;
     unsigned int i;
@@ -1147,6 +1177,11 @@ x86Compute(virCPUDefPtr host,
 
         if (!found) {
             VIR_DEBUG("CPU arch %s does not match host arch", cpu->arch);
+            if (message &&
+                virAsprintf(message,
+                            _("CPU arch %s does not match host arch"),
+                            cpu->arch) < 0)
+                goto no_memory;
             return VIR_CPU_COMPARE_INCOMPATIBLE;
         }
     }
@@ -1155,6 +1190,12 @@ x86Compute(virCPUDefPtr host,
         (!host->vendor || STRNEQ(cpu->vendor, host->vendor))) {
         VIR_DEBUG("host CPU vendor does not match required CPU vendor %s",
                   cpu->vendor);
+        if (message &&
+            virAsprintf(message,
+                        _("host CPU vendor does not match required "
+                          "CPU vendor %s"),
+                        cpu->vendor) < 0)
+            goto no_memory;
         return VIR_CPU_COMPARE_INCOMPATIBLE;
     }
 
@@ -1167,24 +1208,20 @@ x86Compute(virCPUDefPtr host,
         !(cpu_forbid = x86ModelFromCPU(cpu, map, VIR_CPU_FEATURE_FORBID)))
         goto error;
 
-    x86DataIteratorInit(&iter, cpu_forbid->data);
-    while ((cpuid = x86DataCpuidNext(&iter))) {
-        const struct cpuX86cpuid *cpuid2;
-
-        cpuid2 = x86DataCpuid(host_model->data, cpuid->function);
-        if (cpuid2 != NULL && x86cpuidMatchAny(cpuid2, cpuid)) {
-            VIR_DEBUG("Host CPU provides forbidden features in CPUID function 0x%x",
-                      cpuid->function);
-            ret = VIR_CPU_COMPARE_INCOMPATIBLE;
-            goto out;
-        }
+    x86DataIntersect(cpu_forbid->data, host_model->data);
+    if (!x86DataIsEmpty(cpu_forbid->data)) {
+        virX86CpuIncompatible(N_("Host CPU provides forbidden features"),
+                              cpu_forbid->data);
+        goto out;
     }
 
     x86DataSubtract(cpu_require->data, cpu_disable->data);
     result = x86ModelCompare(host_model, cpu_require);
     if (result == SUBSET || result == UNRELATED) {
-        VIR_DEBUG("Host CPU does not provide all required features");
-        ret = VIR_CPU_COMPARE_INCOMPATIBLE;
+        x86DataSubtract(cpu_require->data, host_model->data);
+        virX86CpuIncompatible(N_("Host CPU does not provide required "
+                                 "features"),
+                              cpu_require->data);
         goto out;
     }
 
@@ -1204,8 +1241,9 @@ x86Compute(virCPUDefPtr host,
     if (ret == VIR_CPU_COMPARE_SUPERSET
         && cpu->type == VIR_CPU_TYPE_GUEST
         && cpu->match == VIR_CPU_MATCH_STRICT) {
-        VIR_DEBUG("Host CPU does not strictly match guest CPU");
-        ret = VIR_CPU_COMPARE_INCOMPATIBLE;
+        virX86CpuIncompatible(N_("Host CPU does not strictly match guest CPU: "
+                                 "Extra features"),
+                              diff->data);
         goto out;
     }
 
@@ -1246,22 +1284,24 @@ error:
     ret = VIR_CPU_COMPARE_ERROR;
     goto out;
 }
+#undef virX86CpuIncompatible
 
 
 static virCPUCompareResult
 x86Compare(virCPUDefPtr host,
            virCPUDefPtr cpu)
 {
-    return x86Compute(host, cpu, NULL);
+    return x86Compute(host, cpu, NULL, NULL);
 }
 
 
 static virCPUCompareResult
 x86GuestData(virCPUDefPtr host,
              virCPUDefPtr guest,
-             union cpuData **data)
+             union cpuData **data,
+             char **message)
 {
-    return x86Compute(host, guest, data);
+    return x86Compute(host, guest, data, message);
 }
 
 

src/qemu/qemu_command.c

@@ -3704,6 +3704,7 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver,
     const char *default_model;
     union cpuData *data = NULL;
     bool have_cpu = false;
+    char *compare_msg = NULL;
     int ret = -1;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
     int i;
@@ -3740,11 +3741,17 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver,
             cpuUpdate(cpu, host) < 0)
             goto cleanup;
 
-        cmp = cpuGuestData(host, cpu, &data);
+        cmp = cpuGuestData(host, cpu, &data, &compare_msg);
         switch (cmp) {
         case VIR_CPU_COMPARE_INCOMPATIBLE:
-            qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+            if (compare_msg) {
+                qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                                _("guest and host CPU are not compatible: %s"),
+                                compare_msg);
+            } else {
+                qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                             _("guest CPU is not compatible with host CPU"));
+            }
             /* fall through */
         case VIR_CPU_COMPARE_ERROR:
             goto cleanup;
@@ -3848,6 +3855,7 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver,
     ret = 0;
 
 cleanup:
+    VIR_FREE(compare_msg);
     if (host)
         cpuDataFree(host->arch, data);
     virCPUDefFree(guest);

tests/cputest.c

@@ -269,7 +269,7 @@ cpuTestGuestData(const void *arg)
         !(cpu = cpuTestLoadXML(data->arch, data->name)))
         goto cleanup;
 
-    cmpResult = cpuGuestData(host, cpu, &guestData);
+    cmpResult = cpuGuestData(host, cpu, &guestData, NULL);
     if (cmpResult == VIR_CPU_COMPARE_ERROR ||
         cmpResult == VIR_CPU_COMPARE_INCOMPATIBLE)
         goto cleanup;