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

[Xiaoyao Li]

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.