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

[Xiaoyao Li <xiaoyao.li@intel.com>]

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

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;
     }