On Thu, Oct 31, 2024 at 01:52:24PM +0800, Xiaoyao Li wrote:
> Date: Thu, 31 Oct 2024 13:52:24 +0800
> From: Xiaoyao Li <xiaoyao...@intel.com>
> Subject: Re: [PATCH 4/8] target/i386: add AVX10 feature and AVX10 version
> property
>
> On 10/31/2024 12:39 PM, Tao Su wrote:
> > On Wed, Oct 30, 2024 at 11:55:34PM +0800, Zhao Liu wrote:
> > > On Wed, Oct 30, 2024 at 10:05:51PM +0800, Tao Su wrote:
> > > > Date: Wed, 30 Oct 2024 22:05:51 +0800
> > > > From: Tao Su <tao1...@linux.intel.com>
> > > > Subject: Re: [PATCH 4/8] target/i386: add AVX10 feature and AVX10
> > > > version
> > > > property
> > > >
> > > > 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…
> > >
> > > I see. "Per spec, avx10 version can never be 0", so showing the warning
> > > for avx10-version=0 is as it should be.
> > >
> > > > 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.
> > >
> > > It's unnecessary. Similar cases using -1 are already common, such as for
> > > APIC ID, NUMA node ID, topology IDs, etc. The initial value is -1 simply
> > > because we need to handle the case where users explicitly set it to 0.
> > > If you don’t want to see -1, you can define a macro like APIC ID did
> > > (#define UNSET_AVX10_VERSION -1).
> > >
> >
> > OK, I will change the default value to -1.
>
> Then please remember to handle the issue like ...
>
> > > > > > > @@ -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 agree.
>
> > 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.
>
> avx10_version needs to be assigned with a default valid value, when user
> enables avx10 explicitly without specifying avx10_version. It also applies
> to (existing) named cpu models other than GraniteRapids-v2 (which is added
> by this series). E.g.,
>
> -cpu GraniteRapids-v1,+avx10
>
> So if you are going to make default value as -1, then you need to add
> something in x86_cpu_load_model()
>
> if (!def->avx10_version) {
> def->avx10_version = -1;
> }
Yes, this is because the model's field defaults to 0, and avx10-version
is set once when the model is loaded.
Such a check seems necessary, but it does make the code more redundant,
so I'm starting to agree with default 0. :)
Thanks,
Zhao
