From: Tao Su <tao1.su@linux.intel.com>
When AVX10 enable bit is set, the 0x24 leaf will be present as "AVX10
Converged Vector ISA leaf" containing fields for the version number and
the supported vector bit lengths.
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 can come from the device model or, for the max model,
from KVM's reported value.
Signed-off-by: Tao Su <tao1.su@linux.intel.com>
Link: https://lore.kernel.org/r/20241028024512.156724-3-tao1.su@linux.intel.com
Link: https://lore.kernel.org/r/20241028024512.156724-4-tao1.su@linux.intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.h | 4 +++
target/i386/cpu.c | 64 ++++++++++++++++++++++++++++++++++++++-----
target/i386/kvm/kvm.c | 3 +-
3 files changed, 63 insertions(+), 8 deletions(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index a0a122cb5bf..72e98b25114 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -975,6 +975,8 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
#define CPUID_7_1_EDX_AMX_COMPLEX (1U << 8)
/* PREFETCHIT0/1 Instructions */
#define CPUID_7_1_EDX_PREFETCHITI (1U << 14)
+/* Support for Advanced Vector Extensions 10 */
+#define CPUID_7_1_EDX_AVX10 (1U << 19)
/* Flexible return and event delivery (FRED) */
#define CPUID_7_1_EAX_FRED (1U << 17)
/* Load into IA32_KERNEL_GS_BASE (LKGS) */
@@ -1954,6 +1956,8 @@ typedef struct CPUArchState {
uint32_t cpuid_vendor3;
uint32_t cpuid_version;
FeatureWordArray features;
+ /* AVX10 version */
+ uint8_t avx10_version;
/* Features that were explicitly enabled/disabled */
FeatureWordArray user_features;
uint32_t cpuid_model[12];
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c2f6045ec1c..fdbfcc59da4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -46,6 +46,9 @@
#include "cpu-internal.h"
static void x86_cpu_realizefn(DeviceState *dev, Error **errp);
+static void x86_cpu_get_supported_cpuid(uint32_t func, uint32_t index,
+ uint32_t *eax, uint32_t *ebx,
+ uint32_t *ecx, uint32_t *edx);
/* Helpers for building CPUID[2] descriptors: */
@@ -1132,7 +1135,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
"avx-vnni-int8", "avx-ne-convert", NULL, NULL,
"amx-complex", NULL, "avx-vnni-int16", NULL,
NULL, NULL, "prefetchiti", NULL,
- NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, "avx10",
NULL, NULL, NULL, NULL,
NULL, NULL, NULL, NULL,
NULL, NULL, NULL, NULL,
@@ -1989,6 +1992,7 @@ typedef struct X86CPUDefinition {
int family;
int model;
int stepping;
+ int avx10_version;
FeatureWordArray features;
const char *model_id;
const CPUCaches *const cache_info;
@@ -5338,6 +5342,8 @@ static Property max_x86_cpu_properties[] = {
static void max_x86_cpu_realize(DeviceState *dev, Error **errp)
{
Object *obj = OBJECT(dev);
+ X86CPU *cpu = X86_CPU(dev);
+ CPUX86State *env = &cpu->env;
if (!object_property_get_int(obj, "family", &error_abort)) {
if (X86_CPU(obj)->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
@@ -5351,6 +5357,14 @@ static void max_x86_cpu_realize(DeviceState *dev, Error **errp)
}
}
+ if ((env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) && !env->avx10_version) {
+ uint32_t eax, ebx, ecx, edx;
+ x86_cpu_get_supported_cpuid(0x24, 0,
+ &eax, &ebx, &ecx, &edx);
+
+ env->avx10_version = ebx & 0xff;
+ }
+
x86_cpu_realizefn(dev, errp);
}
@@ -6331,6 +6345,9 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
*/
object_property_set_str(OBJECT(cpu), "vendor", def->vendor, &error_abort);
+ object_property_set_int(OBJECT(cpu), "avx10-version", def->avx10_version,
+ &error_abort);
+
x86_cpu_apply_version_props(cpu, model);
/*
@@ -6859,6 +6876,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
}
break;
}
+ case 0x24: {
+ *eax = 0;
+ *ebx = 0;
+ *ecx = 0;
+ *edx = 0;
+ if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
+ *ebx = env->avx10_version;
+ }
+ break;
+ }
case 0x40000000:
/*
* CPUID code in kvm_arch_init_vcpu() ignores stuff
@@ -7513,6 +7540,11 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
}
+ /* Advanced Vector Extensions 10 (AVX10) requires CPUID[0x24] */
+ if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
+ x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x24);
+ }
+
/* SVM requires CPUID[0x8000000A] */
if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000000A);
@@ -7563,6 +7595,10 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
CPUX86State *env = &cpu->env;
FeatureWord w;
const char *prefix = NULL;
+ bool have_filtered_features;
+
+ uint32_t eax_0, ebx_0, ecx_0, edx_0;
+ uint32_t eax_1, ebx_1, ecx_1, edx_1;
if (verbose) {
prefix = accel_uses_host_cpuid()
@@ -7584,13 +7620,10 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
*/
if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
kvm_enabled()) {
- uint32_t eax_0, ebx_0, ecx_0, edx_0_unused;
- uint32_t eax_1, ebx_1, ecx_1_unused, edx_1_unused;
-
x86_cpu_get_supported_cpuid(0x14, 0,
- &eax_0, &ebx_0, &ecx_0, &edx_0_unused);
+ &eax_0, &ebx_0, &ecx_0, &edx_0);
x86_cpu_get_supported_cpuid(0x14, 1,
- &eax_1, &ebx_1, &ecx_1_unused, &edx_1_unused);
+ &eax_1, &ebx_1, &ecx_1, &edx_1);
if (!eax_0 ||
((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
@@ -7611,7 +7644,23 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
}
}
- return x86_cpu_have_filtered_features(cpu);
+ have_filtered_features = x86_cpu_have_filtered_features(cpu);
+
+ if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
+ x86_cpu_get_supported_cpuid(0x24, 0,
+ &eax_0, &ebx_0, &ecx_0, &edx_0);
+ uint8_t version = ebx_0 & 0xff;
+
+ if (version < env->avx10_version) {
+ if (prefix) {
+ warn_report("%s: avx10.%d", prefix, env->avx10_version);
+ }
+ env->avx10_version = version;
+ have_filtered_features = true;
+ }
+ }
+
+ return have_filtered_features;
}
static void x86_cpu_hyperv_realize(X86CPU *cpu)
@@ -8395,6 +8444,7 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_UINT32("min-level", X86CPU, env.cpuid_min_level, 0),
DEFINE_PROP_UINT32("min-xlevel", X86CPU, env.cpuid_min_xlevel, 0),
DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
+ DEFINE_PROP_UINT8("avx10-version", X86CPU, env.avx10_version, 0),
DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index fd9f1988920..8e17942c3ba 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1923,7 +1923,8 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env,
case 0x7:
case 0x14:
case 0x1d:
- case 0x1e: {
+ case 0x1e:
+ case 0x24: {
uint32_t times;
c->function = i;
--
2.47.0On Tue, Oct 29, 2024 at 04:18:54PM +0100, Paolo Bonzini wrote:
> Date: Tue, 29 Oct 2024 16:18:54 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 4/8] target/i386: add AVX10 feature and AVX10 version
> property
> X-Mailer: git-send-email 2.47.0
>
> From: Tao Su <tao1.su@linux.intel.com>
>
> When AVX10 enable bit is set, the 0x24 leaf will be present as "AVX10
> Converged Vector ISA leaf" containing fields for the version number and
> the supported vector bit lengths.
>
> 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.
> The default can come from the device model or, for the max model,
> from KVM's reported value.
>
> Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> Link: https://lore.kernel.org/r/20241028024512.156724-3-tao1.su@linux.intel.com
> Link: https://lore.kernel.org/r/20241028024512.156724-4-tao1.su@linux.intel.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> target/i386/cpu.h | 4 +++
> target/i386/cpu.c | 64 ++++++++++++++++++++++++++++++++++++++-----
> target/i386/kvm/kvm.c | 3 +-
> 3 files changed, 63 insertions(+), 8 deletions(-)
[snip]
> @@ -7611,7 +7644,23 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
> }
> }
>
> - return x86_cpu_have_filtered_features(cpu);
> + have_filtered_features = x86_cpu_have_filtered_features(cpu);
> +
> + if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
> + x86_cpu_get_supported_cpuid(0x24, 0,
> + &eax_0, &ebx_0, &ecx_0, &edx_0);
> + uint8_t version = ebx_0 & 0xff;
> +
> + if (version < env->avx10_version) {
> + if (prefix) {
> + warn_report("%s: avx10.%d", prefix, env->avx10_version);
Perhaps also tell user about revised version?
warn_report("%s: avx10.%d. Adjust to avx10.%d",
prefix, env->avx10_version, version);
> + }
> + env->avx10_version = version;
> + have_filtered_features = true;
> + }
> + }
Per Tao's comment, perhaps we can check AVX10 and version here (default
version is 0):
@@ -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;
+ } else (version < env->avx10_version) {
if (prefix) {
- warn_report("%s: avx10.%d", prefix, env->avx10_version);
+ warn_report("%s: avx10.%d. Adjust to avx10.%d",
+ prefix, env->avx10_version, version);
}
env->avx10_version = version;
have_filtered_features = true;
}
+ } else if (env->avx10_version && prefix) {
+ if (prefix) {
+ warn_report("%s: avx10.%d.", prefix, env->avx10_version);
+ }
+ have_filtered_features = true;
}
return have_filtered_features;
> + return have_filtered_features;
> }
>
> static void x86_cpu_hyperv_realize(X86CPU *cpu)
> @@ -8395,6 +8444,7 @@ static Property x86_cpu_properties[] = {
> DEFINE_PROP_UINT32("min-level", X86CPU, env.cpuid_min_level, 0),
> DEFINE_PROP_UINT32("min-xlevel", X86CPU, env.cpuid_min_xlevel, 0),
> DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
> + DEFINE_PROP_UINT8("avx10-version", X86CPU, env.avx10_version, 0),
As my first comment, we could consider changing the default value to -1.
Thanks,
Zhao
On Wed, Oct 30, 2024 at 04:44:54PM +0800, Zhao Liu wrote:
> On Tue, Oct 29, 2024 at 04:18:54PM +0100, Paolo Bonzini wrote:
> > Date: Tue, 29 Oct 2024 16:18:54 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: [PATCH 4/8] target/i386: add AVX10 feature and AVX10 version
> > property
> > X-Mailer: git-send-email 2.47.0
> >
> > From: Tao Su <tao1.su@linux.intel.com>
> >
> > When AVX10 enable bit is set, the 0x24 leaf will be present as "AVX10
> > Converged Vector ISA leaf" containing fields for the version number and
> > the supported vector bit lengths.
> >
> > 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 default can come from the device model or, for the max model,
> > from KVM's reported value.
> >
> > Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> > Link: https://lore.kernel.org/r/20241028024512.156724-3-tao1.su@linux.intel.com
> > Link: https://lore.kernel.org/r/20241028024512.156724-4-tao1.su@linux.intel.com
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > target/i386/cpu.h | 4 +++
> > target/i386/cpu.c | 64 ++++++++++++++++++++++++++++++++++++++-----
> > target/i386/kvm/kvm.c | 3 +-
> > 3 files changed, 63 insertions(+), 8 deletions(-)
>
> [snip]
>
> > @@ -7611,7 +7644,23 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
> > }
> > }
> >
> > - return x86_cpu_have_filtered_features(cpu);
> > + have_filtered_features = x86_cpu_have_filtered_features(cpu);
> > +
> > + if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
> > + x86_cpu_get_supported_cpuid(0x24, 0,
> > + &eax_0, &ebx_0, &ecx_0, &edx_0);
> > + uint8_t version = ebx_0 & 0xff;
> > +
> > + if (version < env->avx10_version) {
> > + if (prefix) {
> > + warn_report("%s: avx10.%d", prefix, env->avx10_version);
>
> Perhaps also tell user about revised version?
>
> warn_report("%s: avx10.%d. Adjust to avx10.%d",
> prefix, env->avx10_version, version);
>
I see, thanks!
> > + }
> > + env->avx10_version = version;
> > + have_filtered_features = true;
> > + }
> > + }
>
>
> Per Tao's comment, perhaps we can check AVX10 and version here (default
> version is 0):
>
> @@ -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().
> + } else (version < env->avx10_version) {
> if (prefix) {
> - warn_report("%s: avx10.%d", prefix, env->avx10_version);
> + warn_report("%s: avx10.%d. Adjust to avx10.%d",
> + prefix, env->avx10_version, version);
> }
> env->avx10_version = version;
> have_filtered_features = true;
> }
> + } else if (env->avx10_version && prefix) {
> + if (prefix) {
I think it is reasonable, especially when we don't check AVX10 enable bit
in max_x86_cpu_realize(). But checking prefix here again seems not
necessary.
> + warn_report("%s: avx10.%d.", prefix, env->avx10_version);
> + }
> + have_filtered_features = true;
> }
>
> return have_filtered_features;
>
> > + return have_filtered_features;
> > }
> >
> > static void x86_cpu_hyperv_realize(X86CPU *cpu)
> > @@ -8395,6 +8444,7 @@ static Property x86_cpu_properties[] = {
> > DEFINE_PROP_UINT32("min-level", X86CPU, env.cpuid_min_level, 0),
> > DEFINE_PROP_UINT32("min-xlevel", X86CPU, env.cpuid_min_xlevel, 0),
> > DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
> > + DEFINE_PROP_UINT8("avx10-version", X86CPU, env.avx10_version, 0),
>
> As my first comment, we could consider changing the default value to -1.
>
I still think 0 is better…
> > > 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.
> > > The default can come from the device model or, for the max model,
> > > from KVM's reported value.
> > >
> > > Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> > > Link: https://lore.kernel.org/r/20241028024512.156724-3-tao1.su@linux.intel.com
> > > Link: https://lore.kernel.org/r/20241028024512.156724-4-tao1.su@linux.intel.com
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > > target/i386/cpu.h | 4 +++
> > > target/i386/cpu.c | 64 ++++++++++++++++++++++++++++++++++++++-----
> > > target/i386/kvm/kvm.c | 3 +-
> > > 3 files changed, 63 insertions(+), 8 deletions(-)
> >
> > [snip]
> >
> > > @@ -7611,7 +7644,23 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
> > > }
> > > }
> > >
> > > - return x86_cpu_have_filtered_features(cpu);
> > > + have_filtered_features = x86_cpu_have_filtered_features(cpu);
> > > +
> > > + if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
> > > + x86_cpu_get_supported_cpuid(0x24, 0,
> > > + &eax_0, &ebx_0, &ecx_0, &edx_0);
> > > + uint8_t version = ebx_0 & 0xff;
> > > +
> > > + if (version < env->avx10_version) {
> > > + if (prefix) {
> > > + warn_report("%s: avx10.%d", prefix, env->avx10_version);
> >
> > Perhaps also tell user about revised version?
> >
> > warn_report("%s: avx10.%d. Adjust to avx10.%d",
> > prefix, env->avx10_version, version);
> >
>
> I see, thanks!
Welcome!
> > > + }
> > > + env->avx10_version = version;
> > > + have_filtered_features = true;
> > > + }
> > > + }
> >
> >
> > Per Tao's comment, perhaps we can check AVX10 and version here (default
> > version is 0):
> >
> > @@ -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.
> > + } else (version < env->avx10_version) {
> > if (prefix) {
> > - warn_report("%s: avx10.%d", prefix, env->avx10_version);
> > + warn_report("%s: avx10.%d. Adjust to avx10.%d",
> > + prefix, env->avx10_version, version);
> > }
> > env->avx10_version = version;
> > have_filtered_features = true;
> > }
> > + } else if (env->avx10_version && prefix) {
Oops, here we only need to check env->avx10_version...
> > + if (prefix) {
>
> I think it is reasonable, especially when we don't check AVX10 enable bit
> in max_x86_cpu_realize(). But checking prefix here again seems not
> necessary.
Thanks! We only need the second check since have_filtered_features
should change whether prefix exists or not.
> > + warn_report("%s: avx10.%d.", prefix, env->avx10_version);
> > + }
> > + have_filtered_features = true;
> > }
> >
> > return have_filtered_features;
> >
> > > + return have_filtered_features;
> > > }
> > >
> > > static void x86_cpu_hyperv_realize(X86CPU *cpu)
> > > @@ -8395,6 +8444,7 @@ static Property x86_cpu_properties[] = {
> > > DEFINE_PROP_UINT32("min-level", X86CPU, env.cpuid_min_level, 0),
> > > DEFINE_PROP_UINT32("min-xlevel", X86CPU, env.cpuid_min_xlevel, 0),
> > > DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
> > > + DEFINE_PROP_UINT8("avx10-version", X86CPU, env.avx10_version, 0),
> >
> > As my first comment, we could consider changing the default value to -1.
> >
>
> I still think 0 is better…
>
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…
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.
[ ... ]
> > > @@ -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.
[ ... ]
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
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.
> > > > > > @@ -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.
OK. In this case, extend avx10-version is also consistent with the
semantics of this function. Even if host doesn't support avx10, then in
principle it's ok to read unimplemented avx10-version as 0.
Pls go ahead. :)
Thanks,
Zhao
On Thu, Oct 31, 2024 at 03:18:37PM +0800, Zhao Liu wrote:
> > > > > > > @@ -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.
>
> OK. In this case, extend avx10-version is also consistent with the
> semantics of this function. Even if host doesn't support avx10, then in
> principle it's ok to read unimplemented avx10-version as 0.
>
> Pls go ahead. :)
I will submit v3 based on all your comments, thanks for review :)
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;
}
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.li@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.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;
> }
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
On Thu, Oct 31, 2024 at 01:52:24PM +0800, Xiaoyao Li wrote:
[ ... ]
> > 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;
> }
Good suggestion, thanks for the reminder!
On Tue, Oct 29, 2024 at 04:18:54PM +0100, Paolo Bonzini wrote:
> From: Tao Su <tao1.su@linux.intel.com>
[ ... ]
> static void max_x86_cpu_realize(DeviceState *dev, Error **errp)
> {
> Object *obj = OBJECT(dev);
> + X86CPU *cpu = X86_CPU(dev);
> + CPUX86State *env = &cpu->env;
>
> if (!object_property_get_int(obj, "family", &error_abort)) {
> if (X86_CPU(obj)->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
> @@ -5351,6 +5357,14 @@ static void max_x86_cpu_realize(DeviceState *dev, Error **errp)
> }
> }
>
> + if ((env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) && !env->avx10_version) {
CPUID_7_1_EDX_AVX10 is not set now and will be set in x86_cpu_realizefn().
How about just checking !env->avx10_version? Because cpu_x86_cpuid will
also check (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10).
> + uint32_t eax, ebx, ecx, edx;
> + x86_cpu_get_supported_cpuid(0x24, 0,
> + &eax, &ebx, &ecx, &edx);
> +
> + env->avx10_version = ebx & 0xff;
> + }
> +
> x86_cpu_realizefn(dev, errp);
> }
>
> @@ -6331,6 +6345,9 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
> */
> object_property_set_str(OBJECT(cpu), "vendor", def->vendor, &error_abort);
>
> + object_property_set_int(OBJECT(cpu), "avx10-version", def->avx10_version,
> + &error_abort);
NIT: avx10-version is defined as UINT8, is it better to use object_property_set_uint?
> +
> x86_cpu_apply_version_props(cpu, model);
>
> /*
> @@ -6859,6 +6876,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> }
> break;
> }
> + case 0x24: {
> + *eax = 0;
> + *ebx = 0;
> + *ecx = 0;
> + *edx = 0;
> + if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
> + *ebx = env->avx10_version;
> + }
The CPUIDs of vector lengths are missing here, according to your previous
comment, it should be:
+ if ((env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) && count == 0) {
+ *ebx = env->features[FEAT_24_0_EBX] | env->avx10_version;
+ }
[ ... ]
On Wed, Oct 30, 2024 at 11:05:23AM +0800, Tao Su wrote:
> Date: Wed, 30 Oct 2024 11:05:23 +0800
> From: Tao Su <tao1.su@linux.intel.com>
> Subject: Re: [PATCH 4/8] target/i386: add AVX10 feature and AVX10 version
> property
>
> On Tue, Oct 29, 2024 at 04:18:54PM +0100, Paolo Bonzini wrote:
> > From: Tao Su <tao1.su@linux.intel.com>
>
> [ ... ]
>
> > static void max_x86_cpu_realize(DeviceState *dev, Error **errp)
> > {
> > Object *obj = OBJECT(dev);
> > + X86CPU *cpu = X86_CPU(dev);
> > + CPUX86State *env = &cpu->env;
> >
> > if (!object_property_get_int(obj, "family", &error_abort)) {
> > if (X86_CPU(obj)->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
> > @@ -5351,6 +5357,14 @@ static void max_x86_cpu_realize(DeviceState *dev, Error **errp)
> > }
> > }
> >
> > + if ((env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) && !env->avx10_version) {
>
> CPUID_7_1_EDX_AVX10 is not set now and will be set in x86_cpu_realizefn().
> How about just checking !env->avx10_version? Because cpu_x86_cpuid will
> also check (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10).
If you explicitly turn on avx10 via -cpu max,+avx10, then CPUID_7_1_EDX_AVX10
will be there. But I agree, this is not a good place to check avx10 and
avx10_version.
© 2016 - 2024 Red Hat, Inc.