On Wed, Apr 18, 2012 at 15:19:53 +0200, Peter Krempa wrote:

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 --- Sample of error message: $ virsh start Bare error: Failed to start domain Bare error: unsupported configuration: guest and host CPU are not compatible: Host CPU does not provide required features: svm, avx

src/cpu/cpu.c | 7 ++- src/cpu/cpu.h | 6 ++- src/cpu/cpu_x86.c | 123 +++++++++++++++++++++++++++++++---------------- src/qemu/qemu_command.c | 12 ++++- tests/cputest.c | 2 +- 5 files changed, 100 insertions(+), 50 deletions(-)

diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 01c31bb..b8ccd24 100644 --- a/src/cpu/cpu.c +++ b/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); }

diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 9f01f17..d7bc54e 100644 --- a/src/cpu/cpu.h +++ b/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, diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 8d92649..e1500bf 100644 --- a/src/cpu/cpu_x86.c +++ b/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);

I can't think of any case where ";;" would be a problem but I'd remove the ';' from the end of this macro anyway

+ 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,19 @@ 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("Host CPU provides forbidden features",

You need to mark the string as translatable with N_("...")

+ 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("Host CPU does not provide required features",

N_()

+ cpu_require->data); goto out; }

@@ -1204,8 +1240,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("Host CPU does not strictly match guest CPU: " + "Extra features",

N_()

+ diff->data); goto out; }

@@ -1246,22 +1283,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); }

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8dedd80..45cd417 100644 --- a/src/qemu/qemu_command.c +++ b/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); diff --git a/tests/cputest.c b/tests/cputest.c index 01db8f1..ccc17fd 100644 --- a/tests/cputest.c +++ b/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;

ACK Jirka