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