Re: [Qemu-devel] [PATCH 2/2] qemu-kvm: Add svm cpuid features

[Alexander Graf]

On 10.09.2010, at 17:38, Joerg Roedel wrote:

> This patch adds the svm cpuid feature flags to the qemu
> intialization path.
> 
> Signed-off-by: Joerg Roedel <address@hidden>
> ---
> target-i386/cpu.h   |   12 +++++++
> target-i386/cpuid.c |   80 ++++++++++++++++++++++++++++++++++++++++-----------
> target-i386/kvm.c   |    3 ++
> 3 files changed, 78 insertions(+), 17 deletions(-)
> 
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 1144d4e..77eeab1 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -405,6 +405,17 @@
> #define CPUID_EXT3_IBS     (1 << 10)
> #define CPUID_EXT3_SKINIT  (1 << 12)
> 
> +#define CPUID_SVM_NPT          (1 << 0)
> +#define CPUID_SVM_LBRV         (1 << 1)
> +#define CPUID_SVM_SVMLOCK      (1 << 2)
> +#define CPUID_SVM_NRIPSAVE     (1 << 3)
> +#define CPUID_SVM_TSCSCALE     (1 << 4)
> +#define CPUID_SVM_VMCBCLEAN    (1 << 5)
> +#define CPUID_SVM_FLUSHASID    (1 << 6)
> +#define CPUID_SVM_DECODEASSIST (1 << 7)
> +#define CPUID_SVM_PAUSEFILTER  (1 << 10)
> +#define CPUID_SVM_PFTHRESHOLD  (1 << 12)
> +
> #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
> #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
> #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
> @@ -702,6 +713,7 @@ typedef struct CPUX86State {
>     uint8_t has_error_code;
>     uint32_t sipi_vector;
>     uint32_t cpuid_kvm_features;
> +    uint32_t cpuid_svm_features;
> 
>     /* in order to simplify APIC support, we leave this pointer to the
>        user */
> diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
> index 3fcf78f..ea1ac73 100644
> --- a/target-i386/cpuid.c
> +++ b/target-i386/cpuid.c
> @@ -79,6 +79,17 @@ static const char *kvm_feature_name[] = {
>     NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> };
> 
> +static const char *svm_feature_name[] = {
> +    "npt", "lbrv", "svm_lock", "nrip_save",
> +    "tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
> +    NULL, NULL, "pause_filter", NULL,
> +    "pfthreshold", NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +};
> +
> /* collects per-function cpuid data
>  */
> typedef struct model_features_t {
> @@ -192,13 +203,15 @@ static void add_flagname_to_bitmaps(const char 
> *flagname, uint32_t *features,
>                                     uint32_t *ext_features,
>                                     uint32_t *ext2_features,
>                                     uint32_t *ext3_features,
> -                                    uint32_t *kvm_features)
> +                                    uint32_t *kvm_features,
> +                                    uint32_t *svm_features)
> {
>     if (!lookup_feature(features, flagname, NULL, feature_name) &&
>         !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
>         !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
>         !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
> -        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name))
> +        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
> +        !lookup_feature(svm_features, flagname, NULL, svm_feature_name))
>             fprintf(stderr, "CPU feature %s not found\n", flagname);
> }
> 
> @@ -210,7 +223,8 @@ typedef struct x86_def_t {
>     int family;
>     int model;
>     int stepping;
> -    uint32_t features, ext_features, ext2_features, ext3_features, 
> kvm_features;
> +    uint32_t features, ext_features, ext2_features, ext3_features;
> +    uint32_t kvm_features, svm_features;
>     uint32_t xlevel;
>     char model_id[48];
>     int vendor_override;
> @@ -253,6 +267,7 @@ typedef struct x86_def_t {
>           CPUID_EXT2_PDPE1GB */
> #define TCG_EXT3_FEATURES (CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM | \
>           CPUID_EXT3_CR8LEG | CPUID_EXT3_ABM | CPUID_EXT3_SSE4A)
> +#define TCG_SVM_FEATURES 0
> 
> /* maintains list of cpu model definitions
>  */
> @@ -278,6 +293,8 @@ static x86_def_t builtin_x86_defs[] = {
>             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
>         .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
>             CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
> +        .svm_features = CPUID_SVM_NPT | CPUID_SVM_LBRV | CPUID_SVM_NRIPSAVE |
> +            CPUID_SVM_VMCBCLEAN,
>         .xlevel = 0x8000000A,
>         .model_id = "QEMU Virtual CPU version " QEMU_VERSION,
>     },
> @@ -305,6 +322,8 @@ static x86_def_t builtin_x86_defs[] = {
>                     CPUID_EXT3_OSVW, CPUID_EXT3_IBS */
>         .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
>             CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
> +        .svm_features = CPUID_SVM_NPT | CPUID_SVM_LBRV | CPUID_SVM_NRIPSAVE |
> +            CPUID_SVM_VMCBCLEAN,

Does that phenom already do all those? It does NPT, but I'm not sure about 
NRIPSAVE for example.

>         .xlevel = 0x8000001A,
>         .model_id = "AMD Phenom(tm) 9550 Quad-Core Processor"
>     },
> @@ -505,6 +524,15 @@ static int cpu_x86_fill_host(x86_def_t *x86_cpu_def)
>     cpu_x86_fill_model_id(x86_cpu_def->model_id);
>     x86_cpu_def->vendor_override = 0;
> 
> +
> +    /*
> +     * Every SVM feature requires emulation support in KVM - so we can't just
> +     * read the host features here. KVM might even support SVM features not
> +     * available on the host hardware
> +     */
> +    x86_cpu_def->svm_features = CPUID_SVM_NPT | CPUID_SVM_LBRV |
> +                                CPUID_SVM_NRIPSAVE | CPUID_SVM_VMCBCLEAN;

Hrm. Wouldn't it make more sense to declare this to -1? This will still go 
through the kernel space matcher which tells us which features are available 
anyways, right?


> +
>     return 0;
> }
> 
> @@ -560,8 +588,14 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, 
> const char *cpu_model)
> 
>     char *s = strdup(cpu_model);
>     char *featurestr, *name = strtok(s, ",");
> -    uint32_t plus_features = 0, plus_ext_features = 0, plus_ext2_features = 
> 0, plus_ext3_features = 0, plus_kvm_features = 0;
> -    uint32_t minus_features = 0, minus_ext_features = 0, minus_ext2_features 
> = 0, minus_ext3_features = 0, minus_kvm_features = 0;
> +    /* Features to be added*/
> +    uint32_t plus_features = 0, plus_ext_features = 0;
> +    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
> +    uint32_t plus_kvm_features = 0, plus_svm_features = 0;
> +    /* Features to be removed */
> +    uint32_t minus_features = 0, minus_ext_features = 0;
> +    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
> +    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
>     uint32_t numvalue;
> 
>     for (def = x86_defs; def; def = def->next)
> @@ -579,16 +613,22 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, 
> const char *cpu_model)
> 
>     add_flagname_to_bitmaps("hypervisor", &plus_features,
>         &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> -        &plus_kvm_features);
> +        &plus_kvm_features, &plus_svm_features);
> 
>     featurestr = strtok(NULL, ",");
> 
>     while (featurestr) {
>         char *val;
>         if (featurestr[0] == '+') {
> -            add_flagname_to_bitmaps(featurestr + 1, &plus_features, 
> &plus_ext_features, &plus_ext2_features, &plus_ext3_features, 
> &plus_kvm_features);
> +            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> +                            &plus_ext_features, &plus_ext2_features,
> +                            &plus_ext3_features, &plus_kvm_features,
> +                            &plus_svm_features);
>         } else if (featurestr[0] == '-') {
> -            add_flagname_to_bitmaps(featurestr + 1, &minus_features, 
> &minus_ext_features, &minus_ext2_features, &minus_ext3_features, 
> &minus_kvm_features);
> +            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
> +                            &minus_ext_features, &minus_ext2_features,
> +                            &minus_ext3_features, &minus_kvm_features,
> +                            &minus_svm_features);
>         } else if ((val = strchr(featurestr, '='))) {
>             *val = 0; val++;
>             if (!strcmp(featurestr, "family")) {
> @@ -670,11 +710,13 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, 
> const char *cpu_model)
>     x86_cpu_def->ext2_features |= plus_ext2_features;
>     x86_cpu_def->ext3_features |= plus_ext3_features;
>     x86_cpu_def->kvm_features |= plus_kvm_features;
> +    x86_cpu_def->svm_features |= plus_svm_features;
>     x86_cpu_def->features &= ~minus_features;
>     x86_cpu_def->ext_features &= ~minus_ext_features;
>     x86_cpu_def->ext2_features &= ~minus_ext2_features;
>     x86_cpu_def->ext3_features &= ~minus_ext3_features;
>     x86_cpu_def->kvm_features &= ~minus_kvm_features;
> +    x86_cpu_def->svm_features &= ~minus_svm_features;
>     if (check_cpuid) {
>         if (check_features_against_host(x86_cpu_def) && enforce_cpuid)
>             goto error;
> @@ -816,6 +858,7 @@ int cpu_x86_register (CPUX86State *env, const char 
> *cpu_model)
>     env->cpuid_ext3_features = def->ext3_features;
>     env->cpuid_xlevel = def->xlevel;
>     env->cpuid_kvm_features = def->kvm_features;
> +    env->cpuid_svm_features = def->svm_features;
>     if (!kvm_enabled()) {
>         env->cpuid_features &= TCG_FEATURES;
>         env->cpuid_ext_features &= TCG_EXT_FEATURES;
> @@ -825,6 +868,7 @@ int cpu_x86_register (CPUX86State *env, const char 
> *cpu_model)
> #endif
>             );
>         env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
> +        env->cpuid_svm_features &= TCG_SVM_FEATURES;
>     }
>     {
>         const char *model_id = def->model_id;
> @@ -1135,11 +1179,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>                 *ecx |= 1 << 1;    /* CmpLegacy bit */
>             }
>         }
> -
> -        if (kvm_enabled()) {
> -            /* Nested SVM not yet supported in upstream QEMU */
> -            *ecx &= ~CPUID_EXT3_SVM;
> -        }

Have you made sure that the default cpu type doesn't enable the SVM bit? I 
couldn't find any trace of an override to "kvm64" as default type when KVM is 
used.

The rest looks good :). Thanks a lot for this patch set!


Alex