On 5/26/25 05:47, Xiaoyao Li wrote:
On 1/3/2025 4:48 PM, Xin Li (Intel) wrote:The immediate form of MSR access instructions will use this new CPU feature word.Signed-off-by: Xin Li (Intel) <x...@zytor.com> --- target/i386/cpu.c | 23 ++++++++++++++++++++++- target/i386/cpu.h | 1 + 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 8a1223acb3..2fb05879c3 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c@@ -894,6 +894,7 @@ void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, #define TCG_7_1_EAX_FEATURES (CPUID_7_1_EAX_FZRM | CPUID_7_1_EAX_FSRS | \CPUID_7_1_EAX_FSRC | CPUID_7_1_EAX_CMPCCXADD) +#define TCG_7_1_ECX_FEATURES 0 #define TCG_7_1_EDX_FEATURES 0 #define TCG_7_2_EDX_FEATURES 0 #define TCG_APM_FEATURES 0@@ -1133,6 +1134,25 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {}, .tcg_features = TCG_7_1_EAX_FEATURES, }, + [FEAT_7_1_ECX] = { + .type = CPUID_FEATURE_WORD, + .feat_names = { + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL,This looks silly, and the size of feat_names[] was changed from 32 to 64. Just explicitly assign the first 32 entries with NULL doesn't make any sense after the size change.
64 is just for MSR features. This is a bit silly, I agree, but it is consistent with existing feature words and ultimately it becomes more compact after just 9 features. So I'm queuing Xin's patches as they are.
Thanks for the review though! It's always appreciated even if we disagree. Paolo
We can just merge the next patch into this one and make it, .feat_names = { [5] = "msr-imm", },+ }, + .cpuid = { + .eax = 7, + .needs_ecx = true, .ecx = 1, + .reg = R_ECX, + }, + .tcg_features = TCG_7_1_ECX_FEATURES, + }, [FEAT_7_1_EDX] = { .type = CPUID_FEATURE_WORD, .feat_names = {@@ -6659,9 +6679,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,*edx = env->features[FEAT_7_0_EDX]; /* Feature flags */ } else if (count == 1) { *eax = env->features[FEAT_7_1_EAX]; + *ecx = env->features[FEAT_7_1_ECX]; *edx = env->features[FEAT_7_1_EDX]; *ebx = 0; - *ecx = 0; } else if (count == 2) { *edx = env->features[FEAT_7_2_EDX]; *eax = 0;@@ -7563,6 +7583,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)x86_cpu_adjust_feat_level(cpu, FEAT_6_EAX); x86_cpu_adjust_feat_level(cpu, FEAT_7_0_ECX); x86_cpu_adjust_feat_level(cpu, FEAT_7_1_EAX); + x86_cpu_adjust_feat_level(cpu, FEAT_7_1_ECX); x86_cpu_adjust_feat_level(cpu, FEAT_7_1_EDX); x86_cpu_adjust_feat_level(cpu, FEAT_7_2_EDX); x86_cpu_adjust_feat_level(cpu, FEAT_8000_0001_EDX); diff --git a/target/i386/cpu.h b/target/i386/cpu.h index dbd8f1ffc7..d23fa96549 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -667,6 +667,7 @@ typedef enum FeatureWord { 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 */ + FEAT_7_1_ECX, /* CPUID[EAX=7,ECX=1].ECX */I would prefer the newly added leaf being ordered at least in the same leaf. i.e.,@@ -661,6 +661,7 @@ typedef enum FeatureWord {FEAT_SGX_12_1_EAX, /* CPUID[EAX=0x12,ECX=1].EAX (SGX ATTRIBUTES[31:0]) */FEAT_XSAVE_XSS_LO, /* CPUID[EAX=0xd,ECX=1].ECX */ FEAT_XSAVE_XSS_HI, /* CPUID[EAX=0xd,ECX=1].EDX */ + FEAT_7_1_ECX, /* CPUID[EAX=7,ECX=1].ECX */ 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 */They are QEMU internally data, injecting a new one instead of putting it at the end is OK to me.FEATURE_WORDS, } FeatureWord;
