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

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

On 7/19/2019 4:10 PM, Paolo Bonzini wrote:
> 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.

Thanks. :)
> 
> 
>> @@ -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);
>      }
> 

Got it. One question I'm wondering is, is it possible for users setting
an invalid property like level-func7=2? Do we need some protection?

>> @@ -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.
> 
Would remove it.

Thanks,
Jing