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.
[ ... ]
