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

[Zhao Liu <zhao1.liu@intel.com>]

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

> > > > @@ -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(). :)

Thanks,
Zhao