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

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

On Wed, Oct 30, 2024 at 09:21:36PM +0800, Zhao Liu wrote:
> > > > 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 code implementation is actually similar for different initial
> values. And about this:
> 
> > If user sets version to 0, it will directly use reported version", 
> 
> It's defining a special behavior for the API, which is based on the
> special 0 value, and there needs to be documentation to let the user
> know that 0 will be considered legal as well as that it will be quietly
> overridden... But AFAIK there doesn't seem to be any place to add
> documentation for the property ...
> 
> There may be similar problems with -1, e.g. if the user writes -1, there
> is no way to report an error for the user's behavior. But it's better
> than 0. After all, no one would think that a version of -1 is correct.
> Topology IDs have been initialized to -1 to include the user's 0 value
> in the check.

Thanks for your explanation, but I really think the users who set
avx10-version should also know avx10.0 doesn’t exist, so using 0 is same
as -1…

To solve the initial value issue fundamentally, maybe we can add get/set
callbacks when adding avx10-version property? It should be simpler to
limit what users set.

[ ... ]

> > > @@ -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.

[ ... ]