Re: [PATCH 4/8] target/i386: add AVX10 feature and AVX10 version property

[Tao Su <tao1.su@linux.intel.com>]

On Thu, Oct 31, 2024 at 03:18:37PM +0800, Zhao Liu wrote:
> > > > > > > @@ -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().
> > > > > 
> > > > > It's not proper to get the host's version when AVX10 cannot be enabled,
> > > > > even maybe host doesn't support AVX10.
> > > > > 
> > > > > As you found out earlier, max_x86_cpu_realize doesn't know if AVX10 can
> > > > > be enabled or not.
> > > > > 
> > > > 
> > > > How about moving to x86_cpu_expand_features()? We can set when checking
> > > > cpu->max_features.
> > > 
> > > The feature bit set in x86_cpu_expand_features() is unstable since it
> > > may be masked later in x86_cpu_filter_features(). :)
> > > 
> > 
> > A lot of feature bits are set in x86_cpu_expand_features() with reported
> > value, so I think avx10_version can also be set to reported value there.
> > 
> > I mainly want to let avx10_version be assigned only when -cpu host or max,
> > so that it can be distinguished from the cpu model. This should also be
> > Paolo's original intention in v2.
> 
> OK. In this case, extend avx10-version is also consistent with the
> semantics of this function. Even if host doesn't support avx10, then in
> principle it's ok to read unimplemented avx10-version as 0.
> 
> Pls go ahead. :)

I will submit v3 based on all your comments, thanks for review :)