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

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

On 7/18/2019 4:15 PM, Paolo Bonzini wrote:
> On 18/07/19 06:55, Jing Liu wrote:
>>>
>>> +            *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;
>> +            }
>>
> 
> Good question.  You need to add a new property, for example
> cpuid_level_func7, whose code would be modeled around cpuid_level (and a
> field cpuid_min_level_func7 whose code would be modeled around
> cpuid_min_level).
> 
> 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 ?

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?

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;
[...]
@@ -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()) {
+            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;
+    }
      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),
      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),
[...]

Thanks,
Jing


> 
> Paolo
>