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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list