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

[Jing Liu]

On 7/22/2019 7:50 PM, Paolo Bonzini wrote:
> 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!

Thanks very much. So would you like me to update the patch with v2 now?

Jing