[PATCH v1 0/3] target/i386: Add the immediate form MSR access instruction support

[PATCH v1 0/3] target/i386: Add the immediate form MSR access instruction support

[Xin Li (Intel)]

The immediate form of MSR access instructions are primarily motivated by
performance, not code size: by having the MSR number in an immediate, it
is available *much* earlier in the pipeline, which allows the hardware
much more leeway about how a particular MSR is handled.

This new CPU feature is advertised through bit 5 of CPUID.7.1.ECX, which
needs to be added as a new CPU feature word.

WRMSRNS doesn't become a required feature for FERD, and Linux has removed
the dependency, as such remove the dependency from Qemu.


Xin Li (Intel) (3):
  target/i386: Remove FRED dependency on WRMSRNS
  target/i386: Add a new CPU feature word for CPUID.7.1.ECX
  target/i386: Add the immediate form MSR access instruction support

 target/i386/cpu.c | 27 ++++++++++++++++++++++-----
 target/i386/cpu.h |  4 ++++
 2 files changed, 26 insertions(+), 5 deletions(-)


base-commit: 1ada452efc7d8f8bf42cd5e8a2af1b4ac9167a1f
-- 
2.47.1

[PATCH v1 1/3] target/i386: Remove FRED dependency on WRMSRNS

[Xin Li (Intel)]

WRMSRNS doesn't become a required feature for FERD, and Linux has
removed the dependency, as such remove it from Qemu.

Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
 target/i386/cpu.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0b639848cd..8a1223acb3 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1771,10 +1771,6 @@ static FeatureDep feature_dependencies[] = {
         .from = { FEAT_7_1_EAX,             CPUID_7_1_EAX_LKGS },
         .to = { FEAT_7_1_EAX,               CPUID_7_1_EAX_FRED },
     },
-    {
-        .from = { FEAT_7_1_EAX,             CPUID_7_1_EAX_WRMSRNS },
-        .to = { FEAT_7_1_EAX,               CPUID_7_1_EAX_FRED },
-    },
     {
         .from = { FEAT_7_0_EBX,             CPUID_7_0_EBX_SGX },
         .to = { FEAT_7_0_ECX,               CPUID_7_0_ECX_SGX_LC },
-- 
2.47.1

[PATCH v1 2/3] target/i386: Add a new CPU feature word for CPUID.7.1.ECX

[Xin Li (Intel)]

The immediate form of MSR access instructions will use this new CPU
feature word.

Signed-off-by: Xin Li (Intel) <xin@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,
+        },
+        .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 */
     FEATURE_WORDS,
 } FeatureWord;
 
-- 
2.47.1

[PATCH v1 3/3] target/i386: Add the immediate form MSR access instruction support

[Xin Li (Intel)]

The immediate form of MSR access instructions are primarily motivated by
performance, not code size: by having the MSR number in an immediate, it
is available *much* earlier in the pipeline, which allows the hardware
much more leeway about how a particular MSR is handled.

Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
 target/i386/cpu.c | 2 +-
 target/i386/cpu.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 2fb05879c3..83fb059d09 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1138,7 +1138,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
         .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL, NULL, NULL, NULL,
-            NULL, NULL, NULL, NULL,
+            NULL, "msr-imm", NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index d23fa96549..df27c359d3 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -987,6 +987,9 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
 /* Non-Serializing Write to Model Specific Register (WRMSRNS) */
 #define CPUID_7_1_EAX_WRMSRNS           (1U << 19)
 
+/* The immediate form of MSR access instructions */
+#define CPUID_7_1_ECX_MSR_IMM           (1U << 5)
+
 /* Do not exhibit MXCSR Configuration Dependent Timing (MCDT) behavior */
 #define CPUID_7_2_EDX_MCDT_NO           (1U << 5)
 
-- 
2.47.1

Re: [PATCH v1 0/3] target/i386: Add the immediate form MSR access instruction support

[Xin Li]

