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.
Tested-by: Xuelian Guo <xuelian.guo@intel.com>
Signed-off-by: Tao Su <tao1.su@linux.intel.com>
---
target/i386/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
target/i386/cpu.h | 8 ++++++++
target/i386/kvm/kvm.c | 3 ++-
3 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5b434a107a..91fae0dcb7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -898,6 +898,7 @@ void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
#define TCG_SGX_12_0_EAX_FEATURES 0
#define TCG_SGX_12_0_EBX_FEATURES 0
#define TCG_SGX_12_1_EAX_FEATURES 0
+#define TCG_24_0_EBX_FEATURES 0
#if defined CONFIG_USER_ONLY
#define CPUID_8000_0008_EBX_KERNEL_FEATURES (CPUID_8000_0008_EBX_IBPB | \
@@ -1163,6 +1164,20 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
},
.tcg_features = TCG_7_2_EDX_FEATURES,
},
+ [FEAT_24_0_EBX] = {
+ .type = CPUID_FEATURE_WORD,
+ .feat_names = {
+ [16] = "avx10-128",
+ [17] = "avx10-256",
+ [18] = "avx10-512",
+ },
+ .cpuid = {
+ .eax = 0x24,
+ .needs_ecx = true, .ecx = 0,
+ .reg = R_EBX,
+ },
+ .tcg_features = TCG_24_0_EBX_FEATURES,
+ },
[FEAT_8000_0007_EDX] = {
.type = CPUID_FEATURE_WORD,
.feat_names = {
@@ -6835,6 +6850,26 @@ 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)) {
+ break;
+ }
+
+ if (count == 0) {
+ uint8_t v = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x24,
+ 0, R_EBX);
+ if (env->avx10_version && env->avx10_version < v) {
+ v = env->avx10_version;
+ }
+
+ *ebx = env->features[FEAT_24_0_EBX] | v;
+ }
+ break;
+ }
case 0x40000000:
/*
* CPUID code in kvm_arch_init_vcpu() ignores stuff
@@ -7483,6 +7518,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);
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index d845384dcd..5566a13f4f 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -662,6 +662,7 @@ typedef enum FeatureWord {
FEAT_XSAVE_XSS_HI, /* CPUID[EAX=0xd,ECX=1].EDX */
FEAT_7_1_EDX, /* CPUID[EAX=7,ECX=1].EDX */
FEAT_7_2_EDX, /* CPUID[EAX=7,ECX=2].EDX */
+ FEAT_24_0_EBX, /* CPUID[EAX=0x24,ECX=0].EBX */
FEATURE_WORDS,
} FeatureWord;
@@ -990,6 +991,13 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
/* Packets which contain IP payload have LIP values */
#define CPUID_14_0_ECX_LIP (1U << 31)
+/* AVX10 128-bit vector support is present */
+#define CPUID_24_0_EBX_AVX10_128 (1U << 16)
+/* AVX10 256-bit vector support is present */
+#define CPUID_24_0_EBX_AVX10_256 (1U << 17)
+/* AVX10 512-bit vector support is present */
+#define CPUID_24_0_EBX_AVX10_512 (1U << 18)
+
/* RAS Features */
#define CPUID_8000_0007_EBX_OVERFLOW_RECOV (1U << 0)
#define CPUID_8000_0007_EBX_SUCCOR (1U << 1)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index fd9f198892..8e17942c3b 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.34.1On 10/28/24 03:45, Tao Su wrote:
> @@ -6835,6 +6850,26 @@ 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)) {
> + break;
> + }
> +
> + if (count == 0) {
> + uint8_t v = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x24,
> + 0, R_EBX);
> + if (env->avx10_version && env->avx10_version < v) {
> + v = env->avx10_version;
> + }
> +
> + *ebx = env->features[FEAT_24_0_EBX] | v;
> + }
> + break;
> + }
> case 0x40000000:
> /*
> * CPUID code in kvm_arch_init_vcpu() ignores stuff
This check should be done elsewhere (called by x86_cpu_realizefn());
cpu_x86_cpuid() should only report the value:
if ((env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) && count == 0) {
*ebx = env->features[FEAT_24_0_EBX] | env->avx10_version;
}
Also, the check should use x86_cpu_get_supported_cpuid() because KVM is not the
only accelerator.
>
> + if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
> + uint8_t version =
> + kvm_arch_get_supported_cpuid(cs->kvm_state, 0x24, 0, R_EBX);
> +
> + if (!env->avx10_version) {
> + env->avx10_version = version;
> + }
> +
This should not be done here, but in max_x86_cpu_realize(). It should also
use x86_cpu_get_supported_cpuid().
For Granite Rapids you're only setting the AVX10 version in v2 and therefore
you don't need it, but there should also be (for the future) an avx10_version
field in X86CPUDefinition, which is set into the avx10-version property at
x86_cpu_load_model().
> index d845384dcd..5566a13f4f 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -662,6 +662,7 @@ typedef enum FeatureWord {
> FEAT_XSAVE_XSS_HI, /* CPUID[EAX=0xd,ECX=1].EDX */
> FEAT_7_1_EDX, /* CPUID[EAX=7,ECX=1].EDX */
> FEAT_7_2_EDX, /* CPUID[EAX=7,ECX=2].EDX */
> + FEAT_24_0_EBX, /* CPUID[EAX=0x24,ECX=0].EBX */
Adding FEAT_24_0_EBX should be a separate patch.
Paolo
> FEATURE_WORDS,
> } FeatureWord;
>
> @@ -990,6 +991,13 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
> /* Packets which contain IP payload have LIP values */
> #define CPUID_14_0_ECX_LIP (1U << 31)
>
> +/* AVX10 128-bit vector support is present */
> +#define CPUID_24_0_EBX_AVX10_128 (1U << 16)
> +/* AVX10 256-bit vector support is present */
> +#define CPUID_24_0_EBX_AVX10_256 (1U << 17)
> +/* AVX10 512-bit vector support is present */
> +#define CPUID_24_0_EBX_AVX10_512 (1U << 18)
> +
> /* RAS Features */
> #define CPUID_8000_0007_EBX_OVERFLOW_RECOV (1U << 0)
> #define CPUID_8000_0007_EBX_SUCCOR (1U << 1)
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index fd9f198892..8e17942c3b 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;
On Tue, Oct 29, 2024 at 09:25:53AM +0100, Paolo Bonzini wrote:
> On 10/28/24 03:45, Tao Su wrote:
> > @@ -6835,6 +6850,26 @@ 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)) {
> > + break;
> > + }
> > +
> > + if (count == 0) {
> > + uint8_t v = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x24,
> > + 0, R_EBX);
> > + if (env->avx10_version && env->avx10_version < v) {
> > + v = env->avx10_version;
> > + }
> > +
> > + *ebx = env->features[FEAT_24_0_EBX] | v;
> > + }
> > + break;
> > + }
> > case 0x40000000:
> > /*
> > * CPUID code in kvm_arch_init_vcpu() ignores stuff
>
> This check should be done elsewhere (called by x86_cpu_realizefn());
> cpu_x86_cpuid() should only report the value:
>
> if ((env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) && count == 0) {
> *ebx = env->features[FEAT_24_0_EBX] | env->avx10_version;
> }
>
> Also, the check should use x86_cpu_get_supported_cpuid() because KVM is not the
> only accelerator.
>
Yes, I see, I add check here:
@@ -7679,6 +7719,27 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
x86_cpu_filter_features(cpu, cpu->check_cpuid || cpu->enforce_cpuid);
+ if (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);
+
+ ebx &= 0xff;
+
+ if (ebx < env->avx10_version) {
+ const char *msg = accel_uses_host_cpuid()
+ ? "Host doesn't support requested feature"
+ : "TCG doesn't support requested feature";
+ if (cpu->enforce_cpuid) {
+ error_setg(&local_err, "%s: avx10.%d", msg,
+ env->avx10_version);
+ goto out;
+ } else if (cpu->check_cpuid) {
+ warn_report("%s: avx10.%d", msg, env->avx10_version);
+ }
+ env->avx10_version = ebx;
+ }
+ }
+
if (cpu->enforce_cpuid && x86_cpu_have_filtered_features(cpu)) {
error_setg(&local_err,
accel_uses_host_cpuid() ?
>
> >
> > + if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
> > + uint8_t version =
> > + kvm_arch_get_supported_cpuid(cs->kvm_state, 0x24, 0, R_EBX);
> > +
> > + if (!env->avx10_version) {
> > + env->avx10_version = version;
> > + }
> > +
>
> This should not be done here, but in max_x86_cpu_realize(). It should also
> use x86_cpu_get_supported_cpuid().
>
Yes, I try to add here:
@@ -5327,6 +5365,12 @@ static void max_x86_cpu_realize(DeviceState *dev, Error **errp)
}
}
+ if (!object_property_get_uint(obj, "avx10-version", &error_abort)) {
+ uint32_t eax, ebx, ecx, edx;
+ x86_cpu_get_supported_cpuid(0x24, 0, &eax, &ebx, &ecx, &edx);
+ object_property_set_uint(obj, "avx10-version", ebx & 0xff, &error_abort);
+ }
+
x86_cpu_realizefn(dev, errp);
}
> For Granite Rapids you're only setting the AVX10 version in v2 and therefore
> you don't need it, but there should also be (for the future) an avx10_version
> field in X86CPUDefinition, which is set into the avx10-version property at
> x86_cpu_load_model().
>
Yes, for new CPU model, we should do that.
> > index d845384dcd..5566a13f4f 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -662,6 +662,7 @@ typedef enum FeatureWord {
> > FEAT_XSAVE_XSS_HI, /* CPUID[EAX=0xd,ECX=1].EDX */
> > FEAT_7_1_EDX, /* CPUID[EAX=7,ECX=1].EDX */
> > FEAT_7_2_EDX, /* CPUID[EAX=7,ECX=2].EDX */
> > + FEAT_24_0_EBX, /* CPUID[EAX=0x24,ECX=0].EBX */
>
> Adding FEAT_24_0_EBX should be a separate patch.
>
Yes, all you said above are excellent suggestions and I have tested on
my platform. Should I submit a v2 with these changes or wait for you to
send v2 directly? :-)
On 10/28/2024 10:45 AM, Tao Su wrote:
> 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.
>
> Tested-by: Xuelian Guo <xuelian.guo@intel.com>
> Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> ---
> target/i386/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
> target/i386/cpu.h | 8 ++++++++
> target/i386/kvm/kvm.c | 3 ++-
> 3 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5b434a107a..91fae0dcb7 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -898,6 +898,7 @@ void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> #define TCG_SGX_12_0_EAX_FEATURES 0
> #define TCG_SGX_12_0_EBX_FEATURES 0
> #define TCG_SGX_12_1_EAX_FEATURES 0
> +#define TCG_24_0_EBX_FEATURES 0
>
> #if defined CONFIG_USER_ONLY
> #define CPUID_8000_0008_EBX_KERNEL_FEATURES (CPUID_8000_0008_EBX_IBPB | \
> @@ -1163,6 +1164,20 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> },
> .tcg_features = TCG_7_2_EDX_FEATURES,
> },
> + [FEAT_24_0_EBX] = {
> + .type = CPUID_FEATURE_WORD,
> + .feat_names = {
> + [16] = "avx10-128",
> + [17] = "avx10-256",
> + [18] = "avx10-512",
> + },
> + .cpuid = {
> + .eax = 0x24,
> + .needs_ecx = true, .ecx = 0,
> + .reg = R_EBX,
> + },
> + .tcg_features = TCG_24_0_EBX_FEATURES,
> + },
> [FEAT_8000_0007_EDX] = {
> .type = CPUID_FEATURE_WORD,
> .feat_names = {
> @@ -6835,6 +6850,26 @@ 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)) {
> + break;
> + }
> +
> + if (count == 0) {
> + uint8_t v = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x24,
> + 0, R_EBX);
> + if (env->avx10_version && env->avx10_version < v) {
> + v = env->avx10_version;
> + }
Here, if user specified avx10_version is >= kvm reported value, it uses
KVM's reported value silently.
I think it's not good. It'd better to validate if user specified value
can be satisfied or not, and emit a warning when not. e.g., in
x86_cpu_filter_features() or in kvm_cpu_realizefn(). Also we can put the
behavior along with it that "use KVM reported maximum value when
avx10_version is 0 "
then, here we can simply do
*ebx = env->features[FEAT_24_0_EBX] | env->avx10_version;
> + *ebx = env->features[FEAT_24_0_EBX] | v;
> + }
> + break;
> + }
> case 0x40000000:
> /*
> * CPUID code in kvm_arch_init_vcpu() ignores stuff
> @@ -7483,6 +7518,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);
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index d845384dcd..5566a13f4f 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -662,6 +662,7 @@ typedef enum FeatureWord {
> FEAT_XSAVE_XSS_HI, /* CPUID[EAX=0xd,ECX=1].EDX */
> FEAT_7_1_EDX, /* CPUID[EAX=7,ECX=1].EDX */
> FEAT_7_2_EDX, /* CPUID[EAX=7,ECX=2].EDX */
> + FEAT_24_0_EBX, /* CPUID[EAX=0x24,ECX=0].EBX */
> FEATURE_WORDS,
> } FeatureWord;
>
> @@ -990,6 +991,13 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
> /* Packets which contain IP payload have LIP values */
> #define CPUID_14_0_ECX_LIP (1U << 31)
>
> +/* AVX10 128-bit vector support is present */
> +#define CPUID_24_0_EBX_AVX10_128 (1U << 16)
> +/* AVX10 256-bit vector support is present */
> +#define CPUID_24_0_EBX_AVX10_256 (1U << 17)
> +/* AVX10 512-bit vector support is present */
> +#define CPUID_24_0_EBX_AVX10_512 (1U << 18)
> +
> /* RAS Features */
> #define CPUID_8000_0007_EBX_OVERFLOW_RECOV (1U << 0)
> #define CPUID_8000_0007_EBX_SUCCOR (1U << 1)
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index fd9f198892..8e17942c3b 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;
On Mon, Oct 28, 2024 at 11:04:07PM +0800, Xiaoyao Li wrote:
> On 10/28/2024 10:45 AM, Tao Su wrote:
> > + case 0x24: {
> > + *eax = 0;
> > + *ebx = 0;
> > + *ecx = 0;
> > + *edx = 0;
> > + if (!(env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10)) {
> > + break;
> > + }
> > +
> > + if (count == 0) {
> > + uint8_t v = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x24,
> > + 0, R_EBX);
> > + if (env->avx10_version && env->avx10_version < v) {
> > + v = env->avx10_version;
> > + }
>
> Here, if user specified avx10_version is >= kvm reported value, it uses
> KVM's reported value silently.
>
> I think it's not good. It'd better to validate if user specified value can
> be satisfied or not, and emit a warning when not. e.g., in
> x86_cpu_filter_features() or in kvm_cpu_realizefn(). Also we can put the
> behavior along with it that "use KVM reported maximum value when
> avx10_version is 0 "
>
> then, here we can simply do
>
> *ebx = env->features[FEAT_24_0_EBX] | env->avx10_version;
Is it necessary to add enforce_cpuid for avx10_version too?
How about checking this in x86_cpu_realizefn, because I see this may be a
more suitable place to implement check_cpuid and enforce_cpuid.
@@ -7816,6 +7810,29 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
x86_cpu_filter_features(cpu, cpu->check_cpuid || cpu->enforce_cpuid);
+ if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
+ uint8_t version =
+ kvm_arch_get_supported_cpuid(cs->kvm_state, 0x24, 0, R_EBX);
+
+ if (!env->avx10_version) {
+ env->avx10_version = version;
+ }
+
+ if (version < env->avx10_version) {
+ const char *msg = accel_uses_host_cpuid()
+ ? "Host doesn't support requested feature"
+ : "TCG doesn't support requested feature";
+ if (cpu->enforce_cpuid) {
+ error_setg(&local_err, "%s: avx10.%d", msg,
+ env->avx10_version);
+ goto out;
+ } else if (cpu->check_cpuid) {
+ warn_report("%s: avx10.%d", msg, env->avx10_version);
+ }
+ env->avx10_version = version;
+ }
+ }
+
if (cpu->enforce_cpuid && x86_cpu_have_filtered_features(cpu)) {
error_setg(&local_err,
accel_uses_host_cpuid() ?
© 2016 - 2024 Red Hat, Inc.