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

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