Now it changes to use env->topo_info.threads_per_core and doesn't depend
on qemu_init_vcpu() anymore. Drop the comment of the order dependency on
qemu_init_vcpu() and hoist code to put it together with other feature
checking.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/cpu.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d41768648ab9..fd59da5d445d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7880,6 +7880,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
      */
     cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
 
+    /*
+     * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
+     * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
+     * based on inputs (sockets,cores,threads), it is still better to give
+     * users a warning.
+     */
+    if (IS_AMD_CPU(env) &&
+        !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
+        env->nr_threads > 1) {
+            warn_report_once("This family of AMD CPU doesn't support "
+                             "hyperthreading(%d). Please configure -smp "
+                             "options properly or try enabling topoext "
+                             "feature.", env->nr_threads);
+    }
+
     /* For 64bit systems think about the number of physical bits to present.
      * ideally this should be the same as the host; anything other than matching
      * the host can cause incorrect guest behaviour.
@@ -7984,21 +7999,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     x86_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 
-    /*
-     * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
-     * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
-     * based on inputs (sockets,cores,threads), it is still better to give
-     * users a warning.
-     */
-    if (IS_AMD_CPU(env) &&
-        !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
-        env->topo_info.threads_per_core > 1) {
-            warn_report_once("This family of AMD CPU doesn't support "
-                             "hyperthreading(%d). Please configure -smp "
-                             "options properly or try enabling topoext "
-                             "feature.", env->topo_info.threads_per_core);
-    }
-
 #ifndef CONFIG_USER_ONLY
     x86_cpu_apic_realize(cpu, &local_err);
     if (local_err != NULL) {
-- 
2.34.1

On Thu, Dec 19, 2024 at 06:01:22AM -0500, Xiaoyao Li wrote:
> Date: Thu, 19 Dec 2024 06:01:22 -0500
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH v2 07/10] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT
>  against threads_per_core
> X-Mailer: git-send-email 2.34.1
> 
> Now it changes to use env->topo_info.threads_per_core and doesn't depend
> on qemu_init_vcpu() anymore. Drop the comment of the order dependency on
> qemu_init_vcpu() and hoist code to put it together with other feature
> checking.

The comment was dropped in patch 6. You could move such description to
there.

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/cpu.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index d41768648ab9..fd59da5d445d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7880,6 +7880,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>       */
>      cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
>  
> +    /*
> +     * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
> +     * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
> +     * based on inputs (sockets,cores,threads), it is still better to give
> +     * users a warning.
> +     */
> +    if (IS_AMD_CPU(env) &&
> +        !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
> +        env->nr_threads > 1) {

Typo, env->topo_info.threads_per_core.

> +            warn_report_once("This family of AMD CPU doesn't support "
> +                             "hyperthreading(%d). Please configure -smp "
> +                             "options properly or try enabling topoext "
> +                             "feature.", env->nr_threads);
> +    }
> +
>      /* For 64bit systems think about the number of physical bits to present.
>       * ideally this should be the same as the host; anything other than matching
>       * the host can cause incorrect guest behaviour.

On 12/27/2024 4:07 PM, Zhao Liu wrote:
> On Thu, Dec 19, 2024 at 06:01:22AM -0500, Xiaoyao Li wrote:
>> Date: Thu, 19 Dec 2024 06:01:22 -0500
>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>> Subject: [PATCH v2 07/10] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT
>>   against threads_per_core
>> X-Mailer: git-send-email 2.34.1
>>
>> Now it changes to use env->topo_info.threads_per_core and doesn't depend
>> on qemu_init_vcpu() anymore. Drop the comment of the order dependency on
>> qemu_init_vcpu() and hoist code to put it together with other feature
>> checking.
> 
> The comment was dropped in patch 6. You could move such description to
> there.

Obviously, I didn't take care of the patch split and reorder.

I tent to move the dropping of the comment from patch 6 to this one.

>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   target/i386/cpu.c | 30 +++++++++++++++---------------
>>   1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index d41768648ab9..fd59da5d445d 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -7880,6 +7880,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>        */
>>       cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
>>   
>> +    /*
>> +     * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
>> +     * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
>> +     * based on inputs (sockets,cores,threads), it is still better to give
>> +     * users a warning.
>> +     */
>> +    if (IS_AMD_CPU(env) &&
>> +        !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
>> +        env->nr_threads > 1) {
> 
> Typo, env->topo_info.threads_per_core.

Thanks for catching it.

>> +            warn_report_once("This family of AMD CPU doesn't support "
>> +                             "hyperthreading(%d). Please configure -smp "
>> +                             "options properly or try enabling topoext "
>> +                             "feature.", env->nr_threads);
>> +    }
>> +
>>       /* For 64bit systems think about the number of physical bits to present.
>>        * ideally this should be the same as the host; anything other than matching
>>        * the host can cause incorrect guest behaviour.