| Message ID | 20241028024512.156724-4-tao1.su@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add AVX10.1 CPUID support and GraniteRapids-v2 model | expand |
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() ?
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. > > + 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? :-)
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;