On 1/3/2025 12:48 AM, Xin Li (Intel) wrote:
> The immediate form of MSR access instructions are primarily motivated by
> performance, not code size: by having the MSR number in an immediate, it
> is available *much* earlier in the pipeline, which allows the hardware
> much more leeway about how a particular MSR is handled.
> 
> This new CPU feature is advertised through bit 5 of CPUID.7.1.ECX, which
> needs to be added as a new CPU feature word.

gentle ping!

> 
> WRMSRNS doesn't become a required feature for FERD, and Linux has removed
> the dependency, as such remove the dependency from Qemu.
> 

Maybe this should be sent out as a separate patch?

> 
> Xin Li (Intel) (3):
>    target/i386: Remove FRED dependency on WRMSRNS
>    target/i386: Add a new CPU feature word for CPUID.7.1.ECX
>    target/i386: Add the immediate form MSR access instruction support
> 
>   target/i386/cpu.c | 27 ++++++++++++++++++++++-----
>   target/i386/cpu.h |  4 ++++
>   2 files changed, 26 insertions(+), 5 deletions(-)
> 
> 
> base-commit: 1ada452efc7d8f8bf42cd5e8a2af1b4ac9167a1f

Re: [PATCH v1 0/3] target/i386: Add the immediate form MSR access instruction support

[Xin Li]

On 4/15/2025 8:25 PM, Xin Li wrote:
> On 1/3/2025 12:48 AM, Xin Li (Intel) wrote:
>> The immediate form of MSR access instructions are primarily motivated by
>> performance, not code size: by having the MSR number in an immediate, it
>> is available *much* earlier in the pipeline, which allows the hardware
>> much more leeway about how a particular MSR is handled.
>>
>> This new CPU feature is advertised through bit 5 of CPUID.7.1.ECX, which
>> needs to be added as a new CPU feature word.
> 
> gentle ping!
> 
>>
>> WRMSRNS doesn't become a required feature for FERD, and Linux has removed
>> the dependency, as such remove the dependency from Qemu.
>>
> 
> Maybe this should be sent out as a separate patch?
> 
>>
>> Xin Li (Intel) (3):
>>    target/i386: Remove FRED dependency on WRMSRNS
>>    target/i386: Add a new CPU feature word for CPUID.7.1.ECX
>>    target/i386: Add the immediate form MSR access instruction support
>>
>>   target/i386/cpu.c | 27 ++++++++++++++++++++++-----
>>   target/i386/cpu.h |  4 ++++
>>   2 files changed, 26 insertions(+), 5 deletions(-)
>>
>>
>> base-commit: 1ada452efc7d8f8bf42cd5e8a2af1b4ac9167a1f

Another ping :)

Re: [PATCH v1 1/3] target/i386: Remove FRED dependency on WRMSRNS

[Xiaoyao Li]

On 1/3/2025 4:48 PM, Xin Li (Intel) wrote:
> WRMSRNS doesn't become a required feature for FERD, and Linux has
> removed the dependency, as such remove it from Qemu.
> 
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   target/i386/cpu.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 0b639848cd..8a1223acb3 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1771,10 +1771,6 @@ static FeatureDep feature_dependencies[] = {
>           .from = { FEAT_7_1_EAX,             CPUID_7_1_EAX_LKGS },
>           .to = { FEAT_7_1_EAX,               CPUID_7_1_EAX_FRED },
>       },
> -    {
> -        .from = { FEAT_7_1_EAX,             CPUID_7_1_EAX_WRMSRNS },
> -        .to = { FEAT_7_1_EAX,               CPUID_7_1_EAX_FRED },
> -    },
>       {
>           .from = { FEAT_7_0_EBX,             CPUID_7_0_EBX_SGX },
>           .to = { FEAT_7_0_ECX,               CPUID_7_0_ECX_SGX_LC },

Re: [PATCH v1 2/3] target/i386: Add a new CPU feature word for CPUID.7.1.ECX

[Xiaoyao Li]

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) <xin@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.

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

[Xin Li]

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

[Paolo Bonzini]

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) <xin@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;
> 
> 
> 

Re: [PATCH v1 2/3] target/i386: Add a new CPU feature word for CPUID.7.1.ECX

[Xiaoyao Li]

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) <xin@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.

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

[Xin Li]

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