Re: [Qemu-devel] [PATCH] target-i386: add feature flags for CPUID[EAX=0xd, ECX=1]

[Eduardo Habkost]

On Tue, Nov 25, 2014 at 06:35:42PM +0100, Paolo Bonzini wrote:
> These represent xsave-related capabilities of the processor, and KVM may
> or may not support them.
> 
> Add feature bits so that they are considered by "-cpu ...,enforce", and use
> the new feature work instead of calling kvm_arch_get_supported_cpuid.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>

Cool, eliminating all kvm_arch_get_supported_cpuid() calls from
cpu_x86_cpuid() is on my TODO-list.

I just have one question below:

> ---
>  target-i386/cpu.c | 28 +++++++++++++++++++++++++++-
>  target-i386/cpu.h |  6 ++++++
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e9df33e..a2dde11 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -274,6 +274,17 @@ static const char *cpuid_apm_edx_feature_name[] = {
>      NULL, NULL, NULL, NULL,
>  };
>  
> +static const char *cpuid_xsave_feature_name[] = {
> +    "xsaveopt", "xsavec", "xgetbv1", "xsaves",

None of the above features introduce any new state that might need to be
migrated, or will require other changes in QEMU to work, right?

It looks like they don't introduce any extra state, but if they do, they
need to be added to unmigratable_flags until migration support is
implemented.

If they require other QEMU changes, it would be nice if KVM reported
them using KVM_CHECK_EXTENSION instead of GET_SUPPORTED_CPUID, so it
wouldn't break "-cpu host".


> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +};
> +
>  #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
>  #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
>            CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC)
> @@ -391,6 +402,13 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] 
> = {
>          .tcg_features = TCG_APM_FEATURES,
>          .unmigratable_flags = CPUID_APM_INVTSC,
>      },
> +    [FEAT_XSAVE] = {
> +        .feat_names = cpuid_xsave_feature_name,
> +        .cpuid_eax = 0xd,
> +        .cpuid_needs_ecx = true, .cpuid_ecx = 1,
> +        .cpuid_reg = R_EAX,
> +        .tcg_features = 0,
> +    },
>  };
>  
>  typedef struct X86RegisterInfo32 {
> @@ -1018,6 +1036,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_EXT2_SYSCALL,
>          .features[FEAT_8000_0001_ECX] =
>              CPUID_EXT3_LAHF_LM,
> +        .features[FEAT_XSAVE] =
> +            CPUID_XSAVE_XSAVEOPT,
>          .xlevel = 0x8000000A,
>          .model_id = "Intel Xeon E312xx (Sandy Bridge)",
>      },
> @@ -1051,6 +1071,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP |
>              CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID |
>              CPUID_7_0_EBX_RTM,
> +        .features[FEAT_XSAVE] =
> +            CPUID_XSAVE_XSAVEOPT,
>          .xlevel = 0x8000000A,
>          .model_id = "Intel Core Processor (Haswell)",
>      },
> @@ -1085,6 +1107,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID |
>              CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX |
>              CPUID_7_0_EBX_SMAP,
> +        .features[FEAT_XSAVE] =
> +            CPUID_XSAVE_XSAVEOPT,
>          .xlevel = 0x8000000A,
>          .model_id = "Intel Core Processor (Broadwell)",
>      },
> @@ -1202,6 +1226,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_EXT3_3DNOWPREFETCH | CPUID_EXT3_MISALIGNSSE |
>              CPUID_EXT3_SSE4A | CPUID_EXT3_ABM | CPUID_EXT3_SVM |
>              CPUID_EXT3_LAHF_LM,
> +        /* no xsaveopt! */
>          .xlevel = 0x8000001A,
>          .model_id = "AMD Opteron 62xx class CPU",
>      },
> @@ -1236,6 +1261,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_EXT3_3DNOWPREFETCH | CPUID_EXT3_MISALIGNSSE |
>              CPUID_EXT3_SSE4A | CPUID_EXT3_ABM | CPUID_EXT3_SVM |
>              CPUID_EXT3_LAHF_LM,
> +        /* no xsaveopt! */
>          .xlevel = 0x8000001A,
>          .model_id = "AMD Opteron 63xx class CPU",
>      },
> @@ -2377,7 +2403,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>              *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE);
>              *ebx = *ecx;
>          } else if (count == 1) {
> -            *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX);
> +            *eax = env->features[FEAT_XSAVE];
>          } else if (count < ARRAY_SIZE(ext_save_areas)) {
>              const ExtSaveArea *esa = &ext_save_areas[count];
>              if ((env->features[esa->feature] & esa->bits) == esa->bits &&
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 015f5b5..f9d74c7 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -411,6 +411,7 @@ typedef enum FeatureWord {
>      FEAT_C000_0001_EDX, /* CPUID[C000_0001].EDX */
>      FEAT_KVM,           /* CPUID[4000_0001].EAX (KVM_CPUID_FEATURES) */
>      FEAT_SVM,           /* CPUID[8000_000A].EDX */
> +    FEAT_XSAVE,         /* CPUID[EAX=0xd,ECX=1].EAX */
>      FEATURE_WORDS,
>  } FeatureWord;
>  
> @@ -571,6 +572,11 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_7_0_EBX_AVX512ER (1U << 27) /* AVX-512 Exponential and 
> Reciprocal */
>  #define CPUID_7_0_EBX_AVX512CD (1U << 28) /* AVX-512 Conflict Detection */
>  
> +#define CPUID_XSAVE_XSAVEOPT   (1U << 0)
> +#define CPUID_XSAVE_XSAVEC     (1U << 1)
> +#define CPUID_XSAVE_XGETBV1    (1U << 2)
> +#define CPUID_XSAVE_XSAVES     (1U << 3)
> +
>  /* CPUID[0x80000007].EDX flags: */
>  #define CPUID_APM_INVTSC       (1U << 8)
>  
> -- 
> 1.8.3.1
> 

-- 
Eduardo