| Message ID | 20241205145716.472456-3-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cpu: Drop CPUState::nr_cores | expand |
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. > +{ > + 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); > + > + return val; > +}
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 > > > +{ > > + 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 > > + > > + return val; > > +} >
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; >>> +} >> >
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) +{ + 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 */ + + return val; +} + int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector, target_ulong *base, unsigned int *limit, unsigned int *flags); diff --git a/target/i386/hvf/x86_emu.c b/target/i386/hvf/x86_emu.c index 015f760acb39..69c61c9c0737 100644 --- a/target/i386/hvf/x86_emu.c +++ b/target/i386/hvf/x86_emu.c @@ -765,8 +765,7 @@ void simulate_rdmsr(CPUX86State *env) val = env->mtrr_deftype; break; case MSR_CORE_THREAD_COUNT: - val = cs->nr_threads * cs->nr_cores; /* thread count, bits 15..0 */ - val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */ + val = cpu_x86_get_msr_core_thread_count(cpu); break; default: /* fprintf(stderr, "%s: unknown msr 0x%x\n", __func__, msr); */ diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index 8e17942c3ba1..18a1bd1297a4 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -2602,10 +2602,7 @@ static bool kvm_rdmsr_core_thread_count(X86CPU *cpu, uint32_t msr, uint64_t *val) { - CPUState *cs = CPU(cpu); - - *val = cs->nr_threads * cs->nr_cores; /* thread count, bits 15..0 */ - *val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */ + *val = cpu_x86_get_msr_core_thread_count(cpu); return true; } diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c index 094aa56a20d1..ff7b201b44d8 100644 --- a/target/i386/tcg/sysemu/misc_helper.c +++ b/target/i386/tcg/sysemu/misc_helper.c @@ -468,8 +468,7 @@ void helper_rdmsr(CPUX86State *env) val = x86_cpu->ucode_rev; break; case MSR_CORE_THREAD_COUNT: { - CPUState *cs = CPU(x86_cpu); - val = (cs->nr_threads * cs->nr_cores) | (cs->nr_cores << 16); + val = cpu_x86_get_msr_core_thread_count(x86_cpu); break; } case MSR_APIC_START ... MSR_APIC_END: {
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(-)