[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] cpu: Improve error reporting on incompatible CPUs



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]