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

Re: [libvirt] [PATCH] Use closest CPU model when decoding from CPUID



On Fri, Jan 15, 2010 at 04:58:59PM +0100, Jiri Denemark wrote:
> 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 files changed, 78 insertions(+), 44 deletions(-)
> 
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index 298b632..dae7c90 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)
>  {
> @@ -896,10 +945,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;
>  
> @@ -908,6 +956,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
> @@ -915,65 +965,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;
>  }
> -- 

ACK


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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