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

[Jing Liu]

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