Re: [PATCH v2 07/10] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT against threads_per_core

[Zhao Liu]

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.