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

Re: [libvirt] [PATCH] Enable support for nested SVM



> diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
> index def6974..c7a282e 100644
> --- a/src/cpu/cpu.c
> +++ b/src/cpu/cpu.c
> @@ -424,3 +424,27 @@ cpuUpdate(virCPUDefPtr guest,
>  
>      return driver->update(guest, host);
>  }
> +
> +bool
> +cpuHasFeature(const char *arch,
> +              const union cpuData *data,
> +              const char *feature)
> +{
> +    struct cpuArchDriver *driver;
> +
> +    VIR_DEBUG("arch=%s, data=%p, feature=%s",
> +              arch, data, feature);
> +
> +    if ((driver = cpuGetSubDriver(arch)) == NULL)
> +        return -1;
> +
> +    if (driver->hasFeature == NULL) {
> +        virCPUReportError(VIR_ERR_NO_SUPPORT,
> +                _("cannot check guest CPU data for %s architecture"),
> +                          arch);
> +        return -1;
> +    }
> +
> +    return driver->hasFeature(arch, data, feature);

No need to pass arch down here.

> +}
> +
> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> index a745917..405af48 100644
> --- a/src/cpu/cpu.h
> +++ b/src/cpu/cpu.h
> @@ -82,6 +82,11 @@ typedef int
>  (*cpuArchUpdate)    (virCPUDefPtr guest,
>                       const virCPUDefPtr host);
>  
> +typedef bool
> +(*cpuArchHasFeature) (const char *arch,
> +                      const union cpuData *data,
> +                      const char *feature);
> +

The 'arch' argument is not needed here since cpuHasFeature already selected
appropriate implementation according to CPU architecture.

>  struct cpuArchDriver {
>      const char *name;
> @@ -95,6 +100,7 @@ struct cpuArchDriver {
>      cpuArchGuestData    guestData;
>      cpuArchBaseline     baseline;
>      cpuArchUpdate       update;
> +    cpuArchHasFeature    hasFeature;
>  };
>  
>  
> @@ -151,4 +157,10 @@ extern int
>  cpuUpdate   (virCPUDefPtr guest,
>               const virCPUDefPtr host);
>  
> +extern bool
> +cpuHasFeature(const char *arch,
> +              const union cpuData *data,
> +              const char *feature);
> +
> +
>  #endif /* __VIR_CPU_H__ */
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index 1937901..cc82d58 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -1754,6 +1754,55 @@ cleanup:
>      return ret;
>  }
>  
> +static bool x86HasFeature(const char *arch ATTRIBUTE_UNUSED,
> +                          const union cpuData *data,
> +                          const char *name)

No arch argument needed here as well.

> +{
> +    struct x86_map *map;
> +    struct x86_feature *feature;
> +    bool ret = false;
> +    int i;
> +
> +    if (!(map = x86LoadMap()))
> +        return false;
> +
> +    if (!(feature = x86FeatureFind(map, name)))
> +        goto cleanup;
> +
> +    for (i = 0 ; i < data->x86.basic_len ; i++) {
> +        if (data->x86.basic[i].function == feature->cpuid->function &&
> +            ((data->x86.basic[i].eax & feature->cpuid->eax)
> +             == feature->cpuid->eax) &&
> +            ((data->x86.basic[i].ebx & feature->cpuid->ebx)
> +             == feature->cpuid->ebx) &&
> +            ((data->x86.basic[i].ecx & feature->cpuid->ecx)
> +             == feature->cpuid->ecx) &&
> +            ((data->x86.basic[i].edx & feature->cpuid->edx)
> +             == feature->cpuid->edx)) {
> +            ret = true;
> +            goto cleanup;
> +        }
> +    }
> +
> +    for (i = 0 ; i < data->x86.extended_len ; i++) {
> +        if (data->x86.extended[i].function == feature->cpuid->function &&
> +            ((data->x86.extended[i].eax & feature->cpuid->eax)
> +             == feature->cpuid->eax) &&
> +            ((data->x86.extended[i].ebx & feature->cpuid->ebx)
> +             == feature->cpuid->ebx) &&
> +            ((data->x86.extended[i].ecx & feature->cpuid->ecx)
> +             == feature->cpuid->ecx) &&
> +            ((data->x86.extended[i].edx & feature->cpuid->edx)
> +             == feature->cpuid->edx)) {
> +            ret = true;
> +            goto cleanup;
> +        }
> +    }

The two for loops should be replaced by the following single loop (which in
practise will be walked through only once):


    for (i = 0; i < feature->ncpuid; i++) {
        struct cpuX86cpuid *cpuid;

        cpuid = x86DataCpuid(data, feature->cpuid[i].function);
        if (cpuid && x86cpuidMatchMasked(cpuid, feature->cpuid + i)) {
            ret = true;
            goto cleanup;
        }
    }

> +
> +cleanup:
> +    x86MapFree(map);
> +    return ret;
> +}
>  
>  struct cpuArchDriver cpuDriverX86 = {
>      .name = "x86",
> @@ -1771,4 +1820,5 @@ struct cpuArchDriver cpuDriverX86 = {
>      .guestData  = x86GuestData,
>      .baseline   = x86Baseline,
>      .update     = x86Update,
> +    .hasFeature = x86HasFeature,
>  };

The current code would actually work so this is not a show stopper and the
changes could be done as part of a bigger cleanup patch for cpu_x86.c that I
have in my git tree. However I won't send the cleanup patch before I finish
unit tests for all this to check the cleanup doesn't break anything.

The rest looks fine.

Jirka


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