Re: [PATCH v1 2/3] target/i386: Add a new CPU feature word for CPUID.7.1.ECX
On 5/29/2025 12:13 AM, Paolo Bonzini wrote:
On 5/26/25 05:47, Xiaoyao Li wrote:
On 1/3/2025 4:48 PM, Xin Li (Intel) wrote:
@@ -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.
Paolo, thanks a lot!
Xin
Re: [PATCH v1 2/3] target/i386: Add a new CPU feature word for CPUID.7.1.ECX
On 5/29/2025 3:13 PM, Paolo Bonzini wrote:
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)
---
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.
Yes. It makes sense for this reason, especially that this leaf is
general feature enumeration leaf and destined to be filled up in the future.
Thanks for the review though! It's always appreciated even if we disagree.
My pleasure.
Re: [PATCH v1 2/3] target/i386: Add a new CPU feature word for CPUID.7.1.ECX
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)
---
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;
Re: [PATCH v1 2/3] target/i386: Add a new CPU feature word for CPUID.7.1.ECX
On 5/25/2025 8:47 PM, Xiaoyao Li wrote:
On 1/3/2025 4:48 PM, Xin Li (Intel) wrote:
@@ -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.
We can just merge the next patch into this one and make it,
.feat_names = {
[5] = "msr-imm",
},
I appreciate this format that clearly indicates which bit corresponds to
which feature, and I'm inclined to proceed with the change.
On the flip side, I hope the new format won't be perceived as disrupting
the consistency of the existing codebase. If this form could get called
out by maintainers, there needs a cleanup patch to change all the
instances.
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.
Makes sense to me. Will move FEAT_7_1_ECX, FEAT_7_1_EDX and
FEAT_7_2_EDX immediate after FEAT_7_1_EAX.
Thanks!
Xin
Re: [PATCH v1 2/3] target/i386: Add a new CPU feature word for CPUID.7.1.ECX
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)
---
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.
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;
