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

[Paolo Bonzini]

On 22/07/19 04:59, Jing Liu wrote:
> 
> 
> 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?

No, it's still not found in Intel silicon, but in principle you could
have higher indices than 1.  So it's okay, if something breaks it's the
fault of whoever set the option!

Paolo