Re: [RFC PATCH 2/4] i386: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT

[Xiaoyao Li]

On 12/11/2024 12:35 AM, Igor Mammedov wrote:
> On Thu, 5 Dec 2024 22:38:41 +0100
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> > Hi Xiaoyao,
> >
> > On 5/12/24 15:57, Xiaoyao Li wrote:
> > > There are duplicated code to setup the value of MSR_CORE_THREAD_COUNT.
> > > Extract a common function for it.
> > >
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > ---
> > >    target/i386/cpu.h                    | 11 +++++++++++
> > >    target/i386/hvf/x86_emu.c            |  3 +--
> > >    target/i386/kvm/kvm.c                |  5 +----
> > >    target/i386/tcg/sysemu/misc_helper.c |  3 +--
> > >    4 files changed, 14 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index 4c239a6970fd..5795a497e567 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -2390,6 +2390,17 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU 
> > > *cpu,
> > >        cs->halted = 0;
> > >    }
> > >    
> > > +static inline uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
> >
> > Please do not add more inlined functions in this huge file, the
> > inlining performance isn't justified at all.
> >
> > target/i386/cpu-sysemu.c looks the correct place for this helper.
>
> ack

Will move it to target/i386/cpu-sysemu.c in next version.

> > > +{
> > > +    CPUState *cs = CPU(cpu);
> > > +    uint64_t val;
> > > +
> > > +    val = cs->nr_threads * cs->nr_cores;  /* thread count, bits 15..0 */
> > > +    val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
> >
> > Personally I'd change to:
> >
> >          return deposit64(cs->nr_threads * cs->nr_cores, 16, 16,
> >                           cs->nr_cores);
> that I wouldn't do in this patch to make it easier to compare apples to apples
> That however could be a separate patch on top

OK. I will keep it as-is in this patch.

> > > +
> > > +    return val;
> > > +}