Re: [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling

[Jing Liu <jing2.liu@linux.intel.com>]

On 7/11/2019 1:38 PM, Jing Liu wrote:
> Intel CooperLake cpu adds AVX512_BF16 instruction, defining as
> CPUID.(EAX=7,ECX=1):EAX[bit 05].
> 
> The release spec link as follows,
> https://software.intel.com/sites/default/files/managed/c5/15/\
> architecture-instruction-set-extensions-programming-reference.pdf
> 
> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
> ---
>   target/i386/cpu.c | 29 ++++++++++++++++++++++++++++-
>   target/i386/cpu.h |  3 +++
>   target/i386/kvm.c |  3 ++-
>   3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c1ab86d..de3daf5 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -767,6 +767,7 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
>             /* CPUID_7_0_ECX_OSPKE is dynamic */ \
>             CPUID_7_0_ECX_LA57)
>   #define TCG_7_0_EDX_FEATURES 0
> +#define TCG_7_1_EAX_FEATURES 0
>   #define TCG_APM_FEATURES 0
>   #define TCG_6_EAX_FEATURES CPUID_6_EAX_ARAT
>   #define TCG_XSAVE_FEATURES (CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XGETBV1)
> @@ -1092,6 +1093,25 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>           },
>           .tcg_features = TCG_7_0_EDX_FEATURES,
>       },
> +    [FEAT_7_1_EAX] = {
> +        .type = CPUID_FEATURE_WORD,
> +        .feat_names = {
> +            NULL, NULL, NULL, NULL,
> +            NULL, "avx512-bf16", 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,
> +        },
> +        .cpuid = {
> +            .eax = 7,
> +            .needs_ecx = true, .ecx = 1,
> +            .reg = R_EAX,
> +        },
> +        .tcg_features = TCG_7_1_EAX_FEATURES,
> +    },
>       [FEAT_8000_0007_EDX] = {
>           .type = CPUID_FEATURE_WORD,
>           .feat_names = {
> @@ -4344,13 +4364,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>       case 7:
>           /* Structured Extended Feature Flags Enumeration Leaf */
>           if (count == 0) {
> -            *eax = 0; /* Maximum ECX value for sub-leaves */
> +            /* Maximum ECX value for sub-leaves */
> +            *eax = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x7,
> +                                                count, R_EAX);
This needs to be firstly checked as follows, otherwise some 
architectures would fail to compile.

What about hvf and tcg CPUID 07 EAX value?

+            /* Maximum ECX value for sub-leaves */
+            if (kvm_enabled()) {
+                *eax = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x7,
+                                                    count, R_EAX);
+            } else if (hvf_enabled()) {
+                *eax = hvf_get_supported_cpuid(0x7, count, R_EAX);
+            } else {
+                *eax = 0;
+            }


Thanks,
Jing

>               *ebx = env->features[FEAT_7_0_EBX]; /* Feature flags */
>               *ecx = env->features[FEAT_7_0_ECX]; /* Feature flags */
>               if ((*ecx & CPUID_7_0_ECX_PKU) && env->cr[4] & CR4_PKE_MASK) {
>                   *ecx |= CPUID_7_0_ECX_OSPKE;
>               }
>               *edx = env->features[FEAT_7_0_EDX]; /* Feature flags */
> +        } else if (count == 1) {
> +            *eax = env->features[FEAT_7_1_EAX];
> +            *ebx = 0;
> +            *ecx = 0;
> +            *edx = 0;
>           } else {
>               *eax = 0;
>               *ebx = 0;
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index bd06523..40594a1 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -488,6 +488,7 @@ typedef enum FeatureWord {
>       FEAT_7_0_EBX,       /* CPUID[EAX=7,ECX=0].EBX */
>       FEAT_7_0_ECX,       /* CPUID[EAX=7,ECX=0].ECX */
>       FEAT_7_0_EDX,       /* CPUID[EAX=7,ECX=0].EDX */
> +    FEAT_7_1_EAX,       /* CPUID[EAX=7,ECX=1].EAX */
>       FEAT_8000_0001_EDX, /* CPUID[8000_0001].EDX */
>       FEAT_8000_0001_ECX, /* CPUID[8000_0001].ECX */
>       FEAT_8000_0007_EDX, /* CPUID[8000_0007].EDX */
> @@ -699,6 +700,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>   #define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29)  /*Arch Capabilities*/
>   #define CPUID_7_0_EDX_SPEC_CTRL_SSBD  (1U << 31) /* Speculative Store Bypass Disable */
>   
> +#define CPUID_7_1_EAX_AVX512_BF16 (1U << 5) /* AVX512 BFloat16 Instruction */
> +
>   #define CPUID_8000_0008_EBX_WBNOINVD  (1U << 9)  /* Write back and
>                                                                                do not invalidate cache */
>   #define CPUID_8000_0008_EBX_IBPB    (1U << 12) /* Indirect Branch Prediction Barrier */
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 3b29ce5..977aaa5 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1110,6 +1110,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>                   c = &cpuid_data.entries[cpuid_i++];
>               }
>               break;
> +        case 0x7:
>           case 0x14: {
>               uint32_t times;
>   
> @@ -1122,7 +1123,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>               for (j = 1; j <= times; ++j) {
>                   if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
>                       fprintf(stderr, "cpuid_data is full, no space for "
> -                                "cpuid(eax:0x14,ecx:0x%x)\n", j);
> +                                "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
>                       abort();
>                   }
>                   c = &cpuid_data.entries[cpuid_i++];
>