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.
> 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;
Looks good.
> @@ -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);
}
> + 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;
> + }
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.
Thanks,
Paolo
