On Wed, Oct 30, 2024 at 04:44:54PM +0800, Zhao Liu wrote: > On Tue, Oct 29, 2024 at 04:18:54PM +0100, Paolo Bonzini wrote: > > Date: Tue, 29 Oct 2024 16:18:54 +0100 > > From: Paolo Bonzini <pbonz...@redhat.com> > > Subject: [PATCH 4/8] target/i386: add AVX10 feature and AVX10 version > > property > > X-Mailer: git-send-email 2.47.0 > > > > From: Tao Su <tao1...@linux.intel.com> > > > > When AVX10 enable bit is set, the 0x24 leaf will be present as "AVX10 > > Converged Vector ISA leaf" containing fields for the version number and > > the supported vector bit lengths. > > > > Introduce avx10-version property so that avx10 version can be controlled > > by user and cpu model. Per spec, avx10 version can never be 0, the default > > value of avx10-version is set to 0 to determine whether it is specified by > > user. > > The default value of 0 does not reflect whether the user has set it to 0. > According to the description here, the spec clearly prohibits 0, so > should we report an error when the user sets it to 0? > > If so, it might be better to change the default value to -1 and adjust > based on the host's support. >
If user sets version to 0, it will directly use reported version, this should be a more neat and intuitive way? > > The default can come from the device model or, for the max model, > > from KVM's reported value. > > > > Signed-off-by: Tao Su <tao1...@linux.intel.com> > > Link: > > https://lore.kernel.org/r/20241028024512.156724-3-tao1...@linux.intel.com > > Link: > > https://lore.kernel.org/r/20241028024512.156724-4-tao1...@linux.intel.com > > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> > > --- > > target/i386/cpu.h | 4 +++ > > target/i386/cpu.c | 64 ++++++++++++++++++++++++++++++++++++++----- > > target/i386/kvm/kvm.c | 3 +- > > 3 files changed, 63 insertions(+), 8 deletions(-) > > [snip] > > > @@ -7611,7 +7644,23 @@ static bool x86_cpu_filter_features(X86CPU *cpu, > > bool verbose) > > } > > } > > > > - return x86_cpu_have_filtered_features(cpu); > > + have_filtered_features = x86_cpu_have_filtered_features(cpu); > > + > > + if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) { > > + x86_cpu_get_supported_cpuid(0x24, 0, > > + &eax_0, &ebx_0, &ecx_0, &edx_0); > > + uint8_t version = ebx_0 & 0xff; > > + > > + if (version < env->avx10_version) { > > + if (prefix) { > > + warn_report("%s: avx10.%d", prefix, env->avx10_version); > > Perhaps also tell user about revised version? > > warn_report("%s: avx10.%d. Adjust to avx10.%d", > prefix, env->avx10_version, version); > I see, thanks! > > + } > > + env->avx10_version = version; > > + have_filtered_features = true; > > + } > > + } > > > Per Tao's comment, perhaps we can check AVX10 and version here (default > version is 0): > > @@ -7674,13 +7682,21 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool > verbose) > &eax_0, &ebx_0, &ecx_0, &edx_0); > uint8_t version = ebx_0 & 0xff; > > - if (version < env->avx10_version) { > + if (!env->avx10_version) { > + env->avx10_version = version; x86_cpu_filter_features() is not a good place to assign avx10_version, I still tend to set it in max_x86_cpu_realize(). > + } else (version < env->avx10_version) { > if (prefix) { > - warn_report("%s: avx10.%d", prefix, env->avx10_version); > + warn_report("%s: avx10.%d. Adjust to avx10.%d", > + prefix, env->avx10_version, version); > } > env->avx10_version = version; > have_filtered_features = true; > } > + } else if (env->avx10_version && prefix) { > + if (prefix) { I think it is reasonable, especially when we don't check AVX10 enable bit in max_x86_cpu_realize(). But checking prefix here again seems not necessary. > + warn_report("%s: avx10.%d.", prefix, env->avx10_version); > + } > + have_filtered_features = true; > } > > return have_filtered_features; > > > + return have_filtered_features; > > } > > > > static void x86_cpu_hyperv_realize(X86CPU *cpu) > > @@ -8395,6 +8444,7 @@ static Property x86_cpu_properties[] = { > > DEFINE_PROP_UINT32("min-level", X86CPU, env.cpuid_min_level, 0), > > DEFINE_PROP_UINT32("min-xlevel", X86CPU, env.cpuid_min_xlevel, 0), > > DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0), > > + DEFINE_PROP_UINT8("avx10-version", X86CPU, env.avx10_version, 0), > > As my first comment, we could consider changing the default value to -1. > I still think 0 is better…
