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

[Paolo Bonzini]

On 19/07/19 09:20, Jing Liu wrote:
>> Then CPUID[7,0].EAX is set automatically to 0 or 1 depending on whether
>> BF16 is enabled or not.
> 
> Could I ask why don't we directly check BF16 enabling when
> cpu_x86_cpuid(env, 7, 0, ...) during kvm_arch_init_vcpu ?

Because the code for setting CPUID is common for all accelerators (there
are five supported: KVM, HAX, HVF, TCG, WHPX).

> What is the use of the two new properties? Are they used for users
> setting parameters when boot up guest, and why we need users setting
> func7 level?

For example to test guests with CPUID[7,0].EAX==1, even if the host does
not have BF16.

> I tried to implement the code as follows.
> 
> @@ -4293,13 +4313,19 @@ 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 = env->cpuid_level_func7;
> [...]
> +        } else if (count == 1) {
> +            *eax = env->features[FEAT_7_1_EAX];
> +            *ebx = 0;
> +            *ecx = 0;
> +            *edx = 0;

Looks good.

> @@ -5075,6 +5101,10 @@ static void x86_cpu_expand_features(X86CPU *cpu,
> Error **errp)
>          x86_cpu_adjust_feat_level(cpu, FEAT_SVM);
>          x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE);
> 
> +       if ((env->features[FEAT_7_1_EAX] & CPUID_7_1_EAX_AVX512_BF16) &&
> +            kvm_enabled()) {

No need to check KVM.  You could also do just
x86_cpu_adjust_feat_level(cpu, FEAT_7_1_EAX) and set
cpu->min_level_func7 in x86_cpu_adjust_feat_level with something like

    if (eax == 7) {
        x86_cpu_adjust_level(cpu, &env->cpu_min_level_func7,
                             fi->cpuid.ecx);
    }


> +            x86_cpu_adjust_level(cpu, &env->cpuid_min_level_func7, 1);
> +        }
>          /* Intel Processor Trace requires CPUID[0x14] */
>          if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
>               kvm_enabled() && cpu->intel_pt_auto_level) {
> @@ -5098,6 +5128,9 @@ static void x86_cpu_expand_features(X86CPU *cpu,
> Error **errp)
>      }
> 
>      /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly
> set */
> +    if (env->cpuid_level_func7 == UINT32_MAX) {
> +        env->cpuid_level_func7 = env->cpuid_min_level_func7;
> +    }

Looks good.

>      if (env->cpuid_level == UINT32_MAX) {
>          env->cpuid_level = env->cpuid_min_level;
>      }
> @@ -5869,9 +5902,11 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
>      DEFINE_PROP_UINT8("host-phys-bits-limit", X86CPU,
> host_phys_bits_limit, 0),
>      DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
> +    DEFINE_PROP_UINT32("level-func7", X86CPU, env.cpuid_level_func7,
> UINT32_MAX),

Looks good.

>      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, UINT32_MAX),
>      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, UINT32_MAX),
>      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, UINT32_MAX),
> +    DEFINE_PROP_UINT32("min-level-func7", X86CPU,
> env.cpuid_min_level_func7, 0),

No need for this property, just like there is no min-level property.

Thanks,

Paolo